feat: user differentiation — identify, filter, and label users across the gateway

[PeterStoica]

Summary

• User hash extraction: Extracts a stable user hash from request metadata (API key fingerprint / OAuth subject) and propagates it through the entire event pipeline — conversation_calls, request_logs, session_summaries
• User filtering & badges: History page shows per-user filter dropdown and colored user badges on session cards; request logs support user_hash query parameter
• User labeling: Click a user hash badge to assign a human-readable display name, stored in a user_labels table
• Session summaries materialization: New session_summaries table with denormalized models and preview columns eliminates expensive JOINs for the history page (query dropped from ~10s to <100ms at scale)
• Bounded EventEmitter: Queue-based drain loop with backpressure replaces the old per-event DB write path; SSE throttled to max 10 updates/sec
• Unified config system: config_fields.py as single source of truth, config_registry.py with CLI > env > DB > default resolution and provenance tracking, auto-generated settings.py and .env.example, plus /config admin dashboard
• Policy cache infrastructure: Generic key-value cache with TTL, per-namespace FIFO eviction cap, and full round-trip test coverage
• PROXY_API_KEY → CLIENT_API_KEY rename: Reframes auth as passthrough-first; includes migration, backwards-compat field validator, and doc updates
• CLI additions: luthien restart, luthien agent-tutorial commands
• One-click Railway deploy: OAuth pass-through + default policies
• Docs overhaul: Rewritten ARCHITECTURE.md, canonical dev-README.md, admin auth docs audit, policy authoring skill

Test plan

• Unit tests pass: uv run pytest tests/luthien_proxy/unit_tests
• SQLite e2e tests pass: ./scripts/run_e2e.sh sqlite
• Mock e2e tests pass: ./scripts/run_e2e.sh mock
• History page loads with user badges and filter dropdown
• Clicking a user hash badge allows setting a display name
• Config dashboard at /config shows all fields with provenance
• dev_checks.sh passes clean

🤖 Generated with Claude Code

[github-actions]

CHANGELOG reminder — This PR has no changelog fragment.

Add a file to changelog.d/ (see changelog.d/README.md for format).
If this is a chore/infra-only change, add the skip-changelog or chore label to suppress this reminder.

[claude]

PR Review: feat: user differentiation

Overall this is a well-structured, substantial feature PR. The user hash extraction, session summaries materialization, bounded EventEmitter, and user labeling are all solid additions. Here's a detailed review:

---

Code Quality & Architecture

Strengths:

• The session_summaries materialized table is a great architectural decision — eliminating expensive JOINs in the history page query path is exactly right.
• The bounded EventEmitter with queue-based drain loop is a clean replacement for per-event DB writes. The backpressure model (drop + log) is appropriate for observability data.
• The user hash extraction logic in pipeline/session.py is well-factored, with clear separation between API-key-mode and OAuth-mode identification.
• Migration files are well-structured with proper backfill logic, and SQLite copies correctly match.

Issues to address:

1. _fetch_session_list_pg and _fetch_session_list_sqlite are nearly identical (service.py:377-501). Both functions read from session_summaries with the same SQL, same parameter binding, and same SessionSummary construction. The only historical reason for separate paths was different SQL dialects, but since both now read from the same denormalized table with the same query, they should be unified into one function. This is ~120 lines of pure duplication.

2. UserPrefixMiddleware has no input validation on the username (main.py:352-362). The parts[2] value from the URL path is used directly as a display_name in the user_labels table via the auto-labeling code in anthropic_processor.py. While it's parameterized in SQL (no injection risk), there's no length check or character sanitization — a crafted URL like /u/<500-char-string>/v1/messages would insert an arbitrarily long display name. Consider adding a simple length cap.

3. Auto-labeling on every request is wasteful (anthropic_processor.py:398-413). The INSERT ... ON CONFLICT DO UPDATE runs on every single API request when the /u/{name}/ prefix is present. For a proxy handling hundreds of requests per conversation, this is an unnecessary DB round-trip. Consider caching the label check (e.g., in an in-memory set of already-labeled user hashes) or only running it once per session.

4. emit() busy-waits on queue drain (emitter.py:274-276). The while not self._db_queue.empty(): await asyncio.sleep(0.01) polling loop in emit() is a spin-wait antipattern. If the drain loop is slow, this will consume CPU. Consider using an asyncio.Event that the drain loop sets after processing, or simply document that emit() is only for backward compatibility and tests (which seems to be the case from the docstring).

---

Potential Bugs

5. call_count overcounting acknowledged but not addressed (emitter.py:441). The comment says "call_count may slightly overcount if a call_id spans multiple batches." Since session_summaries is used for the history page display, this means turn counts can drift upward over time. The comment suggests it's "acceptable for observability," but the history page presents this as a definitive count. Consider using COUNT(DISTINCT call_id) in a periodic reconciliation, or tracking unique call_ids in the upsert logic.

6. _extract_session_metadata preview extraction doesn't handle Anthropic content blocks correctly (emitter.py:123-128). The code joins text blocks with spaces: " ".join(b.get("text", "") for b in content ...). But if content is a string (not a list), it falls through to isinstance(content, str) on line 128 — this is fine. However, if a content block has type: "text" but no text key, b.get("text", "") returns "", which produces spurious spaces in the preview. Minor but worth noting.

7. models_used CSV concatenation can produce duplicates (emitter.py:455-458). The ON CONFLICT update appends EXCLUDED.models_used to the existing value with a comma separator. Over many batches, the same model name will appear multiple times (e.g., "claude-3,claude-3,claude-3"). The _parse_models_csv function in service.py deduplicates on read, which is a fine workaround, but the column will grow unbounded. Consider deduplicating on write or periodically.

---

Security

8. User hash from credential is a direct SHA-256 truncation (session.py:116). The 16-hex-char (64-bit) truncated SHA-256 of the credential value is used as the user identifier. For API keys with sufficient entropy this is fine, but for short or low-entropy credentials, 64 bits may not prevent hash collisions in a multi-tenant deployment. This is an acceptable tradeoff for an observability feature, but worth documenting that this is not a cryptographic identity guarantee.

9. editUserLabel in history_list.html uses prompt() and sends to API without CSRF protection (history_list.html:619-640). The PUT/DELETE to /api/history/user-labels/ relies on the admin auth token (cookie-based session), but the fetch() calls don't include any CSRF token. If the admin UI uses cookie-based auth, this could be vulnerable to CSRF. Verify that the verify_admin_token dependency requires a bearer token header (not just a cookie) for these mutation endpoints.

