Skip to content

feat(track-a): foundation — header contract + httpx-sse - #757

Open
PaoloC68 wants to merge 5 commits into
mainfrom
trajectory/track-a-pr-a-foundation
Open

feat(track-a): foundation — header contract + httpx-sse#757
PaoloC68 wants to merge 5 commits into
mainfrom
trajectory/track-a-pr-a-foundation

Conversation

@PaoloC68

@PaoloC68 PaoloC68 commented May 21, 2026

Copy link
Copy Markdown
Collaborator

Part 1 of 3 splitting PR #614.

Changes

  • docs/plugin-header-contract.md: Documents the header contract for the opencode-luthien plugin
  • pyproject.toml: Adds httpx-sse>=0.4 dependency for streaming passthrough routes
  • uv.lock: Updated lock file

Notes

Closes part of #614.

@claude

claude Bot commented May 21, 2026

Copy link
Copy Markdown
Contributor

Review: PR #757 — Track A Foundation

Reviewed as Part 1 of 3 splitting PR #614: pure docs + dependency addition, no behavior change. Scoping is clean (well-aligned with the repo's "One PR = One Concern" guidance), and httpx-sse is a sensible, small dependency for upcoming SSE passthrough. The contract-doc structure with explicit allowed/forbidden change types is a strong pattern.

Findings below, ranked roughly by severity.


Blocking

1. Changelog fragment is missing required YAML frontmatterchangelog.d/track-a-foundation.md

scripts/compile_changelog.py:37-39 requires ^---\n…\n---\n and will sys.exit("Bad fragment format … missing YAML frontmatter") at release time. The current file has none. Per changelog.d/README.md, it should look like:

---
category: Chores & Docs
pr: 757
---

**Track A foundation**: Plugin header contract docs and `httpx-sse` dependency for upcoming multi-provider passthrough routes.

(Categories: Features, Fixes, Refactors, Chores & Docs.)


High — documentation correctness

2. Migration reference points to the wrong numberdocs/plugin-header-contract.md:97

The doc says migrations/postgres/018_add_agent_to_request_logs.sql, but 018 is already taken by 018_add_user_id.sql. The new migration in PR-B will need to be 019+ — and migrations/CLAUDE.md explicitly warns against reusing prefixes (008 is the cautionary tale).

3. Doc asserts a column/migration that doesn't exist yetdocs/plugin-header-contract.md:30, 97

Persisted to request_logs.agent (dedicated column, added by migration 018)

Neither the column nor the migration exists in this PR (confirmed against migrations/postgres/008_add_request_logs_table.sql). A reader of the v1.0 contract today would get a false picture of the current schema. Consider annotating as (planned, lands in PR-B) or splitting the contract into "implemented today" vs. "introduced in PR-B".

4. Broken relative linkdocs/plugin-header-contract.md:5

The link to .sisyphus/plans/track-a-opencode-luthien-bridge.md 404s — that directory isn't in the repo (appears to be a private/local planning path). Drop the link or replace with a public reference (e.g., this PR or PR #614).

5. Forward reference to non-existent filedocs/plugin-header-contract.md:96

src/luthien_proxy/passthrough_routes.py doesn't exist yet. Fine to mention, but worth annotating "(introduced in PR-B)" so readers don't grep in vain.


Medium — contract clarity & security

6. No trust-boundary statement. The contract is silent on whether x-luthien-* headers from end clients should be trusted, stripped on ingress, or only accepted from authorized plugin sessions. If an authenticated client can spoof x-luthien-session-id, they can poison logs or impersonate another session's trail. Same concern applies to x-luthien-plugin-version masking the real client identity. Worth at least a sentence on the threat model — even if the answer is "the gateway trusts whatever it receives, deployer must front it with auth."

7. No header value validation rules. These values land in indexed request_logs columns (session_id, model, future agent). The contract should specify max length, allowed character set, and whether session_id is validated as a UUID. Without this, the gateway either has to invent rules or risk surprise (e.g., a 64KB session_id is a valid HTTP header but a bad DB row).

8. x-luthien-provider unknown-value handling unspecified. The doc enumerates anthropic | openai | google but says nothing about what the gateway does with an unexpected value (reject, log-and-pass, normalize). This matters for forward-compatibility — adding a new provider currently requires a contract bump.

