trajectory/track a opencode luthien bridge

[PaoloC68]

• feat(telemetry): default OTEL exporter to HTTP/protobuf instead of gRPC
• chore: add .opencode symlink
• feat: role-based auth separation — reject CLIENT_API_KEY on admin endpoints
• feat: StringReplacementPolicy request-side filtering and dashboard observability
• fix(history): strip policy-context from session preview message
• chore: regenerate .env.example after PR feat(telemetry): default OTEL exporter to HTTP/protobuf instead of gRPC #562 adds OTEL_EXPORTER_OTLP_PROTOCOL field
• feat(health): report database connectivity in /health, add /ready endpoint
• feat: configurable upstream header injection via UPSTREAM_HEADERS env var
• review: add changelog fragment, document config bypass and security surface
• fix: case-insensitive collision check for reserved upstream headers
• feat: link inbound traceparent to Luthien spans for distributed trace correlation
• feat: extract and store user identity from X-Luthien-User-Id header or JWT sub claim
• chore: workspace init for issue 561
• chore: set objective to data retention policy (Feature: Data retention policy and archival for conversation storage #561)
• feat: add conversation data retention with configurable purge and S3 archival
• fix: add missing retention index migration (015) omitted from PR feat: data retention policy with configurable purge and optional S3 archival #571
• feat: add webhook event export for conversation completion
• feat(history): add server-side session search API with full-text content search
• fix(history): consolidate query paths, fix time filter and SQLite search bugs
• refactor(history): inline shared query impl, drop wrapper indirection
• fix(history): address review feedback on feat(history): server-side session search API (supersedes #578) #581
• fix: renumber session search migration from 014 to 016 (014 and 015 already taken)
• chore: add changelog fragments and clear objective for trajectory week 0
• fix: address PR review findings — KeyError→ValueError, JSONL error message, webhook exception callback, boto3 asyncio.to_thread, PolicyContext.for_testing user_id, migration 016 comment
• fix: generate_settings.py emits Field(validation_alias) when env_var != auto-derived name (fixes OTEL_EXPORTER_OTLP_PROTOCOL)
• chore: gitignore .sisyphus/ agent state and local business/meeting notes
• feat(security): gate X-Luthien-User-Id header behind TRUST_USER_ID_HEADER config flag and sanitize input (length limit + strip control chars)
• fix(history): pass client_api_key to check_auth_or_redirect in history_list_page for consistent role-based auth messaging
• chore(telemetry): use direct attribute access for settings.otel_exporter_protocol (field is now guaranteed to exist)
• feat(hooks): add staged-file safety check to pre-commit hook — blocks sensitive docs, agent state, and meeting transcripts
• fix(retention): use dialect-aware purge on SQLite — CTE DELETE RETURNING not supported in SQLite
• docs(processor): clarify non-streaming webhook fires only on success path
• docs(history): update SessionSearchParams.user docstring — user_id filter is live (PR Feature: User identity extraction and storage (who made each request) #554 merged)
• fix(webhook): store task references in _pending_tasks to prevent GC; narrow exception to httpx/asyncio/OSError; document type: ignore
• fix(migration): add comment explaining SQLite 014 lacks IF NOT EXISTS (SQLite limitation, idempotency via migration runner)
• fix(policy): validate StringReplacementPolicy replacement pairs have exactly 2 elements at config load time
• fix(security): strip CRLF from upstream header template expansion to prevent header injection
• fix(retention): make ConversationPurger.start() idempotent — guard against duplicate task creation
• fix(webhook): sanitize URL before logging to prevent credential leakage from embedded auth in WEBHOOK_URL
• fix(security): sanitize JWT sub claim — strip control chars and truncate to 256 chars (same as header extraction)
• fix(main): use _safe_url for webhook startup log to avoid logging embedded credentials
• fix(policy): reject empty from_str in StringReplacementPolicy config at validation time
• fix(security): validate upstream header names against RFC 7230 token spec — reject malformed names at load time
• fix(history): guard int() casts with 'or 0' fallback to prevent TypeError on None aggregates
• fix(history): validate from_time <= to_time in SessionSearchParams at parse time
• fix(search): extend tsvector search to response events (migration 017) — response text was never searchable
• fix(retention): archive all columns to prevent data loss on new columns (session_id, user_id)
• fix(processor): deep-copy initial_request to preserve original snapshot across in-place policy mutations
• fix(webhook): preserve port in _safe_url — debugging internal webhook endpoints
• fix(search): user filter matches user_id column (not session_id prefix)
• style(history): use lazy %-formatting for logger.warning calls instead of f-strings
• docs(service): add SECURITY INVARIANT comment on fetch_session_list f-string SQL construction
• feat(webhook): add WEBHOOK_MAX_PENDING_TASKS cap to prevent unbounded task accumulation when endpoint is slow or down
• fix(auth): check_auth_or_redirect redirects to /login?error=not_configured when ADMIN_API_KEY is unset — was silently granting access
• refactor(webhook): rename _safe_url to safe_url (public property — accessed from main.py)
• fix(migration): change _extract_event_search_text from IMMUTABLE to STABLE — PL/pgSQL functions calling STABLE functions should not be IMMUTABLE
• style(tests): replace inline import("datetime") with standard import at module level
• docs(history): update user query param description — filter now uses user_id (not session_id prefix)
• docs(changelog): document breaking change — admin UI requires ADMIN_API_KEY when LOCALHOST_AUTH_BYPASS=false
• feat(webhook): add WebhookSender.stop() for graceful shutdown — cancel in-flight tasks on gateway exit
• fix(history): skip corrupt turns in fetch_session_detail instead of raising — single bad event no longer makes entire session unviewable
• **feat(upstream-headers): warn at load time when templates reference potentially sensitive env vars (*KEY, SECRET, PASSWORD, DATABASE_URL, REDIS_URL)
• fix(emitter): move dropped_db_writes from class-level to instance-level attribute
• chore: remove customer-identifying information from committed files
• style(history): clarify is_error coercion — True if truthy else None is more explicit than 'or None'
• fix(settings): use uppercase env_var in validation_alias — resilient to case_sensitive=True if ever added
• fix(processor): clarify empty stream error message — 'No response from policy execution' instead of misleading 'blocked' message
• test(webhook): add stop() graceful shutdown tests
• test(history): add SQL injection regression tests for user filter (LIKE escaping)
• style: remove dead result=[] variable in _apply_capitalization_pattern; clarify callback ordering in fire_and_forget
• fix(history): strip tags from session preview messages
• fix(test): override get_api_key in ui/test_routes fixture
• fix(test): pass admin key in ui/test_routes fixture
• fix(main): remove duplicate _webhook_sender.stop() in shutdown path
• chore: set objective to Track A opencode-luthien plugin + gateway multi-provider bridge

[claude]

PR Review: trajectory/track a opencode luthien bridge (#614)

This is a thorough review covering security, code quality, architecture, test coverage, and migrations across all 92 changed files and 86 commits.

---

Process: "One PR = One Concern" Violation

This PR bundles 10+ independent features into a single 7,052-addition PR. Per the project's own CLAUDE.md:

"Bug fix discovered while building a feature? Separate PR. Infrastructure change needed for a feature? Separate PR."

The bundled features (passthrough routes, retention system, webhook sender, session search, user identity extraction, upstream header injection, 5 database migrations, auth changes, etc.) could each be reviewed, tested, and merged independently. Bundling makes it impossible to revert a single feature without reverting everything, and bypasses the COE process for any bugs found later.

Recommendation: At a minimum, the new passthrough routes (Track A feature) should be split from the "Trajectory Week 0" merges since they are clearly separate concerns with different risk profiles.

---

Critical Issues

1. Passthrough routes forward ALL upstream response headers to client

src/luthien_proxy/passthrough_routes.py:177

return Response(
    content=response.content,
    status_code=response.status_code,
    headers=dict(response.headers),
)

The buffered passthrough response forwards every header from upstream (OpenAI, Gemini) back to the client unfiltered. This includes hop-by-hop headers (transfer-encoding, connection, keep-alive) and potentially set-cookie. In contrast, the existing Anthropic /v1/* passthrough in gateway_routes.py:249-252 correctly uses an allowlist (content-type, x-request-id, request-id).

Risk: Header injection from upstream, HTTP response smuggling if transfer-encoding is forwarded through a reverse proxy, unexpected cookie forwarding.

Fix: Apply an allowlist filter to response.headers before forwarding, matching the pattern used by the existing Anthropic proxy.

2. PASSTHROUGH auth mode creates an open proxy to OpenAI/Gemini

src/luthien_proxy/passthrough_auth.py:42-44

if auth_mode == AuthMode.PASSTHROUGH:
    return token or ""

When auth mode is PASSTHROUGH, requests with no token at all are accepted. Combined with server-side API key injection in passthrough_routes.py:83-89, any unauthenticated client that can reach the gateway can make arbitrary OpenAI/Gemini API calls using the server's credentials.

Risk: Unauthorized consumption of server-side API keys. The gateway becomes an open proxy.

Fix: At minimum, require a token even in PASSTHROUGH mode when server-side keys are configured. Consider documenting the threat model clearly.

3. Streaming passthrough doesn't forward upstream status code

src/luthien_proxy/passthrough_routes.py:156

return StreamingResponse(stream_chunks(), media_type="text/event-stream")

StreamingResponse defaults to HTTP 200. If the upstream returns 400, 429, or 500, the client always sees 200. The upstream's actual content type is also ignored (hardcoded to text/event-stream).

Fix: Capture the upstream status code and content type before streaming begins and pass them to StreamingResponse.

---

Medium Issues

4. Unbounded memory in S3 archiver

src/luthien_proxy/retention/archiver.py:91-103

rows = await db_conn.fetch(
    "SELECT * FROM conversation_calls WHERE created_at < $1 ORDER BY created_at", cutoff)
jsonl_lines = [json.dumps(_row_to_dict(row)) for row in rows]
body = "\n".join(jsonl_lines).encode("utf-8")

Loads all qualifying rows into memory, then builds the full JSONL string, then encodes it. For a deployment with millions of old records, this could exhaust memory. Also uses SELECT * which means schema changes adding large columns would increase memory impact.

Fix: Batch into chunks (e.g., 1000 rows) with streaming S3 multipart upload.

5. Module-level httpx clients never closed

src/luthien_proxy/passthrough_routes.py:42-45

_streaming_client = httpx.AsyncClient(...)
_buffered_client = httpx.AsyncClient(...)

These module-level async clients are created at import time and never cleaned up during shutdown. The main.py lifespan doesn't call aclose() on them.

Fix: Either manage them through the lifespan context, or add cleanup calls to the shutdown sequence.

6. Migration 016 backfill evaluates function twice per row

migrations/postgres/016_add_session_search_tsvector.sql:58-62

UPDATE conversation_events
SET search_vector = to_tsvector('english', COALESCE(_extract_event_search_text(payload), ''))
WHERE event_type = 'transaction.request_recorded'
  AND _extract_event_search_text(payload) IS NOT NULL;

Calls _extract_event_search_text() twice per row (SET + WHERE). On tables with millions of events, this doubles the backfill time and can cause long locks.

Fix: Use a subquery or CTE to evaluate the function once.

7. No S3 encryption-at-rest configuration

src/luthien_proxy/retention/archiver.py:107-113

The put_object call doesn't set ServerSideEncryption. Conversation data (potentially sensitive user prompts and model responses) is archived without explicit encryption. While S3 now defaults to SSE-S3, operators with compliance requirements may need explicit KMS encryption.

Fix: Add an optional ARCHIVE_S3_ENCRYPTION config field, or at minimum pass ServerSideEncryption='AES256' as a default.

8. Webhook sender creates new httpx client per delivery attempt

src/luthien_proxy/webhook/sender.py:157

async with httpx.AsyncClient(timeout=SEND_TIMEOUT_SECONDS) as client:

Each retry attempt creates a new connection pool and TLS handshake. With 3 retries, that's 4 TCP connections per webhook event.

Fix: Create one client in _send_with_retries and reuse it across attempts, or use a shared client.

---

Low Issues

• history/models.py: No length limits on SessionSearchParams string fields (user, model, q). A multi-MB search string would be passed to plainto_tsquery() or LIKE.
• passthrough_routes.py:143,164: body or None converts empty b"" to None, which changes HTTP semantics (no-body vs empty-body).
• history/service.py:544-547: count_params and row_params diverge subtly via positional snapshotting — fragile and easy to break in future edits.
• main.py:181-268: get_settings() called 5+ times during startup; settings variable is shadowed at line 268.
• passthrough_routes.py:83,88: os.environ.get() called on every request for API keys — inconsistent with lru_cache approach used by upstream_headers.py.
• Duplicate migration prefixes: 014, 015, and 016 each have two migration files. While functional (lexicographic ordering), this is fragile and the project conventions discourage it.

---

What's Good

• Test coverage is excellent. All new modules have comprehensive test suites covering happy path, error cases, edge cases, and security-sensitive input. The regression tests in test_search_regressions.py use real SQLite queries rather than mocks — well-designed.
• Security hardening is thorough. CRLF injection prevention in upstream headers, JWT sub claim sanitization, credential leak prevention in URL logging (safe_url), SQL injection prevention (parameterized queries throughout), and RFC 7230 header name validation.
• Migration parity is correct. All Postgres migrations have matching SQLite variants. Type translations follow the documented rules. Embedded copies are identical.
• Webhook sender design — backpressure cap (max_pending_tasks), graceful shutdown (stop()), and the _log_task_exception callback are all solid patterns.
• Emitter fix — moving dropped_db_writes from class-level to instance-level prevents test state leaks.
• Auth role separation — rejecting CLIENT_API_KEY on admin endpoints with clear error messages is a meaningful security improvement.

---

Summary

Severity	Count	Key Themes
Critical	3	Open proxy via passthrough auth, unfiltered response headers, silent status code swallowing
Medium	5	Unbounded memory in archiver, resource leaks, migration perf, missing encryption config
Low	6	Input validation gaps, style nits, fragile parameter handling

Blocking recommendations before merge:

1. Fix the three critical issues in the passthrough routes (response header filtering, auth in PASSTHROUGH mode, streaming status code forwarding)
2. Add batching to the S3 archiver to prevent OOM on large datasets
3. Strongly consider splitting the passthrough routes (Track A) into a separate PR from the Week 0 merges

[claude]

PR #614 Code Review

This is a very large PR (7,244 additions across 92 files) bundling dozens of features, fixes, and migrations. The review below focuses on the highest-risk findings across security, correctness, and test coverage.

---

CRITICAL

1. SSRF via unvalidated upstream base URL env overrides (passthrough_routes.py:33-38)

_upstream_base() trusts OPENAI_BASE_URL, GEMINI_BASE_URL, ANTHROPIC_BASE_URL env vars without validation. If an attacker can influence env vars (shared hosting, PaaS config injection, .env compromise), they can redirect traffic to internal hosts. Combined with the user-controlled path parameter being unsanitized, this is a potent SSRF vector for a proxy service.

Recommendation: Validate resolved upstream URLs use https://, block RFC 1918 / link-local / loopback addresses, and sanitize path to reject .. or encoded traversal sequences.

2. No blocklist for security-sensitive header names (upstream_headers.py:52-53, 122-141)

_HEADER_NAME_RE validates RFC 7230 token syntax, but nothing prevents UPSTREAM_HEADERS from overriding Authorization, x-api-key, Host, or Content-Length. A misconfigured UPSTREAM_HEADERS could override the credential-resolved auth header.

Recommendation: Add a _BLOCKED_HEADER_NAMES frozenset and reject them in _load_header_templates().

---

HIGH

3. Streaming passthrough returns hardcoded HTTP 200 (passthrough_routes.py:156)

StreamingResponse always returns 200. By the time the generator reads the upstream status code, FastAPI has already committed the response line. An upstream 401 or 429 silently appears as 200.

4. Streaming media_type hardcoded to text/event-stream (passthrough_routes.py:156)

The upstream actual Content-Type is ignored. Gemini streaming may use application/json (newline-delimited), breaking clients that check media type.

5. Response headers forwarded without filtering (passthrough_routes.py:174)

The buffered response path forwards dict(response.headers) verbatim, including hop-by-hop headers (transfer-encoding, connection, keep-alive). The existing Anthropic route in gateway_routes.py:249-252 correctly filters to an allowlist.

6. JWT sub claim used without signature verification (session.py:114-153)

extract_user_id_from_bearer_token() decodes JWT payload without signature verification. The extracted user_id flows into conversation events, PolicyContext, OTel spans, and webhook payloads. An attacker can craft a JWT with arbitrary sub to impersonate users in audit logs.

7. Non-atomic archive-then-delete risks data loss (purger.py:76-116, archiver.py:91)

The archive+purge sequence is: SELECT rows, S3 upload, DELETE by timestamp. Between SELECT and DELETE, rows matching the cutoff could be inserted and then deleted without being archived. On SQLite, the COUNT + DELETE are separate statements without a transaction.

Recommendation: Use SELECT ... FOR UPDATE on Postgres, or archive by row ID rather than timestamp. Wrap SQLite COUNT+DELETE in an explicit transaction.

8. _build_outbound_headers (core security function) is completely untested (test_passthrough_routes.py)

This function controls credential stripping, auth header injection per provider, and x-luthien-* removal. Zero direct test coverage. The streaming path (lines 133-156) is also entirely untested.

9. CLIENT_KEY mode with api_key=None: silent full lockout (passthrough_auth.py:46-66)

When auth_mode=CLIENT_KEY but CLIENT_API_KEY is not configured, all requests are silently rejected with 401. No log, no startup warning. BOTH mode with no key silently degrades to PASSTHROUGH.

---

MEDIUM

• Module-level httpx.AsyncClient singletons never closed (passthrough_routes.py:42-45): Created at import time, never aclose()d. Leaks connections in long-running deployments. Move to FastAPI lifespan.
• New httpx.AsyncClient per webhook attempt (sender.py:157): Each _attempt_send creates a new client. Under load, thousands of TCP connections churn. Create a shared client.
• No pagination/batching in archive or purge (archiver.py:91, purger.py:85-96): Loads ALL matching rows into memory. Can OOM the process and exceed S3 put_object limit.
• system field not covered by request-side filtering (string_replacement_policy.py:222): Processes messages but skips system. For prompt injection defense, patterns in the system prompt would be missed.
• Replacement counting inaccurate with interacting patterns (string_replacement_policy.py:235-241): Counts against pre-replacement text but replacements applied sequentially. Also duplicated 4 times (DRY violation).
• No input length limits on q and user search params (history/routes.py:88-106): A multi-megabyte q string would be expensive to tokenize/search. Add max_length.
• Migration 016: text_pattern_ops index on session_id unused: No service query does LIKE on session_id. Adds write overhead without benefit.
• X-Luthien-User-Id not stripped when trust disabled (session.py:86): Header is ignored but not removed. Future code reading headers directly could accidentally trust it.

---

TEST COVERAGE GAPS

1. _build_outbound_headers: zero coverage for core security function in passthrough routes
2. _is_streaming: no tests for streaming detection (Gemini path, OpenAI body parsing)
3. Streaming passthrough path: entirely untested (where the status-code bug lives)
4. CLIENT_KEY mode with api_key=None: no test for silent lockout
5. SQL injection via q param: user injection tested but q is not
6. match_capitalization with apply_to=request: not tested together
7. Empty-string replacement (deletion): valid per validator but never tested
8. Concurrent fire_and_forget + stop(): no test for shutdown-during-traffic race

---

POSITIVE OBSERVATIONS

• SQL injection prevention is solid: add_param() / dollar-N placeholder pattern with security invariant comments.
• LIKE wildcard escaping is correct: escape_like() properly implemented with regression tests.
• Credential sanitization in logs: safe_url strips userinfo/query params from webhook URLs.
• Policy singleton safety: Working state extracted into immutable self._replacements tuple.
• Streaming buffer design: Double-flush protection via pop_request_state / get_request_state.
• Test quality: Regression tests in test_search_regressions.py hit real SQLite with actual migrations.

---

OVERALL ASSESSMENT

Code quality is generally good: parameterized queries, credential stripping, immutable policy state, solid regression tests. However, the passthrough routes module has the densest cluster of issues: SSRF risk, incorrect streaming error propagation, header forwarding without filtering, and critically low test coverage. I would recommend addressing the CRITICAL and HIGH items in passthrough routes before merging, or splitting that module into its own PR for focused review.

The retention/archival system has a real data-loss risk from non-atomic archive+delete. The search and history changes look solid.

Generated with Claude Code

[claude]

PR Review: trajectory/track-a-opencode-luthien-bridge (#614)

Scope: 98 files, +7259/-419 lines. Bundles ~60 commits spanning retention system, webhook events, session search, passthrough proxy (OpenAI/Gemini), upstream headers, user identity extraction, auth hardening, and numerous security fixes.

Overall assessment: The code is well-structured with good documentation, proper error handling, and thoughtful security measures (parameterized SQL, constant-time auth comparison, URL sanitization, user ID sanitization). The issues below are mostly medium-severity and addressable. The biggest concern is the PR size — per the repo's own "One PR = One Concern" rule, this bundles many independent changes that could be reviewed and merged separately.

---

High Severity

1. Module-level httpx clients never closed — resource leak
src/luthien_proxy/passthrough_routes.py lines 42, 45

_streaming_client and _buffered_client are created at module import time and never closed. They aren't referenced in main.py's lifespan shutdown. This leaks connections/file descriptors and causes ResourceWarning on interpreter shutdown. These should be created during app lifespan and closed on shutdown.

2. Streaming passthrough always returns HTTP 200 regardless of upstream status
src/luthien_proxy/passthrough_routes.py line ~156

StreamingResponse is created with the default 200 status code, even when the upstream returns 4xx/5xx. Clients see HTTP 200 with the error body inside the stream. The buffered path (line ~176) correctly forwards the upstream status code. The streaming path should capture and forward it too.

3. OTEL endpoint/protocol default mismatch
src/luthien_proxy/telemetry.py, config_fields.py

The default protocol was changed to http/protobuf but the default endpoint remains http://tempo:4317 (a gRPC port). The HttpSpanExporter appends /v1/traces and sends HTTP to port 4317, which is Tempo's gRPC port — the HTTP OTLP port is typically 4318. Users enabling OTEL with defaults will get silent export failures. Either update the default endpoint to port 4318 or prominently document the incompatibility.

---

Medium Severity

4. Unbounded SELECT * in archiver loads all archivable rows into memory
src/luthien_proxy/retention/archiver.py line ~92

archive_calls fetches ALL rows older than cutoff into memory at once, then builds the full JSONL body in memory before uploading. For deployments with long retention windows, this can OOM. Consider batched fetching with a LIMIT in a loop.

5. Archive-then-delete is not atomic
src/luthien_proxy/retention/purger.py lines 78-96

If the process crashes between S3 upload completing and the DELETE executing, the next run re-archives the same rows (creating S3 duplicates). The archive is idempotent from a data-safety perspective (no data loss), but the reverse case (archive succeeds, delete fails) deserves at minimum a comment, and ideally a deduplication key in the S3 path.

6. Upstream response headers forwarded without filtering
src/luthien_proxy/passthrough_routes.py line ~177

For buffered responses, headers=dict(response.headers) forwards ALL upstream headers including transfer-encoding, content-encoding, and connection. Forwarding transfer-encoding: chunked on a buffered response that FastAPI will re-chunk, or content-encoding: gzip when httpx already decompressed the content, will cause client failures. Hop-by-hop and encoding headers should be stripped.

7. Migration number collisions
migrations/postgres/014-016, migrations/sqlite/014-016

These prefix numbers collide with existing migrations on main (014_fix_auth_mode_default, 015_add_policy_definition, 016_replace_policy_definition_with_policy_type). migrations/AGENTS.md line 16 explicitly says "do not reuse prefixes." The migration runner sorts by filename so both files get applied, but the ordering is confusing. Consider renumbering to avoid collisions.

8. SQLite full-text search has no query timeout
src/luthien_proxy/history/service.py lines 446-505

The SQLite fallback for full-text search uses json_tree() with LIKE patterns, which does a full scan. For large datasets this will be very slow with no timeout or row limit to prevent blocking the connection pool.

---

Low Severity

9. webhook_url not marked sensitive in config
src/luthien_proxy/config_fields.py line ~258

Webhook URLs may contain auth tokens in query strings or userinfo. The WebhookSender.safe_url property strips these for logging (good), but the config dashboard will display the full URL. Consider marking sensitive=True.

10. Replacement counting logic duplicated 4 times
src/luthien_proxy/policies/string_replacement_policy.py lines 234-298

The same counting pattern (iterate replacements, call re.findall or content.count) is repeated for string content, text blocks, tool result strings, and tool result text blocks. Should be extracted to a helper.

11. Plugin URL inconsistency
.gitmodules references https://github.com/PaoloC68/opencode-luthien (personal fork) while docs/plugin-header-contract.md references https://github.com/LuthienResearch/opencode-luthien (org repo). These should be consistent.

12. Broken link in docs/plugin-header-contract.md
Line 5 references .sisyphus/plans/track-a-opencode-luthien-bridge.md which is in .gitignore and blocked by the pre-commit hook. The link will always be broken for other contributors.

13. Unused field_validator import in settings generator
scripts/generate_settings.py line 90 outputs field_validator in the import but the generated Settings class never uses @field_validator. Ruff silently strips it, but the generator output should be correct without relying on autoformatting.

---

Missing Test Coverage

• /ready endpoint (main.py): No tests for this production-critical load-balancer endpoint. /health has tests, /ready does not.
• Retention/webhook wiring in lifespan (main.py lines ~285-320): Individual components are well-tested, but the lifespan wiring (settings → archiver → purger → start/stop) has no test.
• Streaming passthrough path (passthrough_routes.py lines ~133-156): Only covered by e2e tests. No unit test for the error branch (except httpx.RequestError).
• PolicyContext.record_event with user_id (policy_context.py): New user_id field propagation through record_event lacks a dedicated test.

---

Security Positives (good changes in this PR)

• Auth bypass when ADMIN_API_KEY unset: Fixed. Previously check_auth_or_redirect silently granted access; now redirects to /login?error=not_configured.
• Role separation enforced: CLIENT_API_KEY correctly rejected on admin endpoints with specific 403.
• SQL injection prevention: All user-supplied values in history search go through add_param() with a documented SECURITY INVARIANT comment. Regression tests for LIKE escaping included.
• CRLF injection prevention: Upstream header template expansion strips \r and \n.
• User ID sanitization: Control character stripping and 256-char length limit applied consistently to both header-based and JWT-based extraction.
• Credential leak prevention: safe_url strips userinfo/query params before logging; x-goog-api-key added to sensitive header redaction.
• Constant-time auth comparison: secrets.compare_digest used in passthrough auth.

---

Architecture Observation

This PR bundles ~60 independent commits across 6+ features. Per the repo's own guidance ("One PR = One Concern" / "Bug fixes bundled into feature PRs bypass the COE process"), the retention system, webhook system, session search, passthrough proxy, upstream headers, and the numerous security fixes could each be separate PRs. This would make review more tractable and allow independent merging.

🤖 Generated with Claude Code

[claude]

PR Review: #614 — Track A opencode-luthien bridge

This is a very large PR (99 files, ~7,300 additions) that bundles 10+ feature PRs from the Trajectory Labs engagement plus the Track A opencode-luthien bridge. Given the scope, this review focuses on the most impactful areas: security, architectural concerns, code quality, and test coverage.

---

🔴 High-Priority Issues

1. Passthrough auth + server-side key injection creates an open proxy in PASSTHROUGH mode

Files: passthrough_auth.py, passthrough_routes.py

In PASSTHROUGH auth mode, verify_passthrough_token accepts any token or no token at all (line 44: return token or ""). Meanwhile, passthrough_routes.py replaces the client's auth with server-side keys for OpenAI (OPENAI_API_KEY) and Gemini (GOOGLE_API_KEY).

This means in PASSTHROUGH mode, any client who can reach the gateway can spend the operator's OpenAI/Gemini credits with zero authentication. The naming is also misleading: "passthrough" implies the client's own key is forwarded, but for OpenAI/Gemini the opposite happens — the server's key is injected.

Similarly in BOTH mode (line 58): after the CLIENT_API_KEY check fails, any non-empty token is accepted. This makes BOTH mode effectively identical to PASSTHROUGH for passthrough routes.

Recommendation: Either (a) require CLIENT_API_KEY validation for passthrough routes regardless of auth mode, or (b) document this prominently and add a startup warning when PASSTHROUGH mode + server-side keys are both configured.

2. No request body size limit on passthrough routes

File: passthrough_routes.py:111

body = await request.body() reads the entire request body into memory with no size check. The Anthropic pipeline has MAX_REQUEST_PAYLOAD_BYTES enforcement, but the passthrough routes bypass it entirely. A malicious client can OOM the process.

Recommendation: Add a content-length check mirroring the Anthropic pipeline's approach.

3. Module-level httpx clients are never closed

File: passthrough_routes.py:42-45

_streaming_client and _buffered_client are created at module import time but never added to the lifespan shutdown sequence. This will produce warnings/resource leaks on shutdown.

Recommendation: Move client creation into the lifespan context manager and close them during shutdown.

4. Upstream response headers forwarded verbatim to client

File: passthrough_routes.py:174-178

return Response(
    content=response.content,
    status_code=response.status_code,
    headers=dict(response.headers),
)

This forwards all upstream headers to the client, including transfer-encoding, content-encoding, set-cookie, access-control-allow-origin, etc. Forwarding transfer-encoding: chunked on a non-streaming response will break the response. Forwarding set-cookie from the upstream could interfere with the gateway's session management.

Recommendation: Allowlist response headers or at least strip hop-by-hop headers (transfer-encoding, connection, etc.).

---

🟡 Medium-Priority Issues

5. S3 archiver loads all rows into memory at once

File: retention/archiver.py:91

rows = await db_conn.fetch("SELECT * FROM conversation_calls WHERE created_at < $1 ...") loads all qualifying rows into memory. On the first run after enabling retention (with months of accumulated data), this could exhaust memory.

Recommendation: Add batched fetching with a configurable batch size (e.g., 10,000 rows at a time).

6. Webhook sender creates a new httpx.AsyncClient per delivery attempt

File: webhook/sender.py:157

async with httpx.AsyncClient(timeout=SEND_TIMEOUT_SECONDS) as client:

Under high load with retries (up to 1000 pending × 4 attempts), this creates many short-lived connections. Consider a persistent client instance with connection pooling.

7. _extract_event_search_text function volatility mismatch

File: migrations/postgres/016_add_session_search_tsvector.sql

The function is declared STABLE but is called from the backfill UPDATE query directly. While not incorrect, migration 017 updates the trigger function without also updating the backfill to cover response events it may have missed from the initial 016 backfill. The 017 migration does handle this with AND search_vector IS NULL — good catch.

However, the 016 migration calls _extract_event_search_text() twice in the WHERE and SET clauses of the backfill UPDATE, which is inefficient for large tables.

8. SQLite search uses json_tree with complex GLOB patterns

File: history/service.py (SQLite search path)

The SQLite full-text search implementation uses deeply nested json_tree + GLOB + LIKE queries. This is O(n) per conversation event with no index support, and the query is quite fragile (relies on jt.fullkey patterns matching the JSON structure exactly). This is documented as "slow for large datasets" which is fair, but the query complexity makes it hard to maintain.

9. check_auth_or_redirect behavior change when admin_key is unset

File: auth.py:142

Previously, if not admin_key: return None (grant access). Now it redirects to /login?error=not_configured. This is a breaking change for deployments that intentionally run without ADMIN_API_KEY set. The changelog fragment should document this prominently.

---

🟢 Positive Observations

1. Security hardening is thorough. The JWT extraction correctly avoids signature verification (attribution only), input sanitization strips control chars, CRLF injection is prevented in upstream headers, and the TRUST_USER_ID_HEADER flag defaults to false.

2. SQL injection protection in history/service.py is well-designed with the add_param() closure pattern and the explicit SECURITY INVARIANT comment. The LIKE escaping helper is correct.

3. Parameterized queries throughout — retention purger, archiver, emitter all use $1 placeholders.

4. EventEmitter.dropped_db_writes moved from class-level to instance-level — good fix for shared mutable state.

5. Webhook backpressure mechanism with max_pending_tasks cap prevents unbounded resource growth.

6. Deep copy of initial_request in _AnthropicPolicyIO.__init__ — prevents policies from mutating the original snapshot. Good defensive coding.

7. _extract_preview_message now strips <policy-context> tags — prevents internal metadata from leaking into the UI.

8. Corrupt turn tolerance in fetch_session_detail (try/except around _build_turn) — prevents a single bad event from making an entire session unviewable.

9. Migrations are well-structured with matching Postgres/SQLite pairs, appropriate indexes, and clear ABOUTME comments.

10. Test coverage is strong overall — unit tests for passthrough auth, passthrough routes, upstream headers, webhook sender, retention purger/archiver, search regressions including SQL injection tests, and e2e tests for the passthrough routes.

---

📋 Minor / Style Notes

• telemetry.py:148 still uses f-string for logging: logger.info(f"OpenTelemetry configured: {service_name}..."). The rest of the codebase was updated to use lazy %-formatting. Consistency issue.
• passthrough_routes.py:42-45: Global mutable state (_streaming_client, _buffered_client) could cause issues in testing. Consider making them injectable.
• The .gitmodules file references PaoloC68/opencode-luthien.git (personal fork) — should this point to LuthienResearch/opencode-luthien instead?
• scripts/track_a_smoke.sh writes evidence to .sisyphus/evidence/ which is gitignored. The script itself is fine but has hardcoded port numbers (9000-9002, 8000) that could collide with other services.

---

🏗️ Architectural Concern: PR Size

Per the repo's own guidelines in CLAUDE.md: "One PR = One Concern" and "Bug fixes bundled into feature PRs bypass the COE process." This PR bundles ~15 distinct features and numerous bug fixes. While I understand this is a batch merge of a contractor engagement, the sheer scope (99 files) makes thorough review difficult and any issues harder to bisect. For future batches, consider at least grouping by concern area (auth, observability, data layer) into separate merge PRs.

---

Overall: The code quality is generally good with solid security practices. The main concerns are around the passthrough route security model (open proxy risk), missing body size limits, and resource management of httpx clients. The test coverage is comprehensive for most new modules.

[PaoloC68]

Split into 3 focused PRs per .sisyphus/plans/pr614-split-and-fix.md:

• PR-A (Foundation): feat(track-a): foundation — header contract + httpx-sse #757
• PR-B (Passthrough + Security): feat(track-a): multi-provider passthrough routes + 4 security fixes #758
• PR-C (Plugin Integration): feat(track-a): opencode-luthien plugin integration #759

All 4 security findings from the review are addressed in PR #758 with explicit test coverage. The 5 in-main findings are filed as separate GH issues (label: from-pr-614-review).

Original branch state preserved at tag rebase-backup-pr614 on origin.