refactor(session-broker): split broker into reusable runtime-neutral packages

[benvinegar]

Summary

• extract the low-level session broker primitives into @hunk/session-broker-core
• add @hunk/session-broker as the main runtime-neutral broker daemon and connection package
• add thin runtime adapters for Bun and Node in @hunk/session-broker-bun and @hunk/session-broker-node
• migrate Hunk's daemon and client integration to consume the shared packages while keeping Hunk-specific policy and projections in the app layer
• document the intended package boundaries and usage with package READMEs

Package layout

• @hunk/session-broker-core
  • low-level broker types, wire parsing, selectors, state, and terminal metadata
• @hunk/session-broker
  • runtime-neutral broker registry, daemon engine, and session connection lifecycle
• @hunk/session-broker-bun
  • Bun HTTP/websocket adapter
• @hunk/session-broker-node
  • Node HTTP + ws adapter

Hunk integration changes

• update the Hunk daemon server to wrap the shared daemon and Bun adapter
• reuse the shared session connection lifecycle in the Hunk broker client
• keep Hunk-specific config, launch policy, review projections, session commands, and compatibility behavior in the Hunk layer

Verification

• bun run typecheck
• bun run test
• bun run lint
• bun run format:check

This PR description was generated by Pi using GPT-5

[socket-security]

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff	Package	Supply Chain Security	Vulnerability	Quality	Maintenance	License
	@​types/​ws@​8.18.1

View full report

[greptile-apps]

Greptile Summary

This PR extracts the session broker into four layered workspace packages (@hunk/session-broker-core, @hunk/session-broker, @hunk/session-broker-bun, @hunk/session-broker-node) and migrates Hunk's daemon and client to consume them, keeping all Hunk-specific projections and command routing in the app layer. The refactor is structurally sound and well-tested across all four packages, with the package boundaries documented cleanly in READMEs.

Confidence Score: 5/5

Safe to merge; all findings are P2 style and code-quality suggestions with no impact on correctness.

The refactor cleanly separates concerns, preserves Hunk-specific behavior at the app layer, and includes good test coverage across all new packages. The only open findings are a duplicate constant block, an asymmetric double-stop pattern in the Bun adapter vs. the Node adapter, a technically incorrect header type cast, and a misleading engines.node entry—none of which affect runtime behavior.

packages/session-broker-bun/src/serve.ts (double-stop asymmetry), src/session-broker/brokerServer.ts (duplicate default constants)

Important Files Changed

Filename	Overview
packages/session-broker-core/src/brokerState.ts	Renamed from src/session-broker/brokerState.ts; changed Timer type to ReturnType for runtime neutrality—a correct and intentional portability improvement.
packages/session-broker/src/daemon.ts	New runtime-neutral daemon engine; well-structured idle shutdown and stale-session sweep lifecycle. Defines the same timeout defaults as brokerServer.ts (duplication noted separately).
packages/session-broker/src/connection.ts	New runtime-neutral session-side websocket connection helper with reconnect, heartbeat, and command queuing logic. Clean and well-tested.
packages/session-broker-bun/src/serve.ts	Bun HTTP/websocket adapter for the shared daemon. The overridden stop() can trigger originalStop twice when called explicitly; no error guard unlike the Node adapter. Also declares node engine incorrectly.
packages/session-broker-node/src/serve.ts	Node HTTP + ws adapter. Double-stop case is defensively handled via try/catch. Minor issues: IncomingHttpHeaders cast to HeadersInit, and void code/reason suppression pattern.
src/session-broker/brokerServer.ts	Hunk daemon server updated to delegate to shared packages; Hunk-specific routes (health extension, capabilities, legacy MCP 410) properly kept in app layer. Timeout defaults duplicated from packages/session-broker/src/daemon.ts.
src/session-broker/brokerClient.ts	Hunk broker client migrated to use the shared SessionBrokerConnection; daemon-compatibility restart logic and reconnect policy kept in the Hunk layer cleanly.
src/hunk-session/brokerAdapter.ts	Thin adapter wiring Hunk-specific projection functions into the shared SessionBrokerState; correctly encapsulates Hunk-specific parsing and view building.
packages/session-broker/src/broker.ts	Generic SessionBroker wrapping the lower-level SessionBrokerState with a raw-record API; SessionBrokerController interface cleanly bridges the two type systems.

