Skip to content

fix(session): disable generic broker API by default#332

Merged
benvinegar merged 1 commit into
mainfrom
fix/disable-hunk-generic-broker-api
May 19, 2026
Merged

fix(session): disable generic broker API by default#332
benvinegar merged 1 commit into
mainfrom
fix/disable-hunk-generic-broker-api

Conversation

@benvinegar
Copy link
Copy Markdown
Member

@benvinegar benvinegar commented May 19, 2026

Summary

  • Change the reusable @hunk/session-broker daemon so the generic /broker and /broker/capabilities HTTP API is disabled by default.
  • Add an explicit exposeHttpApi: true opt-in for package consumers that intentionally want the generic list / get / dispatch HTTP surface.
  • Keep Hunk's app daemon on the default, so it exposes only /health, /session, and Hunk-specific /session-api routes.
  • Update package docs/tests to opt in where the generic broker API is intentionally exercised.

Why

This makes the safer behavior the library default: Hunk no longer has to explicitly block /broker, and future daemon users only expose the raw broker API when they ask for it.

Tests

  • bun test src/session-broker/brokerServer.test.ts packages/session-broker/src/daemon.test.ts packages/session-broker-bun/src/serve.test.ts packages/session-broker-node/src/serve.test.ts
  • bun run format:check
  • bun run typecheck
  • bun run lint
  • bun test ./test/session ./src/session ./src/hunk-session ./src/session-broker
  • tmux manual verification of hunk session list/context/comment plus /broker returning 404

This PR description was generated by Pi using OpenAI GPT-5

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 19, 2026

Greptile Summary

This PR hides the generic broker HTTP surface (/broker and /broker/capabilities) from Hunk's app daemon by returning 404 for those paths before they reach the underlying daemon handler, and replaces the /health payload's generic paths advertisement with a curated object that only exposes the Hunk-specific session endpoints.

  • brokerServer.ts: Intercepts daemon.paths.api and daemon.paths.capabilities early in the request handler, returning 404 before the generic daemon logic runs; the /health handler now destructures away paths from daemon.getHealth() and replaces it with a hand-curated { health, socket } map plus the three sessionApi/sessionCapabilities/sessionSocket URLs.
  • brokerServer.test.ts: Extends the HealthResponse interface with the new optional fields, renames the test to reflect the broader coverage, and adds assertions for the curated health payload and the two new 404 paths.
  • CHANGELOG.md: Documents the change under the unreleased Fixed section.

Confidence Score: 4/5

Safe to merge; the change is a straightforward route interception with no impact on existing Hunk session paths or the underlying session-broker package.

The logic is small and well-tested: the new 404 block sits correctly before any Hunk session path, Host/Origin validation still runs first, and the curated health payload accurately reflects the advertised surface. The only rough edge is that the 404 body is plain text while every other intentional error response in the file returns JSON.

No files require special attention beyond the minor body-format inconsistency in brokerServer.ts.

Important Files Changed

Filename Overview
src/session-broker/brokerServer.ts Intercepts /broker and /broker/capabilities with a 404 before they reach daemon.handleRequest, and strips those paths from the /health payload; the plain-text 404 body is inconsistent with the JSON errors used elsewhere.
src/session-broker/brokerServer.test.ts Updates the HealthResponse interface for the new path fields, renames the test, and adds assertions that the health payload contains only the curated paths and that both generic broker paths return 404.
CHANGELOG.md Adds a one-line entry in the unreleased section documenting the hidden generic broker API surface.

Sequence Diagram

sequenceDiagram
    participant Client
    participant BunServe as Bun serve (serve.ts)
    participant CustomHandler as handleRequest (brokerServer.ts)
    participant Daemon as SessionBrokerDaemon

    Client->>BunServe: Any request

    BunServe->>CustomHandler: validateHost / validateOrigin

    alt /health
        CustomHandler-->>BunServe: "JSON { ...health (no /broker paths), sessionApi, sessionCapabilities, sessionSocket }"
    else /broker or /broker/capabilities (NEW)
        CustomHandler-->>BunServe: 404 Not found
    else /session-api/capabilities
        CustomHandler-->>BunServe: JSON sessionCapabilities()
    else /session-api
        CustomHandler-->>BunServe: handleSessionApiRequest()
    else /mcp
        CustomHandler-->>BunServe: 410 tombstone
    else undefined (fall-through)
        CustomHandler-->>BunServe: undefined
        BunServe->>Daemon: daemon.handleRequest()
        Daemon-->>BunServe: Response or null
        alt /session (WebSocket)
            BunServe-->>Client: 101 Upgrade
        else unmatched
            BunServe-->>Client: 404 Not found
        end
    end

    BunServe-->>Client: Response
Loading
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
src/session-broker/brokerServer.ts:458-460
Prefer `jsonError` here to match the JSON error format used by every other intentional error response in this file (including the MCP tombstone), so callers that parse the body don't get an unexpected JSON decode failure.

```suggestion
      if (url.pathname === daemon.paths.api || url.pathname === daemon.paths.capabilities) {
        return jsonError("Not found.", 404);
      }
```

Reviews (1): Last reviewed commit: "fix(session): hide generic broker API" | Re-trigger Greptile

Comment thread src/session-broker/brokerServer.ts Outdated
Comment on lines +458 to +460
if (url.pathname === daemon.paths.api || url.pathname === daemon.paths.capabilities) {
return new Response("Not found.", { status: 404 });
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Prefer jsonError here to match the JSON error format used by every other intentional error response in this file (including the MCP tombstone), so callers that parse the body don't get an unexpected JSON decode failure.

Suggested change
if (url.pathname === daemon.paths.api || url.pathname === daemon.paths.capabilities) {
return new Response("Not found.", { status: 404 });
}
if (url.pathname === daemon.paths.api || url.pathname === daemon.paths.capabilities) {
return jsonError("Not found.", 404);
}
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/session-broker/brokerServer.ts
Line: 458-460

Comment:
Prefer `jsonError` here to match the JSON error format used by every other intentional error response in this file (including the MCP tombstone), so callers that parse the body don't get an unexpected JSON decode failure.

```suggestion
      if (url.pathname === daemon.paths.api || url.pathname === daemon.paths.capabilities) {
        return jsonError("Not found.", 404);
      }
```

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation has changed since this comment: Hunk no longer has an explicit /broker rejection branch here. The core broker daemon now leaves the generic HTTP API unregistered by default, so /broker falls through as a normal unknown route instead of being an intentional Hunk JSON error. I also updated the PR description to reflect that opt-in core-library approach.

Responded by Pi using OpenAI GPT-5.

@benvinegar benvinegar force-pushed the fix/disable-hunk-generic-broker-api branch from 0b813fc to c943274 Compare May 19, 2026 13:50
@benvinegar benvinegar changed the title fix(session): hide generic broker API fix(session): disable generic broker API by default May 19, 2026
@benvinegar benvinegar force-pushed the fix/disable-hunk-generic-broker-api branch 2 times, most recently from ececb28 to ca03ebc Compare May 19, 2026 14:11
@benvinegar benvinegar force-pushed the fix/disable-hunk-generic-broker-api branch from ca03ebc to 9f8d923 Compare May 19, 2026 14:11
@benvinegar benvinegar merged commit d77a095 into main May 19, 2026
5 checks passed
benvinegar added a commit that referenced this pull request May 19, 2026
#332 made the generic broker HTTP API opt-in (exposeHttpApi). The new
body-size-limit test must enable it to reach the 413 path.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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.

1 participant