Conversation
Lets `thunderbolt acp serve` be deployed as a shared team agent whose skills and MCP servers are configured once by its operator, rather than by each person's client. Everything is additive and opt-in: with no agent config and no new env vars, the CLI behaves exactly as before. - agent-config.ts: `agent.json` at THUNDERBOLT_AGENT_CONFIG or under the state root. A missing file is the empty config; a present-but-invalid one throws at startup, and one malformed server rejects the whole document — an agent quietly missing half its tools looks healthy while failing the team. - mcp.ts: stdio and streamable-HTTP clients, tools namespaced `<serverId>_<tool>` to match the app's mergeMcpTools. Goes at the MCP SDK directly rather than reusing shared/agent-core/mcp-tools.ts, which would pull `ai` and `@ai-sdk/mcp` into a single compiled binary. One unreachable server is reported and skipped, not fatal. - Harness wiring via HarnessConfig.mcpTools, connected once per process in serve.ts before stdio is claimed. - Agent-owned skills win on a name collision: a hosted agent exists so its operator decides what it can do. - Bridge deployment: THUNDERBOLT_BRIDGE_HOST (default unchanged) and THUNDERBOLT_BRIDGE_TOKEN, so a restart doesn't invalidate every configured client. The advertised URL reflects the actual bind. - Per-server `trustTools` (default false) waives the permission prompt for unknown MCP tool names without waiving the mode — read-only still blocks a trusted tool. MCP's own read-only hints are not honoured: a server should not opt itself out of the gate.
Semgrep Security ScanNo security issues found. |
|
Preview environment deployed 🚀
Stack: Auto-destroys on PR close/merge. Login via the bundled Keycloak realm — |
PR Metrics
Updated Fri, 18 Sep 2026 19:35:46 GMT · run #3049 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔭 thunder-deep-review (advisory)
Complements the other bots — surfaces only what they did not flag. Never approves, never requests changes, never gates merge.
head: d69ac3ce94a8 · mode: deep · deferred 0 item(s) already reported by other bots (best-effort dedup)
The comment claimed a spawned stdio MCP server inherits nothing but the
env its config names. It does not. `StdioClientTransport` builds the
child environment as `{ ...getDefaultEnvironment(), ...params.env }`, so
a platform safelist (HOME, PATH, SHELL, TERM, USER on POSIX) is always
present and the passed env narrows rather than isolates.
The behaviour is correct and worth keeping — the safelist is deliberate
and carries nothing sensitive, and the part that matters still holds:
the agent's own environment holds the Thunderbolt credential and
whatever else the host injects, and none of that is on the safelist.
Only the comment was wrong, and a comment that overstates an isolation
guarantee is worse than none.
Verified by spawning @modelcontextprotocol/server-everything and calling
its get-env tool: THUNDERBOLT_AUTH_TOKEN was absent, TERM and SHELL were
present.
There was a problem hiding this comment.
🔭 thunder-deep-review (advisory)
Complements the other bots — surfaces only what they did not flag. Never approves, never requests changes, never gates merge.
head: a6ff35b839af · mode: deep · deferred 0 item(s) already reported by other bots (best-effort dedup)
Review feedback on the deployed-agent skills/MCP work. Each item is a case where a bad config previously produced an agent that reported itself healthy: - Reject cross-transport keys (`headers` on stdio, `command` on http) instead of dropping them, so auth headers an operator wrote are never silently ignored. A misspelled `trustTool` now fails loudly rather than reading as granted trust. - Tighten the loopback check: match the parsed hostname against 127.0.0.0/8 rather than a `127.` prefix, which would have handed a bearer token to `127.0.0.1.evil.com` over plain http. - Close a connected MCP client when `listTools` fails, which otherwise orphans the client and its stdio child for the agent's lifetime. - Cover mcp.ts with tests over an in-process transport pair, talking the real protocol so the content mapping is exercised as the SDK delivers it. - Add `Dockerfile.dockerignore` so the repo-root build context stops overlaying the host's macOS `node_modules` onto the linux image. - Correct the doc's read-only claim: `acp serve` has no permission mode, so `trustTools` is the only gate there is.
There was a problem hiding this comment.
🔭 thunder-deep-review (advisory)
Complements the other bots — surfaces only what they did not flag. Never approves, never requests changes, never gates merge.
head: 35fa8c012998 · mode: deep · deferred 0 item(s) already reported by other bots (best-effort dedup)
The Dockerfile ships THUNDERBOLT_BRIDGE_HOST=0.0.0.0, so a bare `docker run` published a process that spawns agents behind the per-run generated token — a secret that changes on every restart and is only ever written to this process's stdout, plausibly nowhere the platform keeps. Either nobody can reach the agent or every configured client breaks on redeploy. - resolveBridgeHost now throws when THUNDERBOLT_BRIDGE_HOST is set without THUNDERBOLT_BRIDGE_TOKEN, so the failure lands at startup rather than after the socket is already serving - runBridge resolves the host once before opening the socket - THUNDERBOLT_APP_ORIGIN stays optional: omitting it announces itself with a 403 naming the origin, and the built-in Tauri origins are correct for the LAN case
There was a problem hiding this comment.
🔭 thunder-deep-review (advisory)
Complements the other bots — surfaces only what they did not flag. Never approves, never requests changes, never gates merge.
head: 7d9fe554e054 · mode: deep · deferred 0 item(s) already reported by other bots (best-effort dedup)
- the try/catch existed only to assign a `let`, which hides that the ENOENT branch is the sole reason the read is guarded at all; a helper returning null makes "absent" a value rather than control flow - cover the case the guard is narrow for: a non-ENOENT read (a directory at the config path) is a broken deployment and must still throw, not degrade to an agent with no skills
There was a problem hiding this comment.
🔭 thunder-deep-review (advisory)
Complements the other bots — surfaces only what they did not flag. Never approves, never requests changes, never gates merge.
head: fa56a2449583 · mode: deep · deferred 0 item(s) already reported by other bots (best-effort dedup)
The previous guard keyed on THUNDERBOLT_BRIDGE_HOST being set at all, so spelling out the default (`127.0.0.1`, `::1`, `localhost`) refused to start until an operator supplied a token that changes nothing — a confusing no-op for a bind that cannot leave the machine. It now keys on the address, and names the offending one in the error so the message is actionable. - extract the loopback test the MCP URL check already owned into `lib/loopback.ts`, shared by both callers; it accepts `::1` bracketed and bare because a URL.hostname brackets IPv6 and a hand-written bind address does not - make the documented `agent.json` example strict JSON: the loader is `JSON.parse`, so the jsonc block's trailing commas handed whoever copied it a startup error. A test parses the block out of the doc and through `parseAgentConfig` to keep it that way
There was a problem hiding this comment.
🔭 thunder-deep-review (advisory)
Complements the other bots — surfaces only what they did not flag. Never approves, never requests changes, never gates merge.
head: 505ae546296a · mode: deep · deferred 0 item(s) already reported by other bots (best-effort dedup)
- The printed agent URL is meant to be pasted into the app verbatim, but an unbracketed IPv6 literal is not a URL at all — `ws://::1:8839` parses with an empty host — so an operator who pinned IPv6 loopback was handed something unusable. - New exported `hostForUrl` brackets a colon-bearing host, leaves names and IPv4 alone, and never double-brackets an already-bracketed value.
- `attachPermissionGate`'s `trustedToolNames` option was unreachable. `createMcpRuntime` is called only from `acp serve`, and that path uses `attachAcpPermissionGate` instead — the two local callers (`ui/tui.ts`, `agent/run.ts`) never pass it, and a local run has no MCP tools at all. Removed the option, and added a note on the function saying why per-server trust belongs on the ACP gate rather than this one. - Removed the three tests that exercised it, including the "read-only still blocks a trusted tool" case. That invariant was asserted against a situation that cannot arise: the local gate never sees an MCP tool, and the ACP gate has no permission modes (only `yolo`). A test implying coverage it does not provide is worse than no test. - Added the coverage where the code runs. `harness-agent.test.ts` previously had zero assertions about `trustedToolNames` despite owning the only live path. It now threads an `McpRuntime` through the `connectPair` helper and covers both directions: a tool from a `trustTools` server runs with no `requestPermission` reaching the client, and an unrecognised MCP tool name still prompts — because `isReadOnlyAgentTool` knows only the built-in tools, so an unknown name must fall through to the prompt rather than to allowed. - Both new tests were mutation-tested. Removing the trust check fails the first and only the first. Replacing it with an auto-allow for unrecognised names fails the second, alongside three pre-existing gate tests. The file was restored and re-verified clean after each. - Verified: typecheck clean, 938 CLI tests pass, prettier clean. The single remaining failure is `token-store.test.ts` "cross-process compare-and-set", which fails identically in isolation on this machine in a file this commit does not touch, and passes in CI.
A hosted agent behind a public bridge was unreachable from the web app, which is the entire point of deploying one. Verified against a live bridge before changing anything: a connection with no `Origin` header is refused (`closed 1002 Expected 101`), one with an allowlisted `Origin` opens. The chain breaks in the middle. A browser cannot dial a remote ACP agent directly — `src/acp/transports/index.ts` routes it through the app's universal WebSocket proxy unless running standalone under Tauri — and that proxy dials upstream with `new WebSocket(url, protocols)` (`backend/src/proxy/ws.ts`), sending no `Origin` at all. So the bridge's origin allowlist, which a browser satisfies, is something the proxy can never satisfy. The fix: `authorizeUpgrade` takes a `publicBind` flag, passed from `!isLoopbackHost(host)` at the one production call site. On a public bind the origin check is skipped; the token check, the path check, and everything else are untouched. Why that is not a loosening worth mourning: the allowlist defends a *loopback* bridge — the socket is on the user's own machine, a WebSocket upgrade bypasses CORS, so any page they visit could otherwise drive their agent. That is a real control, and it is unchanged; the default bind still demands an allowlisted origin, with a test pinning it. A public bind has a different threat model: connections arrive from servers rather than browsers, and anything that is a server can set whatever `Origin` it likes. The token is the real gate there, which is why a public bind already refuses to start without a stable one. Blast radius, checked before changing it: - `authorizeUpgrade` and `bridgeAllowedOrigins` are referenced only in `cli/src/commands/bridge.ts` and its test — nothing in `src/` or `backend/` imports them, and the iroh transport pulls only `spawnAgent`/`atProcCapacity`/`redactArgv` and is untouched. - The desktop app is unaffected: it dials a loopback bridge directly with an allowlisted `tauri://` origin. - All 29 pre-existing bridge tests pass unmodified, which was the design goal — the loopback path is byte-identical. - The `thunderbolt mcp --transport wss` bridge shares this code and so also relaxes when publicly bound; same reasoning, and the docs now say so. Tests — five new cases: a proxy-shaped no-Origin connection is accepted on a public bind, an off-allowlist Origin is accepted, a missing or wrong token is still rejected, a non-`/` path is still rejected, and loopback still demands an allowlisted Origin. Mutation-tested both directions: forcing the check on always fails the three new public-bind cases; removing it always fails the two pre-existing loopback cases plus the new guard. Over-relaxing is the dangerous direction and it is pinned from both sides. Docs corrected — the previous version was wrong. `hosted-agent.md` said to paste the printed URL into "the app", which is true for the desktop app and false for a browser, and it listed `THUNDERBOLT_APP_ORIGIN` as required, which a public bind no longer needs. It now carries a reachability table and states the constraint that matters: the backend proxy independently requires `wss:` and rejects private addresses, so a browser can only ever reach a public TLS bridge — a local one is desktop-only, and this change does nothing for it. It also records that Test Connection passes regardless, because `src/acp/connection-test.ts` deliberately dials direct and skips the proxy, so a green check there does not mean chat will work.
There was a problem hiding this comment.
🔭 thunder-deep-review (advisory)
Complements the other bots — surfaces only what they did not flag. Never approves, never requests changes, never gates merge.
head: 612ee769998f · mode: deep · deferred 0 item(s) already reported by other bots (best-effort dedup)
| * | ||
| * A bind **beyond** loopback therefore requires an explicit | ||
| * `THUNDERBOLT_BRIDGE_TOKEN` and refuses to start without one, mirroring the | ||
| * short-token throw above. The generated fallback is right on loopback and |
There was a problem hiding this comment.
📐 Convention — The resolveBridgeHost comment says the origin allowlist still guards a public bind, but authorizeUpgrade drops it
Heads up — I think this doc comment and the actual behavior have drifted apart, and it's exactly the kind of thing an operator would lean on when reasoning about the security posture. Here it says APP_ORIGIN is safe to omit because "every browser upgrade is refused with a 403 naming the origin," and the opening paragraph says "the origin allowlist and token become the only thing between the internet and a process." But in the same change, runBridge calls authorizeUpgrade(req, token, allowedOrigins, !isLoopbackHost(host)), and authorizeUpgrade skips the entire origin block (if (!publicBind) { ... }) whenever the bind is non-loopback. So on the very configuration this comment is documenting — a non-loopback bind — the allowlist is never consulted and no 403 is ever returned; the token is the only gate. Could we reword this so it reflects that the origin allowlist is inert on a public bind and the token alone is the control? Otherwise it reads as reassurance about a check that isn't running.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Lets
thunderbolt acp servebe deployed as a shared team agent whose skills and MCP servers are configured once by its operator, instead of by each person's client. Everything is additive and opt-in: with no agent config and no new env vars, the CLI behaves exactly as before.Why
A 5–6 person team wanting the same MCP servers and skills currently has to configure each client individually. Two things blocked hosting one agent instead: skills could only arrive from the client (
readWireSkills(params._meta)was the only producer anywhere), and there was no MCP client in the served agent at all —createHarnessToolsbuilt exactly the coding tools plus webfetch plus skills.Pieces
agent-config.ts—agent.jsonatTHUNDERBOLT_AGENT_CONFIGor under the state root, declaring skills and MCP servers. A missing file is the empty config; a present-but-invalid one throws at startup rather than silently downgrading, and one malformed server rejects the whole document — an agent quietly missing half its tools looks healthy while failing the team.mcp.ts— stdio and streamable-HTTP MCP clients, tools namespaced<serverId>_<tool>to match the app'smergeMcpTools. Deliberately does not reuseshared/agent-core/mcp-tools.ts: that converts AI-SDK tools, and reaching it means pullingaiand@ai-sdk/mcpinto a single compiled binary. Going at the MCP SDK directly costs one conversion function and is the simpler path anyway, since MCP already hands over JSON Schema and content blocks shaped like what Pi wants. One unreachable server is reported and skipped rather than taking the agent down.HarnessConfig.mcpTools, connected once per process inserve.tsbefore stdio is claimed (clients outlive sessions, and connecting is async while tool assembly is not).THUNDERBOLT_BRIDGE_HOST(default unchanged at127.0.0.1) andTHUNDERBOLT_BRIDGE_TOKEN(a stable operator secret; without it the per-run token changes on restart and breaks every configured client, and a short one is rejected at startup). The advertised URL now reflects the actual bind instead of always claiming127.0.0.1.cli/Dockerfileandcli/docs/hosted-agent.md.The design decision reviewers should weigh
MCP tool names are unknown to
isReadOnlyAgentTool, so by default every MCP call raises a permission prompt. On a shared agent that prompt lands on whichever teammate happens to have a session open.The chosen answer is a per-server
trustToolsflag, defaultingfalse, so an operator grants trust deliberately and per server rather than one global switch letting one person's session authorise a write for everyone. Trust waives the prompt, not the mode —read-onlystill blocks a trusted tool, and a test covers that after an earlier version of this patch got the ordering wrong. MCP's own read-only hints are deliberately not honoured: that is a server describing itself, so honouring it would let a server opt itself out of the gate.Happy to change the shape if reviewers prefer something else.
Two consequences worth saying out loud
Documented, not solved here:
Testing
cd cli && bun run typecheck— cleancd cli && bun test src --timeout 5000— 894 tests pass across 59 files, including 19 newagent-configcases and 3 new permission-policy cases