feat(cli): add 'serve --http' remote mode (review-cleaned)#132
Open
thecombatwombat wants to merge 13 commits into
Open
feat(cli): add 'serve --http' remote mode (review-cleaned)#132thecombatwombat wants to merge 13 commits into
thecombatwombat wants to merge 13 commits into
Conversation
When adb returns 'device offline', 'no devices/emulators found', or "device 'X' not found", run `adb wait-for-device` (3s) and retry the original command once. Cuts through brief USB flakes without papering over real failures: a phone reboot still surfaces an error, just one call later. Does not retry on 'device unauthorized' (user revoked the RSA key; retry can't fix that). Lays groundwork for the upcoming `serve --http` mode where a long-lived host process needs to tolerate a flaky cable.
Adds detectTailscaleIp(): tries `tailscale ip -4` first; if the CLI isn't on PATH or returns non-zero, scans os.networkInterfaces() for an external IPv4 in the CGNAT range Tailscale uses (100.64.0.0/10). Pure function with injectable deps for unit testing. Used by the upcoming `serve --http` command to bind to the tailnet interface only.
Wraps sparfenyuk/mcp-proxy via `uvx` to expose the existing stdio server over HTTP/SSE, so an agent on machine A can drive a phone plugged into machine B over Tailscale (or another private network). npx replicant-mcp serve --http # => detects tailscale ip, binds there, prints client config Tradeoffs: - Bundled subcommand vs external recipe: bundled, for one-command UX. - uvx vs requiring mcp-proxy on PATH: uvx, zero install on first run. - Tailscale-first vs bearer tokens: Tailscale provides the auth; --host overrides for other private networks. The stdio mode is unchanged: when invoked with no args replicant-mcp still speaks MCP over stdio. mcp-proxy spawns a fresh stdio replicant as its backend, so the server itself doesn't learn about transports. Tests cover preflight branches, banner content, child-process exit code propagation, and signal-only exits. End-to-end smoke verified manually with --host 127.0.0.1.
docs/remote.md walks a user from zero to a remote-controlled phone: prereqs, the `serve --http` quickstart, client config snippets for Claude Desktop / Cursor / Claude Code, launchd & systemd recipes for keep-it-running, troubleshooting, and known limitations. Adds a Remote Mode bullet under "More Info" and a row in the docs table in README.md, plus a "Run Modes" section in architecture.md showing the stdio / remote process tree.
Records why we wrap mcp-proxy instead of reimplementing transport, why uvx is the install path, why Tailscale is the trust boundary, and what was deliberately deferred for MVP (token auth, multi-client isolation, Docker recipe).
Brings the 2026-05-09 review and follow-up prompt onto the working branch so the triage table can be updated as findings are addressed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…e waits Addresses review finding H1. The transient-error retry helper used a bare `wait-for-device` call, which on a multi-device host returns the moment any device is online — not necessarily the one the original command targeted. The subsequent retry would then immediately fail again on "device 'X' not found", turning the retry into a 3-second sleep on multi-device hosts. Also, when the original command was already `wait-for-device` (called via AdbAdapter.waitForDevice on a missing device), the retry would issue a *second* wait-for-device, doubling the worst-case timeout for no benefit. Fix: extract `-s <deviceId>` from the original args and pass it through to the retry's wait-for-device. Short-circuit when the original command itself is wait-for-device. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses review finding C1. The serve command's startup banner was being written to stdout via `deps.log`, while the project elsewhere goes to great lengths to keep stdout clean of non-MCP bytes (stdout is the MCP wire when this same binary is re-spawned in stdio mode by mcp-proxy). The banner reaching stdout doesn't corrupt the MCP wire today — mcp-proxy pipes the grandchild's stdio rather than inheriting it from the grandparent — but the principle the codebase has held (CLI never writes to stdout when it might be consumed as a transport) was broken. Drop `log` from `ServeDeps`; route the banner through `errLog` (stderr). Adds a regression test that monkey-patches process.stdout to prove the banner never touches stdout. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… clean up listeners on exit
Addresses review findings C2 and M8.
Three problems fixed:
1. The signal-handler loop ran before `child.on('exit'/'error')` was
wired. A synchronous `error` event from a failed spawn could be
lost, leaving the parent hung.
2. The handler used `child.killed` as the de-dup gate. Per Node docs,
`killed` flips to true once we've sent any signal and stays true,
regardless of whether the child died. So if mcp-proxy ignored the
first SIGTERM (it can take several seconds to drain SSE clients),
pressing Ctrl-C again did nothing — no SIGKILL escalation.
3. `process.on(...)` listeners were never removed. Multiple
`runServe` calls in one process leaked listeners.
Refactor:
- Wire `child.on('exit'/'error')` before the signal loop.
- Track installed listeners explicitly; remove them when the child exits
or errors out.
- Use a `shuttingDown` flag, forward the original signal on first hit,
escalate to SIGKILL on second hit (M8 in spirit — observable cleanup).
- Inject `processOn` / `processOff` via `ServeDeps` so tests can
observe listener registration and removal without monkey-patching the
global `process` event emitter (M8 proper).
Tests added:
- handlers installed on injected processOn
- handlers removed after child exit
- second signal escalates SIGTERM → SIGKILL
- child.on(...) is wired before the first process.on(...) call
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…paste claim
Addresses review findings H4 and L3.
Two banner bugs:
- The JSON snippet was emitted at the top level (`{ "replicant-remote":
{ ... } }`), but every real client config (Cursor, Windsurf, Claude
Desktop) wraps server entries under `mcpServers`. A user copy-pasting
the banner verbatim got a config nothing parsed.
- The banner advertised itself as paste-ready for "Claude Desktop /
Cursor / etc.". Claude Desktop cannot consume `{ "url": ... }`
entries — it requires a stdio command (which is exactly why the
remote-mode docs already show the `npx -y mcp-remote` bridge for
Claude Desktop). The banner therefore promised something that was
guaranteed to fail 100% of the time for that audience.
Fix: wrap the snippet under `mcpServers`, retarget the copy at
url-capable clients (Cursor, Windsurf), and point Claude Desktop /
Claude Code users at `docs/remote.md` for the bridge config. Sync the
docs example to the new banner. Add tests parsing the JSON block.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses review finding M1. CLAUDE.md mandates a privacy-policy review when adding network-reachable capability. Remote mode (`serve --http`) opens a new HTTP/SSE listener on the host and `uvx mcp-proxy` makes a first-launch PyPI fetch — both new surfaces the policy didn't cover. Adds: - A "Remote mode" section explaining what binds where, the trust boundary (Tailscale by default), and the lack of bearer-token auth. - A "Third-party fetches" section noting that `uvx` downloads mcp-proxy from PyPI on first launch (cached after) and that `--pass-environment` forwards the host shell env to the spawned backend (same env exposure as running stdio mode locally). Bumps the effective date to today. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…and crash recovery Addresses review open questions OQ1, OQ2 (and finding C3), and OQ3. Adds a new "Process supervision and environment" section to `docs/remote.md` covering: - What `--pass-environment` actually forwards: the entire host shell environment, including unrelated secrets that happen to be exported. No secret leaves the host (the spawned backend is local and trusted), but a tighter blast radius is available by launching from a minimal shell. Also notes the `REPLICANT_LOG_LEVEL` interleaving footgun. - The new shutdown sequence (signal-forward on first signal, escalate to SIGKILL on second) introduced with the C2 fix, plus the orphan- recovery guidance for the case where mcp-proxy fails to forward. - mcp-proxy's behaviour when only the inner backend crashes: the SSE socket stays open and tool calls hang until the supervisor restarts the whole tree. Documents the launchd/systemd reliance. Future smoke-test work for an operator on a Tailscale host: confirm `kill -TERM <proxy-pid>` leaves no orphan node processes, and that `kill -KILL <inner-node-pid>` doesn't survive into hanging RPCs. We have no clean way to exercise these from CI. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Annotates the triage table on the Claude Opus review with which items landed in code, which are documented-only, and which are deferred. Makes it obvious at a glance to a fresh reviewer what remains for L2 and the deferred lower-priority items.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de8bea7f6e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| ### OQ1 — Does mcp-proxy correctly forward SIGTERM to the **grandchild** stdio backend? | ||
|
|
||
| **Confidence:** LOW |
There was a problem hiding this comment.
This newly added file is 527 lines long, which violates the repo-level AGENTS.md code-health rule: “File limit: 500 lines.” Please split or trim this review artifact so the patch stays within the documented maintainability limit.
Useful? React with 👍 / 👎.
Contributor
Greptile SummaryThis PR adds
Confidence Score: 4/5The core serve logic, signal handling, and retry paths are all correct; the two findings are minor input-validation gaps that don't affect normal operation. Signal lifecycle (child-before-process wiring, SIGKILL escalation, listener cleanup) and the device-targeted retry are implemented correctly and thoroughly tested. The two findings are narrow: invalid --port input produces a confusing mcp-proxy error rather than a clear one from replicant-mcp, and the wait-for-device short-circuit only guards the exact string. src/cli/serve.ts (port validation) and src/adapters/adb.ts (wait-for-* short-circuit breadth) Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant ServeCmd as replicant-mcp serve --http
participant Preflight
participant TailscaleDetect
participant uvx as uvx mcp-proxy
participant Backend as replicant-mcp (stdio)
User->>ServeCmd: replicant-mcp serve --http
ServeCmd->>Preflight: checkUvAvailable()
Preflight-->>ServeCmd: ok
ServeCmd->>TailscaleDetect: detectTailscaleIp()
TailscaleDetect-->>ServeCmd: 100.64.1.42
ServeCmd->>ServeCmd: print banner to stderr
ServeCmd->>uvx: spawn(uvx mcp-proxy --port 8765 --host 100.64.1.42 --pass-environment -- node self)
uvx->>Backend: spawn(node dist/index.js) with inherited env
Note over ServeCmd,uvx: signal handlers installed (SIGINT/SIGTERM)
User->>ServeCmd: "Ctrl-C SIGINT #1"
ServeCmd->>uvx: child.kill(SIGINT)
User->>ServeCmd: "Ctrl-C SIGINT #2"
ServeCmd->>uvx: child.kill(SIGKILL)
uvx-->>ServeCmd: exit event
ServeCmd->>ServeCmd: cleanupSignals(), process.exit
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
src/cli/serve.ts:189
No validation is performed on the `--port` value before it is forwarded to mcp-proxy. `parseInt("abc", 10)` returns `NaN`, which becomes the string `"NaN"` in the args array. mcp-proxy will then fail with its own opaque error rather than a clear diagnostic from replicant-mcp. Zero and values above 65535 are also silently passed through.
```suggestion
.option("--port <port>", "Port to listen on", (v) => {
const n = parseInt(v, 10);
if (isNaN(n) || n < 1 || n > 65535) throw new Error(`Invalid port: ${v}`);
return n;
}, DEFAULT_PORT)
```
### Issue 2 of 2
src/adapters/adb.ts:135
The short-circuit that prevents recursive `wait-for-device` retries checks only for the exact string `"wait-for-device"`. Other adb transport variants — `wait-for-usb-device`, `wait-for-local-device`, `wait-for-transport-device` — are not matched, so if a future method uses one of those, it would be eligible for a retry that spawns a new (unmatched) `wait-for-device`, doubling the worst-case timeout unnecessarily. A prefix check is more defensive.
```suggestion
if (args.some((a) => a.startsWith("wait-for-"))) {
```
Reviews (1): Last reviewed commit: "docs(reviews): mark addressed triage ite..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return new Command("serve") | ||
| .description("Run replicant-mcp as a long-lived server (remote mode via mcp-proxy)") | ||
| .option("--http", "Expose over HTTP/SSE via mcp-proxy (required for now)", false) | ||
| .option("--port <port>", "Port to listen on", (v) => parseInt(v, 10), DEFAULT_PORT) |
Contributor
There was a problem hiding this comment.
--port value before it is forwarded to mcp-proxy. parseInt("abc", 10) returns NaN, which becomes the string "NaN" in the args array. mcp-proxy will then fail with its own opaque error rather than a clear diagnostic from replicant-mcp. Zero and values above 65535 are also silently passed through.
Suggested change
| .option("--port <port>", "Port to listen on", (v) => parseInt(v, 10), DEFAULT_PORT) | |
| .option("--port <port>", "Port to listen on", (v) => { | |
| const n = parseInt(v, 10); | |
| if (isNaN(n) || n < 1 || n > 65535) throw new Error(`Invalid port: ${v}`); | |
| return n; | |
| }, DEFAULT_PORT) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/cli/serve.ts
Line: 189
Comment:
No validation is performed on the `--port` value before it is forwarded to mcp-proxy. `parseInt("abc", 10)` returns `NaN`, which becomes the string `"NaN"` in the args array. mcp-proxy will then fail with its own opaque error rather than a clear diagnostic from replicant-mcp. Zero and values above 65535 are also silently passed through.
```suggestion
.option("--port <port>", "Port to listen on", (v) => {
const n = parseInt(v, 10);
if (isNaN(n) || n < 1 || n > 65535) throw new Error(`Invalid port: ${v}`);
return n;
}, DEFAULT_PORT)
```
How can I resolve this? If you propose a fix, please make it concise.| // Don't recursively wait when the caller is already waiting — a failing | ||
| // `wait-for-device` retried with another `wait-for-device` would just | ||
| // double the worst-case timeout for no benefit. | ||
| if (args.includes("wait-for-device")) { |
Contributor
There was a problem hiding this comment.
wait-for-device retries checks only for the exact string "wait-for-device". Other adb transport variants — wait-for-usb-device, wait-for-local-device, wait-for-transport-device — are not matched, so if a future method uses one of those, it would be eligible for a retry that spawns a new (unmatched) wait-for-device, doubling the worst-case timeout unnecessarily. A prefix check is more defensive.
Suggested change
| if (args.includes("wait-for-device")) { | |
| if (args.some((a) => a.startsWith("wait-for-"))) { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/adapters/adb.ts
Line: 135
Comment:
The short-circuit that prevents recursive `wait-for-device` retries checks only for the exact string `"wait-for-device"`. Other adb transport variants — `wait-for-usb-device`, `wait-for-local-device`, `wait-for-transport-device` — are not matched, so if a future method uses one of those, it would be eligible for a retry that spawns a new (unmatched) `wait-for-device`, doubling the worst-case timeout unnecessarily. A prefix check is more defensive.
```suggestion
if (args.some((a) => a.startsWith("wait-for-"))) {
```
How can I resolve this? If you propose a fix, please make it concise.
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.
Summary
Adds a
replicant-mcp serve --httpsubcommand that wrapssparfenyuk/mcp-proxy(viauvx) to expose the stdio MCP server over SSE for remote use, plus a one-shot retry inAdbAdapterfor transient device errors. Includes the fixes from theclaude-opus.mdreview pass.What's in the box
Feature (5 commits — original
feature/remote-modework):feat(adb)— retry once on transient device errors (now wait-for-device-aware, see H1 below)feat(serve)— detect Tailscale interface for default bind addressfeat(cli)—serve --httpsubcommand backed bymcp-proxydocs— remote setup guide (docs/remote.md) + DECISIONS.md entriesReview fixes (8 commits on top —
fix/remote-mode-review):fa6534e) — startup banner now goes to stderr, never stdout (would have corrupted MCP framing if anyone ever ranserve --httpand piped stdout).29a13b1) — childexit/errorlisteners wired before signal handlers; second SIGINT/SIGTERM escalates to SIGKILL;processlisteners cleaned up on child exit.ServeDepsnow exposesprocessOn/processOfffor testability.406a118) — retry helper carries-s <deviceId>intowait-for-deviceand short-circuits when the original command already targetswait-for-*. Stops the retry from racing against any device when multiple are connected, and avoids recursive timeouts.0e81d9f) — banner config snippet wraps in\"mcpServers\": {…}(was previously not paste-ready) and drops the misleading "paste into Claude Desktop" claim.c976ed1) —PRIVACY.mddocuments remote-mode network surface and theuvx → PyPIfirst-launch fetch.5055dbc) —docs/remote.mdadds a Process Supervision and Environment section covering--pass-environmentsemantics, signal forwarding, and crash recovery behaviour.dd61769,de8bea7) — review doc + status table on the branch so reviewers can see what was addressed.Full review and per-item status:
docs/reviews/2026-05-09-feature-remote-mode/claude-opus.md.Verification
npm run build— cleannpx vitest --run— 711 passed, 4 skipped, 0 failedDeferred (pre-merge checklist for human reviewer)
claude mcp add --transport sseflag is still current (claude mcp add --help).serve --http, confirm env passthrough, SIGTERM forwarding, and recovery match whatdocs/remote.mddocuments. Code-reading +mcp-proxy --helpagree, but a real run hasn't been done from this session.Test plan
serve --httpagainst a real device, thenclaude mcp add --transport sse …🤖 Generated with Claude Code