Conversation
- Bump all pyproject.toml and __version__ to 0.4.0-dev - BACKLOG: mark FEAT-003, BUG-010, BUG-011, DEBT-002/003/008 as completed (v0.3.0) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Bump all component versions to 0.4.0-dev - Mark Sprint 1 items completed in BACKLOG: FEAT-003, BUG-010, BUG-011, DEBT-002/003/008 Reviews: 4/4 addressed (won't fix: pre-existing, out of scope) Tests: 576 passed
- Health check on startup via GET /health (5s timeout) - "unavailable" banner with disabled input when API unreachable - Auto-retry every 30s with "reconnecting" status - Banner clears and input re-enables when connection is restored - I18N: en/it for unavailable and reconnecting messages
- Add _learn_sse_generator and StreamingResponse support in learn API - When stream=true, use pipeline.execute_stream() instead of execute() - Widget: change stream flag from false to true - Widget: detect no_relevant_context in SSE (no tokens before done) - Widget: handle plain-text token events alongside JSON events
Minimal built-in parser (~2KB) covering bold, italic, inline code, code blocks, links, headings, and ordered/unordered lists. - HTML entities escaped before parsing (XSS prevention) - Streaming: re-renders full markdown on each token (simple, reliable) - Non-streaming: renders complete message in one pass - CSS styles for code, lists, headings, links in chat bubbles - User messages remain plain text - BACKLOG updated with third-party library alternatives
- Detect 401 responses and attempt token refresh before showing error
- Two refresh mechanisms: onTokenExpired callback or data-token-refresh-url
- Callback-based: host system provides async function returning new token
- URL-based: POST to configured endpoint, expects {token} response
- Single retry with _refreshing guard to prevent loops
- I18N: en/it "session expired" message when refresh fails
- data-token-refresh-url attribute read from script tag
- Remove duplicate JSDoc block in api-client.js - Add data-token-refresh-url to usage example in index.js - Sanitize link URLs: only allow http/https (prevent javascript: XSS) - Disable input on sessionExpired status (was only disabled for unavailable) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Root cause: SSE generator sent tokens as plain text (data: word),
which broke newlines (\n becomes empty SSE line, widget trim() ate it).
Fix: wrap all tokens as JSON ({"type":"token","data":"..."}) in the
learn SSE generator. Remove plain-text token handler from widget.
Remove trim() on payload to preserve whitespace in JSON strings.
Lines between <pre> and </pre> were getting wrapped in <p> tags because the parser only checked if a line contained the tag markers, missing intermediate lines. Now tracks inPre state across lines. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The widget never received conversation_id in streaming mode because the pipeline emits done with empty data and the learn SSE generator sent only [DONE]. Now sends a JSON done event with conversation_id before [DONE], so the widget can track multi-turn conversations. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Escape double quotes in markdown link href to prevent XSS (CRITICAL) - Group consecutive text lines into single <p> instead of per-line wrapping - Prevent infinite token refresh loop with single-retry guard - Preserve HTTP 401 in error message for session expiry detection - Respect _sending state in setConnectionStatus to prevent premature re-enable - Add h6 to assistant message heading CSS selector Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add "sessionExpired" to setConnectionStatus JSDoc union type - Fix heading range in markdown.js docstring (h3-h6, not h3-h4) - Add noreferrer to rel attribute on external links for privacy Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ion status Persist connection status in this._status and derive disabled from both _sending and _status via a single _updateControlsDisabled method. Prevents doneSending from re-enabling controls during unavailable or sessionExpired states. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- SSE streaming for learn query endpoint (FEAT-010) - Markdown rendering for assistant messages (FEAT-007) - Token auto-refresh on JWT expiry (FEAT-009) - API connectivity check with error feedback (FEAT-006) - XSS fix, retry guard, centralized disabled state from review Reviews: 17/17 addressed (Gemini 6, CodeRabbit 11) Tests: 576 passed Refs: FEAT-006, FEAT-007, FEAT-009, FEAT-010
…sion
- Use XML tags (<context><source>) for retrieved chunks instead of plain
numbered markers, following Anthropic's recommended prompt structure
- Convert conversation history from text labels ("User:"/"Assistant:")
inside a single role="user" message to native role-based messages in
the API messages array
- Add explicit instructions in system prompt explaining that <context>
content is reference material from the knowledge base, not user input
These changes address a reported issue where the LLM confused RAG source
chunks with user messages in multi-turn conversations.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add system prompt rules to prevent the model from mentioning sources, passages, or retrieval mechanics to the user. Also instruct it to handle truncated source passages gracefully without commenting on the truncation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…05, INFRA-006) - BUG-013: wire trace persistence to query_traces table - DEBT-009: debug logging for rewritten queries - INFRA-005: Docker log persistence across restarts - INFRA-006: Loki + Grafana monitoring stack Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Restructured context.j2 template with XML delimiters to separate chunks from user query - Added system prompt instruction to never expose retrieval internals to users - Applied boundary markers in both simple and advanced pipeline prompt construction - Added backlog entries for observability gaps (BUG-013, DEBT-009, INFRA-005, INFRA-006) Reviews: 0/0 actionable (Gemini summary only) Tests: 576 passed locally, CI green Refs: BUG-012
The PersistentConversationStore was initialized but never registered
in the ProviderRegistry, causing GET /api/v1/conversations/{id} to
return 503 "not configured" even when VEKTRA_CONVERSATION_KEY was set.
Part of BUG-014.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Tracks the missing create_conversation() call that causes PersistentConversationStore to silently discard all turns. Annotates BUG-010 as partially incomplete. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Require consulting docs/reference/api.md or /openapi.json before constructing any API call to prevent auth, parameter, and endpoint mistakes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
PersistentConversationStore.add_turn() requires an existing ConversationOrm row, but no one was creating it. All turns were silently discarded and multi-turn context was broken. Changes: - Add ensure_conversation() to PersistentConversationStore (upsert that creates the row only if it doesn't exist) - Extend create_conversation() to accept an optional conversation_id - Core API: create conversation before calling pipeline, using namespace_id and key_id from the auth layer - Learn API: ensure conversation exists using sentinel key_id (JWT auth has no API key) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The system prompt told the LLM not to reference sources, but the instruction was in English only and too generic. The model would use translated equivalents like "brani forniti" in Italian, which confuses users who don't know a retrieval system exists. Changes to system.j2: - Explicit blacklist of forbidden expressions in both English and Italian (extensible to other languages) - Reframe assistant persona as "knowledgeable" rather than "with access to a knowledge base" - Provide concrete alternative phrasings for partial-knowledge scenarios - Structure instructions with clear sections for context handling and user communication Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Previous prompt still allowed the LLM to say "the text I consulted" and offer to "search for more" - both expose the retrieval mechanism or make promises the system cannot keep. Changes: - Blacklist "consulted/reviewed" phrasing in addition to "provided" - Explicitly forbid offering to search, look up, or provide more information later (the system has no such capability) - Instruct to state incomplete knowledge plainly without hinting that more information exists elsewhere Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The previous structured prompt was too verbose for gpt-5-mini, which ignored the blacklist and used "brani forniti" anyway. The word "passage" in the prompt itself was being translated and echoed back. Rewritten as a compact prompt with numbered critical rules. Removed all words the model could recycle (passage, document, source). Added "brani" to the forbidden list since gpt-5-mini translates "passage" to this Italian word. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Rewritten based on research findings (see vektra-internal
20260324-rag-prompt-best-practices-research.md):
- 4 rules only (within compliance threshold for smaller models)
- Positive framing ("Sound like you simply know") instead of blacklists
- Reintroduce <source> clarification to prevent BUG-012 regression
- Add language-matching instruction (rule 4)
- Remove forbidden phrase lists (brittle, secondary defense)
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
VEKTRA_LLM_API_KEY existed in config but was never passed to litellm, forcing users to set provider-specific env vars (OPENAI_API_KEY) even for OpenAI-compatible endpoints like vLLM. Changes: - LitellmProvider now passes config.api_key to litellm as api_key kwarg - Add VEKTRA_LLM_EXTRA_BODY config field (JSON dict) for provider- specific params (e.g., chat_template_kwargs to disable thinking on Qwen3.5/DeepSeek-R1 via vLLM) - Update configuration.md with vLLM setup instructions and correct VEKTRA_LLM_API_KEY documentation Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Move structlog import to top level (was inline in except block) - Extract _LEARN_SENTINEL_KEY_ID as module-level constant - Fix import ordering (ruff I001) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…l quality BUG-015: reranker scores discarded (threshold applied to wrong scores) BUG-016: English-only reranker on Italian content TECH-002: RAG evaluation harness (50-question dataset, RAGAS) DEBT-010: recalibrate relevance threshold with empirical data All traced to analysis in vektra-internal/stack/20260324-reranker-threshold-gap-analysis.md Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ckfile Pin litellm <1.82.7 (versions 1.82.7-1.82.8 contain credential stealer). Replace direct uv pip install of qdrant-client/fastembed with lockfile-based --extra sparse --extra qdrant. Drop unused torchvision. All dependencies now verified against lockfile hashes via uv sync --frozen. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
component:core
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds grounding-mode resolution, trace persistence, conversation response_id plumbing and admin turns endpoint; introduces eval retrieval and end‑to‑end harnesses and dataset; changes reranker return shape and templates; adds SSE support and widget token/health handling; updates config defaults, LLM fallbacks, many tests, CI, Docker, Make targets, and package versions. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client / Widget
participant API as Query API
participant Pipeline as Query Pipeline
participant Reranker as Reranker Service
participant LLM as LLM Provider
participant Analytics as Analytics Service
participant DB as Database
Client->>API: POST /api/v1/query (or stream)
API->>API: resolve_grounding_mode(namespace)
API->>DB: SELECT config FROM namespaces
DB-->>API: grounding_mode (or none)
API->>Pipeline: execute_stream / execute(QueryRequest{grounding_mode,...})
Pipeline->>Pipeline: _history_to_messages(), retrieve chunks
Pipeline->>Reranker: rerank(candidates)
Reranker-->>Pipeline: RerankResult{top_k, all_scores}
alt run LLM (has_context or grounding_mode == "hybrid")
Pipeline->>LLM: complete(prompt)
LLM-->>Pipeline: CompletionChunk(model, token)
else skip LLM (strict & no_context or safeguard_blocked)
Pipeline-->>API: indicate no_relevant_context / blocked
end
Pipeline->>Analytics: store_trace(trace) (best-effort)
Analytics->>DB: INSERT trace
API-->>Client: SSE stream / final response {tokens, sources, trace}
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
Warning Gemini is experiencing higher than usual traffic and was unable to create the review. Please try again in a few hours by commenting |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
vektra-core/src/vektra_core/advanced_pipeline.py (1)
375-404:⚠️ Potential issue | 🔴 CriticalPreserve a hard block when
post_retrievalreturnsallowed=False.This path collapses “safeguard denied” into ordinary “no relevant context”. In hybrid mode,
execute()and_stream()then continue and can still call the LLM with zero context, which bypasses the safeguard’s explicit deny decision.SimpleQueryPipelinealready carries a separatesafeguard_blockedflag; this flow needs the same.💡 Fix shape
) -> tuple[ list[StepTrace], list[SearchResult], bool, + bool, str, list[dict[str, str | None]], ]: @@ + safeguard_blocked = False if filtered: @@ if not sg_result.allowed: filtered = [] + safeguard_blocked = True elif sg_result.filtered_ids: @@ - return steps, filtered, no_relevant_context, effective_query, history + return ( + steps, + filtered, + no_relevant_context, + safeguard_blocked, + effective_query, + history, + )( steps, filtered, no_relevant_context, + safeguard_blocked, _effective_query, history, ) = await self._run_pre_llm_steps(query) - if (no_relevant_context or not filtered) and query.grounding_mode != "hybrid": + if safeguard_blocked or ( + no_relevant_context and query.grounding_mode != "hybrid" + ):As per coding guidelines,
vektra-core/**changes must preserveSafeguardHookintegration.Also applies to: 494-517, 648-662
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vektra-core/src/vektra_core/advanced_pipeline.py` around lines 375 - 404, When post_retrieval_safeguard returns sg_result.allowed == False you must preserve a hard block instead of collapsing it into "no relevant context": set the pipeline's safeguard_blocked flag (the same flag used by SimpleQueryPipeline, e.g., self.safeguard_blocked or the pipeline/context object) when sg_result.allowed is False, keep filtered empty, and ensure StepTrace still records the denial; do the same change in the other equivalent blocks (the ones around the other post_retrieval_safeguard calls referenced in the comment) and keep SafeguardHook integration intact so execute() and _stream() can short-circuit based on safeguard_blocked rather than proceeding with zero context.vektra-shared/src/vektra_shared/config.py (2)
188-206:⚠️ Potential issue | 🟠 MajorAdd field constraints to
QueryPipelineConfignumeric fields.
grounding_modeis validated via@field_validator, butmin_relevance_scoreandcontext_chunk_ratioremain unconstrained. Direct instantiation ofQueryPipelineConfigcan accept out-of-range values that would fail inVektraSettings, bypassing validation and pushing invalid settings into retrieval filtering or token budgeting. Constraints must be enforced at the sub-config level to align with independent validation per the architectural pattern.Proposed fix
min_relevance_score: float = Field( 0.15, + ge=0.0, + le=1.0, alias="VEKTRA_MIN_RELEVANCE_SCORE", description="Minimum relevance score for chunk inclusion (ARCH-056). Safety net filter; top-k is the primary control.", )context_chunk_ratio: float = Field( 0.6, + gt=0.0, + lt=1.0, alias="VEKTRA_CONTEXT_CHUNK_RATIO", description="Fraction of context window allocated to retrieved chunks (ARCH-055).", )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vektra-shared/src/vektra_shared/config.py` around lines 188 - 206, QueryPipelineConfig's numeric fields lack range constraints so invalid values can be instantiated directly; add validation constraints to the Field declarations: set min_relevance_score = Field(0.15, alias="VEKTRA_MIN_RELEVANCE_SCORE", ge=0.0, le=1.0, ...), context_chunk_ratio = Field(0.6, alias="VEKTRA_CONTEXT_CHUNK_RATIO", ge=0.0, le=1.0, ...), and tighten response_token_reserve = Field(2048, alias="VEKTRA_RESPONSE_TOKEN_RESERVE", ge=0) (or ge=1 if zero is invalid); leave grounding_mode's `@field_validator` in place — use these ge/le constraints on the existing Field calls (min_relevance_score, context_chunk_ratio, response_token_reserve) to enforce sub-config level validation.
539-572:⚠️ Potential issue | 🟠 MajorAdd production guard for
VEKTRA_EVAL_MODEin settings validation.Documentation states eval_mode is "staging only", but the configuration class does not enforce this. When enabled, eval_mode captures raw queries and full prompt messages into
StepTrace.metadata, which is stored verbatim by analytics. A misconfiguration could put user content into the production database.Add a model validator to reject
eval_mode=Truewhenenv="production":Proposed fix
`@field_validator`("prompt_grounding_mode") `@classmethod` def validate_prompt_grounding_mode(cls, v: str) -> str: if v not in ("strict", "hybrid"): raise ValueError( f"prompt_grounding_mode must be 'strict' or 'hybrid', got '{v}'" ) return v + + `@model_validator`(mode="after") + def validate_sensitive_modes(self) -> VektraSettings: + if self.env == "production" and self.eval_mode: + raise ValueError("VEKTRA_EVAL_MODE must be disabled in production") + return self🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vektra-shared/src/vektra_shared/config.py` around lines 539 - 572, Add a model-level validator (e.g., validate_eval_mode_production) to the settings class that checks os.environ.get("ENV") (or os.getenv) and rejects eval_mode=True when the environment equals "production" by raising ValueError; import os, annotate with `@model_validator` (after validation) and place it alongside the existing field validators (referencing eval_mode and model_config/SettingsConfigDict) so misconfiguration is blocked at model instantiation.
🧹 Nitpick comments (6)
Dockerfile (1)
73-76: Move optional extras to the manifest-only sync layer for better cache efficiency.The
sparse,qdrant, andocrextras are defined entirely as third-party dependencies (fastembed, qdrant-client, unstructured[pdf]) in the workspace package manifests. Since these manifests are already copied before the source code at line 46, they can be installed in the earlier manifest-onlyuv syncto prevent source-only changes from invalidating that cached layer. Remove them from the conditional sync at lines 73-76 and keep that layer focused solely on building and installing workspace packages.Suggested refactor
RUN if [ "$INSTALL_UNSTRUCTURED" = "true" ]; then \ + uv sync --frozen --no-install-workspace --no-dev --extra sparse --extra qdrant --extra ocr; \ + else \ + uv sync --frozen --no-install-workspace --no-dev --extra sparse --extra qdrant; \ + fiThen at lines 73-76, remove the extras:
RUN if [ "$INSTALL_UNSTRUCTURED" = "true" ]; then \ - uv sync --frozen --no-editable --no-dev --extra sparse --extra qdrant --extra ocr; \ + uv sync --frozen --no-editable --no-dev; \ else \ - uv sync --frozen --no-editable --no-dev --extra sparse --extra qdrant; \ + uv sync --frozen --no-editable --no-dev; \ fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` around lines 73 - 76, The conditional uv sync block referencing INSTALL_UNSTRUCTURED should not include third-party extras (sparse, qdrant, ocr); remove the flags "--extra sparse --extra qdrant --extra ocr" from the uv sync inside the INSTALL_UNSTRUCTURED conditional (the layer that builds workspace packages) and instead add those extras to the earlier manifest-only uv sync invocation that runs after copying only the workspace manifests (so the third-party deps are installed in the manifest-only layer and benefit from caching); keep the conditional only for installing unstructured-specific runtime bits if needed and leave workspace package build/install logic in the existing uv sync (functionally the INSTALL_UNSTRUCTURED conditional should only control whether source-specific unstructured pieces are installed, not the third-party extras).vektra-core/tests/test_conversation.py (1)
88-90: Consider asserting the stored answer too for fuller regression coverage.A quick
history[0]["answer"] == "A1"check would make this test more complete.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vektra-core/tests/test_conversation.py` around lines 88 - 90, The test currently checks stored history length and the question but not the stored answer; update the assertion in the test (around the history retrieval using store.get_history and variable cid in test_conversation.py) to also assert history[0]["answer"] == "A1" so the stored answer is validated alongside the question.vektra-shared/src/vektra_shared/types.py (1)
262-274: Makegrounding_modea real shared type.
QueryRequestnow carriesgrounding_modeacross package boundaries, but a typo like"hybird"still type-checks and silently falls into the non-hybrid branch downstream. Sincevektra-shareddefines the contract, encode the allowed values here with an enum orLiteral, the same waySearchModeis modeled.♻️ Suggested shape
- grounding_mode: str = "strict" # "strict" | "hybrid" (FEAT-020) + grounding_mode: GroundingMode = GroundingMode.STRICTclass GroundingMode(str, Enum): STRICT = "strict" HYBRID = "hybrid"As per coding guidelines,
vektra-shared/**: Check for: protocol violations, backward compatibility of types and config.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vektra-shared/src/vektra_shared/types.py` around lines 262 - 274, QueryRequest currently uses a plain string for grounding_mode which allows typos; define a shared enum GroundingMode (e.g., class GroundingMode(str, Enum) with values "strict" and "hybrid") in vektra_shared.types and change the QueryRequest grounding_mode type to GroundingMode with default GroundingMode.STRICT, updating any imports/usages and preserving backward-compatibility by accepting GroundingMode values where QueryRequest is constructed.vektra-admin/tests/test_admin_turns.py (1)
12-90: Cover audit logging on this authenticated endpoint.These tests validate retrieval and error branches, but they never assert that the admin request is audited. Since this route is authenticated, please add audit-log expectations for the success and failure paths so the compliance requirement stays protected.
As per coding guidelines,
vektra-admin/**: Check for: audit log completeness (all authenticated requests must be logged).vektra-core/tests/test_pipeline.py (1)
539-559: Consider test isolation for module-level warning sets.The tests modify module-level
_token_count_fallback_warnedand_context_window_fallback_warnedsets viadiscard(). If tests run in parallel or in unexpected order, state could leak. Consider using a fixture to reset these sets.Optional: Add fixture for test isolation
`@pytest.fixture`(autouse=True) def reset_fallback_warnings(): """Reset warning sets before each test.""" _context_window_fallback_warned.clear() _token_count_fallback_warned.clear() yield _context_window_fallback_warned.clear() _token_count_fallback_warned.clear()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vektra-core/tests/test_pipeline.py` around lines 539 - 559, The tests mutate module-level sets _token_count_fallback_warned and _context_window_fallback_warned which can leak state between tests; add an autouse fixture that clears both sets before and after each test to guarantee isolation (reference the fixture to affect tests like test_count_tokens_fallback_warns and test_count_tokens_fallback_warns_once and any other tests touching those sets) so each test starts with empty sets and no inter-test ordering or parallelism issues.vektra-shared/src/vektra_shared/namespace.py (1)
39-42: Consider adding validation for thedefault_modeparameter.The function validates
ns_modefrom the database against_VALID_GROUNDING_MODES, butdefault_modeis used directly without validation. If an invaliddefault_modeis passed, it would be returned.Optional: Validate default_mode parameter
async def resolve_grounding_mode( namespace: str, session_factory: Any, default_mode: str = "strict", ) -> str: + if default_mode not in _VALID_GROUNDING_MODES: + default_mode = "strict" """Resolve grounding_mode: namespace JSONB config > default.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vektra-shared/src/vektra_shared/namespace.py` around lines 39 - 42, The code validates ns_mode against _VALID_GROUNDING_MODES but returns default_mode unvalidated; update the logic so default_mode is checked as well: before returning default_mode, verify it is in _VALID_GROUNDING_MODES (using something like `if default_mode in _VALID_GROUNDING_MODES: return str(default_mode)`), and if it's not valid either raise a ValueError or fall back to a safe known mode; ensure you reference the same symbols (row, ns_mode, default_mode, _VALID_GROUNDING_MODES) when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/CLAUDE.md:
- Around line 17-20: The guidance currently tells operators to paste
VEKTRA_CONVERSATION_KEY into a SELECT with pgp_sym_decrypt on
conversation_turns, which risks leaking secrets; update the docs to show using a
psql variable or env-backed flow (e.g., instruct to set a psql variable via \set
from an environment variable) or to use the admin/decryption endpoint to view
plaintext instead of embedding the key in the SQL, and mention the exact symbols
to change: replace direct use of VEKTRA_CONVERSATION_KEY in the
pgp_sym_decrypt(...) example and the conversation_turns SELECT example with the
safer \set/env approach or a pointer to the admin endpoint that performs
decryption.
In `@scripts/eval_e2e.py`:
- Around line 108-153: The summary currently counts any non-None r.answer as
success in print_summary, which reports presence not correctness; update
print_summary to use expected answer/refusal fields from the dataset (e.g.,
r.expected_answer and r.expected_refusal) and compute a true quality metric
(accuracy or refusal-correct) by comparing r.answer/r.refusal to those fields,
include counts/percentages for correct answers and correct refusals and show
them alongside/replace the current "Answered" line; if dataset lacks those
fields, either add expected_answer/expected_refusal to dataset records and
handle missing values (fall back to smoke-only reporting) or change the headline
text to explicitly mark this output as a "smoke harness" so it doesn't present
presence-as-quality.
In `@scripts/eval_retrieval.py`:
- Around line 86-95: The flag use_query currently makes client.post(...) call
"/api/v1/query" (using variables question, namespace, top_k), which triggers
full RAG/LLM answer generation and breaks retrieval-only benchmarking; change
the behavior so that when the intent is retrieval-only you either (A) remove the
use_query flag entirely or (B) route it to the retrieval-only endpoint (e.g.,
call "/api/v1/search" or your retrieval-rerank endpoint) and pass the same
question/namespace/top_k (and any rerank parameter) so answer-generation is
skipped; update both occurrences that currently call "/api/v1/query" (the block
using use_query and the second occurrence around lines 297-301) to use the
retrieval-only route or remove the flag as appropriate.
- Around line 59-62: The keyword-matching in chunk_matches_keywords currently
only lowercases text, missing matches due to diacritics; normalize both the
input chunk text and each keyword by applying Unicode normalization (e.g., NFKD)
and stripping combining diacritic marks before lowercasing so ASCII-fallbacks
like "liberta'" match "libertà"; update chunk_matches_keywords to perform this
normalization on text_lower and on each kw before performing the containment
check.
In `@tests/eval/dataset.jsonl`:
- Line 49: The test entry with id "ADV-04" incorrectly leaves expected_keywords
empty so the harness will skip a legitimate retrieval check; update the object
for "ADV-04" to either (A) move it from the adversarial/unscored bucket into a
scored bucket by changing "category" to "scored" and populate
"expected_keywords" with representative Italian phrases that must appear in a
correct answer (e.g. "pena di morte vietata", "Costituzione italiana",
"abolita", or other exact grounding tokens you expect), or (B) if you want to
assert a refusal/contextual response instead, keep it in adversarial but set an
explicit refusal assertion (e.g. add a field like
"expected_behavior":"refuse_to_answer" and document the exact refusal tokens in
"expected_keywords") so the harness can evaluate the behavior; edit the JSON
object for id "ADV-04" accordingly (modify "expected_keywords" and/or "category"
and/or add an "expected_behavior" field).
In `@vektra-admin/src/vektra_admin/api.py`:
- Around line 462-491: The get_conversation_turns admin endpoint lacks audit
logging; update the get_conversation_turns function to record an audit entry for
every authenticated call (including actor from _key and conversation_id and
success/failure outcome) by enqueuing an audit background task (use the same
audit helper used by other admin endpoints) instead of blocking the request;
ensure you add the BackgroundTasks parameter to the function signature, call the
registry/audit helper (or the existing create_audit_task utility) after
obtaining conv_store and before returning turns, and also log failures
(404/501/503/500) via the same audit mechanism so all authenticated requests are
logged.
In `@vektra-core/src/vektra_core/api.py`:
- Around line 153-168: The current logic only calls
analytics_service.store_trace when chunk.type == "trace", but SSE trace chunks
are not emitted yet so QueryTrace never gets persisted; update the persistence
to trigger independently of SSE emission by detecting QueryTrace payloads (e.g.,
inspect chunk.data for a QueryTrace/trace structure or hook into the
non-streaming response path that returns QueryTrace) and call
trace_from_dict(...) and analytics_service.store_trace(sess, trace_obj,
namespace=namespace) using the existing db_session_factory/async session pattern
(same try/except and log.warning on failure) so traces get stored even when
chunk.type is not "trace" (ensure you reference chunk.data, trace_from_dict,
analytics_service.store_trace, db_session_factory and accommodate
AdvancedQueryPipeline/non-streaming response flow).
In `@vektra-core/src/vektra_core/pipeline.py`:
- Around line 61-69: The _history_to_messages function currently converts turns
with answer=None into an assistant Message containing the literal "[No
response]", which injects synthetic assistant content; change the logic so that
for each turn you always append the user Message (Message(role="user",
content=turn["question"] or "")) but only append an assistant Message when
turn["answer"] is not None (i.e., skip creating Message(role="assistant", ...)
for missing answers) so that absent answers are not made visible to the model;
update the code paths around _history_to_messages/Message accordingly to
preserve ordering without inserting placeholder assistant turns.
In `@vektra-core/src/vektra_core/templates/context.j2`:
- Line 2: The template injects raw chunk.text into XML-like <source> tags;
escape chunk.text to prevent tag-breaking content. Update the for-loop output
from <source id="{{ loop.index }}">{{ chunk.text }}</source> to use Jinja2's
escape filter (e.g., {{ chunk.text|e }} or {{ chunk.text|escape }}) so any
special characters are encoded before embedding; ensure the change is applied in
the template that iterates over chunks.
In `@vektra-core/src/vektra_core/templates/system.j2`:
- Around line 27-29: The else branch in the Jinja template (the block that
currently outputs "You do not have reference material for this question. Say you
do not have enough information to answer.") is too strict and drops permission
to use prior assistant turns; update that branch so it instructs the model to
first consult previous assistant answers in the conversation even when
has_context is false, and only refuse if prior conversation and current input
still lack the needed grounding; modify the text emitted by this else branch in
templates/system.j2 to include the allowance to use previous assistant responses
and then a fallback to say "not enough information" if still insufficient.
In `@vektra-learn/src/vektra_learn/api.py`:
- Around line 473-480: Remove the request-scoped AsyncSession parameter from the
course_query signature and instead acquire a short-lived session only where the
enrollment lookup occurs: delete the session: AsyncSession =
Depends(_get_session) parameter from course_query, and around the enrollment
lookup use a local context-managed session (e.g. async with _get_session() as
session:) to perform the lookup, then close that session before proceeding to
streaming; leave the LearnService dependency and other parameters unchanged so
the streaming path does not hold a pooled connection for the request lifetime.
In `@vektra-learn/tests/test_trace_persistence.py`:
- Around line 71-165: These tests duplicate the trace-persistence branch instead
of exercising the real API path; replace the inline persistence logic in
test_learn_store_trace_called_when_enabled,
test_learn_store_trace_not_called_when_disabled,
test_learn_store_trace_failure_does_not_propagate, and
test_learn_store_trace_skipped_when_trace_is_none with calls to the actual learn
endpoint/handler (referencing vektra_learn.api or the exposed learn handler
function) so the production code opens the DB session and runs the try/except;
construct the app state via _make_app_state with mocked analytics_svc and
db_session_factory, invoke the real handler (or test client request) with the
trace and namespace, then assert mock_svc.store_trace was/wasn't called and that
the namespace and trace arguments match as before.
---
Outside diff comments:
In `@vektra-core/src/vektra_core/advanced_pipeline.py`:
- Around line 375-404: When post_retrieval_safeguard returns sg_result.allowed
== False you must preserve a hard block instead of collapsing it into "no
relevant context": set the pipeline's safeguard_blocked flag (the same flag used
by SimpleQueryPipeline, e.g., self.safeguard_blocked or the pipeline/context
object) when sg_result.allowed is False, keep filtered empty, and ensure
StepTrace still records the denial; do the same change in the other equivalent
blocks (the ones around the other post_retrieval_safeguard calls referenced in
the comment) and keep SafeguardHook integration intact so execute() and
_stream() can short-circuit based on safeguard_blocked rather than proceeding
with zero context.
In `@vektra-shared/src/vektra_shared/config.py`:
- Around line 188-206: QueryPipelineConfig's numeric fields lack range
constraints so invalid values can be instantiated directly; add validation
constraints to the Field declarations: set min_relevance_score = Field(0.15,
alias="VEKTRA_MIN_RELEVANCE_SCORE", ge=0.0, le=1.0, ...), context_chunk_ratio =
Field(0.6, alias="VEKTRA_CONTEXT_CHUNK_RATIO", ge=0.0, le=1.0, ...), and tighten
response_token_reserve = Field(2048, alias="VEKTRA_RESPONSE_TOKEN_RESERVE",
ge=0) (or ge=1 if zero is invalid); leave grounding_mode's `@field_validator` in
place — use these ge/le constraints on the existing Field calls
(min_relevance_score, context_chunk_ratio, response_token_reserve) to enforce
sub-config level validation.
- Around line 539-572: Add a model-level validator (e.g.,
validate_eval_mode_production) to the settings class that checks
os.environ.get("ENV") (or os.getenv) and rejects eval_mode=True when the
environment equals "production" by raising ValueError; import os, annotate with
`@model_validator` (after validation) and place it alongside the existing field
validators (referencing eval_mode and model_config/SettingsConfigDict) so
misconfiguration is blocked at model instantiation.
---
Nitpick comments:
In `@Dockerfile`:
- Around line 73-76: The conditional uv sync block referencing
INSTALL_UNSTRUCTURED should not include third-party extras (sparse, qdrant,
ocr); remove the flags "--extra sparse --extra qdrant --extra ocr" from the uv
sync inside the INSTALL_UNSTRUCTURED conditional (the layer that builds
workspace packages) and instead add those extras to the earlier manifest-only uv
sync invocation that runs after copying only the workspace manifests (so the
third-party deps are installed in the manifest-only layer and benefit from
caching); keep the conditional only for installing unstructured-specific runtime
bits if needed and leave workspace package build/install logic in the existing
uv sync (functionally the INSTALL_UNSTRUCTURED conditional should only control
whether source-specific unstructured pieces are installed, not the third-party
extras).
In `@vektra-core/tests/test_conversation.py`:
- Around line 88-90: The test currently checks stored history length and the
question but not the stored answer; update the assertion in the test (around the
history retrieval using store.get_history and variable cid in
test_conversation.py) to also assert history[0]["answer"] == "A1" so the stored
answer is validated alongside the question.
In `@vektra-core/tests/test_pipeline.py`:
- Around line 539-559: The tests mutate module-level sets
_token_count_fallback_warned and _context_window_fallback_warned which can leak
state between tests; add an autouse fixture that clears both sets before and
after each test to guarantee isolation (reference the fixture to affect tests
like test_count_tokens_fallback_warns and test_count_tokens_fallback_warns_once
and any other tests touching those sets) so each test starts with empty sets and
no inter-test ordering or parallelism issues.
In `@vektra-shared/src/vektra_shared/namespace.py`:
- Around line 39-42: The code validates ns_mode against _VALID_GROUNDING_MODES
but returns default_mode unvalidated; update the logic so default_mode is
checked as well: before returning default_mode, verify it is in
_VALID_GROUNDING_MODES (using something like `if default_mode in
_VALID_GROUNDING_MODES: return str(default_mode)`), and if it's not valid either
raise a ValueError or fall back to a safe known mode; ensure you reference the
same symbols (row, ns_mode, default_mode, _VALID_GROUNDING_MODES) when making
the change.
In `@vektra-shared/src/vektra_shared/types.py`:
- Around line 262-274: QueryRequest currently uses a plain string for
grounding_mode which allows typos; define a shared enum GroundingMode (e.g.,
class GroundingMode(str, Enum) with values "strict" and "hybrid") in
vektra_shared.types and change the QueryRequest grounding_mode type to
GroundingMode with default GroundingMode.STRICT, updating any imports/usages and
preserving backward-compatibility by accepting GroundingMode values where
QueryRequest is constructed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 28f8b613-d480-4282-8f50-af91c10113af
⛔ Files ignored due to path filters (5)
.s2s/BACKLOG.mdis excluded by!.s2s/**.s2s/plans/20260325-rag-retrieval-quality.mdis excluded by!.s2s/**.s2s/plans/20260328-core-trace-observability.mdis excluded by!.s2s/**.s2s/plans/20260403-prompt-grounding-mode.mdis excluded by!.s2s/**uv.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (53)
.claude/CLAUDE.md.env.example.github/workflows/integration.yml.gitignoreDockerfileMakefiledocs/reference/configuration.mdscripts/eval_e2e.pyscripts/eval_retrieval.pytests/eval/dataset.jsonlvektra-admin/pyproject.tomlvektra-admin/src/vektra_admin/api.pyvektra-admin/tests/test_admin_turns.pyvektra-analytics/pyproject.tomlvektra-analytics/src/vektra_analytics/__init__.pyvektra-app/pyproject.tomlvektra-app/src/vektra_app/__init__.pyvektra-app/src/vektra_app/main.pyvektra-core/pyproject.tomlvektra-core/src/vektra_core/advanced_pipeline.pyvektra-core/src/vektra_core/api.pyvektra-core/src/vektra_core/conversation.pyvektra-core/src/vektra_core/pipeline.pyvektra-core/src/vektra_core/providers/litellm_provider.pyvektra-core/src/vektra_core/reranker.pyvektra-core/src/vektra_core/templates.pyvektra-core/src/vektra_core/templates/context.j2vektra-core/src/vektra_core/templates/conversation.j2vektra-core/src/vektra_core/templates/system.j2vektra-core/tests/test_advanced_pipeline.pyvektra-core/tests/test_conversation.pyvektra-core/tests/test_litellm_provider.pyvektra-core/tests/test_pipeline.pyvektra-core/tests/test_reranker.pyvektra-core/tests/test_templates.pyvektra-core/tests/test_trace_persistence.pyvektra-index/pyproject.tomlvektra-ingest/pyproject.tomlvektra-learn/pyproject.tomlvektra-learn/src/vektra_learn/api.pyvektra-learn/tests/test_trace_persistence.pyvektra-learn/widget/src/api-client.jsvektra-learn/widget/src/chat-ui.jsvektra-learn/widget/src/index.jsvektra-learn/widget/src/markdown.jsvektra-learn/widget/src/styles.jsvektra-shared/pyproject.tomlvektra-shared/src/vektra_shared/__init__.pyvektra-shared/src/vektra_shared/config.pyvektra-shared/src/vektra_shared/namespace.pyvektra-shared/src/vektra_shared/types.pyvektra-shared/tests/test_config.pyvektra-shared/tests/test_namespace.py
💤 Files with no reviewable changes (1)
- vektra-core/src/vektra_core/templates/conversation.j2
CRITICAL: post_retrieval safeguard bypass in hybrid mode. When the safeguard denied retrieval (allowed=False), hybrid mode continued to call the LLM without context, bypassing the deny decision. Added safeguard_blocked flag to AdvancedQueryPipeline (matching SimpleQueryPipeline pattern) that short-circuits regardless of grounding mode. HIGH: XML injection via chunk text in context.j2. Raw chunk text was embedded in <source> tags without escaping. Added Jinja2 |e filter. HIGH: synthetic [No response] injected into conversation history when answer was None. Polluted LLM context with artificial content. Now skips turns with no answer entirely. HIGH: missing range constraints on QueryPipelineConfig numeric fields (min_relevance_score, context_chunk_ratio, response_token_reserve). Added ge/le/gt/lt constraints to Field declarations. HIGH: no production guard for VEKTRA_EVAL_MODE. Added model_validator that rejects eval_mode=true when env=production. MEDIUM: unreachable code in learn trace persistence test. Removed redundant condition that could never be true. Also isolated test helpers from env var leakage by defaulting VEKTRA_EVAL_MODE=False in _make_pipeline_config(). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
4 tasks
Fix return type annotation of _run_pre_llm_steps to include the new safeguard_blocked bool, and fix pre_query safeguard early return to include all 6 tuple values. Fix mypy arg-type in _history_to_messages by binding turn["answer"] to a local variable for proper type narrowing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When post_retrieval safeguard denies access, the response now correctly reports context_only=False and no_relevant_context=False (not a context issue, it's a safeguard denial). Log event is query_safeguard_blocked instead of query_no_relevant_context. Added regression test: test_post_retrieval_safeguard_denied_blocks_llm verifies that safeguard denial prevents LLM call even in hybrid mode. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
eval_e2e.py: distinguish grounded answers (with retrieval context) from
ungrounded answers (LLM without context). Previously any non-None
answer counted as success, masking hallucinations on adversarial
prompts.
eval_retrieval.py: normalize diacritics before keyword matching so
ASCII dataset entries ("liberta'") match Unicode chunk text ("liberta").
Rename --use-query to --use-pipeline with clearer help text explaining
it runs the full RAG pipeline, not retrieval-only.
dataset.jsonl: reclassify ADV-04 (death penalty) from adversarial to
factual with expected_keywords, since Art. 27 explicitly covers it.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
GET /api/v1/admin/conversations/{id}/turns returns sensitive decrypted
content but had no audit trail. Added background audit log with
action=conversation_turns_read, matching the pattern used by other
admin endpoints.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add namespace (from request.state.rls_namespace) to conversation turns audit log metadata. Add test assertion verifying audit background task is queued with correct action and metadata. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
_normalize() now strips apostrophe variants so ASCII dataset entries like "liberta'" match Unicode chunk text "liberta" (after diacritic removal). Also updated module docstring to reflect that --use-pipeline invokes the full RAG pipeline including LLM. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- CRITICAL: safeguard bypass in hybrid mode (post_retrieval allowed=False now hard-blocks LLM) - HIGH: XML escape chunk text in context.j2 (prevent tag injection) - HIGH: remove synthetic [No response] from conversation history - HIGH: config range constraints (min_relevance_score, context_chunk_ratio, response_token_reserve) - HIGH: production guard rejecting VEKTRA_EVAL_MODE in env=production - Eval harness: diacritic+apostrophe normalization, grounded vs ungrounded metrics, --use-pipeline rename - Audit logging for admin conversation turns endpoint with namespace metadata Reviews: 18/18 addressed (CodeRabbit 14, Gemini 2, github-code-quality 2) Tests: 197 passed Refs: PR #56 release review findings
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
vektra-core/src/vektra_core/pipeline.py (1)
427-453:⚠️ Potential issue | 🟠 MajorDon’t report safeguard blocks as
context_only.When
safeguard_blockedis true, this branch returnscontext_only=Trueand logsquery_no_relevant_context. That misclassifies a policy block as an LLM/context fallback, which can drive the wrong client behavior and analytics.Proposed fix
- if safeguard_blocked or ( - no_relevant_context and query.grounding_mode != "hybrid" - ): + if safeguard_blocked or ( + no_relevant_context and query.grounding_mode != "hybrid" + ): trace = QueryTrace( response_id=response_id, steps=steps, total_duration_ms=_elapsed_ms(t_total), chunks_retrieved=[], llm_model=self._llm_config.provider, prompt_version=self._renderer.prompt_version, created_at=datetime.now(UTC), ) - log.info( - "query_no_relevant_context", - response_id=str(response_id), - namespace=query.namespace, - ) + log.info( + "query_safeguard_blocked" if safeguard_blocked else "query_no_relevant_context", + response_id=str(response_id), + namespace=query.namespace, + ) return QueryResponse( response_id=response_id, answer=None, sources=[], conversation_id=query.conversation_id, - context_only=safeguard_blocked, - no_relevant_context=no_relevant_context, + context_only=False, + no_relevant_context=no_relevant_context and not safeguard_blocked, ), trace🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vektra-core/src/vektra_core/pipeline.py` around lines 427 - 453, The branch that handles safeguard_blocked currently returns QueryResponse(..., context_only=safeguard_blocked) and logs "query_no_relevant_context", which mislabels policy blocks; change this so context_only is true only when no_relevant_context is true (i.e., context_only=no_relevant_context) and adjust logging to differentiate safeguard blocks (e.g., log "query_safeguard_blocked" when safeguard_blocked is true) while preserving no_relevant_context in the response and trace values; update the block around safeguard_blocked, QueryResponse, and log.info accordingly.vektra-core/src/vektra_core/advanced_pipeline.py (1)
781-789:⚠️ Potential issue | 🟠 MajorPersist streamed turns even when the final answer is empty or blocked.
execute()stores the turn regardless ofanswer, but_stream()skipsadd_turn(...)whenfull_answeris falsy. That makes streamed conversations lose the user turn for context-only, empty, or safeguard-blocked responses, so history diverges by transport.Proposed fix
- if query.conversation_id is not None and full_answer: + if query.conversation_id is not None: try: await self._conversation_store.add_turn( query.conversation_id, query.question, full_answer, response_id=response_id, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vektra-core/src/vektra_core/advanced_pipeline.py` around lines 781 - 789, In _stream(), the code only calls await self._conversation_store.add_turn(...) when full_answer is truthy, causing streamed turns with empty/blocked/context-only responses to be dropped; update the logic to mirror execute() by persisting the turn whenever query.conversation_id is not None (and a response_id exists) regardless of full_answer value—i.e., remove the truthiness gate on full_answer and always call self._conversation_store.add_turn(query.conversation_id, query.question, full_answer, response_id=response_id) so streamed conversations keep the user turn for history alignment with execute().
♻️ Duplicate comments (2)
scripts/eval_e2e.py (1)
108-145:⚠️ Potential issue | 🟠 MajorQuality headline is still measuring response presence, not correctness.
This summary reports delivery/grounding state but not answer correctness, while the script and heading present it as quality evaluation. Please either add expected-answer/refusal scoring or explicitly rename this as smoke reporting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/eval_e2e.py` around lines 108 - 145, The print_summary function currently only reports delivery/grounding (answered/no_relevant_context) but not correctness; either update print_summary to compute correctness metrics by comparing each E2EResult's expected answer/refusal fields with the produced answer/refusal (e.g., add counts for correct_answer, incorrect_answer, correct_refusal, incorrect_refusal and an overall accuracy = correct / evaluated and include pct values in the printed summary), or explicitly change the heading/labels to indicate this is a smoke/reporting summary (e.g., replace "End-to-end evaluation results" with "Smoke evaluation (delivery only)" and remove wording implying quality). Locate and modify the print_summary function and use the E2EResult attributes (expected_answer, refused, answer, no_relevant_context, etc.) to implement the chosen fix and update printed lines accordingly.vektra-core/src/vektra_core/pipeline.py (1)
61-70:⚠️ Potential issue | 🟠 MajorKeep the user turn even when the assistant answer is missing.
The
continueon Line 66 drops the whole turn, so prior user questions disappear from history after context-only or blocked responses. Preserve the user message and skip only the assistant message whenansweris absent.Proposed fix
def _history_to_messages(history: list[dict[str, str | None]]) -> list[Message]: """Convert conversation history to role-based Message objects.""" messages: list[Message] = [] for turn in history: + question = turn.get("question") answer = turn.get("answer") - if not answer: - continue # Skip turns with no answer to avoid polluting history - messages.append(Message(role="user", content=turn["question"] or "")) + if question: + messages.append(Message(role="user", content=question)) + if not answer: + continue messages.append(Message(role="assistant", content=answer)) return messages🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vektra-core/src/vektra_core/pipeline.py` around lines 61 - 70, The _history_to_messages function currently drops an entire turn when turn.get("answer") is falsy; instead preserve the user's message and only omit the assistant message. Update _history_to_messages to always append Message(role="user", content=turn["question"] or "") for each turn, and only append Message(role="assistant", content=answer) if answer is truthy (i.e., remove the continue and guard the assistant append). Ensure Message creation uses the same role strings ("user"/"assistant") and that history/list handling remains unchanged.
🧹 Nitpick comments (2)
vektra-admin/src/vektra_admin/api.py (1)
466-472: Narrow the endpoint return type annotation.
get_conversation_turnscurrently returnsAny, but the contract is a list ofConversationTurnDetail. Tightening the signature improves type safety and editor/static analysis quality.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vektra-admin/src/vektra_admin/api.py` around lines 466 - 472, The get_conversation_turns endpoint is annotated to return Any; change it to return a list of ConversationTurnDetail to match the contract. Update the function signature from -> Any to -> list[ConversationTurnDetail] (or typing.List[ConversationTurnDetail] if using older Python typing), ensure ConversationTurnDetail is imported or referenced from its module, and adjust any related imports (typing.List or from __future__ import annotations) so static checkers and editor tooling pick up the tighter return type.scripts/eval_e2e.py (1)
236-236: Status label is inconsistent withno_relevant_context.A non-empty
answeris labeledOKeven whenno_relevant_context=True, which conflicts with the summary semantics.Proposed fix
- status = "OK" if result.answer else ("ERR" if result.error else "NO_CTX") + status = ( + "ERR" + if result.error + else "NO_CTX" + if result.no_relevant_context + else "OK" + if result.answer is not None + else "UNANSWERED" + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/eval_e2e.py` at line 236, The status assignment logic incorrectly marks results with a non-empty result.answer as "OK" even when result.no_relevant_context is True; update the conditional that sets status (the line assigning status) to first check result.no_relevant_context and return "NO_CTX" if true, otherwise fall back to the existing logic (use "OK" when result.answer, "ERR" when result.error, else "NO_CTX"), ensuring references to result.answer, result.error and result.no_relevant_context are used in that order.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/eval_e2e.py`:
- Around line 95-104: The code building E2EResult assumes every item in sources
has a "score" and will crash on missing keys; update the source_scores
construction (used when creating the E2EResult) to safely extract scores from
the sources list by using a safe access (e.g., s.get("score") with a sensible
default like 0 or None) and optionally coerce/validate numeric values so that
missing or malformed entries do not raise; locate references to E2EResult and
the sources variable and replace the current [s["score"] for s in sources] usage
with a resilient extraction that handles partial payloads.
- Around line 61-63: The code currently assumes every JSONL row has required
keys by directly accessing entry["id"] and entry["question"]; instead, validate
those fields before using them in the API call inside the loop that processes
each entry (the block that assigns question_id/question/namespace). Use safe
lookup (entry.get(...)) and if either id or question is missing or falsy, log a
warning including the raw entry and skip to the next iteration (i.e., continue)
so a single malformed row won’t crash the whole run; keep namespace defaulting
to "default" as done now.
In `@scripts/eval_retrieval.py`:
- Around line 138-141: The code assumes every item in results has a "score"
causing a KeyError for malformed items; update the scores calculation (the
results and scores variables) to use r.get("score") and either skip items
missing a score or substitute a safe default (e.g., 0), and optionally emit a
warning/log when an item lacks "score" so evaluation continues without
hard-failing.
- Around line 124-135: The error-return branch is forcing has_ground_truth=False
even when the dataset row actually contains ground-truth keywords; update the
QuestionResult construction in the exception handler to preserve the same
ground-truth check used elsewhere (e.g., set has_ground_truth to the boolean
check used for keywords/row, such as bool(keywords) or
bool(row.get("keywords"))), so the field reflects the real availability of
ground truth instead of always False; keep all other error fields (error, hit,
reciprocal_rank, etc.) the same.
- Around line 94-99: Validate required fields 'id' and 'question' on each
dataset row before performing the request: in scripts/eval_retrieval.py where
question_id and question are pulled from entry (variables question_id and
question), check if "id" and "question" exist and are truthy on entry, and if
not log a warning/error (including the entry or index) and continue/skip that
row so the missing-field error doesn't escape the request try block; ensure you
do this check immediately before assigning question_id/question and before the
request logic.
In `@vektra-admin/src/vektra_admin/api.py`:
- Around line 482-488: The current check only uses hasattr(conv_store,
"get_turns_detail") which can still let a non-callable/non-async value slip
through and cause a TypeError at await
conv_store.get_turns_detail(conversation_id); change the guard to verify the
attribute is callable and async: fetch the attribute via getattr(conv_store,
"get_turns_detail", None) and if it's not callable or not an async function (use
inspect.iscoroutinefunction or asyncio.iscoroutinefunction on
conv_store.get_turns_detail) raise the same HTTPException( status_code=501,
detail=... ); then safely await conv_store.get_turns_detail(conversation_id).
In `@vektra-core/src/vektra_core/pipeline.py`:
- Around line 902-912: The streamed-path currently skips calling
self._conversation_store.add_turn when full_answer is falsy, which differs from
execute() that always persists the turn with answer=None; change the block in
pipeline.py to always call await
self._conversation_store.add_turn(query.conversation_id, query.question,
full_answer or None, response_id=response_id) (or explicitly pass None when
full_answer is falsy) when query.conversation_id is not None, keeping the
existing try/except and log.warning("conversation_turn_store_failed",
error=str(exc)) behavior so blocked/empty streamed turns are persisted the same
way as execute().
In `@vektra-shared/src/vektra_shared/config.py`:
- Around line 223-232: The raw-text flags eval_mode and debug_log_queries are
currently only guarded at VektraSettings and can be bypassed by instantiating
Phase 2 configs (e.g., QueryPipelineConfig) directly; change the design so each
Phase 2 config (start with QueryPipelineConfig) validates these flags at
instantiation: read the runtime environment/phase indicator available to the
config (the same signal VektraSettings uses), and if running in
production/locked mode and eval_mode or debug_log_queries are true, either throw
a clear configuration error or coerce them to false; apply the same
instantiation-time validation to all Phase 2 sub-configs (the other flags around
lines 579-585) so the privacy guard cannot be bypassed by constructing
sub-config objects directly.
---
Outside diff comments:
In `@vektra-core/src/vektra_core/advanced_pipeline.py`:
- Around line 781-789: In _stream(), the code only calls await
self._conversation_store.add_turn(...) when full_answer is truthy, causing
streamed turns with empty/blocked/context-only responses to be dropped; update
the logic to mirror execute() by persisting the turn whenever
query.conversation_id is not None (and a response_id exists) regardless of
full_answer value—i.e., remove the truthiness gate on full_answer and always
call self._conversation_store.add_turn(query.conversation_id, query.question,
full_answer, response_id=response_id) so streamed conversations keep the user
turn for history alignment with execute().
In `@vektra-core/src/vektra_core/pipeline.py`:
- Around line 427-453: The branch that handles safeguard_blocked currently
returns QueryResponse(..., context_only=safeguard_blocked) and logs
"query_no_relevant_context", which mislabels policy blocks; change this so
context_only is true only when no_relevant_context is true (i.e.,
context_only=no_relevant_context) and adjust logging to differentiate safeguard
blocks (e.g., log "query_safeguard_blocked" when safeguard_blocked is true)
while preserving no_relevant_context in the response and trace values; update
the block around safeguard_blocked, QueryResponse, and log.info accordingly.
---
Duplicate comments:
In `@scripts/eval_e2e.py`:
- Around line 108-145: The print_summary function currently only reports
delivery/grounding (answered/no_relevant_context) but not correctness; either
update print_summary to compute correctness metrics by comparing each
E2EResult's expected answer/refusal fields with the produced answer/refusal
(e.g., add counts for correct_answer, incorrect_answer, correct_refusal,
incorrect_refusal and an overall accuracy = correct / evaluated and include pct
values in the printed summary), or explicitly change the heading/labels to
indicate this is a smoke/reporting summary (e.g., replace "End-to-end evaluation
results" with "Smoke evaluation (delivery only)" and remove wording implying
quality). Locate and modify the print_summary function and use the E2EResult
attributes (expected_answer, refused, answer, no_relevant_context, etc.) to
implement the chosen fix and update printed lines accordingly.
In `@vektra-core/src/vektra_core/pipeline.py`:
- Around line 61-70: The _history_to_messages function currently drops an entire
turn when turn.get("answer") is falsy; instead preserve the user's message and
only omit the assistant message. Update _history_to_messages to always append
Message(role="user", content=turn["question"] or "") for each turn, and only
append Message(role="assistant", content=answer) if answer is truthy (i.e.,
remove the continue and guard the assistant append). Ensure Message creation
uses the same role strings ("user"/"assistant") and that history/list handling
remains unchanged.
---
Nitpick comments:
In `@scripts/eval_e2e.py`:
- Line 236: The status assignment logic incorrectly marks results with a
non-empty result.answer as "OK" even when result.no_relevant_context is True;
update the conditional that sets status (the line assigning status) to first
check result.no_relevant_context and return "NO_CTX" if true, otherwise fall
back to the existing logic (use "OK" when result.answer, "ERR" when
result.error, else "NO_CTX"), ensuring references to result.answer, result.error
and result.no_relevant_context are used in that order.
In `@vektra-admin/src/vektra_admin/api.py`:
- Around line 466-472: The get_conversation_turns endpoint is annotated to
return Any; change it to return a list of ConversationTurnDetail to match the
contract. Update the function signature from -> Any to ->
list[ConversationTurnDetail] (or typing.List[ConversationTurnDetail] if using
older Python typing), ensure ConversationTurnDetail is imported or referenced
from its module, and adjust any related imports (typing.List or from __future__
import annotations) so static checkers and editor tooling pick up the tighter
return type.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3aaa0383-0825-4f66-b305-a6740005242b
📒 Files selected for processing (12)
scripts/eval_e2e.pyscripts/eval_retrieval.pytests/eval/dataset.jsonlvektra-admin/src/vektra_admin/api.pyvektra-admin/tests/test_admin_turns.pyvektra-core/src/vektra_core/advanced_pipeline.pyvektra-core/src/vektra_core/pipeline.pyvektra-core/src/vektra_core/templates/context.j2vektra-core/tests/test_advanced_pipeline.pyvektra-core/tests/test_pipeline.pyvektra-learn/tests/test_trace_persistence.pyvektra-shared/src/vektra_shared/config.py
✅ Files skipped from review due to trivial changes (1)
- tests/eval/dataset.jsonl
🚧 Files skipped from review as they are similar to previous changes (5)
- vektra-admin/tests/test_admin_turns.py
- vektra-core/tests/test_pipeline.py
- vektra-learn/tests/test_trace_persistence.py
- vektra-core/tests/test_advanced_pipeline.py
- vektra-core/src/vektra_core/templates/context.j2
Contributor
Author
|
Replies to review comments that couldn't be posted inline (outside diff range):
|
The "Query latency measurement" step blocks indefinitely in CI because no LLM is available in GitHub Actions. When the 30-minute job timeout expires, the entire job is cancelled, blocking merge on main. The test remains in tests/nfr/test_performance.py for manual execution on machines with a live LLM (e.g. Kalypso). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Remove query latency measurement step from CI integration workflow - Step required a live LLM unavailable in GitHub Actions, causing 30-min timeout and job cancellation - Test preserved at tests/nfr/test_performance.py for manual execution Reviews: 0 comments Tests: workflow-only change, no code affected
This was referenced Apr 21, 2026
This was referenced Jul 14, 2026
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
1 participant
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.
What
This release includes 7 PRs merged since v0.3.0, focused on pipeline observability, prompt quality, and widget production-readiness.
Added
strict(context + history only) vshybrid(+ training data fallback) viaVEKTRA_PROMPT_GROUNDING_MODEwith per-namespace override (FEAT-020, PR feat(prompt): add configurable grounding mode strict/hybrid #54)VEKTRA_EVAL_MODE=true, traces capture rewritten query text, full prompt messages, and per-candidate reranker scores (FEAT-019, DEBT-015, DEBT-014, PR feat(observability): add reranker scores, streaming model fix, eval mode tracing #55)VEKTRA_DEBUG_LOG_QUERIES=truelogs original and rewritten queries at debug level (DEBT-009, PR feat(observability): add reranker scores, streaming model fix, eval mode tracing #55)GET /api/v1/admin/conversations/{id}/turnswith pgp_sym_decrypt (DEBT-011, PR fix(observability): persist QueryTrace and add admin conversation turns endpoint #53)Fixed
llm_modelin traces now consistent between streaming and non-streaming paths (BUG-019, PR feat(observability): add reranker scores, streaming model fix, eval mode tracing #55)conversation.j2andrender_conversation()(DEBT-016, PR feat(prompt): add configurable grounding mode strict/hybrid #54)Changed
RerankResultwith full score visibility for all evaluated candidates (DEBT-014)eval_modeanddebug_log_queriesadded toQueryPipelineConfig(no constructor changes)CompletionChunkgains optionalmodelfield for streaming model resolutiontrim_blocks/lstrip_blocksfor clean template outputStats
Milestone: https://github.com/vektralabs/vektra-stack/milestone/4?closed=1
Summary by CodeRabbit
New Features
Improvements
Documentation
UI
Tests