Add local HTTP/OpenAPI server and JS SDK#623
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a local HTTP/OpenAPI server with session, file, event, and approval endpoints, CLI HTTP-mode startup and agent execution, plus a typed JavaScript SDK with SSE support. ChangesHTTP Server and CLI Integration
JavaScript SDK Package
TUI Test Fixture
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 5
🧹 Nitpick comments (3)
sdk/js/index.d.ts (1)
65-113: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider defining response types instead of
Promise<unknown>.Nearly every method returns
Promise<unknown>, which provides no type safety for SDK consumers. For a v1 this is acceptable, but defining at least the key response shapes (e.g.,Session,RunResultis already defined,HealthStatus,FileMeta) would significantly improve the SDK's value.🤖 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 `@sdk/js/index.d.ts` around lines 65 - 113, The ZeroClient declaration is using Promise<unknown> for most API methods, which leaves SDK consumers without usable type safety. Update the method signatures in ZeroClient to return concrete response types where possible, starting with the key shapes already represented in the file such as RunResult and adding named response interfaces for endpoints like global.health, file.get/content/status, session.get/list/create/update, and the various list/get methods. Keep the interface definitions organized near ZeroClient so the return types are easy to find and reuse.internal/cli/serve_http.go (1)
57-57: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSet server timeouts to prevent resource exhaustion.
The
http.Serveris constructed withoutReadTimeout,ReadHeaderTimeout,WriteTimeout, orIdleTimeout. Even on loopback, a misbehaving client or stuck connection can hold resources indefinitely. Set at leastReadHeaderTimeout(e.g., 10s) andIdleTimeoutto bound connection lifetime.⏱️ Proposed fix: add server timeouts
- server := &http.Server{Handler: api} + server := &http.Server{ + Handler: api, + ReadHeaderTimeout: 10 * time.Second, + IdleTimeout: 120 * time.Second, + }🤖 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 `@internal/cli/serve_http.go` at line 57, The http.Server setup in serve_http.go is missing connection timeouts, which can leave resources tied up by slow or stuck clients. Update the server initialization around the http.Server literal to set appropriate timeout fields, at minimum ReadHeaderTimeout and IdleTimeout, and consider also adding ReadTimeout and WriteTimeout if they fit the API behavior. Keep the change localized to the server construction used by the serve_http path so the API handler continues to work with bounded connection lifetime.Source: Linters/SAST tools
sdk/js/test/client.test.mjs (1)
41-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a chunk-boundary SSE test.
The SDK implements its own streaming parser; add one test where a
data:JSON line is split across chunks, with a heartbeat/comment interleaved, to prevent regressions in real network streaming.Test coverage idea
+test('event.subscribe handles SSE chunks split across line boundaries', async () => { + const encoder = new TextEncoder() + const stream = new ReadableStream({ + start(controller) { + controller.enqueue(encoder.encode(': ping\n\n')) + controller.enqueue(encoder.encode('event: text\n')) + controller.enqueue(encoder.encode('data: {"type":"text",')) + controller.enqueue(encoder.encode('"delta":"hi"}\n\n')) + controller.close() + }, + }) + const client = createZeroClient({ + baseUrl: 'http://zero.local', + fetch: async () => new Response(stream, { headers: { 'content-type': 'text/event-stream' } }), + }) + + const events = [] + for await (const event of client.event.subscribe()) { + events.push(event) + } + + assert.deepEqual(events, [{ type: 'text', delta: 'hi' }]) +})🤖 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 `@sdk/js/test/client.test.mjs` around lines 41 - 66, Add a new SSE coverage test in client.test.mjs alongside event.subscribe to exercise the custom streaming parser with real chunk boundaries. Build a ReadableStream for createZeroClient whose chunks split a single data: JSON line across boundaries and include an interleaved heartbeat/comment event, then verify client.event.subscribe({ sessionId: 's1' }) still yields the parsed text event correctly. Use the existing event.subscribe test structure and identifiers (createZeroClient, client.event.subscribe) to place the new regression case.
🤖 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 `@internal/httpapi/broker.go`:
- Around line 47-50: The non-blocking send in the event dispatch path is
dropping control events, which can cause permission/ask requests to disappear
before the client sees them. Update the broker’s event publishing logic in the
affected send path so permission/ask events are never silently lost: either make
these control events durable/replayable or switch slow subscribers to
backpressure/disconnect behavior instead of using the default drop branch.
Reference the event fanout code around the channel send in the broker
implementation so the fix applies to the correct control flow.
In `@internal/httpapi/files.go`:
- Around line 116-137: The file handling in the WalkDir callback keeps
descriptors open too long because `defer file.Close()` is used for every opened
file. Update the `os.Open`/scanner logic in `internal/httpapi/files.go` to close
the file explicitly right after scanning completes (including the early-break
path), so each iteration releases the descriptor before the next `WalkDir`
callback continues.
In `@internal/httpapi/middleware.go`:
- Around line 63-69: The LoopbackHost helper in middleware.go is treating an
empty hostname as loopback, which allows --hostname= to bypass the auth gate.
Update LoopbackHost so only explicit loopback values such as "localhost" or
loopback IPs return true, and ensure an empty trimmed host returns false. Keep
the behavior centered on the LoopbackHost function since it is used by the
--no-auth check.
In `@internal/httpapi/server.go`:
- Around line 320-324: The async path in server.executeRun is launching a
goroutine without any panic protection, so a panic there can escape net/http
recovery and skip terminal run events. Wrap the go server.executeRun(...) call
in a recover guard inside the HTTP handler flow, and on panic publish a
failure/terminal event before the goroutine exits; use server.executeRun and the
surrounding async handler branch to locate the fix.
In `@sdk/js/index.d.ts`:
- Line 115: The type declaration for createZeroClient currently makes options
required even though the implementation defaults it to an empty object. Update
the createZeroClient signature in the declaration so callers can omit the
argument, keeping it aligned with the runtime behavior; check the
createZeroClient export and ZeroClientOptions type usage in the declaration
file.
---
Nitpick comments:
In `@internal/cli/serve_http.go`:
- Line 57: The http.Server setup in serve_http.go is missing connection
timeouts, which can leave resources tied up by slow or stuck clients. Update the
server initialization around the http.Server literal to set appropriate timeout
fields, at minimum ReadHeaderTimeout and IdleTimeout, and consider also adding
ReadTimeout and WriteTimeout if they fit the API behavior. Keep the change
localized to the server construction used by the serve_http path so the API
handler continues to work with bounded connection lifetime.
In `@sdk/js/index.d.ts`:
- Around line 65-113: The ZeroClient declaration is using Promise<unknown> for
most API methods, which leaves SDK consumers without usable type safety. Update
the method signatures in ZeroClient to return concrete response types where
possible, starting with the key shapes already represented in the file such as
RunResult and adding named response interfaces for endpoints like global.health,
file.get/content/status, session.get/list/create/update, and the various
list/get methods. Keep the interface definitions organized near ZeroClient so
the return types are easy to find and reuse.
In `@sdk/js/test/client.test.mjs`:
- Around line 41-66: Add a new SSE coverage test in client.test.mjs alongside
event.subscribe to exercise the custom streaming parser with real chunk
boundaries. Build a ReadableStream for createZeroClient whose chunks split a
single data: JSON line across boundaries and include an interleaved
heartbeat/comment event, then verify client.event.subscribe({ sessionId: 's1' })
still yields the parsed text event correctly. Use the existing event.subscribe
test structure and identifiers (createZeroClient, client.event.subscribe) to
place the new regression case.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: abd347f8-9e28-40d8-adc0-8cdaaf69b860
📒 Files selected for processing (19)
internal/cli/http_runner.gointernal/cli/serve.gointernal/cli/serve_http.gointernal/cli/serve_test.gointernal/httpapi/broker.gointernal/httpapi/errors.gointernal/httpapi/files.gointernal/httpapi/middleware.gointernal/httpapi/openapi.gointernal/httpapi/server.gointernal/httpapi/server_test.gointernal/httpapi/sessions.gointernal/httpapi/types.gointernal/tui/scroll_test.gosdk/js/README.mdsdk/js/index.d.tssdk/js/index.jssdk/js/package.jsonsdk/js/test/client.test.mjs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/httpapi/broker.go (1)
44-72: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDon’t hold
broker.muacross the control-event send. A stalled subscriber can blockpublishforcontrolEventSendTimeout, freezingsubscribe,unsubscribe, and other publishes behind the same mutex. Snapshot the targets or hand off delivery outside the global lock so one slow consumer can’t stall the broker.🤖 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 `@internal/httpapi/broker.go` around lines 44 - 72, Move the blocking control-event delivery in eventBroker.publish out from under broker.mu so a slow subscriber cannot stall the broker. In publish, snapshot the target subscribers under the lock, release the mutex before sending isBlockingControlEvent events with the timeout, and only reacquire the lock to remove/close timed-out channels if needed. Keep the existing sessionID filtering and non-blocking send behavior intact while ensuring subscribe, unsubscribe, and other publishes are not blocked by one slow consumer.
🧹 Nitpick comments (1)
sdk/js/index.d.ts (1)
149-149: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
FileInfo.typeunion collapses tostring.
'file' | 'directory' | stringsimplifies to juststringbecausestringis a supertype of the literals, so the named variants provide no type-narrowing benefit. If arbitrary backend values are possible, consider'file' | 'directory' | (string & {})to preserve autocomplete hints while allowing other strings, or drop| stringif only those two values are expected.♻️ Suggested type refinement
export interface FileInfo { path: string - type: 'file' | 'directory' | string + type: 'file' | 'directory' | (string & {}) size: number modTime: string children?: FileInfo[] }🤖 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 `@sdk/js/index.d.ts` at line 149, The FileInfo.type union in the type definition currently collapses to plain string, so the literal variants provide no narrowing or autocomplete benefit. Update the FileInfo type declaration to either keep only the expected literals if those are the only valid values, or use a string-preserving pattern such as a branded string alongside the literals; make the change in the FileInfo/type definition where type is declared.
🤖 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.
Outside diff comments:
In `@internal/httpapi/broker.go`:
- Around line 44-72: Move the blocking control-event delivery in
eventBroker.publish out from under broker.mu so a slow subscriber cannot stall
the broker. In publish, snapshot the target subscribers under the lock, release
the mutex before sending isBlockingControlEvent events with the timeout, and
only reacquire the lock to remove/close timed-out channels if needed. Keep the
existing sessionID filtering and non-blocking send behavior intact while
ensuring subscribe, unsubscribe, and other publishes are not blocked by one slow
consumer.
---
Nitpick comments:
In `@sdk/js/index.d.ts`:
- Line 149: The FileInfo.type union in the type definition currently collapses
to plain string, so the literal variants provide no narrowing or autocomplete
benefit. Update the FileInfo type declaration to either keep only the expected
literals if those are the only valid values, or use a string-preserving pattern
such as a branded string alongside the literals; make the change in the
FileInfo/type definition where type is declared.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 38185ad8-d3ea-461e-939a-a3823babedc4
📒 Files selected for processing (8)
internal/cli/serve_http.gointernal/httpapi/broker.gointernal/httpapi/files.gointernal/httpapi/middleware.gointernal/httpapi/server.gointernal/httpapi/server_test.gosdk/js/index.d.tssdk/js/test/client.test.mjs
🚧 Files skipped from review as they are similar to previous changes (4)
- sdk/js/test/client.test.mjs
- internal/httpapi/middleware.go
- internal/httpapi/files.go
- internal/cli/serve_http.go
kevincodex1
requested review from
Vasanthdev2004,
anandh8x,
gnanam1990 and
jatmn
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Complete CodeRabbit's control-event delivery request for late SSE subscribers
internal/httpapi/broker.go:55
The follow-up only waits for a control event when a subscriber is already present.prompt_asyncstarts the run before its caller can open/event; if the run immediately requests permission or asks the user,publishsnapshots no targets and discards the opaque request ID. The run then waits in the pending broker with no endpoint that can recover that ID, so it cannot be approved. Please retain/replay pending permission and ask events to a later matching subscription (or establish the subscription before accepting the async run). This completes the resolved CodeRabbit request that control events must not be silently lost. -
[P1] Reject permission decisions that were not offered for the request
internal/httpapi/server.go:497
This endpoint accepts every non-empty action, while the broker discardsPermissionRequest.AvailableDecisions. The agent loop normalizes values but does not enforce that set, so an API client can submitalways_alloworalways_allow_prefixfor a prompt that offered only a one-time decision; those actions can persist a grant. Store the allowed decisions with the pending request and return a 4xx for actions outside that list. -
[P2] Keep the SDK constructor declaration consistent with its runtime requirement
sdk/js/index.d.ts:232
The new optional parameter lets TypeScript callers writecreateZeroClient(), but the implementation immediately rejects that invocation becausebaseUrlis empty. Keepoptionsrequired (as it was before the follow-up) or supply an actual runtime default base URL.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
sdk/js/index.d.ts (1)
232-232: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
createZeroClientoptions regression — still required in type declaration.This was previously fixed (commit c83fec7) to make
optionsoptional, but the current diff reverts it to required. The runtime implementation still defaults tooptions = {}, socreateZeroClient()is a valid call that TypeScript will reject.Fix type declaration
-export function createZeroClient(options: ZeroClientOptions): ZeroClient +export function createZeroClient(options?: ZeroClientOptions): ZeroClient🤖 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 `@sdk/js/index.d.ts` at line 232, Update the createZeroClient declaration to make its options parameter optional, matching the runtime implementation’s default options behavior and allowing createZeroClient() without arguments.
🧹 Nitpick comments (1)
internal/httpapi/broker.go (1)
108-110: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
ask_user_request/ask_user_answerconstants and use them here.
internal/streamjson/streamjson.goalready centralizesEventPermissionRequest; these ask-user event types should be defined once too, then reused ininternal/httpapi/broker.goand the matching answer path to avoid wire-contract typos.🤖 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 `@internal/httpapi/broker.go` around lines 108 - 110, Define exported constants for the ask-user request and answer event types in internal/streamjson/streamjson.go alongside EventPermissionRequest, then update isBlockingControlEvent in broker.go and the corresponding answer-handling path to reference those constants instead of inline string values or streamjson.EventType conversions.
🤖 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.
Duplicate comments:
In `@sdk/js/index.d.ts`:
- Line 232: Update the createZeroClient declaration to make its options
parameter optional, matching the runtime implementation’s default options
behavior and allowing createZeroClient() without arguments.
---
Nitpick comments:
In `@internal/httpapi/broker.go`:
- Around line 108-110: Define exported constants for the ask-user request and
answer event types in internal/streamjson/streamjson.go alongside
EventPermissionRequest, then update isBlockingControlEvent in broker.go and the
corresponding answer-handling path to reference those constants instead of
inline string values or streamjson.EventType conversions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 74f9e75c-a8db-4aae-b1ce-4d54bc41343c
📒 Files selected for processing (4)
internal/httpapi/broker.gointernal/httpapi/server.gointernal/httpapi/server_test.gosdk/js/index.d.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/httpapi/server.go
|
Thanks for the thorough work on this the HTTP server and SDK are a solid v1 design, and the security defaults show real care. However, it adds a full local HTTP server + JS SDK which is basically plumbing for a web UI. We don’t think we need that right now. We don’t have a web product planned anytime soon, and we'd rather keep fixing bugs and polishing what Zero already does (CLI, TUI, MCP, sandbox) instead of taking on a whole new surface we have to maintain forever. Also, CONTRIBUTING.md requires community PRs to target an issue with the |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Reviewed this against current main — build, vet, gofmt, and the httpapi/cli/tui tests all pass, including the scroll-test fixture this PR pins (that was the path-length-sensitive test I'd been skipping on Windows; pinning cwd fixes it cleanly, not a mask). The security model is solid: auth fails closed (empty token → 500, bad bearer → 401), --no-auth is rejected off loopback, withAuth wraps every route including SSE, file paths are confined to the workspace with symlink-escape checks, request bodies are capped and content-type-checked, and errors are redacted. The part I cared most about — an HTTP run can't bypass the permission gate — checks out: a tool needing permission publishes a request over SSE and blocks until a human POSTs a decision, and the decision action is allowlisted against what the agent offered, so there's no auto-approve or forged-allow path.
Holding for two things, both kevin's. Issue #622 is still open with no maintainer sign-off — like #653, this is a large feature landing against the author's own open issue, so I'd want your go-ahead on scope before it merges. And the branch is off the 0.3.0 release commit, seven commits behind main; it merges clean, but a rebase onto current main would let it pick up the workspace-trust gating from #529 and the Go 1.26.5 bump in tests. Minor nit: the bearer-token check in withAuth uses a plain string compare rather than constant-time — low-stakes for a local server, but crypto/subtle.ConstantTimeCompare would be the careful choice.
|
@gnanam1990 when you have time — #623 is the other big one from this contributor: a local HTTP/OpenAPI server + JS SDK (+3691, 19 files) with bearer auth, loopback-only --no-auth, CORS, SSE, sessions, workspace-confined file/find endpoints, and a permission/ask-user broker that blocks HTTP runs on permission requests. I reviewed it inline and the security model looks right (fail-closed auth, symlink-aware path confinement, no auto-approve path), but I'd value your independent read on the permission broker and SSE control-event flow. I'm holding it for an owner call on issue #622 and a rebase, not on the code. |
Closes #622
Summary
zero serve --httpwith local bearer auth, loopback-only no-auth, CORS, OpenAPI, SSE, session, run, approval, abort, file, and search endpoints.internal/httpapiwith stdlibnet/http, run manager, permission broker, ask-user broker, JSON error shape, and path confinement.agent.Runso permission and ask-user callbacks can block and resume over HTTP.sdk/jsas@gitlawb/zero-sdk.zero serve --mcpbehavior unchanged.Security and scope
--no-authis explicitly used on loopback.Notes
go test ./...passes from this long worktree path.Summary by CodeRabbit
zero serve --httpmode with HTTP/OpenAPI docs, auth (env token or auto-generated), CORS, session management, and streamed run execution/events.