Skip to content

Commit efe61dd

Browse files
committed
fix: refuse component reads when no app is connected
`devtools errors` printed "No components with errors or warnings" with nothing attached. The tree is empty because no app is connected, not because nothing matched, so a check that was never performed rendered identically to a check that passed. For an agent reading command output as evidence that is worse than an error: a vacuous pass propagates. Refuse instead. `get tree`, `get component`, `find`, `count` and `errors` now return `{ ok: false, code: 'NO_APP_CONNECTED' }` when no app is attached. The check reads connection health in the same synchronous turn that reads the tree, so nothing can attach or detach in between, and the code is machine-readable so callers need not match on message text. `get tree` used to attach a "disconnected Ns ago" hint to an empty success. That context now qualifies the refusal, where it says something actionable rather than decorating an answer that looked fine. Profiling commands are untouched: they read captured session data, which legitimately outlives the app that produced it. Two auto-restart tests used `get tree` purely to route a command through ensureDaemon; they now assert the refusal alongside the restart they are actually about, since `status` bypasses that path.
1 parent f22798b commit efe61dd

6 files changed

Lines changed: 209 additions & 29 deletions

File tree

.changeset/lucky-hounds-shave.md

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
---
2+
'agent-react-devtools': minor
3+
---
4+
5+
Component reads (`get tree`, `get component`, `find`, `count`, `errors`) now fail with a
6+
structured `NO_APP_CONNECTED` response when no app is attached, instead of answering from an
7+
empty component tree.
8+
9+
Previously `devtools errors` printed `No components with errors or warnings` with nothing
10+
attached, so a check that was never performed was indistinguishable from a check that passed.
11+
The refusal names how long ago the last app disconnected, replacing the empty-tree hint that
12+
`get tree` used to attach to a successful response.

packages/agent-react-devtools/src/daemon.ts

Lines changed: 58 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,13 @@ import path from 'node:path';
44
import { DevToolsBridge } from './devtools-bridge.js';
55
import { ComponentTree } from './component-tree.js';
66
import { Profiler } from './profiler.js';
7-
import type { IpcCommand, IpcResponse, DaemonInfo, StatusInfo } from './types.js';
7+
import type {
8+
IpcCommand,
9+
IpcResponse,
10+
DaemonInfo,
11+
StatusInfo,
12+
ConnectionHealth,
13+
} from './types.js';
814

915
const DEFAULT_STATE_DIR = path.join(
1016
process.env.HOME || process.env.USERPROFILE || '/tmp',
@@ -37,6 +43,17 @@ function enrichWithLabels(
3743
}
3844
}
3945

46+
/**
47+
* How long the last app has been gone, when one was ever attached. A read that
48+
* missed a live app by seconds deserves a different answer from one against a
49+
* daemon nothing has ever connected to.
50+
*/
51+
function describeDisconnect(health: ConnectionHealth): string {
52+
if (!health.hasEverConnected || health.lastDisconnectAt === null) return '';
53+
const seconds = Math.round((Date.now() - health.lastDisconnectAt) / 1000);
54+
return ` (the last app disconnected ${seconds}s ago)`;
55+
}
56+
4057
class Daemon {
4158
private ipcServer: net.Server | null = null;
4259
private bridge: DevToolsBridge;
@@ -139,6 +156,25 @@ class Daemon {
139156
});
140157
}
141158