---

Performance

10. SSE throttling is a good addition (activity_monitor.js). The 100ms throttle with 500-event cap prevents DOM thrashing during high-throughput scenarios. The slice(-200) on buffer overflow is smart — keeps recent events.

11. The session_summaries backfill in migration 013 could be slow on large databases (013_add_session_summaries.sql:22-37). The GROUP BY ce.session_id with the correlated subquery for user_hash will do a sequential scan of conversation_events. For large deployments, consider adding a note that this migration may take time, or make it idempotent so it can be re-run if interrupted.

12. CREATE INDEX CONCURRENTLY in Postgres migration 014 is good — avoids locking the table during index creation on a potentially large conversation_events table.

---

Test Coverage

Good coverage for:

• User hash extraction (test_extract_user_hash.py) — 8 tests covering API key mode, OAuth mode, fallbacks, edge cases
• SQLite session list queries (test_service_sqlite.py) — thorough integration tests against a real SQLite DB
• Emitter bounded queue behavior (test_emitter.py) — tests for queue full/drop and batch drain
• Request log user_hash field propagation

Missing test coverage:

• No tests for UserPrefixMiddleware — URL prefix stripping and request.state.luthien_user assignment
• No tests for the auto-labeling logic in process_anthropic_request (the INSERT INTO user_labels block)
• No tests for set_user_label / delete_user_label routes — the PUT/DELETE endpoints in history routes
• No tests for list_users route — the GET /api/history/users endpoint
• No tests for _extract_session_metadata in the emitter — the preview extraction and model collection logic
• No tests for _update_session_summaries — the incremental update logic in the emitter

These are all new public-facing code paths that should have unit tests per the repo's testing requirements.

---

Minor Nits

• emitter.py:79: _SYSTEM_REMINDER_RE uses __import__("re") twice in the module body instead of importing re at the top. This works but is unusual and harder to read. Consider adding import re and using re.compile(...).
• The PR description mentions several features (config system, policy cache, PROXY_API_KEY rename, CLI additions, Railway deploy, docs overhaul) that don't appear in the diff. Either the description is stale or these changes were merged separately — consider updating the PR description to match the actual diff.

---

Summary

This is solid foundational work for user differentiation. The main concerns are:

1. Code duplication in the Postgres/SQLite session list functions (easy fix)
2. Missing tests for new endpoints and middleware (important per repo guidelines)
3. Auto-label DB write on every request is a performance concern worth addressing
4. Busy-wait in emit() should use a proper async signaling mechanism

None of these are blocking, but items 2 and 3 should ideally be addressed before merge.

[claude]

Code Review: PR #580 — User Differentiation

Thanks for a substantial feature set. The user hash extraction, session summaries materialization, and bounded EventEmitter are well-motivated and address real production issues (the OOM from unbounded task accumulation, the 10s history page query). Below is structured feedback.

---

Scope concern — One PR = One Concern

This PR bundles at least 4 independently reviewable/mergeable concerns:

1. User hash extraction + propagation
2. Session summaries materialization (performance fix)
3. Bounded EventEmitter (OOM fix / bug fix)
4. SSE throttling (performance fix)

Per the repo's own CLAUDE.md guidelines, bug fixes bundled into feature PRs bypass the COE process. Items 2–4 are infrastructure/performance fixes that could each be their own PR with independent review and rollback.

---

Bugs / Correctness

1. dropped_db_writes is a class variable, dropped_events is an instance variable — mixed semantics
emitter.py:202 declares dropped_db_writes: int = 0 as a class-level attribute, then emitter.py:226 declares self.dropped_events in __init__. Line 364 increments EventEmitter.dropped_db_writes (class-level). This means all instances (including test instances) share and increment the same counter. It should be self.dropped_db_writes initialized in __init__, consistent with dropped_events.

2. Memory leak: _labeled_user_hashes grows unbounded
anthropic_processor.py:75 defines _labeled_user_hashes: set[tuple[str, str]] = set() at module level. Every unique (user_hash, luthien_user) pair is added and never evicted. On a long-running proxy with many distinct users or rotating credentials, this set grows without bound. Consider using an LRU cache (e.g. functools.lru_cache or a bounded dict) or periodically clearing it.

3. N+1 query in _update_session_summaries
emitter.py:451-453 — For each session in the batch, a separate SELECT models_used FROM session_summaries WHERE session_id = $1 is issued. If a batch has events from 20 sessions, that's 20 SELECT queries inside the transaction. This could be collapsed into a single SELECT session_id, models_used FROM session_summaries WHERE session_id = ANY($1) and indexed by session_id in a dict.

4. models_used CSV concatenation can produce duplicates across batches
emitter.py:475-478 — The ON CONFLICT upsert appends new models via session_summaries.models_used || ',' || EXCLUDED.models_used. The dedup logic at lines 450-457 only deduplicates against what's already in the DB before this batch, but the UPSERT itself blindly concatenates. If the same model appears in the existing CSV and in EXCLUDED (which the dedup should prevent), or if there are empty strings, the CSV can get malformed. Consider normalizing models into a separate table or using array types in Postgres.

5. Inconsistent preview truncation length
emitter.py:79 sets _PREVIEW_MAX_LENGTH = 200 for the EventEmitter metadata extraction path, while history/service.py:298 sets _FIRST_MESSAGE_MAX_LENGTH = 100 for the service-layer preview extraction. Both truncate preview messages but to different lengths. The emitter's preview is stored in session_summaries.preview_message and displayed on the history page, while the service function is used for on-the-fly extraction. This inconsistency means the same content could show different previews depending on code path. Pick one authoritative length.

6. emit() has a potential infinite loop
emitter.py:280-283 — The emit() method waits in a while not self._db_queue.empty() loop for the drain task to process items. If the drain loop encounters repeated errors (e.g. DB down), it drops events via dropped_db_writes but the queue items are already consumed by get() in the drain loop, so the queue does eventually empty. However, if new record() calls are happening concurrently, the queue may never go empty, causing emit() to block indefinitely. Consider adding a timeout or attempt count.

---

Security

7. No length validation on display_name
history/routes.py:42 — UserLabelRequest.display_name has no max_length constraint. A malicious admin could set a multi-MB display name, wasting database storage and potentially causing rendering issues. Add Field(..., max_length=255) or similar.

