feat(knowledge): inline citations & sources + simplified embeddings config UX

[dolev31]

Summary

Ships inline citations & sources for the knowledge engine end-to-end, plus a ground-up simplification of the knowledge / embeddings configuration UX.

Citations & sources — Closes #154, Closes #155

• Backend data model ([Design]: Citations & sources data model for knowledge retrieval #154): a thread-scoped SourceLedger stamps a stable, content-hashed cite_id on every retrieved chunk at both retrieval seams (client.search_envelope + routes.py::search, covering chat / sandbox / MCP / SDK). The id is carried on the wire and resolved by FinalAnswerNode into per-message [1]..[k] numbers with self-contained source snapshots that survive history replay. Marker parsing is hardened against model drift (ASCII + fullwidth/CJK bracket families) with a code-aware canary that logs unsupported bracket styles instead of silently dropping them.
• UI ([Design]: Inline citation markers + hover previews + sources dropdown (UI) #155): a shared <cuga-cite> web component renders [n] markers as clickable chips with a hover preview (filename · page · snippet). The preview renders in the browser top layer (Popover API) so it isn't clipped inside markdown tables. A Sources panel/footer enumerates the cited documents; clicking a chip opens the source. Works in both the carbon-chat web UI and the extension CardManager surface.
• SDK: CugaAgent(enable_knowledge=True, enable_citations=True) → InvokeResult.sources ([{n, cite_id, filename, page, section_path, scope, snippet, score, query}]); per-agent and per-session citation toggles.

Knowledge / embeddings configuration UX

• Reworked the embeddings/provider setup for progressive disclosure: the entire embedder config now lives under Advanced settings (hidden by default); the provider picker is a native grouped Select (no clipped labels, no confusing non-clickable rows); and the re-index prompt sits prominently at the seam directly under the Retrieval Profile cards, decoupled from the now-hidden config.
• Per-provider field visibility, required-ness, and the security-critical field reset (prevents a stale base_url sending a key to the wrong server) all derive from a single providerSpec source of truth.
• Fixes along the way: non-ASCII (e.g. Hebrew) document filenames on "Open document"; reindex-banner persistence; "Modified"-tag semantics; a temporal-dead-zone render crash that blanked the config modal; adopt-on-empty-KB semantics; unit-test DB isolation.

Verification

• Reviews: ponytail (over-engineering), high-effort correctness, and an xhigh ship-review — all confirmed findings fixed.
• tsc clean; citation unit + integration tests green (a deterministic real search → resolve → sources net, plus an opt-in live-model smoke); served frontend bundle rebuilt.
• Branch is up to date / merges into main with no conflicts.

Follow-ups (tracked in #444)

Live-model citation e2e in CI, storage-at-scale validation (pgvector / Milvus), ingest streaming & performance, and the chunk_size-vs-embedder cap UX.

Part of the knowledge production-grade epic #444.

---

@sami-marreed — would you mind giving this a review when you get a chance? 🙏 It's a biggish one, so happy to walk you through any area (or split it) if that's easier. Thanks so much!

Summary by CodeRabbit

• New Features

  • Added numbered citations and source metadata to knowledge-based answers.
  • Added clickable citation chips, source lists, previews, and a Sources side panel.
  • Added per-conversation citation controls and agent-level citation settings.
  • SDK responses now include answer sources.
  • Added improved embedding health visibility and session knowledge settings.

• Bug Fixes

  • Improved reindexing safeguards, task tracking, document handling, and stream-event persistence.
  • Improved citation handling across streamed, resumed, and fallback answers.

Citation correctness (added on this branch)

Fixes #505 — cite_ids from an earlier turn no longer resolve in a later answer (turn-scoping), and stream_events is now cumulative (durable reload + citation rehydration). Reuse follow-ups: #506 (content verification) and #507 (provenance binding).

[coderabbitai]

Caution

Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted.

Error details

putComment timed out

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/cuga/sdk.py (1)

2439-2466: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Fallback citation resolution bypasses the citations-disabled gate.

FinalAnswerNode.apply_citation_resolution only resolves via a live ledger when effective_citations_enabled(thread_id) is true, otherwise it strips markers (ledger=None). This fallback path (hit when final_answer is empty, e.g. reasoning models) skips that check and always does get_ledger(thread_id, create=False) — if a ledger already exists for the thread from an earlier turn, markers get resolved into display numbers even when citations were subsequently disabled for this thread/session, diverging from the primary path's documented and tested behavior.

🐛 Proposed fix

             from cuga.backend.knowledge.sources import (
+                effective_citations_enabled,
                 get_ledger,
                 has_citation_markers,
                 resolve_citations,
             )

             if final_answer and has_citation_markers(final_answer):
-                ledger = get_ledger(thread_id, create=False)
+                ledger = (
+                    get_ledger(thread_id, create=False)
+                    if thread_id and effective_citations_enabled(thread_id)
+                    else None
+                )
                 final_answer, fallback_sources = resolve_citations(final_answer, ledger)
             else:
                 fallback_sources = []

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/cuga/sdk.py` around lines 2439 - 2466, The fallback citation cleanup in
src/cuga/sdk.py is bypassing the citations-disabled behavior used by
FinalAnswerNode.apply_citation_resolution. Update the fallback path that
extracts final_answer from the last AI chat message so it first checks
effective_citations_enabled(thread_id) and only calls
get_ledger(...)/resolve_citations when citations are enabled; otherwise strip
citation markers or leave them unresolved to match the primary path’s behavior.
Keep the fix localized around the fallback_sources/final_answer logic and reuse
the same citation-resolution helpers already used by FinalAnswerNode.

🧹 Nitpick comments (10)

src/cuga/backend/knowledge/client.py (1)

240-260: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract the repeated citation-annotation gate into a helper.

The if citations_enabled_for(...): annotate_envelope_with_citations(...) block is duplicated verbatim between the multi-scope (_run_multi) and single-scope paths. Extracting a small helper (e.g. _maybe_annotate_citations(envelope, results, config, thread_id, query)) would keep the gate logic in one place as citation rules evolve.

♻️ Proposed refactor

+    def _maybe_annotate_citations(envelope, results):
+        if citations_enabled_for(self._engine._config, thread_id):
+            annotate_envelope_with_citations(envelope, results, thread_id=thread_id, query=query)
+
     async def _run_multi(scope_requested: str, fallback_from: str | None) -> dict[str, Any]:
         ...
             envelope = build_retrieval_envelope(...)
-            if citations_enabled_for(self._engine._config, thread_id):
-                annotate_envelope_with_citations(envelope, results, thread_id=thread_id, query=query)
+            _maybe_annotate_citations(envelope, results)
             return envelope
     ...
         envelope = build_retrieval_envelope(...)
-        if citations_enabled_for(self._engine._config, thread_id):
-            annotate_envelope_with_citations(envelope, results, thread_id=thread_id, query=query)
+        _maybe_annotate_citations(envelope, results)
         return envelope

Also applies to: 295-307

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/cuga/backend/knowledge/client.py` around lines 240 - 260, The
citation-annotation gate is duplicated in the multi-scope and single-scope
retrieval paths, so centralize it in a small helper to keep the rule in one
place. Add a private helper near `_run_multi`/the single-scope equivalent, such
as `_maybe_annotate_citations(envelope, results, thread_id, query)`, that checks
`citations_enabled_for(self._engine._config, thread_id)` and calls
`annotate_envelope_with_citations(...)`; then replace both inline `if` blocks
with that helper.

tests/unit/test_knowledge_reindex_busy_mapping.py (1)

20-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Mutable class-level defaults could leak state across instances.

_reindex_in_progress/_reindex_deferred are shared set() objects at the class level; any instance mutation (.add()/.discard()) would be visible to every other instance/subclass unless overridden in __init__. Not exercised today, but worth an __init__ assignment to avoid future cross-test leakage.

♻️ Suggested fix

 class _FakeEngineBase:
-    _reindex_in_progress: set = set()
-    _reindex_deferred: set = set()
     _files_dir = None
+
+    def __init__(self):
+        self._reindex_in_progress: set = set()
+        self._reindex_deferred: set = set()

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/unit/test_knowledge_reindex_busy_mapping.py` around lines 20 - 37, The
_FakeEngineBase test helper uses mutable class-level defaults for
_reindex_in_progress and _reindex_deferred, which can leak state across
instances and subclasses. Move these set initializations into an __init__ method
on _FakeEngineBase so each instance gets its own fresh sets, and keep the
existing _Config/vector_config_hash behavior unchanged for the reindex path
being tested.

Source: Linters/SAST tools

src/cuga/backend/knowledge/engine.py (1)

4193-4302: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value

zip() without strict=True in reindex worker.

zip(file_list, task_ids) (lines 4240, 4261) will silently truncate if the two lists ever diverge in length. Traced the construction: task_ids is appended 1:1 from file_list right before the worker is defined, and any exception during that loop aborts before the worker starts — so this can't currently misfire. Adding strict=True is cheap defensive insurance against a future refactor breaking that invariant silently.

♻️ Suggested tweak

-                                for fp, tid in zip(file_list, task_ids)
+                                for fp, tid in zip(file_list, task_ids, strict=True)

and similarly at the timeout-handling zip on line 4261.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/cuga/backend/knowledge/engine.py` around lines 4193 - 4302, The reindex
worker uses zip(file_list, task_ids) in two places, which can silently truncate
if the lists ever diverge. Update the pairing logic in the reindex flow around
_reindex_worker and the timeout cleanup to use a strict zip variant so
mismatches fail fast instead of dropping entries, preserving the intended 1:1
mapping between file_list and task_ids.

Source: Linters/SAST tools

tests/unit/conftest.py (1)

29-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider logging the swallowed exception instead of silent pass.

Static analysis flags the bare except Exception: pass blocks. Since this fixture exists specifically to prevent state leaking across tests, silently swallowing an invalidate failure could hide the exact class of bug it's meant to guard against.

♻️ Suggested tweak

     try:
         facade.get_storage().invalidate_relational_stores()
-    except Exception:
-        pass
+    except Exception as e:
+        logger.debug(f"invalidate_relational_stores (setup) skipped: {e}")
     yield
     try:
         facade.get_storage().invalidate_relational_stores()
-    except Exception:
-        pass
+    except Exception as e:
+        logger.debug(f"invalidate_relational_stores (teardown) skipped: {e}")

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/unit/conftest.py` around lines 29 - 37, The fixture in conftest.py is
swallowing failures from facade.get_storage().invalidate_relational_stores()
with bare except blocks, which hides useful debugging context. Update the
teardown/setup around the shared invalidate_relational_stores call to catch the
exception and log it through the existing test logging mechanism (or re-raise
after logging if appropriate), so failures are visible instead of being silently
ignored.

Source: Linters/SAST tools

tests/unit/test_knowledge_chunking_tokenizer_accessor.py (1)

154-163: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Replace assert False with pytest.fail/raised assertion.

Static analysis flags both: a blind except Exception (BLE001) and two assert False statements (B011) that are stripped under python -O, silently defeating the intended failure checks.

♻️ Proposed fix

-        try:
-            dataclasses.replace(tok, kind="hf")  # via replace is fine
-        except Exception:
-            assert False, "replace must work on frozen dataclass"
+        dataclasses.replace(tok, kind="hf")  # via replace is fine; let any error surface naturally
         # Direct mutation must fail.
         try:
             tok.kind = "hf"  # type: ignore[misc]
         except dataclasses.FrozenInstanceError:
             return
-        assert False, "direct attribute mutation should have raised"
+        pytest.fail("direct attribute mutation should have raised")

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/unit/test_knowledge_chunking_tokenizer_accessor.py` around lines 154 -
163, The frozen-dataclass test in the token accessor check uses blind exception
handling and assert False fallbacks; update the test body to use explicit
failure signaling instead. In the test around dataclasses.replace(tok,
kind="hf") and the direct tok.kind mutation, replace the broad except Exception
and both assert False statements with pytest.fail or a raised assertion so
failures remain visible under optimization and the intended checks are explicit.

Source: Linters/SAST tools

src/cuga/backend/knowledge/sources.py (1)

352-362: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Add strict=True to zip() to enforce the chunk-result correspondence invariant.

The docstring states envelope["results"][i] must correspond to results[i], but zip(chunks, results) silently truncates on length mismatch. If envelope.get("results") returns None or a shorter list than results, some chunks would be left unstamped without any error. strict=True turns this silent failure into a loud ValueError.

♻️ Proposed fix

-        for chunk, result in zip(chunks, results):
+        for chunk, result in zip(chunks, results, strict=True):

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/cuga/backend/knowledge/sources.py` around lines 352 - 362, Add
strict=True to the zip in the citation stamping logic so the chunk/result
pairing invariant is enforced. In the function that gets the ledger and iterates
over chunks and results, change the `zip(chunks, results)` usage to a strict zip
so a length mismatch raises immediately instead of silently truncating and
leaving some chunks unstamped.

src/cuga/backend/knowledge/routes.py (1)

760-771: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Missing exception chaining (raise ... from).

Static analysis (Ruff B904) flags three raise HTTPException(...) statements inside except blocks without from err/from None (Lines 760-771, 848-851, 862-863). This is a minor observability nit — without chaining, the original traceback context is dropped from logs, making it harder to debug the underlying cause (e.g. a malformed JSON body vs. a deserialization bug).

♻️ Suggested fix (session settings excerpt)

     try:
         body = await request.json()
-    except Exception:
-        raise HTTPException(status_code=400, detail="request body must be a JSON object")
+    except Exception as exc:
+        raise HTTPException(status_code=400, detail="request body must be a JSON object") from exc
     ...
     except ValueError as exc:
-        raise HTTPException(status_code=400, detail=str(exc))
+        raise HTTPException(status_code=400, detail=str(exc)) from exc

Also applies to: 848-863

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/cuga/backend/knowledge/routes.py` around lines 760 - 771, Add explicit
exception chaining to the HTTPException raises inside the except blocks flagged
by Ruff B904 in routes.py. Update the reindex/delete handler and the other
HTTPException paths in the same module so the raised HTTPException preserves the
caught exception context using the original exception variable (or suppresses it
with from None where appropriate). Use the surrounding except blocks around
ReindexInProgressError and the other affected route handlers to locate and fix
all three raise sites consistently.

Source: Linters/SAST tools

src/frontend_workspaces/frontend/src/carbon-chat/customSendMessage.ts (1)

487-575: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Wrap the Answer/FinalAnswer case body in a block to clear the Biome noSwitchDeclarations errors.

Biome flags the new let/const declarations (answerSources, answerCompleteItem, answerGenericItems, sourcesItem, finalResponse) as errors because their scope leaks across the other case clauses. There is no runtime fallthrough today (the case ends in return), but these are lint errors that can fail CI. Wrapping the case body in { } — as the sibling customLoadHistory.ts already does — resolves them.

♻️ Proposed block-scoping

         case "Answer":
-        case "FinalAnswer":
+        case "FinalAnswer": {
           console.log("Received Answer event, finalizing message...");

           let answerText = accumulatedText || "";
           let answerSources: MessageSource[] = [];
           ...
           instance.messaging.addMessageChunk(finalResponse);
           
           console.log("Message finalized successfully");
           return; // Exit after finalizing
+        }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/frontend_workspaces/frontend/src/carbon-chat/customSendMessage.ts` around
lines 487 - 575, The Answer/FinalAnswer branch in customSendMessage’s switch
needs block scoping to satisfy Biome’s noSwitchDeclarations rule. Wrap the
entire case body in its own braces so the local declarations like answerSources,
answerCompleteItem, answerGenericItems, sourcesItem, and finalResponse are
scoped to that branch only, matching the pattern used in customLoadHistory.ts
and preventing lint errors from leaking into other case clauses.

Source: Linters/SAST tools

src/cuga/backend/server/conversation_history.py (1)

435-467: 🧹 Nitpick | 🔵 Trivial

Consider scheduling gc_ephemeral_stream_events periodically, not just at startup.

It currently only runs once during lifespan startup (main.py ~1042-1047). For long-running deployments, orphaned Try-It-Out rows will keep accumulating between restarts. Consider wiring it into the existing periodic background-task pattern (same style as the knowledge engine's background maintenance tasks).

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/cuga/backend/server/conversation_history.py` around lines 435 - 467,
gc_ephemeral_stream_events is only invoked during startup, so orphaned
Try-It-Out stream_events can accumulate in long-running deployments. Wire
conversation_history.gc_ephemeral_stream_events into the existing periodic
background-task mechanism used elsewhere (for example, the knowledge engine
maintenance pattern) so it runs on a schedule, while keeping the existing
startup call as-is if desired.

src/cuga/backend/server/manage_routes/knowledge_routes.py (1)

95-108: 🩺 Stability & Availability | 🔵 Trivial | ⚖️ Poor tradeoff

asyncio.wait_for won't stop the embed thread — a hung/unreachable endpoint leaks a shared-executor worker.

On timeout, wait_for cancels the future wrapping to_thread(_do_test), but the underlying thread keeps running create_embeddings/embed_query until the network call itself returns. Since to_thread uses the loop's default ThreadPoolExecutor (shared with e.g. patch_draft_knowledge's apply_knowledge_config), repeated Test-connection calls against a hanging host can pin all workers and stall unrelated request-thread offloads. Consider running _do_test on a dedicated single-worker executor for this endpoint, and/or documenting the leak so operators size the pool accordingly.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/cuga/backend/server/manage_routes/knowledge_routes.py` around lines 95 -
108, The timeout handling in the Test-connection flow does not actually stop the
underlying embedding work, so a hung endpoint can keep a shared executor thread
busy. Update the async test path in knowledge_routes’ endpoint around the
_do_test / asyncio.wait_for / asyncio.to_thread call so it uses a dedicated
single-worker executor (or equivalent isolated execution) instead of the loop
default ThreadPoolExecutor, and keep the timeout response behavior intact.
Reference the _do_test helper and the wait_for/to_thread block so the fix stays
localized to this route and does not affect other offloaded work like
apply_knowledge_config.

Source: Linters/SAST tools

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@src/cuga/sdk.py`:
- Around line 2439-2466: The fallback citation cleanup in src/cuga/sdk.py is
bypassing the citations-disabled behavior used by
FinalAnswerNode.apply_citation_resolution. Update the fallback path that
extracts final_answer from the last AI chat message so it first checks
effective_citations_enabled(thread_id) and only calls
get_ledger(...)/resolve_citations when citations are enabled; otherwise strip
citation markers or leave them unresolved to match the primary path’s behavior.
Keep the fix localized around the fallback_sources/final_answer logic and reuse
the same citation-resolution helpers already used by FinalAnswerNode.

---

Nitpick comments:
In `@src/cuga/backend/knowledge/client.py`:
- Around line 240-260: The citation-annotation gate is duplicated in the
multi-scope and single-scope retrieval paths, so centralize it in a small helper
to keep the rule in one place. Add a private helper near `_run_multi`/the
single-scope equivalent, such as `_maybe_annotate_citations(envelope, results,
thread_id, query)`, that checks `citations_enabled_for(self._engine._config,
thread_id)` and calls `annotate_envelope_with_citations(...)`; then replace both
inline `if` blocks with that helper.

In `@src/cuga/backend/knowledge/engine.py`:
- Around line 4193-4302: The reindex worker uses zip(file_list, task_ids) in two
places, which can silently truncate if the lists ever diverge. Update the
pairing logic in the reindex flow around _reindex_worker and the timeout cleanup
to use a strict zip variant so mismatches fail fast instead of dropping entries,
preserving the intended 1:1 mapping between file_list and task_ids.

In `@src/cuga/backend/knowledge/routes.py`:
- Around line 760-771: Add explicit exception chaining to the HTTPException
raises inside the except blocks flagged by Ruff B904 in routes.py. Update the
reindex/delete handler and the other HTTPException paths in the same module so
the raised HTTPException preserves the caught exception context using the
original exception variable (or suppresses it with from None where appropriate).
Use the surrounding except blocks around ReindexInProgressError and the other
affected route handlers to locate and fix all three raise sites consistently.

In `@src/cuga/backend/knowledge/sources.py`:
- Around line 352-362: Add strict=True to the zip in the citation stamping logic
so the chunk/result pairing invariant is enforced. In the function that gets the
ledger and iterates over chunks and results, change the `zip(chunks, results)`
usage to a strict zip so a length mismatch raises immediately instead of
silently truncating and leaving some chunks unstamped.

In `@src/cuga/backend/server/conversation_history.py`:
- Around line 435-467: gc_ephemeral_stream_events is only invoked during
startup, so orphaned Try-It-Out stream_events can accumulate in long-running
deployments. Wire conversation_history.gc_ephemeral_stream_events into the
existing periodic background-task mechanism used elsewhere (for example, the
knowledge engine maintenance pattern) so it runs on a schedule, while keeping
the existing startup call as-is if desired.

In `@src/cuga/backend/server/manage_routes/knowledge_routes.py`:
- Around line 95-108: The timeout handling in the Test-connection flow does not
actually stop the underlying embedding work, so a hung endpoint can keep a
shared executor thread busy. Update the async test path in knowledge_routes’
endpoint around the _do_test / asyncio.wait_for / asyncio.to_thread call so it
uses a dedicated single-worker executor (or equivalent isolated execution)
instead of the loop default ThreadPoolExecutor, and keep the timeout response
behavior intact. Reference the _do_test helper and the wait_for/to_thread block
so the fix stays localized to this route and does not affect other offloaded
work like apply_knowledge_config.

In `@src/frontend_workspaces/frontend/src/carbon-chat/customSendMessage.ts`:
- Around line 487-575: The Answer/FinalAnswer branch in customSendMessage’s
switch needs block scoping to satisfy Biome’s noSwitchDeclarations rule. Wrap
the entire case body in its own braces so the local declarations like
answerSources, answerCompleteItem, answerGenericItems, sourcesItem, and
finalResponse are scoped to that branch only, matching the pattern used in
customLoadHistory.ts and preventing lint errors from leaking into other case
clauses.

In `@tests/unit/conftest.py`:
- Around line 29-37: The fixture in conftest.py is swallowing failures from
facade.get_storage().invalidate_relational_stores() with bare except blocks,
which hides useful debugging context. Update the teardown/setup around the
shared invalidate_relational_stores call to catch the exception and log it
through the existing test logging mechanism (or re-raise after logging if
appropriate), so failures are visible instead of being silently ignored.

In `@tests/unit/test_knowledge_chunking_tokenizer_accessor.py`:
- Around line 154-163: The frozen-dataclass test in the token accessor check
uses blind exception handling and assert False fallbacks; update the test body
to use explicit failure signaling instead. In the test around
dataclasses.replace(tok, kind="hf") and the direct tok.kind mutation, replace
the broad except Exception and both assert False statements with pytest.fail or
a raised assertion so failures remain visible under optimization and the
intended checks are explicit.

In `@tests/unit/test_knowledge_reindex_busy_mapping.py`:
- Around line 20-37: The _FakeEngineBase test helper uses mutable class-level
defaults for _reindex_in_progress and _reindex_deferred, which can leak state
across instances and subclasses. Move these set initializations into an __init__
method on _FakeEngineBase so each instance gets its own fresh sets, and keep the
existing _Config/vector_config_hash behavior unchanged for the reindex path
being tested.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: f3a68c7f-27f8-46d9-a55c-5875c929ce52

📥 Commits

Reviewing files that changed from the base of the PR and between df17367 and 029a17d.

⛔ Files ignored due to path filters (4)

• src/cuga/frontend/dist/index.html is excluded by !**/dist/**
• src/cuga/frontend/dist/main.5fd7de660b36475e3f99.js is excluded by !**/dist/**
• src/cuga/frontend/dist/main.8f3239b467c01791bc29.js is excluded by !**/dist/**
• src/cuga/frontend/dist/vendors.254bfd43ed6c401b5770.js is excluded by !**/dist/**

📒 Files selected for processing (69)

• docs/superpowers/plans/2026-07-06-knowledge-citations.md
• scripts/build_frontend.sh
• src/cuga/backend/cuga_graph/nodes/answer/final_answer.py
• src/cuga/backend/cuga_graph/nodes/answer/final_answer_agent/prompts/system.jinja2
• src/cuga/backend/cuga_graph/nodes/cuga_lite/adapter/prepare_node.py
• src/cuga/backend/cuga_graph/policy/enactment.py
• src/cuga/backend/cuga_graph/state/agent_state.py
• src/cuga/backend/cuga_graph/utils/agent_loop.py
• src/cuga/backend/knowledge/awareness.py
• src/cuga/backend/knowledge/client.py
• src/cuga/backend/knowledge/config.py
• src/cuga/backend/knowledge/engine.py
• src/cuga/backend/knowledge/envelope.py
• src/cuga/backend/knowledge/routes.py
• src/cuga/backend/knowledge/sources.py
• src/cuga/backend/server/conversation_history.py
• src/cuga/backend/server/main.py
• src/cuga/backend/server/manage_routes/__init__.py
• src/cuga/backend/server/manage_routes/config_routes.py
• src/cuga/backend/server/manage_routes/knowledge_reindex.py
• src/cuga/backend/server/manage_routes/knowledge_routes.py
• src/cuga/cli/main.py
• src/cuga/configurations/knowledge/knowledge_settings.toml
• src/cuga/sdk.py
• src/frontend_workspaces/agentic_chat/package.json
• src/frontend_workspaces/agentic_chat/src/App.tsx
• src/frontend_workspaces/agentic_chat/src/CardManager.tsx
• src/frontend_workspaces/agentic_chat/src/KnowledgeConfig.tsx
• src/frontend_workspaces/agentic_chat/src/KnowledgeSidePanel.css
• src/frontend_workspaces/agentic_chat/src/KnowledgeSidePanel.tsx
• src/frontend_workspaces/agentic_chat/src/citations/MessageSources.tsx
• src/frontend_workspaces/agentic_chat/src/citations/SourcesPanel.css
• src/frontend_workspaces/agentic_chat/src/citations/SourcesPanel.tsx
• src/frontend_workspaces/agentic_chat/src/citations/citeElement.ts
• src/frontend_workspaces/agentic_chat/src/citations/index.ts
• src/frontend_workspaces/agentic_chat/src/citations/injectCitations.test.ts
• src/frontend_workspaces/agentic_chat/src/citations/injectCitations.ts
• src/frontend_workspaces/agentic_chat/src/citations/types.ts
• src/frontend_workspaces/agentic_chat/vite.config.ts
• src/frontend_workspaces/frontend/src/ChatLanding.tsx
• src/frontend_workspaces/frontend/src/ManagePage.tsx
• src/frontend_workspaces/frontend/src/api.ts
• src/frontend_workspaces/frontend/src/carbon-chat/CarbonChat.tsx
• src/frontend_workspaces/frontend/src/carbon-chat/carbonChatHelpers.ts
• src/frontend_workspaces/frontend/src/carbon-chat/customLoadHistory.ts
• src/frontend_workspaces/frontend/src/carbon-chat/customSendMessage.ts
• tests/integration/test_knowledge_citation_e2e.py
• tests/integration/test_knowledge_citation_e2e_live.py
• tests/unit/conftest.py
• tests/unit/test_citation_resolver.py
• tests/unit/test_citation_settings.py
• tests/unit/test_envelope_citations.py
• tests/unit/test_ephemeral_stream_events.py
• tests/unit/test_final_answer_citations.py
• tests/unit/test_knowledge_chunker_hf_tokenizer.py
• tests/unit/test_knowledge_chunking_tokenizer_accessor.py
• tests/unit/test_knowledge_cli_publish.py
• tests/unit/test_knowledge_reindex_busy_mapping.py
• tests/unit/test_knowledge_reindex_in_progress_guard.py
• tests/unit/test_knowledge_resplit_unit_mismatch.py
• tests/unit/test_knowledge_routes.py
• tests/unit/test_knowledge_text_splitter_tiktoken_canary.py
• tests/unit/test_knowledge_tokenizer_w5i1_synth_fixes.py
• tests/unit/test_manage_publish_sync.py
• tests/unit/test_sdk_citations.py
• tests/unit/test_server_citation_egress.py
• tests/unit/test_session_citation_override.py
• tests/unit/test_session_settings_routes.py
• tests/unit/test_source_ledger.py

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

docs/superpowers/plans/2026-07-06-knowledge-citations.md (2)

2203-2208: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Open the document window before awaiting the fetch.

Calling window.open after await api.getKnowledgeDocumentFile(...) can be blocked by browser popup protection. Open a blank tab synchronously from the click handler, then assign its URL after the blob request succeeds.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/superpowers/plans/2026-07-06-knowledge-citations.md` around lines 2203 -
2208, Update the onOpenDocument handler to synchronously open a blank window
before awaiting api.getKnowledgeDocumentFile, then assign the created object URL
to that window after the fetch succeeds. Preserve the existing scope, filename,
threadId arguments and successful return behavior.

---

2173-2183: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Resolve citation clicks against the owning message.

The listener currently falls back to the latest stored source set, so clicking a citation in an older message can open the wrong sources. Also, event.target is retargeted across shadow DOM; carry the message key in CITE_CLICK_EVENT.detail from <cuga-cite> and look up that exact entry. Add a two-message E2E case to verify this mapping.

Also applies to: 2215-2220

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/superpowers/plans/2026-07-06-knowledge-citations.md` around lines 2173 -
2183, The CITE_CLICK_EVENT listener must resolve sources by the citation’s
owning message instead of falling back to the latest entry. Pass the message key
in the event detail from <cuga-cite>, use that key to look up
lastSourcesByMessage.current, and preserve the citation number as activeN; add a
two-message E2E case confirming each citation opens its own sources.

🧹 Nitpick comments (1)

docs/superpowers/plans/2026-07-06-knowledge-citations.md (1)

2425-2431: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Include integration tests in “full-suite verification.”

The listed commands run tests/unit only, despite the PR objectives explicitly requiring citation unit and integration coverage. Add the repository’s integration-test command or rename this step so it does not imply full-suite coverage.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/superpowers/plans/2026-07-06-knowledge-citations.md` around lines 2425 -
2431, Update “Step 2: Full-suite verification” to include the repository’s
integration-test command alongside the existing unit-test command, ensuring
citation integration coverage is exercised; alternatively, rename the step to
accurately describe unit-only verification if no integration command is
available.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@docs/superpowers/plans/2026-07-06-knowledge-citations.md`:
- Around line 2203-2208: Update the onOpenDocument handler to synchronously open
a blank window before awaiting api.getKnowledgeDocumentFile, then assign the
created object URL to that window after the fetch succeeds. Preserve the
existing scope, filename, threadId arguments and successful return behavior.
- Around line 2173-2183: The CITE_CLICK_EVENT listener must resolve sources by
the citation’s owning message instead of falling back to the latest entry. Pass
the message key in the event detail from <cuga-cite>, use that key to look up
lastSourcesByMessage.current, and preserve the citation number as activeN; add a
two-message E2E case confirming each citation opens its own sources.

---

Nitpick comments:
In `@docs/superpowers/plans/2026-07-06-knowledge-citations.md`:
- Around line 2425-2431: Update “Step 2: Full-suite verification” to include the
repository’s integration-test command alongside the existing unit-test command,
ensuring citation integration coverage is exercised; alternatively, rename the
step to accurately describe unit-only verification if no integration command is
available.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 23b23bc0-0bd2-4379-a17a-51b60471a680

📥 Commits

Reviewing files that changed from the base of the PR and between 029a17d and 2ae94de.

⛔ Files ignored due to path filters (2)

• src/cuga/frontend/dist/index.html is excluded by !**/dist/**
• src/cuga/frontend/dist/main.2ea10f484d495d5d953e.js is excluded by !**/dist/**

📒 Files selected for processing (4)

• docs/superpowers/plans/2026-07-06-knowledge-citations.md
• src/cuga/backend/cuga_graph/nodes/cuga_lite/adapter/prepare_node.py
• src/cuga/backend/server/main.py
• src/frontend_workspaces/agentic_chat/src/KnowledgeConfig.tsx

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

• src/cuga/backend/cuga_graph/nodes/cuga_lite/adapter/prepare_node.py
• src/cuga/backend/server/main.py
• src/frontend_workspaces/agentic_chat/src/KnowledgeConfig.tsx

[sami-marreed]

@dolev31 can you check the rebase if needed?

[dolev31]

Thanks @sami-marreed — already rebased ✅

#468 now has #410's merged work integrated (merge 2ae94de5, all overlapping files resolved to keep both the citations UI and your reindex/publish hardening), it's 0 commits behind main, merges cleanly, CI is fully green, and CodeRabbit re-reviewed with no findings.

Should be all set — would really appreciate your review whenever you get a chance 🙏 (and as flagged earlier, this one's an important customer-facing feature for us). Thanks!

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/frontend_workspaces/frontend/src/manage/hooks/useKnowledgeDraftSave.ts (1)

194-232: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Retry budget is never reset for a genuinely new edit, causing premature failure.

knowledgeSaveRetryRef.current is only reset to 0 on a successful save (Line 141) or via the parent's manual "Retry" click. If a save attempt exhausts MAX_REINDEX_SAVE_RETRIES (20 × 3s ≈ 60s) against a still-running reindex_in_progress, and the user then edits the config again while that same reindex is still in flight, the new debounce cycle re-enters this branch with the counter already at 20 — it immediately reports "Re-index still running — click Retry once it finishes." instead of giving the new edit its own retry window. The counter should be reset whenever the debounce cycle is triggered by an actual config change rather than by the retry nonce.

🔧 Proposed fix: reset the retry budget when the attempted config actually changes

+  const lastAttemptedConfigRef = useRef<string>("");
+
   useEffect(() => {
     if (skipDraftSaveRef.current) return;
 
     if (knowledgeReindexing) {
       knowledgeAbortRef.current?.abort();
       return;
     }
 
     knowledgeAbortRef.current?.abort();
     const ac = new AbortController();
     knowledgeAbortRef.current = ac;
 
     const debounceMs = forceImmediateSaveRef.current ? 0 : 800;
     forceImmediateSaveRef.current = false;
 
+    const serializedConfig = JSON.stringify(knowledgeConfig);
+    if (serializedConfig !== lastAttemptedConfigRef.current) {
+      knowledgeSaveRetryRef.current = 0;
+    }
+    lastAttemptedConfigRef.current = serializedConfig;
+
     const t = setTimeout(async () => {

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/frontend_workspaces/frontend/src/manage/hooks/useKnowledgeDraftSave.ts`
around lines 194 - 232, Reset knowledgeSaveRetryRef.current when a debounce
cycle is triggered by a genuinely changed configuration, while preserving the
existing counter for retry-nonce-driven reattempts. Update the effect or
change-detection logic surrounding the save flow in useKnowledgeDraftSave so new
edits receive a fresh MAX_REINDEX_SAVE_RETRIES budget without resetting it on
scheduled retries.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@src/frontend_workspaces/frontend/src/manage/hooks/useKnowledgeDraftSave.ts`:
- Around line 194-232: Reset knowledgeSaveRetryRef.current when a debounce cycle
is triggered by a genuinely changed configuration, while preserving the existing
counter for retry-nonce-driven reattempts. Update the effect or change-detection
logic surrounding the save flow in useKnowledgeDraftSave so new edits receive a
fresh MAX_REINDEX_SAVE_RETRIES budget without resetting it on scheduled retries.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: ec4ed2b4-21c1-482c-8195-04049359ca4e

📥 Commits

Reviewing files that changed from the base of the PR and between 2ae94de and c638d9d.

⛔ Files ignored due to path filters (2)

• src/cuga/frontend/dist/index.html is excluded by !**/dist/**
• src/cuga/frontend/dist/main.aab7d1812e3bee202945.js is excluded by !**/dist/**

📒 Files selected for processing (7)

• src/cuga/backend/server/config_store.py
• src/cuga/sdk.py
• src/frontend_workspaces/frontend/src/ManagePage.tsx
• src/frontend_workspaces/frontend/src/api.ts
• src/frontend_workspaces/frontend/src/manage/hooks/useKnowledgeDraftSave.ts
• src/frontend_workspaces/frontend/src/manage/hooks/usePublishConfig.ts
• tests/unit/test_config_db_reset_wal_sidecars.py

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

• src/frontend_workspaces/frontend/src/api.ts
• src/cuga/sdk.py

[offerakrabi]

A personal note before the chewed up summary, this PR is too big, it is very hard to review (even for an LLM) and it is hard to track what issues/bugs/fixed are done in each file , ideally it should be split to at least 2 PRs (frontend and backend).

PR Review: #468 — feat(knowledge): inline citations & sources + simplified embeddings config UX

Summary

Verdict: merge with fixes.

This is a substantial, well-executed feature. The architecture is genuinely sound: the SourceLedger design — content-hash deduplication, thread-scoped registry, LRU eviction, and restart rehydration from persisted Answer events — is the right model for multi-hop, multi-turn citation stability and was clearly thought through carefully. The dual-seam stamping (in-process KnowledgeClient.search_envelope + HTTP POST /api/knowledge/search) is correctly placed and covers every execution path including MCP, E2B, and SDK. Citation resolution at FinalAnswerNode with bracket-family support (ASCII, fullwidth, lenticular, tortoise-shell) and a monitoring canary for unsupported styles is a smart hedge against model drift. The frontend <cuga-cite> component's use of the Popover API to escape CSS overflow clipping — with a correct CSS fallback for browsers without support — is technically sharp. Test coverage is extensive: 16 new test files, including deterministic integration tests against a real KnowledgeEngine/fastembed pipeline. The PR delivers exactly what #154 and #155 specified.

Strengths:

• Content-hash chunk identity (sha1(scope|filename|page|sha1(text))) gives stable cite_id across turns and across reindex without schema changes.
• resolve_citations is correctness-first: code fence detection (fenced blocks, unterminated fences, double-backtick inline spans), explicit strip-mode for feature-off, and per-marker warnings only for real ledger misses.
• The _rehydrate_citation_ledger helper correctly gates on the session-aware citations_enabled_for() predicate — not just the agent-level flag — so a per-session override can't cause counter collisions after a server restart.
• Session settings endpoints (GET/PATCH /api/knowledge/session/settings) are properly ownership-enforced and have full route-level tests.
• SourcesPanel query-term highlighting is built with DOM text nodes (<mark> via React children), never innerHTML — hostile snippets are inert.
• All new backend code runs ruff clean and follows the established sources.py no-engine-import discipline.

What to resolve before merge:

Major:

1. All 13 new test files (9 unit + 4 integration) lack the required pytest type marker — pytest -m unit silently skips every new unit test and the fastembed integration tests run without a marker gate. See MAJ-1.
2. apply_citation_resolution clears state.sources when final_answer has no [sN] markers (already-resolved [1]...[n] text). The supervisor callback path (state.sender == CUGA_SUPERVISOR) can reach this with an already-resolved last_planner_answer, silently dropping correctly-resolved sources. See MAJ-2.
3. remove plan file (under docs/superpowers/plans)

Minor:
4. SourcesPanel keys list items by s.n — per-message display numbers repeat (both message A and B have n=1). When the panel stays mounted and sources prop switches to a different message's set, React reuses key="1" DOM nodes, causing stale expanded/ref state. s.cite_id is the correct key. See MIN-1.
5. handleSessionCitationsChange in KnowledgeSidePanel only guards if (!threadId) before calling PATCH /api/knowledge/session/settings. If sessionScopeEnabled is false but threadId is non-empty, programmatic dispatch (automation/test tooling) bypasses the disabled checkbox and creates a phantom override entry for an inactive session. See MIN-2.

Nice to have:
6. customLoadHistory.ts unconditionally console.logs every event including full source snippets (filename, page, content) on reload. See NTH-1.
7. citeElement.ts Popover card position is computed once on mouseenter and not updated on scroll — card drifts off the chip if the user scrolls while hovering. See NTH-2.

Notes / out of scope:

• The three compiled dist bundles (src/cuga/frontend/dist/*.js, index.html) are tracked in git and replaced by this PR. Not a finding introduced by this PR — this is pre-existing policy — but worth a decision: binary artifacts in git make diffs unreadable and cause guaranteed merge conflicts on concurrent frontend PRs.
• 3 candidate findings were dropped after sub-agent verification: MIN-1 ("+N more" scroll target) was invalid — no upstream filtering exists; MIN-4 (TOCTOU in rehydration) was invalid — get_ledger(create=True) holds its lock across both check and creation, making the race benign; NTH-1 (layering on apply_citation_resolution) was invalid — AGENTS.md's layering rule targets cross-node-layer deps, and knowledge imports already exist in multiple other node files.

---

Issues

🟠 Major

[MAJ-1] All 13 new test files lack the required pytest type marker
tests/integration/test_knowledge_citation_e2e.py:119

What's wrong: AGENTS.md (project root) explicitly mandates that every test carry a registered pytest type marker (@pytest.mark.unit, @pytest.mark.e2e, @pytest.mark.slow, etc.) and states "Do not leave tests unmarked when a type marker applies." The 4 integration tests in test_knowledge_citation_e2e.py carry only @pytest.mark.asyncio — no type marker. All 9 new unit test files (test_source_ledger.py, test_citation_resolver.py, test_citation_settings.py, test_envelope_citations.py, test_final_answer_citations.py, test_session_citation_override.py, test_server_citation_egress.py, test_sdk_citations.py, test_session_settings_routes.py) carry no type marker at all. test_knowledge_citation_e2e_live.py correctly uses @pytest.mark.e2e — so the author knows the convention.

What it can cause: pytest -m unit skips all 9 new unit tests silently — a unit-only CI gate passes while the entire citation test net goes unexercised. pytest -m "not slow" picks up the fastembed-backed integration tests, which drive a real KnowledgeEngine, embed documents, and can take 30+ seconds — causing unexpected slowness in a supposedly-fast suite. The convention is explicit in AGENTS.md; callers who copy these files as templates will perpetuate the gap.

---

[MAJ-2] Supervisor callback path clears already-resolved state.sources via a second call to apply_citation_resolution
src/cuga/backend/cuga_graph/nodes/answer/final_answer.py:168-170

What's wrong: apply_citation_resolution is documented to be destructive when called on already-resolved text: "The second run sees no [sN] markers in the already-resolved text and clears state.sources." Lines 99-102 confirm this — if final_answer has no [sN] markers, state.sources = [] and return. In the supervisor callback path at lines 167-170: when state.final_answer is empty but state.last_planner_answer holds an already-resolved answer (e.g., CugaLite resolved [s1] → [1] and set sources before the supervisor transition), answer_to_forward = state.last_planner_answer is non-empty, the if branch executes at line 168, state.final_answer is set to that already-resolved text, then apply_citation_resolution is called — which sees no [sN] markers and wipes state.sources.

What it can cause: A CugaLite subgraph run that correctly resolved [s1] → [1] and set state.sources = [{n:1, cite_id:"s1", filename:"report.pdf", ...}] before the supervisor transition arrives at FinalAnswerNode with state.final_answer="" and state.last_planner_answer="The answer is 42 [1].". Lines 167–170 fire, apply_citation_resolution sees "The answer is 42 [1]." (no [sN] markers), sets state.sources = [], and the user receives an answer with [1] in the text but no clickable chips and no Sources panel. The test test_supervisor_empty_fallback_clears_stale_sources only verifies the desired clearing when both final_answer and last_planner_answer are empty; this path is untested.

---

🟡 Minor

[MIN-1] SourcesPanel keys list items by s.n — per-message display numbers repeat across messages, causing stale React DOM when the panel updates to a different message's sources
src/frontend_workspaces/agentic_chat/src/citations/SourcesPanel.tsx:82

What's wrong: {items.map((s) => (<div key={s.n} ...>))} uses s.n as the React reconciliation key. Display numbers are 1-based per message — message A and message B both have a source {n:1}. Both CarbonChat.tsx and App.tsx keep SourcesPanel mounted persistently (rendered conditionally via sourcesPanel && / sourcesView &&) and update the sources prop when a different message's chip is clicked. When the prop switches from A's sources to B's sources, React sees key="1" in both renders and reuses the existing DOM node instead of replacing it.

What it can cause: User clicks chip [1] in message A (filename: "report.pdf", snippet: "...Q4 revenue..."). Panel opens. User then clicks chip [1] in message B (filename: "handbook.docx", snippet: "...onboarding..."). React patches the existing DOM node keyed n=1: the activeRef does not re-fire (ref callback only fires on mount), scrollIntoView does not run, and the expanded state in SourcesPanel at line 47 — keyed by s.n — carries over. Users see stale expand state for "report.pdf" applied to "handbook.docx". Using s.cite_id (e.g., "s3" vs "s7") as the key is unique across all messages and fixes this.

---

[MIN-2] handleSessionCitationsChange in KnowledgeSidePanel calls the API even when the session scope is disabled
src/frontend_workspaces/agentic_chat/src/KnowledgeSidePanel.tsx:132-143

What's wrong: The handler guards only if (!threadId) return at line 133 before calling api.patchSessionKnowledgeSettings. The checkbox carries disabled={!conversationReady} where conversationReady = sessionScopeEnabled && Boolean(effectiveThreadId). In normal browser use a disabled checkbox cannot fire onChange, but the disabled attribute does not prevent programmatic dispatch (Cypress cy.get(…).click({force:true}), Playwright .dispatchEvent('change'), or direct element.dispatchEvent(new Event('change'))). When sessionScopeEnabled is false but threadId is non-empty, the !threadId guard passes, and the PATCH fires against a session that is not active.

What it can cause: An automated test or accessibility tool forces a click on the disabled citations toggle when session knowledge is turned off. PATCH /api/knowledge/session/settings fires with {citations_enabled: false} for that threadId. _apply_session_settings_patch creates a SessionKnowledgeState entry with overrides: {citations_enabled: false}. Later, when session knowledge is re-enabled for that agent, the stale override is loaded for the thread and citations silently start disabled, inconsistent with the agent-level default of true.

---

🔵 Nice to Have

[NTH-1] customLoadHistory.ts unconditionally logs every event including full citation source data to the browser console
src/frontend_workspaces/frontend/src/carbon-chat/customLoadHistory.ts:79,87,206

What's wrong: Lines 79, 87, and 206 call console.log(...) unconditionally, including logging each full event object on line 87. These log calls pre-date this PR, but the PR adds sources payloads (filename, page, snippet, score, query) to Answer events, so reloading a conversation now logs document content to the console.

What it can cause: In a production deployment any user who opens DevTools on history load can see the full source snippets and filenames of every cited document in that conversation. This may expose document content to users who should not have direct file access (they have search access through the agent, not raw read access). It also produces log noise that buries real errors.

---

[NTH-2] citeElement.ts Popover card position is computed once on hover enter — card drifts from the chip if the user scrolls while the card is visible
src/frontend_workspaces/agentic_chat/src/citations/citeElement.ts:101-105

What's wrong: place() computes left/top from this.getBoundingClientRect() once on mouseenter / focusin. The Popover API places the card in the browser top layer at position:fixed, relative to the viewport. If the user scrolls the page while the card is visible (e.g., in long answers with many paragraphs), the chip scrolls but the card stays fixed at its original coordinates.

What it can cause: On a long answer the user hovers chip [1] near the bottom, the card pops up above it. The user scrolls up slightly to read context. The chip moves up 50px but the card remains at the original top-layer coordinate, floating in empty space. Minor cosmetic issue; the card dismisses on mouseleave, which fires as soon as the pointer leaves the chip, so practical exposure is limited to the brief scroll-while-hovering window.

---

Test Suite Audit

114 tests across 14 new files. The suite is well-designed — 110 tests are fully justified. 4 should be cut, 2 collapsed into 1.

Cut these 4 tests

File	Test	Why
test_source_ledger.py	test_get_unknown_returns_none	Returns None on a cache miss — covered implicitly by every test that checks a non-existent ID
test_source_ledger.py	test_get_ledger_without_create_returns_none_on_miss	Tests that create=False doesn't auto-create — covered implicitly by the thread isolation test
test_sdk_citations.py	test_enable_citations_param_stored	Verifies agent._enable_citations = False after CugaAgent(enable_citations=False) — testing Python assignment, not behavior
test_sdk_citations.py	test_enable_citations_defaults_to_none	Verifies omitting a param gives None — testing Python default argument semantics

Reduce these 2 into 1

test_footer_single_entry_with_page and test_footer_single_entry_no_page in test_server_citation_egress.py both call _format_sources_footer with a one-element list, differing only in whether page is an int or None. Collapse into a single @pytest.mark.parametrize. Keep test_footer_page_zero_is_included standalone — page=0 is a real edge case (falsy in Python, must still render).

Everything else: keep

The remaining 110 tests all pin real, non-obvious behavioral invariants: the SourceLedger eviction policy, the regex edge cases in resolve_citations (CJK brackets, unterminated fences, double-backtick spans), the rehydration counter-bump regression, the HTTP ownership enforcement, and the end-to-end fastembed pipeline that caught the lenticular-bracket regression that unit tests with hand-seeded ledgers missed.

No tests were found that test the wrong thing (implementation details rather than behavior), and no assertion was found that would survive a real bug.

The audit also confirmed that test_session_citation_override.py (3 tests) should be kept despite appearing redundant with test_session_settings_routes.py — the helper-level tests verify the allowlist filtering and string coercion in isolation; the route tests exercise the full HTTP stack on top of it. Both layers are worth pinning.

The primary CI concern is not the count — it's the missing @pytest.mark type markers (MAJ-1). Without them, pytest -m unit silently skips all 9 new unit test files.

[offerakrabi]

this file should not be committed, please remove

[dolev31]

Removed the committed plan doc in 963247c — thanks @offerakrabi. It was a working design note, not product docs; won't be re-added.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

src/cuga/backend/server/main.py (1)

214-227: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Redundant DB read of the same stream-events row on new/restarted threads.

_seed_stream_events_buffer (called at Line 1536) and _rehydrate_citation_ledger's internal fetch (Lines 271-272) both call get_conversation_db().get_stream_events(app_state.agent_id, thread_id, user_id) for the identical (agent_id, thread_id, user_id) within the same turn. The second fetch only happens when the in-memory ledger is absent (first turn of a thread, or any turn right after a server restart), but that's still a common hot path, and it duplicates a query whose result (stream_events_buffer) is already sitting in scope at the call site (Line 1537).

Consider threading the already-fetched buffer into _rehydrate_citation_ledger instead of re-querying:

♻️ Reuse the seeded buffer instead of re-fetching

 async def _rehydrate_citation_ledger(
-    app_state: "AppState", thread_id: str, user_id: str, is_resume: bool = False
+    app_state: "AppState", thread_id: str, user_id: str, is_resume: bool = False,
+    events_list: list | None = None,
 ) -> None:
     ...
         _ledger = _get_ledger(thread_id, create=False)
         if _ledger is None:
-            conversation_db = get_conversation_db()
-            stream_history = await conversation_db.get_stream_events(app_state.agent_id, thread_id, user_id)
-            events_list = stream_history.events if stream_history else []
+            events_list = events_list or []
             _ledger = _get_ledger(thread_id)  # create
             for ev in events_list:
-                if ev.event_name != "Answer":
+                name = ev.get("event_name") if isinstance(ev, dict) else ev.event_name
+                if name != "Answer":
                     continue
                 try:
-                    payload = json.loads(ev.event_data)
+                    data = ev.get("event_data") if isinstance(ev, dict) else ev.event_data
+                    payload = json.loads(data)

And at the call site (Line 1598):

-        await _rehydrate_citation_ledger(app_state, thread_id, user_id, is_resume=bool(resume))
+        await _rehydrate_citation_ledger(
+            app_state, thread_id, user_id, is_resume=bool(resume), events_list=stream_events_buffer
+        )

Also applies to: 269-276

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/cuga/backend/server/main.py` around lines 214 - 227, Update
_rehydrate_citation_ledger to accept the already-seeded stream_events_buffer as
an argument and use it when reconstructing the citation ledger, removing its
duplicate get_stream_events database query. At the call site near the stream
buffer initialization, pass the buffer returned by _seed_stream_events_buffer
while preserving existing behavior when the buffer is empty or the in-memory
ledger already exists.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/cuga/backend/server/main.py`:
- Around line 253-254: Update the ledger initialization flow so the resume path
skips only begin_turn(), not ledger rehydration. In the logic around
_get_ledger(thread_id, create=False), continue loading or restoring the existing
ledger when thread_id is present even when is_resume is true, while retaining
the early return for missing thread IDs and preserving normal begin_turn()
behavior for non-resume requests.

---

Nitpick comments:
In `@src/cuga/backend/server/main.py`:
- Around line 214-227: Update _rehydrate_citation_ledger to accept the
already-seeded stream_events_buffer as an argument and use it when
reconstructing the citation ledger, removing its duplicate get_stream_events
database query. At the call site near the stream buffer initialization, pass the
buffer returned by _seed_stream_events_buffer while preserving existing behavior
when the buffer is empty or the in-memory ledger already exists.

🪄 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: 04148729-3d1c-4bf2-b880-51186fdb94dc

📥 Commits

Reviewing files that changed from the base of the PR and between c638d9d and 212bd21.

📒 Files selected for processing (8)

• src/cuga/backend/knowledge/awareness.py
• src/cuga/backend/knowledge/sources.py
• src/cuga/backend/server/main.py
• src/cuga/sdk.py
• tests/unit/test_citation_resolver.py
• tests/unit/test_citation_settings.py
• tests/unit/test_ephemeral_stream_events.py
• tests/unit/test_sdk_citations.py

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

• src/cuga/backend/knowledge/awareness.py
• tests/unit/test_sdk_citations.py
• tests/unit/test_citation_settings.py
• src/cuga/sdk.py
• src/cuga/backend/knowledge/sources.py

[dolev31]

Thanks @offerakrabi — genuinely thorough review, and the architecture notes are appreciated. I've pushed fixes for all the actionable findings (682c590):

• MAJ-1 (CI coverage): the citation / source-ledger / session-settings / durability-reset test files were never in CI's explicit allowlist — 13 files (93 tests) ran only locally. Added them to tests.yml.
• MAJ-2 (source clobber): apply_citation_resolution is now idempotent — the supervisor callback re-entering with an already-resolved last_planner_answer keeps its sources instead of clearing them; a genuinely uncited answer still clears (no stale leak). +2 regression tests.
• MIN-1: SourcesPanel now keys by the stable cite_id, not the per-message n.
• MIN-2: handleSessionCitationsChange now guards on conversationReady (mirrors the toggle's own disabled condition), so a programmatic dispatch can't write a phantom session override.
• MAJ-3 (plan doc): removed in 963247c.
• Plus a CodeRabbit follow-up: the resume path now rehydrates on restart-during-interrupt and only skips begin_turn().

NTH-1/NTH-2 (reload console.log, popover reposition-on-scroll) I've left as follow-ups — happy to fold in if you'd prefer them here.

On splitting into frontend/backend PRs — I hear you that the size makes review hard, and I don't disagree in principle. My reasoning for keeping it together this once:

• The feature is a single vertical contract: the backend stamps cite_id on the wire and the frontend <cuga-cite> renders it. Split down the middle, neither half is independently testable or mergeable — a frontend PR renders chips with no ids to bind, a backend PR emits ids nothing consumes — so the integration tests (which are much of the value here) can't live in either half.
• Two PRs are already stacked on this branch (fix(knowledge): make the embedding-model cache durable and self-healing #497 embedder-cache, feat(knowledge): open a cited PDF at the cited page #498 open-at-page); re-basing a mid-flight split onto two new bases would churn all three.
• The single biggest source of "unreadable diff" is the committed src/cuga/frontend/dist/* bundles (minified JS), not the source. That's pre-existing repo policy — if we .gitignore the dist and build on release, future frontend PRs get dramatically smaller diffs. Happy to open that as a separate proposal.

If you still want it split after this pass, I'll carve the frontend out — just confirm and I'll coordinate the stack.

Re CI red: the failing checks are all groq 413 rate_limit_exceeded (org TPM cap) on the live-LLM stability/integration steps — the pure unit + lint + pgvector + CodeQL steps are green. Not from this change; a re-run once TPM resets should clear them.

@sami-marreed / @offerakrabi — ready for another look when you have a moment.