fix(observability): persist QueryTrace and add admin conversation turns endpoint

[fvadicamo]

What

Fix for QueryTrace never persisted to database (BUG-013) and conversation observability gaps (DEBT-011).

Problem

AnalyticsService.store_trace() existed and was tested but never called by any pipeline or endpoint. The query_traces table was always empty, making post-hoc diagnosis of query behavior impossible. Additionally, response_id in conversation_turns was never populated, and no admin endpoint existed to read decrypted conversation content.

Solution

• Wire store_trace() in both core and learn API layers (non-streaming and streaming paths)
• Add VEKTRA_ANALYTICS_STORE_TRACES config flag (auto: on in dev, off in prod)
• Populate response_id in conversation_turns via add_turn() keyword arg
• Add trace_from_dict() helper in vektra_shared for SSE trace reconstruction
• Add GET /api/v1/admin/conversations/{id}/turns endpoint with pgp_sym_decrypt
• Add get_turns_detail() to PersistentConversationStore

Also includes backlog updates from analysis session: 13 new entries (BUG-019/020, DEBT-012 to 016, DOCS-009, FEAT-018 to 021) and .env.example context window documentation.

Plan: .s2s/plans/20260328-core-trace-observability.md

Testing

• 9 new unit tests (trace persistence, conversation response_id, admin endpoint)
• End-to-end verified: non-streaming query, streaming query, learn endpoint (Moodle widget), admin turns endpoint, config flag disable
• 596 tests pass, lint clean

Checklist

• Bug resolved (query_traces populated after queries)
• Regression tests added
• No side effects introduced
• Best-effort semantics: DB failure never turns successful query into 500

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Admin API: retrieve detailed conversation turn history (turn numbers, timestamps, question/answer text, model and token metrics).
  • Optional analytics trace persistence: configurable via settings, auto-enabled in development; traces persisted best-effort for streaming and non-streaming queries without impacting responses.

• Chores

  • Improved environment documentation with expanded guidance and concrete model context-window examples.

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds best-effort QueryTrace persistence (streaming and non-streaming), a new admin endpoint to retrieve decrypted conversation turns, propagates response_id through conversation-turn persistence, and introduces a tri-state setting to control trace storage with a development fallback; also updates .env.example documentation and adds trace serialization/deserialization helpers and tests.

Changes