8. User hash regex extracts unsanitized content
session.py:83 — _USER_HASH_PATTERN = re.compile(r"^user_(.+?)_account__") extracts the user hash from untrusted metadata.user_id. The .+? group captures arbitrary content. While this is properly parameterized in SQL queries (no injection risk), the extracted value is stored and rendered in the UI. The escapeHtml() function in history_list.html:799 handles XSS correctly for now, but consider validating the extracted hash matches expected format (hex characters only) as defense-in-depth.

9. Auto-label silently swallows all exceptions
anthropic_processor.py:418-419 — The bare except Exception catches everything including ConnectionRefusedError, PermissionError, etc. and logs at DEBUG level. This means if the DB is misconfigured, you'll never know labels aren't being saved. At minimum, log at WARNING.

---

Performance

10. /api/history/users endpoint has no pagination
history/routes.py:120-129 — SELECT DISTINCT user_hash FROM conversation_calls with no LIMIT. On a high-traffic proxy with thousands of users, this returns a large unbounded dataset. Add pagination or at minimum a LIMIT.

11. Two separate queries for count + paginated list
history/service.py:374-405 — Each fetch_session_list call issues two queries (COUNT + SELECT). These could be merged into one query using COUNT(*) OVER() as a window function.

12. Session summaries user_hash column missing index in Postgres partial form
Migration 011 correctly creates partial indexes (WHERE user_hash IS NOT NULL) for Postgres, but the session_summaries table in migration 013 creates a plain index. This is fine functionally — just noting the inconsistency in indexing strategy. The session_summaries(user_hash) index at line 18-19 of migration 013 does exist, which addresses the filter query performance.

---

Migration Quality

13. Postgres/SQLite parity is correctly handled — the SQL dialect differences (FILTER (WHERE ...) vs SUM(CASE ...), string_agg vs GROUP_CONCAT, TIMESTAMPTZ vs TEXT) are all correctly adapted per the migration guide. Good.

14. Migration 014 doesn't backfill preview_message — only models_used gets backfilled in migration 014. Existing sessions will show no preview until the drain loop processes new events for those sessions. This is acceptable but should be documented (or a backfill query added).

---

Test Coverage

15. Missing test: user_hash filtering in list_sessions
The route test at test_routes.py mocks fetch_session_list but always passes user_hash=None. There's no test that verifies the user_hash query parameter is correctly forwarded to the service layer.

16. Missing test: emitter.record() receives correct user_hash from processor
test_anthropic_processor.py tests that extract_user_hash is called, but doesn't verify the extracted hash is included in emitter.record() call payloads. This is the critical integration point.

17. Good coverage overall — The _extract_session_metadata tests are thorough (probe skipping, system reminder stripping, model dedup, content block filtering). The bounded EventEmitter tests cover overflow, batching, and error recovery well.

---

Minor / Style

• emitter.py:33 — DbQueueItem as a 6-element tuple is fragile for future changes. A NamedTuple or dataclass would make unpacking at lines 384, 405, 432 self-documenting and refactor-safe.
• The dev/plans/bounded-event-emitter-plan.md (734 lines) and dev/plans/bounded-event-emitter.md should these be committed? They seem like planning artifacts.
• history_list.html:684 — Building HTML via string interpolation in JS (onclick="editUserLabel('${escapeHtml(...)}'")) works but is fragile. Consider using data attributes and event delegation for cleaner separation.

---

Summary

Must fix before merge:

• chore(logging): replace prints with structured logging #1 (class vs instance variable for dropped_db_writes)
• Remove PolicyEngine and inline Redis setup #2 (unbounded _labeled_user_hashes set)
• [minor] starlette args #7 (no max_length on display_name)

Strongly recommended:

• datetime.datetime.utcnow() is deprecated #3 (N+1 query in _update_session_summaries)
• Fix deprecated utcnow usage in tests #5 (inconsistent preview truncation)
• fix: update TemplateResponse parameter order to fix deprecation warning #9 (silent exception swallowing at DEBUG level)
• DB pooling and client management #10 (unbounded /api/history/users response)

Consider for follow-up:

• Splitting this into multiple PRs per the One PR = One Concern guideline
• Converting DbQueueItem tuple to a NamedTuple
• Adding preview_message backfill to migration 014

[claude]

PR #580 Code Review — User Differentiation

This is a substantial feature PR (3175 additions, 39 files). The core design is sound — extracting user hashes from request metadata, materializing session summaries for fast history queries, and adding bounded queue semantics to the EventEmitter are all well-motivated. Below are the issues found, grouped by severity.

---

Critical / Must-Fix

1. EventEmitter shutdown can lose events (emitter.py:241-260)

After _drain_task.cancel(), events consumed from the queue by the drain loop but not yet written to DB are lost. The drain loop's _collect_batch() removes items from the queue; if cancellation hits during _write_db_batch(), those items are gone — the transaction rolls back but the queue entries are already consumed. The subsequent shutdown() final-drain only sees what's still in the queue.

Suggestion: Instead of cancel(), set a shutdown flag and let the drain loop exit gracefully after draining the remaining queue. Then do the final flush:

self._shutting_down = True
# Signal the drain loop to exit after processing remaining items
await self._drain_task  # no cancel — loop checks flag and returns

2. CREATE INDEX CONCURRENTLY in Postgres migration 014 (line 5)

CREATE INDEX CONCURRENTLY cannot run inside a transaction block. If the migration runner wraps statements in a transaction (common), this will fail outright. Replace with CREATE INDEX IF NOT EXISTS.

3. XSS in inline onclick handler (history_list.html:684)

onclick="editUserLabel('${escapeHtml(session.user_hash)}', event)"

The escapeHtml() function (line 799) uses textContent/innerHTML, which encodes <, >, & but not single quotes. In an inline onclick attribute, the browser HTML-decodes the value before evaluating it as JS, so a user_hash containing ' breaks out of the string literal.

Current risk: Low, since user_hash values are server-generated hex strings. Defense-in-depth fix: Use addEventListener instead of inline onclick, or add single-quote escaping.

4. XSS in activity_monitor.js filter dropdown (lines 164, 178)

categoryDiv.innerHTML = `... ${category} ... ${subtypes.length} ...`;
subtypeDiv.innerHTML = `... ${subtype} ...`;