Sequence Diagram

sequenceDiagram
    participant App as Hunk App (brokerClient.ts)
    participant SBC as SessionBrokerConnection<br/>(@hunk/session-broker)
    participant BunAdapt as serveSessionBrokerDaemon<br/>(@hunk/session-broker-bun)
    participant Daemon as SessionBrokerDaemon<br/>(@hunk/session-broker)
    participant Broker as SessionBroker<br/>(@hunk/session-broker)
    participant State as SessionBrokerState<br/>(@hunk/session-broker-core)

    App->>SBC: start() / updateSnapshot()
    SBC-->>BunAdapt: WebSocket connect → register / snapshot / heartbeat
    BunAdapt->>Daemon: handleConnectionMessage(peer, msg)
    Daemon->>Broker: registerSession / updateSnapshot / markSessionSeen
    Broker->>State: (delegates all session storage)

    Note over App,State: HTTP path (agent / CLI)
    App->>BunAdapt: POST /session-api (brokerServer handleRequest)
    BunAdapt->>Daemon: handleRequest(request)
    Daemon->>Broker: listSessions / getSession / dispatchCommand
    Broker->>State: resolves session, enqueues command
    State-->>SBC: WebSocket command message
    SBC-->>App: bridge.dispatchCommand(msg) → result
    App-->>State: command-result via WebSocket
    State-->>Broker: pending command resolved
    Broker-->>Daemon: response
    Daemon-->>BunAdapt: Response.json(result)
    BunAdapt-->>App: HTTP 200

Loading

Comments Outside Diff (5)

1. src/session-broker/brokerServer.ts, line 36-38 (link)

   Duplicate default constants with daemon.ts

   These three constants are identical to the private defaults now declared in packages/session-broker/src/daemon.ts (DEFAULT_STALE_SESSION_TTL_MS, DEFAULT_STALE_SESSION_SWEEP_INTERVAL_MS, DEFAULT_IDLE_TIMEOUT_MS). The CLAUDE.md guideline calls for one source of truth; if the shared package's defaults are ever adjusted, this Hunk-layer copy will silently drift. Consider removing these locals and letting createSessionBrokerDaemon apply its own defaults when the options fields are undefined, or export the constants from @hunk/session-broker and import them here.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: src/session-broker/brokerServer.ts
   Line: 36-38
   
   Comment:
   **Duplicate default constants with `daemon.ts`**
   
   These three constants are identical to the private defaults now declared in `packages/session-broker/src/daemon.ts` (`DEFAULT_STALE_SESSION_TTL_MS`, `DEFAULT_STALE_SESSION_SWEEP_INTERVAL_MS`, `DEFAULT_IDLE_TIMEOUT_MS`). The CLAUDE.md guideline calls for one source of truth; if the shared package's defaults are ever adjusted, this Hunk-layer copy will silently drift. Consider removing these locals and letting `createSessionBrokerDaemon` apply its own defaults when the options fields are `undefined`, or export the constants from `@hunk/session-broker` and import them here.
   
   How can I resolve this? If you propose a fix, please make it concise.

