Skip to content

Commit 1bcdc8c

Browse files
okwasniewskiokwasniewski
committed
refactor(daemon): takeover decision as a value, one SemVer comparator
Review follow-ups on the newer-daemon refusal: - resolveDaemonTakeover returns a typed decision (reuse | replace | refuseNewer) instead of a reason string that sometimes throws; the lifecycle layer, which already owns the takeover notice and knows the state dir, builds the refusal error and its shell-quoted hint. The launch-spec module no longer imports device-shell. - compareVersions is exported from host-kit/version and src/cli/update-check.ts uses it, deleting its numeric-collation copy (which ranked 0.12.0 below 0.12.0-dev and never prompted a -dev build). parseVersion is one anchored regex; a malformed version reads as 0.0.0 and the doc says so. - the loopback daemon fixture and captureStderr move to src/__tests__/test-utils/daemon-http-fixture.ts, shared by the lifecycle suite and the new refusal test instead of copied.
1 parent f13e49f commit 1bcdc8c

11 files changed

Lines changed: 261 additions & 253 deletions

File tree

‎packages/host-kit/src/internal/version.test.ts‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ import { afterEach, test, vi } from 'vitest';
55
import { mkdtempForTestSync } from './tmp-dir.fixtures.ts';
66
import { resetAllProcessMemosForTests } from '@agent-device/kernel/ttl-memo';
77
import { resolveAgentDeviceProjectRoot } from './project-root.ts';
8-
import { findProjectRoot, isNewerVersion, readVersion } from './version.ts';
8+
import { compareVersions, findProjectRoot, isNewerVersion, readVersion } from './version.ts';
99