9. Internal contradiction on default vs. NULL. Section 1.2 (x-luthien-agent) says it "defaults to \"unknown\" when agent name is unavailable", but the "Missing headers" paragraph says the corresponding column is NULL when the header is absent. Which is it — does the plugin send "unknown", or does the gateway store NULL? They produce different query behavior.

10. No contract-version header. The plugin sends x-luthien-plugin-version, but the gateway has no programmatic way to know which contract revision the plugin speaks. If the contract evolves to v1.1 / v2.0, the gateway will have to infer from plugin version, coupling them. A dedicated x-luthien-contract-version header would decouple plugin releases from contract revisions and make version-skew detection explicit.


Low / nits

11. Loose dependency pin. httpx-sse>=0.4 has no upper bound. httpx-sse is small and stable, but >=0.4,<1 would guard against a future breaking 1.0. Matches the style of anthropic>=0.84.0 (open) but not litellm>=…,!=1.82.7,!=1.82.8 (constrained). Take it or leave it.

12. Versioning scope ambiguity. The doc opens with Version: v1.0 but doesn't clarify that this is the contract version, independent of the plugin npm version. A one-liner would help.


What I checked

  • Diff against origin/main (4 files, +106/-0)
  • Existence of referenced migrations/files (008_add_request_logs_table.sql ✓, 018_add_agent_to_request_logs.sql ✗, passthrough_routes.py ✗, .sisyphus/ ✗)
  • httpx-sse usage in src/ and tests/ (none yet — expected for PR-A)
  • Changelog fragment format vs. scripts/compile_changelog.py requirements
  • Migration numbering conventions in migrations/CLAUDE.md

No test coverage required for this PR (pure docs + dep, no functional code).

- changelog: add required YAML frontmatter (category + pr)
- contract: fix broken .sisyphus link → PR #757 reference
- contract: fix migration ref 018 → 019 (collision resolved in PR-B)
- contract: annotate forward refs (passthrough_routes.py, agent column) as 'introduced in PR-B'
- contract: clarify agent NULL vs unknown — header absent → NULL; plugin sends 'unknown' when unavailable
- contract: add Trust Boundary section (gateway trusts headers as received; operator responsibility)
- contract: add header validation rules (session_id max 36 chars/UUIDv4, agent max 64 chars)
- contract: document provider unknown-value handling (log + pass through, no rejection)
- contract: clarify contract version is independent of plugin npm version
@claude

claude Bot commented May 23, 2026

Copy link
Copy Markdown
Contributor

Code Review — PR #757 (Track A foundation)

Scope is small (docs + one dep), so this review focuses on the doc contract since it'll be referenced from downstream PRs and external plugin code.

Findings

1. Inaccurate truncation claim (docs/plugin-header-contract.md:30)

Values longer than 36 characters are truncated to fit the column.

session_id TEXT in both Postgres (migrations/postgres/008_add_request_logs_table.sql:10) and SQLite has no length bound, and I don't see any truncation logic in src/luthien_proxy/request_log/recorder.py. Either drop the truncation claim or add explicit validation in PR-B and document the actual behavior (reject? truncate? log a warning?). Right now this would mislead a plugin author into thinking the gateway is enforcing something it isn't.

2. Trust-boundary advice is incomplete (docs/plugin-header-contract.md:13)

The doc tells operators to "strip inbound x-luthien-* headers before they reach the gateway." But x-luthien-user-id already exists (src/luthien_proxy/pipeline/session.py:23) and is gated by the TRUST_USER_ID_HEADER config. A blanket strip rule would silently disable user attribution where an operator has intentionally enabled it. Worth either:

  • Cross-referencing TRUST_USER_ID_HEADER so operators understand the interaction, or
  • Narrowing the advice to the specific headers documented here (session-id, agent, provider, model, plugin-version).

3. x-luthien-provider is presented as both closed and open enum (lines 50–52)

The type row says one of: anthropic, openai, google (closed list) while semantics say the gateway does not reject unrecognised provider strings, supporting forward-compatibility (open list). Suggest rephrasing the type as String — known values: anthropic, openai, google to remove the contradiction.

