From efe61dd3cb443ab4b9c89127bf8175db442de8ff Mon Sep 17 00:00:00 2001 From: Brad Anderson Date: Thu, 10 Sep 2026 09:15:33 -0400 Subject: [PATCH] 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. --- .changeset/lucky-hounds-shave.md | 12 ++ packages/agent-react-devtools/src/daemon.ts | 77 ++++++++--- packages/agent-react-devtools/src/types.ts | 5 + .../src/component-read-attachment.test.ts | 121 ++++++++++++++++++ .../e2e-tests/src/connection-health.test.ts | 16 +-- .../e2e-tests/src/daemon-auto-restart.test.ts | 7 +- 6 files changed, 209 insertions(+), 29 deletions(-) create mode 100644 .changeset/lucky-hounds-shave.md create mode 100644 packages/e2e-tests/src/component-read-attachment.test.ts diff --git a/.changeset/lucky-hounds-shave.md b/.changeset/lucky-hounds-shave.md new file mode 100644 index 0000000..8842d87 --- /dev/null +++ b/.changeset/lucky-hounds-shave.md @@ -0,0 +1,12 @@ +--- +'agent-react-devtools': minor +--- + +Component reads (`get tree`, `get component`, `find`, `count`, `errors`) now fail with a +structured `NO_APP_CONNECTED` response when no app is attached, instead of answering from an +empty component tree. + +Previously `devtools errors` printed `No components with errors or warnings` with nothing +attached, so a check that was never performed was indistinguishable from a check that passed. +The refusal names how long ago the last app disconnected, replacing the empty-tree hint that +`get tree` used to attach to a successful response. diff --git a/packages/agent-react-devtools/src/daemon.ts b/packages/agent-react-devtools/src/daemon.ts index d0aa12d..98c28a5 100644 --- a/packages/agent-react-devtools/src/daemon.ts +++ b/packages/agent-react-devtools/src/daemon.ts @@ -4,7 +4,13 @@ import path from 'node:path'; import { DevToolsBridge } from './devtools-bridge.js'; import { ComponentTree } from './component-tree.js'; import { Profiler } from './profiler.js'; -import type { IpcCommand, IpcResponse, DaemonInfo, StatusInfo } from './types.js'; +import type { + IpcCommand, + IpcResponse, + DaemonInfo, + StatusInfo, + ConnectionHealth, +} from './types.js'; const DEFAULT_STATE_DIR = path.join( process.env.HOME || process.env.USERPROFILE || '/tmp', @@ -37,6 +43,17 @@ function enrichWithLabels( } } +/** + * How long the last app has been gone, when one was ever attached. A read that + * missed a live app by seconds deserves a different answer from one against a + * daemon nothing has ever connected to. + */ +function describeDisconnect(health: ConnectionHealth): string { + if (!health.hasEverConnected || health.lastDisconnectAt === null) return ''; + const seconds = Math.round((Date.now() - health.lastDisconnectAt) / 1000); + return ` (the last app disconnected ${seconds}s ago)`; +} + class Daemon { private ipcServer: net.Server | null = null; private bridge: DevToolsBridge; @@ -139,6 +156,25 @@ class Daemon { }); } + /** + * A component read answers a question about an attached app's tree. With no + * app attached the tree is empty for a reason the caller cannot see, so an + * empty answer is indistinguishable from "nothing matched" and a check that + * was never performed reads as a check that passed. Refuse instead. + * + * This is read in the same synchronous turn as the tree itself, so nothing + * can attach or detach between the check and the answer. + */ + private componentReadUnavailable(): IpcResponse | null { + const health = this.bridge.getConnectionHealth(); + if (health.connectedApps > 0) return null; + return { + ok: false, + code: 'NO_APP_CONNECTED', + 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.`, + }; + } + private async handleCommand(cmd: IpcCommand, conn: net.Socket): Promise { try { switch (cmd.type) { @@ -160,6 +196,8 @@ class Daemon { }; case 'get-tree': { + const unobservable = this.componentReadUnavailable(); + if (unobservable) return unobservable; let resolvedRoot: number | undefined; if (cmd.root !== undefined) { resolvedRoot = this.tree.resolveId(cmd.root); @@ -178,21 +216,15 @@ class Daemon { if (resolvedRoot !== undefined && treeData.length === 0) { return { ok: false, error: `Component ${cmd.root} not found` }; } - const response: IpcResponse = { + return { ok: true, data: { nodes: treeData, totalCount }, }; - if (treeData.length === 0) { - const health = this.bridge.getConnectionHealth(); - if (health.hasEverConnected && health.connectedApps === 0 && health.lastDisconnectAt !== null) { - const ago = Math.round((Date.now() - health.lastDisconnectAt) / 1000); - response.hint = `app disconnected ${ago}s ago, waiting for reconnect...`; - } - } - return response; } case 'get-component': { + const unobservable = this.componentReadUnavailable(); + if (unobservable) return unobservable; const resolvedId = this.tree.resolveId(cmd.id); if (resolvedId === undefined) { return { ok: false, error: `Component ${cmd.id} not found` }; @@ -212,23 +244,30 @@ class Daemon { } case 'find': - return { - ok: true, - data: this.tree.findByName(cmd.name, cmd.exact), - }; + return ( + this.componentReadUnavailable() ?? { + ok: true, + data: this.tree.findByName(cmd.name, cmd.exact), + } + ); case 'count': - return { - ok: true, - data: this.tree.getCountByType(), - }; + return ( + this.componentReadUnavailable() ?? { + ok: true, + data: this.tree.getCountByType(), + } + ); - case 'errors': + case 'errors': { + const unobservable = this.componentReadUnavailable(); + if (unobservable) return unobservable; this.tree.getTree(); return { ok: true, data: this.tree.getComponentsWithErrorsOrWarnings(), }; + } case 'profile-start': this.profiler.start(cmd.name); diff --git a/packages/agent-react-devtools/src/types.ts b/packages/agent-react-devtools/src/types.ts index 1ca62fa..66530d1 100644 --- a/packages/agent-react-devtools/src/types.ts +++ b/packages/agent-react-devtools/src/types.ts @@ -168,6 +168,9 @@ export interface ConnectionEvent { timestamp: number; } +/** No app is attached, so a component read has nothing to observe. */ +export type IpcErrorCode = 'NO_APP_CONNECTED'; + export interface ConnectionHealth { connectedApps: number; hasEverConnected: boolean; @@ -200,6 +203,8 @@ export interface IpcResponse { ok: boolean; data?: unknown; error?: string; + /** Machine-readable reason for a refusal, so callers need not match on `error`. */ + code?: IpcErrorCode; /** The @cN label, passed through when commands use label-based IDs */ label?: string; /** Contextual hint for empty or stale results */ diff --git a/packages/e2e-tests/src/component-read-attachment.test.ts b/packages/e2e-tests/src/component-read-attachment.test.ts new file mode 100644 index 0000000..8686cf5 --- /dev/null +++ b/packages/e2e-tests/src/component-read-attachment.test.ts @@ -0,0 +1,121 @@ +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import path from 'node:path'; +import type { ChildProcess } from 'node:child_process'; +import { WebSocket } from 'ws'; +import { + createTempStateDir, + getTestPort, + startDaemon, + waitForDaemon, + stopDaemon, + sendIpcCommand, + connectMockApp, + sendOperations, + buildOperations, + rootOp, + addOp, + ELEMENT_TYPE_FUNCTION, + sleep, +} from './helpers.js'; + +// A component read with no app attached answers from an empty tree, which is +// indistinguishable from "nothing matched": a check that was never performed +// reads as a check that passed. Every read must refuse instead. +const COMPONENT_READS = [ + { label: 'get-tree', command: { type: 'get-tree' } }, + { label: 'get-component', command: { type: 'get-component', id: 1 } }, + { label: 'find', command: { type: 'find', name: 'App' } }, + { label: 'count', command: { type: 'count' } }, + { label: 'errors', command: { type: 'errors' } }, +] as const; + +describe('Component reads require an attached app (e2e)', () => { + let stateDir: string; + let port: number; + let daemon: ChildProcess | null = null; + let socketPath: string; + + beforeEach(async () => { + stateDir = createTempStateDir(); + port = getTestPort(); + daemon = startDaemon(port, stateDir); + await waitForDaemon(stateDir); + socketPath = path.join(stateDir, 'daemon.sock'); + }); + + afterEach(async () => { + await stopDaemon(daemon, stateDir); + daemon = null; + }); + + for (const { label, command } of COMPONENT_READS) { + it(`should refuse ${label} when no app has ever connected`, async () => { + const resp = await sendIpcCommand(socketPath, command as never); + + expect(resp.ok).toBe(false); + expect(resp.code).toBe('NO_APP_CONNECTED'); + expect(resp.error).toContain('No app is connected'); + // Nothing has ever attached, so there is no disconnect to describe. + expect(resp.error).not.toContain('disconnected'); + }); + } + + it('should refuse a read issued after the app disconnected, not answer from its stale tree', async () => { + const ws = await connectMockApp(port); + sendOperations( + ws, + buildOperations(1, 100, (s) => [ + rootOp(100), + addOp(1, ELEMENT_TYPE_FUNCTION, 100, s('App')), + ]), + ); + await sleep(200); + + const attached = await sendIpcCommand(socketPath, { type: 'find', name: 'App' }); + expect(attached.ok).toBe(true); + expect(attached.data).toHaveLength(1); + + ws.close(); + await sleep(300); + + const afterDisconnect = await sendIpcCommand(socketPath, { type: 'find', name: 'App' }); + expect(afterDisconnect.ok).toBe(false); + expect(afterDisconnect.code).toBe('NO_APP_CONNECTED'); + expect(afterDisconnect.error).toContain('disconnected'); + + const errors = await sendIpcCommand(socketPath, { type: 'errors' }); + expect(errors.ok).toBe(false); + expect(errors.code).toBe('NO_APP_CONNECTED'); + }); + + it('should answer component reads while an app is attached', async () => { + const ws = await connectMockApp(port); + sendOperations( + ws, + buildOperations(1, 100, (s) => [ + rootOp(100), + addOp(1, ELEMENT_TYPE_FUNCTION, 100, s('App')), + ]), + ); + await sleep(200); + + for (const { command } of COMPONENT_READS) { + // `get-component` resolves a real id here; the others take no argument. + const resp = await sendIpcCommand(socketPath, command as never); + expect(resp.code).toBeUndefined(); + } + + const errors = await sendIpcCommand(socketPath, { type: 'errors' }); + expect(errors.ok).toBe(true); + expect(errors.data).toEqual([]); + + ws.close(); + }); + + it('should keep status and wait answerable with nothing attached', async () => { + const status = await sendIpcCommand(socketPath, { type: 'status' }); + + expect(status.ok).toBe(true); + expect((status.data as { connectedApps: number }).connectedApps).toBe(0); + }); +}); diff --git a/packages/e2e-tests/src/connection-health.test.ts b/packages/e2e-tests/src/connection-health.test.ts index d50dd5c..df1a53e 100644 --- a/packages/e2e-tests/src/connection-health.test.ts +++ b/packages/e2e-tests/src/connection-health.test.ts @@ -87,7 +87,7 @@ describe('Connection health (e2e)', () => { await sleep(300); }); - it('should show hint when tree is empty after disconnect', async () => { + it('should refuse a tree read after disconnect and say how long ago it happened', async () => { const ws = await connectMockApp(port); await sleep(300); @@ -103,14 +103,14 @@ describe('Connection health (e2e)', () => { ws.close(); await sleep(300); - // get-tree should return hint + // The tree is empty because nothing is attached, not because nothing + // matched: answering `ok` here made a read that observed nothing look + // like a read that found nothing. const resp = await sendIpcCommand(socketPath, { type: 'get-tree' }); - expect(resp.ok).toBe(true); - expect(resp.hint).toBeDefined(); - expect(resp.hint).toContain('disconnected'); - expect(resp.hint).toContain('waiting for reconnect'); - const { nodes } = resp.data as { nodes: Array }; - expect(nodes).toHaveLength(0); + expect(resp.ok).toBe(false); + expect(resp.code).toBe('NO_APP_CONNECTED'); + expect(resp.error).toContain('disconnected'); + expect(resp.data).toBeUndefined(); }); it('wait --connected should resolve immediately when already connected', async () => { diff --git a/packages/e2e-tests/src/daemon-auto-restart.test.ts b/packages/e2e-tests/src/daemon-auto-restart.test.ts index 4f62415..5477c3c 100644 --- a/packages/e2e-tests/src/daemon-auto-restart.test.ts +++ b/packages/e2e-tests/src/daemon-auto-restart.test.ts @@ -37,8 +37,11 @@ describe('Daemon auto-restart on rebuild', () => { infoBefore.buildMtime = 1000; fs.writeFileSync(infoPath, JSON.stringify(infoBefore, null, 2)); + // Any command routed through ensureDaemon exercises the rebuild check. + // `get tree` refuses with no app attached, which is beside the point here: + // reaching the daemon at all is what proves the restart happened. const result = await runCli(['get', 'tree'], stateDir); - expect(result.exitCode).toBe(0); + expect(result.stderr).toContain('No app is connected'); const infoAfter = JSON.parse(fs.readFileSync(infoPath, 'utf-8')); expect(infoAfter.pid).not.toBe(infoBefore.pid); @@ -50,7 +53,7 @@ describe('Daemon auto-restart on rebuild', () => { const infoBefore = JSON.parse(fs.readFileSync(infoPath, 'utf-8')); const result = await runCli(['get', 'tree'], stateDir); - expect(result.exitCode).toBe(0); + expect(result.stderr).toContain('No app is connected'); const infoAfter = JSON.parse(fs.readFileSync(infoPath, 'utf-8')); expect(infoAfter.pid).toBe(infoBefore.pid);