Event type strings (category, subtype) are interpolated into innerHTML without escaping. If a policy ever generates an event type containing HTML, it would execute.

Fix: Use textContent for the label text, or apply escaping.

---

High — Should Fix Before Merge

5. LIKE wildcard injection in request_log search (request_log/service.py:129-141)

"(request_body::text ILIKE '%' || ? || '%' OR response_body::text ILIKE '%' || ? || '%')"

Parameterized queries prevent SQL injection, but LIKE metacharacters (%, _) in the search string are not escaped. A search for % matches everything; _ matches any single character. This gives users unexpected results.

Fix: Escape LIKE specials before passing to the query:

escaped = search.replace("\\", "\\\\").replace("%", "\\%").replace("_", "\\_")

6. list_users fetches ALL labels unbounded (history/routes.py:137)

label_rows = await conn.fetch("SELECT user_hash, display_name FROM user_labels")

Users are paginated (LIMIT $1 OFFSET $2), but labels are fetched for all users with no limit. At scale this is a full table scan returned to the client.

Fix: Either limit labels to the returned user hashes (via IN clause), or add a LIMIT to the labels query.

7. Missing error handling in editUserLabel JS (history_list.html:619-640)

The fetch() calls don't check response.ok. On API failure, the UI updates optimistically (line 635: userLabels[hash] = name.trim()) and never rolls back. Add error handling and show feedback on failure.

8. Global cache race condition (anthropic_processor.py:404-421)

_labeled_user_hashes is a module-level OrderedDict with a check-then-act pattern separated by an await:

if cache_key not in _labeled_user_hashes:     # ← check
    async with db_pool.connection() as conn:    # ← yields
        await conn.execute(...)                  # ← act
    _labeled_user_hashes[cache_key] = None

Concurrent coroutines can all pass the check, causing redundant DB writes. Safe due to ON CONFLICT, but wasteful.

Fix: Mark the cache entry before the await, roll back on failure:

if cache_key not in _labeled_user_hashes:
    _labeled_user_hashes[cache_key] = None  # optimistic mark
    try:
        ...
    except Exception:
        del _labeled_user_hashes[cache_key]

---

Medium — Worth Addressing

9. call_count overcounting across batches (emitter.py:460-503)

The comment on line 477 acknowledges this: "call_count may slightly overcount if a call_id spans multiple batches." This is because each batch increments call_count by the number of unique transaction IDs in that batch. If the same transaction ID appears in two batches (common — a single request generates multiple event types), it's counted twice. The SQL ON CONFLICT DO UPDATE SET call_count = call_count + EXCLUDED.call_count compounds the error.

Consider tracking a set of seen call_ids per session across the batch-write path, or accepting this as a known approximation and documenting it more prominently.

10. Migration 014 SQLite backfill is missing IF NOT EXISTS on ALTER TABLE ADD COLUMN

ALTER TABLE session_summaries ADD COLUMN models_used TEXT;
ALTER TABLE session_summaries ADD COLUMN preview_message TEXT;