2. packages/session-broker-bun/src/serve.ts, line 591-609 (link)

   originalStop called twice in the explicit-stop path

   When a caller invokes the overridden stop(...):

   1. daemon.shutdown() runs → daemon.stopped resolves.
   2. originalStop(closeActiveConnections) is called.
   3. finish() marks the promise resolved.
   4. The already-scheduled daemon.stopped.then(() => { originalStop(true); finish(); }) microtask fires and calls originalStop(true) a second time.

   The Node adapter (packages/session-broker-node) wraps its equivalent of step 4 in try { await stop() } catch { finish() }, absorbing the double-close error. The Bun adapter has no such guard. Bun's server.stop() appears idempotent in current releases, but the asymmetry is fragile—consider mirroring the Node adapter's defensive pattern here.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: packages/session-broker-bun/src/serve.ts
   Line: 591-609
   
   Comment:
   **`originalStop` called twice in the explicit-stop path**
   
   When a caller invokes the overridden `stop(...)`:
   1. `daemon.shutdown()` runs → `daemon.stopped` resolves.
   2. `originalStop(closeActiveConnections)` is called.
   3. `finish()` marks the promise resolved.
   4. The already-scheduled `daemon.stopped.then(() => { originalStop(true); finish(); })` microtask fires and calls `originalStop(true)` a second time.
   
   The Node adapter (`packages/session-broker-node`) wraps its equivalent of step 4 in `try { await stop() } catch { finish() }`, absorbing the double-close error. The Bun adapter has no such guard. Bun's `server.stop()` appears idempotent in current releases, but the asymmetry is fragile—consider mirroring the Node adapter's defensive pattern here.
   
   How can I resolve this? If you propose a fix, please make it concise.

3. packages/session-broker-node/src/serve.ts, line 1102-1115 (link)

   request.headers as HeadersInit is a technically incorrect cast

   IncomingMessage.headers is typed as IncomingHttpHeaders, where values can be string | string[] | undefined. The Headers constructor (and HeadersInit) only accepts string values per entry, so multi-value headers (string[]) and undefined entries would either be silently dropped or behave unexpectedly. In practice the broker API only sends content-type and similar single-value headers, so this doesn't break anything today—but the cast suppresses a legitimate type mismatch. Consider filtering out undefined/array values explicitly:

   const safeHeaders = Object.fromEntries(
     Object.entries(request.headers).flatMap(([key, val]) =>
       typeof val === "string" ? [[key, val]] : [],
     ),
   );
   return new Request(url, { ..., headers: safeHeaders, ... });

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: packages/session-broker-node/src/serve.ts
   Line: 1102-1115
   
   Comment:
   **`request.headers as HeadersInit` is a technically incorrect cast**
   
   `IncomingMessage.headers` is typed as `IncomingHttpHeaders`, where values can be `string | string[] | undefined`. The `Headers` constructor (and `HeadersInit`) only accepts `string` values per entry, so multi-value headers (`string[]`) and `undefined` entries would either be silently dropped or behave unexpectedly. In practice the broker API only sends `content-type` and similar single-value headers, so this doesn't break anything today—but the cast suppresses a legitimate type mismatch. Consider filtering out `undefined`/array values explicitly:
   
   ```ts
   const safeHeaders = Object.fromEntries(
     Object.entries(request.headers).flatMap(([key, val]) =>
       typeof val === "string" ? [[key, val]] : [],
     ),
   );
   return new Request(url, { ..., headers: safeHeaders, ... });
   ```
   
   How can I resolve this? If you propose a fix, please make it concise.

4. packages/session-broker-node/src/serve.ts, line 1190-1193 (link)

   Unusual void suppression pattern

   void code; void reason; is a valid-but-non-idiomatic way to suppress "declared but never read" TypeScript warnings. The more common pattern is to prefix unused parameters with _ or omit them entirely:

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: packages/session-broker-node/src/serve.ts
   Line: 1190-1193
   
   Comment:
   **Unusual `void` suppression pattern**
   
   `void code; void reason;` is a valid-but-non-idiomatic way to suppress "declared but never read" TypeScript warnings. The more common pattern is to prefix unused parameters with `_` or omit them entirely:
   
   
   
   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!

5. packages/session-broker-bun/src/serve.ts, line 499-501 (link)

   engines.node entry in a Bun-specific package

   packages/session-broker-bun/package.json declares "node": ">=18" in engines, but serveSessionBrokerDaemon calls Bun.serve(...) which is a Bun-only API. The entry could mislead tooling or consumers into thinking this package works under Node. Consider removing the node field or replacing it with a note-only comment in the README.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: packages/session-broker-bun/src/serve.ts
   Line: 499-501
   
   Comment:
   **`engines.node` entry in a Bun-specific package**
   
   `packages/session-broker-bun/package.json` declares `"node": ">=18"` in `engines`, but `serveSessionBrokerDaemon` calls `Bun.serve(...)` which is a Bun-only API. The entry could mislead tooling or consumers into thinking this package works under Node. Consider removing the `node` field or replacing it with a note-only comment in the README.
   
   How can I resolve this? If you propose a fix, please make it concise.

