fix(core): address release review findings for v0.4.0#57
Merged
Conversation
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>
fvadicamo
added
bug
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Requestor
participant Pipeline as AdvancedPipeline
participant PreSteps as Pre-LLM Steps
participant Retriever as Retriever
participant Safeguard as SafeguardHook
participant LLM as LLM Executor
participant Builder as Response Builder
Client->>Pipeline: execute(query)
Pipeline->>PreSteps: _run_pre_llm_steps(query)
PreSteps->>Retriever: retrieve & filter docs
Retriever-->>PreSteps: filtered_docs
PreSteps->>Safeguard: post_retrieval(filtered_docs)
Safeguard-->>PreSteps: SafeguardResult(allowed=false) / allowed=true
alt Safeguard denies (allowed=false)
PreSteps-->>Pipeline: return (safeguard_blocked=true)
Pipeline->>Builder: build sources-only empty response + trace
Builder-->>Client: return sources-only response (no LLM)
else Safeguard allows
PreSteps-->>Pipeline: return (safeguard_blocked=false, effective_query)
Pipeline->>LLM: invoke with effective_query + context
LLM-->>Pipeline: generation/result
Pipeline->>Builder: build full response + trace
Builder-->>Client: return full response
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/advanced_pipeline.py (1)
202-215:⚠️ Potential issue | 🔴 CriticalFix return type annotation — mypy errors at lines 407, 502, 659.
The function now returns 6 values (including
safeguard_blocked), but the type annotation still declares 5. This causes the lint failures.🐛 Proposed fix for return type annotation
async def _run_pre_llm_steps( self, query: QueryRequest, ) -> tuple[ list[StepTrace], list[SearchResult], bool, + bool, str, list[dict[str, str | None]], ]: """Run steps 0-6 (rewrite through post_retrieval safeguard). - Returns (steps, filtered_results, no_relevant_context, effective_query, history). + Returns (steps, filtered_results, no_relevant_context, safeguard_blocked, effective_query, history). """🤖 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 202 - 215, The return type annotation for _run_pre_llm_steps is out of sync with its actual returns: update the annotated tuple to include the sixth boolean value (safeguard_blocked) so mypy stops complaining; specifically adjust the annotated tuple in the _run_pre_llm_steps signature to reflect (list[StepTrace], list[SearchResult], bool, str, list[dict[str, str | None]], bool) — keep the order matching the function's return order and update any docstring or callers if they rely on the old 5-tuple.vektra-core/src/vektra_core/pipeline.py (1)
61-69:⚠️ Potential issue | 🔴 CriticalFix mypy type error on line 68.
The pipeline lint failure indicates that
turn["answer"]has typestr | None, butMessage.contentexpectsstr. After the truthiness check on line 65, the type system doesn't narrow the type automatically sinceturn.get("answer")andturn["answer"]are separate lookups.🐛 Proposed fix to satisfy mypy
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: - if not turn.get("answer"): + 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 "")) - messages.append(Message(role="assistant", content=turn["answer"])) + messages.append(Message(role="assistant", content=answer)) return messagesThis binds
answerto a local variable, allowing mypy to narrow its type after the truthiness check.🤖 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 - 69, In _history_to_messages, mypy complains because turn["answer"] is typed as str | None and the lookup isn't narrowed by the earlier truthiness check; fix by binding the value to a local variable (e.g., answer = turn.get("answer")) then check if not answer: continue and use that local answer when constructing Message(role="assistant", content=answer); likewise bind question = turn.get("question") or "" (or use turn.get("question") in the user Message) to ensure Message.content receives a str and types are properly narrowed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@vektra-core/src/vektra_core/advanced_pipeline.py`:
- Around line 202-215: The return type annotation for _run_pre_llm_steps is out
of sync with its actual returns: update the annotated tuple to include the sixth
boolean value (safeguard_blocked) so mypy stops complaining; specifically adjust
the annotated tuple in the _run_pre_llm_steps signature to reflect
(list[StepTrace], list[SearchResult], bool, str, list[dict[str, str | None]],
bool) — keep the order matching the function's return order and update any
docstring or callers if they rely on the old 5-tuple.
In `@vektra-core/src/vektra_core/pipeline.py`:
- Around line 61-69: In _history_to_messages, mypy complains because
turn["answer"] is typed as str | None and the lookup isn't narrowed by the
earlier truthiness check; fix by binding the value to a local variable (e.g.,
answer = turn.get("answer")) then check if not answer: continue and use that
local answer when constructing Message(role="assistant", content=answer);
likewise bind question = turn.get("question") or "" (or use turn.get("question")
in the user Message) to ensure Message.content receives a str and types are
properly narrowed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: eae84e2b-8c99-4887-973a-929c8b387646
📒 Files selected for processing (7)
vektra-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
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a safeguard_blocked flag in the advanced pipeline to enable early exit from LLM processing when content is blocked by a safeguard. It refines conversation history processing by skipping turns without answers and ensures HTML escaping for context chunks in templates to prevent XSS. Configuration validation is enhanced with range constraints for min_relevance_score, response_token_reserve, and context_chunk_ratio. A critical validator was added to prevent eval_mode from being enabled in production environments, enhancing data privacy. Test configurations were updated to reflect new VEKTRA_EVAL_MODE and VEKTRA_DEBUG_LOG_QUERIES settings, and a minor adjustment was made to a test condition in test_trace_persistence.py. There is no feedback to provide.
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>
Contributor
Author
|
Fixed in 1d2b26e. Return type annotation updated to include safeguard_blocked bool. Also fixed pre_query safeguard early return to include all 6 tuple values. Addresses outside-diff comment on advanced_pipeline.py:202-215. |
Contributor
Author
|
Fixed in 1d2b26e. Bound turn answer to local variable for mypy type narrowing. Addresses outside-diff comment on pipeline.py:61-69. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
vektra-core/src/vektra_core/advanced_pipeline.py (1)
378-380: Please add a regression test for the new hard-block branch.This
allowed=Falsepath is the actual safeguard-bypass fix, but the provided test context only coversfiltered_idsand exception handling. Add explicit coverage for this branch so both the empty-sources short-circuit and trace shape stay locked in.🤖 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 378 - 380, Add a regression test that exercises the hard-block branch where sg_result.allowed is False: call the function/method in advanced_pipeline.py (the code path that produces sg_result) with inputs that trigger a safeguard denial and assert that filtered becomes an empty list, safeguard_blocked is set to True, and the pipeline returns/locks the expected trace shape (i.e., the same short-circuit behavior as the empty-sources case) rather than proceeding to filtered_ids or exception-handling flows; place the test alongside existing tests for filtered_ids/exception handling and name it to indicate it targets the "hard-block / safeguard-bypass" scenario.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@vektra-core/src/vektra_core/advanced_pipeline.py`:
- Line 237: The current early return after pre_query uses the tuple (steps, [],
False, True, query.question, []) which mislabels safeguard denials as
"context_only" outcomes; change that return so the third boolean signals the
safeguard/blocking condition and the fourth boolean is False (i.e., return
(steps, [], True, False, query.question, [])) so denials are not reported as
no-relevant-context; apply the same swap of the two boolean fields in the
analogous branch covering lines 506-530 and ensure you update both return sites
in advanced_pipeline.py where pre_query triggers a block.
---
Nitpick comments:
In `@vektra-core/src/vektra_core/advanced_pipeline.py`:
- Around line 378-380: Add a regression test that exercises the hard-block
branch where sg_result.allowed is False: call the function/method in
advanced_pipeline.py (the code path that produces sg_result) with inputs that
trigger a safeguard denial and assert that filtered becomes an empty list,
safeguard_blocked is set to True, and the pipeline returns/locks the expected
trace shape (i.e., the same short-circuit behavior as the empty-sources case)
rather than proceeding to filtered_ids or exception-handling flows; place the
test alongside existing tests for filtered_ids/exception handling and name it to
indicate it targets the "hard-block / safeguard-bypass" scenario.
🪄 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: 99724b85-4e87-4a27-87d2-3762420be5c1
📒 Files selected for processing (2)
vektra-core/src/vektra_core/advanced_pipeline.pyvektra-core/src/vektra_core/pipeline.py
✅ Files skipped from review due to trivial changes (1)
- vektra-core/src/vektra_core/pipeline.py
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>
Contributor
Author
|
Fixed in b9f8c0c. Added test_post_retrieval_safeguard_denied_blocks_llm: verifies LLM not called when safeguard denies, even in hybrid mode. Addresses nitpick on advanced_pipeline.py:378-380. |
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>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
scripts/eval_e2e.py (1)
138-141: Consider percentage parity across summary rows.For faster scanability, consider adding percentages to
Answered (no ctx),Unanswered, andNo context, similar toAnswered (grounded).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/eval_e2e.py` around lines 138 - 141, Add percentage formatting to the other summary print lines so they match the "Answered (grounded)" row: update the print statements that reference answered_no_ctx, unanswered, and no_context to include their percentage of evaluated using the same format expression (e.g., {variable / evaluated:.0%}) so all four summary rows show counts and percentages consistently.
🤖 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_retrieval.py`:
- Around line 309-312: The module docstring in scripts/eval_retrieval.py
currently claims "No LLM calls are made" but the ArgumentParser flag
"--use-pipeline" (action="store_true") help text correctly states the pipeline
may call the LLM; update the top-level module docstring to reflect that using
the pipeline mode can invoke the full RAG pipeline which may make LLM calls (or
explicitly document both modes: raw retrieval has no LLM calls, pipeline mode
may call an LLM), so users get consistent expectations between the docstring and
the --use-pipeline help text.
- Around line 60-63: The current _normalize function strips diacritics but
leaves Unicode apostrophe variants intact, so ASCII keyword "liberta'" won't
match "libertà"; update _normalize to, after removing combining marks, map
common apostrophe/quote variants (e.g., U+2019 RIGHT SINGLE QUOTATION MARK,
U+2018 LEFT SINGLE QUOTATION MARK, U+02BC MODIFIER LETTER APOSTROPHE, U+FF07
FULLWIDTH APOSTROPHE, etc.) to the plain ASCII apostrophe (') or remove them
consistently so chunk_matches_keywords sees "liberta'" and "libertà" as
equivalent; ensure you modify the _normalize function and keep
chunk_matches_keywords docstring behavior intact.
---
Nitpick comments:
In `@scripts/eval_e2e.py`:
- Around line 138-141: Add percentage formatting to the other summary print
lines so they match the "Answered (grounded)" row: update the print statements
that reference answered_no_ctx, unanswered, and no_context to include their
percentage of evaluated using the same format expression (e.g., {variable /
evaluated:.0%}) so all four summary rows show counts and percentages
consistently.
🪄 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: 23b25481-29b3-4494-b6b2-89d96d9bee4a
📒 Files selected for processing (3)
scripts/eval_e2e.pyscripts/eval_retrieval.pytests/eval/dataset.jsonl
✅ Files skipped from review due to trivial changes (1)
- tests/eval/dataset.jsonl
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>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
vektra-admin/tests/test_admin_turns.py (1)
41-48: Assert the new audit side effect in the success-path test.This test should verify that
background_tasks.add_task(...)is called withaction="conversation_turns_read"and expected metadata; otherwise the new audit behavior can regress unnoticed.Suggested test enhancement
- request.state.request_id = "test-req-id" + request.state.request_id = uuid4() @@ result = await get_conversation_turns(cid, request, background_tasks, key_info) assert result == turns conv_store.get_turns_detail.assert_called_once_with(cid) + background_tasks.add_task.assert_called_once() + _, kwargs = background_tasks.add_task.call_args + assert kwargs["action"] == "conversation_turns_read" + assert kwargs["log_metadata"]["conversation_id"] == str(cid) + assert kwargs["log_metadata"]["turn_count"] == len(turns)As per coding guidelines, authenticated admin flows should have complete audit coverage.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vektra-admin/tests/test_admin_turns.py` around lines 41 - 48, Update the success-path test for get_conversation_turns to also assert the audit side effect: verify that background_tasks.add_task was called with the audit function (or callable) and that the keyword args include action="conversation_turns_read" and the expected metadata (e.g., request_id from request.state and the cid/actor info from key_info); keep the existing assert result == turns and add an assertion using background_tasks.add_task.assert_called_once_with(...) or equivalent to check the action and metadata keys so the admin-audit behavior is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@vektra-admin/src/vektra_admin/api.py`:
- Around line 492-507: Remove the conditional that skips audit logging when
request_id is absent and always queue the audit via background_tasks.add_task
calling _audit.log_event for the admin conversation turns read; include
namespace in log_metadata by reading getattr(request.state, "rls_namespace",
None) and add it alongside conversation_id and turn_count, and still pass
request_id (possibly None) as before so the log record contains namespace and
the event is written for all authenticated admin reads (use symbols:
background_tasks.add_task, _audit.log_event, request_id,
request.state.rls_namespace, log_metadata, key_id, conversation_id, turns).
---
Nitpick comments:
In `@vektra-admin/tests/test_admin_turns.py`:
- Around line 41-48: Update the success-path test for get_conversation_turns to
also assert the audit side effect: verify that background_tasks.add_task was
called with the audit function (or callable) and that the keyword args include
action="conversation_turns_read" and the expected metadata (e.g., request_id
from request.state and the cid/actor info from key_info); keep the existing
assert result == turns and add an assertion using
background_tasks.add_task.assert_called_once_with(...) or equivalent to check
the action and metadata keys so the admin-audit behavior is covered.
🪄 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: 89578c88-a3c0-40a8-bd94-b1b22be24d5e
📒 Files selected for processing (2)
vektra-admin/src/vektra_admin/api.pyvektra-admin/tests/test_admin_turns.py
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>
Contributor
Author
|
Fixed in 8c9a5db. Added audit assertion to test_returns_turns_from_persistent_store. Addresses nitpick on test_admin_turns.py:41-48. |
_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>
fvadicamo
added a commit
that referenced
this pull request
Apr 11, 2026
- Configurable grounding mode strict/hybrid with per-namespace override (FEAT-020) - QueryTrace persistence for all pipelines, streaming and non-streaming (BUG-013) - Reranker scores, eval mode tracing, debug query logging (DEBT-014, DEBT-015, DEBT-009, FEAT-019) - Safeguard bypass fix in hybrid mode, XML escaping, config constraints (PR #57) - Production-ready chatbot widget, RAG prompt restructure, conversation persistence - CI: remove latency step requiring LLM (PR #58) Reviews: 21 inline + 3 outside-diff addressed (CodeRabbit, Gemini, github-code-quality) Tests: all passing, 28/28 CI checks green Refs: PRs #49-#58, milestone v0.4.0
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
Why
Details
CRITICAL: safeguard bypass in hybrid mode
allowed=Falsewas collapsed into "no relevant context"safeguard_blockedflag to AdvancedQueryPipeline (matches SimpleQueryPipeline)HIGH: XML injection in context.j2
<source>tags without escaping|efilter to escape<,>,&HIGH: synthetic [No response] in conversation history
_history_to_messages()injected[No response]for turns with no answerHIGH: config range constraints
min_relevance_score,context_chunk_ratio,response_token_reservehad no boundsge/le/gt/ltconstraints to Field declarationsHIGH: production guard for eval_mode
VEKTRA_EVAL_MODE=truecaptures user text in traces (GDPR)MEDIUM: unreachable code in learn test
test_learn_store_trace_skipped_when_trace_is_noneChecklist
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Chores
CLI / Tools
Tests