From b1624ded4ef1f0ea8b266869695cbe6dbf417108 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sat, 31 Oct 2026 05:43:43 -0700 Subject: [PATCH 1/6] feat(cli): commonly doctor, plus a stale-CLI notice on agent run and daemon run (TASK-660) D213 keeps the Node daemon and hardens install by making breakage visible. `commonly doctor` prints one pass/fail/skip line per check, with a fix command on each fail, and exits 1 when any check fails: Node against the engines floor, the commonly on PATH and its version, the saved login and whose token it is (never printed), the daemon (record, service file, server liveness), every adapter CLI a configured agent uses, and on macOS the claude adapter's Seatbelt and keychain-file prerequisites (stat only; it never runs `security`). `agent run` and `daemon run` print one line when the installed CLI is 3+ releases behind npm latest. The lookup is fired, not awaited, capped at 2 s by its own timer, cached 24 h, and silent offline or with COMMONLY_NO_UPDATE_CHECK=1. Co-Authored-By: Claude Opus 5.5 Co-authored-by: otto Agent-Model: claude/claude-opus-5-5/default --- cli/__tests__/command-registration.test.mjs | 8 + cli/__tests__/doctor.test.mjs | 296 ++++++++++++++++++++ cli/__tests__/update-check.test.mjs | 204 ++++++++++++++ cli/jest.setup.cjs | 5 + cli/src/commands/agent.js | 7 + cli/src/commands/daemon.js | 4 + cli/src/commands/doctor.js | 106 +++++++ cli/src/index.js | 5 + cli/src/lib/doctor.js | 221 +++++++++++++++ cli/src/lib/update-check.js | 180 ++++++++++++ docs/cli/README.md | 4 + docs/cli/doctor.md | 46 +++ 12 files changed, 1086 insertions(+) create mode 100644 cli/__tests__/doctor.test.mjs create mode 100644 cli/__tests__/update-check.test.mjs create mode 100644 cli/src/commands/doctor.js create mode 100644 cli/src/lib/doctor.js create mode 100644 cli/src/lib/update-check.js create mode 100644 docs/cli/doctor.md diff --git a/cli/__tests__/command-registration.test.mjs b/cli/__tests__/command-registration.test.mjs index 0e87afd684..62ff263462 100644 --- a/cli/__tests__/command-registration.test.mjs +++ b/cli/__tests__/command-registration.test.mjs @@ -22,6 +22,7 @@ import { Command } from 'commander'; import { cascadeOverridesFromOpts, registerAgent } from '../src/commands/agent.js'; import { registerDaemon } from '../src/commands/daemon.js'; +import { registerDoctor } from '../src/commands/doctor.js'; import { CASCADE_ENV_VARS } from '../src/lib/enforcement.js'; const registerAll = () => { @@ -29,6 +30,7 @@ const registerAll = () => { program.exitOverride(); registerAgent(program); registerDaemon(program); + registerDoctor(program); return program; }; @@ -112,3 +114,9 @@ describe('command registration', () => { } }); }); + +test('doctor is wired with its --instance option (TASK-660)', () => { + const doctor = findCommand(registerAll(), ['doctor']); + expect(doctor).toBeDefined(); + expect(doctor.options.map((option) => option.long)).toContain('--instance'); +}); diff --git a/cli/__tests__/doctor.test.mjs b/cli/__tests__/doctor.test.mjs new file mode 100644 index 0000000000..07b09cf802 --- /dev/null +++ b/cli/__tests__/doctor.test.mjs @@ -0,0 +1,296 @@ +import { + checkAdapters, + checkDaemon, + checkInstall, + checkLogin, + checkMacClaude, + checkNode, + doctorExitCode, + enginesFloor, + findOnPath, + formatDoctorLine, + runDoctor, + FAIL, + PASS, + SKIP, +} from '../src/lib/doctor.js'; + +// TASK-660 item 2: every check passes and fails here against a faked +// environment. Nothing in this file reads the real PATH, keychain, network, +// daemon record or adapter CLIs. + +const NOW = Date.parse('2026-10-31T12:00:00Z'); + +describe('(a) node', () => { + test('passes at or above the engines floor', () => { + expect(checkNode({ nodeVersion: '22.3.0', enginesRange: '>=20.0.0' })).toMatchObject({ status: PASS }); + expect(checkNode({ nodeVersion: '20.0.0', enginesRange: '>=20' })).toMatchObject({ status: PASS }); + }); + + test('fails below it, naming the floor', () => { + const r = checkNode({ nodeVersion: '18.19.1', enginesRange: '>=20.0.0' }); + expect(r).toMatchObject({ status: FAIL, fix: 'install Node 20 or newer' }); + expect(r.detail).toContain('18.19.1'); + }); + + test('skips a range it cannot read as a floor', () => { + expect(checkNode({ nodeVersion: '22.0.0', enginesRange: '^20 || ^22' })).toMatchObject({ status: SKIP }); + expect(enginesFloor('>= 20.1')).toEqual([20, 1, 0]); + }); +}); + +describe('(b) install', () => { + const fakeFs = (paths) => (p) => paths.includes(p); + + test('passes with the first commonly on PATH and its version', () => { + const r = checkInstall({ + pathValue: '/nope:/opt/homebrew/bin:/usr/local/bin', + platform: 'darwin', + isExecutable: fakeFs(['/opt/homebrew/bin/commonly', '/usr/local/bin/commonly']), + runVersion: (bin) => (bin === '/opt/homebrew/bin/commonly' ? '0.1.210' : '0.1.100'), + runningVersion: '0.1.210', + }); + expect(r.status).toBe(PASS); + expect(r.detail).toBe('commonly 0.1.210 at /opt/homebrew/bin/commonly (1 more on PATH)'); + }); + + test('says when the doctor running is not the commonly on PATH', () => { + const r = checkInstall({ + pathValue: '/bin', platform: 'linux', isExecutable: fakeFs(['/bin/commonly']), runVersion: () => '0.1.161', runningVersion: '0.1.210', + }); + expect(r.detail).toContain('this doctor is 0.1.210'); + }); + + test('fails when nothing is on PATH', () => { + expect(checkInstall({ + pathValue: '/a:/b', platform: 'darwin', isExecutable: () => false, runVersion: () => '1.0.0', + })).toMatchObject({ status: FAIL, fix: 'npm install -g @commonlyai/cli' }); + }); + + test('fails when the binary does not answer --version', () => { + expect(checkInstall({ + pathValue: '/a', platform: 'darwin', isExecutable: fakeFs(['/a/commonly']), runVersion: () => null, + })).toMatchObject({ status: FAIL }); + }); + + test('looks for the .cmd shim on Windows', () => { + expect(findOnPath({ + command: 'commonly', pathValue: 'C:\\npm;C:\\other', platform: 'win32', isExecutable: (p) => p.endsWith('commonly.cmd'), + })).toHaveLength(2); + }); +}); + +describe('(c) login', () => { + const instance = { key: 'default', url: 'https://api.commonly.me', hasToken: true }; + + test('passes with the token\'s user, and never prints the token', async () => { + const r = await checkLogin({ instance, fetchUser: async () => ({ username: 'lily-shen', token: 'cm_secret' }) }); + expect(r).toMatchObject({ status: PASS, detail: '@lily-shen on default (https://api.commonly.me)' }); + expect(JSON.stringify(r)).not.toContain('cm_secret'); + }); + + test('fails when not logged in', async () => { + expect(await checkLogin({ instance: null, fetchUser: async () => ({}) })).toMatchObject({ status: FAIL, fix: 'commonly login' }); + }); + + test('fails when the saved instance has no token, without calling the server', async () => { + let called = false; + const r = await checkLogin({ instance: { ...instance, hasToken: false }, fetchUser: async () => { called = true; } }); + expect(r).toMatchObject({ status: FAIL, fix: 'commonly login --instance default' }); + expect(called).toBe(false); + }); + + test('fails when the server rejects the token', async () => { + const r = await checkLogin({ instance, fetchUser: async () => { throw Object.assign(new Error('Session expired'), { status: 401 }); } }); + expect(r).toMatchObject({ status: FAIL, detail: 'default (https://api.commonly.me) rejected the saved token' }); + }); + + test('fails when the server cannot be reached', async () => { + const r = await checkLogin({ instance, fetchUser: async () => { throw new TypeError('fetch failed'); } }); + expect(r.status).toBe(FAIL); + expect(r.detail).toContain('could not reach https://api.commonly.me'); + }); +}); + +describe('(d) daemon', () => { + const record = { machineName: 'Lab Mac', machineDbId: 'm1', machineId: 'mid' }; + const deps = (overrides = {}) => ({ + loadRecord: () => record, + serviceFile: '/home/x/Library/LaunchAgents/me.commonly.daemon.plist', + fileExists: () => true, + fetchMachine: async () => ({ name: 'Lab Mac', status: 'online', lastSeenAt: new Date(NOW - 15_000).toISOString() }), + describeError: (e) => `Daemon status failed: ${e.message}`, + now: NOW, + ...overrides, + }); + + test('passes when registered, installed and online', async () => { + expect(await checkDaemon(deps())).toMatchObject({ + status: PASS, detail: 'Lab Mac online, service installed, last heartbeat 15s ago', + }); + }); + + test('skips when no daemon is registered', async () => { + expect(await checkDaemon(deps({ loadRecord: () => null }))).toMatchObject({ status: SKIP }); + }); + + test('fails when the record is unreadable or insecure', async () => { + const r = await checkDaemon(deps({ loadRecord: () => { throw new Error('Daemon credential file permissions are insecure'); } })); + expect(r).toMatchObject({ status: FAIL }); + expect(r.detail).toContain('insecure'); + }); + + test('fails when the login service is not installed', async () => { + expect(await checkDaemon(deps({ fileExists: () => false }))).toMatchObject({ status: FAIL, fix: 'commonly daemon install' }); + }); + + test('fails when the server sees it offline, with the heartbeat age', async () => { + const r = await checkDaemon(deps({ + fetchMachine: async () => ({ name: 'Lab Mac', status: 'offline', lastSeenAt: new Date(NOW - 3 * 3600_000).toISOString() }), + })); + expect(r).toMatchObject({ status: FAIL, detail: 'Lab Mac is offline, last heartbeat 3h ago' }); + }); + + test('fails with the daemon status wording when the key is revoked', async () => { + const r = await checkDaemon(deps({ + fetchMachine: async () => { throw new Error('revoked'); }, + describeError: () => "This computer's old key was revoked. Run: commonly daemon register", + })); + expect(r).toMatchObject({ status: FAIL, detail: "This computer's old key was revoked. Run: commonly daemon register" }); + }); +}); + +describe('(e) adapters', () => { + const registry = { + claude: { detect: async () => ({ path: '/opt/homebrew/bin/claude', version: '2.5.1' }) }, + codex: { detect: async () => null }, + stub: {}, + }; + const getAdapter = (name) => registry[name] || null; + + test('one pass line per adapter in use, naming its agents', async () => { + const results = await checkAdapters({ agents: [{ name: 'otto', adapter: 'claude' }, { name: 'anvil', adapter: 'claude' }], getAdapter }); + expect(results).toEqual([{ + id: 'adapter:claude', status: PASS, detail: 'claude 2.5.1 at /opt/homebrew/bin/claude, used by anvil, otto', + }]); + }); + + test('fails an adapter whose CLI is missing or does not answer --version', async () => { + const [r] = await checkAdapters({ agents: [{ name: 'vera', adapter: 'codex' }], getAdapter }); + expect(r).toMatchObject({ id: 'adapter:codex', status: FAIL }); + }); + + test('fails an adapter whose detect throws', async () => { + const [r] = await checkAdapters({ + agents: [{ name: 'a', adapter: 'claude' }], + getAdapter: () => ({ detect: async () => { throw new Error('ENOENT'); } }), + }); + expect(r.status).toBe(FAIL); + }); + + test('fails an adapter name this CLI does not know', async () => { + const [r] = await checkAdapters({ agents: [{ name: 'x', adapter: 'gemini' }], getAdapter }); + expect(r).toMatchObject({ status: FAIL }); + expect(r.detail).toContain("unknown adapter 'gemini'"); + }); + + test('fails a token file that records no adapter', async () => { + const [r] = await checkAdapters({ agents: [{ name: 'old', adapter: '?' }], getAdapter }); + expect(r).toMatchObject({ id: 'adapter:?', status: FAIL }); + expect(r.detail).toContain('no adapter recorded'); + }); + + test('names at most five agents, then counts the rest', async () => { + const agents = ['a', 'b', 'c', 'd', 'e', 'f', 'g'].map((name) => ({ name, adapter: 'claude' })); + const [r] = await checkAdapters({ agents, getAdapter }); + expect(r.detail).toMatch(/used by a, b, c, d, e and 2 more$/); + }); + + test('skips an adapter with no local CLI, and skips when no agents are attached', async () => { + expect((await checkAdapters({ agents: [{ name: 's', adapter: 'stub' }], getAdapter }))[0].status).toBe(SKIP); + expect(await checkAdapters({ agents: [], getAdapter })).toEqual([expect.objectContaining({ status: SKIP })]); + }); +}); + +describe('(f) macOS claude prerequisites', () => { + const keychain = '/Users/x/Library/Keychains/login.keychain-db'; + const deps = (overrides = {}) => ({ + platform: 'darwin', + agents: [{ name: 'otto', adapter: 'claude' }], + detectSeatbelt: () => ({ available: true, path: '/usr/bin/sandbox-exec' }), + isFile: (p) => [keychain, '/usr/bin/security'].includes(p), + loginKeychainPath: keychain, + ...overrides, + }); + + test('passes when Seatbelt runs and both files exist', () => { + expect(checkMacClaude(deps())).toMatchObject({ status: PASS }); + }); + + test('fails naming every missing prerequisite', () => { + const r = checkMacClaude(deps({ + detectSeatbelt: () => ({ available: false, error: 'sandbox-exec preflight failed: exit 71' }), + isFile: () => false, + })); + expect(r.status).toBe(FAIL); + expect(r.detail).toContain('Seatbelt (sandbox-exec preflight failed: exit 71)'); + expect(r.detail).toContain(`login keychain (${keychain})`); + expect(r.detail).toContain('/usr/bin/security'); + }); + + test('fails when the Seatbelt probe throws', () => { + expect(checkMacClaude(deps({ detectSeatbelt: () => { throw new Error('spawn EPERM'); } })).status).toBe(FAIL); + }); + + test('skips off macOS and when no agent uses claude', () => { + expect(checkMacClaude(deps({ platform: 'linux' }))).toMatchObject({ status: SKIP, detail: 'not macOS' }); + expect(checkMacClaude(deps({ agents: [{ name: 'v', adapter: 'codex' }] }))).toMatchObject({ status: SKIP }); + }); + + test('only stats files: it is handed no way to run security', () => { + const touched = []; + checkMacClaude(deps({ isFile: (p) => { touched.push(p); return true; } })); + expect(touched).toEqual([keychain, '/usr/bin/security']); + }); +}); + +describe('runDoctor', () => { + const healthy = () => ({ + listAgents: () => [{ name: 'otto', adapter: 'claude' }], + getAdapter: () => ({ detect: async () => ({ path: '/bin/claude', version: '2.5.1' }) }), + node: { nodeVersion: '22.0.0', enginesRange: '>=20.0.0' }, + install: { pathValue: '/bin', platform: 'darwin', isExecutable: () => true, runVersion: () => '0.1.210', runningVersion: '0.1.210' }, + login: { instance: { key: 'default', url: 'https://api.commonly.me', hasToken: true }, fetchUser: async () => ({ username: 'otto' }) }, + daemon: { + loadRecord: () => ({ machineName: 'Mac' }), + serviceFile: '/s', + fileExists: () => true, + fetchMachine: async () => ({ name: 'Mac', status: 'online', lastSeenAt: new Date(NOW).toISOString() }), + describeError: (e) => e.message, + now: NOW, + }, + macos: { platform: 'darwin', detectSeatbelt: () => ({ available: true }), isFile: () => true, loginKeychainPath: '/k' }, + }); + + test('all checks pass: one line each, in order, exit 0', async () => { + const results = await runDoctor(healthy()); + expect(results.map((r) => r.id)).toEqual(['node', 'install', 'login', 'daemon', 'adapter:claude', 'macos-claude']); + expect(results.every((r) => r.status === PASS)).toBe(true); + expect(doctorExitCode(results)).toBe(0); + }); + + test('any fail gives exit 1; skips alone do not', async () => { + const deps = healthy(); + deps.daemon.loadRecord = () => null; + expect(doctorExitCode(await runDoctor(deps))).toBe(0); + deps.login.instance = null; + expect(doctorExitCode(await runDoctor(deps))).toBe(1); + }); +}); + +test('formatDoctorLine shows the fix only on a fail', () => { + expect(formatDoctorLine({ id: 'login', status: FAIL, detail: 'not logged in', fix: 'commonly login' })) + .toBe('fail login not logged in — fix: commonly login'); + expect(formatDoctorLine({ id: 'node', status: PASS, detail: 'Node 22.0.0', fix: 'x' })) + .toBe('pass node Node 22.0.0'); +}); diff --git a/cli/__tests__/update-check.test.mjs b/cli/__tests__/update-check.test.mjs new file mode 100644 index 0000000000..fbb0bf68a6 --- /dev/null +++ b/cli/__tests__/update-check.test.mjs @@ -0,0 +1,204 @@ +import { + checkForCliUpdate, + fetchLatestVersion, + readCachedLatest, + startUpdateNotice, + updateNoticeLine, + versionsBehind, + UPDATE_CACHE_TTL_MS, + UPDATE_NOTICE_MIN_BEHIND, +} from '../src/lib/update-check.js'; + +// TASK-660 item 3: a stale CLI says so in one start-up line; everything else +// (current, offline, slow, opted out) says nothing and costs start-up nothing. +// Every registry here is a fake, and every cache is an in-memory map. + +const NOW = Date.parse('2026-10-31T12:00:00Z'); + +const registry = (version, calls = []) => async (url, init) => { + calls.push({ url, init }); + return { ok: true, json: async () => ({ name: '@commonlyai/cli', version }) }; +}; + +const memoryCache = (initial = {}) => { + const files = { ...initial }; + return { + files, + readText: (path) => (path in files ? files[path] : null), + writeText: (path, text) => { files[path] = text; }, + }; +}; + +const base = (overrides = {}) => { + const cache = memoryCache(); + return { + installedVersion: '0.1.165', + env: {}, + now: NOW, + cachePath: '/fake/.commonly/update-check.json', + readText: cache.readText, + writeText: cache.writeText, + ...overrides, + }; +}; + +describe('versionsBehind', () => { + test('counts the patch gap within one release line', () => { + expect(versionsBehind('0.1.165', '0.1.168')).toBe(3); + expect(versionsBehind('0.1.168', '0.1.168')).toBe(0); + }); + + test('a newer minor or major is behind by an unknown, large distance', () => { + expect(versionsBehind('0.1.168', '0.2.0')).toBe(Infinity); + expect(versionsBehind('0.1.168', '1.0.0')).toBe(Infinity); + }); + + test('ahead of the registry is not behind', () => { + expect(versionsBehind('0.1.170', '0.1.168')).toBe(0); + expect(versionsBehind('0.2.0', '0.1.168')).toBe(0); + }); + + test('a non-version on either side is no answer', () => { + expect(versionsBehind('dev', '0.1.168')).toBeNull(); + expect(versionsBehind('0.1.168', undefined)).toBeNull(); + }); +}); + +describe('updateNoticeLine', () => { + test('names the gap, both versions, the update command and the opt-out', () => { + const line = updateNoticeLine({ installed: '0.1.165', latest: '0.1.168' }); + expect(line).toBe( + 'your CLI is 3 versions behind (installed 0.1.165, latest 0.1.168). ' + + 'Update: npm install -g @commonlyai/cli@latest (silence with COMMONLY_NO_UPDATE_CHECK=1)', + ); + }); + + test('stays quiet below the threshold', () => { + expect(UPDATE_NOTICE_MIN_BEHIND).toBe(3); + expect(updateNoticeLine({ installed: '0.1.166', latest: '0.1.168' })).toBeNull(); + }); + + test('prints only the parsed triple of what the registry sent', () => { + const line = updateNoticeLine({ installed: '0.1.100', latest: '0.1.168\u001b[2J; rm -rf ~' }); + expect(line).toContain('latest 0.1.168)'); + expect(line).not.toContain('rm -rf'); + expect(line).not.toContain('\u001b'); + }); +}); + +describe('checkForCliUpdate', () => { + test('a registry 3 versions ahead gives the notice, and the answer is cached', async () => { + const calls = []; + const opts = base({ fetchImpl: registry('0.1.168', calls) }); + const line = await checkForCliUpdate(opts); + expect(line).toMatch(/^your CLI is 3 versions behind \(installed 0\.1\.165, latest 0\.1\.168\)/); + expect(calls).toHaveLength(1); + expect(calls[0].url).toBe('https://registry.npmjs.org/@commonlyai%2fcli/latest'); + expect(JSON.parse(opts.readText(opts.cachePath))).toEqual({ + checkedAt: new Date(NOW).toISOString(), latest: '0.1.168', + }); + }); + + test('an equal version gives none', async () => { + expect(await checkForCliUpdate(base({ installedVersion: '0.1.168', fetchImpl: registry('0.1.168') }))).toBeNull(); + }); + + test('a registry timeout gives none, inside the timeout, even when fetch ignores its abort signal', async () => { + const hanging = () => new Promise(() => {}); + const started = Date.now(); + const line = await checkForCliUpdate(base({ fetchImpl: hanging, timeoutMs: 50 })); + expect(line).toBeNull(); + expect(Date.now() - started).toBeLessThan(1000); + }); + + test('offline (fetch rejects) gives none and writes no cache', async () => { + const opts = base({ fetchImpl: async () => { throw new TypeError('fetch failed'); } }); + expect(await checkForCliUpdate(opts)).toBeNull(); + expect(opts.readText(opts.cachePath)).toBeNull(); + }); + + test('a registry error status or a body without a version gives none', async () => { + expect(await checkForCliUpdate(base({ fetchImpl: async () => ({ ok: false, json: async () => ({}) }) }))).toBeNull(); + expect(await checkForCliUpdate(base({ fetchImpl: registry(undefined) }))).toBeNull(); + }); + + test('COMMONLY_NO_UPDATE_CHECK=1 never asks the registry', async () => { + const calls = []; + const line = await checkForCliUpdate(base({ env: { COMMONLY_NO_UPDATE_CHECK: '1' }, fetchImpl: registry('0.1.200', calls) })); + expect(line).toBeNull(); + expect(calls).toHaveLength(0); + }); + + test('a cache younger than 24 h is used without a lookup', async () => { + const cache = memoryCache({ + '/fake/.commonly/update-check.json': JSON.stringify({ checkedAt: new Date(NOW - 60_000).toISOString(), latest: '0.1.170' }), + }); + const calls = []; + const line = await checkForCliUpdate(base({ ...cache, fetchImpl: registry('0.1.999', calls) })); + expect(calls).toHaveLength(0); + expect(line).toContain('5 versions behind'); + }); + + test('a cache 24 h old or older is looked up again', async () => { + const cache = memoryCache({ + '/fake/.commonly/update-check.json': JSON.stringify({ checkedAt: new Date(NOW - UPDATE_CACHE_TTL_MS).toISOString(), latest: '0.1.165' }), + }); + const calls = []; + const line = await checkForCliUpdate(base({ ...cache, fetchImpl: registry('0.1.168', calls) })); + expect(calls).toHaveLength(1); + expect(line).toContain('3 versions behind'); + }); + + test('a malformed or future-dated cache is ignored', () => { + const path = '/c'; + expect(readCachedLatest({ cachePath: path, now: NOW, readText: () => 'not json' })).toBeNull(); + expect(readCachedLatest({ + cachePath: path, now: NOW, readText: () => JSON.stringify({ checkedAt: new Date(NOW + 60_000).toISOString(), latest: '0.1.1' }), + })).toBeNull(); + expect(readCachedLatest({ + cachePath: path, now: NOW, readText: () => JSON.stringify({ checkedAt: new Date(NOW).toISOString(), latest: '../x' }), + })).toBeNull(); + }); + + test('an unwritable cache still returns the notice', async () => { + const line = await checkForCliUpdate(base({ + fetchImpl: registry('0.1.168'), + writeText: () => { throw new Error('EACCES'); }, + })); + expect(line).toContain('3 versions behind'); + }); +}); + +describe('startUpdateNotice', () => { + test('returns before a hung registry answers, and logs nothing', async () => { + const logged = []; + const started = Date.now(); + const pending = startUpdateNotice({ ...base({ fetchImpl: () => new Promise(() => {}), timeoutMs: 50 }), log: (l) => logged.push(l) }); + // Start-up continues at once: the call returned a promise without waiting on it. + expect(Date.now() - started).toBeLessThan(25); + expect(logged).toEqual([]); + await expect(pending).resolves.toBeNull(); + expect(logged).toEqual([]); + }); + + test('logs the one line when stale', async () => { + const logged = []; + await startUpdateNotice({ ...base({ fetchImpl: registry('0.1.168') }), log: (l) => logged.push(l) }); + expect(logged).toHaveLength(1); + expect(logged[0]).toMatch(/^your CLI is 3 versions behind/); + }); + + test('a throwing log sink does not reject', async () => { + await expect(startUpdateNotice({ + ...base({ fetchImpl: registry('0.1.168') }), + log: () => { throw new Error('EPIPE'); }, + })).resolves.toContain('3 versions behind'); + }); +}); + +test('fetchLatestVersion sends an abort signal, and returns null without fetch', async () => { + const calls = []; + expect(await fetchLatestVersion({ fetchImpl: registry('0.1.168', calls), timeoutMs: 1000 })).toBe('0.1.168'); + expect(calls[0].init.signal).toBeInstanceOf(AbortSignal); + expect(await fetchLatestVersion({ fetchImpl: null })).toBeNull(); +}); diff --git a/cli/jest.setup.cjs b/cli/jest.setup.cjs index 8a41f3a58f..3f6c4df3e6 100644 --- a/cli/jest.setup.cjs +++ b/cli/jest.setup.cjs @@ -54,3 +54,8 @@ const mcpHome = originalMkdtempSync(join(tmpdir(), 'commonly-mcp-home-jest-')); process.env.COMMONLY_MCP_HOME = mcpHome; globalThis.__commonlyMcpHomeForCleanup = mcpHome; fs.writeFileSync(join(mcpHome, '.warm.lock'), 'warming\n'); + +// A test that reaches `agent run` or `daemon run` start-up must not ask the +// real npm registry whether this CLI is stale (TASK-660). The update-check +// suite passes its own env, so it is unaffected. +process.env.COMMONLY_NO_UPDATE_CHECK = '1'; diff --git a/cli/src/commands/agent.js b/cli/src/commands/agent.js index 8ea421e9e2..6bf66979c9 100644 --- a/cli/src/commands/agent.js +++ b/cli/src/commands/agent.js @@ -88,6 +88,7 @@ import { detectBwrap } from '../lib/sandbox/bwrap.js'; import { resolvePublicSandboxMode } from '../lib/sandbox/mode.js'; import { hostSandboxRefusal, windowsSandboxMessage } from '../lib/sandbox/host-check.js'; import { prepareMcpBeforeFirstPoll } from '../lib/mcp-first-run.js'; +import { installedCliVersion, startUpdateNotice } from '../lib/update-check.js'; import { createSeatStop } from '../lib/seat-stop.js'; import { detectSeatbelt } from '../lib/sandbox/seatbelt.js'; import { describeUpstreamRefusal } from '../lib/upstream-refusal.js'; @@ -4100,6 +4101,12 @@ Docs: console.log(`${stamp()} [${name}] polling ${record.instanceUrl} for events (ctrl+c to stop)`); console.log(`${stamp()} [${name}] ${foregroundDaemonHint({ daemonRegistered: hasDaemonRecord() })}`); + // TASK-660: one line when this CLI is stale. Fired, not awaited, so a + // slow registry never delays the first poll. + startUpdateNotice({ + installedVersion: installedCliVersion(), + log: (line) => console.log(`${stamp()} [${name}] ${line}`), + }); const { stop } = performRun({ instanceUrl: record.instanceUrl, diff --git a/cli/src/commands/daemon.js b/cli/src/commands/daemon.js index b04a4225b7..694e92b7dc 100644 --- a/cli/src/commands/daemon.js +++ b/cli/src/commands/daemon.js @@ -38,6 +38,7 @@ import { } from '../lib/daemon-service.js'; import { daemonLogsDir, daemonSeatLogPath, readLogTail } from '../lib/daemon-logs.js'; import { loadDaemonState, saveDaemonState } from '../lib/daemon-state.js'; +import { installedCliVersion, startUpdateNotice } from '../lib/update-check.js'; // One supervised seat, as `commonly daemon status --verbose` names it. Two of these // fields are worth reading closely: the seat's `instanceId` (the identity the @@ -451,6 +452,9 @@ Examples: for (const name of missingProviderKeys({ names: providerKeyEnvNames() })) { stampLog(`WARNING: ${name} is not set in this environment — a seat whose adapter needs it will fail to start. Export it and re-run: commonly daemon install (docs/agents/daemon-service-environment.md)`); } + // TASK-660: the daemon outlives many publishes, so say once at start-up + // when it is stale. Not awaited: supervision never waits on the registry. + startUpdateNotice({ installedVersion: installedCliVersion(), log: stampLog }); let pollTimer; let heartbeatTimer; diff --git a/cli/src/commands/doctor.js b/cli/src/commands/doctor.js new file mode 100644 index 0000000000..5dab360ae6 --- /dev/null +++ b/cli/src/commands/doctor.js @@ -0,0 +1,106 @@ +/** + * commonly doctor — one line per check, exit 1 when any fails (TASK-660). + * The checks live in ../lib/doctor.js; this file supplies the real host. + */ +import { accessSync, constants, existsSync, readFileSync, statSync } from 'fs'; +import { spawnSync } from 'child_process'; +import { homedir } from 'os'; +import { join } from 'path'; +import { createClient } from '../lib/api.js'; +import { getToken, resolveInstance } from '../lib/config.js'; +import { loadDaemonRecord } from '../lib/daemon-store.js'; +import { servicePaths } from '../lib/daemon-service.js'; +import { getAdapter } from '../lib/adapters/index.js'; +import { detectSeatbelt } from '../lib/sandbox/seatbelt.js'; +import { daemonStatusErrorMessage, getDaemonMachineStatus } from './daemon.js'; +import { listLocalAgents } from './agent.js'; +import { doctorExitCode, formatDoctorLine, runDoctor } from '../lib/doctor.js'; + +const REQUEST_TIMEOUT_MS = 5000; + +// Every server read doctor makes is bounded, so a hung instance is a fail line, not a hang. +const timedClient = (client) => ({ + ...client, + get: (path, params = {}, options = {}) => client.get(path, params, { signal: AbortSignal.timeout(REQUEST_TIMEOUT_MS), ...options }), +}); + +const pkg = JSON.parse(readFileSync(new URL('../../package.json', import.meta.url), 'utf8')); + +const isExecutable = (path) => { + try { + if (!statSync(path).isFile()) return false; + accessSync(path, constants.X_OK); + return true; + } catch { + return false; + } +}; + +const isFile = (path) => { + try { + return statSync(path).isFile(); + } catch { + return false; + } +}; + +const runVersion = (binary) => { + const res = spawnSync(binary, ['--version'], { encoding: 'utf8', timeout: REQUEST_TIMEOUT_MS }); + if (res.error || res.status !== 0) return null; + return (res.stdout || '').trim().split(/\s+/)[0] || null; +}; + +export const hostDoctorDeps = ({ instance: instanceArg = null } = {}) => { + const saved = resolveInstance(instanceArg); + const tokenKey = saved?.key || saved?.url || instanceArg; + return { + listAgents: () => listLocalAgents(), + getAdapter, + node: { nodeVersion: process.versions.node, enginesRange: pkg.engines?.node }, + install: { + pathValue: process.env.PATH, + platform: process.platform, + isExecutable, + runVersion, + runningVersion: pkg.version, + }, + login: { + instance: saved ? { key: saved.key, url: saved.url, hasToken: Boolean(getToken(tokenKey)) } : null, + fetchUser: () => timedClient(createClient({ instance: tokenKey })).get('/api/auth/user'), + }, + daemon: { + loadRecord: loadDaemonRecord, + serviceFile: servicePaths().file, + fileExists: existsSync, + fetchMachine: (record) => getDaemonMachineStatus({ + client: timedClient(createClient({ instance: record.instanceUrl, token: record.daemonToken })), + record, + }), + describeError: daemonStatusErrorMessage, + now: Date.now(), + }, + macos: { + platform: process.platform, + detectSeatbelt, + isFile, + loginKeychainPath: join(homedir(), 'Library', 'Keychains', 'login.keychain-db'), + }, + }; +}; + +export const registerDoctor = (program) => { + program + .command('doctor') + .description('Check this computer: Node, the install, login, the daemon, adapter CLIs, macOS prerequisites') + .option('--instance ', 'Saved instance to check the login of (default: the active one)') + .action(async (opts) => { + try { + const results = await runDoctor(hostDoctorDeps({ instance: opts.instance || null })); + for (const line of results.map(formatDoctorLine)) console.log(line); + process.exitCode = doctorExitCode(results); + } catch (error) { + console.error(`commonly doctor could not finish: ${error.message}`); + process.exitCode = 1; + } + }); +}; diff --git a/cli/src/index.js b/cli/src/index.js index 2f719bcc2d..59b2762b81 100755 --- a/cli/src/index.js +++ b/cli/src/index.js @@ -21,6 +21,7 @@ import { registerDaemon } from './commands/daemon.js'; import { registerPod } from './commands/pod.js'; import { registerDev } from './commands/dev.js'; import { registerVerify } from './commands/verify.js'; +import { registerDoctor } from './commands/doctor.js'; const __dirname = dirname(fileURLToPath(import.meta.url)); const pkg = JSON.parse(readFileSync(join(__dirname, '../package.json'), 'utf8')); @@ -54,6 +55,9 @@ registerPod(program); registerDev(program); registerVerify(program); +// Install health: one line per check (TASK-660) +registerDoctor(program); + // Each subcommand owns its own `--instance ` flag; a program-level // duplicate shadowed the subcommand value on commander v12, causing // `commonly login --instance …` to silently fall back to the default URL. @@ -66,6 +70,7 @@ Quick start: $ commonly agent detach my-claude # clean uninstall $ commonly daemon register --name "My MacBook" $ commonly daemon status + $ commonly doctor # check Node, install, login, daemon, adapters $ commonly inbox list --token-file ~/.commonly/bin/ops-token # an operator's own queue Custom Python agent: diff --git a/cli/src/lib/doctor.js b/cli/src/lib/doctor.js new file mode 100644 index 0000000000..b7b94f2dac --- /dev/null +++ b/cli/src/lib/doctor.js @@ -0,0 +1,221 @@ +/** + * `commonly doctor` — the checks (TASK-660, D213). + * + * D213 keeps the Node daemon and hardens install by making breakage visible. + * Each check here answers one question a broken seat used to leave to a log + * autopsy, prints one line (pass, fail or skip) and, when it fails, the command + * that fixes it. Every outside fact (PATH, the saved login, the server, the + * daemon record, the adapters, the macOS files) comes in through `deps`, so the + * tests fake the environment and never touch this host. + * + * What a check must never do: print a token, or touch the keychain. The macOS + * check only stats the login keychain file and `/usr/bin/security`; it never + * runs `security`, so it cannot prompt, unlock or read a secret. + */ +import { delimiter, join } from 'node:path'; +import { parseVersion, versionOlderThan } from './mcp-server-version.js'; + +export const PASS = 'pass'; +export const FAIL = 'fail'; +export const SKIP = 'skip'; + +const result = (id, status, detail, fix = null) => ({ id, status, detail, ...(fix ? { fix } : {}) }); + +/** `>=20`, `>=20.1` or `>=20.1.2` as a triple; null for any other range shape. */ +export const enginesFloor = (range) => { + const match = /^>=\s*(\d+)(?:\.(\d+))?(?:\.(\d+))?\s*$/.exec(String(range || '')); + return match ? [Number(match[1]), Number(match[2] || 0), Number(match[3] || 0)] : null; +}; + +// (a) Node against the package's engines floor. +export const checkNode = ({ nodeVersion, enginesRange }) => { + const floor = enginesFloor(enginesRange); + if (!floor) return result('node', SKIP, `Node ${nodeVersion}; engines range '${enginesRange || 'none'}' is not a >= floor`); + const version = parseVersion(nodeVersion); + if (!version) return result('node', FAIL, `could not read the Node version '${nodeVersion}'`, `install Node ${enginesRange}`); + if (versionOlderThan(version, floor)) { + return result('node', FAIL, `Node ${nodeVersion} is older than the CLI needs (${enginesRange})`, `install Node ${floor[0]} or newer`); + } + return result('node', PASS, `Node ${nodeVersion} (needs ${enginesRange})`); +}; + +/** Every `commonly` on PATH, first one first (the one a shell runs). */ +export const findOnPath = ({ command, pathValue, platform, isExecutable }) => { + const names = platform === 'win32' ? [`${command}.cmd`, `${command}.exe`, command] : [command]; + const found = []; + for (const dir of String(pathValue || '').split(platform === 'win32' ? ';' : delimiter)) { + if (!dir) continue; + for (const name of names) { + const candidate = join(dir, name); + if (isExecutable(candidate) && !found.includes(candidate)) found.push(candidate); + } + } + return found; +}; + +// (b) Which `commonly` a shell runs, and its version. +export const checkInstall = ({ + pathValue, platform, isExecutable, runVersion, runningVersion, +}) => { + const fix = 'npm install -g @commonlyai/cli'; + const found = findOnPath({ command: 'commonly', pathValue, platform, isExecutable }); + if (!found.length) return result('install', FAIL, 'no `commonly` on PATH', fix); + const [binary] = found; + const version = runVersion(binary); + if (!parseVersion(version)) return result('install', FAIL, `${binary} did not answer --version`, `reinstall: ${fix}`); + const notes = []; + if (runningVersion && parseVersion(runningVersion) && version !== runningVersion) { + notes.push(`this doctor is ${runningVersion}`); + } + if (found.length > 1) notes.push(`${found.length - 1} more on PATH`); + return result('install', PASS, `commonly ${version} at ${binary}${notes.length ? ` (${notes.join('; ')})` : ''}`); +}; + +// (c) The saved instance and whose token it holds. The token is sent, never shown. +export const checkLogin = async ({ instance, fetchUser }) => { + if (!instance) return result('login', FAIL, 'not logged in', 'commonly login'); + const where = `${instance.key || 'unsaved'} (${instance.url})`; + const relogin = `commonly login --instance ${instance.key || instance.url}`; + if (!instance.hasToken) return result('login', FAIL, `${where} has no saved token`, relogin); + let user; + try { + user = await fetchUser(instance); + } catch (error) { + if ([401, 403].includes(error?.status)) { + return result('login', FAIL, `${where} rejected the saved token`, relogin); + } + return result('login', FAIL, `could not reach ${instance.url}: ${error?.message || error}`, 'check the network, then re-run commonly doctor'); + } + const who = user?.username || user?.email || user?._id; + if (!who) return result('login', FAIL, `${where} answered without a user`, relogin); + return result('login', PASS, `@${who} on ${where}`); +}; + +const ago = (iso, now) => { + const at = Date.parse(iso); + if (!Number.isFinite(at)) return 'never'; + const seconds = Math.max(0, Math.round((now - at) / 1000)); + if (seconds < 120) return `${seconds}s ago`; + if (seconds < 7200) return `${Math.round(seconds / 60)}m ago`; + return `${Math.round(seconds / 3600)}h ago`; +}; + +// (d) The daemon: registered, service installed, heard from lately. +export const checkDaemon = async ({ + loadRecord, serviceFile, fileExists, fetchMachine, describeError, now, +}) => { + let record; + try { + record = loadRecord(); + } catch (error) { + return result('daemon', FAIL, error?.message || String(error), 'fix the file permissions named above, then re-run'); + } + if (!record) { + return result('daemon', SKIP, 'no daemon registered on this computer (optional; agents then run in the foreground)'); + } + const name = record.machineName || 'this computer'; + if (!fileExists(serviceFile)) { + return result('daemon', FAIL, `${name} is registered but its login service is not installed (${serviceFile})`, 'commonly daemon install'); + } + let machine; + try { + machine = await fetchMachine(record); + } catch (error) { + return result('daemon', FAIL, describeError(error), 'commonly daemon status'); + } + const heard = `last heartbeat ${ago(machine?.lastSeenAt, now)}`; + if (machine?.status !== 'online') { + return result('daemon', FAIL, `${machine?.name || name} is ${machine?.status || 'unknown'}, ${heard}`, 'commonly daemon restart, then commonly daemon logs'); + } + return result('daemon', PASS, `${machine.name || name} online, service installed, ${heard}`); +}; + +const MAX_NAMED_AGENTS = 5; + +// (e) Every adapter CLI a configured agent uses, found and answering --version. +export const checkAdapters = async ({ agents, getAdapter }) => { + const byAdapter = new Map(); + for (const agent of agents) { + const adapter = agent?.adapter || '?'; + if (!byAdapter.has(adapter)) byAdapter.set(adapter, []); + byAdapter.get(adapter).push(agent.name); + } + if (!byAdapter.size) return [result('adapters', SKIP, 'no agents are attached on this computer')]; + const results = []; + for (const [name, agentNames] of [...byAdapter.entries()].sort(([a], [b]) => a.localeCompare(b))) { + const id = `adapter:${name}`; + const sorted = agentNames.sort(); + const shown = sorted.slice(0, MAX_NAMED_AGENTS).join(', '); + const used = `used by ${shown}${sorted.length > MAX_NAMED_AGENTS ? ` and ${sorted.length - MAX_NAMED_AGENTS} more` : ''}`; + if (name === '?') { + results.push(result(id, FAIL, `no adapter recorded in the token file, ${used}`, 'commonly agent detach --force, then attach it again')); + continue; + } + const adapter = getAdapter(name); + if (!adapter) { + results.push(result(id, FAIL, `unknown adapter '${name}', ${used}`, `commonly agent detach --force, then attach it again`)); + continue; + } + if (typeof adapter.detect !== 'function') { + results.push(result(id, SKIP, `${name} runs no local CLI, ${used}`)); + continue; + } + let found = null; + try { + // eslint-disable-next-line no-await-in-loop + found = await adapter.detect(); + } catch { + found = null; + } + if (!found) { + results.push(result(id, FAIL, `${name} is not on PATH or its --version failed, ${used}`, `install the ${name} CLI, or put it on PATH`)); + continue; + } + const at = found.path ? ` at ${found.path}` : ''; + results.push(result(id, PASS, `${name} ${found.version || 'unknown version'}${at}, ${used}`)); + } + return results; +}; + +// (f) macOS: what the claude adapter's confined spawn needs, by file stat only. +export const checkMacClaude = ({ + platform, agents, detectSeatbelt, isFile, loginKeychainPath, securityPath = '/usr/bin/security', +}) => { + if (platform !== 'darwin') return result('macos-claude', SKIP, 'not macOS'); + if (!agents.some((agent) => agent?.adapter === 'claude')) { + return result('macos-claude', SKIP, 'no agent here uses the claude adapter'); + } + const missing = []; + let seatbelt; + try { + seatbelt = detectSeatbelt(); + } catch (error) { + seatbelt = { available: false, error: error?.message || String(error) }; + } + if (!seatbelt?.available) missing.push(`Seatbelt (${seatbelt?.error || 'sandbox-exec unavailable'})`); + if (!isFile(loginKeychainPath)) missing.push(`login keychain (${loginKeychainPath})`); + if (!isFile(securityPath)) missing.push(`${securityPath}`); + if (missing.length) { + return result('macos-claude', FAIL, `missing: ${missing.join('; ')}`, 'sign in to this Mac as the user the agent runs as, and run claude once to log in'); + } + return result('macos-claude', PASS, 'Seatbelt runs, the login keychain and /usr/bin/security are present'); +}; + +/** Every check, in the order the lines print. */ +export const runDoctor = async (deps) => { + const agents = deps.listAgents(); + return [ + checkNode(deps.node), + checkInstall(deps.install), + await checkLogin(deps.login), + await checkDaemon(deps.daemon), + ...(await checkAdapters({ agents, getAdapter: deps.getAdapter })), + checkMacClaude({ ...deps.macos, agents }), + ]; +}; + +export const formatDoctorLine = ({ id, status, detail, fix }) => ( + `${status.padEnd(4)} ${id.padEnd(16)} ${detail}${status === FAIL && fix ? ` — fix: ${fix}` : ''}` +); + +export const doctorExitCode = (results) => (results.some((r) => r.status === FAIL) ? 1 : 0); diff --git a/cli/src/lib/update-check.js b/cli/src/lib/update-check.js new file mode 100644 index 0000000000..557ac6961d --- /dev/null +++ b/cli/src/lib/update-check.js @@ -0,0 +1,180 @@ +/** + * "Your CLI is N versions behind" — one line at `agent run` and `daemon run` + * start-up (TASK-660, D213). + * + * A seat pinned to an old CLI fails in ways that look like platform bugs, and + * nothing on the seat said it was old. This prints one line when the installed + * CLI trails npm latest by UPDATE_NOTICE_MIN_BEHIND or more, and is otherwise + * silent: offline, a slow registry, a malformed answer and + * COMMONLY_NO_UPDATE_CHECK=1 all print nothing. + * + * It must never slow start-up. `startUpdateNotice` returns at once; the lookup + * runs beside the poll loop, is capped at UPDATE_TIMEOUT_MS by its own timer + * (a fetch that ignores its abort signal still cannot hold it), and is cached + * for UPDATE_CACHE_TTL_MS so a restarting seat asks the registry once a day. + */ +import { existsSync, mkdirSync, readFileSync, writeFileSync } from 'node:fs'; +import { homedir } from 'node:os'; +import { dirname, join } from 'node:path'; +import { exactVersion, parseVersion } from './mcp-server-version.js'; + +export const CLI_PACKAGE = '@commonlyai/cli'; +export const REGISTRY_LATEST_URL = 'https://registry.npmjs.org/@commonlyai%2fcli/latest'; +export const UPDATE_CHECK_OPT_OUT_ENV = 'COMMONLY_NO_UPDATE_CHECK'; +export const UPDATE_CACHE_TTL_MS = 24 * 60 * 60 * 1000; +export const UPDATE_TIMEOUT_MS = 2000; +// The CLI publishes often, so one or two releases behind is the normal gap +// between a publish and a seat's next restart. Three is where it is stale. +export const UPDATE_NOTICE_MIN_BEHIND = 3; + +export const updateCachePath = (home = homedir()) => join(home, '.commonly', 'update-check.json'); + +/** + * How far `installed` trails `latest`: 0 when equal or ahead, the patch gap + * within one major.minor, and Infinity when latest is a newer minor or major + * (the patch numbers of two release lines say nothing about the distance). + * null when either side is not a version. + */ +export const versionsBehind = (installed, latest) => { + const a = parseVersion(installed); + const b = parseVersion(latest); + if (!a || !b) return null; + if (a[0] !== b[0] || a[1] !== b[1]) { + return (b[0] > a[0] || (b[0] === a[0] && b[1] > a[1])) ? Infinity : 0; + } + return Math.max(0, b[2] - a[2]); +}; + +/** The one line to print, or null when there is nothing worth saying. */ +export const updateNoticeLine = ({ installed, latest, minBehind = UPDATE_NOTICE_MIN_BEHIND }) => { + const behind = versionsBehind(installed, latest); + if (behind === null || behind < minBehind) return null; + // Only the parsed triple is printed: the registry's string never reaches the log. + const from = exactVersion(installed); + const to = exactVersion(latest); + const gap = Number.isFinite(behind) ? `${behind} versions behind` : 'behind a newer release line'; + return `your CLI is ${gap} (installed ${from}, latest ${to}). Update: npm install -g ${CLI_PACKAGE}@latest` + + ` (silence with ${UPDATE_CHECK_OPT_OUT_ENV}=1)`; +}; + +const defaultReadText = (path) => (existsSync(path) ? readFileSync(path, 'utf8') : null); +const defaultWriteText = (path, text) => { + mkdirSync(dirname(path), { recursive: true }); + writeFileSync(path, text, { encoding: 'utf8', mode: 0o600 }); +}; + +/** The cached latest version when the cache is younger than the TTL, else null. */ +export const readCachedLatest = ({ cachePath, now, readText = defaultReadText, ttlMs = UPDATE_CACHE_TTL_MS }) => { + let raw; + try { + raw = readText(cachePath); + } catch { + return null; + } + if (!raw) return null; + try { + const cache = JSON.parse(raw); + const checkedAt = Date.parse(cache?.checkedAt); + const latest = exactVersion(cache?.latest); + if (!Number.isFinite(checkedAt) || !latest) return null; + if (now - checkedAt < 0 || now - checkedAt >= ttlMs) return null; + return latest; + } catch { + return null; + } +}; + +/** + * npm latest, or null on any failure. The timer is the cap, not the signal: + * the signal asks fetch to stop, the race guarantees the answer is on time. + */ +export const fetchLatestVersion = async ({ + fetchImpl = globalThis.fetch, + timeoutMs = UPDATE_TIMEOUT_MS, + url = REGISTRY_LATEST_URL, +} = {}) => { + if (typeof fetchImpl !== 'function') return null; + const controller = new AbortController(); + let timer; + const deadline = new Promise((resolve) => { + timer = setTimeout(() => { + controller.abort(); + resolve(null); + }, timeoutMs); + timer.unref?.(); + }); + const lookup = (async () => { + const res = await fetchImpl(url, { signal: controller.signal, headers: { accept: 'application/json' } }); + if (!res?.ok) return null; + const body = await res.json(); + return exactVersion(body?.version); + })().catch(() => null); + try { + return await Promise.race([lookup, deadline]); + } finally { + clearTimeout(timer); + } +}; + +/** The notice line for this start-up, or null. Never throws. */ +export const checkForCliUpdate = async ({ + installedVersion, + env = process.env, + now = Date.now(), + cachePath = updateCachePath(), + fetchImpl = globalThis.fetch, + timeoutMs = UPDATE_TIMEOUT_MS, + readText = defaultReadText, + writeText = defaultWriteText, + minBehind = UPDATE_NOTICE_MIN_BEHIND, +} = {}) => { + try { + if (env?.[UPDATE_CHECK_OPT_OUT_ENV] === '1') return null; + if (!parseVersion(installedVersion)) return null; + let latest = readCachedLatest({ cachePath, now, readText }); + if (!latest) { + latest = await fetchLatestVersion({ fetchImpl, timeoutMs }); + if (!latest) return null; + try { + writeText(cachePath, `${JSON.stringify({ checkedAt: new Date(now).toISOString(), latest })}\n`); + } catch { + // An unwritable cache costs one lookup per start, nothing more. + } + } + return updateNoticeLine({ installed: installedVersion, latest, minBehind }); + } catch { + return null; + } +}; + +/** + * Fire the check and return at once. The returned promise (for tests) settles + * to the line that was logged, or null. + */ +export const startUpdateNotice = ({ log, ...options }) => { + const pending = checkForCliUpdate(options).then((line) => { + if (line) { + try { + log(line); + } catch { + // A closed log sink must not surface as an unhandled rejection. + } + } + return line; + }); + return pending; +}; + +let cachedCliVersion; +/** This CLI's own version, from the package.json it shipped with. */ +export const installedCliVersion = () => { + if (cachedCliVersion === undefined) { + try { + const pkg = JSON.parse(readFileSync(new URL('../../package.json', import.meta.url), 'utf8')); + cachedCliVersion = typeof pkg?.version === 'string' ? pkg.version : null; + } catch { + cachedCliVersion = null; + } + } + return cachedCliVersion; +}; diff --git a/docs/cli/README.md b/docs/cli/README.md index 25c9e8ac37..9bde4a9968 100644 --- a/docs/cli/README.md +++ b/docs/cli/README.md @@ -1,3 +1,5 @@ +| `COMMONLY_NO_UPDATE_CHECK=1` | Silences the start-up "your CLI is N versions behind" line on `agent run` and `daemon run` ([doctor.md](doctor.md)). | +| `commonly doctor [--instance ]` | One line per check: Node, the install, login, the daemon, each adapter CLI in use, macOS claude prerequisites. Exit 1 on any fail. See [doctor.md](doctor.md). | # Commonly CLI The `commonly` CLI is the primary developer entry point to a Commonly instance. Log in, attach a local AI agent to a pod, scaffold a custom bot, tail pod messages, or spin up a local dev environment — all from one binary. @@ -355,6 +357,8 @@ commonly dev down # When done ## Troubleshooting +Start with `commonly doctor`: it names the failing piece and the command that fixes it. + ### `commonly agent run` exits with "Runtime token rejected 3 times in a row" The token was revoked (usually because the agent was uninstalled from the pod elsewhere). Run: diff --git a/docs/cli/doctor.md b/docs/cli/doctor.md new file mode 100644 index 0000000000..7a0c9215bd --- /dev/null +++ b/docs/cli/doctor.md @@ -0,0 +1,46 @@ +# `commonly doctor` and the stale-CLI notice + +TASK-660, from D213: the Node daemon stays, and install gets harder to break +silently by making breakage visible. Code: `cli/src/lib/doctor.js` (checks), +`cli/src/commands/doctor.js` (host wiring), `cli/src/lib/update-check.js` +(notice). Tests: `cli/__tests__/doctor.test.mjs`, `cli/__tests__/update-check.test.mjs`. + +## The checks + +`commonly doctor [--instance ]` prints one line per check: `pass`, +`fail` (with a `fix:` command) or `skip`. It exits 1 when any line fails, else 0. + +| Line | What it checks | Fails when | +|------|----------------|------------| +| `node` | Node against `engines.node` in the CLI's package.json | Node is below the floor | +| `install` | The first `commonly` on PATH and its `--version`; notes when the doctor you ran is a different version, or more copies are on PATH | none on PATH, or it does not answer `--version` | +| `login` | The saved instance (active, or `--instance`) and the user its token belongs to, via `GET /api/auth/user`. The token is sent, never printed. | not logged in, no token, token rejected, or the instance cannot be reached in 5 s | +| `daemon` | The daemon record, its launchd/systemd service file, and the server's view (`online`, last heartbeat) | the record is unreadable or insecure, the service is not installed, the key is revoked, or the machine is offline. **Skips** when no daemon is registered: foreground agents are a supported setup. | +| `adapter:` | One line per adapter that an agent in `~/.commonly/tokens/` uses: the adapter's own `detect()`, which runs ` --version` | the CLI is missing or fails `--version`, the adapter is unknown, or the token file records none | +| `macos-claude` | On macOS, when an agent uses the claude adapter: `sandbox-exec` runs, and `~/Library/Keychains/login.keychain-db` and `/usr/bin/security` exist | any of the three is missing | + +The macOS check only **stats** the two files. It never runs `security`, so it +cannot prompt, unlock or read a keychain item. + +## The stale-CLI notice + +`agent run` and `daemon run` print one line at start-up when the installed CLI +is 3 or more patch releases behind npm latest, or behind a newer minor or major: + +``` + [] your CLI is 42 versions behind (installed 0.1.168, latest 0.1.210). Update: npm install -g @commonlyai/cli@latest (silence with COMMONLY_NO_UPDATE_CHECK=1) +``` + +- **Never blocks start-up.** The check is fired, not awaited; the first poll + goes out before it answers. +- **2 s cap** on the registry lookup, enforced by its own timer. +- **24 h cache** in `~/.commonly/update-check.json`; only a successful lookup is cached. +- **Silent** when offline, on a registry error, or with `COMMONLY_NO_UPDATE_CHECK=1`. +- The threshold is `UPDATE_NOTICE_MIN_BEHIND` (3). One or two releases behind + is the normal gap between a publish and a seat's next restart. +- **Trap: a source checkout reads as stale.** `.github/workflows/npm-publish.yml` + stamps the version into its working copy only (`npm version + --no-git-tag-version`), so `cli/package.json` on main lags npm: 0.1.168 on + main against 0.1.210 published, 2026-10-31. Run from a checkout and you get + the notice, so set the opt-out. A seat on a pinned `~/.commonly/cli-/` + install reads its real published version. From 8642626c030b9d09ddc0e861e0a7ffb0dec2a975 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sat, 31 Oct 2026 05:54:02 -0700 Subject: [PATCH 2/6] =?UTF-8?q?fix(cli):=20doctor=20review=20fixes=20?= =?UTF-8?q?=E2=80=94=20login=20resolves=20like=20createClient,=20opt-out?= =?UTF-8?q?=20reaches=20the=20daemon=20service=20(TASK-660)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From the self-review of b1624ded4: - login: resolve instance and token the way createClient does, so a COMMONLY_TOKEN-only host passes and a rejected env token is named with the right fix; keep the user's --instance in the login hint. The resolution is a pure loginTarget() with its own tests. - daemon install carries COMMONLY_NO_UPDATE_CHECK into the plist/unit when it is set (optionalEnvNames, no missing-key warning), so the opt-out reaches service-started seats. - startUpdateNotice runs everything inside its promise chain and ends in a catch: no throw or unhandled rejection can reach start-up. - Windows: run a .cmd shim's --version through cmd.exe, quoted, and stop counting npm's bare sh shim as a second install. - adapter: a throwing detect() reports its error instead of "not on PATH". - macos-claude: the fix hint matches what is missing (a failed Seatbelt preflight usually means doctor is inside a sandbox). - every line is one line: control characters in server text are folded. - README: the two table rows that landed above the title move into the command and env-var tables. Co-Authored-By: Claude Opus 5.5 Co-authored-by: otto Agent-Model: claude/claude-opus-5-5/default --- cli/__tests__/daemon-service.test.mjs | 24 +++++++ cli/__tests__/doctor-host.test.mjs | 59 +++++++++++++++ cli/__tests__/doctor.test.mjs | 84 +++++++++++++++++----- cli/__tests__/update-check.test.mjs | 6 ++ cli/src/commands/daemon.js | 8 ++- cli/src/commands/doctor.js | 100 ++++++++++++++++---------- cli/src/lib/daemon-service.js | 6 +- cli/src/lib/doctor.js | 56 ++++++++++----- cli/src/lib/update-check.js | 17 +++-- docs/cli/README.md | 4 +- docs/cli/doctor.md | 11 ++- 11 files changed, 292 insertions(+), 83 deletions(-) create mode 100644 cli/__tests__/doctor-host.test.mjs diff --git a/cli/__tests__/daemon-service.test.mjs b/cli/__tests__/daemon-service.test.mjs index 5b3747d7aa..a8f4f2413f 100644 --- a/cli/__tests__/daemon-service.test.mjs +++ b/cli/__tests__/daemon-service.test.mjs @@ -188,6 +188,30 @@ describe('install', () => { expect(withoutKey.warn).toHaveBeenCalledWith(expect.stringContaining('COMMONLY_LITELLM_KEY is not set in this shell')); }); + test('install carries a set opt-out into the unit, and says nothing when it is absent (TASK-660)', async () => { + const set = makeDeps(); + set.warn = jest.fn(); + await installDaemonService({ + platform: 'linux', home, nodePath, cliPath, + optionalEnvNames: ['COMMONLY_NO_UPDATE_CHECK'], + env: { COMMONLY_NO_UPDATE_CHECK: '1' }, + ...set, + }); + expect(set.writeFile.mock.calls[0][1]).toContain('Environment="COMMONLY_NO_UPDATE_CHECK=1"'); + expect(set.warn).not.toHaveBeenCalled(); + + const absent = makeDeps(); + absent.warn = jest.fn(); + await installDaemonService({ + platform: 'darwin', home, nodePath, cliPath, + optionalEnvNames: ['COMMONLY_NO_UPDATE_CHECK'], + env: {}, + ...absent, + }); + expect(absent.writeFile.mock.calls[0][1]).not.toContain('COMMONLY_NO_UPDATE_CHECK'); + expect(absent.warn).not.toHaveBeenCalled(); + }); + // The mode is held by construction — a sibling file created 0600 and renamed // over the target — so the property to witness is the mode and the content, // not which call did it. Real file through the REAL default writer, with chmod diff --git a/cli/__tests__/doctor-host.test.mjs b/cli/__tests__/doctor-host.test.mjs new file mode 100644 index 0000000000..4a3fb9d626 --- /dev/null +++ b/cli/__tests__/doctor-host.test.mjs @@ -0,0 +1,59 @@ +import { loginTarget, versionCommand } from '../src/commands/doctor.js'; + +// The host wiring's two decisions, with config resolution faked. loginTarget +// must agree with createClient (api.js): getToken prefers COMMONLY_TOKEN, and +// resolveInstanceUrl falls back to the default URL when nothing is saved. + +const DEFAULT = 'https://api.commonly.me'; +const fakes = ({ saved = null, env = {} } = {}) => ({ + env, + resolve: () => saved, + resolveUrl: () => saved?.url || DEFAULT, + tokenFor: () => (env.COMMONLY_TOKEN || saved?.token || null), +}); + +describe('loginTarget', () => { + test('a saved login sends the saved token', () => { + const saved = { key: 'default', url: DEFAULT, token: 'cm_x' }; + expect(loginTarget(fakes({ saved }))).toEqual({ + key: 'default', url: DEFAULT, tokenSource: 'saved', loginHint: 'commonly login', + }); + }); + + test('COMMONLY_TOKEN with nothing saved is a usable login on the default URL, as it is for every command', () => { + expect(loginTarget(fakes({ env: { COMMONLY_TOKEN: 'cm_env' } }))).toMatchObject({ + key: null, url: DEFAULT, tokenSource: 'env', + }); + }); + + test('COMMONLY_TOKEN beside a saved login is the token sent, and is named as such', () => { + const saved = { key: 'default', url: DEFAULT, token: 'cm_saved' }; + expect(loginTarget(fakes({ saved, env: { COMMONLY_TOKEN: 'cm_env' } })).tokenSource).toBe('env'); + }); + + test('nothing saved and no env token is no token, and the hint keeps the --instance given', () => { + expect(loginTarget({ ...fakes(), instanceArg: 'foo' })).toMatchObject({ + tokenSource: null, loginHint: 'commonly login --instance foo', + }); + }); + + test('never carries the token itself', () => { + const saved = { key: 'default', url: DEFAULT, token: 'cm_secret' }; + expect(JSON.stringify(loginTarget(fakes({ saved, env: { COMMONLY_TOKEN: 'cm_env_secret' } })))).not.toMatch(/cm_/); + }); +}); + +describe('versionCommand', () => { + test('a Windows .cmd shim goes through the shell, quoted', () => { + expect(versionCommand('C:\\Program Files\\nodejs\\commonly.cmd', 'win32')).toEqual({ + command: '"C:\\Program Files\\nodejs\\commonly.cmd" --version', args: [], shell: true, + }); + }); + + test('everything else is spawned directly', () => { + expect(versionCommand('/opt/homebrew/bin/commonly', 'darwin')).toEqual({ + command: '/opt/homebrew/bin/commonly', args: ['--version'], shell: false, + }); + expect(versionCommand('C:\\npm\\commonly.exe', 'win32').shell).toBe(false); + }); +}); diff --git a/cli/__tests__/doctor.test.mjs b/cli/__tests__/doctor.test.mjs index 07b09cf802..8b977bd18c 100644 --- a/cli/__tests__/doctor.test.mjs +++ b/cli/__tests__/doctor.test.mjs @@ -73,40 +73,67 @@ describe('(b) install', () => { })).toMatchObject({ status: FAIL }); }); - test('looks for the .cmd shim on Windows', () => { + test('looks for the .cmd shim on Windows, and does not count npm\'s bare sh shim beside it', () => { expect(findOnPath({ command: 'commonly', pathValue: 'C:\\npm;C:\\other', platform: 'win32', isExecutable: (p) => p.endsWith('commonly.cmd'), })).toHaveLength(2); + expect(findOnPath({ + command: 'commonly', pathValue: 'C:\\npm', platform: 'win32', isExecutable: () => true, + })).toHaveLength(2); // commonly.cmd and commonly.exe, never the bare `commonly` }); }); describe('(c) login', () => { - const instance = { key: 'default', url: 'https://api.commonly.me', hasToken: true }; + const saved = { + key: 'default', url: 'https://api.commonly.me', tokenSource: 'saved', loginHint: 'commonly login', + }; test('passes with the token\'s user, and never prints the token', async () => { - const r = await checkLogin({ instance, fetchUser: async () => ({ username: 'lily-shen', token: 'cm_secret' }) }); + const r = await checkLogin({ target: saved, fetchUser: async () => ({ username: 'lily-shen', token: 'cm_secret' }) }); expect(r).toMatchObject({ status: PASS, detail: '@lily-shen on default (https://api.commonly.me)' }); expect(JSON.stringify(r)).not.toContain('cm_secret'); }); - test('fails when not logged in', async () => { - expect(await checkLogin({ instance: null, fetchUser: async () => ({}) })).toMatchObject({ status: FAIL, fix: 'commonly login' }); + test('says when the token it sent came from COMMONLY_TOKEN', async () => { + const r = await checkLogin({ target: { ...saved, tokenSource: 'env' }, fetchUser: async () => ({ username: 'ci' }) }); + expect(r.detail).toBe('@ci on default (https://api.commonly.me) (token from COMMONLY_TOKEN)'); }); - test('fails when the saved instance has no token, without calling the server', async () => { + test('fails when not logged in, without calling the server', async () => { let called = false; - const r = await checkLogin({ instance: { ...instance, hasToken: false }, fetchUser: async () => { called = true; } }); - expect(r).toMatchObject({ status: FAIL, fix: 'commonly login --instance default' }); + const r = await checkLogin({ + target: { key: null, url: 'https://api.commonly.me', tokenSource: null, loginHint: 'commonly login' }, + fetchUser: async () => { called = true; }, + }); + expect(r).toMatchObject({ status: FAIL, detail: 'not logged in', fix: 'commonly login' }); expect(called).toBe(false); }); - test('fails when the server rejects the token', async () => { - const r = await checkLogin({ instance, fetchUser: async () => { throw Object.assign(new Error('Session expired'), { status: 401 }); } }); - expect(r).toMatchObject({ status: FAIL, detail: 'default (https://api.commonly.me) rejected the saved token' }); + test('fails when a saved instance has no token, keeping the --instance the user gave', async () => { + const r = await checkLogin({ + target: { ...saved, key: 'dev', tokenSource: null, loginHint: 'commonly login --instance dev' }, + fetchUser: async () => ({}), + }); + expect(r).toMatchObject({ status: FAIL, fix: 'commonly login --instance dev' }); + expect(r.detail).toContain('has no saved token'); + }); + + test('fails when the server rejects the saved token', async () => { + const r = await checkLogin({ target: saved, fetchUser: async () => { throw Object.assign(new Error('Session expired'), { status: 401 }); } }); + expect(r).toMatchObject({ status: FAIL, detail: 'default (https://api.commonly.me) rejected the saved token', fix: 'commonly login' }); + }); + + test('a rejected COMMONLY_TOKEN is named, and the fix is the env var, not a login', async () => { + const r = await checkLogin({ + target: { ...saved, tokenSource: 'env' }, + fetchUser: async () => { throw Object.assign(new Error('nope'), { status: 401 }); }, + }); + expect(r).toMatchObject({ status: FAIL, fix: 'replace or unset COMMONLY_TOKEN' }); + expect(r.detail).toContain('the token in COMMONLY_TOKEN'); }); test('fails when the server cannot be reached', async () => { - const r = await checkLogin({ instance, fetchUser: async () => { throw new TypeError('fetch failed'); } }); + const r = await checkLogin({ target: saved, fetchUser: async () => { throw new TypeError('fetch failed'); } }); expect(r.status).toBe(FAIL); expect(r.detail).toContain('could not reach https://api.commonly.me'); }); @@ -180,12 +207,14 @@ describe('(e) adapters', () => { expect(r).toMatchObject({ id: 'adapter:codex', status: FAIL }); }); - test('fails an adapter whose detect throws', async () => { + test('fails an adapter whose detect throws, keeping the reason instead of calling it missing', async () => { const [r] = await checkAdapters({ - agents: [{ name: 'a', adapter: 'claude' }], - getAdapter: () => ({ detect: async () => { throw new Error('ENOENT'); } }), + agents: [{ name: 'a', adapter: 'dsh' }], + getAdapter: () => ({ detect: async () => { throw new Error('EACCES: mkdtemp'); } }), }); expect(r.status).toBe(FAIL); + expect(r.detail).toContain('could not be checked (EACCES: mkdtemp)'); + expect(r.detail).not.toContain('not on PATH'); }); test('fails an adapter name this CLI does not know', async () => { @@ -236,6 +265,18 @@ describe('(f) macOS claude prerequisites', () => { expect(r.detail).toContain('Seatbelt (sandbox-exec preflight failed: exit 71)'); expect(r.detail).toContain(`login keychain (${keychain})`); expect(r.detail).toContain('/usr/bin/security'); + expect(r.fix).toContain('normal Terminal'); + expect(r.fix).toContain('run claude once'); + }); + + test('a failed Seatbelt preflight alone points at running outside a sandbox', () => { + const r = checkMacClaude(deps({ detectSeatbelt: () => ({ available: false, error: 'exit 71' }) })); + expect(r.fix).toBe('run commonly doctor from a normal Terminal, not from inside an agent\'s sandbox'); + }); + + test('missing files alone point at signing in', () => { + const r = checkMacClaude(deps({ isFile: () => false })); + expect(r.fix).toBe('sign in to this Mac as the user the agent runs as, and run claude once to log in'); }); test('fails when the Seatbelt probe throws', () => { @@ -260,7 +301,10 @@ describe('runDoctor', () => { getAdapter: () => ({ detect: async () => ({ path: '/bin/claude', version: '2.5.1' }) }), node: { nodeVersion: '22.0.0', enginesRange: '>=20.0.0' }, install: { pathValue: '/bin', platform: 'darwin', isExecutable: () => true, runVersion: () => '0.1.210', runningVersion: '0.1.210' }, - login: { instance: { key: 'default', url: 'https://api.commonly.me', hasToken: true }, fetchUser: async () => ({ username: 'otto' }) }, + login: { + target: { key: 'default', url: 'https://api.commonly.me', tokenSource: 'saved', loginHint: 'commonly login' }, + fetchUser: async () => ({ username: 'otto' }), + }, daemon: { loadRecord: () => ({ machineName: 'Mac' }), serviceFile: '/s', @@ -283,11 +327,17 @@ describe('runDoctor', () => { const deps = healthy(); deps.daemon.loadRecord = () => null; expect(doctorExitCode(await runDoctor(deps))).toBe(0); - deps.login.instance = null; + deps.login.target = { ...deps.login.target, tokenSource: null }; expect(doctorExitCode(await runDoctor(deps))).toBe(1); }); }); +test('formatDoctorLine keeps a multi-line or ANSI server error on one line', () => { + const line = formatDoctorLine({ id: 'login', status: FAIL, detail: 'could not reach x: bad\n\u001b[31mgateway\u001b[0m', fix: 'retry' }); + expect(line).not.toMatch(/[\n\u001b]/); + expect(line).toContain('bad [31mgateway [0m'); +}); + test('formatDoctorLine shows the fix only on a fail', () => { expect(formatDoctorLine({ id: 'login', status: FAIL, detail: 'not logged in', fix: 'commonly login' })) .toBe('fail login not logged in — fix: commonly login'); diff --git a/cli/__tests__/update-check.test.mjs b/cli/__tests__/update-check.test.mjs index fbb0bf68a6..b3cdee40d9 100644 --- a/cli/__tests__/update-check.test.mjs +++ b/cli/__tests__/update-check.test.mjs @@ -188,6 +188,12 @@ describe('startUpdateNotice', () => { expect(logged[0]).toMatch(/^your CLI is 3 versions behind/); }); + test('a throw while reading its own arguments resolves null instead of rejecting', async () => { + const params = { log: () => {} }; + Object.defineProperty(params, 'installedVersion', { enumerable: true, get() { throw new Error('boom'); } }); + await expect(startUpdateNotice(params)).resolves.toBeNull(); + }); + test('a throwing log sink does not reject', async () => { await expect(startUpdateNotice({ ...base({ fetchImpl: registry('0.1.168') }), diff --git a/cli/src/commands/daemon.js b/cli/src/commands/daemon.js index 694e92b7dc..26ea1c0a97 100644 --- a/cli/src/commands/daemon.js +++ b/cli/src/commands/daemon.js @@ -38,7 +38,7 @@ import { } from '../lib/daemon-service.js'; import { daemonLogsDir, daemonSeatLogPath, readLogTail } from '../lib/daemon-logs.js'; import { loadDaemonState, saveDaemonState } from '../lib/daemon-state.js'; -import { installedCliVersion, startUpdateNotice } from '../lib/update-check.js'; +import { installedCliVersion, startUpdateNotice, UPDATE_CHECK_OPT_OUT_ENV } from '../lib/update-check.js'; // One supervised seat, as `commonly daemon status --verbose` names it. Two of these // fields are worth reading closely: the seat's `instanceId` (the identity the @@ -352,7 +352,11 @@ Examples: // adapter. The keys are captured here because this is the only moment // the operator's shell is in reach; the ones that are missing are named // by installDaemonService rather than left to be discovered per seat. - await installDaemonService({ ...serviceDeps(), providerKeyEnvNames: providerKeyEnvNames() }); + await installDaemonService({ + ...serviceDeps(), + providerKeyEnvNames: providerKeyEnvNames(), + optionalEnvNames: [UPDATE_CHECK_OPT_OUT_ENV], + }); console.log('The daemon now starts at login and is kept alive. Uninstall with: commonly daemon uninstall'); } catch (error) { console.error(`Daemon install failed: ${error.message}`); diff --git a/cli/src/commands/doctor.js b/cli/src/commands/doctor.js index 5dab360ae6..019a49855b 100644 --- a/cli/src/commands/doctor.js +++ b/cli/src/commands/doctor.js @@ -7,7 +7,7 @@ import { spawnSync } from 'child_process'; import { homedir } from 'os'; import { join } from 'path'; import { createClient } from '../lib/api.js'; -import { getToken, resolveInstance } from '../lib/config.js'; +import { getToken, resolveInstance, resolveInstanceUrl } from '../lib/config.js'; import { loadDaemonRecord } from '../lib/daemon-store.js'; import { servicePaths } from '../lib/daemon-service.js'; import { getAdapter } from '../lib/adapters/index.js'; @@ -44,50 +44,78 @@ const isFile = (path) => { } }; +// Node refuses to spawn a .cmd without a shell (CVE-2024-27980), and npm's +// Windows shim is a .cmd, so that one case goes through cmd.exe, quoted. +export const versionCommand = (binary, platform = process.platform) => ( + platform === 'win32' && /\.(cmd|bat)$/i.test(binary) + ? { command: `"${binary}" --version`, args: [], shell: true } + : { command: binary, args: ['--version'], shell: false } +); + const runVersion = (binary) => { - const res = spawnSync(binary, ['--version'], { encoding: 'utf8', timeout: REQUEST_TIMEOUT_MS }); + const { command, args, shell } = versionCommand(binary); + const res = spawnSync(command, args, { encoding: 'utf8', timeout: REQUEST_TIMEOUT_MS, shell }); if (res.error || res.status !== 0) return null; return (res.stdout || '').trim().split(/\s+/)[0] || null; }; -export const hostDoctorDeps = ({ instance: instanceArg = null } = {}) => { - const saved = resolveInstance(instanceArg); - const tokenKey = saved?.key || saved?.url || instanceArg; +/** + * The instance and token every other command would use for `instanceArg`: + * createClient's own resolution (api.js), so doctor cannot pass a login that + * `agent attach` then fails, or fail one that works. COMMONLY_TOKEN wins over + * the saved token there, so it wins here, and the line says which one it sent. + */ +export const loginTarget = ({ + instanceArg = null, + env = process.env, + resolve = resolveInstance, + resolveUrl = resolveInstanceUrl, + tokenFor = getToken, +} = {}) => { + const saved = resolve(instanceArg); + const token = tokenFor(instanceArg); return { - listAgents: () => listLocalAgents(), - getAdapter, - node: { nodeVersion: process.versions.node, enginesRange: pkg.engines?.node }, - install: { - pathValue: process.env.PATH, - platform: process.platform, - isExecutable, - runVersion, - runningVersion: pkg.version, - }, - login: { - instance: saved ? { key: saved.key, url: saved.url, hasToken: Boolean(getToken(tokenKey)) } : null, - fetchUser: () => timedClient(createClient({ instance: tokenKey })).get('/api/auth/user'), - }, - daemon: { - loadRecord: loadDaemonRecord, - serviceFile: servicePaths().file, - fileExists: existsSync, - fetchMachine: (record) => getDaemonMachineStatus({ - client: timedClient(createClient({ instance: record.instanceUrl, token: record.daemonToken })), - record, - }), - describeError: daemonStatusErrorMessage, - now: Date.now(), - }, - macos: { - platform: process.platform, - detectSeatbelt, - isFile, - loginKeychainPath: join(homedir(), 'Library', 'Keychains', 'login.keychain-db'), - }, + key: saved?.key || null, + url: resolveUrl(instanceArg), + tokenSource: !token ? null : (env.COMMONLY_TOKEN ? 'env' : 'saved'), + loginHint: instanceArg ? `commonly login --instance ${instanceArg}` : 'commonly login', }; }; +export const hostDoctorDeps = ({ instance: instanceArg = null } = {}) => ({ + listAgents: () => listLocalAgents(), + getAdapter, + node: { nodeVersion: process.versions.node, enginesRange: pkg.engines?.node }, + install: { + pathValue: process.env.PATH, + platform: process.platform, + isExecutable, + runVersion, + runningVersion: pkg.version, + }, + login: { + target: loginTarget({ instanceArg }), + fetchUser: () => timedClient(createClient({ instance: instanceArg })).get('/api/auth/user'), + }, + daemon: { + loadRecord: loadDaemonRecord, + serviceFile: servicePaths().file, + fileExists: existsSync, + fetchMachine: (record) => getDaemonMachineStatus({ + client: timedClient(createClient({ instance: record.instanceUrl, token: record.daemonToken })), + record, + }), + describeError: daemonStatusErrorMessage, + now: Date.now(), + }, + macos: { + platform: process.platform, + detectSeatbelt, + isFile, + loginKeychainPath: join(homedir(), 'Library', 'Keychains', 'login.keychain-db'), + }, +}); + export const registerDoctor = (program) => { program .command('doctor') diff --git a/cli/src/lib/daemon-service.js b/cli/src/lib/daemon-service.js index 36cdefdb4b..4f95e17e5b 100644 --- a/cli/src/lib/daemon-service.js +++ b/cli/src/lib/daemon-service.js @@ -203,6 +203,10 @@ export const installDaemonService = async ({ cliPath = resolve(process.argv[1]), // The provider keys the seats' adapters declare (adapters/index.js). providerKeyEnvNames = [], + // Settings carried only when set, never warned about when absent: the + // operator's opt-outs (COMMONLY_NO_UPDATE_CHECK), which reach the seats the + // daemon spawns only through this file. + optionalEnvNames = [], env = process.env, writeFile = writeServiceFile, mkdirp, @@ -221,7 +225,7 @@ export const installDaemonService = async ({ chmod(daemonLogPath(home), 0o600); } - const providerEnv = providerEnvPairs({ names: providerKeyEnvNames, env }); + const providerEnv = providerEnvPairs({ names: [...new Set([...providerKeyEnvNames, ...optionalEnvNames])], env }); const missing = missingProviderKeys({ names: providerKeyEnvNames, env }); if (target.kind === 'launchd') { diff --git a/cli/src/lib/doctor.js b/cli/src/lib/doctor.js index b7b94f2dac..ea9284f680 100644 --- a/cli/src/lib/doctor.js +++ b/cli/src/lib/doctor.js @@ -41,7 +41,9 @@ export const checkNode = ({ nodeVersion, enginesRange }) => { /** Every `commonly` on PATH, first one first (the one a shell runs). */ export const findOnPath = ({ command, pathValue, platform, isExecutable }) => { - const names = platform === 'win32' ? [`${command}.cmd`, `${command}.exe`, command] : [command]; + // npm also drops a bare sh shim beside the .cmd on Windows; cmd.exe cannot run + // it, so counting it would report a second install that is not one. + const names = platform === 'win32' ? [`${command}.cmd`, `${command}.exe`] : [command]; const found = []; for (const dir of String(pathValue || '').split(platform === 'win32' ? ';' : delimiter)) { if (!dir) continue; @@ -71,24 +73,29 @@ export const checkInstall = ({ return result('install', PASS, `commonly ${version} at ${binary}${notes.length ? ` (${notes.join('; ')})` : ''}`); }; -// (c) The saved instance and whose token it holds. The token is sent, never shown. -export const checkLogin = async ({ instance, fetchUser }) => { - if (!instance) return result('login', FAIL, 'not logged in', 'commonly login'); - const where = `${instance.key || 'unsaved'} (${instance.url})`; - const relogin = `commonly login --instance ${instance.key || instance.url}`; - if (!instance.hasToken) return result('login', FAIL, `${where} has no saved token`, relogin); +// (c) The instance every other command would use, and whose token it sends. +// The token is sent, never shown. `target` comes from loginTarget (commands/ +// doctor.js), which resolves it the way createClient does, COMMONLY_TOKEN first. +export const checkLogin = async ({ target, fetchUser }) => { + const where = target.key ? `${target.key} (${target.url})` : target.url; + if (!target.tokenSource) { + return result('login', FAIL, target.key ? `${where} has no saved token` : 'not logged in', target.loginHint); + } + const fromEnv = target.tokenSource === 'env'; + const tokenName = fromEnv ? 'the token in COMMONLY_TOKEN' : 'the saved token'; + const refix = fromEnv ? 'replace or unset COMMONLY_TOKEN' : target.loginHint; let user; try { - user = await fetchUser(instance); + user = await fetchUser(); } catch (error) { if ([401, 403].includes(error?.status)) { - return result('login', FAIL, `${where} rejected the saved token`, relogin); + return result('login', FAIL, `${where} rejected ${tokenName}`, refix); } - return result('login', FAIL, `could not reach ${instance.url}: ${error?.message || error}`, 'check the network, then re-run commonly doctor'); + return result('login', FAIL, `could not reach ${target.url}: ${error?.message || error}`, 'check the network, then re-run commonly doctor'); } const who = user?.username || user?.email || user?._id; - if (!who) return result('login', FAIL, `${where} answered without a user`, relogin); - return result('login', PASS, `@${who} on ${where}`); + if (!who) return result('login', FAIL, `${where} answered without a user`, refix); + return result('login', PASS, `@${who} on ${where}${fromEnv ? ' (token from COMMONLY_TOKEN)' : ''}`); }; const ago = (iso, now) => { @@ -161,11 +168,16 @@ export const checkAdapters = async ({ agents, getAdapter }) => { continue; } let found = null; + let detectError = null; try { // eslint-disable-next-line no-await-in-loop found = await adapter.detect(); - } catch { - found = null; + } catch (error) { + detectError = error?.message || String(error); + } + if (detectError) { + results.push(result(id, FAIL, `${name} could not be checked (${detectError}), ${used}`, `run the ${name} CLI with --version by hand`)); + continue; } if (!found) { results.push(result(id, FAIL, `${name} is not on PATH or its --version failed, ${used}`, `install the ${name} CLI, or put it on PATH`)); @@ -196,7 +208,15 @@ export const checkMacClaude = ({ if (!isFile(loginKeychainPath)) missing.push(`login keychain (${loginKeychainPath})`); if (!isFile(securityPath)) missing.push(`${securityPath}`); if (missing.length) { - return result('macos-claude', FAIL, `missing: ${missing.join('; ')}`, 'sign in to this Mac as the user the agent runs as, and run claude once to log in'); + // A failed preflight most often means doctor itself is inside a sandbox + // (an agent's turn), where a nested sandbox-exec is refused. + const fixes = []; + if (!seatbelt?.available) fixes.push('run commonly doctor from a normal Terminal, not from inside an agent\'s sandbox'); + if (missing.length > (seatbelt?.available ? 0 : 1)) { + fixes.push('sign in to this Mac as the user the agent runs as, and run claude once to log in'); + } + const fix = fixes.join('; '); + return result('macos-claude', FAIL, `missing: ${missing.join('; ')}`, fix); } return result('macos-claude', PASS, 'Seatbelt runs, the login keychain and /usr/bin/security are present'); }; @@ -214,8 +234,12 @@ export const runDoctor = async (deps) => { ]; }; +// One line each, whatever a server or a CLI put in its error text. +// eslint-disable-next-line no-control-regex -- matching control characters is the point +const oneLine = (text) => String(text ?? '').replace(/[\u0000-\u001f\u007f-\u009f]+/g, ' ').trim(); + export const formatDoctorLine = ({ id, status, detail, fix }) => ( - `${status.padEnd(4)} ${id.padEnd(16)} ${detail}${status === FAIL && fix ? ` — fix: ${fix}` : ''}` + `${status.padEnd(4)} ${id.padEnd(16)} ${oneLine(detail)}${status === FAIL && fix ? ` — fix: ${oneLine(fix)}` : ''}` ); export const doctorExitCode = (results) => (results.some((r) => r.status === FAIL) ? 1 : 0); diff --git a/cli/src/lib/update-check.js b/cli/src/lib/update-check.js index 557ac6961d..807c88eb94 100644 --- a/cli/src/lib/update-check.js +++ b/cli/src/lib/update-check.js @@ -151,19 +151,24 @@ export const checkForCliUpdate = async ({ * Fire the check and return at once. The returned promise (for tests) settles * to the line that was logged, or null. */ -export const startUpdateNotice = ({ log, ...options }) => { - const pending = checkForCliUpdate(options).then((line) => { +export const startUpdateNotice = (params = {}) => Promise.resolve() + // Everything, the argument destructuring and checkForCliUpdate's default + // parameters (homedir()) included, runs inside the chain, so the trailing + // catch is the only exit a failure has. Start-up never sees a throw or an + // unhandled rejection from here. + .then(async () => { + const { log, ...options } = params; + const line = await checkForCliUpdate(options); if (line) { try { log(line); } catch { - // A closed log sink must not surface as an unhandled rejection. + // A closed log sink is not start-up's problem. } } return line; - }); - return pending; -}; + }) + .catch(() => null); let cachedCliVersion; /** This CLI's own version, from the package.json it shipped with. */ diff --git a/docs/cli/README.md b/docs/cli/README.md index 9bde4a9968..ae183ad557 100644 --- a/docs/cli/README.md +++ b/docs/cli/README.md @@ -1,5 +1,3 @@ -| `COMMONLY_NO_UPDATE_CHECK=1` | Silences the start-up "your CLI is N versions behind" line on `agent run` and `daemon run` ([doctor.md](doctor.md)). | -| `commonly doctor [--instance ]` | One line per check: Node, the install, login, the daemon, each adapter CLI in use, macOS claude prerequisites. Exit 1 on any fail. See [doctor.md](doctor.md). | # Commonly CLI The `commonly` CLI is the primary developer entry point to a Commonly instance. Log in, attach a local AI agent to a pod, scaffold a custom bot, tail pod messages, or spin up a local dev environment — all from one binary. @@ -201,6 +199,7 @@ The two `list` modes answer different questions — backend mode is "who is inst | `commonly daemon status [--verbose]` | Show server liveness and, with `--verbose`, supervised-seat state. | | `commonly daemon unregister` | Revoke the machine and remove its local key; a previously revoked key can also be removed locally. | | `commonly daemon logs [--seat ] [--follow]` | Read daemon or per-seat logs. | +| `commonly doctor [--instance ]` | One line per check: Node, the install, login, the daemon, each adapter CLI in use, macOS claude prerequisites. Exit 1 on any fail. See [doctor.md](doctor.md). | If this computer's key was revoked, `daemon status` points to `daemon register`. Registration replaces the old local record only after the server confirms that @@ -301,6 +300,7 @@ Written by `commonly agent run` during spawn cycles. Per-pod session IDs so wrap |----------|--------| | `COMMONLY_TOKEN` | Overrides the saved user token for every command (CI / scripts). | | `COMMONLY_BASE_URL` | Overrides the base URL (Python SDK `run()` honors this). | +| `COMMONLY_NO_UPDATE_CHECK=1` | Silences the start-up "your CLI is N versions behind" line on `agent run` and `daemon run`. `daemon install` carries it into the service when set ([doctor.md](doctor.md)). | --- diff --git a/docs/cli/doctor.md b/docs/cli/doctor.md index 7a0c9215bd..4f7a744121 100644 --- a/docs/cli/doctor.md +++ b/docs/cli/doctor.md @@ -14,7 +14,7 @@ silently by making breakage visible. Code: `cli/src/lib/doctor.js` (checks), |------|----------------|------------| | `node` | Node against `engines.node` in the CLI's package.json | Node is below the floor | | `install` | The first `commonly` on PATH and its `--version`; notes when the doctor you ran is a different version, or more copies are on PATH | none on PATH, or it does not answer `--version` | -| `login` | The saved instance (active, or `--instance`) and the user its token belongs to, via `GET /api/auth/user`. The token is sent, never printed. | not logged in, no token, token rejected, or the instance cannot be reached in 5 s | +| `login` | The instance and token every other command would use (`--instance`, or the active one; `COMMONLY_TOKEN` wins over the saved token, as in `createClient`) and the user the token belongs to, via `GET /api/auth/user`. The line says when the token came from `COMMONLY_TOKEN`. The token is sent, never printed. | no token, token rejected, or the instance cannot be reached in 5 s | | `daemon` | The daemon record, its launchd/systemd service file, and the server's view (`online`, last heartbeat) | the record is unreadable or insecure, the service is not installed, the key is revoked, or the machine is offline. **Skips** when no daemon is registered: foreground agents are a supported setup. | | `adapter:` | One line per adapter that an agent in `~/.commonly/tokens/` uses: the adapter's own `detect()`, which runs ` --version` | the CLI is missing or fails `--version`, the adapter is unknown, or the token file records none | | `macos-claude` | On macOS, when an agent uses the claude adapter: `sandbox-exec` runs, and `~/Library/Keychains/login.keychain-db` and `/usr/bin/security` exist | any of the three is missing | @@ -36,8 +36,13 @@ is 3 or more patch releases behind npm latest, or behind a newer minor or major: - **2 s cap** on the registry lookup, enforced by its own timer. - **24 h cache** in `~/.commonly/update-check.json`; only a successful lookup is cached. - **Silent** when offline, on a registry error, or with `COMMONLY_NO_UPDATE_CHECK=1`. -- The threshold is `UPDATE_NOTICE_MIN_BEHIND` (3). One or two releases behind - is the normal gap between a publish and a seat's next restart. + A daemon started as a login service gets only the environment in its service + file, so export the opt-out and re-run `commonly daemon install`; install + carries it into the plist or unit when it is set, and its seats inherit it. +- The threshold is `UPDATE_NOTICE_MIN_BEHIND` (3), the row's acceptance case. + At the October 2026 cadence (9 publishes on 10-30) that is under a day, so a + seat restarted daily will usually print it; raise the constant if that + proves to be noise. - **Trap: a source checkout reads as stale.** `.github/workflows/npm-publish.yml` stamps the version into its working copy only (`npm version --no-git-tag-version`), so `cli/package.json` on main lags npm: 0.1.168 on From 58727be540430af7446edc2b8774255e0dc2b4c9 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sat, 31 Oct 2026 06:03:25 -0700 Subject: [PATCH 3/6] fix(cli): answer CodeRabbit on doctor and the update notice (TASK-660) - runDoctor: an agent listing that throws (unreadable ~/.commonly/tokens) is one adapters FAIL line; node, install, login and daemon still print. - daemon install on Linux: enable + restart instead of enable --now, so a unit that is already running picks up the env just written (provider keys, COMMONLY_NO_UPDATE_CHECK), as launchd's unload/load already does. - Tests: the timeout aborts the fetch signal; a saved token answered without a user fails; timedClient puts a signal on every get. Co-Authored-By: Claude Opus 5.5 Co-authored-by: otto Agent-Model: claude/claude-opus-5-5/default --- cli/__tests__/daemon-service.test.mjs | 5 +++-- cli/__tests__/doctor-host.test.mjs | 13 ++++++++++++- cli/__tests__/doctor.test.mjs | 17 +++++++++++++++++ cli/__tests__/update-check.test.mjs | 8 +++++++- cli/src/commands/doctor.js | 2 +- cli/src/lib/daemon-service.js | 5 ++++- cli/src/lib/doctor.js | 20 +++++++++++++++++--- docs/agents/daemon-service-environment.md | 4 ++-- docs/cli/doctor.md | 3 ++- 9 files changed, 65 insertions(+), 12 deletions(-) diff --git a/cli/__tests__/daemon-service.test.mjs b/cli/__tests__/daemon-service.test.mjs index a8f4f2413f..8e4d5f7ad0 100644 --- a/cli/__tests__/daemon-service.test.mjs +++ b/cli/__tests__/daemon-service.test.mjs @@ -123,7 +123,7 @@ describe('install', () => { expect(deps.execCmd).toHaveBeenLastCalledWith(['launchctl', 'load', '-w', target.file]); }); - test('linux: writes the unit, reloads, enables --now', async () => { + test('linux: writes the unit, reloads, enables, and restarts so a running unit takes the new env', async () => { const deps = makeDeps(); const target = await installDaemonService({ platform: 'linux', home, nodePath, cliPath, ...deps, @@ -131,7 +131,8 @@ describe('install', () => { expect(target.kind).toBe('systemd'); expect(deps.execCmd.mock.calls).toEqual([ [['systemctl', '--user', 'daemon-reload']], - [['systemctl', '--user', 'enable', '--now', SYSTEMD_UNIT]], + [['systemctl', '--user', 'enable', SYSTEMD_UNIT]], + [['systemctl', '--user', 'restart', SYSTEMD_UNIT]], ]); }); diff --git a/cli/__tests__/doctor-host.test.mjs b/cli/__tests__/doctor-host.test.mjs index 4a3fb9d626..548eccdb20 100644 --- a/cli/__tests__/doctor-host.test.mjs +++ b/cli/__tests__/doctor-host.test.mjs @@ -1,4 +1,4 @@ -import { loginTarget, versionCommand } from '../src/commands/doctor.js'; +import { loginTarget, timedClient, versionCommand } from '../src/commands/doctor.js'; // The host wiring's two decisions, with config resolution faked. loginTarget // must agree with createClient (api.js): getToken prefers COMMONLY_TOKEN, and @@ -57,3 +57,14 @@ describe('versionCommand', () => { expect(versionCommand('C:\\npm\\commonly.exe', 'win32').shell).toBe(false); }); }); + +describe('timedClient', () => { + test('every get carries a timeout signal, so a hung server is a fail line, not a hang', async () => { + const seen = []; + const client = timedClient({ get: async (path, params, options) => { seen.push({ path, params, options }); return {}; } }); + await client.get('/api/auth/user'); + expect(seen[0].path).toBe('/api/auth/user'); + expect(seen[0].options.signal).toBeInstanceOf(AbortSignal); + expect(seen[0].options.signal.aborted).toBe(false); + }); +}); diff --git a/cli/__tests__/doctor.test.mjs b/cli/__tests__/doctor.test.mjs index 8b977bd18c..17c9827ae0 100644 --- a/cli/__tests__/doctor.test.mjs +++ b/cli/__tests__/doctor.test.mjs @@ -132,6 +132,12 @@ describe('(c) login', () => { expect(r.detail).toContain('the token in COMMONLY_TOKEN'); }); + test('fails when the server answers the saved token without a user', async () => { + const r = await checkLogin({ target: saved, fetchUser: async () => ({}) }); + expect(r).toMatchObject({ status: FAIL, fix: 'commonly login' }); + expect(r.detail).toBe('default (https://api.commonly.me) answered without a user'); + }); + test('fails when the server cannot be reached', async () => { const r = await checkLogin({ target: saved, fetchUser: async () => { throw new TypeError('fetch failed'); } }); expect(r.status).toBe(FAIL); @@ -330,6 +336,17 @@ describe('runDoctor', () => { deps.login.target = { ...deps.login.target, tokenSource: null }; expect(doctorExitCode(await runDoctor(deps))).toBe(1); }); + + test('an agent listing that throws is one fail line, and every other check still runs', async () => { + const deps = healthy(); + deps.listAgents = () => { throw Object.assign(new Error("EACCES: permission denied, scandir '/h/.commonly/tokens'"), { code: 'EACCES' }); }; + const results = await runDoctor(deps); + expect(results.map((r) => [r.id, r.status])).toEqual([ + ['node', PASS], ['install', PASS], ['login', PASS], ['daemon', PASS], ['adapters', FAIL], ['macos-claude', SKIP], + ]); + expect(results[4].detail).toContain('EACCES'); + expect(doctorExitCode(results)).toBe(1); + }); }); test('formatDoctorLine keeps a multi-line or ANSI server error on one line', () => { diff --git a/cli/__tests__/update-check.test.mjs b/cli/__tests__/update-check.test.mjs index b3cdee40d9..99822c15ca 100644 --- a/cli/__tests__/update-check.test.mjs +++ b/cli/__tests__/update-check.test.mjs @@ -104,11 +104,17 @@ describe('checkForCliUpdate', () => { }); test('a registry timeout gives none, inside the timeout, even when fetch ignores its abort signal', async () => { - const hanging = () => new Promise(() => {}); + let signal; + const hanging = (_url, init) => { + signal = init.signal; + return new Promise(() => {}); + }; const started = Date.now(); const line = await checkForCliUpdate(base({ fetchImpl: hanging, timeoutMs: 50 })); expect(line).toBeNull(); expect(Date.now() - started).toBeLessThan(1000); + // The timer also asks fetch to stop, so a fetch that honours it frees its socket. + expect(signal.aborted).toBe(true); }); test('offline (fetch rejects) gives none and writes no cache', async () => { diff --git a/cli/src/commands/doctor.js b/cli/src/commands/doctor.js index 019a49855b..fac87c0488 100644 --- a/cli/src/commands/doctor.js +++ b/cli/src/commands/doctor.js @@ -19,7 +19,7 @@ import { doctorExitCode, formatDoctorLine, runDoctor } from '../lib/doctor.js'; const REQUEST_TIMEOUT_MS = 5000; // Every server read doctor makes is bounded, so a hung instance is a fail line, not a hang. -const timedClient = (client) => ({ +export const timedClient = (client) => ({ ...client, get: (path, params = {}, options = {}) => client.get(path, params, { signal: AbortSignal.timeout(REQUEST_TIMEOUT_MS), ...options }), }); diff --git a/cli/src/lib/daemon-service.js b/cli/src/lib/daemon-service.js index 4f95e17e5b..11d36fd8ac 100644 --- a/cli/src/lib/daemon-service.js +++ b/cli/src/lib/daemon-service.js @@ -241,7 +241,10 @@ export const installDaemonService = async ({ } else { writeFile(target.file, systemdUnit({ nodePath, cliPath, providerEnv })); await execCmd(['systemctl', '--user', 'daemon-reload']); - await execCmd(['systemctl', '--user', 'enable', '--now', SYSTEMD_UNIT]); + // restart, not `enable --now`: a unit that is already running must pick up + // the environment just written, the way launchd's unload/load does. + await execCmd(['systemctl', '--user', 'enable', SYSTEMD_UNIT]); + await execCmd(['systemctl', '--user', 'restart', SYSTEMD_UNIT]); } for (const name of missing) { warn(`${name} is not set in this shell, so it is NOT in the service file. A seat whose adapter needs it will fail to start; export it and re-run: commonly daemon install (docs/agents/daemon-service-environment.md)`); diff --git a/cli/src/lib/doctor.js b/cli/src/lib/doctor.js index ea9284f680..6e8707a31b 100644 --- a/cli/src/lib/doctor.js +++ b/cli/src/lib/doctor.js @@ -223,14 +223,28 @@ export const checkMacClaude = ({ /** Every check, in the order the lines print. */ export const runDoctor = async (deps) => { - const agents = deps.listAgents(); + // An unreadable tokens dir is one fail line, not a doctor that prints nothing. + let agents = null; + let listError = null; + try { + agents = deps.listAgents(); + } catch (error) { + listError = error?.message || String(error); + } return [ checkNode(deps.node), checkInstall(deps.install), await checkLogin(deps.login), await checkDaemon(deps.daemon), - ...(await checkAdapters({ agents, getAdapter: deps.getAdapter })), - checkMacClaude({ ...deps.macos, agents }), + ...(listError + ? [ + result('adapters', FAIL, `could not list the attached agents (${listError})`, 'check that ~/.commonly/tokens is a directory you can read'), + result('macos-claude', SKIP, 'the attached agents could not be listed'), + ] + : [ + ...(await checkAdapters({ agents, getAdapter: deps.getAdapter })), + checkMacClaude({ ...deps.macos, agents }), + ]), ]; }; diff --git a/docs/agents/daemon-service-environment.md b/docs/agents/daemon-service-environment.md index 0c13016c00..1fe0eee8bb 100644 --- a/docs/agents/daemon-service-environment.md +++ b/docs/agents/daemon-service-environment.md @@ -66,8 +66,8 @@ Three surfaces now say so, in this order: ```bash export COMMONLY_LITELLM_KEY=... # the shell matters: install reads THIS one -commonly daemon install # rewrites the unit/plist with the key -commonly daemon restart # the running daemon still holds the old env +commonly daemon install # rewrites the unit/plist with the key and reloads it + # (launchd unload/load; systemd restart, since #3082) ``` Read back what the installed service actually carries rather than assuming: diff --git a/docs/cli/doctor.md b/docs/cli/doctor.md index 4f7a744121..9423828aaf 100644 --- a/docs/cli/doctor.md +++ b/docs/cli/doctor.md @@ -38,7 +38,8 @@ is 3 or more patch releases behind npm latest, or behind a newer minor or major: - **Silent** when offline, on a registry error, or with `COMMONLY_NO_UPDATE_CHECK=1`. A daemon started as a login service gets only the environment in its service file, so export the opt-out and re-run `commonly daemon install`; install - carries it into the plist or unit when it is set, and its seats inherit it. + carries it into the plist or unit when it is set and reloads the service + (launchd unload/load, systemd restart), and its seats inherit it. - The threshold is `UPDATE_NOTICE_MIN_BEHIND` (3), the row's acceptance case. At the October 2026 cadence (9 publishes on 10-30) that is under a day, so a seat restarted daily will usually print it; raise the constant if that From d68ad074a2f7ebee9630a21605c8d753182e93ae Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sat, 31 Oct 2026 06:11:25 -0700 Subject: [PATCH 4/6] fix(cli): doctor fails on a token file that is not readable JSON (TASK-660) listLocalAgents drops a token file it cannot parse, so doctor read a broken seat as "no agents attached" and could exit 0. A new listUnreadableAgentTokens names those files and doctor adds an `adapters` FAIL line for them (CodeRabbit thread on doctor.js:230). Co-Authored-By: Claude Opus 5.5 Co-authored-by: otto Agent-Model: claude/claude-opus-5-5/default --- cli/__tests__/detach.test.mjs | 17 +++++++++++++++++ cli/__tests__/doctor.test.mjs | 21 +++++++++++++++++++++ cli/src/commands/agent.js | 15 +++++++++++++++ cli/src/commands/doctor.js | 3 ++- cli/src/lib/doctor.js | 27 ++++++++++++++++++++------- docs/cli/doctor.md | 2 +- 6 files changed, 76 insertions(+), 9 deletions(-) diff --git a/cli/__tests__/detach.test.mjs b/cli/__tests__/detach.test.mjs index b28db37c04..c44d56dc64 100644 --- a/cli/__tests__/detach.test.mjs +++ b/cli/__tests__/detach.test.mjs @@ -29,6 +29,7 @@ const { saveAgentToken, loadAgentToken, listLocalAgents, + listUnreadableAgentTokens, } = await import('../src/commands/agent.js'); const { setSession, getSession } = await import('../src/lib/session-store.js'); @@ -233,4 +234,20 @@ describe('listLocalAgents', () => { expect(agent.name).toBe('corrupt'); expect(agent.lastTurn).toBeNull(); }); + + test('a token file that is not JSON is dropped from the listing and named as unreadable', () => { + saveAgentToken('fine', { + agentName: 'fine', instanceId: 'default', podId: 'pod-f', + instanceUrl: 'http://localhost:5000', runtimeToken: 'cm_agent_f', + adapter: 'claude', + }); + fs.writeFileSync(path.join(tmp, '.commonly', 'tokens', 'broken.json'), '{"agentName": ', 'utf8'); + + expect(listLocalAgents().map((a) => a.name)).toEqual(['fine']); + expect(listUnreadableAgentTokens()).toEqual(['broken']); + }); + + test('no tokens dir means nothing unreadable', () => { + expect(listUnreadableAgentTokens()).toEqual([]); + }); }); diff --git a/cli/__tests__/doctor.test.mjs b/cli/__tests__/doctor.test.mjs index 17c9827ae0..57b87d2d61 100644 --- a/cli/__tests__/doctor.test.mjs +++ b/cli/__tests__/doctor.test.mjs @@ -337,6 +337,27 @@ describe('runDoctor', () => { expect(doctorExitCode(await runDoctor(deps))).toBe(1); }); + test('a token file that is not readable JSON fails doctor, even with no other agent', async () => { + const deps = healthy(); + deps.listAgents = () => []; + deps.listUnreadable = () => ['broken']; + const results = await runDoctor(deps); + const line = results.find((r) => r.id === 'adapters'); + expect(line).toMatchObject({ status: FAIL, fix: 'commonly agent detach --force, then attach it again' }); + expect(line.detail).toContain('broken'); + expect(doctorExitCode(results)).toBe(1); + }); + + test('an unreadable token file is reported beside the agents that did load', async () => { + const deps = healthy(); + deps.listUnreadable = () => ['broken']; + const results = await runDoctor(deps); + expect(results.map((r) => [r.id, r.status])).toEqual([ + ['node', PASS], ['install', PASS], ['login', PASS], ['daemon', PASS], + ['adapter:claude', PASS], ['adapters', FAIL], ['macos-claude', PASS], + ]); + }); + test('an agent listing that throws is one fail line, and every other check still runs', async () => { const deps = healthy(); deps.listAgents = () => { throw Object.assign(new Error("EACCES: permission denied, scandir '/h/.commonly/tokens'"), { code: 'EACCES' }); }; diff --git a/cli/src/commands/agent.js b/cli/src/commands/agent.js index 6bf66979c9..ddcf81c030 100644 --- a/cli/src/commands/agent.js +++ b/cli/src/commands/agent.js @@ -536,6 +536,21 @@ export const listLocalAgents = () => { .filter(Boolean); }; +/** + * The token files listLocalAgents drops: present, but not readable JSON. + * `commonly doctor` reports them, because a dropped file otherwise reads as + * "no agent here" and lets doctor pass on a seat that cannot start. + */ +export const listUnreadableAgentTokens = () => { + const dir = tokensDir(); + if (!existsSync(dir)) return []; + return readdirSync(dir) + .filter((f) => f.endsWith('.json')) + .map((f) => f.slice(0, -'.json'.length)) + .filter((name) => !loadAgentToken(name)) + .sort(); +}; + // Chat event types whose payload already contains the prompt the wrapper // should forward verbatim. Heartbeat and consult events need event-specific // framing, so extractPrompt handles them separately below. diff --git a/cli/src/commands/doctor.js b/cli/src/commands/doctor.js index fac87c0488..63d6df44bc 100644 --- a/cli/src/commands/doctor.js +++ b/cli/src/commands/doctor.js @@ -13,7 +13,7 @@ import { servicePaths } from '../lib/daemon-service.js'; import { getAdapter } from '../lib/adapters/index.js'; import { detectSeatbelt } from '../lib/sandbox/seatbelt.js'; import { daemonStatusErrorMessage, getDaemonMachineStatus } from './daemon.js'; -import { listLocalAgents } from './agent.js'; +import { listLocalAgents, listUnreadableAgentTokens } from './agent.js'; import { doctorExitCode, formatDoctorLine, runDoctor } from '../lib/doctor.js'; const REQUEST_TIMEOUT_MS = 5000; @@ -84,6 +84,7 @@ export const loginTarget = ({ export const hostDoctorDeps = ({ instance: instanceArg = null } = {}) => ({ listAgents: () => listLocalAgents(), + listUnreadable: () => listUnreadableAgentTokens(), getAdapter, node: { nodeVersion: process.versions.node, enginesRange: pkg.engines?.node }, install: { diff --git a/cli/src/lib/doctor.js b/cli/src/lib/doctor.js index 6e8707a31b..aab8fb4fae 100644 --- a/cli/src/lib/doctor.js +++ b/cli/src/lib/doctor.js @@ -139,21 +139,32 @@ export const checkDaemon = async ({ const MAX_NAMED_AGENTS = 5; +const nameList = (names) => { + const sorted = [...names].sort(); + const shown = sorted.slice(0, MAX_NAMED_AGENTS).join(', '); + return `${shown}${sorted.length > MAX_NAMED_AGENTS ? ` and ${sorted.length - MAX_NAMED_AGENTS} more` : ''}`; +}; + // (e) Every adapter CLI a configured agent uses, found and answering --version. -export const checkAdapters = async ({ agents, getAdapter }) => { +// `unreadable` names token files that exist but are not readable JSON: the +// agent listing drops them, so without this line they would read as no agent. +export const checkAdapters = async ({ agents, getAdapter, unreadable = [] }) => { const byAdapter = new Map(); for (const agent of agents) { const adapter = agent?.adapter || '?'; if (!byAdapter.has(adapter)) byAdapter.set(adapter, []); byAdapter.get(adapter).push(agent.name); } - if (!byAdapter.size) return [result('adapters', SKIP, 'no agents are attached on this computer')]; + const unreadableLine = unreadable.length + ? [result('adapters', FAIL, `the token file of ${nameList(unreadable)} is not readable JSON (~/.commonly/tokens)`, 'commonly agent detach --force, then attach it again')] + : []; + if (!byAdapter.size) { + return unreadableLine.length ? unreadableLine : [result('adapters', SKIP, 'no agents are attached on this computer')]; + } const results = []; for (const [name, agentNames] of [...byAdapter.entries()].sort(([a], [b]) => a.localeCompare(b))) { const id = `adapter:${name}`; - const sorted = agentNames.sort(); - const shown = sorted.slice(0, MAX_NAMED_AGENTS).join(', '); - const used = `used by ${shown}${sorted.length > MAX_NAMED_AGENTS ? ` and ${sorted.length - MAX_NAMED_AGENTS} more` : ''}`; + const used = `used by ${nameList(agentNames)}`; if (name === '?') { results.push(result(id, FAIL, `no adapter recorded in the token file, ${used}`, 'commonly agent detach --force, then attach it again')); continue; @@ -186,7 +197,7 @@ export const checkAdapters = async ({ agents, getAdapter }) => { const at = found.path ? ` at ${found.path}` : ''; results.push(result(id, PASS, `${name} ${found.version || 'unknown version'}${at}, ${used}`)); } - return results; + return [...results, ...unreadableLine]; }; // (f) macOS: what the claude adapter's confined spawn needs, by file stat only. @@ -225,9 +236,11 @@ export const checkMacClaude = ({ export const runDoctor = async (deps) => { // An unreadable tokens dir is one fail line, not a doctor that prints nothing. let agents = null; + let unreadable = []; let listError = null; try { agents = deps.listAgents(); + unreadable = deps.listUnreadable ? deps.listUnreadable() : []; } catch (error) { listError = error?.message || String(error); } @@ -242,7 +255,7 @@ export const runDoctor = async (deps) => { result('macos-claude', SKIP, 'the attached agents could not be listed'), ] : [ - ...(await checkAdapters({ agents, getAdapter: deps.getAdapter })), + ...(await checkAdapters({ agents, getAdapter: deps.getAdapter, unreadable })), checkMacClaude({ ...deps.macos, agents }), ]), ]; diff --git a/docs/cli/doctor.md b/docs/cli/doctor.md index 9423828aaf..edab936a5c 100644 --- a/docs/cli/doctor.md +++ b/docs/cli/doctor.md @@ -16,7 +16,7 @@ silently by making breakage visible. Code: `cli/src/lib/doctor.js` (checks), | `install` | The first `commonly` on PATH and its `--version`; notes when the doctor you ran is a different version, or more copies are on PATH | none on PATH, or it does not answer `--version` | | `login` | The instance and token every other command would use (`--instance`, or the active one; `COMMONLY_TOKEN` wins over the saved token, as in `createClient`) and the user the token belongs to, via `GET /api/auth/user`. The line says when the token came from `COMMONLY_TOKEN`. The token is sent, never printed. | no token, token rejected, or the instance cannot be reached in 5 s | | `daemon` | The daemon record, its launchd/systemd service file, and the server's view (`online`, last heartbeat) | the record is unreadable or insecure, the service is not installed, the key is revoked, or the machine is offline. **Skips** when no daemon is registered: foreground agents are a supported setup. | -| `adapter:` | One line per adapter that an agent in `~/.commonly/tokens/` uses: the adapter's own `detect()`, which runs ` --version` | the CLI is missing or fails `--version`, the adapter is unknown, or the token file records none | +| `adapter:` | One line per adapter that an agent in `~/.commonly/tokens/` uses: the adapter's own `detect()`, which runs ` --version` | the CLI is missing or fails `--version`, the adapter is unknown, or the token file records none. A separate `adapters` line fails when a token file is not readable JSON or the directory cannot be listed | | `macos-claude` | On macOS, when an agent uses the claude adapter: `sandbox-exec` runs, and `~/Library/Keychains/login.keychain-db` and `/usr/bin/security` exist | any of the three is missing | The macOS check only **stats** the two files. It never runs `security`, so it From e60f3e65ec6311ba87eb2291513e2037e6832c10 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sat, 31 Oct 2026 06:22:09 -0700 Subject: [PATCH 5/6] fix(cli): doctor reports a tokens dir it cannot read (TASK-660) listUnreadableAgentTokens read with existsSync first, which answers false for EACCES on an ancestor, so a locked ~/.commonly read as no agents. It now reads the directory and treats only ENOENT/ENOTDIR as empty; any other error reaches runDoctor's adapters FAIL line. Adds a test of the hostDoctorDeps wiring with a malformed token under a temp HOME (CodeRabbit threads on agent.js:546 and doctor.js:87). Co-Authored-By: Claude Opus 5.5 Co-authored-by: otto Agent-Model: claude/claude-opus-5-5/default --- cli/__tests__/detach.test.mjs | 18 ++++++++++++ cli/__tests__/doctor-host-tokens.test.mjs | 36 +++++++++++++++++++++++ cli/src/commands/agent.js | 14 +++++++-- 3 files changed, 65 insertions(+), 3 deletions(-) create mode 100644 cli/__tests__/doctor-host-tokens.test.mjs diff --git a/cli/__tests__/detach.test.mjs b/cli/__tests__/detach.test.mjs index c44d56dc64..0f40b7c118 100644 --- a/cli/__tests__/detach.test.mjs +++ b/cli/__tests__/detach.test.mjs @@ -250,4 +250,22 @@ describe('listLocalAgents', () => { test('no tokens dir means nothing unreadable', () => { expect(listUnreadableAgentTokens()).toEqual([]); }); + + test('a tokens path that is a file means nothing unreadable', () => { + fs.mkdirSync(path.join(tmp, '.commonly'), { recursive: true }); + fs.writeFileSync(path.join(tmp, '.commonly', 'tokens'), '', 'utf8'); + expect(listUnreadableAgentTokens()).toEqual([]); + }); + + // root reads through mode 000, so the denial cannot be staged there. + (process.getuid?.() === 0 ? test.skip : test)('a tokens dir it cannot traverse throws instead of reading as empty', () => { + const state = path.join(tmp, '.commonly'); + fs.mkdirSync(path.join(state, 'tokens'), { recursive: true }); + fs.chmodSync(state, 0o000); + try { + expect(() => listUnreadableAgentTokens()).toThrow(/EACCES/); + } finally { + fs.chmodSync(state, 0o700); + } + }); }); diff --git a/cli/__tests__/doctor-host-tokens.test.mjs b/cli/__tests__/doctor-host-tokens.test.mjs new file mode 100644 index 0000000000..fc5b0bb6a9 --- /dev/null +++ b/cli/__tests__/doctor-host-tokens.test.mjs @@ -0,0 +1,36 @@ +/** + * The production wiring of doctor's agent listing (TASK-660): hostDoctorDeps + * must hand runDoctor the unreadable token files, or a malformed file reads as + * "no agent here" again. HOME is a temp dir, so nothing on this host is read. + */ +import { jest } from '@jest/globals'; +import os from 'os'; +import path from 'path'; +import fs from 'fs'; + +const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'cli-doctor-host-')); + +await jest.unstable_mockModule('os', () => ({ + ...os, + default: { ...os, homedir: () => tmp }, + homedir: () => tmp, +})); + +const { hostDoctorDeps } = await import('../src/commands/doctor.js'); +const { saveAgentToken } = await import('../src/commands/agent.js'); + +afterAll(() => { + fs.rmSync(tmp, { recursive: true, force: true }); +}); + +test('hostDoctorDeps lists the agents that load and names the token file that does not', () => { + saveAgentToken('fine', { + agentName: 'fine', instanceId: 'default', podId: 'pod-f', + instanceUrl: 'http://localhost:5000', runtimeToken: 'cm_agent_f', adapter: 'claude', + }); + fs.writeFileSync(path.join(tmp, '.commonly', 'tokens', 'broken.json'), '{"agentName": ', 'utf8'); + + const deps = hostDoctorDeps(); + expect(deps.listAgents().map((a) => a.name)).toEqual(['fine']); + expect(deps.listUnreadable()).toEqual(['broken']); +}); diff --git a/cli/src/commands/agent.js b/cli/src/commands/agent.js index ddcf81c030..209da246c5 100644 --- a/cli/src/commands/agent.js +++ b/cli/src/commands/agent.js @@ -540,11 +540,19 @@ export const listLocalAgents = () => { * The token files listLocalAgents drops: present, but not readable JSON. * `commonly doctor` reports them, because a dropped file otherwise reads as * "no agent here" and lets doctor pass on a seat that cannot start. + * Throws when the directory exists but cannot be read, for doctor to report. */ export const listUnreadableAgentTokens = () => { - const dir = tokensDir(); - if (!existsSync(dir)) return []; - return readdirSync(dir) + let files; + try { + files = readdirSync(tokensDir()); + } catch (error) { + // Only a missing directory means no agents. existsSync also answers false + // for EACCES on an ancestor, which is why this reads instead of checking. + if (error?.code === 'ENOENT' || error?.code === 'ENOTDIR') return []; + throw error; + } + return files .filter((f) => f.endsWith('.json')) .map((f) => f.slice(0, -'.json'.length)) .filter((name) => !loadAgentToken(name)) From 148acf52792f737d2721e99ce6871aea25f70106 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sat, 31 Oct 2026 06:29:07 -0700 Subject: [PATCH 6/6] test(cli): skip the mode-000 tokens test on Windows (TASK-660) Windows chmod cannot remove a directory's read permission, so the fixture cannot stage the EACCES there (CodeRabbit, detach.test.mjs:261). Co-Authored-By: Claude Opus 5.5 Co-authored-by: otto Agent-Model: claude/claude-opus-5-5/default --- cli/__tests__/detach.test.mjs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/cli/__tests__/detach.test.mjs b/cli/__tests__/detach.test.mjs index 0f40b7c118..218e3fa32e 100644 --- a/cli/__tests__/detach.test.mjs +++ b/cli/__tests__/detach.test.mjs @@ -257,8 +257,9 @@ describe('listLocalAgents', () => { expect(listUnreadableAgentTokens()).toEqual([]); }); - // root reads through mode 000, so the denial cannot be staged there. - (process.getuid?.() === 0 ? test.skip : test)('a tokens dir it cannot traverse throws instead of reading as empty', () => { + // Mode 000 stages the denial only on POSIX as a non-root user: root reads + // through it, and Windows chmod cannot remove a directory's read permission. + (process.platform === 'win32' || process.getuid?.() === 0 ? test.skip : test)('a tokens dir it cannot traverse throws instead of reading as empty', () => { const state = path.join(tmp, '.commonly'); fs.mkdirSync(path.join(state, 'tokens'), { recursive: true }); fs.chmodSync(state, 0o000);