1010
afterEach(() => {
1111
vi.restoreAllMocks();
@@ -150,3 +150,10 @@ test('isNewerVersion ranks a release above the prerelease of the same base', ()
150150
assert.equal(isNewerVersion('0.21.13-beta', '0.21.13-alpha.1'), true);
151151
assert.equal(isNewerVersion('0.21.13+build.2', '0.21.13+build.1'), false);
152152
});
153+
154+
test('compareVersions reads malformed or prefixed versions conservatively', () => {
155+
assert.equal(compareVersions('v0.21.13', '0.21.13'), 0);
156+
assert.equal(compareVersions('garbage', '0.0.1'), -1);
157+
assert.equal(compareVersions('garbage', '0.0.0'), 0);
158+
assert.equal(compareVersions('1.2.3.4', '1.2.3'), -1, 'four segments is not a version');
159+
});

‎packages/host-kit/src/internal/version.ts‎

Lines changed: 26 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -33,35 +33,49 @@ export function findProjectRoot(): string {
3333
}
3434

3535
/**
36-
* Whether `candidate` is a later release than `baseline` under SemVer ordering: numeric
37-
* `major.minor.patch` first, then a release sorts after any prerelease of the same base
38-
* (`0.21.13` > `0.21.13-dev`), and prerelease identifiers compare per dot-separated field,
39-
* numerically when both are numbers and lexically otherwise. Build metadata is ignored. The daemon
40-
* takeover decision needs exactly this to tell an upgrade from a downgrade across the `-dev`
41-
* versions main carries between releases.
36+
* Whether `candidate` is a later release than `baseline` (see {@link compareVersions}).
4237
*/
4338
export function isNewerVersion(candidate: string, baseline: string): boolean {
4439
return compareVersions(candidate, baseline) > 0;
4540
}
4641

47-
function compareVersions(left: string, right: string): number {
42+
/**
43+
* SemVer order for the versions this package publishes: numeric `major.minor.patch` first, then a
44+
* release sorts after any prerelease of the same base (`0.21.13` > `0.21.13-dev`, the shape main
45+
* carries between releases), and prerelease fields compare per dot-separated field, numerically
46+
* when both are numbers and lexically otherwise. Build metadata is ignored. A string that is not a
47+
* version at all reads as `0.0.0`, so a malformed version always compares as the oldest.
48+
*/
49+
export function compareVersions(left: string, right: string): number {
4850
const a = parseVersion(left);
4951
const b = parseVersion(right);
5052
return compareRelease(a.release, b.release) || comparePrerelease(a.prerelease, b.prerelease);
5153
}
5254

53-
function compareRelease(a: number[], b: number[]): number {
55+
const SEMVER = /^v?(\d+)\.(\d+)\.(\d+)(?:-([0-9A-Za-z.-]+))?(?:\+[0-9A-Za-z.-]+)?$/;
56+
57+
type ParsedVersion = { release: [number, number, number]; prerelease: string[] };
58+
59+
function parseVersion(version: string): ParsedVersion {
60+
const match = SEMVER.exec(version.trim());
61+
if (!match) return { release: [0, 0, 0], prerelease: [] };
62+
return {
63+
release: [Number(match[1]), Number(match[2]), Number(match[3])],
64+
prerelease: match[4]?.split('.') ?? [],
65+
};
66+
}
67+
68+
function compareRelease(a: ParsedVersion['release'], b: ParsedVersion['release']): number {
5469
for (let i = 0; i < 3; i += 1) {
55-
const x = a[i] ?? 0;
56-
const y = b[i] ?? 0;
57-
if (x !== y) return x > y ? 1 : -1;
70+
if (a[i] !== b[i]) return a[i]! > b[i]! ? 1 : -1;
5871
}
5972
return 0;
6073
}
6174

6275
/** A release (no prerelease) sorts after every prerelease of the same base. */
6376
function comparePrerelease(a: string[], b: string[]): number {
64-
if (a.length === 0 || b.length === 0) return Math.sign(b.length - a.length);
77+
if (a.length === 0) return b.length === 0 ? 0 : 1;
78+
if (b.length === 0) return -1;
6579
const fields = Math.max(a.length, b.length);
6680
for (let i = 0; i < fields; i += 1) {
6781
const x = a[i];
@@ -83,13 +97,3 @@ function comparePrereleaseField(x: string, y: string): number {
8397
if (xNumeric !== yNumeric) return xNumeric ? -1 : 1;
8498
return x > y ? 1 : -1;
8599
}
86-
87-
function parseVersion(version: string): { release: number[]; prerelease: string[] } {
88-
const [core = '', prerelease = ''] = version.split('+', 1)[0]!.split(/-(.*)/s, 2);
89-
const release = core.split('.').map((part) => Number.parseInt(part, 10));
90-
while (release.length < 3) release.push(0);
91-
return {
92-
release: release.map((part) => (Number.isNaN(part) ? 0 : part)),
93-
prerelease: prerelease ? prerelease.split('.') : [],
94-
};
95-
}

‎packages/host-kit/src/version.ts‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,7 @@
1-
export { findProjectRoot, isNewerVersion, readVersion } from './internal/version.ts';
1+
export {
2+
compareVersions,
3+
findProjectRoot,
4+
isNewerVersion,
5+
readVersion,
6+
} from './internal/version.ts';
27
export { DAEMON_SOURCE_ENTRY, isSourceCheckoutProjectRoot } from './internal/project-root.ts';

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

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
import http from 'node:http';
2+
import { listenOnLoopback } from './loopback.ts';
3+
4+
// A loopback stand-in for a running daemon: answers `GET /health`, echoes `responseData` as the
5+
// result of every `POST /rpc`, and records what it was asked, for the daemon-client tests that
6+
// decide which daemon a command keeps.
7+
8+
export type HttpDaemonFixture = {
9+
server: http.Server;
10+
port: number;
11+
seenPaths: string[];
12+
rpcRequests: Record<string, any>[];
13+
};
14+
15+
export async function startHttpDaemonFixture(
16+
responseData: Record<string, unknown>,
17+
): Promise<HttpDaemonFixture> {
18+
const seenPaths: string[] = [];
19+
const rpcRequests: Record<string, any>[] = [];
20+
const server = http.createServer((req, res) => {
21+
const url = new URL(req.url || '/', 'http://127.0.0.1');
22+
seenPaths.push(`${req.method ?? 'GET'} ${url.pathname}`);
23+
24+
if (req.method === 'GET' && url.pathname === '/health') {
25+
res.writeHead(200);
26+
res.end('ok');
27+
return;
28+
}
29+
30+
if (req.method === 'POST' && url.pathname === '/rpc') {
31+
const chunks: Buffer[] = [];
32+
req.on('data', (chunk) => {
33+
chunks.push(Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk));
34+
});
35+
req.on('end', () => {
36+
const rpcRequest = JSON.parse(Buffer.concat(chunks).toString('utf8')) as Record<
37+
string,
38+
any
39+
>;
40+
rpcRequests.push(rpcRequest);
41+
res.writeHead(200, { 'content-type': 'application/json' });
42+
res.end(
43+
JSON.stringify({
44+
jsonrpc: '2.0',
45+
id: rpcRequest.id,
46+
result: { ok: true, data: responseData },
47+
}),
48+
);
49+
});
50+
return;
51+
}
52+
53+
res.writeHead(404);
54+
res.end('not found');
55+
});
56+
const port = await listenOnLoopback(server);
57+
return { server, port, seenPaths, rpcRequests };
58+
}
59+
60+
/** Swaps `process.stderr.write` for a buffer until `restore`, so a test can read what was printed. */
61+
export function captureStderr(): { read: () => string; restore: () => void } {
62+
const originalWrite = process.stderr.write.bind(process.stderr);
63+
let captured = '';
64+
(process.stderr as { write: typeof process.stderr.write }).write = ((chunk: unknown) => {
65+
captured += String(chunk);
66+
return true;
67+
}) as typeof process.stderr.write;
68+
return {
69+
read: () => captured,
70+
restore: () => {
71+
process.stderr.write = originalWrite;
72+
},
73+
};
74+
}

‎src/__tests__/update-check.test.ts‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,32 @@ test('notifier prints cached upgrade notice once for a newly discovered version'
8787
assert.equal(cache.prompted, true);
8888
});
8989

90+
test('notifier treats the release as newer than the -dev build of the same base', () => {
91+
// main carries `-dev` between releases; the shared SemVer comparator ranks the release above it,
92+
// where numeric string collation ranked it below and never prompted.
93+
const stateDir = makeTempStateDir();
94+
cleanupPaths.push(stateDir);
95+
writeCache(stateDir, {
96+
latestVersion: '0.12.0',
97+
checkedAt: '2026-03-25T10:00:00.000Z',
98+
});
99+
100+
let stderr = '';
101+
vi.spyOn(process.stderr, 'write').mockImplementation(((chunk: unknown) => {
102+
stderr += String(chunk);
103+
return true;
104+
}) as typeof process.stderr.write);
105+
106+
maybeRunUpgradeNotifier({
107+
command: 'devices',
108+
currentVersion: '0.12.0-dev',
109+
stateDir,
110+
flags: {},
111+
});
112+
113+
assert.match(stderr, /Update available: agent-device 0\.12\.0-dev -> 0\.12\.0/);
114+
});
115+
90116
test('notifier skips repeat prompts after the cached version was already shown', () => {
91117
const stateDir = makeTempStateDir();
92118
cleanupPaths.push(stateDir);

‎src/cli/update-check.ts‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import fs from 'node:fs';
22
import path from 'node:path';
33
import { fileURLToPath } from 'node:url';
44
import { runCmdDetached } from '@agent-device/host-kit/command';
5+
import { compareVersions } from '@agent-device/host-kit/version';
56

67
const PACKAGE_NAME = 'agent-device';
78
const UPDATE_CHECK_INTERVAL_MS = 14 * 24 * 60 * 60 * 1000;
@@ -168,10 +169,6 @@ function parseTimestamp(value: string | undefined): number | undefined {
168169
return Number.isNaN(parsed) ? undefined : parsed;
169170
}
170171

171-
function compareVersions(left: string, right: string): number {
172-
return left.localeCompare(right, undefined, { numeric: true });
173-
}
174-
175172
export function readUpdateCheckWorkerArgs(
176173
argv: string[],
177174
): { cachePath: string; currentVersion: string } | null {

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

Lines changed: 5 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,11 @@ import {
2828
listenOnLoopback,
2929
supportsLoopbackBind,
3030
} from '../../__tests__/test-utils/loopback.ts';
31+
import {
32+
captureStderr,
33+
startHttpDaemonFixture,
34+
type HttpDaemonFixture,
35+
} from '../../__tests__/test-utils/daemon-http-fixture.ts';
3136
import { AppError } from '@agent-device/kernel/errors';
3237
import { runCmdDetachedMonitored, runCmdSync } from '@agent-device/host-kit/command';
3338
import { shellQuoteIfNeeded } from '@agent-device/kernel/device-shell';
@@ -46,13 +51,6 @@ type DaemonInfoFixture = {
4651
processStartTime?: string;
4752
};
4853

49-
type HttpDaemonFixture = {
50-
server: http.Server;
51-
port: number;
52-
seenPaths: string[];
53-
rpcRequests: Record<string, any>[];
54-
};
55-
5654
const mockRunCmdDetached = vi.mocked(runCmdDetachedMonitored);
5755
const mockRunCmdSync = vi.mocked(runCmdSync);
5856
const mockSleep = vi.mocked(sleep);
@@ -109,51 +107,6 @@ function writeDaemonLock(
109107
);
110108
}
111109

112-
async function startHttpDaemonFixture(
113-
responseData: Record<string, unknown>,
114-
): Promise<HttpDaemonFixture> {
115-
const seenPaths: string[] = [];
116-
const rpcRequests: Record<string, any>[] = [];
117-
const server = http.createServer((req, res) => {
118-
const url = new URL(req.url || '/', 'http://127.0.0.1');
119-
seenPaths.push(`${req.method ?? 'GET'} ${url.pathname}`);
120-
121-
if (req.method === 'GET' && url.pathname === '/health') {
122-
res.writeHead(200);
123-
res.end('ok');
124-
return;
125-
}
126-
127-
if (req.method === 'POST' && url.pathname === '/rpc') {
128-
const chunks: Buffer[] = [];
129-
req.on('data', (chunk) => {
130-
chunks.push(Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk));
131-
});
132-
req.on('end', () => {
133-
const rpcRequest = JSON.parse(Buffer.concat(chunks).toString('utf8')) as Record<
134-
string,
135-
any
136-
>;
137-
rpcRequests.push(rpcRequest);
138-
res.writeHead(200, { 'content-type': 'application/json' });
139-
res.end(
140-
JSON.stringify({
141-
jsonrpc: '2.0',
142-
id: rpcRequest.id,
143-
result: { ok: true, data: responseData },
144-
}),
145-
);
146-
});
147-
return;
148-
}
149-
150-
res.writeHead(404);
151-
res.end('not found');
152-
});
153-
const port = await listenOnLoopback(server);
154-
return { server, port, seenPaths, rpcRequests };
155-
}
156-
157110
/** Like `startHttpDaemonFixture`, but every RPC call returns `errorResult` as an `{ok:false}` result. */
158111
async function startHttpDaemonErrorFixture(
159112
errorResult: Record<string, unknown>,
@@ -649,21 +602,6 @@ test('sendToDaemon replaces socket-only daemon metadata when HTTP transport is r
649602
}
650603
});
651604

652-
function captureStderr(): { read: () => string; restore: () => void } {
653-
const originalWrite = process.stderr.write.bind(process.stderr);
654-
let captured = '';
655-
(process.stderr as { write: typeof process.stderr.write }).write = ((chunk: unknown) => {
656-
captured += String(chunk);
657-
return true;
658-
}) as typeof process.stderr.write;
659-
return {
660-
read: () => captured,
661-
restore: () => {
662-
process.stderr.write = originalWrite;
663-
},
664-
};
665-
}
666-
667605
test('sendRequest timeout cleanup uses resolved daemon paths instead of request flags', async (t) => {
668606
if (!(await supportsLoopbackBind())) {
669607
t.skip('loopback listeners are not permitted in this environment');

0 commit comments

Comments
 (0)