feat(serve): add HTTP/SSE transport with bearer-token auth#154
feat(serve): add HTTP/SSE transport with bearer-token auth#154greatjourney589 wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds VEP-0004 and implements an HTTP transport: spec and models, bearer-token auth and actor resolution, Starlette HTTP/SSE server with POST /kb/{method} and GET /kb/events, CLI flags, optional HTTP deps, and tests for auth and HTTP flows. ChangesHTTP Transport Feature Implementation
Sequence DiagramsequenceDiagram
participant Agent as Agent / Client
participant HTTPServer as HTTP Server
participant Auth as auth module
participant ConfigFile as config.yaml
participant KBStore as KBStore
participant AuditLog as audit.log.jsonl
Agent->>HTTPServer: POST /kb/propose_claim\nAuthorization: Bearer token123
HTTPServer->>Auth: require_auth(header, mode, kb_dir)
Auth->>ConfigFile: load server.tokens
ConfigFile-->>Auth: token entries with sha256:hash
Auth->>Auth: compute sha256(token123)
Auth->>Auth: compare with stored hash
Auth-->>HTTPServer: actor = "ci-agent"
HTTPServer->>KBStore: dispatch handler
KBStore->>AuditLog: log proposal_created(actor="ci-agent")
KBStore-->>HTTPServer: proposal_id
HTTPServer-->>Agent: 200 OK {proposal_id}
Agent->>HTTPServer: GET /kb/events?topics=proposals\nAuthorization: Bearer token123
HTTPServer->>Auth: require_auth(...)
Auth-->>HTTPServer: actor (authenticated)
HTTPServer->>AuditLog: tail file, filter by topics
AuditLog-->>HTTPServer: JSON audit records
HTTPServer-->>Agent: text/event-stream\nevent: proposal_created\ndata: {...}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
tests/test_auth.py (1)
103-119: ⚡ Quick winAdd coverage for authenticating a config-only token.
The suite verifies
resolve_actoragainstserver.tokensbut never asserts that such a token actually passesverify_token/require_auth. A test where a token exists only inconfig.yaml(not as the staticVOUCH_SERVER_TOKEN) would surface the auth gap flagged inauth.py(verify_token/resolve_actordisconnect).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_auth.py` around lines 103 - 119, Add a test that covers authenticating a config-only token by writing a token entry via _write_token_config (e.g., token "configtoken" -> actor "config-agent"), ensuring VOUCH_SERVER_TOKEN env is not set, then call the auth path used in production (either verify_token or require_auth helper used by your routes) with that token and assert it authenticates and resolves to "config-agent"; specifically reference resolve_actor, verify_token/require_auth, and _write_token_config so the test verifies the token present only in server.tokens (config.yaml) is accepted by the verification logic.src/vouch/models.py (1)
390-396: 💤 Low valueConsider constraining
authto known modes.
auth: straccepts arbitrary values; a typo inconfig.yaml(e.g.auth: bearrer) loads silently and only surfaces as a behavioral mismatch later. ALiteral["none", "bearer", "token-file"]gives validation at config-load time.♻️ Proposed change
- bind: str = "127.0.0.1:7749" - auth: str = "token-file" + bind: str = "127.0.0.1:7749" + auth: Literal["none", "bearer", "token-file"] = "token-file"(
Literalis already imported at line 13.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vouch/models.py` around lines 390 - 396, The ServerConfig model's auth field currently accepts any string; change its type annotation to a constrained Literal to validate allowed modes at load time (use Literal["none", "bearer", "token-file"]) while keeping the default value "token-file" so invalid values in config.yaml raise a validation error; update the auth declaration in class ServerConfig accordingly (the Literal import is already available).src/vouch/auth.py (1)
84-91: 💤 Low value
modeparameter is unused in_read_static_token.
bearerandtoken-fileresolve identically here (both re-read env/file on each call). The intendedbearer-vs-token-filedistinction (cache vs re-read) is not implemented. Either drop themodeparameter or implement the caching difference.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vouch/auth.py` around lines 84 - 91, The _read_static_token(mode: str) function currently ignores the mode; either remove the unused mode parameter or implement the intended difference between "bearer" (cache env token once) and "token-file" (re-read file each call). To fix, choose one: (A) remove the mode parameter from _read_static_token and update all callers to call it without mode; or (B) implement caching by introducing a module-level variable (e.g. _CACHED_STATIC_TOKEN) and, in _read_static_token, when mode == "bearer" read VOUCH_SERVER_TOKEN once into that cache and return it on subsequent calls, while when mode == "token-file" always read _DEFAULT_TOKEN_FILE.read_text().strip(); ensure callers still pass the same mode strings and handle None results unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/vouch/auth.py`:
- Around line 55-81: verify_token currently only checks a single static token
via _read_static_token and therefore ignores per-token entries in config.yaml;
update verify_token(token: str, mode: str, kb_dir: Path) to first handle mode ==
"none" as now, then load the server config with _load_server_config(kb_dir) and
if cfg and cfg.tokens iterate those entries comparing _sha256_hex(token)
(stripping optional "sha256:" prefix from entry.token_hash) with each stored
hash using _safe_eq and return True on match; if no match in cfg.tokens fall
back to the existing static-token check (_read_static_token) so existing
VOUCH_SERVER_TOKEN / token-file behavior remains; also ensure _read_static_token
respects the mode parameter (or change verify_token to call the correct
static-token reader for different modes) so "bearer" vs "token-file" behave
correctly.
In `@src/vouch/capabilities.py`:
- Around line 73-87: The advertised transport token in capabilities() is
inconsistent with the CLI; update the transports list (the local variable
transports used before the Capabilities(...) return) to advertise the canonical
CLI token "http" instead of "http-jsonl" so clients can use the same token as
the vouch serve CLI; leave other transports (e.g., "mcp-http") unchanged and
ensure the Capabilities(...) call returns the updated transports list.
In `@src/vouch/cli.py`:
- Around line 796-811: The mcp-http branch is not forwarding the parsed bind
host/port and is not handling auth modes consistently: update the code around
the mcp.run call so mcp.run(transport="streamable-http") receives the host and
port parsed from bind (pass host and port args extracted from bind into
mcp.run), and unify auth handling by calling assert_loopback_for_no_auth(bind)
when auth_mode == "none" but otherwise either wire the chosen auth
(bearer/token-file) into the streamable-http server startup or explicitly raise
a ClickException for unsupported auth modes; adjust the logic in the try block
that imports .auth.assert_loopback_for_no_auth and calls mcp.run to forward
host/port and enforce/reject non-none auth modes accordingly.
In `@src/vouch/http_server.py`:
- Around line 111-112: The current exception handler returns
traceback.format_exc() to clients (in the except block that calls
_error_response), which leaks internal stack traces; change this to log the full
traceback server-side using a module logger (create or use a logger instance in
src/vouch/http_server.py), call logger.exception or logger.error with
traceback.format_exc() and the exception, and then call _error_response with a
generic error message like "internal_error" and a non-sensitive client-facing
message (no stack trace). Ensure the unique symbols referenced are the except
block surrounding the call to _error_response and the _error_response function
itself so you replace the response payload while preserving server-side logging.
- Around line 103-117: The dispatch() function currently mutates
os.environ["VOUCH_AGENT"] and calls HANDLERS[full_method](params) synchronously
which blocks the loop and risks clobbering agent attribution; instead remove any
global VOUCH_AGENT mutation and pass the actor explicitly into handlers (e.g.,
add an actor parameter to the jsonl_server handler functions named _h_* and the
helper _agent() should be changed to read the actor argument rather than
os.environ), and run handler execution off the event loop if they are CPU-bound
by using asyncio.get_running_loop().run_in_executor(...) or by making handlers
async and awaiting them; preserve the existing exception mapping (KeyError ->
missing_param, ValueError/ProposalError/ArtifactNotFoundError ->
invalid_request, others -> internal_error) and drop the finally block that
restores VOUCH_AGENT.
- Around line 190-197: The SSE endpoint currently returns
Response(content=generate()) where generate() is an async generator—replace
Response with starlette.responses.StreamingResponse and pass generate() as the
content so the async generator is streamed properly; change internal_error to
return a non-sensitive generic error message instead of traceback.format_exc()
to avoid leaking internals; update dispatch to avoid calling HANDLERS[...]
synchronously by either awaiting the handler if it is async or offloading sync
handlers with asyncio.to_thread/anyio.to_thread, and stop reading request-scoped
state from global os.environ["VOUCH_AGENT"]—accept the agent value from the
request context/parameters (or request.state) and pass it into dispatch/handlers
instead.
---
Nitpick comments:
In `@src/vouch/auth.py`:
- Around line 84-91: The _read_static_token(mode: str) function currently
ignores the mode; either remove the unused mode parameter or implement the
intended difference between "bearer" (cache env token once) and "token-file"
(re-read file each call). To fix, choose one: (A) remove the mode parameter from
_read_static_token and update all callers to call it without mode; or (B)
implement caching by introducing a module-level variable (e.g.
_CACHED_STATIC_TOKEN) and, in _read_static_token, when mode == "bearer" read
VOUCH_SERVER_TOKEN once into that cache and return it on subsequent calls, while
when mode == "token-file" always read _DEFAULT_TOKEN_FILE.read_text().strip();
ensure callers still pass the same mode strings and handle None results
unchanged.
In `@src/vouch/models.py`:
- Around line 390-396: The ServerConfig model's auth field currently accepts any
string; change its type annotation to a constrained Literal to validate allowed
modes at load time (use Literal["none", "bearer", "token-file"]) while keeping
the default value "token-file" so invalid values in config.yaml raise a
validation error; update the auth declaration in class ServerConfig accordingly
(the Literal import is already available).
In `@tests/test_auth.py`:
- Around line 103-119: Add a test that covers authenticating a config-only token
by writing a token entry via _write_token_config (e.g., token "configtoken" ->
actor "config-agent"), ensuring VOUCH_SERVER_TOKEN env is not set, then call the
auth path used in production (either verify_token or require_auth helper used by
your routes) with that token and assert it authenticates and resolves to
"config-agent"; specifically reference resolve_actor, verify_token/require_auth,
and _write_token_config so the test verifies the token present only in
server.tokens (config.yaml) is accepted by the verification logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9e4ac460-cfe8-4141-bbb4-e5a8f93cc230
📒 Files selected for processing (10)
proposals/README.mdproposals/VEP-0004-http-transport.mdpyproject.tomlsrc/vouch/auth.pysrc/vouch/capabilities.pysrc/vouch/cli.pysrc/vouch/http_server.pysrc/vouch/models.pytests/test_auth.pytests/test_http_server.py
…ak, and mcp-http bind
|
Hi,@plind-junior |
ReviewSummary: This PR adds a Starlette/uvicorn HTTP transport ( What works
Suggestions
VerdictRequest changes — the concurrent |
|
Hi,@plind-junior |
2 participants
What changed
Closes #153
Adds an HTTP transport to
vouch serve(--transport http) backed by a newStarlette/uvicorn server (
src/vouch/http_server.py) and a bearer-token authlayer (
src/vouch/auth.py). The fullkb.*method surface is exposed overPOST /kb/<method>by reusing the existingHANDLERSdict fromjsonl_server.py. Audit events are streamed to subscribers viaGET /kb/events(Server-Sent Events, topic-filtered). A companion--transport mcp-httpflag runs FastMCP over its streamable-HTTP transport.kb.capabilitiesnow advertises available transports and auth modes.New
ServerConfigandTokenEntryPydantic models back the optionalserver:stanza in.vouch/config.yaml. The[http]optional-dependencygroup (
starlette,uvicorn[standard]) is added topyproject.toml.Why
The two existing transports (MCP stdio, JSONL stdio) require the agent and KB
server to share a process or pipe. This breaks cross-machine review workflows
(CI proposes on Machine A; human approves on Machine B), prevents running a
persistent shared sidecar with a warm FTS5 index, and is unavailable on
subprocess-hostile runtimes (Lambda, Cloud Run, Replit). Closes the feature
request: feat: HTTP transport with bearer-token auth and SSE streaming for
vouch serve.What might break
Nothing for existing users. Specifically:
vouch servewith no flags still starts MCP stdio — no change.stdioandjsonltransports are untouched..vouch/config.yamlgains an optionalserver:key; old configs withoutit load fine (defaults apply).
kb.capabilitiesgains two new fields (transports,auth_modes);old adapters that don't read them are unaffected.
kb.*method behaviour changes.VEP
VEP-0004: HTTP transport with bearer-token auth and SSE streaming — filed as
draftin this PR; implementation follows the design in the VEP.Tests
make checkpasses locally (lint + mypy + pytest)tests/test_auth.py— 17 tests: all auth modes, timing-safe compare, actor resolution, loopback enforcementtests/test_http_server.py— 11 tests: auth rejection (401), route dispatch, error mapping (404/422), propose→approve flow, per-token actor attributionCHANGELOG.mdupdated under## [Unreleased]Summary by CodeRabbit
New Features
Documentation
Tests
Chores