Skip to content

Commit 2b15636

Browse files
sirtimidclaude
andauthored
feat(wallet-cli): add daemon JSON-RPC socket transport (#9108)
## Explanation `@metamask/wallet-cli` runs a background daemon that hosts a `@metamask/wallet` instance; the CLI talks to it over a Unix socket. This PR adds that transport layer. It is purely additive plumbing — no `mm` subcommand consumes it yet (commands land in a follow-up), so there is no user-visible behavior change. - **`socket-line`** — `writeLine`/`readLine` for newline-delimited framing over a `net.Socket`, with an optional read timeout and listener cleanup. - **`daemon-client`** — `sendCommand` opens a connection, writes one JSON-RPC request, reads one response, and closes; it correlates the response `id` with the request and retries once on transient connection errors (`ECONNREFUSED`/`ECONNRESET`). `pingDaemon` is a lightweight `getStatus` health probe that distinguishes "no daemon" (`absent`) from "daemon present but unreachable", classifying the latter by failure mode (`refused` / `timeout` / `permission` / `protocol` / `other`). - **`rpc-socket-server`** — `startRpcSocketServer` listens on a Unix socket and dispatches one JSON-RPC request per connection to a handler map. It intercepts a built-in `shutdown` method, enforces one-request-per-connection, times out idle connections, and returns a `close()` handle. - **`daemon-spawn`** — `ensureDaemon` spawns a detached daemon process and polls until the socket is responsive, refusing to take over a wedged or foreign-owned socket. Resolves the entry point from `dist` (prod) or `src` via `tsx` (dev). - **`stop-daemon`** — `stopDaemon` escalates from a graceful `shutdown` RPC through `SIGTERM` to `SIGKILL`, then removes the PID and socket files best-effort. - **`prompts`** — `confirmPurge` wraps the ESM-only `@inquirer/confirm` via dynamic import. - **`types`** — adds `RpcHandler`, `RpcHandlerMap`, `DaemonStatusInfo`, and `DaemonSpawnConfig`. Adds `@inquirer/confirm` and `@metamask/rpc-errors` dependencies. Every daemon module is covered to the package's 100% coverage thresholds, plus a `socket-integration.test.ts` that exercises the client and server together over a real Unix socket (framing, id correlation, the one-request-per-connection invariant, and real shutdown timing). ### Hardening applied on top of the original branch A multi-agent review of the ported code surfaced a few issues worth fixing before any command consumes this transport. These are the only deviations from the original `rekm/wallet-cli` modules (otherwise lifted verbatim): - **Owner-only socket permissions** (`rpc-socket-server.ts`) — `listen()` created the Unix socket with umask-derived (typically world-accessible) permissions. Since the daemon hosts an unlocked wallet, any local user could connect and drive it. The server now `chmod`s the socket to `0600` immediately after binding. - **Fail fast on a failed spawn** (`daemon-spawn.ts`) — a `ChildProcess` `'error'` (bad interpreter, `EACCES`, `ENOENT`) was logged to stderr but never recorded, so the readiness loop polled for the full 30s and then threw a generic timeout, discarding the real cause. The error is now captured and surfaced immediately. - **No unhandled `'error'` during a write** (`socket-line.ts`) — `writeLine` relied solely on the write callback; a socket `'error'` arriving mid-write had no listener and would crash the process instead of rejecting. It now attaches a one-shot `'error'` listener for the duration of the write. - **Test robustness** (`socket-integration.test.ts`) — replaced an `await import('node:net')` with a static import so the file runs standalone without `--experimental-vm-modules`. ## References - The daemon transport modules originate from Erik Marks's wallet-cli branch (`rekm/wallet-cli`, #8446). They are lifted essentially verbatim from there, split out into this standalone, additive PR and adjusted to apply on the current `main` (see the hardening deltas above). ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've communicated my changes to consumers by [updating changelogs for packages I've changed](https://github.com/MetaMask/core/tree/main/docs/processes/updating-changelogs.md) - [ ] I've introduced [breaking changes](https://github.com/MetaMask/core/tree/main/docs/processes/breaking-changes.md) in this PR and have prepared draft pull requests for clients and consumer packages to resolve them 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Touches local IPC and process lifecycle around an unlocked-wallet daemon, including owner-only socket permissions and signal-based stop; additive only with no CLI wiring yet, but mistakes could affect security or orphan processes once commands land. > > **Overview** > Adds **additive daemon IPC plumbing** to `@metamask/wallet-cli` so the CLI can talk to a background wallet daemon over a Unix socket. Nothing in this PR wires it into `mm` subcommands yet. > > The stack is newline-delimited JSON-RPC: **`socket-line`** for framing, **`daemon-client`** (`sendCommand` with request/response id checks and one retry on transient connect errors; **`pingDaemon`** with `absent` / `responsive` / categorized `unreachable`), and **`startRpcSocketServer`** (one request per connection, handler map, built-in `shutdown`, idle timeouts). **`ensureDaemon`** spawns a detached process and polls readiness while refusing to replace a wedged or other-user socket; **`stopDaemon`** escalates shutdown RPC → SIGTERM → SIGKILL and cleans up PID/socket files. Shared **`utils`** cover PID files and signaling; **`confirmPurge`** dynamically imports `@inquirer/confirm`. **`types.ts`** gains RPC and spawn config types. > > **Security / robustness hardening** in this PR: socket files are **`chmod` 0600** after bind (wallet daemon must not be world-connectable), failed child **`spawn` errors fail fast** instead of a 30s timeout, and **`writeLine`** handles mid-write socket errors. Dependencies **`@metamask/rpc-errors`** and **`@inquirer/confirm`** are added; changelog documents the transport layer. Broad unit tests plus **`socket-integration.test.ts`** exercise real sockets end-to-end. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 556cbd7. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 3cbad62 commit 2b15636

19 files changed

Lines changed: 3427 additions & 27 deletions

packages/wallet-cli/CHANGELOG.md

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
99

1010
### Added
1111

12+
- Add a daemon transport layer: a JSON-RPC client and server over a Unix socket, plus daemon spawn/stop lifecycle helpers ([#9108](https://github.com/MetaMask/core/pull/9108))
1213
- Add SQLite-backed persistence for wallet controller state ([#9067](https://github.com/MetaMask/core/pull/9067))
13-
- A `KeyValueStore` backed by `better-sqlite3` for synchronous reads and writes.
14-
- `loadState` to rehydrate persist-flagged controller state from the store and `subscribeToChanges` to write persist-flagged controller state through to disk on every `stateChanged` event.
1514
- Initial package scaffold for `@metamask/wallet-cli`, an [oclif](https://oclif.io)-based `mm` CLI for `@metamask/wallet` ([#9065](https://github.com/MetaMask/core/pull/9065)).
1615

1716
### Changed

packages/wallet-cli/package.json

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,9 @@
4343
"test:watch": "NODE_OPTIONS=--experimental-vm-modules jest --watch"
4444
},
4545
"dependencies": {
46+
"@inquirer/confirm": "^6.0.11",
4647
"@metamask/base-controller": "^9.1.0",
48+
"@metamask/rpc-errors": "^7.0.2",
4749
"@metamask/utils": "^11.11.0",
4850
"@metamask/wallet": "^4.0.0",
4951
"@oclif/core": "^4.10.5",
Lines changed: 368 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,368 @@
1+
import type { JsonRpcResponse } from '@metamask/utils';
2+
import { EventEmitter } from 'node:events';
3+
import { createConnection } from 'node:net';
4+
import type { Socket } from 'node:net';
5+
6+
import { sendCommand, pingDaemon } from './daemon-client';
7+
import { readLine, writeLine } from './socket-line';
8+
9+
jest.mock('node:net');
10+
jest.mock('./socket-line');
11+
12+
const mockCreateConnection = jest.mocked(createConnection);
13+
const mockReadLine = jest.mocked(readLine);
14+
const mockWriteLine = jest.mocked(writeLine);
15+
16+
/**
17+
* Create a mock Socket and wire up createConnection to return it.
18+
* The connection callback is deferred via process.nextTick to match
19+
* real behavior (the `socket` const must be assigned before the callback
20+
* references it).
21+
*
22+
* @returns The mock socket.
23+
*/
24+
function setupMockSocket(): Socket {
25+
const emitter = new EventEmitter();
26+
const socket = Object.assign(emitter, {
27+
destroy: jest.fn(),
28+
write: jest.fn(),
29+
removeListener: emitter.removeListener.bind(emitter),
30+
}) as unknown as Socket;
31+
32+
mockCreateConnection.mockImplementation(
33+
(_path: unknown, callback: unknown) => {
34+
process.nextTick(() => (callback as () => void)());
35+
return socket;
36+
},
37+
);
38+
39+
return socket;
40+
}
41+
42+
/**
43+
* Build a JSON-RPC response that mirrors back the request id from the most
44+
* recent `mockWriteLine` call. `sendCommand` now verifies id correlation, so
45+
* static fixtures no longer work — the response must echo the generated id.
46+
*
47+
* @param overrides - Optional fields to override on the response.
48+
* @returns A function suitable for `mockReadLine.mockImplementation`.
49+
*/
50+
function respondWithMatchingId(
51+
overrides: Partial<JsonRpcResponse> = {},
52+
): () => Promise<string> {
53+
return async () => {
54+
const lastWrite = mockWriteLine.mock.calls.at(-1)?.[1];
55+
const sentId =
56+
typeof lastWrite === 'string'
57+
? (JSON.parse(lastWrite).id as string)
58+
: 'test-id';
59+
return JSON.stringify({
60+
jsonrpc: '2.0',
61+
id: sentId,
62+
result: { status: 'ok' },
63+
...overrides,
64+
});
65+
};
66+
}
67+
68+
describe('sendCommand', () => {
69+
it('sends a JSON-RPC request and returns the response', async () => {
70+
const socket = setupMockSocket();
71+
mockWriteLine.mockResolvedValue(undefined);
72+
mockReadLine.mockImplementation(respondWithMatchingId());
73+
74+
const response = await sendCommand({
75+
socketPath: '/tmp/test.sock',
76+
method: 'getStatus',
77+
});
78+
79+
expect(mockCreateConnection).toHaveBeenCalledWith(
80+
'/tmp/test.sock',
81+
expect.any(Function),
82+
);
83+
expect(mockWriteLine).toHaveBeenCalledWith(
84+
socket,
85+
expect.stringContaining('"method":"getStatus"'),
86+
);
87+
expect(response.result).toStrictEqual({ status: 'ok' });
88+
expect(socket.destroy).toHaveBeenCalled();
89+
});
90+
91+
it('includes params when provided', async () => {
92+
setupMockSocket();
93+
mockWriteLine.mockResolvedValue(undefined);
94+
mockReadLine.mockImplementation(respondWithMatchingId());
95+
96+
await sendCommand({
97+
socketPath: '/tmp/test.sock',
98+
method: 'test',
99+
params: { key: 'value' },
100+
});
101+
102+
const written = mockWriteLine.mock.calls[0][1];
103+
expect(JSON.parse(written)).toHaveProperty('params', { key: 'value' });
104+
});
105+
106+
it('omits params when undefined', async () => {
107+
setupMockSocket();
108+
mockWriteLine.mockResolvedValue(undefined);
109+
mockReadLine.mockImplementation(respondWithMatchingId());
110+
111+
await sendCommand({
112+
socketPath: '/tmp/test.sock',
113+
method: 'test',
114+
});
115+
116+
const written = mockWriteLine.mock.calls[0][1];
117+
expect(JSON.parse(written)).not.toHaveProperty('params');
118+
});
119+
120+
it('passes timeoutMs to readLine', async () => {
121+
setupMockSocket();
122+
mockWriteLine.mockResolvedValue(undefined);
123+
mockReadLine.mockImplementation(respondWithMatchingId());
124+
125+
await sendCommand({
126+
socketPath: '/tmp/test.sock',
127+
method: 'test',
128+
timeoutMs: 5000,
129+
});
130+
131+
expect(mockReadLine).toHaveBeenCalledWith(expect.anything(), 5000);
132+
});
133+
134+
it('throws when the response id does not match the request id', async () => {
135+
setupMockSocket();
136+
mockWriteLine.mockResolvedValue(undefined);
137+
mockReadLine.mockResolvedValue(
138+
JSON.stringify({
139+
jsonrpc: '2.0',
140+
id: 'unrelated-id',
141+
result: { status: 'ok' },
142+
}),
143+
);
144+
145+
await expect(
146+
sendCommand({ socketPath: '/tmp/test.sock', method: 'test' }),
147+
).rejects.toThrow(/does not match request id/u);
148+
});
149+
150+
it('retries once on ECONNREFUSED', async () => {
151+
const socket = setupMockSocket();
152+
mockWriteLine.mockResolvedValue(undefined);
153+
mockReadLine
154+
.mockRejectedValueOnce(
155+
Object.assign(new Error('refused'), { code: 'ECONNREFUSED' }),
156+
)
157+
.mockImplementationOnce(respondWithMatchingId());
158+
159+
const response = await sendCommand({
160+
socketPath: '/tmp/test.sock',
161+
method: 'test',
162+
});
163+
164+
expect(response.result).toStrictEqual({ status: 'ok' });
165+
expect(socket.destroy).toHaveBeenCalledTimes(2);
166+
});
167+
168+
it('retries once on ECONNRESET', async () => {
169+
setupMockSocket();
170+
mockWriteLine.mockResolvedValue(undefined);
171+
mockReadLine
172+
.mockRejectedValueOnce(
173+
Object.assign(new Error('reset'), { code: 'ECONNRESET' }),
174+
)
175+
.mockImplementationOnce(respondWithMatchingId());
176+
177+
const response = await sendCommand({
178+
socketPath: '/tmp/test.sock',
179+
method: 'test',
180+
});
181+
182+
expect(response).toHaveProperty('result');
183+
});
184+
185+
it('does not retry on other errors', async () => {
186+
setupMockSocket();
187+
mockWriteLine.mockResolvedValue(undefined);
188+
mockReadLine.mockRejectedValue(new Error('parse error'));
189+
190+
await expect(
191+
sendCommand({ socketPath: '/tmp/test.sock', method: 'test' }),
192+
).rejects.toThrow('parse error');
193+
194+
expect(mockReadLine).toHaveBeenCalledTimes(1);
195+
});
196+
197+
it('destroys socket even when attempt throws', async () => {
198+
const socket = setupMockSocket();
199+
mockWriteLine.mockRejectedValue(new Error('write error'));
200+
201+
await expect(
202+
sendCommand({ socketPath: '/tmp/test.sock', method: 'test' }),
203+
).rejects.toThrow('write error');
204+
205+
expect(socket.destroy).toHaveBeenCalled();
206+
});
207+
208+
it('destroys each socket when the connection fails, including the retry', async () => {
209+
const sockets: Socket[] = [];
210+
mockCreateConnection.mockImplementation((_path: unknown) => {
211+
const emitter = new EventEmitter();
212+
const socket = Object.assign(emitter, {
213+
destroy: jest.fn(),
214+
write: jest.fn(),
215+
removeListener: emitter.removeListener.bind(emitter),
216+
}) as unknown as Socket;
217+
sockets.push(socket);
218+
process.nextTick(() =>
219+
socket.emit(
220+
'error',
221+
Object.assign(new Error('refused'), { code: 'ECONNREFUSED' }),
222+
),
223+
);
224+
return socket;
225+
});
226+
227+
await expect(
228+
sendCommand({ socketPath: '/tmp/test.sock', method: 'test' }),
229+
).rejects.toThrow('refused');
230+
231+
expect(sockets).toHaveLength(2);
232+
sockets.forEach((socket) => expect(socket.destroy).toHaveBeenCalled());
233+
});
234+
});
235+
236+
describe('pingDaemon', () => {
237+
/**
238+
* Configure `createConnection` to emit a connection error synchronously.
239+
*
240+
* @param code - The Node errno code (e.g. ENOENT, ECONNREFUSED) the mock
241+
* socket should emit on the next attempt.
242+
*/
243+
function mockConnectionError(code: string): void {
244+
mockCreateConnection.mockImplementation((_path: unknown) => {
245+
const emitter = new EventEmitter();
246+
const socket = Object.assign(emitter, {
247+
destroy: jest.fn(),
248+
write: jest.fn(),
249+
removeListener: emitter.removeListener.bind(emitter),
250+
}) as unknown as Socket;
251+
process.nextTick(() =>
252+
socket.emit('error', Object.assign(new Error(code), { code })),
253+
);
254+
return socket;
255+
});
256+
}
257+
258+
it('returns responsive when daemon responds', async () => {
259+
setupMockSocket();
260+
mockWriteLine.mockResolvedValue(undefined);
261+
mockReadLine.mockImplementation(respondWithMatchingId());
262+
263+
expect(await pingDaemon('/tmp/test.sock')).toStrictEqual({
264+
status: 'responsive',
265+
});
266+
});
267+
268+
it('returns absent when the socket file does not exist', async () => {
269+
mockConnectionError('ENOENT');
270+
271+
expect(await pingDaemon('/tmp/test.sock')).toStrictEqual({
272+
status: 'absent',
273+
});
274+
});
275+
276+
it('returns unreachable with reason=refused when the socket refuses connection', async () => {
277+
// ECONNREFUSED is retried once; both attempts will reject with the same
278+
// mock implementation.
279+
mockConnectionError('ECONNREFUSED');
280+
281+
const result = await pingDaemon('/tmp/test.sock');
282+
expect(result).toStrictEqual({
283+
status: 'unreachable',
284+
reason: 'refused',
285+
error: expect.any(Error),
286+
});
287+
});
288+
289+
it('returns unreachable with reason=permission on EACCES', async () => {
290+
mockConnectionError('EACCES');
291+
292+
const result = await pingDaemon('/tmp/test.sock');
293+
expect(result).toMatchObject({
294+
status: 'unreachable',
295+
reason: 'permission',
296+
});
297+
});
298+
299+
it('returns unreachable with reason=timeout when the socket read times out', async () => {
300+
setupMockSocket();
301+
mockWriteLine.mockResolvedValue(undefined);
302+
mockReadLine.mockRejectedValue(new Error('Socket read timed out'));
303+
304+
const result = await pingDaemon('/tmp/test.sock');
305+
expect(result).toMatchObject({
306+
status: 'unreachable',
307+
reason: 'timeout',
308+
});
309+
});
310+
311+
it('returns unreachable with reason=protocol on a JSON-RPC id mismatch', async () => {
312+
setupMockSocket();
313+
mockWriteLine.mockResolvedValue(undefined);
314+
mockReadLine.mockResolvedValue(
315+
JSON.stringify({
316+
jsonrpc: '2.0',
317+
id: 'unrelated-id',
318+
result: { status: 'ok' },
319+
}),
320+
);
321+
322+
const result = await pingDaemon('/tmp/test.sock');
323+
expect(result).toMatchObject({
324+
status: 'unreachable',
325+
reason: 'protocol',
326+
});
327+
});
328+
329+
it('returns unreachable with reason=protocol on a JSON parse error', async () => {
330+
setupMockSocket();
331+
mockWriteLine.mockResolvedValue(undefined);
332+
mockReadLine.mockResolvedValue('not json');
333+
334+
const result = await pingDaemon('/tmp/test.sock');
335+
expect(result).toMatchObject({
336+
status: 'unreachable',
337+
reason: 'protocol',
338+
});
339+
});
340+
341+
it('returns unreachable with reason=other for unclassified errors', async () => {
342+
setupMockSocket();
343+
mockWriteLine.mockResolvedValue(undefined);
344+
mockReadLine.mockRejectedValue(new Error('something weird'));
345+
346+
const result = await pingDaemon('/tmp/test.sock');
347+
expect(result).toMatchObject({
348+
status: 'unreachable',
349+
reason: 'other',
350+
});
351+
});
352+
353+
it('normalizes non-Error throws into an Error instance', async () => {
354+
setupMockSocket();
355+
mockWriteLine.mockResolvedValue(undefined);
356+
// Simulate a non-Error throw; the producer must normalize it.
357+
mockReadLine.mockImplementation(async () =>
358+
Promise.reject('string-throw' as unknown as Error),
359+
);
360+
361+
const result = await pingDaemon('/tmp/test.sock');
362+
expect(result).toStrictEqual({
363+
status: 'unreachable',
364+
reason: 'other',
365+
error: expect.objectContaining({ message: 'string-throw' }),
366+
});
367+
});
368+
});

0 commit comments

Comments
 (0)