Cohort / File(s)	Summary
Configuration & Examples .env.example	Expanded VEKTRA_LLM_CONTEXT_WINDOW docs with provider lookup guidance, fallback behavior, and concrete model examples.
Settings vektra-shared/src/vektra_shared/config.py	Added optional tri-state `analytics_store_traces: bool
Trace Serialization Helpers vektra-shared/src/vektra_shared/types.py	Added trace_from_dict(data: dict) -> QueryTrace to reconstruct QueryTrace from serialized SSE payloads.
API: Core & Learn vektra-core/src/vektra_core/api.py, vektra-learn/src/vektra_learn/api.py	Extended SSE generators and query handlers to accept analytics_service, db_session_factory, namespace, and store_traces flags; implemented best-effort trace persistence on trace chunks and post-execute traces with exceptions logged and swallowed.
Application Lifespan vektra-app/src/vektra_app/main.py	Initialize app.state.store_traces_enabled from settings (analytics_store_traces with dev fallback) during lifespan.
Conversation Store & Pipeline vektra-core/src/vektra_core/conversation.py, vektra-core/src/vektra_core/pipeline.py, vektra-core/src/vektra_core/advanced_pipeline.py	Propagated response_id into ConversationStore.add_turn(..., response_id=...); PersistentConversationStore persists response_id; added PersistentConversationStore.get_turns_detail() returning decrypted turn details.
Admin API & Tests vektra-admin/src/vektra_admin/api.py, vektra-admin/tests/test_admin_turns.py	Added admin-only GET /api/v1/admin/conversations/{conversation_id}/turns returning list[ConversationTurnDetail] with error cases for missing registry/store and unsupported decryption; added unit tests covering success, 404, and 501 cases.
Trace & Conversation Tests vektra-core/tests/test_trace_persistence.py, vektra-learn/tests/test_trace_persistence.py, vektra-core/tests/test_conversation.py	Added comprehensive tests for trace round-trip (trace_from_dict), persistence behavior when enabled/disabled/failing, and a test confirming InMemoryConversationStore.add_turn accepts response_id.

Sequence Diagrams

sequenceDiagram
    participant Client
    participant API as Query API
    participant Pipeline
    participant AnalyticsSvc as Analytics Service
    participant DB

    rect rgba(100, 150, 200, 0.5)
    Note over Client,DB: Non-Streaming Trace Persistence
    Client->>API: POST /query (store_traces_enabled=true)
    API->>Pipeline: execute(query_req)
    Pipeline-->>API: (response, trace)
    API->>AnalyticsSvc: store_trace(sess, trace, namespace=...)
    AnalyticsSvc->>DB: INSERT trace record
    DB-->>AnalyticsSvc: success
    AnalyticsSvc-->>API: ack
    API-->>Client: response
    end

    rect rgba(200, 100, 150, 0.5)
    Note over Client,DB: Streaming Trace Persistence (SSE)
    Client->>API: POST /query (stream=true)
    API->>Pipeline: stream(query_req)
    loop For each chunk
        Pipeline-->>API: chunk (data)
        alt chunk.type == "trace"
            API->>AnalyticsSvc: store_trace(sess, trace_from_dict(data), namespace=...)
            AnalyticsSvc->>DB: INSERT trace record
            DB-->>AnalyticsSvc: ack
        end
        API-->>Client: chunk (SSE)
    end
    end

    rect rgba(150, 200, 100, 0.5)
    Note over Client,DB: Conversation Turn Retrieval
    Client->>API: GET /admin/conversations/{id}/turns
    API->>API: verify admin scope
    API->>ConversationStore: get_turns_detail(id)
    ConversationStore->>DB: SELECT turns for conversation
    DB-->>ConversationStore: turn records
    ConversationStore-->>API: [decrypted turns + metadata]
    API-->>Client: [ConversationTurnDetail, ...]
    end

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

• feat(analytics): add vektra-analytics component (Phase 2 Wave 3) #30: Implements AnalyticsService and QueryTrace storage API used by the new trace persistence calls (store_trace, ORM/models).
• feat(core): advanced query pipeline, Presidio safeguard, streaming trace #28: Adds _trace_to_dict and richer trace emission for SSE; directly complements the new trace_from_dict and SSE persistence logic.
• release: v0.3.0 #44: Overlaps on QueryTrace handling and conversation/turn persistence changes that mirror this PR’s control paths.

Poem

🐰 A hop, a trace, a careful stitch—

Traces saved without a hitch,
Turns revealed for admin eyes,
Response IDs thread through the ties,
Best-effort hops keep services spry.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically describes the two main changes: QueryTrace persistence and the new admin conversation turns endpoint.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/query-trace-observability

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[gemini-code-assist]

Code Review

This pull request implements QueryTrace persistence and enhances conversation observability across the system. Key changes include the addition of an admin endpoint for retrieving decrypted conversation turns, the integration of best-effort trace storage within the core and learn API layers for both streaming and non-streaming paths, and the propagation of response_id to conversation turns. Additionally, a new implementation plan for trace observability was added and the backlog was updated with several planned features. Feedback identifies a performance improvement opportunity by removing a redundant database check for conversation existence in the conversation store.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

vektra-core/src/vektra_core/pipeline.py (1)

562-566: Make conversation-turn writes best-effort (consistency with advanced pipeline).

Consider guarding these add_turn writes so a storage error does not fail an otherwise successful response.

Proposed refactor

-        if query.conversation_id is not None:
-            await self._conversation_store.add_turn(
-                query.conversation_id,
-                query.question,
-                answer,
-                response_id=response_id,
-            )
+        if query.conversation_id is not None:
+            try:
+                await self._conversation_store.add_turn(
+                    query.conversation_id,
+                    query.question,
+                    answer,
+                    response_id=response_id,
+                )
+            except Exception as exc:
+                log.warning("conversation_turn_store_failed", error=str(exc))

-        if query.conversation_id is not None and full_answer:
-            await self._conversation_store.add_turn(
-                query.conversation_id,
-                query.question,
-                full_answer,
-                response_id=response_id,
-            )
+        if query.conversation_id is not None and full_answer:
+            try:
+                await self._conversation_store.add_turn(
+                    query.conversation_id,
+                    query.question,
+                    full_answer,
+                    response_id=response_id,
+                )
+            except Exception as exc:
+                log.warning("conversation_turn_store_failed", error=str(exc))

Also applies to: 871-875

🤖 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 562 - 566, The call to
add_turn when writing conversation turns (the invocation that passes
query.conversation_id, query.question, answer, response_id) should be made
best-effort so storage errors don't break a successful response; wrap the
add_turn call in a try/except that catches storage/IO errors, log the exception
(with context including conversation_id and response_id) and do not re-raise,
and apply the same pattern to the other add_turn site around the lines
referenced (the second occurrence that uses the same parameters).

vektra-core/tests/test_trace_persistence.py (1)

92-262: Mirror these persistence cases for vektra_learn.api.course_query.

This suite only pins vektra_core.api.query, but the PR adds near-identical store_trace logic in the learn endpoint too. The hidden unit failures show that path already diverged, so please add the same enabled/disabled/failure cases there to keep the two handlers in sync.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@vektra-core/tests/test_trace_persistence.py` around lines 92 - 262, Add three
tests mirroring the ones in vektra_core/tests/test_trace_persistence.py but
targeting vektra_learn.api.course_query: create
test_store_trace_called_when_enabled, test_store_trace_not_called_when_disabled,
and test_store_trace_failure_does_not_propagate for the course_query handler;
each should mock an AsyncMock analytics_service with store_trace, a mock
pipeline that returns (response, trace) using _make_trace(), set
request.app.state.registry to return the pipeline and safeguard mocks, toggle
app_state.store_traces_enabled, attach app_state.analytics_service and
app_state.db_session_factory (for the enabled and failure cases), call
vektra_learn.api.course_query(body, request, key_info) and assert store_trace
called with the trace and namespace when enabled, not called when disabled, and
that exceptions raised by analytics_service.store_trace do not propagate
(handler still returns a result) for the failure case.

