feat: chatbot Retrieval-Augmented-Generation (RAG) phase (v1) - #303
Closed
nicoalegria11 wants to merge 113 commits into
Closed
feat: chatbot Retrieval-Augmented-Generation (RAG) phase (v1) #303nicoalegria11 wants to merge 113 commits into
nicoalegria11 wants to merge 113 commits into
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Lite Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughImplements a citation-bearing Chatbot RAG MVP: adds pgvector migration and images, embedding provider abstraction (mock + Azure), PDF parsing/chunking and ingest/activate CLIs with advisory-lock activation, searchKnowledge retrieval, LLM streaming/tool-call extensions, sendMessage tool orchestration and citation persistence, frontend citation UI, Zod schemas, tests, ESLint/env updates, and OpenSpec docs. ChangesChatbot RAG MVP
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes |
Collaborator
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openspec/changes/chatbot-rag-mvp/specs/chatbot-corpus-embeddings/spec.md (1)
185-198: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winClarify whether mock provider batches computations.
Line 187 states the mock "checks
signal.abortedbetween batched computations," but the mock implementation requirements (lines 22-50) don't mention internal batching. This creates ambiguity:
- Does the mock batch SHA-256 computations for performance?
- Or should "batched" here mean "per-input iteration"?
- If the mock doesn't batch, how frequently should it check
signal.aborted?Consider clarifying the abort-check granularity for the mock provider.
📝 Suggested clarification
-The mock checks `signal.aborted` between batched computations; +The mock checks `signal.aborted` between computing each input's embedding (i.e., after each SHA-256 + normalization step);Or if the mock does batch internally for performance:
-The mock checks `signal.aborted` between batched computations; +The mock processes inputs in internal batches (e.g., 10 at a time) and checks `signal.aborted` between batches;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openspec/changes/chatbot-rag-mvp/specs/chatbot-corpus-embeddings/spec.md` around lines 185 - 198, Clarify the mock provider's abort-check behavior: state explicitly in the mock provider requirement (referencing the "mock" provider and its use of options.signal / signal.aborted) whether it performs internal batching or iterates per-input, and if batching is used specify the batch size or describe "batch boundaries" and that signal.aborted is checked between batches; if no batching, state that the mock checks signal.aborted between each input iteration. Also explicitly keep the azureOpenAI requirement that options.signal is passed through to the underlying OpenAI SDK call so upstream cancellation is honored.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openspec/changes/chatbot-rag-mvp/specs/chatbot-corpus-embeddings/spec.md`:
- Around line 1-21: Add explicit input validation and typed error behavior to
the EmbeddingProvider contract and implementations: update the
EmbeddingProvider/type definitions in
apps/api/src/features/chatbot/embeddingProvider/types.ts to require embed(texts:
string[], options?: { signal?: AbortSignal }) => Promise<EmbeddingResult> to
validate that every element of texts is a non-empty string and reject with a
TypeError for null/undefined/non-string values; define a typed error (e.g.,
EmbeddingProviderError with transient vs permanent codes or names) and ensure
mock.ts and azureOpenAI.ts propagate network/rate-limit/quota failures as that
typed error (including status/code for 429/transient cases); and add a
post-embedding validation step in mock.ts and azureOpenAI.ts to assert each
returned vector has length === 1024 and throw/return a validation error if not,
while preserving the existing empty-array behavior (vectors: [], inputTokens: 0,
model: non-empty string).
- Around line 51-99: The spec omits behavior for a single input whose
estimateTokens exceeds the 8192 token bound; update the
chatbot-corpus-embeddings spec to require explicit rejection of such inputs by
adding a scenario and mandating the provider validate inputs before calling the
SDK. Specify that the azureOpenAI implementation
(apps/api/src/features/chatbot/embeddingProvider/azureOpenAI.ts) MUST run
estimateTokens on each item and, if any single input > 8192, throw a specific
error (e.g., InputTooLargeError) and not invoke Azure, include this behavior in
the scenarios (add "Single input exceeding token bound is rejected") and require
tests to assert the error is thrown rather than letting Azure return HTTP 400.
- Around line 22-50: Update the spec to explicitly state the normalization
algorithm: require L2 (Euclidean) normalization (i.e., sum of squared components
equals 1.0 within floating-point precision), specify that normalization is
applied after producing the 1024-dim vector (after any truncation/padding), and
note the zero-vector fallback behavior (e.g., if norm==0 leave as-is or define a
deterministic fallback), and add a concrete scenario titled "Mock normalization
uses L2 norm" that asserts the returned vector from the mock in
apps/api/src/features/chatbot/embeddingProvider/mock.ts is L2-normalized and
still deterministic/byte-equal across calls; keep the model literal
"mock-sha256-1024" and the token-count requirement using estimateTokens intact.
In `@openspec/changes/chatbot-rag-mvp/specs/chatbot-corpus-retrieval/spec.md`:
- Around line 64-85: The spec omits behavior for invalid filter values for
options.scope and options.sourceType used by searchKnowledge; decide and
document whether searchKnowledge should validate inputs and throw
InvalidQueryError (use InvalidQueryError as the rejection path) or accept any
value and let SQL return an empty set, then update the spec: add a Scenario
(e.g., "Invalid filter values are rejected") that references searchKnowledge and
explicitly states the chosen behavior for invalid scope (not one of the enum
values) and invalid sourceType (not a known type) so implementations of
searchKnowledge, options.scope, and options.sourceType follow the same
validation contract.
- Around line 1-39: The spec should validate and normalize the query before
embedding: in searchKnowledge trim leading/trailing whitespace and reject empty
strings by throwing InvalidQueryError (so calls to
getEmbeddingProvider().embed([query]) and SQL are not made for empty input); add
a maximum token/length check (e.g., use an estimateTokens(query) helper and
reject >512 tokens with InvalidQueryError); and ensure embedding failures from
getEmbeddingProvider().embed(...) are surfaced (either re-throw the original
error or wrap it with context) rather than swallowed — implement these checks
and error flows in searchKnowledge to run before any embed/SQL work.
- Around line 86-105: Update the spec's topK validation language and scenarios
for searchKnowledge to explicitly state handling of non-integer, negative, and
string values: require topK be an integer in [1,20] (reject floats like 3.5 with
InvalidQueryError), treat negative values as out-of-range (reject with
InvalidQueryError), and declare that string values (e.g., "8") are not accepted
and should either be rejected with InvalidQueryError or coerced only if an
explicit rule is added; also add a new Scenario (e.g., "Non-integer topK is
rejected") referencing searchKnowledge and InvalidQueryError so tests must
assert error.name === "InvalidQueryError" and that no SQL executes when invalid.
In `@openspec/changes/chatbot-rag-mvp/specs/chatbot-corpus-schema/spec.md`:
- Around line 31-40: The PR uses the Prisma field annotation
Unsupported("vector(1024)")? to mark the embedding column as nullable and
prevent drift; confirm that your Prisma CLI/Client version supports wrapping the
native type in Unsupported(... ) with the nullability suffix (the exact symbol
Unsupported("vector(1024)")?) and, if it does not, change the schema to use the
supported pattern for marking native/unsupported types plus nullability (e.g.,
apply Unsupported("vector(1024)") on the field and append ? for nullability
according to the Prisma version docs) and regenerate the migration so the
generated SQL and migrate diff no longer produce warnings; check and update the
Prisma version or schema to match the documented syntax for Unsupported on the
embedding field identifier (the field using Unsupported("vector(1024)")?).
In `@openspec/changes/chatbot-rag-mvp/specs/chatbot-llm-provider/spec.md`:
- Around line 101-106: The LLM provider's environment handling must enforce the
production guard like the embedding provider: in
apps/api/src/features/chatbot/llmProvider/azureOpenAI.ts (and its env/bootstrap
helper if present) add an import-time validation that throws when IS_PROD is
true and AZURE_OPENAI_API_KEY is a non-empty trimmed string; keep the existing
behavior of using the API key when AZURE_OPENAI_API_KEY is set and
DefaultAzureCredential otherwise, but fail fast in production by checking the
AZURE_OPENAI_API_KEY value and throwing a clear error (reference symbols:
AZURE_OPENAI_API_KEY, IS_PROD, DefaultAzureCredential, and the
module/initializer that constructs AzureOpenAI) so accidental API-key auth in
prod cannot silently occur.
In `@openspec/changes/chatbot-rag-mvp/specs/chatbot-widget/spec.md`:
- Around line 65-68: Update the "Scenario: Snippet is rendered beneath the link"
in chatbot-widget/spec.md to explicitly state that the widget displays the
snippet exactly as received in the done payload (which the streaming handler
already truncates to 240 chars per chatbot-message-streaming/spec.md), and that
the UI layer MUST NOT perform any further truncation or re-truncation; reference
the done payload and the streaming-spec truncation behavior to avoid ambiguity
during implementation.
---
Outside diff comments:
In `@openspec/changes/chatbot-rag-mvp/specs/chatbot-corpus-embeddings/spec.md`:
- Around line 185-198: Clarify the mock provider's abort-check behavior: state
explicitly in the mock provider requirement (referencing the "mock" provider and
its use of options.signal / signal.aborted) whether it performs internal
batching or iterates per-input, and if batching is used specify the batch size
or describe "batch boundaries" and that signal.aborted is checked between
batches; if no batching, state that the mock checks signal.aborted between each
input iteration. Also explicitly keep the azureOpenAI requirement that
options.signal is passed through to the underlying OpenAI SDK call so upstream
cancellation is honored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Lite
Run ID: ad3b8e0e-ad63-4222-9e04-630795c209a3
📒 Files selected for processing (10)
openspec/changes/chatbot-rag-mvp/design.mdopenspec/changes/chatbot-rag-mvp/proposal.mdopenspec/changes/chatbot-rag-mvp/specs/chatbot-corpus-embeddings/spec.mdopenspec/changes/chatbot-rag-mvp/specs/chatbot-corpus-ingest/spec.mdopenspec/changes/chatbot-rag-mvp/specs/chatbot-corpus-retrieval/spec.mdopenspec/changes/chatbot-rag-mvp/specs/chatbot-corpus-schema/spec.mdopenspec/changes/chatbot-rag-mvp/specs/chatbot-llm-provider/spec.mdopenspec/changes/chatbot-rag-mvp/specs/chatbot-message-streaming/spec.mdopenspec/changes/chatbot-rag-mvp/specs/chatbot-widget/spec.mdopenspec/changes/chatbot-rag-mvp/tasks.md
Collaborator
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (3)
openspec/changes/chatbot-rag-mvp/tasks.md (1)
55-55:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAdvisory lock collision vulnerability in Task 6.1 (cross-reference).
Line 55 specifies the same advisory lock key pattern flagged in
chatbot-corpus-ingest/spec.mdanddesign.md:$key = 'chatbot-corpus:' + name + ':' + scope. This construction is vulnerable to delimiter-based collisions whennamecontains:characters.See the detailed analysis and proposed fix in the review comment on
chatbot-corpus-ingest/spec.mdlines 127-162.Update this task to use a collision-resistant key format (null-byte delimiter or hash-based).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openspec/changes/chatbot-rag-mvp/tasks.md` at line 55, The advisory-lock key construction in the Activate CLI task is vulnerable to delimiter collisions; update the task implementation guidance for apps/api/scripts/chatbot/activateCorpusSource.ts so that the prisma.$transaction(...) first statement still acquires an advisory lock but builds $key using a collision-resistant format (e.g., concatenate with a NUL delimiter like 'chatbot-corpus:\0' + name + '\0' + scope or compute a hash of 'chatbot-corpus:' + name + ':' + scope and use that hash as the input to the md5->bigint expression), keep the preferred md5-to-bigint SQL form SELECT pg_advisory_xact_lock(('x' || substr(md5($key), 1, 16))::bit(64)::bigint), and maintain the existing post-lock validation that target.status === 'DRAFT' and reject non-DRAFT targets with a Spanish error message.openspec/changes/chatbot-rag-mvp/design.md (1)
79-88:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAdvisory lock collision issue also documented here (cross-reference).
Lines 81-82 document the same advisory lock key pattern flagged in
chatbot-corpus-ingest/spec.md:$key = 'chatbot-corpus:' || name || ':' || scope. The delimiter-based collision vulnerability applies here as well. See the review comment onchatbot-corpus-ingest/spec.mdlines 127-162 for the detailed analysis and proposed fix.This design decision should be updated to specify a collision-resistant key format once the spec is corrected.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openspec/changes/chatbot-rag-mvp/design.md` around lines 79 - 88, The advisory-lock key construction ($key = 'chatbot-corpus:' || name || ':' || scope used in the SELECT pg_advisory_xact_lock(('x' || substr(md5($key), 1, 16))::bit(64)::bigint) statement) is vulnerable to delimiter-collision; update the spec to mandate a collision-resistant canonicalization for the key (e.g., length-prefixed fields, explicit escaping, or stable encoding such as JSON/array encoding of {name,scope}) instead of simple ':' concatenation, and call out that the same corrected key format must be used everywhere (including the chatbot-corpus-ingest flow) so advisory-locks are unambiguous across name+scope combinations.openspec/changes/chatbot-rag-mvp/specs/chatbot-corpus-retrieval/spec.md (1)
69-90: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winSpecify validation for invalid filter values.
The requirement (lines 69-71) defines how
scopeandsourceTypefilters are applied but doesn't specify what happens when invalid values are provided. The past review comment on this same line range flagged this gap, and it remains unaddressed.Should
searchKnowledgevalidate that:
scopeis one of the valid enum values (e.g.,'GLOBAL' | 'NATIONAL')?sourceTypeis a known type (e.g.,'PDF' | 'MD')?Three options:
- TypeScript-only validation: Use strict enum types in the function signature to prevent invalid values at compile time (no runtime check needed).
- Runtime validation: Throw
InvalidQueryErrorfor invalid filter values before executing SQL.- SQL filtering: Accept any value and let the SQL
WHEREclause return an empty set.The spec should explicitly state the chosen approach. If relying on TypeScript types, add a scenario that asserts the function signature uses strict enums (not
string). If runtime validation, add a scenario like:#### Scenario: Invalid filter values are rejected - **WHEN** `searchKnowledge` is invoked with `options = { scope: 'INVALID' as any }` (bypassing TypeScript) - **THEN** the function SHALL throw `InvalidQueryError` indicating the invalid filter valueWithout this clarification, implementations might diverge on their validation approach, and the streaming handler won't know whether to expect
InvalidQueryErrorfrom invalid filters.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openspec/changes/chatbot-rag-mvp/specs/chatbot-corpus-retrieval/spec.md` around lines 69 - 90, The spec is missing behavior for invalid options.scope/options.sourceType values; update the chatbot-corpus-retrieval spec to explicitly choose and document one validation strategy (pick TypeScript-only enums, runtime validation throwing InvalidQueryError, or SQL filtering) and add a scenario reflecting that choice: if choosing runtime validation add a "Scenario: Invalid filter values are rejected" that invokes searchKnowledge with invalid values and expects InvalidQueryError; if choosing TypeScript-only add a scenario asserting the searchKnowledge signature uses strict enums (not string) for scope/sourceType; reference searchKnowledge, options.scope, options.sourceType, and InvalidQueryError in the new text so implementations know which behavior to follow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openspec/changes/chatbot-rag-mvp/specs/chatbot-corpus-embeddings/spec.md`:
- Around line 63-73: Add explicit error-handling requirements to the azureOpenAI
embedding provider spec: describe that the provider at
apps/api/src/features/chatbot/embeddingProvider/azureOpenAI.ts (which already
enforces InputTooLargeError via estimateTokens) must also surface network,
rate-limit (HTTP 429), quota/503 and 5xx failures with distinguishable error
metadata so callers can decide retries; require the thrown Error to include
either a stable error.name or code (e.g., "TransientEmbeddingError" for
retryable 429/5xx/network timeouts and "PermanentEmbeddingError" for
non-retryable failures) or include a boolean retryable flag plus the underlying
HTTP status in error.statusCode / error.cause so consumers (ingest CLI,
retrieval path) can programmatically inspect and implement backoff/retry logic;
alternatively allow preserving the upstream openai SDK error but mandate that
the provider must not swallow/mask statusCode/cause and must document which SDK
error types map to retryable vs permanent outcomes.
In `@openspec/changes/chatbot-rag-mvp/specs/chatbot-corpus-ingest/spec.md`:
- Around line 127-162: The advisory-lock key concatenation using
'chatbot-corpus:' || name || ':' || scope is vulnerable to delimiter collisions;
update the lock key construction used by the script activateCorpusSource.ts (the
SQL statement that builds $key for SELECT pg_advisory_xact_lock(('x' ||
substr(md5($key), 1, 16))::bit(64)::bigint)) to compute the hash over an
unambiguous tuple—e.g. replace $key with md5(name || E'\0' || scope) (or
md5(length(name) || ':' || name || ':' || scope) if you prefer length-prefixing)
so the md5 input cannot collide, and/or add CLI validation in the --label
parsing to reject ':' if you choose to keep simple concatenation; update the
spec text and tests to reflect the chosen change and include a test case
exercising labels containing ':'.
In `@openspec/changes/chatbot-rag-mvp/specs/chatbot-corpus-retrieval/spec.md`:
- Line 37: Specify and standardize the estimateTokens helper used by
searchKnowledge: declare that estimateTokens(query: string) is imported from a
shared utility (e.g., apps/api/src/features/chatbot/utils/estimateTokens.ts) and
define its contract (returns an integer token count, uses cl100k_base encoding
consistent with GPT-4 series, and trims input before counting); update the spec
to reference this import and require all paths (searchKnowledge, streaming RAG
context caps, history enforcement) to call that same function so implementations
use a single tokenization method rather than ad-hoc token counters.
- Line 28: The spec currently mandates calling
getEmbeddingProvider().embed([query]) but omits failure semantics; add an
explicit failure scenario that states when getEmbeddingProvider().embed([query])
throws (network/rate-limit/invalid-key), the error SHALL propagate to the caller
unchanged (i.e., do not swallow or wrap it), and note that the streaming handler
will map such upstream provider errors to HTTP 503 with code
EXTERNAL_SERVICE_ERROR; update the spec text near the THEN clause and include
the new "Scenario: Embedding provider failures propagate to caller" block so
tests and implementers know to surface embedding provider errors rather than
converting them to InvalidQueryError or hiding them behind domain-specific
wrappers like EmbeddingError.
In `@openspec/changes/chatbot-rag-mvp/specs/chatbot-message-streaming/spec.md`:
- Around line 95-99: Update the "Modo C welcome response covers capabilities and
roadmap on greeting/help queries" scenario to tighten assertions by requiring an
explicit capability phrase and an explicit roadmap phrase instead of single
keywords: require at least one concrete capability phrase such as "responder
preguntas sobre metodología de huella de carbono" or "explicar alcances 1/2/3"
to appear, and require a clear roadmap phrase such as "guía de uso de la
plataforma llegará en próximas versiones" or "la guía de uso estará disponible
en la próxima versión" to appear; alternatively, if the looser keyword-based
checks are intentional, add a short rationale sentence to the scenario
explaining why flexibility is preferred so reviewers understand the design
choice.
- Line 5: The spec ambiguity: clarify and enforce explicit boot-time loading so
missing prompt fails startup; implement an exported async init function (e.g.,
initChatbotPrompts) that reads
apps/api/src/features/chatbot/prompts/es/system.md into a module-scoped constant
(SYSTEM_PROMPT) using fs.readFileSync/async and throws on error, call this init
during API startup, and have the streaming handler (the function that calls
LLMProvider.streamCompletion) always prepend a SYSTEM role message using the
cached SYSTEM_PROMPT so the file is read once and failure surfaces at boot
instead of lazily.
In `@openspec/changes/chatbot-rag-mvp/tasks.md`:
- Around line 67-147: The system prompt's three-mode routing (Modo A / Modo B /
Modo C) and K=0 guardrail lack validation and tests for misclassification; add a
regression test covering borderline inputs (e.g., "¿Cómo calculo las emisiones
de mi empresa?", "Necesito el factor de emisión para mi inventario") to ensure
the prompt doesn't wrongly route methodology questions to Modo B or platform
questions to Modo A, and update the test suite references near tests 10.35/10.36
to include this edge-case. If changing tests is not desired, implement a
lightweight server-side validation step after the LLM classification that
inspects the original user message for methodology keywords (e.g., "alcance",
"factor", "GHG", "IPCC", "calculo") versus platform keywords ("inventario",
"invitar", "verificación", "navegación") and: (1) if the model emitted Modo B
but message contains methodology keywords, emit a warning/telemetry event for
prompt tuning and optionally re-run classification or flag for human review; (2)
if searchKnowledge returns K=0 but the original message contained platform-usage
keywords, also emit telemetry/warn. Ensure the validation references the routing
outputs (Modo A/Modo B/Modo C), the searchKnowledge result (K=0), and hooks into
the existing logging/telemetry pipeline for later prompt tuning.
---
Duplicate comments:
In `@openspec/changes/chatbot-rag-mvp/design.md`:
- Around line 79-88: The advisory-lock key construction ($key =
'chatbot-corpus:' || name || ':' || scope used in the SELECT
pg_advisory_xact_lock(('x' || substr(md5($key), 1, 16))::bit(64)::bigint)
statement) is vulnerable to delimiter-collision; update the spec to mandate a
collision-resistant canonicalization for the key (e.g., length-prefixed fields,
explicit escaping, or stable encoding such as JSON/array encoding of
{name,scope}) instead of simple ':' concatenation, and call out that the same
corrected key format must be used everywhere (including the
chatbot-corpus-ingest flow) so advisory-locks are unambiguous across name+scope
combinations.
In `@openspec/changes/chatbot-rag-mvp/specs/chatbot-corpus-retrieval/spec.md`:
- Around line 69-90: The spec is missing behavior for invalid
options.scope/options.sourceType values; update the chatbot-corpus-retrieval
spec to explicitly choose and document one validation strategy (pick
TypeScript-only enums, runtime validation throwing InvalidQueryError, or SQL
filtering) and add a scenario reflecting that choice: if choosing runtime
validation add a "Scenario: Invalid filter values are rejected" that invokes
searchKnowledge with invalid values and expects InvalidQueryError; if choosing
TypeScript-only add a scenario asserting the searchKnowledge signature uses
strict enums (not string) for scope/sourceType; reference searchKnowledge,
options.scope, options.sourceType, and InvalidQueryError in the new text so
implementations know which behavior to follow.
In `@openspec/changes/chatbot-rag-mvp/tasks.md`:
- Line 55: The advisory-lock key construction in the Activate CLI task is
vulnerable to delimiter collisions; update the task implementation guidance for
apps/api/scripts/chatbot/activateCorpusSource.ts so that the
prisma.$transaction(...) first statement still acquires an advisory lock but
builds $key using a collision-resistant format (e.g., concatenate with a NUL
delimiter like 'chatbot-corpus:\0' + name + '\0' + scope or compute a hash of
'chatbot-corpus:' + name + ':' + scope and use that hash as the input to the
md5->bigint expression), keep the preferred md5-to-bigint SQL form SELECT
pg_advisory_xact_lock(('x' || substr(md5($key), 1, 16))::bit(64)::bigint), and
maintain the existing post-lock validation that target.status === 'DRAFT' and
reject non-DRAFT targets with a Spanish error message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Lite
Run ID: 419c9a1e-4358-4a66-94f5-3dea9c9edd27
📒 Files selected for processing (10)
openspec/changes/chatbot-rag-mvp/design.mdopenspec/changes/chatbot-rag-mvp/proposal.mdopenspec/changes/chatbot-rag-mvp/specs/chatbot-corpus-embeddings/spec.mdopenspec/changes/chatbot-rag-mvp/specs/chatbot-corpus-ingest/spec.mdopenspec/changes/chatbot-rag-mvp/specs/chatbot-corpus-retrieval/spec.mdopenspec/changes/chatbot-rag-mvp/specs/chatbot-corpus-schema/spec.mdopenspec/changes/chatbot-rag-mvp/specs/chatbot-llm-provider/spec.mdopenspec/changes/chatbot-rag-mvp/specs/chatbot-message-streaming/spec.mdopenspec/changes/chatbot-rag-mvp/specs/chatbot-widget/spec.mdopenspec/changes/chatbot-rag-mvp/tasks.md
nicoalegria11
force-pushed
the
feat/chatbot-rag-mvp
branch
from
May 6, 2026 21:46
abea8af to
e088702
Compare
Collaborator
Author
|
@coderabbitai review Generated by Claude Code |
|
Tip For best results, initiate chat on the files or code changes.
[review] |
Collaborator
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 21
♻️ Duplicate comments (1)
openspec/changes/chatbot-rag-mvp/specs/chatbot-llm-provider/spec.md (1)
107-108:⚠️ Potential issue | 🟠 Major | ⚡ Quick winProduction auth-path guard is still implicit instead of enforced.
Line 107 says production leaves
AZURE_OPENAI_API_KEYunset, but this is policy text only. Add an explicit fail-fast production guard to prevent accidental API-key auth drift.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openspec/changes/chatbot-rag-mvp/specs/chatbot-llm-provider/spec.md` around lines 107 - 108, The spec currently relies on policy text for production auth but lacks an enforced guard; add a fail-fast check at startup (or inside the Azure/OpenAI client factory such as createAzureOpenAIClient) that inspects AZURE_OPENAI_API_KEY and the runtime environment (e.g., NODE_ENV or ENVIRONMENT) and if NODE_ENV === 'production' (or equivalent production flag) and AZURE_OPENAI_API_KEY is set to a non-empty trimmed string, throw a clear error and exit (or reject initialization) to prevent API-key auth drift, otherwise proceed to use API key when present or DefaultAzureCredential when absent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/scripts/chatbot/activateCorpusSource.ts`:
- Around line 26-31: The parser in activateCorpusSource.ts currently allows "0"
because it uses /^\d+$/: change the validation so zero is rejected up-front
(e.g. use /^[1-9]\d*$/ or parse the string and require >0) before returning {
sourceId: BigInt(raw) }, and keep throwing the same CliArgumentError with the
localized message when raw is not a positive integer; update the check that
references raw and the returned object to ensure invalid "0" produces the
argument error rather than falling through to the lookup path.
In `@apps/api/scripts/chatbot/chunking.ts`:
- Around line 81-94: tryHeaderAlignedSplit() currently only reports whether a
header exists nearby, but the caller (_shouldSplit logic around where
`_shouldSplit` is set to true) ignores the location and always flushes at the
current sentence boundary; change tryHeaderAlignedSplit() to return the header
line index (or null) instead of a boolean, scan the same window and return the
matching i when HEADER_REGEX.test(lines[i].trim()) succeeds, and update the
caller that currently forces `_shouldSplit = true` to use this returned index to
reposition the split boundary (e.g., set the split cursor/position to the start
of that header line) so that an overflow will flush at the detected header
location rather than the original sentence boundary — make sure to update any
callers (including the logic referenced around lines ~119-128) to handle the new
return type.
In `@apps/api/scripts/chatbot/ingestCorpus.ts`:
- Around line 145-159: The preflight DRAFT collision check using
prisma.chatbotCorpusSource.findFirst with CorpusSourceStatus.DRAFT is raceable
because it occurs outside the transaction that creates the source; fix by
enforcing uniqueness at the DB level (add a unique or partial unique index on
(name, version) where status = 'DRAFT') or by acquiring an advisory lock and
performing a second existence check inside the same transaction that calls
create/upsert for the source; update the ingest/activate code paths (the code
around the preflight check and the transaction that creates the new
chatbotCorpusSource) to either rely on the DB constraint (handle
unique-violation errors) or to use the advisory lock + re-check-before-insert
pattern so concurrent ingests cannot both create DRAFT records.
In `@apps/api/scripts/chatbot/parsePdf.ts`:
- Around line 18-27: The PDFParse instance created as parser (PDFParse) isn't
cleaned up; modify the function in apps/api/scripts/chatbot/parsePdf.ts to
ensure parser.destroy() is called in a finally block after extraction:
initialize parser in the outer scope, keep the existing try/catch for extraction
(using parser.getText()), and in finally await parser.destroy() (guarding with
if (parser) and a try/catch around destroy() to avoid masking the original
error); reference parser, PDFParse, destroy(), and filePath when applying the
change.
In `@apps/api/src/config/environment.ts`:
- Around line 365-370: Update the EMBEDDING_PROVIDER boot-time validation in
environment.ts to also require AZURE_OPENAI_ENDPOINT when EMBEDDING_PROVIDER
(the local variable raw) equals "azure-openai"; currently only
AZURE_OPENAI_EMBEDDING_DEPLOYMENT_NAME is checked. Mirror the LLM_PROVIDER
pattern by adding a check for AZURE_OPENAI_ENDPOINT alongside
AZURE_OPENAI_EMBEDDING_DEPLOYMENT_NAME and throw a clear Error mentioning both
AZURE_OPENAI_ENDPOINT and AZURE_OPENAI_EMBEDDING_DEPLOYMENT_NAME if either is
missing.
In `@apps/api/src/features/chatbot/embeddingProvider/azureOpenAI.ts`:
- Around line 128-144: The loop over batches currently checks
options?.signal?.aborted only before calling client.embeddings.create, which can
delay aborting until the next iteration; after you push embeddings into
allVectors and update totalInputTokens inside the same loop (the block handling
response.data and totalInputTokens), immediately re-check
options?.signal?.aborted and if set throw the same "Embedding aborted" Error so
processing terminates promptly; update the loop in the function handling batches
(the for (const batch of batches) block that calls client.embeddings.create and
mutates allVectors and totalInputTokens) to perform this additional post-push
abort check.
In `@apps/api/src/features/chatbot/embeddingProvider/mock.ts`:
- Around line 27-29: The comment above the vector assignment incorrectly states
the range "[-1, 1)" even though the expression vector[offset++] = u32 /
0xffffffff - 0.5 produces values in approximately [-0.5, 0.5]; update the
comment to accurately describe the produced range (e.g., "Map u32 to roughly
[-0.5, 0.5) so the normalization step...") and reference the u32 -> u32 /
0xffffffff - 0.5 transformation in the comment for clarity.
In `@apps/api/src/features/chatbot/llmProvider/mock.ts`:
- Around line 16-33: isSecondRound() currently scans the whole history and
returns true when any ASSISTANT(with toolCalls) → TOOL pair exists anywhere;
change it to only inspect the tail of the conversation so it only detects a
second-round when the most recent exchange is ASSISTANT(toolCalls) followed
immediately by a TOOL. Modify the function (isSecondRound(messages:
LlmMessage[])) to check that messages.length >= 2, then examine the last message
and its immediate predecessor (last and last-1) and return true only if
last.role === ChatMessageRole.TOOL and prev.role === ChatMessageRole.ASSISTANT
and prev.toolCalls?.length > 0; otherwise return false.
In `@apps/api/src/features/chatbot/llmProvider/types.ts`:
- Line 1: The import of ChatMessageRole is used only in type-level "typeof"
expressions (e.g., where types reference typeof ChatMessageRole) so change the
statement to a type-only import: replace the current import of ChatMessageRole
with "import type { ChatMessageRole }" to avoid pulling runtime code; update the
import line that currently reads "import { ChatMessageRole }" to use "import
type" and leave all references (typeof ChatMessageRole) unchanged.
In `@apps/api/src/features/chatbot/tools/searchKnowledge/execute.ts`:
- Around line 7-9: The code currently treats model-generated argsJson as
trusted; wrap the JSON.parse of argsJson in a try/catch and validate with
SearchKnowledgeArgsSchema.safeParse (or z.safeParse) instead of parse() so a
malformed JSON or invalid shape doesn't throw, and additionally normalize the
query by trimming whitespace and treating an empty/whitespace-only query as a
controlled "no sources" or explicit tool-error response rather than letting it
bubble an exception; update the same parsing/validation logic used around the
argsJson handling (the block that reads argsJson and the subsequent validation)
to return a standardized tool-error/no-results object when validation fails.
In `@apps/web/src/components/Chatbot/ChatbotWidget.tsx`:
- Around line 95-101: Add a "deleting" boolean state and use it in the confirm
dialog so the confirm button is disabled and shows a loading indicator while
deleteHistory() is in flight: set deleting=true before calling deleteHistory()
in handleConfirmDelete, await deleteHistory(), set deleting=false after the
await, and still close the dialog with setConfirmDeleteOpen(false) (or keep it
closed) and call setSnackbarMessage(result.ok ? DELETE_SUCCESS_MESSAGE :
DELETE_ERROR_MESSAGE); update the confirm button to read the deleting state to
disable clicks and show a spinner, referencing handleConfirmDelete,
deleteHistory, setConfirmDeleteOpen, and setSnackbarMessage.
In `@apps/web/src/components/Chatbot/MessageBubble.tsx`:
- Around line 106-115: The code renders a <Link> even when source.cite_url can
be null; change the mapping in MessageBubble (the sources.map callback) to
conditionally render a Link when source.cite_url is truthy and otherwise render
plain text (e.g., a Typography or span) with the same variant/visual styling
using source.cite_label; keep the same key
(`${source.source_id}-${source.chunk_id}`) and only include href, target, rel on
the Link branch so null URLs don’t produce inert clickable elements.
In `@apps/web/src/components/Chatbot/useChatStream.ts`:
- Around line 343-368: After a successful delete in deleteHistory, cancel any
in-flight generation before calling clearLocalConversation by either aborting
the active AbortController used by sendMessage/SSE consumers or advancing a
per-turn generation token that sendMessage checks; ensure sendMessage (and any
SSE consumer) checks controller.signal.aborted or compares the generation token
before applying setState/setMessages/lastEventIdRef updates so stale completions
are ignored, then create/reset a fresh AbortController or increment the
generation token inside clearLocalConversation (and update
inFlightAssistantIndexRef/consecutiveFailuresRef only after ensuring the old
turn is cancelled).
In `@openspec/changes/chatbot-rag-mvp/specs/chatbot-corpus-ingest/spec.md`:
- Around line 89-92: Clarify and enforce precedence by updating the ingest CLI
flow so argument validation runs before any DB writes: perform all input/flag
validation and authorization checks first, and only after validation succeeds
insert the audit row into chatbot_corpus_ingest_run with started_at = NOW(),
triggered_by populated, source_id = NULL, completed_at = NULL; proceed with the
embedding-and-source transaction and on success update that same audit row with
source_id, completed_at = NOW(), chunks_created and embedding_model; if
validation fails, return error and do not insert the audit row (preserving the
“invalid args write nothing” behavior).
- Line 134: Update the spec text in the chatbot corpus activation section
(apps/api/scripts/chatbot/activateCorpusSource.ts) to stop claiming the 64-bit
md5-derived advisory lock key eliminates cross-key collisions; instead state
that the md5-cast-to-bigint form reduces collision risk compared to 32-bit
hashtext and is preferred for matching the advisory-lock width and portability
vs hashtextextended, but that hash-derived keys still have a non-zero collision
probability and the choice is an acceptable tradeoff. Mention the specific SQL
expression SELECT pg_advisory_xact_lock(('x' || substr(md5($key), 1,
16))::bit(64)::bigint) and keep the rationale about portability and matching
lock-space width while softening the absolute language about collisions.
In `@openspec/changes/chatbot-rag-mvp/specs/chatbot-corpus-retrieval/spec.md`:
- Around line 78-99: Add a short explicit scenario to the spec clarifying filter
validation: state that searchKnowledge's options.scope and options.sourceType
are typed as CorpusSourceScope and CorpusSourceType enums (so TypeScript callers
cannot pass invalid strings at compile time) and define runtime behavior for
non-TypeScript callers (e.g., raw JSON) — if an invalid enum string is supplied
the SQL will not match any rows and the searchKnowledge call will return an
empty result set; reference the searchKnowledge function and the
options.scope/options.sourceType parameters so reviewers can locate where to
document this.
In `@openspec/changes/chatbot-rag-mvp/specs/chatbot-llm-provider/spec.md`:
- Around line 45-46: The spec is inconsistent about tool_call.arguments
(declared as a string in the streamCompletion contract but later required to be
a JSON object); update the LLMProvider.streamCompletion contract so
tool_call.arguments has a single, explicit representation—preferably a
serialized JSON string on the public interface (streamCompletion return values)
and document that implementations may parse it into an object only when passing
to tool handlers. Ensure the types/examples for tool_call in the
streamCompletion description and the later usage examples/reference (including
any mentions around lines ~86-87) all use the chosen representation and note
where parsing should occur (handler boundary).
In `@openspec/changes/chatbot-rag-mvp/specs/chatbot-message-streaming/spec.md`:
- Around line 107-114: The spec currently injects untrusted chunk content
directly into the tool-result string; update the behavior in
executeSearchKnowledgeTool to treat all retrieved chunk text strictly as data by
wrapping snippets in an explicit, non-executable delimiter/quotation (e.g.,
blockquote or fenced code style) and escaping or sanitizing characters that
could be interpreted as instructions, ensure Zod metadata validation remains,
add a clear rule in the function's output formatting that the assistant must not
treat chunk content as executable instructions, and add unit/integration tests
that supply hostile chunk content (e.g., "ignore previous; do X", embedded
markdown/HTML/JS) to verify the formatted output preserves content as quoted
data only and that the LLM prompt does not allow instruction-following from
retrieved chunks.
In `@openspec/changes/chatbot-rag-mvp/specs/chatbot-widget/spec.md`:
- Around line 89-90: The requirement text currently uses the invalid attribute
term `aria-role`; update that sentence so it forbids a real ARIA role by
replacing `aria-role` with role="button" (or rephrase to “must not be
interactive/button-like”) so the spec reads that the element SHALL NOT carry any
onClick handler, role="button", or visible dismiss control and SHALL be a static
text node only, ensuring test and implementation clarity.
- Around line 103-104: Update the contradictory spec language so the “SHALL NOT
issue any HTTP request” rule is scoped to the trash icon's handler only: clarify
that the trash icon click must not call any backend (no DELETE
/chat/conversations/..., and persisted chatbot_chat_conversation and
chatbot_chat_message rows remain untouched) and that the DELETE
/api/chatbot/conversations/me requirement later in the doc is a separate API
behavior for a different action/endpoint; reference the trash icon/aria label
"Limpiar conversación" and the DELETE /api/chatbot/conversations/me requirement
to ensure the two behaviors are distinct.
In `@packages/types/src/chatbot/sourceCitation/schemas.ts`:
- Around line 5-24: Replace the loose stringOrBigInt/z.number usage so IDs
reject non-ID numbers: import and use the repository's IdSchema for string-form
IDs and constrain the bigint branch to positive integers (e.g.,
z.bigint().min(1)); update SourceCitationSchema to use this tightened union for
source_id and chunk_id (referencing stringOrBigInt or the new union name if you
rename it) so decimals, negatives, Infinity, and other non-identifier numbers
are rejected.
---
Duplicate comments:
In `@openspec/changes/chatbot-rag-mvp/specs/chatbot-llm-provider/spec.md`:
- Around line 107-108: The spec currently relies on policy text for production
auth but lacks an enforced guard; add a fail-fast check at startup (or inside
the Azure/OpenAI client factory such as createAzureOpenAIClient) that inspects
AZURE_OPENAI_API_KEY and the runtime environment (e.g., NODE_ENV or ENVIRONMENT)
and if NODE_ENV === 'production' (or equivalent production flag) and
AZURE_OPENAI_API_KEY is set to a non-empty trimmed string, throw a clear error
and exit (or reject initialization) to prevent API-key auth drift, otherwise
proceed to use API key when present or DefaultAzureCredential when absent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Lite
Run ID: da572870-a24d-4022-a11e-aeca965c4d8d
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (61)
apps/api/eslint.config.tsapps/api/package.jsonapps/api/scripts/chatbot/activateCorpusSource.tsapps/api/scripts/chatbot/chunking.tsapps/api/scripts/chatbot/ingestCorpus.tsapps/api/scripts/chatbot/parsePdf.tsapps/api/src/config/environment.tsapps/api/src/features/chatbot/embeddingProvider/azureOpenAI.tsapps/api/src/features/chatbot/embeddingProvider/index.tsapps/api/src/features/chatbot/embeddingProvider/mock.tsapps/api/src/features/chatbot/embeddingProvider/types.tsapps/api/src/features/chatbot/llmProvider/azureOpenAI.tsapps/api/src/features/chatbot/llmProvider/index.tsapps/api/src/features/chatbot/llmProvider/mock.tsapps/api/src/features/chatbot/llmProvider/types.tsapps/api/src/features/chatbot/prompts/es/system.mdapps/api/src/features/chatbot/prompts/loader.tsapps/api/src/features/chatbot/searchKnowledge/errors.tsapps/api/src/features/chatbot/searchKnowledge/index.tsapps/api/src/features/chatbot/searchKnowledge/searchKnowledge.tsapps/api/src/features/chatbot/searchKnowledge/types.tsapps/api/src/features/chatbot/sendMessage/handler.tsapps/api/src/features/chatbot/tools/searchKnowledge/execute.tsapps/api/src/features/chatbot/tools/searchKnowledge/index.tsapps/api/src/features/chatbot/tools/searchKnowledge/schema.tsapps/api/test/features/chatbot/embeddingProvider/unit.test.tsapps/api/test/features/chatbot/lint/noReferencesToCorpusTables.test.tsapps/api/test/features/chatbot/llmProvider/unit.test.tsapps/api/test/features/chatbot/prompts/loader.test.tsapps/api/test/features/chatbot/searchKnowledge/unit.test.tsapps/api/test/features/chatbot/sendMessage/integration.test.tsapps/api/test/setup/testcontainers.tsapps/api/tsconfig.eslint.jsonapps/api/vitest.config.tsapps/web/src/components/Chatbot/ChatbotWidget.tsxapps/web/src/components/Chatbot/MessageBubble.tsxapps/web/src/components/Chatbot/types.tsapps/web/src/components/Chatbot/useChatStream.tsdocker-compose.ymldocs/development/environment-variables.mddocs/development/local-setup.mddocs/operations/runbook.mdopenspec/changes/chatbot-rag-mvp/design.mdopenspec/changes/chatbot-rag-mvp/proposal.mdopenspec/changes/chatbot-rag-mvp/specs/chatbot-corpus-embeddings/spec.mdopenspec/changes/chatbot-rag-mvp/specs/chatbot-corpus-ingest/spec.mdopenspec/changes/chatbot-rag-mvp/specs/chatbot-corpus-retrieval/spec.mdopenspec/changes/chatbot-rag-mvp/specs/chatbot-corpus-schema/spec.mdopenspec/changes/chatbot-rag-mvp/specs/chatbot-llm-provider/spec.mdopenspec/changes/chatbot-rag-mvp/specs/chatbot-message-streaming/spec.mdopenspec/changes/chatbot-rag-mvp/specs/chatbot-widget/spec.mdopenspec/changes/chatbot-rag-mvp/tasks.mdpackages/database/docker-compose.ymlpackages/database/src/prisma/migrations/20260506220000_add_chatbot_embedding_and_pgvector/migration.sqlpackages/database/src/prisma/schema.prismapackages/types/src/chatbot/index.tspackages/types/src/chatbot/sendMessage/schemas.tspackages/types/src/chatbot/sourceCitation/index.tspackages/types/src/chatbot/sourceCitation/schemas.tspackages/types/src/chatbot/sourceCitation/types.tsturbo.json
💤 Files with no reviewable changes (1)
- apps/api/test/features/chatbot/lint/noReferencesToCorpusTables.test.ts
Collaborator
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 19
♻️ Duplicate comments (1)
openspec/changes/chatbot-rag-mvp/specs/chatbot-llm-provider/spec.md (1)
86-87:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUnify
tool_call.argumentsrepresentation (string vs object) across contract and scenarios.Line 86 currently asserts
argumentsequals a JSON object, but Lines 54 and 62 define the interface boundary as a JSON-serialized string. Keep one representation in all examples/scenarios (prefer string on provider boundary, parsed object only at handler boundary).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openspec/changes/chatbot-rag-mvp/specs/chatbot-llm-provider/spec.md` around lines 86 - 87, Update the spec text so the tool_call.arguments uses the JSON-serialized string representation (not a parsed object) consistently across contract and scenarios: change the assertion in the THEN clause for the emitted tool_call (symbol: tool_call.arguments) to require arguments equal to a JSON-serialized string containing the original user message verbatim, and keep the rest of the behavior the same (iterable yields exactly one tool_call with name = "searchKnowledge" and then completes without yielding any delta or usage).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/scripts/chatbot/activateCorpusSource.ts`:
- Around line 80-96: The two separate new Date() calls in the transaction cause
small timestamp drift; instead capture a single timestamp and use it for both
updates so deactivatedAt and activatedAt are identical. In the transaction that
calls tx.chatbotCorpusSource.updateMany(...) and
tx.chatbotCorpusSource.update(...), create one now value (or use a DB-side
single timestamp) before both calls and set deactivatedAt and activatedAt to
that single now; update the code around the locked variable to reference that
shared timestamp.
In `@apps/api/scripts/chatbot/ingestCorpus.ts`:
- Around line 251-259: The audit-row update after creating the source/chunks
must not run separately because if it fails the committed source/chunks appear
as a failed ingest; move the prisma.chatbotCorpusIngestRun.update that writes
sourceId/completedAt/chunksCreated/embeddingModel into the same transactional
block that creates the source and chunks (the code that produces
result.source.id and chunks) so the commit is atomic, or alternatively change
the function to return a partial-success structure that explicitly surfaces the
committed result.source.id when the audit update fails, and ensure callers treat
that as success rather than triggering the generic failure path.
In `@apps/api/src/config/environment.ts`:
- Around line 300-305: The code currently allows AZURE_OPENAI_API_KEY to be used
even in production; enforce the policy by rejecting this env var when NODE_ENV
=== 'production'. Modify the initialization of AZURE_OPENAI_API_KEY (and/or the
helper trimEnv) to throw or exit with a clear error if process.env.NODE_ENV ===
'production' and AZURE_OPENAI_API_KEY is set, so production cannot silently fall
back to API-key auth; include AZURE_OPENAI_API_KEY and NODE_ENV in the error
message to aid diagnosis.
In `@apps/api/src/features/chatbot/embeddingProvider/azureOpenAI.ts`:
- Around line 38-63: In buildClient(), trim AZURE_OPENAI_API_KEY,
AZURE_OPENAI_ENDPOINT, and AZURE_OPENAI_EMBEDDING_DEPLOYMENT_NAME before any
validation or branching so that whitespace-only values are treated as empty; use
the trimmed variables for the initial existence check (throwing the Error if
endpoint or deployment is empty after trim) and for deciding whether to take the
API key branch (check trimmed API key), and pass those trimmed values into the
AzureOpenAI constructor and into getBearerTokenProvider/Azure credential logic.
- Around line 116-123: The empty-input fast path currently returns before
validating provider configuration, which lets embed([]) succeed with model: ""
and hides misconfiguration; update the logic in the embed function to call
getClient() (or otherwise validate AZURE_OPENAI_EMBEDDING_DEPLOYMENT_NAME)
before returning for texts.length === 0 so the provider contract is enforced —
ensure getClient() is invoked (or throw a clear error if
AZURE_OPENAI_EMBEDDING_DEPLOYMENT_NAME is missing/blank) and return the actual
deployment name from AZURE_OPENAI_EMBEDDING_DEPLOYMENT_NAME instead of an empty
string; reference getClient(), validatePerInputBudget(), and
AZURE_OPENAI_EMBEDDING_DEPLOYMENT_NAME to locate the change.
In `@apps/api/src/features/chatbot/embeddingProvider/index.ts`:
- Around line 11-13: The current selection logic silently falls back to
mockEmbeddingProvider for any EMBEDDING_PROVIDER value; update the initializer
in index.ts to validate EMBEDDING_PROVIDER and fail fast on unknown values by
throwing an error or logging and exiting when EMBEDDING_PROVIDER is neither
"azure-openai" nor an accepted option. Specifically, check the
EMBEDDING_PROVIDER env before choosing between azureOpenAIEmbeddingProvider and
mockEmbeddingProvider, and if it’s an unexpected value include the variable name
(EMBEDDING_PROVIDER) and the invalid value in the error message so deployment
errors surface immediately.
In `@apps/api/src/features/chatbot/embeddingProvider/mock.ts`:
- Around line 63-65: The current yield uses await Promise.resolve(), which only
drains microtasks and can delay timer-driven abort propagation; replace that
await with a real macrotask yield such as await new Promise(resolve => (typeof
setImmediate !== "undefined" ? setImmediate(resolve) : setTimeout(resolve, 0)))
so AbortController.abort() scheduled via timers is observed promptly; update the
spot where Promise.resolve() is awaited in the mock embeddings provider (the
block that checks options?.signal?.aborted) to use this setImmediate/setTimeout
pattern.
In `@apps/api/src/features/chatbot/searchKnowledge/searchKnowledge.ts`:
- Around line 41-43: The formatVectorLiteral function currently interpolates
vector values directly into a SQL vector literal; add defensive validation in
formatVectorLiteral to ensure every element of the input array is a finite
number (no NaN, Infinity, -Infinity, null, or non-number), and throw a clear,
descriptive error (including the offending index/value and the function name) if
validation fails so malformed vectors never get formatted and sent to
PostgreSQL; keep the return behavior the same for valid inputs (string like
`[x,y,z]`) and reference formatVectorLiteral when locating the change.
In `@apps/api/src/features/chatbot/sendMessage/handler.ts`:
- Around line 75-105: consumeStream currently accumulates deltas in the local
buffer and only returns them at the end, which means the persisted
assistantBuffer (DB row identified by assistantRowIdString) isn't updated until
the stream finishes; change the loop in consumeStream so that after appending
each delta to buffer and calling writeSseEvent you also persist the new partial
content to the DB (e.g., call your existing updater that writes assistantBuffer
using assistantRowIdString), batching or rate-limiting the writes if needed
(e.g., every N chars or T ms) and handling errors without breaking the stream;
apply the same incremental-persist change to the other similar block referenced
at lines 183-199.
- Around line 253-255: The code currently overwrites firstResult.usage with
secondResult.usage on tool turns, losing round-1 usage; instead, create a small
merge that accumulates numeric usage fields (e.g., tokensUsed, promptTokens,
completionTokens) from firstResult.usage and secondResult.usage (adding them)
and merges any non-numeric metadata safely, then assign that merged object to
usage (or create a helper like accumulateUsage(firstUsage, secondUsage) and call
it). Apply this same accumulation fix wherever usage is set from two provider
calls (references: variables usage, firstResult, secondResult, assistantBuffer
and the similar blocks at the other occurrences mentioned).
In `@apps/api/src/features/chatbot/tools/searchKnowledge/execute.ts`:
- Around line 46-52: The buildCandidateCitation helper currently defaults
cite_label and cite_url to empty strings which intentionally causes
SourceCitationSchema's z.string().url().refine(/^https:\/\//i) validation to
fail and thus silently filter non-HTTPS or missing URLs; add a concise inline
comment above buildCandidateCitation (and the nearby validation/safeParse usage
in the citation filtering block) stating that cite_label/cite_url default to ""
on purpose to trigger schema rejection for non-citable chunks, so readers
understand this is intentional rather than accidental.
In `@apps/api/src/features/chatbot/tools/searchKnowledge/schema.ts`:
- Around line 19-26: The query property currently allows empty and unbounded
strings; update the tool schema by adding length constraints to the "query"
field (e.g., add "minLength": 1 and a reasonable "maxLength" like 512 or 1024)
so empty queries are rejected and very large inputs are limited; ensure these
constraints remain consistent with the existing "required": ["query"] and
"additionalProperties": false entries and validate any callers of the schema
(where the schema object is defined/used) to handle validation errors
accordingly.
In `@apps/api/test/features/chatbot/embeddingProvider/unit.test.ts`:
- Around line 65-81: The string-matching test in the "mockEmbeddingProvider
source — no network imports" block is redundant and brittle; replace it by
invoking the existing ESLint rule programmatically: use the ESLint Node API in
the unit.test.ts to lint the mock provider file and assert there are zero
violations for the chatbot/no-network-imports-in-mock rule instead of checking
FORBIDDEN_NETWORK_IMPORTS via readFileSync and manual string contains; locate
the describe/it block (the test using FORBIDDEN_NETWORK_IMPORTS) and swap its
implementation to run ESLint.lintFiles or ESLint.lintText against the mock
provider source and expect no results for that specific rule.
In `@docs/development/local-setup.md`:
- Line 111: Replace the concrete volume name "postgres-data" in the pgvector
image note with a generic phrase like "existing Postgres Docker volume/data
directory from the old image" to avoid implying a specific compose volume key;
keep the rest of the guidance about running `docker compose down -v` and `docker
compose up -d` against `pgvector/pgvector:pg18`, and optionally add a
parenthetical noting this repo uses the compose volume key `postgres_data` and
runtime names are project-prefixed to clarify why names may differ.
In `@openspec/changes/chatbot-rag-mvp/design.md`:
- Around line 95-109: Decision 14 (allowing caveated quantitative content after
the K=0 opener literal) conflicts with Decision 19 / test 10.4 (which fails K=0
responses that contain digit-bearing tokens near domain keywords); resolve by
choosing one of two fixes: either tighten Decision 14 to forbid any numeric
tokens in the K=0 path (amend Decision 14 language to remove "Soft guidance on
quantitative claims" and state numeric tokens are disallowed after the opener),
or relax Decision 19/test 10.4 to only flag fabricated citations/uncaveated
numeric claims (update test 10.4 so it ignores caveated numbers that include
qualifiers like "aproximadamente", "típicamente", or explicit source hedges and
only fails on invented citation patterns or unqualified digits). Ensure
references to K=0, the exact opener literal, Decision 14, Decision 19, and test
10.4 are updated accordingly so the spec is internally consistent.
In `@openspec/changes/chatbot-rag-mvp/specs/chatbot-llm-provider/spec.md`:
- Line 62: The spec text incorrectly shows Zod usage as
JSON.parse(...).safeParse(SearchKnowledgeArgsSchema); update the sentence to use
the schema's safeParse method instead, for example describe that the streaming
handler parses the JSON arguments and validates them with
SearchKnowledgeArgsSchema.safeParse(JSON.parse(LlmToolCall.arguments)), and
mention this occurs in the streaming handler code that parses
LlmToolCall.arguments (see SearchKnowledgeArgsSchema and the interface in
apps/api/src/features/chatbot/llmProvider/types.ts).
In `@openspec/changes/chatbot-rag-mvp/tasks.md`:
- Around line 31-32: The ESLint config instruction is contradictory: instead of
adding a second files: [...] block, update apps/api/eslint.config.ts to use a
single files block that lists both mock file paths and attach the existing rule
"chatbot/no-network-imports-in-mock" to that single block; locate the rule
definition referencing "chatbot/no-network-imports-in-mock" in the config and
merge the two target arrays into one files array that includes both
apps/api/src/features/chatbot/embeddingProvider/mock.ts and the other mock file
so the rule is declared once and applies to both files.
- Around line 49-50: The requirements conflict: Line 49's "SHALL use compatible
encoding/overestimate" for tokenizer conflicts with Line 24's requirement that
the shared estimateTokens helper is the single source of truth; reconcile by
updating the task wording or the helper contract. Edit the task text referencing
estimateTokens to either (a) mark the encoding/overestimation guidance as a
rationale/known limitation rather than a hard SHALL, or (b) update the
estimateTokens helper contract to explicitly promise compatibility with
text-embedding-3-large (cl100k_base) and safe overestimation; reference the
function name estimateTokens wherever the contract is asserted so both places
use the same normalized language.
---
Duplicate comments:
In `@openspec/changes/chatbot-rag-mvp/specs/chatbot-llm-provider/spec.md`:
- Around line 86-87: Update the spec text so the tool_call.arguments uses the
JSON-serialized string representation (not a parsed object) consistently across
contract and scenarios: change the assertion in the THEN clause for the emitted
tool_call (symbol: tool_call.arguments) to require arguments equal to a
JSON-serialized string containing the original user message verbatim, and keep
the rest of the behavior the same (iterable yields exactly one tool_call with
name = "searchKnowledge" and then completes without yielding any delta or
usage).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Lite
Run ID: 3a0edfcc-5c7c-41ce-a75b-a26533e9cafa
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (61)
apps/api/eslint.config.tsapps/api/package.jsonapps/api/scripts/chatbot/activateCorpusSource.tsapps/api/scripts/chatbot/chunking.tsapps/api/scripts/chatbot/ingestCorpus.tsapps/api/scripts/chatbot/parsePdf.tsapps/api/src/config/environment.tsapps/api/src/features/chatbot/embeddingProvider/azureOpenAI.tsapps/api/src/features/chatbot/embeddingProvider/index.tsapps/api/src/features/chatbot/embeddingProvider/mock.tsapps/api/src/features/chatbot/embeddingProvider/types.tsapps/api/src/features/chatbot/llmProvider/azureOpenAI.tsapps/api/src/features/chatbot/llmProvider/index.tsapps/api/src/features/chatbot/llmProvider/mock.tsapps/api/src/features/chatbot/llmProvider/types.tsapps/api/src/features/chatbot/prompts/es/system.mdapps/api/src/features/chatbot/prompts/loader.tsapps/api/src/features/chatbot/searchKnowledge/errors.tsapps/api/src/features/chatbot/searchKnowledge/index.tsapps/api/src/features/chatbot/searchKnowledge/searchKnowledge.tsapps/api/src/features/chatbot/searchKnowledge/types.tsapps/api/src/features/chatbot/sendMessage/handler.tsapps/api/src/features/chatbot/tools/searchKnowledge/execute.tsapps/api/src/features/chatbot/tools/searchKnowledge/index.tsapps/api/src/features/chatbot/tools/searchKnowledge/schema.tsapps/api/test/features/chatbot/embeddingProvider/unit.test.tsapps/api/test/features/chatbot/lint/noReferencesToCorpusTables.test.tsapps/api/test/features/chatbot/llmProvider/unit.test.tsapps/api/test/features/chatbot/prompts/loader.test.tsapps/api/test/features/chatbot/searchKnowledge/unit.test.tsapps/api/test/features/chatbot/sendMessage/integration.test.tsapps/api/test/setup/testcontainers.tsapps/api/tsconfig.eslint.jsonapps/api/vitest.config.tsapps/web/src/components/Chatbot/ChatbotWidget.tsxapps/web/src/components/Chatbot/MessageBubble.tsxapps/web/src/components/Chatbot/types.tsapps/web/src/components/Chatbot/useChatStream.tsdocker-compose.ymldocs/development/environment-variables.mddocs/development/local-setup.mddocs/operations/runbook.mdopenspec/changes/chatbot-rag-mvp/design.mdopenspec/changes/chatbot-rag-mvp/proposal.mdopenspec/changes/chatbot-rag-mvp/specs/chatbot-corpus-embeddings/spec.mdopenspec/changes/chatbot-rag-mvp/specs/chatbot-corpus-ingest/spec.mdopenspec/changes/chatbot-rag-mvp/specs/chatbot-corpus-retrieval/spec.mdopenspec/changes/chatbot-rag-mvp/specs/chatbot-corpus-schema/spec.mdopenspec/changes/chatbot-rag-mvp/specs/chatbot-llm-provider/spec.mdopenspec/changes/chatbot-rag-mvp/specs/chatbot-message-streaming/spec.mdopenspec/changes/chatbot-rag-mvp/specs/chatbot-widget/spec.mdopenspec/changes/chatbot-rag-mvp/tasks.mdpackages/database/docker-compose.ymlpackages/database/src/prisma/migrations/20260506220000_add_chatbot_embedding_and_pgvector/migration.sqlpackages/database/src/prisma/schema.prismapackages/types/src/chatbot/index.tspackages/types/src/chatbot/sendMessage/schemas.tspackages/types/src/chatbot/sourceCitation/index.tspackages/types/src/chatbot/sourceCitation/schemas.tspackages/types/src/chatbot/sourceCitation/types.tsturbo.json
💤 Files with no reviewable changes (1)
- apps/api/test/features/chatbot/lint/noReferencesToCorpusTables.test.ts
nicoalegria11
marked this pull request as draft
May 7, 2026 13:10
nivek0o0
force-pushed
the
feat/chatbot-foundation
branch
from
May 7, 2026 21:05
d2db6f8 to
f4a4284
Compare
nivek0o0
force-pushed
the
feat/chatbot-rag-mvp
branch
2 times, most recently
from
May 7, 2026 21:28
c40f2e9 to
dfd03c2
Compare
nivek0o0
force-pushed
the
feat/chatbot-foundation
branch
from
May 7, 2026 21:36
e5d29e4 to
f4a4284
Compare
Adds 5 chatbot tables (chat_conversation, chat_message, corpus_source, corpus_chunk, corpus_ingest_run) in the public schema with chatbot_ prefix. Adds ChatMessageRole and corpus enums. Migration declares CHECK constraint on conversation identity, indexes for session lookup and retention sweeps, and explicit ON DELETE SET NULL / CASCADE per the persistence spec.
Adds packages/types/src/chatbot/ with schemas for the streaming sendMessage request body and SSE event payloads (delta, done, error), plus empty schemas for the deleteMyConversation endpoint. Re-exports the chatbot domain from the package barrel.
…, and endpoints - Constants: token caps and TTL in apps/api/src/config/constants.ts. - Env: LLM_PROVIDER, COOKIE_SECRET, AZURE_OPENAI_* with boot-time guards. - Errors: ExternalServiceError (503), RequestTooLargeError (413). - Cookie plugin registering @fastify/cookie with signed parsing. - LLMProvider abstraction: types, shared estimateTokens helper, mock and azureOpenAI implementations, env-based selection cache. - ESLint: chatbot/no-network-imports-in-mock and chatbot/single-source-estimate-tokens custom rules wired inline. - Identity preHandler resolving currentUser then signed session cookie, minting a fresh UUID when requireIdentity is true; never 401s. - POST /api/chatbot/message streaming handler with advisory-lock-guarded lazy conversation creation, persistence, SSE wire format, mid-stream disconnect finalizer (idempotent via latency_ms IS NULL). - DELETE /api/chatbot/conversations/me idempotent handler that clears the session cookie for anonymous callers. - Routes mounted under /api/chatbot via autoload.
- SSE test helper using app.listen + real fetch with timeout/cleanup. - LLMProvider unit tests covering estimateTokens edge cases, mock determinism, abort handling, and a static no-network-imports check. - POST /api/chatbot/message integration tests: anonymous happy path, 413 oversized input, 400 malformed body, generic error constant. - DELETE /api/chatbot/conversations/me integration tests: no-identity 204, idempotency, cascade on conversation removal. - Lint tests enforcing dormant columns and corpus tables in foundation. - Vitest env adds LLM_PROVIDER=mock + COOKIE_SECRET for the suite.
- Custom widget under apps/web/src/components/Chatbot/ rendering a collapsed floating button + expandable chat panel. - useChatStream hook consumes POST /api/chatbot/message via raw fetch + ReadableStream, parses SSE frames inline, dispatches deltas/usage/error. - Two-strike fallback transitions to degraded state on consecutive transport failures during turn initiation; mid-turn disconnects mark the message truncated with no retry. - HTTP 413 / 503 error responses surface tone-appropriate Spanish copy. - Markdown rendering for assistant messages reuses the existing react-markdown + remark-math + rehype-katex setup. - Widget mounted in __root.tsx as minimum-viable placement. - Vite dev proxy forwards /api/* to VITE_API_BASE_URL so the widget's relative-URL fetches stay same-origin and carry the session cookie.
- docs/security/sensitive-data.md: chatbot persistence, identity scoping, 30-day TTL semantics, cookie security, right-to-be-forgotten flow. - docs/development/environment-variables.md: LLM_PROVIDER, COOKIE_SECRET, AZURE_OPENAI_*, plus a note on the same-origin Vite proxy. - docs/operations/runbook.md: pg_cron purge as a pending infra change, the SQL the future job will run, and cookie rotation guidance.
Closes 10.17, 10.18, 10.19, 10.20, 10.21, 10.25, 10.35, 10.36 after the three handler fixes (commits d71c639, 8b618c6, 95a106b) and the two test commits (0800cd5, 03ea26b) landed and the toolRound suite reports 16/16 green, full chatbot suite 39 green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds three ingest CLI integration tests plus a small test-only
mechanism on the mock embedding provider needed for the failure
scenario.
10.5 - re-ingest with new --version creates a new DRAFT alongside
an existing ACTIVE row. Asserts the seeded ACTIVE source is
untouched, the new DRAFT has the expected status, and chunks
attach only to the new row. Spec: chatbot-corpus-ingest
"Re-ingest with new version creates a new DRAFT alongside the
existing ACTIVE row" (design.md Decision 11).
10.6 - re-ingest collision with existing DRAFT fails fast with
non-zero exit and a Spanish error naming the offending source
id. Asserts no new source row, no chunks, no audit row (the
pre-flight collision check runs BEFORE the audit insert in the
CLI flow). Spec: chatbot-corpus-ingest "Re-ingest fail-fast on
(name, version, status=DRAFT) collision".
10.7 - audit row persists on embedding failure with NULL completed_at
and NULL source_id; no source row created for the attempt.
Spec: chatbot-corpus-ingest "Audit row persists on failure
with NULL completed_at".
Mechanism for 10.7: the ingest CLI runs in a subprocess via
execSync, so vi.spyOn from the test process cannot reach the mock
embedding provider's `embed` method. To force a failure, the mock
now honors the __TEST_FORCE_EMBEDDING_FAILURE env var, but only
when NODE_ENV=test. Both layers are required (test-only
mechanism + production-side guard) so an accidental env-var set
in staging is a no-op. Declared in turbo.json so the
no-undeclared-env-vars lint passes.
The mock embedding module is already test-only by contract: the
chatbot-corpus-embeddings spec's boot guard rejects
EMBEDDING_PROVIDER=mock when NODE_ENV=production. Adding a
test-only env var inside that module is on-pattern, not "test
code in production".
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Closes after commit 700d609 landed the three ingest CLI integration tests and the ingest suite reports 4/4 green, full chatbot suite 48 green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds three activate CLI integration tests covering the atomicity and
serialization invariants from chatbot-corpus-ingest spec and
design.md Decision 12.
10.8 - atomic flip: activating a DRAFT flips the prior ACTIVE for the
same (name, scope) to OUTDATED with deactivated_at populated,
and sets the target to ACTIVE with activated_at populated.
Both timestamps SHALL be byte-for-byte equal (single-instant
cutover semantics — the CLI captures one `new Date()` and
threads it through both UPDATEs inside the same $transaction).
Spec: "Activate CLI flips state atomically under an
identity-scoped advisory lock".
10.9 - advisory lock serializes concurrent activates of two distinct
DRAFTs sharing (name, scope). Fired in parallel via
Promise.allSettled on promisified exec (execSync would
serialize the subprocesses in the parent and not exercise the
race). Without the lock, both transactions would see "0 ACTIVE
rows" in their updateMany predicates and both would flip their
DRAFT to ACTIVE, leaving two ACTIVE rows for the same bucket.
With the lock, the second invocation unblocks AFTER the first
commits, OUTDATEs the first's just-committed ACTIVE row via
updateMany, and then promotes its own DRAFT. End state has
exactly one ACTIVE and one OUTDATED.
10.10 - refuses non-DRAFT target with explicit Spanish error and
exits non-zero. Asserts the CliArgumentError path leaves the
row untouched (activated_at and deactivated_at preserved
byte-for-byte). Spec rationale: reviving an OUTDATED row
without re-ingest would leave stale chunks under an ACTIVE
flag, exactly the failure mode the citation rule exists to
prevent (Decision 12 rationale).
Zero production-code bugs surfaced in the activate CLI during this
audit — the implementation honors the lock-key composition, the
single-instant cutover, and the non-DRAFT refusal exactly as the
spec describes.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Closes after commit 08b1b67 landed the three activate CLI integration tests and the activate suite reports 3/3 green, full chatbot suite 51 green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
….13, 10.32, 10.33)
Adds five integration tests covering searchKnowledge's filter and
validation surface.
10.11 - scope and sourceType filters restrict results to chunks
under matching sources; status=ACTIVE always enforced.
Seeds four ACTIVE sources across (GLOBAL/NATIONAL) x
(PDF/MD/URL) and exercises no-filter, single-axis, and
combined-filter (AND-composed) queries.
10.12 - empty / whitespace-only query throws InvalidQueryError
synchronously (no embedding or SQL side-effect).
10.13 - out-of-range topK (0 and 21) throws InvalidQueryError via
the range-check branch in validateTopK.
10.32 - oversized query (estimateTokens > 512, exercised at 513
tokens = 2049 chars) is rejected by validateQuery BEFORE
the embedding provider is invoked AND before SQL fires.
Verified via vi.spyOn on mockEmbeddingProvider.embed (the
cached singleton) + a stub-prisma whose $queryRaw is a
vi.fn() asserted not-to-have-been-called.
10.33 - non-integer topK (3.5) and out-of-range topK (-1) both
throw InvalidQueryError before SQL fires. Same spy/stub
pattern as 10.32.
Note on the stub-prisma pattern for 10.32 and 10.33: Prisma 7's
Proxy-based client surfaces $queryRaw via a get trap rather than
a direct property, so vi.spyOn(prisma, "$queryRaw") fails with
"Received undefined". Since searchKnowledge only touches the
passed-in prisma reference (no global DB state), passing a stub
client with $queryRaw = vi.fn() is functionally equivalent for
verifying "SQL was not executed".
Zero production-code bugs surfaced — searchKnowledge's validation
gates fire in the documented order (validateQuery →
validateTopK → embed → SQL), and both throw paths abort before
any side-effect.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
10.11, 10.12, 10.13, 10.32, 10.33 — added by commit bb68bb7. Suite searchKnowledge reports 6/6 green, full chatbot suite 56 green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…5, 10.16, 10.26, 10.27, 10.31)
Adds five (seven counting sub-cases) unit tests covering the
embedding provider's boot guard, factory selection, and Azure
batcher invariants.
10.15 - EMBEDDING_PROVIDER=mock + NODE_ENV=production throws at
environment.ts evaluation time. The test also sets
LLM_PROVIDER=azure-openai + AZURE_OPENAI_ENDPOINT +
AZURE_OPENAI_DEPLOYMENT_NAME + COOKIE_SECRET so the
embedding guard is reached (the LLM guard fires before
the embedding guard and would short-circuit otherwise).
10.16 - factory selection by EMBEDDING_PROVIDER (3 sub-cases):
mock returns mockEmbeddingProvider; azure-openai returns
azureOpenAIEmbeddingProvider; invalid value 'banana'
throws at boot.
10.26 - Azure batcher splits on 17+ inputs even when cumulative
tokens are small. Asserts >=2 SDK calls AND no SDK call's
input array exceeded the per-request size cap of 16.
Spec: chatbot-corpus-embeddings "Azure batcher splits on
the per-request array-size cap".
10.27 - Azure batcher splits on cumulative token threshold even
with fewer than 16 inputs. Ten inputs of 1000 estimated
tokens each = 10000 cumulative tokens; the internal
threshold (floor(8192 * 0.95) = 7782, 5% safety margin)
forces a split. Asserts >=2 SDK calls AND no call's
cumulative input tokens exceeded the 8192 hard cap.
10.31 - single oversized input (9000 estimated tokens > 8192
hard cap) rejected with InputTooLargeError whose message
names the offending index. Verifies ZERO SDK calls fired
— the per-input check pre-empts the SDK invocation.
Test mechanics: vi.hoisted shares the embeddingsCreate spy
between the vi.mock factory and the test bodies. The mocked
AzureOpenAI is a real class (not vi.fn().mockImplementation),
because Vitest 4 enforces [[Construct]] strictly and an
arrow-function impl cannot be invoked via `new`. @azure/identity
is also mocked defensively so the managed-identity branch cannot
reach Azure even if a future refactor flips the API-key path.
The factory/boot-guard tests use vi.resetModules + dynamic
imports so each test evaluates environment.ts and the embedding
modules with its own process.env override; ORIGINAL_ENV is
snapshotted at file load and restored after each test.
Zero production-code bugs surfaced — the boot guard, factory
selection, and batcher split logic all behave exactly as the
spec describes.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
10.15, 10.16, 10.26, 10.27, 10.31 — added by commit cdbb592. Suite embeddingProvider reports 14/14 green, full chatbot suite 63 green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ed web tests Opens a follow-up change to pick up the five web tests deferred from chatbot-rag-mvp: 10.22 - MessageBubble renders Fuentes consultadas panel 10.23 - MessageBubble does NOT render panel when sourcesCited absent/empty 10.28 - trash icon click clears local state only 10.34 - foot-of-chat disclaimer present in every state 10.38 - "Eliminar mi historial" link triggers D11 DELETE flow All five require a Vitest + React Testing Library + jsdom infrastructure in apps/web/ that does not exist today. Setting that up is a scope of its own (vitest.config, jsdom env, MUI theme provider wrapping, MSAL bypass, TanStack Router test helpers, fetch spy harness) and bundling it inside chatbot-rag-mvp would have ballooned that change. This is a minimal draft — only proposal.md plus a single spec delta under specs/chatbot-widget/ (required by openspec validate --strict). design.md and tasks.md will be added when implementation work picks up. Validated with: openspec validate chatbot-web-test-infra --strict. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ilures
12.2 verified for chatbot scope (63/63 passing). Full API suite has 5
pre-merge known failures in non-chatbot features caused by collation
divergence between alpine/musl and debian/glibc after the pgvector
image bump (task 1.1):
- carbonInventories/getCarbonInventoryMethodology
- countrySectors/getAllCountrySectors
- jobPositions/getAllJobPositions
- measurementUnits/getAllRateMeasurementUnits
- organizationMainActivities/getAllOrganizationMainActivities
Pending team coordination — see operator note. A working technical fix
(Intl.Collator('es', { ignorePunctuation: true })) was empirically
verified (149/149 files, 1292/1293 tests passing) but held back from
this PR pending the conversation.
12.3 (web MessageBubble citation-panel tests) deferred to the
chatbot-web-test-infra follow-up change: apps/web has no test runner
in V1 ("test": "echo 'Not implemented yet'"). The six widget tests
listed in section 10 (10.14, 10.22, 10.23, 10.28, 10.34, 10.38) are
formalized in openspec/changes/chatbot-web-test-infra/.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…dels
The Azure provider was passing `max_tokens` in the chat completion
request, which reasoning models (gpt-5 family, o-series) reject with
HTTP 400 ("Unsupported parameter: 'max_tokens' is not supported with
this model. Use 'max_completion_tokens' instead."). The deployment
targeted by this change (`huella-chat-v1` running gpt-5.4-mini) is a
reasoning model, so every first-round LLM call short-circuited before
any tool or streaming logic ran — the widget surfaced the generic
CHATBOT_GENERIC_ERROR_MESSAGE every time.
The fix is a one-line rename to `max_completion_tokens`. The parameter
is forward-compatible: non-reasoning chat models (gpt-4o, gpt-4.1)
accept it in Azure API versions >= 2024-08, and `max_tokens` is
deprecated in favor of it across the OpenAI/Azure API surface. The
mock provider is unaffected (it doesn't talk to the OpenAI SDK), so
the 63-test chatbot suite stays green.
Discovered during Bloque E smoke (Modo A K>=1) — the curl probes in
Paso 1 of the bloque-e-checklist used the correct param manually, but
the production code path did not. This is the gap Bloque E exists to
catch.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ducate-mode-full Records a Bloque-E pre-flight finding that the V1 widget never had a load-history-on-init feature. Verified empirically against feat/chatbot-foundation: foundation's useChatStream also initialized messages: [] with no useEffect, and the backend exposed only POST /api/chatbot/message and DELETE /api/chatbot/conversations/me — never a GET endpoint. The rag-mvp branch only *removed* backend coupling relative to foundation (deleteHistory → startNewConversation per Decision 25) and never had a load-history coupling to begin with. Conclusion: missing feature, not a regression. Backend persistence is intact — loadConversationHistory in sendMessage/service.ts injects prior turns into LLM context on every new turn, so coherence survives a refresh even though the UI starts visually empty. The follow-up chatbot-educate-mode-full change can expose GET /api/chatbot/conversations/me/messages and add the corresponding useEffect; the backend helper is ready to reuse behind the new route.
The Vite dev proxy uses Node's writable stream with 16KB default highWaterMark to pipe upstream responses. Small SSE frames (50-200 bytes) accumulate in the pipe buffer until the upstream stream ends, defeating delta-by-delta streaming for the chatbot widget in local dev. The backend already sets X-Accel-Buffering: no in SSE response headers, which production reverse proxies (nginx, Azure App Service, Front Door) honor. Vite dev proxy ignores it because it is not a real reverse proxy — it is a Node-based wrapper. Fix: register a proxyRes handler that calls res.flushHeaders() when the upstream content-type is text/event-stream. This commits response status+headers immediately and puts the socket into streaming mode, so subsequent piped writes flow without waiting for the buffer threshold. Non-streaming JSON responses are unaffected (gate on content-type). Production impact: none. This is dev-environment only.
Per Decision 17, the chatbot widget renders citations as inline Markdown links ([cite_label](cite_url)). The previous ExternalLink component rendered as a plain <a> tag, which inherits the bubble's text color and has no text-decoration, making citations visually indistinguishable from prose. Per Decision 14, the mandatory citation rule is the verifiability guarantee against hallucinations — invisible citations break that guarantee in practice even if the DOM contains them. Replace ExternalLink's plain <a> with MUI Link, applying color="primary" and underline="always" for unambiguous visual affordance during continuous reading. Preserves target="_blank" and rel="noopener noreferrer" for safety. The Link in the "Fuentes consultadas" panel (lines 112-119) is unaffected — different context, no reported issue.
The bot now redirects to its domain when it receives factual questions clearly outside the carbon-footprint and platform scope (e.g. "¿cuál es la población de Marte?"), instead of answering them like a generalist assistant. Aligns behavior with the V1 Educar product plan and the documented "Lo que el bot NO es" exclusion. Changes: - system.md: Modo C split into sub-modo C.1 off-domain redirect (byte-for-byte literal) and C.2 welcome/saludo (unchanged). Modo A enumeration broadened to explicitly cover specific products, services, sectors, and activities. - mock.ts: new off-domain branch with hardcoded fixtures (marte, 2+2, clima en santiago, mundial, messi, población), ordered after the tool-keyword check to keep Modo A routing intact. - spec.md: new "Modo C off-domain redirect" scenario parallel to Modo B; item 3 of the system-prompt requirement updated to describe the two C sub-modes. - tests: 10.36(c) sub-suite (3 phrasings) + loader literal assertion.
Prettier 3.6.2 reformats inline timeouts written multi-line in these pre-existing tests. Without this, CI format:check breaks.
…affordances PM confirmed V1 scope: the widget exposes only a "Nueva conversación" affordance (AddIcon, clear-local state + regen conversation_id), without user-facing delete-history UI. D11 right-to-be-forgotten is covered in V1 by the support-operated DELETE endpoint; the UI affordance ships in chatbot-educate-mode-full. Files updated: - chatbot-rag-mvp/specs/chatbot-widget/spec.md: rewrote the local-clear requirement for AddIcon + "Nueva conversación"; removed the deferred "Eliminar mi historial" requirement (and its empty MODIFIED header). - chatbot-rag-mvp/tasks.md: updated 9.5 literals; marked 9.7 as deferred with explicit reason; updated 10.28 to reflect the new affordance literal; marked 10.38 as deferred with the feature it tests; updated the 12.3 cross-reference note. - chatbot-rag-mvp/design.md: rewrote Decision 25 to document the single-affordance V1 model and the D11 V1 coverage frame (support-operated endpoint, user-facing UI deferred). - chatbot-rag-mvp/proposal.md: updated the foundation-fix bullet to reference "Nueva conversación"; added an explicit Deferred Debt entry for the deferred UI affordance. - chatbot-web-test-infra/proposal.md and specs/chatbot-widget/spec.md: shrunk scope from five tests to four (test 10.38 deferred with the affordance it tests); refreshed the 10.28 literal expectations. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Handler used to persist sources_cited whenever searchKnowledge returned chunks with valid cite_url, regardless of whether the model emitted the K=0 opener. This produced a contradictory UX: assistant says "no dispongo de fuentes verificadas" while the widget shows 8 sources. Now: when assistantBuffer.trimStart().startsWith(CHATBOT_K0_OPENER), finalSources is emptied in the common post-streaming path so both sources_cited (DB) and the SSE done payload reflect the K=0 semantics the assistant text declares. CHATBOT_K0_OPENER hoisted to apps/api/src/features/chatbot/constants.ts as the single source of truth — byte-for-byte mirror of the literal in prompts/es/system.md, shared by handler, mock, and tests. Spec scenario added; integration test covers the case where searchKnowledge returns 8 validated chunks but the stubbed provider emits the opener on round 2 (the Bloque E smoke E2E "scope 4" repro).
Two changes to reduce visual noise in assistant responses: 1. Inline citations: ReactMarkdown's `a` renderer is overridden to return null, so any `[label](url)` marker emitted by the model disappears from the rendered DOM — both the link and its label. The Markdown source text in `message.content` is preserved untouched; the suppression is render-only. This eliminates the "[Source][Source][Source]" repetition pattern common in V1 where a single corpus source (GHG Protocol) dominates citations. When the corpus diversifies in V2/V3, re-introducing inline citations requires only a renderer change, not a content-pipeline change. 2. Sources panel: entries are deduped by `cite_url` before render, and the chunk snippet (mid-PDF excerpt with no user-facing value) is no longer rendered. The panel shows only the clickable `cite_label`, one row per unique URL. Header count reflects unique URLs, not raw chunks. Backend persistence (`sources_cited` JSONB on the assistant row) is unchanged — `SourceCitationSchema` still requires `snippet`, and rows still carry one entry per validated chunk. The dedup and snippet suppression live entirely at render time. `ExternalLink` helper (apps/web/src/components/Chatbot/MessageBubble.tsx, introduced 3b22765 for the "force inline citations to be visible" bug fix) is removed — its only callsite was the markdown `a` component override that this commit replaces with a null renderer. The panel uses MUI `Link` directly with `variant="caption"` and keeps its own styling, distinct from the inline-prose styling ExternalLink encoded.
Adds GET /api/chatbot/conversations/me/current returning the user's active conversation (messages + metadata) when their chatbot_conversation_id cookie is set and the conversation is within its TTL window and the identity (session_id or user_id) matches the requester. The widget reads this cookie on mount and renders historical messages before allowing new input. The "Nueva conversación" affordance clears the cookie client-side in addition to resetting React state. Per PM decision, this lands in V1 to avoid surprising users who F5 and lose their thread. D11 retention is preserved at the endpoint level by filtering expired conversations regardless of whether pg_cron has run. V1 scope: anon → auth transition is NOT supported. If a user starts a conversation anonymously and then logs in, their cookie still points to the anon conversation but the endpoint rejects the match for user_id requests. Client falls back to a new conversation. Claim logic is deferred to a future change covering private data (V5 in the original plan). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Commit Z added setConversationCookie() but Set-Cookie never reached the browser. Real cause is not in the API or cookie helper — a new integration test against POST /api/chatbot/message confirms the API emits Content-Type text/event-stream, Cache-Control no-cache, X-Accel-Buffering: no, and Set-Cookie chatbot_conversation_id=... correctly. Direct curl against the running API confirmed the same. The regression was in 450de20 (Vite dev-proxy SSE fix). http-proxy-3 emits `proxyRes` BEFORE its `writeHeaders` pass copies upstream headers onto res, and that pass is gated on `!res.headersSent`. The sync flushHeaders() inside our proxyRes handler flipped headersSent to true before the gate, so the pass was skipped and the upstream SSE headers + Set-Cookie were never copied. Vite's own CORS middleware set Access-Control-Allow-Origin / Vary via setHeader earlier in the chain, which is why those two leaked through and made the symptom look chatbot-specific. Fix: defer flushHeaders to process.nextTick so the writeHeaders pass runs first. Production is unaffected (no Vite dev proxy). No changes to the SSE flow base or cookie helpers. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Verified the V1 RAG MVP end-to-end against a manually-provisioned Azure OpenAI deployment with a 5-page GHG Protocol Corporate Standard fair-use excerpt. All four scenarios from the task checklist passed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
nivek0o0
force-pushed
the
feat/chatbot-foundation
branch
from
July 9, 2026 16:54
907de3c to
9d6f543
Compare
nivek0o0
self-requested a review
July 10, 2026 01:46
21 tasks
Collaborator
|
Superseded by #592. Closing this in favour of that branch; keeping this PR and its Why it was re-applied rather than merged. This branch forked from the chatbot foundation while the foundation PR (#276) was still in review, so it never received the ~600 lines of review changes that landed on The result was a merge that git could not resolve meaningfully: 40 conflicted files, most of them add/add, because both sides had independently created the same foundation files. Resolving them file by file would have meant adjudicating ~1,000 lines where both sides descended from the same work, with a high chance of silently reverting merged PRs. What #592 does instead. It branches fresh from Beyond the reconciliation, that PR also resolves nine issues that would otherwise have merged in silently — a lint test asserting the corpus tables stay dormant, a test asserting TOOL messages are coerced to user messages, a route-gate spy missing a No need to reopen this. Follow-ups found along the way are tracked in #589, #590 and #591. |
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
3 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.
CONTEXT:
chatbot-rag-mvp-shipped.html
Summary
V1 minimum-viable RAG slice on top of
chatbot-foundation, end-to-end against real Azure OpenAI: the bot answers methodology questions with verifiable citations through foundation's SSE streaming endpoint. Lifts foundation's deferredpgvector+embeddingcolumn, adds theEmbeddingProviderabstraction, the ingest/activate CLIs, thesearchKnowledgeretrieval function, single-round server-side tool calling, a tri-mode system prompt (Modo A methodology / B platform / C conversational), and post-F5 conversation rehydration. Forward-compatible by design: identity is generic ("Asistente de Huella Latam") and the tri-mode scaffold ships from day one so V2 (measure) and V3 (platform guide) plug in without rewriting the prompt. Full OpenSpec change atopenspec/changes/chatbot-rag-mvp/— 3 new capabilities + 4 modified,openspec validate --strictpasses.What's included
sendMessageextended with single-round tool calling (peek first event → tool round defersreply.hijack()until the second round is committed; non-tool round hijacks immediately), K=0 override that emptiesfinalSourceswhen the assistant text starts with the canonical opener, newGET /api/chatbot/conversations/me/currentendpoint with strict identity match +expires_at > NOW()TTL filter,searchKnowledgeretrieval over pgvector with cosine distance,EmbeddingProviderfactory (mock+azureOpenAI),LLMProviderextended withtool_calldiscriminated event androle=TOOLmessages, tri-mode Spanish system prompt with i18n-ready loader.CREATE EXTENSION IF NOT EXISTS vector,embedding vector(1024)column onchatbot_corpus_chunk, HNSW index withvector_cosine_ops; lifts dormancy on the three corpus tables; Postgres bumped topgvector/pgvector:pg18in three docker-compose files. Forward-only additive migration — zero changes to existing tables.MessageBubblerenders a collapsible "Fuentes consultadas" panel withcite_url-dedup and inline-citation strip at render time (raw content preserved in DB);useChatStreamparsessourcesfrom thedoneSSE event and rehydrates persisted conversations on mount viaGET /me/current; "Nueva conversación" affordance separated from the deferred DELETE-history affordance per PM scope decision.pnpm --filter api chatbot:ingest(pdf-parse + header-aware chunker ~600 tokens / ~80 overlap + batched embeddings + DRAFT source + audit row) andpnpm --filter api chatbot:activate(advisory-lock single-transaction DRAFT→ACTIVE cutover that flips prior ACTIVE→OUTDATED atomically).chatbot/no-network-imports-in-mockESLint rule extended to a second file scope onembeddingProvider/mock.ts;CHATBOT_K0_OPENERconstant mirrors the system prompt literal byte-for-byte (single source of truth for K=0 detection at the handler);mockprovider rejected at boot in production for both LLM and embedding factories;searchKnowledgetool arguments treated as untrusted input (Zod-validated, malformed JSON routes through K=0 fallback instead of throwing).Out of scope (V1 — see
proposal.mdDeferred Debt)pnpm test:evalgate — deferred tochatbot-educate-mode-full. The 4 CRITICAL tests (10.1–10.4) gate structural invariants (recall on synthetic embeddings, single-round tool flow, K=0 fallback, atomic ingest), not response quality. A meaningful eval gate requires the operator-supplied PDF + 10–15 goldens written by a domain expert per country (country-agnosticism: what passes for Chile may not pass for Argentina); shipping a gate without those inputs asserts noise.pg_cronpurge job for the 30-day conversation TTL — still deferred from foundation. Compensated in V1 byGET /me/currentfilteringexpires_at > NOW()in the query, so expired rows become invisible even before physical purge.chatbot-educate-mode-fullper PM. BackendDELETE /api/chatbot/conversations/mealready exists from foundation and is operable by support staff for D11 right-to-be-forgotten in V1 (Ley 21.719 / LGPD / GDPR met via support intake).chatbot-private-data). V1 returns 404 + fresh thread when the caller is authenticated and theirchatbot_conversation_idcookie points to an anonymous row.getEmissionFactors, …) — V2.Non-obvious patterns to focus on during review
Six places where a future change could silently regress a load-bearing invariant:
apps/api/src/features/chatbot/sendMessage/handler.ts:503— K=0 guard:if (assistantBuffer.trimStart().startsWith(CHATBOT_K0_OPENER)) { finalSources.length = 0; }. Applied in the common path post-streaming, pre-UPDATE, so it covers both the tool round and the non-tool round. Removing it re-introduces the DB↔UI mismatch surfaced during verification (panel showed 8 entries while the bot said "no verified sources").apps/api/src/features/chatbot/constants.ts:15-16—CHATBOT_K0_OPENERis the canonical literal, a byte-for-byte mirror of the opener inprompts/es/system.md. Both the mock provider and the handler's K=0 guard import it. Drift between the three locations is silent: tests stay green while production behavior diverges.apps/api/src/features/chatbot/sendMessage/handler.ts:149—setConversationCookie(reply, conversationId.toString())runs after the$transactioncommit and beforereply.hijack(). Once hijacked, Fastify no longer flushes its accumulated headers;writeSseHeadershas to forward them explicitly. Setting the cookie post-hijack breaks F5 rehydration silently in production.apps/api/src/features/chatbot/getCurrentConversation/service.ts:38-48—findCurrentConversationreusesconversationIdentityFilterfromsendMessage/service.ts(strict match: authenticated →user_id = ? AND session_id IS NULL; anonymous → dual) plus a TTL filter. Relaxing the identity match toORopens an IDOR window between users whose expired conversations could be recycled.apps/web/src/components/Chatbot/MessageBubble.tsx:79-82—components={{ a: () => null }}strips inline citations at render time only; raw content is preserved in the DB. This is the lever V2/V3 will pull when multi-corpus makes inline citations relevant again — reverting then requires a spec + render change, not a DB shape change.apps/web/vite.config.ts:84-101—process.nextTickdefer onflushHeaders()for SSE responses.http-proxy-3skips itswriteHeadersupstream pass whenres.headersSent === trueat the gate; a synchronousflushHeaders()flips that gate before the pass. Removing the defer re-introduces a dev-only bug whereSet-Cookie,Content-Type: text/event-stream,Cache-Control, andX-Accel-Bufferingnever reach the browser in dev — production unaffected, but all empirical end-to-end validation breaks.Self-review
searchKnowledgeOUTDATED/DRAFT exclusion, 10.2 ingest happy path, 10.3 single-round tool calling end-to-end, 10.4 K=0 fallback all-sources-filtered). Full API suite passes except for 2 preexisting collation failures (carbonInventories/getCarbonInventorySubcategoriesSummary,measurementUnits/getAllRateMeasurementUnits);git log feat/chatbot-foundation..HEAD --confirms this branch does not touch them.localhost:5173against real Azure OpenAI, verified with real response headers, DB queries, and screenshots — Modo A K≥1, Modo A K=0, Modo B redirect, Modo C.1 off-domain, Modo C.2 welcome, F5 restores a conversation with 10+ messages, "Nueva conversación" clears the cookie. TTFT 1.7–5.6s in dev (expect 30–50% improvement behind prod CDN). Deferred validation: authenticated path (covered by integration tests; smoke was anonymous),DELETE /mefrom the widget (UI affordance deferred per PM; endpoint stays operable via curl/support), formal concurrency stress onfindCurrentConversation(foundation-grade advisory lock covers persistence; the new endpoint is read-only with no write race).sources_citedcontradicted the assistant text (panel showed 8 entries while the bot said "no verified sources"). Fix: post-streaming pre-UPDATEguard emptiesfinalSourceswhen the buffer starts withCHATBOT_K0_OPENER(17a31533).cite_urlper message plus an undeduplicated panel. Fix: strip inline links at render time (preserves raw content in DB) + dedup the panel bycite_url+ hide chunk snippets (2a640f49).chatbot_conversation_idcookie +GET /api/chatbot/conversations/me/currentendpoint with strict identity match + TTL filter;useChatStreamrehydrates on mount with ahistoryLoadingflag (51cf3317).http-proxy-3skipped itswriteHeadersupstream pass because synchronousflushHeaders()flippedres.headersSentbefore the gate. Dev-only impact, but it blocked every empirical end-to-end check of the integration. Fix: deferflushHeaders()toprocess.nextTick(450de204→57e3d2d9).design.mddecisions — single-round tool invariant (second consecutivetool_callaborts withExternalServiceError→ 503), deferredreply.hijack()until after the second-round outcome is known,tokens_usedsourced from the SECONDusageevent in tool turns (not summed across rounds),sources_citedZod-validated before persistence (malformedcite_urlfiltered out), tool args treated as untrusted input (malformed JSON routes through K=0 fallback rather than throwing), tri-mode prompt ships generic identity ("Asistente de Huella Latam") forward-compatible with V2/V3. All match.Known follow-ups (separate PRs, not blocking)
chatbot_session_idduplicated inSet-Cookie. Cosmetic — browser dedupes silently. Foundation's identity preHandler andsendMessage's pre-stream pass both emit it; consolidate to one.chatbot_session_id. It is regenerated on demand bychatbotIdentityPreHandler, so no functional impact, but the side-effect is broader than the affordance name suggests.useChatStream. Density rose with this PR (historyLoading+ initial fetch + retry + EOF flush). The draft changeopenspec/changes/chatbot-web-test-infra/opens the conversation about a frontend testing convention before backfilling.Test plan
git checkout feat/chatbot-rag-mvp && pnpm installapplies cleanly.pnpm format:check && pnpm lint && pnpm type-checkall pass.pnpm test --filter=api -- chatbot --coverage=false→ 77/77 pass.pnpm openspec validate chatbot-rag-mvp --strictpasses.docker compose up -dbrings uppgvector/pgvector:pg18;pnpm --filter database migrate:devapplies the additive migration (vectorextension +embedding vector(1024)column + HNSW index).pnpm --filter api chatbot:ingest --file apps/api/test/fixtures/chatbot/ghg-protocol-sample.pdf --name "GHG Protocol" --version "1.0"produces a DRAFT source and audit row.pnpm --filter api chatbot:activate --source-id <id>flips DRAFT→ACTIVE atomically.pnpm dev+ openhttp://localhost:5173; dogfood the widget:truncated = falseexcept on explicit disconnect;sources_cited = []when the K=0 opener is incontent.