Prompt To Fix All With AI

This is a comment left during a code review.
Path: src/session-broker/brokerServer.ts
Line: 36-38

Comment:
**Duplicate default constants with `daemon.ts`**

These three constants are identical to the private defaults now declared in `packages/session-broker/src/daemon.ts` (`DEFAULT_STALE_SESSION_TTL_MS`, `DEFAULT_STALE_SESSION_SWEEP_INTERVAL_MS`, `DEFAULT_IDLE_TIMEOUT_MS`). The CLAUDE.md guideline calls for one source of truth; if the shared package's defaults are ever adjusted, this Hunk-layer copy will silently drift. Consider removing these locals and letting `createSessionBrokerDaemon` apply its own defaults when the options fields are `undefined`, or export the constants from `@hunk/session-broker` and import them here.

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

---

This is a comment left during a code review.
Path: packages/session-broker-bun/src/serve.ts
Line: 591-609

Comment:
**`originalStop` called twice in the explicit-stop path**

When a caller invokes the overridden `stop(...)`:
1. `daemon.shutdown()` runs → `daemon.stopped` resolves.
2. `originalStop(closeActiveConnections)` is called.
3. `finish()` marks the promise resolved.
4. The already-scheduled `daemon.stopped.then(() => { originalStop(true); finish(); })` microtask fires and calls `originalStop(true)` a second time.

The Node adapter (`packages/session-broker-node`) wraps its equivalent of step 4 in `try { await stop() } catch { finish() }`, absorbing the double-close error. The Bun adapter has no such guard. Bun's `server.stop()` appears idempotent in current releases, but the asymmetry is fragile—consider mirroring the Node adapter's defensive pattern here.

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

---

This is a comment left during a code review.
Path: packages/session-broker-node/src/serve.ts
Line: 1102-1115

Comment:
**`request.headers as HeadersInit` is a technically incorrect cast**

`IncomingMessage.headers` is typed as `IncomingHttpHeaders`, where values can be `string | string[] | undefined`. The `Headers` constructor (and `HeadersInit`) only accepts `string` values per entry, so multi-value headers (`string[]`) and `undefined` entries would either be silently dropped or behave unexpectedly. In practice the broker API only sends `content-type` and similar single-value headers, so this doesn't break anything today—but the cast suppresses a legitimate type mismatch. Consider filtering out `undefined`/array values explicitly:

```ts
const safeHeaders = Object.fromEntries(
  Object.entries(request.headers).flatMap(([key, val]) =>
    typeof val === "string" ? [[key, val]] : [],
  ),
);
return new Request(url, { ..., headers: safeHeaders, ... });
```

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

---

This is a comment left during a code review.
Path: packages/session-broker-node/src/serve.ts
Line: 1190-1193

Comment:
**Unusual `void` suppression pattern**

`void code; void reason;` is a valid-but-non-idiomatic way to suppress "declared but never read" TypeScript warnings. The more common pattern is to prefix unused parameters with `_` or omit them entirely:

```suggestion
  socket.on("close", () => {
    options.daemon.handleConnectionClose(peerBySocket.get(socket) ?? peer);
  });
```

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

---

This is a comment left during a code review.
Path: packages/session-broker-bun/src/serve.ts
Line: 499-501

Comment:
**`engines.node` entry in a Bun-specific package**

`packages/session-broker-bun/package.json` declares `"node": ">=18"` in `engines`, but `serveSessionBrokerDaemon` calls `Bun.serve(...)` which is a Bun-only API. The entry could mislead tooling or consumers into thinking this package works under Node. Consider removing the `node` field or replacing it with a note-only comment in the README.

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

_{Reviews (1): Last reviewed commit: "docs(session-broker): document package r..." | Re-trigger Greptile}