Skip to content

Commit 1258452

Browse files
szdziedzicclaude
andcommitted
fix: close daemon policy startup race and tighten review follow-ups
- Check the policy digest in ensureDaemon after both the reuse and the start branch, so a caller naming a policy cannot accept a daemon that another caller started without it. - Restrict servesPublicCommand to the internal catalog group in the type. - Test that an allow list naming runtime admits it, and assert denial details instead of message text. - Read the fixture code signature through resolveDaemonLaunchSpec. - Correct the protocol-plumbing examples (human_control, not takeover) and describe the category as internal commands with platform execution. Co-Authored-By: Claude Opus 5.5 <[email protected]>
1 parent 3f58844 commit 1258452

7 files changed

Lines changed: 125 additions & 64 deletions

File tree

‎docs/adr/0029-daemon-policy.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,10 +44,10 @@ Accepted (2026-09-29).
4444
- `commands` — exactly one of `allow` or `deny`. Which daemon commands a rule decides, and by what
4545
name, is derived from the command registry, never from a hand list:
4646
- public commands, by their own name;
47-
- internal commands that execute on a device, by the public command their
47+
- internal commands with platform execution, by the public command their
4848
`catalog.servesPublicCommand` names (`install_source` as `install-from-source`), or else by
4949
their own name (`runtime`, which `react-devtools` and Maestro flows send);
50-
- internal commands with no platform execution (leases, takeover, session bookkeeping) are
50+
- internal commands with no platform execution (leases, `human_control`, session bookkeeping) are
5151
protocol plumbing that no rule decides;
5252
- any other name is decided by the rules, so an allow list fails closed for it.
5353

‎packages/command-registry/src/types.ts‎

Lines changed: 20 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -127,25 +127,33 @@ export type CommandCatalogGroup = 'public' | 'internal' | 'local-cli';
127127
*/
128128
export type CommandFrameworkTier = 'core' | 'extended';
129129

130-
export type CommandCatalogFacet = {
131-
/**
132-
* The command catalog group. This is explicit on every descriptor so new
133-
* descriptors cannot accidentally become public CLI/MCP commands by omission.
134-
*/
135-
group: CommandCatalogGroup;
130+
type CommandCatalogFacetBase = {
136131
/**
137132
* Stable property name used by catalog object projections, e.g.
138133
* `longPress` for the command name `longpress`.
139134
*/
140135
key?: string;
141-
/**
142-
* Internal commands only: the public command this one carries to the daemon, so ADR 0029
143-
* daemon-policy command rules name the public command (`install_source` serves
144-
* `install-from-source`). An internal device-executing command without it is named by itself.
145-
*/
146-
servesPublicCommand?: string;
147136
};
148137

138+
/**
139+
* The command catalog group is explicit on every descriptor so new descriptors cannot
140+
* accidentally become public CLI/MCP commands by omission.
141+
*/
142+
export type CommandCatalogFacet = CommandCatalogFacetBase &
143+
(
144+
| {
145+
group: 'internal';
146+
/**
147+
* The public command this internal command carries to the daemon, so ADR 0029
148+
* daemon-policy command rules name the public command (`install_source` serves
149+
* `install-from-source`). An internal command with platform execution and no value is
150+
* named by itself.
151+
*/
152+
servesPublicCommand?: string;
153+
}
154+
| { group: Exclude<CommandCatalogGroup, 'internal'>; servesPublicCommand?: never }
155+
);
156+
149157
/**
150158
* ADR 0016: whether a recorded request changes app-visible state or only
151159
* observes it. The resolver form keeps subcommand-sensitive decisions on the

‎src/__tests__/test-utils/daemon-http-fixture.ts‎

Lines changed: 7 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,6 @@
1-
import fs from 'node:fs';
21
import http from 'node:http';
3-
import path from 'node:path';
42
import { computeDaemonCodeSignature } from '@agent-device/host-kit/code-signature';
5-
import { findProjectRoot } from '@agent-device/host-kit/version';
3+
import { resolveDaemonLaunchSpec } from '../../daemon-client/daemon-launch-spec.ts';
64
import { listenOnLoopback } from './loopback.ts';
75

86
// A loopback stand-in for a running daemon: answers `GET /health`, echoes `responseData` as the
@@ -61,16 +59,13 @@ export async function startHttpDaemonFixture(
6159
return { server, port, seenPaths, rpcRequests };
6260
}
6361

64-
/** The code signature a daemon started from this checkout records, so a fixture daemon is reused. */
62+
/**
63+
* The code signature a daemon started from this checkout records, read through the same launch
64+
* spec the client uses to launch and sign its daemon, so a fixture daemon is reused.
65+
*/
6566
export function currentDaemonCodeSignature(): string {
66-
const root = findProjectRoot();
67-
const distPath = path.join(root, 'dist', 'src', 'internal', 'daemon.js');
68-
const sourcePath = path.join(root, 'src', 'daemon.ts');
69-
const entryPath =
70-
process.execArgv.includes('--experimental-strip-types') || !fs.existsSync(distPath)
71-
? sourcePath
72-
: distPath;
73-
return computeDaemonCodeSignature(entryPath, root);
67+
const spec = resolveDaemonLaunchSpec();
68+
return computeDaemonCodeSignature(spec.useSrc ? spec.srcPath : spec.distPath, spec.root);
7469
}
7570

7671
/** Swaps `process.stderr.write` for a buffer until `restore`, so a test can read what was printed. */

‎src/daemon-client/__tests__/daemon-client-policy-reuse.test.ts‎

Lines changed: 62 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,13 @@ import assert from 'node:assert/strict';
22
import fs from 'node:fs';
33
import path from 'node:path';
44
import { afterEach, test, vi } from 'vitest';
5+
6+
vi.mock('@agent-device/host-kit/command', async (importOriginal) => ({
7+
...(await importOriginal<typeof import('@agent-device/host-kit/command')>()),
8+
runCmdDetachedMonitored: vi.fn(),
9+
}));
10+
11+
import { runCmdDetachedMonitored } from '@agent-device/host-kit/command';
512
import { readProcessStartTime } from '@agent-device/host-kit/process';
613
import { readVersion } from '@agent-device/host-kit/version';
714
import { AppError } from '@agent-device/kernel/errors';
@@ -15,9 +22,12 @@ import { loadDaemonPolicy } from '../../daemon-policy-file.ts';
1522
import { resolveDaemonPaths } from '../../daemon-resolution.ts';
1623
import { sendToDaemon } from '../daemon-client.ts';
1724

18-
// ADR 0029: a caller that names a daemon policy must not reuse a daemon that enforces another one.
25+
// ADR 0029: a caller that names a daemon policy must not use a daemon that enforces another one.
26+
27+
const mockSpawnDaemon = vi.mocked(runCmdDetachedMonitored);
1928

2029
afterEach(() => {
30+
mockSpawnDaemon.mockReset();
2131
vi.unstubAllEnvs();
2232
});
2333

@@ -81,6 +91,30 @@ test('a caller naming a policy reuses a daemon that enforces the same policy', a
8191
}
8292
});
8393

94+
test('a caller naming a policy refuses a daemon another caller started without it', async (t) => {
95+
if (!(await supportsLoopbackBind())) {
96+
t.skip('loopback listeners are not permitted in this environment');
97+
return;
98+
}
99+
// No daemon runs yet. While this caller starts one, another caller's daemon without the policy
100+
// wins the state dir: its metadata is what startup finds.
101+
const fixture = await startReusableDaemon({ policyDigest: undefined, publish: 'on-spawn' });
102+
try {
103+
await assert.rejects(
104+
() => sendSmoke(fixture.stateDir),
105+
(error: unknown) => {
106+
assert.ok(error instanceof AppError);
107+
assert.equal(error.details?.reason, 'DAEMON_POLICY_MISMATCH');
108+
return true;
109+
},
110+
);
111+
assert.equal(mockSpawnDaemon.mock.calls.length, 1, 'this caller did start a daemon');
112+
assert.ok(!fixture.seenPaths.includes('POST /rpc'), 'no command reaches the daemon');
113+
} finally {
114+
await fixture.close();
115+
}
116+
});
117+
84118
async function sendSmoke(stateDir: string) {
85119
return await sendToDaemon({
86120
session: 'default',
@@ -91,7 +125,10 @@ async function sendSmoke(stateDir: string) {
91125
});
92126
}
93127

94-
async function startReusableDaemon(options: { policyDigest: string | undefined }) {
128+
async function startReusableDaemon(options: {
129+
policyDigest: string | undefined;
130+
publish?: 'now' | 'on-spawn';
131+
}) {
95132
const stateDir = mkdtempForTestSync('agent-device-policy-reuse-');
96133
const policyPath = path.join(stateDir, 'policy.json');
97134
fs.writeFileSync(
@@ -102,20 +139,29 @@ async function startReusableDaemon(options: { policyDigest: string | undefined }
102139
const daemon = await startHttpDaemonFixture({ via: 'http' });
103140
const paths = resolveDaemonPaths(stateDir);
104141
fs.mkdirSync(paths.baseDir, { recursive: true });
105-
fs.writeFileSync(
106-
paths.infoPath,
107-
`${JSON.stringify({
108-
httpPort: daemon.port,
109-
transport: 'http',
110-
token: 'local-secret',
111-
pid: process.pid,
112-
version: readVersion(),
113-
codeSignature: currentDaemonCodeSignature(),
114-
processStartTime: readProcessStartTime(process.pid) ?? undefined,
115-
policyDigest:
116-
options.policyDigest === 'matching' ? loadDaemonPolicy()?.digest : options.policyDigest,
117-
})}\n`,
118-
);
142+
const publishInfo = () =>
143+
fs.writeFileSync(
144+
paths.infoPath,
145+
`${JSON.stringify({
146+
httpPort: daemon.port,
147+
transport: 'http',
148+
token: 'local-secret',
149+
pid: process.pid,
150+
version: readVersion(),
151+
codeSignature: currentDaemonCodeSignature(),
152+
processStartTime: readProcessStartTime(process.pid) ?? undefined,
153+
policyDigest:
154+
options.policyDigest === 'matching' ? loadDaemonPolicy()?.digest : options.policyDigest,
155+
})}\n`,
156+
);
157+
if (options.publish === 'on-spawn') {
158+
mockSpawnDaemon.mockImplementation(() => {
159+
publishInfo();
160+
return { pid: process.pid, exited: new Promise(() => {}) };
161+
});
162+
} else {
163+
publishInfo();
164+
}
119165
return {
120166
stateDir,
121167
seenPaths: daemon.seenPaths,

‎src/daemon-client/daemon-client-lifecycle.ts‎

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -157,6 +157,13 @@ export async function ensureDaemon(settings: DaemonClientSettings): Promise<Ensu
157157
return await ensureRemoteDaemon(settings);
158158
}
159159

160+
const ensured = await ensureLocalDaemon(settings);
161+
// Checked on both branches: a startup can resolve to a daemon another caller raced in.
162+
await assertDaemonPolicyMatches(ensured.info, settings.paths.baseDir);
163+
return ensured;
164+
}
165+
166+
async function ensureLocalDaemon(settings: DaemonClientSettings): Promise<EnsuredDaemon> {
160167
const reusable = await readReusableLocalDaemon(settings);
161168
if (reusable) return { info: reusable, startedByClient: false };
162169

@@ -194,10 +201,7 @@ async function readReusableLocalDaemon(settings: DaemonClientSettings): Promise<
194201
onAnyAdvertisedTransport: async () =>
195202
viaClientTransport || (await canConnectReusableDaemon(existing, 'auto')),
196203
});
197-
if (decision.kind === 'reuse') {
198-
await assertReusableDaemonPolicy(existing, settings.paths.baseDir);
199-
return existing;
200-
}
204+
if (decision.kind === 'reuse') return existing;
201205
if (decision.kind === 'refuseNewer') {
202206
throw newerDaemonRefusedError(existing, decision, settings.paths.baseDir);
203207
}
@@ -209,10 +213,10 @@ async function readReusableLocalDaemon(settings: DaemonClientSettings): Promise<
209213
}
210214

211215
/**
212-
* ADR 0029: a caller that names a daemon policy must not silently reuse a daemon that enforces a
213-
* different one (or none). A caller that names no policy reuses whatever the daemon enforces.
216+
* ADR 0029: a caller that names a daemon policy must not silently use a daemon that enforces a
217+
* different one (or none). A caller that names no policy uses whatever the daemon enforces.
214218
*/
215-
async function assertReusableDaemonPolicy(existing: DaemonInfo, stateDir: string): Promise<void> {
219+
async function assertDaemonPolicyMatches(existing: DaemonInfo, stateDir: string): Promise<void> {
216220
if (!process.env.AGENT_DEVICE_DAEMON_POLICY?.trim()) return;
217221
const { loadDaemonPolicy } = await import('../daemon-policy-file.ts');
218222
const expected = loadDaemonPolicy(process.env)?.digest;

‎src/daemon-policy-file.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ const DESCRIPTORS = commandDescriptors as readonly CommandDescriptor[];
2929

3030
/**
3131
* Derived from the registry (catalog group plus platform execution). Internal daemon commands with
32-
* no platform execution — leases, takeover, session bookkeeping — are protocol plumbing that no
32+
* no platform execution — leases, `human_control`, session bookkeeping — are protocol plumbing that no
3333
* command rule decides. Every other command a request names is decided by command rules, including
3434
* a name the registry does not know, so an allow list fails closed.
3535
*/
@@ -43,7 +43,7 @@ const PROTOCOL_COMMANDS: ReadonlySet<string> = new Set(
4343
);
4444

4545
/**
46-
* The names a policy may use: public commands, and internal commands that execute on a device,
46+
* The names a policy may use: public commands, and internal commands with platform execution,
4747
* each by the public command it serves (`install_source` as `install-from-source`) or else by its
4848
* own name (`runtime`).
4949
*/

‎src/daemon/__tests__/daemon-policy.test.ts‎

Lines changed: 21 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -228,34 +228,42 @@ test('a provider answering with only out-of-scope devices does not hide allowed
228228

229229
test('an allow list denies the internal runtime command unless it names runtime', async () => {
230230
const { handler, bind } = makeHandler(policy({ commands: { allow: ['snapshot'] } }));
231+
const runtimeClear = request('runtime', { positionals: ['clear'] });
231232

232-
const response = await handler(request('runtime', { positionals: ['clear'] }));
233+
const response = await handler(runtimeClear);
233234

234235
expectPolicyDenied(response, 'command');
235236
expect(response).toMatchObject({ error: { details: { command: 'runtime' } } });
236237
expect(bind).not.toHaveBeenCalled();
238+
expect(deniedCommand(policy({ commands: { allow: ['runtime'] } }), runtimeClear)).toBeUndefined();
237239
});
238240

239241
test('command rules name internal device commands the registry way', () => {
240242
const denyRuntime = policy({ commands: { deny: ['runtime', 'install-from-source'] } });
241243
const allowSnapshot = policy({ commands: { allow: ['snapshot'] } });
242244

243-
expect(() => assertDaemonPolicyAdmitsRequest(denyRuntime, request('runtime'))).toThrow(
244-
/denies the runtime command/,
245-
);
246-
expect(() => assertDaemonPolicyAdmitsRequest(denyRuntime, request('install_source'))).toThrow(
247-
/denies the install-from-source command/,
248-
);
245+
expect(deniedCommand(denyRuntime, request('runtime'))).toBe('runtime');
246+
expect(deniedCommand(denyRuntime, request('install_source'))).toBe('install-from-source');
249247
// Protocol plumbing (leases, session bookkeeping) is never decided by command rules.
250-
expect(() =>
251-
assertDaemonPolicyAdmitsRequest(allowSnapshot, request('lease_heartbeat')),
252-
).not.toThrow();
248+
expect(deniedCommand(allowSnapshot, request('lease_heartbeat'))).toBeUndefined();
253249
// A name the registry does not know fails closed under an allow list.
254-
expect(() => assertDaemonPolicyAdmitsRequest(allowSnapshot, request('not-a-command'))).toThrow(
255-
/denies the not-a-command command/,
256-
);
250+
expect(deniedCommand(allowSnapshot, request('not-a-command'))).toBe('not-a-command');
257251
});
258252

253+
/** The command a policy denial names, or undefined when the policy admits the request. */
254+
function deniedCommand(daemonPolicy: ReturnType<typeof policy>, req: DaemonRequest) {
255+
try {
256+
assertDaemonPolicyAdmitsRequest(daemonPolicy, req);
257+
return undefined;
258+
} catch (error) {
259+
expect(error).toMatchObject({
260+
code: 'UNAUTHORIZED',
261+
details: { reason: 'DAEMON_POLICY_DENIED', rule: 'command' },
262+
});
263+
return (error as { details: { command: string } }).details.command;
264+
}
265+
}
266+
259267
test('doctor counts only the devices the policy allows', async () => {
260268
const { handler } = makeHandler(policy({ devices: { allow: [{ udid: IOS_SIMULATOR.id }] } }), {
261269
inventory: [IOS_SIMULATOR, OTHER_SIMULATOR],

0 commit comments

Comments
 (0)