The Postgres version uses ADD COLUMN IF NOT EXISTS, but the SQLite version omits the guard. Re-running the migration will fail on the duplicate column. (Note: the migration runner may handle this, but it's inconsistent with the PG version.)

11. Partial index mismatch between PG and SQLite in migration 011

Postgres migration 011 creates partial indexes with WHERE user_hash IS NOT NULL, but the SQLite version creates full indexes. This means query plans may differ between the two backends. Not a correctness issue but worth noting.

---

Test Coverage Gaps

1. No tests for user_hash filter in fetch_session_list — The user_hash parameter is wired through routes and service, but I don't see test coverage for filtering by it.

2. No tests for LIKE wildcard behavior in request_log search — No test verifies what happens when search contains % or _.

3. No multi-batch session summary test — The incremental update logic for _update_session_summaries isn't tested across multiple batches for the same session (call_count accumulation, models deduplication, preview persistence).

4. No shutdown race test — No test verifies that events enqueued during shutdown are not lost.

---

Positive Notes

• User hash extraction (session.py:90-118) is well-designed — no PII leakage, proper SHA-256 fallback, clean regex pattern.
• Session summaries materialization is a solid performance optimization (10s → <100ms).
• Bounded queue with backpressure in the EventEmitter addresses a real OOM crash. The put_nowait + drop semantics are the right choice for telemetry data.
• UserLabelRequest properly validates max_length=255 (routes.py:42-46).
• _MAX_USERNAME_LENGTH = 64 cap on URL prefix usernames (main.py:352) prevents abuse.
• SSE throttling (activity_monitor.js) with 100ms batching and 500-event cap is well-implemented.
• All queries use parameterized values — no raw SQL injection vectors.

---

PR Scope Concern

Per the repo's "One PR = One Concern" guideline, this PR bundles several independently reviewable features: user hash extraction, user labeling, session summaries materialization, bounded EventEmitter, SSE throttling. Consider whether the EventEmitter bounded queue work (which addresses a production OOM) should be its own PR — it's a significant infrastructure change that deserves independent review and its own COE-eligible tracking.

[claude]

PR Review: feat: user differentiation — identify, filter, and label users across the gateway

Overall Assessment

This is a well-structured PR that adds multi-user identification and filtering across the gateway. The architecture is sound — extracting user hashes from request metadata, propagating them through the event pipeline, and materializing session summaries for fast queries. The test coverage is thorough and the migration pairs (Postgres + SQLite) are correctly matched.

That said, I have concerns ranging from moderate bugs to security considerations. See below.

---

Issues

1. Global mutable state for auto-labeling cache (moderate concern)

src/luthien_proxy/pipeline/anthropic_processor.py:77-78

_LABEL_CACHE_MAX_SIZE = 10_000
_labeled_user_hashes: OrderedDict[tuple[str, str], None] = OrderedDict()

This module-level OrderedDict is shared across all concurrent requests with no synchronization. While CPython's GIL makes individual dict operations atomic, the check-then-act pattern at lines 406-427 has a subtle issue: two coroutines can both see cache_key not in _labeled_user_hashes as True simultaneously (the not in check + the subsequent DB write are separated by an await). The optimistic mark (line 409) mitigates duplicate DB writes, but the comment says "rollback on failure" — the except block at line 425 does pop(cache_key, None), which would remove the entry even if a different coroutine successfully wrote it first.

In practice this is low-severity (the ON CONFLICT upsert is idempotent), but the stale cache entries could grow unbounded across process restarts. Consider whether this cache should live on the EventEmitter or be part of a request-scoped dependency instead of a module global.

2. CREATE INDEX CONCURRENTLY in Postgres migration 014 (bug)

migrations/postgres/014_session_summaries_denormalize.sql:6

CREATE INDEX CONCURRENTLY IF NOT EXISTS idx_conversation_events_session_type
    ON conversation_events(session_id, event_type)
    WHERE session_id IS NOT NULL;

CREATE INDEX CONCURRENTLY cannot run inside a transaction block. If your migration runner wraps each migration file in a transaction (which most do, including Django, Alembic, and many custom runners), this will fail with:

ERROR: CREATE INDEX CONCURRENTLY cannot run inside a transaction block

Either remove the CONCURRENTLY keyword or ensure your migration runner supports non-transactional DDL for this specific migration. The SQLite counterpart correctly uses plain CREATE INDEX IF NOT EXISTS.

3. session_summaries.call_count can overcount across batches

src/luthien_proxy/observability/emitter.py:487 (acknowledged in comment at line 488)

The call_count increment uses len(call_ids) per batch. If the same call_id spans two drain batches, it gets counted twice. The comment says "acceptable for an observability summary" — I agree this is fine for display purposes, but worth noting that the session_summaries.call_count may drift upward over time compared to SELECT COUNT(DISTINCT call_id).

If you want exact counts later, you'll need a reconciliation mechanism or a different counting strategy.

4. models_used concatenation can produce duplicates across batches

src/luthien_proxy/observability/emitter.py:501-504

models_used = CASE
    WHEN EXCLUDED.models_used IS NOT NULL THEN
        COALESCE(session_summaries.models_used || ',' || EXCLUDED.models_used, EXCLUDED.models_used)
    ELSE session_summaries.models_used
END

While the Python code deduplicates new models against existing_models_by_session (line 483), there's a TOCTOU race: between the SELECT models_used at line 458 and the INSERT ... ON CONFLICT at line 488, another drain batch could have already appended the same model. This would produce "claude-sonnet-4-20250514,claude-sonnet-4-20250514" in the DB.

The _parse_models_csv() function in service.py deduplicates on read, so this won't affect the UI, but the stored data will accumulate garbage over time. Consider deduplicating in the SQL itself or accepting this as a known imperfection.

5. UserPrefixMiddleware — username not sanitized for DB writes

src/luthien_proxy/main.py:355-363

The middleware extracts parts[2][:64] and stores it as request.state.luthien_user. This value is then written directly to the user_labels.display_name column via parameterized SQL (safe from injection), but there's no validation on the characters. A URL-encoded path like /u/<script>alert(1)</script>/v1/messages would store an XSS payload as the display name.

The history_list.html frontend uses escapeHtml() on display, which mitigates this for the current UI. But if any future consumer renders this label without escaping, it becomes an XSS vector. Consider adding basic alphanumeric validation (e.g., re.match(r'^[a-zA-Z0-9_-]+$', username)) in the middleware itself.

6. History page fetches hardcoded limit=50 — no pagination for initial load

src/luthien_proxy/static/history_list.html (diff line ~563)

The fetch was changed from limit=10000 to limit=50, which is a good performance improvement. However, the client-side filtering (quick filters, search, user filter) now operates on only the first 50 sessions. Users with more than 50 sessions will see incomplete filter counts and missing sessions.

The SessionListResponse includes has_more and total — consider adding a "Load more" button or implementing server-side filtering (the user_hash query parameter is already supported on the API).

7. Credential.value hashed directly — credential rotation changes user identity

src/luthien_proxy/pipeline/session.py:116

return hashlib.sha256(credential.value.encode()).hexdigest()[:_CREDENTIAL_HASH_LENGTH]

When a user rotates their API key, their user_hash changes entirely, creating a new identity in the system. This is documented behavior ("stable user identifier"), but it's worth noting in the docstring that stability is scoped to the lifetime of a single credential. If the API key is the raw secret (sk-ant-...), this also means the hash is derived from a high-entropy secret, which is fine for uniqueness but creates an implicit coupling between identity and credential lifecycle.

---

Positive Observations

• Migration quality: All four migration pairs are correctly matched between Postgres and SQLite, including proper type translations (TIMESTAMPTZ → TEXT, NOW() → datetime('now'), COUNT(*) FILTER (WHERE ...) → SUM(CASE WHEN ... THEN 1 ELSE 0 END)). The backfill queries in 013 and 014 are well-crafted.

• Bounded EventEmitter: The queue-based drain loop with backpressure (QueueFull → drop + log), graceful shutdown, and batch writes is a solid improvement over per-event DB writes. The _batch_drained event for emit() callers that need synchronous confirmation is a nice touch.

• SSE throttle: The 100ms batching in activity_monitor.js with a 500-event buffer cap is appropriate for preventing DOM thrashing during high-throughput scenarios.

• Test coverage: New tests cover user hash extraction (7 test cases), session metadata extraction (17 test cases), emitter batch writes, route handlers for user labels (CRUD), and the UserPrefixMiddleware. The SQLite integration test (test_service_sqlite.py) is particularly valuable for catching DB-compatibility issues.

• Security: All SQL queries use parameterized placeholders. Admin endpoints correctly require verify_admin_token. The UserLabelRequest model has max_length=255 validation. The escapeHtml() usage in the frontend is consistent.

---

Minor / Nit

• deploy.sh outputs secrets to terminal (scripts/aws/deploy.sh:260-262): The Postgres password, proxy API key, and admin key are echoed to stdout. This is fine for initial setup but consider writing them to a file with restricted permissions instead.

• Credential import in session.py: extract_user_hash accepts Credential | None but only uses .value. If this is the only consumer, consider accepting str | None instead to reduce coupling.

• list_users queries conversation_calls for distinct hashes: For large tables, SELECT DISTINCT user_hash FROM conversation_calls will be slow without a dedicated index. Migration 011 adds an index on user_hash but it's a partial index (WHERE user_hash IS NOT NULL), which should help. Consider whether querying session_summaries.user_hash (much smaller table) would be sufficient.

---

Summary

The PR delivers solid multi-user identification infrastructure with good test coverage and migration discipline. The main actionable items are:

1. Fix CREATE INDEX CONCURRENTLY in migration 014 (will fail in transactional migration runners)
2. Add username character validation in UserPrefixMiddleware (defense in depth against stored XSS)
3. Consider server-side pagination for the history page now that the client only fetches 50 sessions

[claude]

Review: PR #580 — User Differentiation

Substantial, well-scoped PR. The user-hash propagation, bounded EventEmitter, and materialized session_summaries are all solid designs. Tests are comprehensive (4 new test files, meaningful coverage of extraction, middleware, emitter, and history routes). A few issues worth addressing before merge.

Security — please address

1. Stored XSS via user_hash (HIGH). _USER_HASH_PATTERN = re.compile(r\"^user_(.+?)_account__\") in pipeline/session.py:81 captures any characters (.+?). An adversarial client on a shared proxy can send:

metadata.user_id = 'user_\"><script>alert(1)</script>_account__session_x'

…which gets stored as-is in conversation_calls.user_hash and then rendered in the admin UI. In static/history_list.html:747:

`<span class=\"user-badge\" data-user-hash=\"${escapeHtml(session.user_hash)}\" ...>`

escapeHtml (line 869–873) uses textContent → innerHTML, which only escapes & < > — it does not escape \" or '. A user_hash containing \" breaks out of the attribute context ⇒ stored XSS against any admin viewing history. The UserPrefixMiddleware username validation (main.py:359) shows the intended pattern — please apply similar server-side validation to extract_user_hash (e.g., restrict captured chars to [A-Za-z0-9_-] or hash the raw value deterministically) and fix escapeHtml to also escape \" and ' as defense-in-depth.

2. Stored XSS via session_id (MEDIUM). OAuth format (session.py:46) and the x-session-id header accept arbitrary strings with no validation. The admin UI uses onclick=\"viewSession('${escapeHtml(session.session_id)}')\" (history_list.html:752); a session_id containing ' escapes the JS string literal. Same escapeHtml gap. Suggest validating session_id server-side (UUID / allowlist) OR moving the click handler to event delegation like the user-badge already does on line 768–773.

Correctness

3. Auto-label overwrites manual labels. In anthropic_processor.py:2544–2554, the auto-label upsert runs ON CONFLICT (user_hash) DO UPDATE SET display_name = EXCLUDED.display_name unconditionally on every first-seen (user_hash, luthien_user) pair per process. If an admin manually sets a display name via the UI and the same user subsequently makes a request through /u/<different>/, the manual label is silently overwritten. Recommend either:

• ON CONFLICT (user_hash) DO NOTHING (manual labels win), or
• track auto-label origin separately (e.g., source column) and only overwrite when the prior row was also auto-generated.

Also worth noting: any authenticated client who controls their own ANTHROPIC_BASE_URL path prefix can claim arbitrary display names for their user_hash — is that intended for shared-proxy deployments?

4. models_used CSV append is fragile. emitter.py:_update_session_summaries stores models as CSV and uses COALESCE(session_summaries.models_used || ',' || EXCLUDED.models_used, ...). Model names could theoretically contain commas (unusual for Anthropic but not structurally prohibited) and _parse_models_csv would split them incorrectly. Also, repeated appends across many batches can grow the string unboundedly if there's no dedup between existing-in-DB and existing-in-batch for the same session across time (the dedup is only against models already fetched). A separate session_models table or JSON array would be more robust, though the current approach is probably fine in practice.

5. Migration 014 uses CREATE INDEX CONCURRENTLY (migrations/postgres/014_session_summaries_denormalize.sql:9), which migrations/CLAUDE.md explicitly tells authors to avoid. The accompanying comment justifies safety (psql autocommit) — acceptable, but please either update migrations/CLAUDE.md to allow CONCURRENTLY with a documented runner contract, or revert to plain CREATE INDEX so the repo rule stays consistent. Right now the guideline and the migration disagree.

6. Migration 014 backfill only populates models_used, not preview_message. Existing sessions will show "(no message preview)" until a new event arrives. If that's intentional (expensive to backfill from event payloads), say so in the migration comment. Otherwise consider a backfill mirror of _extract_session_metadata.

Performance / quality

7. Drain-loop writes are serialized per-batch. _write_db_batch issues one INSERT per call (loop over seen_calls.values()) and one INSERT per event. With batch size 50 that's ~100 round-trips per drain cycle. Using executemany/COPY or a multi-row VALUES (...), (...) statement would cut this by ~10× with negligible added complexity. Not blocking, but this is the hot path the whole PR was built around.

8. list_users endpoint (history/routes.py:118) builds an IN clause with up to 5000 parameters. Postgres is fine up to ~65k params but it's not the cheapest query shape. If a user_hash-indexed join against user_labels is already fast, prefer LEFT JOIN user_labels ON cc.user_hash = ul.user_hash.

9. Dropped-event log message (emitter.py:343) logs self.dropped_events as a running total, which is helpful, but since it's rate-limited to every 10s the operator can't tell how many events dropped in the last 10s. Consider logging (dropped_since_last, total) for incident triage.

Minor

• pipeline/session.py:_USER_HASH_PATTERN — regex match is anchored only at the start (^user_), so a user_id like user_attacker_account__session_x_account__ would still capture attacker. Probably fine given how Claude Code generates these, but worth a $ anchor or stricter format.
• dev/plans/bounded-event-emitter-plan.md (734 lines) is a scaffolding doc — should this be moved to dev/context/ or deleted after merge per the "clear dev/OBJECTIVE.md and dev/NOTES.md" workflow? Leaving large agent plans in-tree adds maintenance debt.
• PR description mentions "Unified config system", "Policy cache infrastructure", "PROXY_API_KEY → CLIENT_API_KEY rename", "CLI additions", "Railway deploy", "docs overhaul" — none of those are in the diff. Either the description is out of date, or those changes landed in prior PRs and should be removed to match the "One PR = One Concern" guideline.

What's good

• Solid test coverage for the new extraction paths and middleware, with edge cases (underscores in hash, OAuth fallback, empty credential).
• Bounded queue with drain loop correctly handles shutdown without cancel-mid-write.
• freeze_configured_state-compatible use of OrderedDict as module-global for the label cache, with a clean eviction cap.
• Nice touch on the max_tokens <= 1 probe filter in _extract_session_metadata — structural rather than content-based.
• data-user-hash / event delegation refactor for click handlers (line 768–773) is the right pattern.

Overall: block on (1) and (3), strongly recommend fixes for (2), (4), (5); the rest is polish.

[claude]

Code Review — PR #580

Thorough, well-tested work that materially improves observability and fixes a real OOM bug. A few things stood out.

Strengths

• Tight defense-in-depth on UserPrefixMiddleware — _USERNAME_RE allowlist plus 64-char cap, with tests covering <script>, spaces, @, empty string. _USER_HASH_PATTERN similarly restricts the capture group specifically to prevent stored-XSS in the history UI — comment explains the reasoning well (src/luthien_proxy/pipeline/session.py:83-86).
• Session summaries materialization is a big win. Moving the history-page GROUP BY to a denormalized table queried via indexed last_seen is the right call, and the drain-loop upsert avoids a refresh job.
• Bounded emitter shutdown logic is careful — using _shutting_down flag instead of cancel() so events already pulled from the queue aren't lost (emitter.py:240-261). Good test coverage for overflow, batch drain, error recovery, and graceful shutdown.
• Tests mirror source structure and cover happy paths, edge cases (OAuth without credential, unrecognized format, empty metadata).

Concerns

1. User-hash spoofing in API-key mode (security). extract_user_hash reads metadata.user_id directly from the client body — any client holding a valid credential can set metadata.user_id = \"user_<target_hash>_account__session_x\" to masquerade as another user in logs and the history UI. Currently mitigated by XSS-safe character class, but the identifier itself is forgeable. At minimum, document this trust model; ideally bind user_hash to the credential (e.g., HMAC(credential, metadata_user_id)) so an attacker can't spoof a known hash without holding that user's credential.

2. Unsalted credential-hash fallback (security). sha256(credential.value)[:16] with no deployment salt means a DB-only leak exposes deterministic fingerprints usable for cross-deployment correlation or rainbow-style lookups of known API-key formats. Add a per-deployment salt (e.g., derived from ADMIN_API_KEY or a dedicated secret) — stability across credentials for a single deployment is preserved.

3. PR scope violates "One PR = One Concern" (CLAUDE.md). This bundles user differentiation + bounded EventEmitter + session_summaries materialization + AWS deploy script + user labeling UI. Each is substantial and could be reviewed/merged independently. The CLAUDE.md explicitly flags this pattern ("Bug fixes bundled into feature PRs bypass the COE process" — the bounded emitter is a fix for a production OOM).

4. Module-level _labeled_user_hashes cache (anthropic_processor.py:77). Mutable module-level state that isn't cleared between requests or tests. In multi-worker deployments (gunicorn/uvicorn with --workers N) each worker has its own cache → inconsistent label attempts. Consider storing on app.state or a dependency-injected object.

5. session_summaries.models_used CSV can grow duplicates across batches. The per-batch dedup at emitter.py:442-456 queries existing models, but if two concurrent drain loops (e.g., after a restart with pending queue) write for the same session, the ||','|| concat runs without cross-batch dedup. Minor — purely cosmetic in the UI — but worth a _parse_models_csv dedup on read, or switching to a DISTINCT-enforcing column layout.

6. Batch writes loop conn.execute instead of executemany/copy_records_to_table. Batch size 50 keeps this fine in practice, but each batch is still 2N+K round-trips. Low-priority perf improvement.

7. Auto-label via /u/{name}/ only fires ON CONFLICT DO NOTHING (anthropic_processor.py:425), while the PUT endpoint does DO UPDATE. Intentional asymmetry? Worth a one-line comment so future readers don't "fix" it.

8. Backfill in migration 013 is a single blocking statement. On a large conversation_events table this may lock startup for minutes. Consider documenting in the PR body that this needs a maintenance window for deployments with >10M events, or chunking it.

Nits

• dev/plans/bounded-event-emitter*.md are task-tracking artifacts — once implementation lands, move to dev/context/ or delete per the CLAUDE.md convention.
• history/routes.py:115-151 returns dict without a response model — typed responses elsewhere in the file use response_model=.
• anthropic_processor.py:404 — the auto-label block is ~35 lines inside process_anthropic_request; consider extracting to _maybe_autolabel_user() for readability.
• list_user_labels (no pagination) will OOM on deployments with many labeled users. Add a limit/offset like list_users.

Overall this is solid work — the materialization rewrite and bounded emitter are both correct and well-tested. The user-hash spoofability is the one that deserves a response before merge.

[claude]

Code review

Overall a well-documented feature with strong test coverage on the happy paths, solid security hygiene (allowlisted username regex, parameterized SQL, documented trust model for client-asserted user_hash), and real performance wins from the materialized session_summaries table. Notes below, ordered roughly by impact.

Scope — violates "One PR = One Concern"

CLAUDE.md is explicit: "Infrastructure change needed for a feature? Separate PR, feature depends on it." This PR bundles at least four independently-reviewable concerns:

1. User differentiation (the stated feature) — session.py, anthropic_processor.py, history routes/UI, user_labels/user_hash migrations, UserPrefixMiddleware
2. Bounded EventEmitter drain-loop refactor (observability/emitter.py +449/-91, with its own 734-line plan in dev/plans/)
3. Session-summaries materialization (migrations 013/014 + history/service.py -193 lines replaced)
4. AWS deploy script (scripts/aws/deploy.sh, 272 new lines)

Each could be reviewed and merged independently. Any one of them reverting would be painful right now.

The PR description also lists changes that are not in the diff (config system overhaul, PROXY_API_KEY → CLIENT_API_KEY rename, CLI restart/agent-tutorial commands, Railway deploy, ARCHITECTURE.md rewrite). Either they silently dropped out or the description was copied from a broader plan — worth reconciling before merge so reviewers and the changelog aren't misleading.

Bugs / correctness

History UI pagination regression. src/luthien_proxy/static/history_list.html:564 drops limit=10000 to SESSIONS_PAGE_SIZE = 50 with no pagination control wired up. The status line honestly says "showing N of M · search/date filters apply to loaded set only", but the user has no way to reach the other M-N sessions. Quick filters like "Last week" now silently operate on only the 50 most recent. This is a real UX regression that deserves either an "Load more" button or a note in the changelog.

io._user_hash accessed outside its class. anthropic_processor.py:781, 871 read the private attribute from a module-level _handle_execution_streaming / _handle_execution_non_streaming. Either promote it to a public attribute (io.user_hash) or thread it through as an explicit parameter like session_id already is.

Auto-label path is untested. anthropic_processor.py:395-436 adds ~40 lines of non-trivial logic: module-level _labeled_user_hashes cache, FIFO eviction, optimistic cache marking, DB insert with ON CONFLICT DO NOTHING, exception swallowing. None of it is covered. test_user_prefix_middleware.py only verifies prefix stripping and request.state.luthien_user assignment. CLAUDE.md's testing guidelines explicitly call this out ("PRs without tests for new functionality will be considered incomplete").

Auto-label revival after admin delete. _labeled_user_hashes is in-memory, per-process. If an admin deletes a label via DELETE /api/history/user-labels/{hash}, the cache still holds (hash, name) so the active process won't re-insert — good. But on process restart, the cache starts empty and the next request from that user through /u/{name}/ will re-apply the auto-label, silently reverting the admin's intentional delete. Either clear the cache entry on DELETE (needs cross-process invalidation — probably not worth it) or use a DB sentinel row / explicit "suppressed" flag. At minimum document this in the auto-label comment block.

Module-level global state leaks across tests. _labeled_user_hashes is defined at module scope in anthropic_processor.py:77. Tests that create multiple create_app() instances (and there are many) share it. Not a practical bug today (idempotent + 10k cap), but a smell — safer to hang it off the app or emitter lifecycle.

Migrations

• Migration 013 backfill uses a correlated subquery (SELECT cc.user_hash FROM conversation_calls ... LIMIT 1) per session. On large deployments this can be slow — consider a LEFT JOIN with DISTINCT ON. Not a blocker.
• Migration 014's decision to skip preview_message backfill and show "(no message preview)" for existing sessions is documented in the SQL comment — good. Worth mirroring in the user-facing changelog so operators aren't surprised.
• CREATE INDEX CONCURRENTLY safety comment ("docker/run-migrations.sh runs each file via psql -f in autocommit") is load-bearing — if that script ever changes to --single-transaction, this migration silently breaks. Consider adding an assertion or at least a # IMPORTANT: marker in run-migrations.sh.

Performance

• Good: GET /api/history/users now scans session_summaries (one row per session, indexed on user_hash) instead of conversation_calls. Correct call.
• list_users builds IN ($1,$2,...) from DB-derived user_hashes — safe, and capped at 5000 by the query limit.
• Bounded queue + batch inserts is a sound design. max_queue_size=10_000, batch_size=50, drain_interval_ms=100 are reasonable defaults but none are configurable via config_fields.py. If this saves the gateway from OOMing, operators will eventually want to tune it.

Security

Nicely handled overall:

• _USERNAME_RE = ^[A-Za-z0-9._-]+$ on the URL prefix — defense-in-depth against stored XSS in display_name.
• _USER_HASH_PATTERN = ^user_([A-Za-z0-9_-]+?)_account__ explicitly restricts the capture group; the XSS test case verifies it.
• Trust-model docstring on extract_user_hash (client-asserted, observability only, not access control) is exactly the kind of note future reviewers need.
• Parameterized queries throughout. The escapeHtml fix to also escape \" and ' matters because user_hash is now interpolated into data-* and title attributes.

Test coverage

Strong on the pieces that were tested:

• test_extract_user_hash.py: API key / OAuth / fallback / XSS / unicode edges — thorough.
• test_user_prefix_middleware.py: validation, length cap, invalid chars, nested paths — parametrized well.
• test_extract_metadata.py (339 lines): probe skipping, content-block handling, system-reminder stripping, dedup, truncation.
• test_routes.py additions: list_users, user-labels PUT/DELETE, pagination, empty cases.

Gaps:

• No test for the _labeled_user_hashes auto-label path (see above).
• No integration test for user_hash flowing end-to-end (request → emitter queue → session_summaries.user_hash → history API).
• Emitter shutdown / drain-on-SIGTERM behavior has unit tests but no e2e — given the scope of the emitter rewrite, at least one sqlite_e2e test through the drain loop would be valuable.

Nits

• anthropic_processor.py:433 — except Exception: with exc_info=True is fine, but the comment says the cache entry is intentionally not rolled back; the log line doesn't mention the cache key, so operators debugging a label that "should have applied" will have trouble correlating. Include user_hash and luthien_user in the warning.
• history/routes.py:130 — SELECT DISTINCT user_hash FROM session_summaries ... ORDER BY user_hash LIMIT ... OFFSET without a total count means the UI dropdown silently truncates at 500 users. Probably fine, worth a total field for symmetry with /sessions.
• changelog.d/user-differentiation.md lumps three concerns into one fragment — consistent with the PR being oversized but worth splitting if the PR splits.

Summary

Strong feature work with a real gap between what was asked (user differentiation) and what was shipped (user differentiation + emitter rewrite + deploy script). I'd push for at least splitting the emitter work into its own PR so it gets an independent review pass, and for adding tests on the auto-label hot path before merge. The pagination regression is the most user-visible issue and should be resolved before release.

[jaidhyani]

Closing this as the vehicle for user differentiation — it's become a poor one. This was a 40-file kitchen-sink PR, and roughly half of its diff has since landed on main independently:

Already merged separately (now redundant here):

• Unified config system (config_fields.py / config_registry.py / /config dashboard)
• PROXY_API_KEY → CLIENT_API_KEY rename
• luthien restart and luthien agent-tutorial CLI commands

Still unmerged — the actual user-differentiation core:

• User-hash extraction (API-key fingerprint / OAuth subject) + propagation through the event pipeline
• session_summaries materialized table (denormalized models + preview)
• user_labels table + click-to-label UI
• History UI per-user filter dropdown + colored user badges

Resurrecting this branch would mean rebasing 40 files across 600+ commits where half the changes conflict-or-no-op against work that already landed. Cleaner to reimplement just the user-differentiation slice on current main. A focused replacement PR is being prepared now and will be linked here. The user-differentiation branch is preserved as the reference implementation.

---

Posted by Claude Code (Opus 4.8)

[jaidhyani]

Replacement PR opened: #780 — reimplements the user-differentiation core on current main (user_id propagation, session_summaries + user_labels tables with paired migrations, history filter/badges/labeling UI). It reuses main's existing user_id concept (from #743/#577) instead of reintroducing a parallel user_hash column.

---

Posted by Claude Code (Opus 4.8)