PR #614 review follow-ups

[jaidhyani]

PR #614 review follow-ups

Three review follow-ups from PR #614. Investigated all three against the current
checkout; only one corresponds to live code. Details below.

Fixes

FTS backfill computes search text once per row — card 6a0f94f1 (GH #763)

The Postgres backfill in migrations/postgres/014_add_session_search_fts.sql
called _extract_event_search_text(payload) twice per row — once in the
UPDATE ... SET clause and once in the WHERE filter — doubling the per-row
work across the whole conversation_events table on migration.

Fix: compute the value once in a subquery, then use it for both the tsvector and
the IS NOT NULL filter. Results are identical (only rows with non-NULL extracted
text get a search_vector). This mirrors the structure the SQLite-side backfill
already uses (which was correct — single-pass via a subquery — so no SQLite
change was needed).

Regression test (tests/luthien_proxy/unit_tests/utils/test_search.py):
asserts the backfill UPDATE evaluates the helper exactly once. Encodes the
issue's acceptance criterion directly. (A behavioral Postgres test would need a
real server — integration tier — so this is a content-level guard in the existing
unit suite.)

Note: the GH issue named "migration 016 ... or equivalent"; the equivalent in
this tree is 014_add_session_search_fts.sql (migration 016 here is the
unrelated 016_replace_policy_definition_with_policy_type.sql).

Investigated, no change needed

Webhook sender per-attempt httpx.AsyncClient — card 6a0f94ee (GH #761)

Already fixed on main. WebhookSender constructs the httpx.AsyncClient
once in __init__ and reuses self._client across every delivery attempt
(_attempt_send calls self._client.post(...); no per-attempt construction).
The merged webhook changelog fragment (webhook-event-export.md, PR #741) even
documents "Singleton httpx.AsyncClient". Nothing to change.

Passthrough agent column length cap — card 6a125105 (GH #770)

Not applicable to this checkout. There is no agent column on request_logs
(migration 019 adds only user_id), no x-luthien-agent header is read anywhere
in the codebase, and record_inbound_request has no agent parameter. The code
the card describes does not exist here, so there was nothing to cap. If/when the
agent column lands, the cap should be added at write time in
record_inbound_request (truncate, with a warning log).

---

Posted by Claude Code (Opus 4.8)

🤖 Generated with Claude Code

[claude]

Review

Tight, focused PR. The fix is correct and the test is a reasonable static guard. A few notes below.

Migration fix — migrations/postgres/014_add_session_search_fts.sql

Correctness. The new UPDATE is semantically equivalent to the original:

• Original: WHERE … AND _extract_event_search_text(payload) IS NOT NULL excluded NULL rows, so the COALESCE(_extract_event_search_text(payload), '') in SET was effectively dead — it could only run on rows where the helper returned non-NULL. Dropping the COALESCE in the rewrite is therefore safe; the new WHERE extracted.search_text IS NOT NULL does the same filtering.
• The event_type = 'transaction.request_recorded' filter is preserved (now in the subquery).
• IMMUTABLE in theory permits the planner to CSE the duplicate call, but Postgres makes no guarantee it actually does — computing once in a subquery is a real, predictable speedup, not a no-op.

One nit on the comment. The block comment on line 52–54 already explains why, but it overlaps with what the rewritten SQL plainly shows. Per CLAUDE.md guidance ("Default to writing no comments"), I'd shorten it to one line — the why worth keeping is the IMMUTABLE-doesn't-guarantee-CSE point, since that's the non-obvious bit. Not blocking.

Scope sanity. The trigger is still BEFORE INSERT only, so payload UPDATEs would leave search_vector stale. That's pre-existing behavior and out of scope here, but worth a note if payload mutation is ever introduced.

Test — tests/luthien_proxy/unit_tests/utils/test_search.py

It catches the regression it's meant to catch. I verified the slicing logic against the actual file:

• sql.index("UPDATE conversation_events") is unambiguous — there's exactly one such statement in the migration.
• The first ; after that point terminates the UPDATE (the subquery contains no semicolons).
• The trigger body (which also calls _extract_event_search_text) lives well after the UPDATE's terminating ;, so it's correctly excluded from the slice. The count == 1 assertion is accurate for the current state.

Brittleness. This is static SQL parsing, so a future innocuous edit could break it in surprising ways:

• A ; inside a string literal or a single-line -- comment in the UPDATE block would shorten the slice ('transaction.request_recorded' is currently safe, but any new literal containing ; would not be).
• Renaming the helper would (correctly) make count go to 0 and fail loudly — good.
• Adding a second UPDATE later in the migration would not be caught by this test — also fine, but worth knowing.

I think the trade-off is acceptable for a regression guard. A more robust version would normalize whitespace and look for the helper only in the SET/SELECT clauses, but that's overkill here.

Semantic equivalence is not tested. The PR body acknowledges this — verifying the new and old UPDATEs produce identical search_vector results would need a real Postgres in the integration tier. That's the right call given current infra.

Test placement. Putting a Postgres-static test in test_search.py (which is otherwise SQLite FTS5 behavior) is a slight mismatch in scope, but the file already mixes dialect-agnostic helpers and the test is a single function — fine.

Changelog & PR body

Clean. The "investigated, no change needed" section is genuinely useful — explicitly documenting that the webhook httpx.AsyncClient is already a singleton on main and that the agent column doesn't exist yet saves the next reviewer a hunt.

Summary

• Fix: correct, real perf improvement, semantically equivalent.
• Test: useful regression guard; brittleness is acceptable.
• No blocking issues. Optional: shorten the migration comment.