4. Self-referential link

The doc header links back to PR #757 itself, which is this PR. Once merged, that link points to the doc-introducing PR rather than the place where the contract is actually exercised. Consider linking to PR-B (#758) where the gateway code reading these headers lands, or dropping the inline PR link in favor of the existing "Related" section at the bottom.

5. httpx-sse added ahead of use

The dependency is added here but no source file imports it yet. Acceptable given the documented merge chain (#757#758#759), but worth noting that if PR-B is reworked or delayed, this leaves an unused dependency on main. Not blocking — just flagging the coupling.

Looks good

  • Forward references to migration 019 / passthrough_routes.py are clearly labeled as PR-B artifacts.
  • Versioning policy (additive-only, no rename, no resemantic) is exactly right for a contract that crosses repo boundaries.
  • Changelog fragment follows the format in changelog.d/README.md correctly.
  • "One PR = One Concern" is well-respected — pure additions, no behavior change, safe to merge independently.

Test coverage

N/A for this PR (docs + dep only). Behavior tests will need to land in PR-B alongside the actual header-reading code — happy to flag specific cases there (missing headers → NULL columns, oversized session_id, unknown provider).


Review by claude-opus-4-7. Constructive nits — none block the merge.

- session_id validation: drop inaccurate truncation claim; gateway stores
  as-is with no enforcement (behavior for malformed values deferred to PR-B)
- trust boundary: narrow strip advice to specific headers listed in this doc;
  add note about TRUST_USER_ID_HEADER interaction to avoid breaking user attribution
- x-luthien-provider: fix closed/open enum contradiction; rephrase type as
  'String — known values: ...' to make open-ended nature explicit
- header link: replace self-referential PR #757 link with PR #758 (gateway impl)
@claude

claude Bot commented May 23, 2026

Copy link
Copy Markdown
Contributor

Review

Scope is appropriately minimal — pure docs + dependency for the upcoming Track A series. Approving in spirit; a few points worth considering before merge.

Code quality / correctness

  • docs/plugin-header-contract.md is well-organized and accurate. Trust boundary section is especially good — the explicit note that operators must not blanket-strip x-luthien-* (because x-luthien-user-id is gated by TRUST_USER_ID_HEADER) prevents a real footgun. Migration references check out: request_logs.session_id is indeed created in 008_add_request_logs_table.sql.
  • Versioning policy is clear and conservative (add = minor, remove/redefine = forbidden without new name). Good.

Potential issues / nits

  • x-luthien-agent has two ways of saying "unknown". Plugin sends literal \"unknown\" when the agent name is unavailable, but an absent header maps to NULL in request_logs.agent. This means logs will mix NULL and the string \"unknown\" for the same condition. Worth either (a) normalizing in the gateway when ingesting (treat literal \"unknown\" as NULL), or (b) documenting that consumers should COALESCE(agent, 'unknown') at query time. Worth nailing down before PR-B locks in the ingestion shape.
  • Validation deferred to PR-B is explicitly called out, which is good — but landing the published contract before the validation lands means any plugin (intentionally or accidentally) sending a 4 KB "session id" will get it stored as-is. Acceptable for a series that will merge in order (#757 → #758 → #759), but if there's any chance feat(track-a): foundation — header contract + httpx-sse #757 merges and feat(track-a): multi-provider passthrough routes + 4 security fixes #758 stalls, the gateway will be running with an underspecified ingest. Consider gating validation as part of the same release.
  • Forward references to artifacts that don't exist yet (src/luthien_proxy/passthrough_routes.py, migrations/postgres/019_add_agent_to_request_logs.sql). These are fine if the series merges as planned, but if either changes name during PR-B review, this doc goes stale silently. Cheap fix: a short "Status: contract published ahead of implementation — see PR feat(track-a): multi-provider passthrough routes + 4 security fixes #758 for current implementation paths" disclaimer at the top, or a follow-up commit to fix names once feat(track-a): multi-provider passthrough routes + 4 security fixes #758 lands.

pyproject.toml / uv.lock

Test coverage

Security

  • Trust boundary section is the security story for this PR and it's handled correctly. No new attack surface introduced by the doc or dep alone.

Performance

  • N/A.

Recommendation

LGTM with a small request to either (a) decide the \"unknown\"-vs-NULL agent normalization story now (before #758 hardcodes it) or (b) explicitly defer it in the doc so reviewers of #758 know to address it. Other points are non-blocking.

- x-luthien-agent: document that gateway stores literal 'unknown' as-is
  (not normalized to NULL); advise consumers to COALESCE(agent, 'unknown')
  at query time to treat both cases uniformly
- add status disclaimer at top: contract published ahead of implementation,
  forward refs to passthrough_routes.py and migration 019 land in PR #758
@claude

claude Bot commented May 23, 2026

Copy link
Copy Markdown
Contributor

Review — PR #757 (foundation: header contract + httpx-sse)

Read the full diff and cross-checked against the current src/ and migrations/ trees. Scope is exactly what the description claims: docs + one dependency + a changelog fragment, no behavior change. Overall this is a clean, low-risk foundation PR. The contract doc is unusually thorough — the trust-boundary discussion and versioning policy are both better than I usually see in plugin contracts.

A few things worth a look before merging, mostly in the doc itself:

1. The "unknown" agent-name convention loses signal

"The plugin sends the literal string \"unknown\" when the agent name is unavailable — the gateway stores this as-is (not normalized to NULL). Consumers should use COALESCE(agent, 'unknown') at query time to treat both cases uniformly."

The two cases are not actually uniform:

  • agent IS NULL → header was absent (plugin not loaded / non-OpenCode client / pre-plugin traffic).
  • agent = 'unknown' → plugin loaded but couldn't resolve the agent name.

Collapsing them with COALESCE discards information that an operator debugging plugin rollout will want. Two safer alternatives:

  • Preferred: have the plugin omit the header when unavailable; let NULL mean "unknown" everywhere.
  • Otherwise, document this distinction explicitly ("use agent IS NULL to detect non-plugin traffic; agent = 'unknown' indicates plugin-injected but unresolved") and drop the COALESCE recommendation.

This is the only thing in the doc I'd consider semi-blocking — the contract is hard to change later per its own versioning policy ("Change the format of an existing header — FORBIDDEN without a new header name"), so it's worth getting right in v1.0.

2. x-luthien-session-id pattern is looser than the prose

The prose says "UUIDv4 format, max 36 chars". The regex [0-9a-f-]{36} accepts any 36-char hex-and-dash string — ------------------------------------ matches. If the contract is "must be UUIDv4", either say so without a regex or use a stricter one (e.g. the canonical UUIDv4 pattern with version/variant nibbles). The "validation deferred to PR-B" caveat already covers gateway-side behavior, so this is just about the contract being precise.

3. httpx-sse>=0.4 has no upper bound

Minor: pinning >=0.4 with no upper bound means a future httpx-sse 1.0 with breaking changes auto-upgrades on fresh installs. Given this dep has no consumer in this PR, the blast radius is currently zero, but worth either capping (>=0.4,<1) or noting that the consumer in #758 will pin appropriately.

4. Adding a dep before its consumer

Adding httpx-sse here means anyone installing between #757 and #758 picks up an unused dependency. Acceptable for a 3-PR split and the merge order is documented (#757#758#759), but worth confirming the train won't stall — if #758 gets held up, this dep sits unused in main indefinitely.

5. Doc/code mismatch window is real but bounded

The doc states "The gateway reads x-luthien-session-id, x-luthien-agent, and x-luthien-model from inbound requests and persists them" — this is not true after this PR alone (verified: no code in src/ reads x-luthien-session-id or x-luthien-agent; migration 019 doesn't exist; passthrough_routes.py doesn't exist). The status banner at the top of the doc does call this out, but the body text is written in present tense. Two options:

The banner is probably enough on its own — flagging in case you want to tighten.

Minor nits

  • docs/plugin-header-contract.md: "max 64 chars, printable ASCII" for x-luthien-agent — no enforcement is described. Same comment as session-id: either tighten or drop the pseudo-spec.
  • The Persisted to rows mix dedicated columns and JSONB. Worth a one-liner upfront clarifying that request_headers JSONB stores the raw inbound header set (so readers don't wonder if there's special routing logic).
  • Doc location: docs/ is correct, matches docs/policies.md and docs/standalone-container.md conventions.

What's fine

  • Changelog fragment format matches changelog.d/README.md exactly (category Chores & Docs, PR number, single-line summary).
  • Trust-boundary callout on TRUST_USER_ID_HEADER is a real catch — a naive "strip all x-luthien-*" reverse-proxy rule would silently break user attribution for operators who've intentionally enabled that setting. Good defensive doc.
  • Versioning policy with explicit add/remove/change rules is the right shape for a plugin contract.
  • No test changes needed — CLAUDE.md's test requirement applies to new functionality, and this PR adds none.

Security

No new attack surface in this PR (no code consumes these headers yet). The trust-boundary section correctly characterizes what the contract will require of operators in #758. Confirm in #758 that the actual header-stripping logic on the outbound path matches the "All x-luthien-* headers are stripped before forwarding" promise — particularly that it strips by prefix match, not by enumerating the documented header names (which would miss the "unknown headers are logged and ignored" case for headers added in future plugin versions).

Recommendation

LGTM with the agent-"unknown" semantics question addressed (item 1) — either change the plugin contract to omit-when-unavailable, or update the consumer guidance to preserve the NULL-vs-"unknown" distinction. Other items are polish.

- x-luthien-agent: change convention to omit-when-unavailable (NULL = unknown
  everywhere); drop 'unknown' placeholder guidance; document that NULL is the
  canonical signal for both 'plugin not loaded' and 'agent name unresolved'
- x-luthien-session-id: tighten regex to proper UUIDv4 pattern with version/
  variant nibbles ([0-9a-f]{8}-...-4...-[89ab]...) instead of loose hex-and-dash
- Gateway Behavior: reword present tense to future tense ('will read', 'will be
  stripped') since code doesn't exist until PR-B; clarify outbound strip is by
  prefix match not enumeration
- Add JSONB note clarifying request_headers stores raw inbound header set
- Drop unenforced pseudo-spec max-length claims on x-luthien-agent
@claude

claude Bot commented May 23, 2026

Copy link
Copy Markdown
Contributor

Review

Small, well-scoped foundation PR. Pure additions (docs + dep) — nothing to break at runtime. Verified the doc's claims against the codebase before reviewing:

  • request_logs.session_id, model, endpoint, request_headers columns all exist (migrations/postgres/008_add_request_logs_table.sql)
  • TRUST_USER_ID_HEADER config exists (src/luthien_proxy/config_fields.py:79)
  • USER_ID_HEADER = "x-luthien-user-id" matches the doc (src/luthien_proxy/pipeline/session.py:23)
  • httpx-sse is not yet imported anywhere — forward-looking add for feat(track-a): multi-provider passthrough routes + 4 security fixes #758, as advertised

What's good

  • Honest forward references. The "Status" line at the top of plugin-header-contract.md explicitly flags passthrough_routes.py and 019_add_agent_to_request_logs.sql as landing in feat(track-a): multi-provider passthrough routes + 4 security fixes #758. Doc-first publishing of a contract is a fine pattern and the disclaimer is in the right place.
  • Trust Boundary section is thoughtful. Enumerating the headers to strip (rather than recommending a blanket x-luthien-* strip) is the right call, and the rationale — preserving TRUST_USER_ID_HEADER semantics — is spelled out. This is the kind of subtle thing operators get wrong.
  • The "omit, don't placeholder" guidance for x-luthien-agent is a good design call and the doc explains why (NULL is the canonical "unavailable" signal). Saves a future bug.
  • Versioning policy is explicit about additive-only and forbids semantic changes to existing headers — a sane contract for a multi-repo boundary.
  • Changelog fragment added per CLAUDE.md
  • One PR = One Concern ✓ — splitting trajectory/track a opencode luthien bridge #614 into three reviewable PRs is the right move.

Suggestions

  1. x-luthien-user-id has no header entry in the contract. The Trust Boundary section references it as a "separate header controlled by TRUST_USER_ID_HEADER" but it doesn't appear in the Headers section, so a reader has to infer its contract by reading the codebase. Either:

    • Add a brief ### x-luthien-user-id entry that cross-links to TRUST_USER_ID_HEADER config and pipeline/session.py, or
    • Add a sentence in "Related" pointing readers to where its contract lives.

    Otherwise the next operator who reads this will assume the listed five headers are the complete set of x-luthien-* headers the gateway understands, and may strip x-luthien-user-id thinking it's undocumented.

  2. Contract version vs. plugin version is ambiguous. The doc says contract version is "independent of plugin npm version" and prescribes bumping to v1.1 on additive change — but the only version header on the wire is x-luthien-plugin-version (npm version). There's no mechanism to surface the contract version. Two options:

    • Add an x-luthien-contract-version header so the gateway can detect contract-version mismatches in logs, or
    • Clarify that contract version is documentation-only and the plugin npm version is the operational signal (then drop the "independent" framing).

    As-is, "contract version v1.1" doesn't appear anywhere queryable.

  3. httpx-sse>=0.4 has no upper bound. Library is on 0.4.x and pre-1.0, so a minor bump could be breaking. Consider >=0.4,<1.0 or >=0.4,<0.5. Low priority — matches the loose pinning convention elsewhere in pyproject.toml, but pre-1.0 deps are the case where pinning earns its keep.

  4. x-luthien-session-id validation deferred to "undefined". Explicitly documented as "behavior for malformed or oversized values is undefined until PR-B adds explicit validation." That's fine as a contract statement, but worth ensuring feat(track-a): multi-provider passthrough routes + 4 security fixes #758 actually adds (a) a length cap and (b) a UUID-format check before persisting to request_logs.session_id — an attacker-controlled header that flows directly into a TEXT column without bounds is the kind of thing that's easy to forget once the contract says "we'll validate later." Suggest opening a checklist item on feat(track-a): multi-provider passthrough routes + 4 security fixes #758 explicitly for this.

  5. Outbound stripping is described as prefix-based ("by prefix match, not by enumerating") but inbound trust-boundary advice is enumeration-based. This asymmetry is correct and intentional (to preserve x-luthien-user-id opt-in), but a one-line note in the doc explaining the asymmetry would help future readers.

Tests / coverage

N/A — pure docs + dep. CLAUDE.md's test requirement applies to "new modules" and "new functions/classes," neither of which this PR introduces.

Verdict

LGTM with the suggestions above, none of which are blockers. The x-luthien-user-id omission (#1) is the one I'd most encourage addressing before merging since the contract doc will be referenced by external plugin authors.

@PaoloC68
PaoloC68 marked this pull request as ready for review May 23, 2026 22:28
PaoloC68 added a commit that referenced this pull request May 25, 2026
jaidhyani pushed a commit that referenced this pull request May 29, 2026
…matter

httpx-sse was added as a direct dependency for passthrough streaming, but
passthrough_routes.py streams with raw httpx (httpx.AsyncClient.stream); the
httpx_sse package is imported nowhere. It is also already available
transitively via mcp, so the direct declaration was redundant as well as
unused. Remove it from pyproject.toml and regenerate uv.lock.

Also give changelog.d/track-a-passthrough.md the YAML frontmatter that
compile_changelog.py requires (category + pr); without it the fragment would
fail at release-time compilation. Drop the now-obsolete "Depends on PR-A"
note since this branch carries the dependency change itself (PR #757 is being
dropped).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@jaidhyani jaidhyani closed this May 29, 2026
@jaidhyani jaidhyani reopened this May 29, 2026
@scottwofford

Copy link
Copy Markdown
Member

Claude-generated merge-queue triage of all open Luthien PRs, requested by Scott (Jul 7, 2026). Advisory only; Scott has not yet acted on these recommendations.

Recommendation: close, together with #758 and #759.

This foundation PR only has a purpose as step 1 of the track-a chain: the header-contract doc describes routes and a migration that exist only in #758, and the httpx-sse dependency is imported by nothing until #758 lands. With multi-provider passthrough now arriving via #796 (current against main and verified against a live deployment), merging this alone would add an unused dependency and documentation for routes not in the codebase. The doc is recoverable from the branch if the plugin track revives.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants