Skip to content

fix(generation): resolve real source titles instead of "Untitled"#19

Open
yuzushi-dev wants to merge 3 commits into
mainfrom
fix/source-titles-untitled
Open

fix(generation): resolve real source titles instead of "Untitled"#19
yuzushi-dev wants to merge 3 commits into
mainfrom
fix/source-titles-untitled

Conversation

@yuzushi-dev

@yuzushi-dev yuzushi-dev commented Jul 15, 2026

Copy link
Copy Markdown
Owner

Problema

Due feedback negativi in prod (2026-07-14) lamentavano fonti senza titolo ("Untitled") e link "view document" rotto. Le fonti mostravano document_id corretto ma titolo Untitled.

Causa radice

_get_document_titles() estraeva document_id da candidate.metadata["document_id"], ma Candidate (retrieval/domain/candidate.py) lo tiene come field top-level .document_id; il vector search Milvus popola .metadata con solo content (milvus.py:511). Risultato: doc_ids sempre vuoto → get_titles_by_ids() mai interrogato → titolo sempre in fallback → "Untitled".

Il document_id nelle fonti restava corretto perché il loop cited_sources (fix a8bc3065) legge il field top-level; quel fix però non toccò _get_document_titles, lasciando il bug del titolo.

Fix

  • _get_document_titles: legge document_id dal field top-level (chiave dict per i dict candidates), come già fa cited_sources. Ora get_titles_by_ids() risolve i filename reali da Postgres.
  • _map_sources (path non-stream): stesso difetto — leggeva document_id/title da .metadata (→ "unknown"/"Untitled"). Corretto per coerenza.

Test

tests/unit/test_source_titles.py — verifica che document_id top-level raggiunga il lookup titoli. Fallisce sul codice pre-fix (lookup mai chiamato).

Rischio

Nessuna perdita dati, nessun downtime. Solo SELECT di lettura già esistenti; nessuno schema change. Deploy = resync main + restart api/worker.

@yuzushi-dev
fix(generation): resolve real source titles instead of "Untitled"
d8f9e6d 
_get_document_titles() extracted document_id from candidate.metadata, but
Candidate stores it as a top-level field and vector search only puts
"content" in metadata. So doc_ids came out empty, get_titles_by_ids() was
never queried, and every source fell back to "Untitled" — even though the
document_id on each source was correct (the a8bc306 fix only touched the
cited_sources loop, not the title lookup).

Read document_id from the top-level field (dict key for dict candidates),
matching the cited_sources loop. Also fix the sibling _map_sources() path,
which read both document_id and title from .metadata (yielding "unknown" /
"Untitled") and never did a DB title lookup.

Regression test in tests/unit/test_source_titles.py.
@yuzushi-dev
fix(generation): resolve source titles under RLS (deterministic, no "…
23df5fb 
…Untitled")

The a8bc306/d8f9e6db title fixes were undermined at runtime by two issues,
so cited sources still rendered as "Untitled" intermittently:

1. _map_sources() (the path that feeds QueryResponse.sources) only read the
   title from candidate.metadata["document_title"]. Vector-search candidates
   never carry that key, so it always fell back to "Untitled". The sibling
   context-builder already resolved real titles via _get_document_titles() (a
   DB lookup) but that map never reached _map_sources. generate() now fetches
   the doc_titles map once and passes it in; _map_sources prefers it over
   metadata. This is the authoritative, deterministic title source.

2. The DB title lookup is RLS-gated on the documents table (FORCE RLS).
   get_db_session set app.current_tenant from get_current_tenant(), a contextvar
   set inside the auth BaseHTTPMiddleware that does not reliably propagate to
   endpoint dependencies. On pooled connections where it was empty, the lookup
   returned 0 rows -> empty titles. It now reads request.state.tenant_id (set
   deterministically by AuthenticationMiddleware), mirroring how permissions /
   group_ids / tenant_role are already sourced a few lines below.

Verified on the prod mirror: same query x5 -> 0 "Untitled" (was flaky/all-Untitled).
Regression test extended in tests/unit/test_source_titles.py.
@yuzushi-dev

yuzushi-dev commented Jul 23, 2026

Copy link
Copy Markdown
Owner Author

Follow-up commit 23df5fb6: found on the prod-data mirror that titles still resolved to "Untitled" intermittently despite this PR.

Two root causes fixed:

  1. _map_sources() (the path feeding QueryResponse.sources) only read metadata["document_title"], which vector-search candidates never carry → always "Untitled". It now receives the document_id→filename map from _get_document_titles() (DB lookup) and prefers it.
  2. The DB lookup is RLS-gated on documents (FORCE RLS). get_db_session set app.current_tenant from the get_current_tenant() contextvar, set inside the auth BaseHTTPMiddleware and not reliably propagated to endpoint deps → empty GUC on some pooled connections → 0 rows. Now sourced from request.state.tenant_id (deterministic), like permissions/groups/role already are.

Verified on the mirror: same query ×5 → 0 Untitled (was flaky/all-Untitled). Regression test extended.

@yuzushi-dev
test(api): cover get_db_session RLS tenant-GUC source (request.state)
93da7a3 
The deps.py change (sourcing app.current_tenant from request.state.tenant_id
instead of the get_current_tenant() contextvar) shipped with no test, despite
touching shared infra used by every get_db_session call site. Add unit tests
that drive the dependency with a mocked session and assert:
- app.current_tenant is set from request.state.tenant_id (a contextvar
  returning a different value must NOT win);
- when request.state has no tenant_id, it falls back to the contextvar
  (superset of prior behaviour, never worse).
@yuzushi-dev

yuzushi-dev commented Jul 23, 2026

Copy link
Copy Markdown
Owner Author

Correction to the follow-up commit narrative (23df5fb6), plus test coverage (93da7a39):

Narrative fix: the earlier message framed the contextvar issue as "the propagation bug is resolved". That overstates it — this PR only fixes the RLS tenant GUC in get_db_session. Other consumers still read get_current_tenant() and are not touched: rate_limit.py (and RateLimitMiddleware runs before AuthenticationMiddleware, so the contextvar isn't set yet), the LLM-audit tenant tagging in ollama.py/anthropic.py, generation_service.py, chunks.py, etc. A full fix would standardise all readers on request.state or install a context-propagating middleware — tracked separately, not claimed here.

Coverage: added tests/unit/test_get_db_session_tenant.py — asserts app.current_tenant is sourced from request.state.tenant_id (a contextvar with a different value must not win) and falls back to the contextvar when request.state has no tenant. Closes the 'shared-infra change ships with no regression test' gap.

@yuzushi-dev yuzushi-dev mentioned this pull request Jul 23, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@yuzushi-dev