🤖 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-learn/src/vektra_learn/api.py`:
- Around line 597-608: The persistence block assumes trace is present and
dereferences trace.response_id in the exception handler; update the logic inside
the if _store_traces and _analytics_svc and _db_factory: guard the persistence
attempt with an explicit check that trace is not None (e.g., only enter async
with _db_factory() when trace is truthy) and change the except logger call
(structlog.get_logger(__name__).warning) to avoid dereferencing
trace.response_id by using a safe lookup (e.g., include
response_id=getattr(trace, "response_id", None) or a descriptive message when
trace is missing) so a swallowed analytics error cannot raise when trace is
None.

---

Nitpick comments:
In `@vektra-core/src/vektra_core/pipeline.py`:
- Around line 562-566: The call to add_turn when writing conversation turns (the
invocation that passes query.conversation_id, query.question, answer,
response_id) should be made best-effort so storage errors don't break a
successful response; wrap the add_turn call in a try/except that catches
storage/IO errors, log the exception (with context including conversation_id and
response_id) and do not re-raise, and apply the same pattern to the other
add_turn site around the lines referenced (the second occurrence that uses the
same parameters).

In `@vektra-core/tests/test_trace_persistence.py`:
- Around line 92-262: Add three tests mirroring the ones in
vektra_core/tests/test_trace_persistence.py but targeting
vektra_learn.api.course_query: create test_store_trace_called_when_enabled,
test_store_trace_not_called_when_disabled, and
test_store_trace_failure_does_not_propagate for the course_query handler; each
should mock an AsyncMock analytics_service with store_trace, a mock pipeline
that returns (response, trace) using _make_trace(), set
request.app.state.registry to return the pipeline and safeguard mocks, toggle
app_state.store_traces_enabled, attach app_state.analytics_service and
app_state.db_session_factory (for the enabled and failure cases), call
vektra_learn.api.course_query(body, request, key_info) and assert store_trace
called with the trace and namespace when enabled, not called when disabled, and
that exceptions raised by analytics_service.store_trace do not propagate
(handler still returns a result) for the failure case.

🪄 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: 76b5f269-0cf4-4c30-8d61-b543017df222

📥 Commits

Reviewing files that changed from the base of the PR and between e9d7bd0 and 75266b6.

⛔ Files ignored due to path filters (2)

• .s2s/BACKLOG.md is excluded by !.s2s/**
• .s2s/plans/20260328-core-trace-observability.md is excluded by !.s2s/**

📒 Files selected for processing (13)

• .env.example
• vektra-admin/src/vektra_admin/api.py
• vektra-admin/tests/test_admin_turns.py
• vektra-app/src/vektra_app/main.py
• vektra-core/src/vektra_core/advanced_pipeline.py
• vektra-core/src/vektra_core/api.py
• vektra-core/src/vektra_core/conversation.py
• vektra-core/src/vektra_core/pipeline.py
• vektra-core/tests/test_conversation.py
• vektra-core/tests/test_trace_persistence.py
• vektra-learn/src/vektra_learn/api.py
• vektra-shared/src/vektra_shared/config.py
• vektra-shared/src/vektra_shared/types.py

[fvadicamo]

Fixed in b6262cd. Addresses CodeRabbit nitpick on pipeline.py:562-566,871-875: wrapped SimpleQueryPipeline add_turn calls in try/except for best-effort consistency with AdvancedQueryPipeline. Also added learn endpoint trace persistence tests (test_trace_persistence.py in vektra-learn).

[coderabbitai]

🧹 Nitpick comments (1)

vektra-learn/tests/test_trace_persistence.py (1)

37-68: Consider adding a return type hint for clarity.

The function returns a tuple (app_state, trace, response) but this isn't documented. A type hint would improve discoverability.

♻️ Suggested improvement

 def _make_app_state(
     *,
     store_traces: bool,
     analytics_svc: AsyncMock | None = None,
     db_factory: MagicMock | None = None,
     pipeline: AsyncMock | None = None,
-) -> MagicMock:
+) -> tuple[MagicMock, QueryTrace, MagicMock]:
     """Build a mock app.state with registry and services."""

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@vektra-learn/tests/test_trace_persistence.py` around lines 37 - 68, The
helper _make_app_state currently returns a tuple (app_state, trace, response)
but lacks a return type hint; update its signature to include an explicit return
type such as -> tuple[MagicMock, TraceType, ResponseType] (or the concrete test
types used) so callers and readers know the expected types; ensure to import or
reference the correct types (e.g., MagicMock, the Trace and Response classes or
test fixtures) and update any type aliases if needed to keep static typing
consistent with registry, pipeline, and db_factory mocks.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@vektra-learn/tests/test_trace_persistence.py`:
- Around line 37-68: The helper _make_app_state currently returns a tuple
(app_state, trace, response) but lacks a return type hint; update its signature
to include an explicit return type such as -> tuple[MagicMock, TraceType,
ResponseType] (or the concrete test types used) so callers and readers know the
expected types; ensure to import or reference the correct types (e.g.,
MagicMock, the Trace and Response classes or test fixtures) and update any type
aliases if needed to keep static typing consistent with registry, pipeline, and
db_factory mocks.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 31863875-5e9f-4371-85bf-65883571a253

📥 Commits

Reviewing files that changed from the base of the PR and between 75266b6 and b6262cd.

📒 Files selected for processing (4)

• vektra-core/src/vektra_core/api.py
• vektra-core/src/vektra_core/pipeline.py
• vektra-learn/src/vektra_learn/api.py
• vektra-learn/tests/test_trace_persistence.py

🚧 Files skipped from review as they are similar to previous changes (1)

• vektra-core/src/vektra_core/pipeline.py

[fvadicamo]

Won't fix: CodeRabbit nitpick on vektra-learn/tests/test_trace_persistence.py:37-68 (return type hint on test helper). The function is a test fixture helper, adding explicit tuple type hints provides minimal value here.