159+
/**
160+
* A component read answers a question about an attached app's tree. With no
161+
* app attached the tree is empty for a reason the caller cannot see, so an
162+
* empty answer is indistinguishable from "nothing matched" and a check that
163+
* was never performed reads as a check that passed. Refuse instead.
164+
*
165+
* This is read in the same synchronous turn as the tree itself, so nothing
166+
* can attach or detach between the check and the answer.
167+
*/
168+
private componentReadUnavailable(): IpcResponse | null {
169+
const health = this.bridge.getConnectionHealth();
170+
if (health.connectedApps > 0) return null;
171+
return {
172+
ok: false,
173+
code: 'NO_APP_CONNECTED',
174+
error: `No app is connected, so there is no component tree to read${describeDisconnect(health)}. Run \`devtools status\` to check the daemon, and \`devtools wait --connected\` to block until an app attaches.`,
175+
};
176+
}
177+
142178
private async handleCommand(cmd: IpcCommand, conn: net.Socket): Promise<IpcResponse> {
143179
try {
144180
switch (cmd.type) {
@@ -160,6 +196,8 @@ class Daemon {
160196
};
161197

162198
case 'get-tree': {
199+
const unobservable = this.componentReadUnavailable();
200+
if (unobservable) return unobservable;
163201
let resolvedRoot: number | undefined;
164202
if (cmd.root !== undefined) {
165203
resolvedRoot = this.tree.resolveId(cmd.root);
@@ -178,21 +216,15 @@ class Daemon {
178216
if (resolvedRoot !== undefined && treeData.length === 0) {
179217
return { ok: false, error: `Component ${cmd.root} not found` };
180218
}
181-
const response: IpcResponse = {
219+
return {
182220
ok: true,
183221
data: { nodes: treeData, totalCount },
184222
};
185-
if (treeData.length === 0) {
186-
const health = this.bridge.getConnectionHealth();
187-
if (health.hasEverConnected && health.connectedApps === 0 && health.lastDisconnectAt !== null) {
188-
const ago = Math.round((Date.now() - health.lastDisconnectAt) / 1000);
189-
response.hint = `app disconnected ${ago}s ago, waiting for reconnect...`;
190-
}
191-
}
192-
return response;
193223
}
194224

195225
case 'get-component': {
226+
const unobservable = this.componentReadUnavailable();
227+
if (unobservable) return unobservable;
196228
const resolvedId = this.tree.resolveId(cmd.id);
197229
if (resolvedId === undefined) {
198230
return { ok: false, error: `Component ${cmd.id} not found` };
@@ -212,23 +244,30 @@ class Daemon {
212244
}
213245

214246
case 'find':
215-
return {
216-
ok: true,
217-
data: this.tree.findByName(cmd.name, cmd.exact),
218-
};
247+
return (
248+
this.componentReadUnavailable() ?? {
249+
ok: true,
250+
data: this.tree.findByName(cmd.name, cmd.exact),
251+
}
252+
);
219253

220254
case 'count':
221-
return {
222-
ok: true,
223-
data: this.tree.getCountByType(),
224-
};
255+
return (
256+
this.componentReadUnavailable() ?? {
257+
ok: true,
258+
data: this.tree.getCountByType(),
259+
}
260+
);
225261

226-
case 'errors':
262+
case 'errors': {
263+
const unobservable = this.componentReadUnavailable();
264+
if (unobservable) return unobservable;
227265
this.tree.getTree();
228266
return {
229267
ok: true,
230268
data: this.tree.getComponentsWithErrorsOrWarnings(),
231269
};
270+
}
232271

233272
case 'profile-start':
234273
this.profiler.start(cmd.name);

packages/agent-react-devtools/src/types.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -168,6 +168,9 @@ export interface ConnectionEvent {
168168
timestamp: number;
169169
}
170170

171+
/** No app is attached, so a component read has nothing to observe. */
172+
export type IpcErrorCode = 'NO_APP_CONNECTED';
173+
171174
export interface ConnectionHealth {
172175
connectedApps: number;
173176
hasEverConnected: boolean;
@@ -200,6 +203,8 @@ export interface IpcResponse {
200203
ok: boolean;
201204
data?: unknown;
202205
error?: string;
206+
/** Machine-readable reason for a refusal, so callers need not match on `error`. */
207+
code?: IpcErrorCode;
203208
/** The @cN label, passed through when commands use label-based IDs */
204209
label?: string;
205210
/** Contextual hint for empty or stale results */
Lines changed: 121 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,121 @@
1+
import { describe, it, expect, beforeEach, afterEach } from 'vitest';
2+
import path from 'node:path';
3+
import type { ChildProcess } from 'node:child_process';
4+
import { WebSocket } from 'ws';
5+
import {
6+
createTempStateDir,
7+
getTestPort,
8+
startDaemon,
9+
waitForDaemon,
10+
stopDaemon,
11+
sendIpcCommand,
12+
connectMockApp,
13+
sendOperations,
14+
buildOperations,
15+
rootOp,
16+
addOp,
17+
ELEMENT_TYPE_FUNCTION,
18+
sleep,
19+
} from './helpers.js';
20+
21+
// A component read with no app attached answers from an empty tree, which is
22+
// indistinguishable from "nothing matched": a check that was never performed
23+
// reads as a check that passed. Every read must refuse instead.
24+
const COMPONENT_READS = [
25+
{ label: 'get-tree', command: { type: 'get-tree' } },
26+
{ label: 'get-component', command: { type: 'get-component', id: 1 } },
27+
{ label: 'find', command: { type: 'find', name: 'App' } },
28+
{ label: 'count', command: { type: 'count' } },
29+
{ label: 'errors', command: { type: 'errors' } },
30+
] as const;
31+
32+
describe('Component reads require an attached app (e2e)', () => {
33+
let stateDir: string;
34+
let port: number;
35+
let daemon: ChildProcess | null = null;
36+
let socketPath: string;
37+
38+
beforeEach(async () => {
39+
stateDir = createTempStateDir();
40+
port = getTestPort();
41+
daemon = startDaemon(port, stateDir);
42+
await waitForDaemon(stateDir);
43+
socketPath = path.join(stateDir, 'daemon.sock');
44+
});
45+
46+
afterEach(async () => {
47+
await stopDaemon(daemon, stateDir);
48+
daemon = null;
49+
});
50+
51+
for (const { label, command } of COMPONENT_READS) {
52+
it(`should refuse ${label} when no app has ever connected`, async () => {
53+
const resp = await sendIpcCommand(socketPath, command as never);
54+
55+
expect(resp.ok).toBe(false);
56+
expect(resp.code).toBe('NO_APP_CONNECTED');
57+
expect(resp.error).toContain('No app is connected');
58+
// Nothing has ever attached, so there is no disconnect to describe.
59+
expect(resp.error).not.toContain('disconnected');
60+
});
61+
}
62+
63+
it('should refuse a read issued after the app disconnected, not answer from its stale tree', async () => {
64+
const ws = await connectMockApp(port);
65+
sendOperations(
66+
ws,
67+
buildOperations(1, 100, (s) => [
68+
rootOp(100),
69+
addOp(1, ELEMENT_TYPE_FUNCTION, 100, s('App')),
70+
]),
71+
);
72+
await sleep(200);
73+
74+
const attached = await sendIpcCommand(socketPath, { type: 'find', name: 'App' });
75+
expect(attached.ok).toBe(true);
76+
expect(attached.data).toHaveLength(1);
77+
78+
ws.close();
79+
await sleep(300);
80+
81+
const afterDisconnect = await sendIpcCommand(socketPath, { type: 'find', name: 'App' });
82+
expect(afterDisconnect.ok).toBe(false);
83+
expect(afterDisconnect.code).toBe('NO_APP_CONNECTED');
84+
expect(afterDisconnect.error).toContain('disconnected');
85+
86+
const errors = await sendIpcCommand(socketPath, { type: 'errors' });
87+
expect(errors.ok).toBe(false);
88+
expect(errors.code).toBe('NO_APP_CONNECTED');
89+
});
90+
91+
it('should answer component reads while an app is attached', async () => {
92+
const ws = await connectMockApp(port);
93+
sendOperations(
94+
ws,
95+
buildOperations(1, 100, (s) => [
96+
rootOp(100),
97+
addOp(1, ELEMENT_TYPE_FUNCTION, 100, s('App')),
98+
]),
99+
);
100+
await sleep(200);
101+
102+
for (const { command } of COMPONENT_READS) {
103+
// `get-component` resolves a real id here; the others take no argument.
104+
const resp = await sendIpcCommand(socketPath, command as never);
105+
expect(resp.code).toBeUndefined();
106+
}
107+
108+
const errors = await sendIpcCommand(socketPath, { type: 'errors' });
109+
expect(errors.ok).toBe(true);
110+
expect(errors.data).toEqual([]);
111+
112+
ws.close();
113+
});
114+
115+
it('should keep status and wait answerable with nothing attached', async () => {
116+
const status = await sendIpcCommand(socketPath, { type: 'status' });
117+
118+
expect(status.ok).toBe(true);
119+
expect((status.data as { connectedApps: number }).connectedApps).toBe(0);
120+
});
121+
});

packages/e2e-tests/src/connection-health.test.ts

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,7 @@ describe('Connection health (e2e)', () => {
8787
await sleep(300);
8888
});
8989

90-
it('should show hint when tree is empty after disconnect', async () => {
90+
it('should refuse a tree read after disconnect and say how long ago it happened', async () => {
9191
const ws = await connectMockApp(port);
9292
await sleep(300);
9393

@@ -103,14 +103,14 @@ describe('Connection health (e2e)', () => {
103103
ws.close();
104104
await sleep(300);
105105

106-
// get-tree should return hint
106+
// The tree is empty because nothing is attached, not because nothing
107+
// matched: answering `ok` here made a read that observed nothing look
108+
// like a read that found nothing.
107109
const resp = await sendIpcCommand(socketPath, { type: 'get-tree' });
108-
expect(resp.ok).toBe(true);
109-
expect(resp.hint).toBeDefined();
110-
expect(resp.hint).toContain('disconnected');
111-
expect(resp.hint).toContain('waiting for reconnect');
112-
const { nodes } = resp.data as { nodes: Array<unknown> };
113-
expect(nodes).toHaveLength(0);
110+
expect(resp.ok).toBe(false);
111+
expect(resp.code).toBe('NO_APP_CONNECTED');
112+
expect(resp.error).toContain('disconnected');
113+
expect(resp.data).toBeUndefined();
114114
});
115115

116116
it('wait --connected should resolve immediately when already connected', async () => {

packages/e2e-tests/src/daemon-auto-restart.test.ts

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -37,8 +37,11 @@ describe('Daemon auto-restart on rebuild', () => {
3737
infoBefore.buildMtime = 1000;
3838
fs.writeFileSync(infoPath, JSON.stringify(infoBefore, null, 2));
3939

40+
// Any command routed through ensureDaemon exercises the rebuild check.
41+
// `get tree` refuses with no app attached, which is beside the point here:
42+
// reaching the daemon at all is what proves the restart happened.
4043
const result = await runCli(['get', 'tree'], stateDir);
41-
expect(result.exitCode).toBe(0);
44+
expect(result.stderr).toContain('No app is connected');
4245

4346
const infoAfter = JSON.parse(fs.readFileSync(infoPath, 'utf-8'));
4447
expect(infoAfter.pid).not.toBe(infoBefore.pid);
@@ -50,7 +53,7 @@ describe('Daemon auto-restart on rebuild', () => {
5053
const infoBefore = JSON.parse(fs.readFileSync(infoPath, 'utf-8'));
5154

5255
const result = await runCli(['get', 'tree'], stateDir);
53-
expect(result.exitCode).toBe(0);
56+
expect(result.stderr).toContain('No app is connected');
5457

5558
const infoAfter = JSON.parse(fs.readFileSync(infoPath, 'utf-8'));
5659
expect(infoAfter.pid).toBe(infoBefore.pid);

0 commit comments

Comments
 (0)