feat(track-a): foundation — header contract + httpx-sse - #757
Conversation
Review: PR #757 — Track A FoundationReviewed 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 Findings below, ranked roughly by severity. Blocking1. Changelog fragment is missing required YAML frontmatter —
---
category: Chores & Docs
pr: 757
---
**Track A foundation**: Plugin header contract docs and `httpx-sse` dependency for upcoming multi-provider passthrough routes.(Categories: High — documentation correctness2. Migration reference points to the wrong number — The doc says 3. Doc asserts a column/migration that doesn't exist yet —
Neither the column nor the migration exists in this PR (confirmed against 4. Broken relative link — The link to 5. Forward reference to non-existent file —
Medium — contract clarity & security6. No trust-boundary statement. The contract is silent on whether 7. No header value validation rules. These values land in indexed 8. 9. Internal contradiction on default vs. NULL. Section 1.2 ( 10. No contract-version header. The plugin sends Low / nits11. Loose dependency pin. 12. Versioning scope ambiguity. The doc opens with What I checked
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
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. Findings1. Inaccurate truncation claim (
2. Trust-boundary advice is incomplete ( The doc tells operators to "strip inbound
3. The type row says 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. 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 Looks good
Test coverageN/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)
ReviewScope 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
Potential issues / nits
|
- 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
Review — PR #757 (foundation: header contract + httpx-sse)Read the full diff and cross-checked against the current A few things worth a look before merging, mostly in the doc itself: 1. The
|
- 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
ReviewSmall, well-scoped foundation PR. Pure additions (docs + dep) — nothing to break at runtime. Verified the doc's claims against the codebase before reviewing:
What's good
Suggestions
Tests / coverageN/A — pure docs + dep. VerdictLGTM with the suggestions above, none of which are blockers. The |
…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>
|
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 |
Part 1 of 3 splitting PR #614.
Changes
docs/plugin-header-contract.md: Documents the header contract for the opencode-luthien pluginpyproject.toml: Addshttpx-sse>=0.4dependency for streaming passthrough routesuv.lock: Updated lock fileNotes
Closes part of #614.