From 799da57df1e8cc3cec3b92905bf8bcf5392925fc Mon Sep 17 00:00:00 2001 From: kiwigitops Date: Wed, 20 May 2026 22:35:10 -0400 Subject: [PATCH 1/3] Avoid overwriting malformed global config --- src/cli/__tests__/global-config.test.ts | 9 +++++++++ src/lib/schemas/io/global-config.ts | 14 +++++++++++++- 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/src/cli/__tests__/global-config.test.ts b/src/cli/__tests__/global-config.test.ts index 6e2038973..425421561 100644 --- a/src/cli/__tests__/global-config.test.ts +++ b/src/cli/__tests__/global-config.test.ts @@ -106,6 +106,15 @@ describe('global-config', () => { }); }); + it('does not overwrite malformed config', async () => { + await writeFile(tmp.configFile, '{ invalid json'); + + const ok = await updateGlobalConfig({ telemetry: { enabled: true } }, tmp.configDir, tmp.configFile); + + expect(ok).toBe(false); + expect(await readFile(tmp.configFile, 'utf-8')).toBe('{ invalid json'); + }); + it('returns false on write failures', async () => { const ok = await updateGlobalConfig( { telemetry: { enabled: true } }, diff --git a/src/lib/schemas/io/global-config.ts b/src/lib/schemas/io/global-config.ts index fc64eb39b..2e105d870 100644 --- a/src/lib/schemas/io/global-config.ts +++ b/src/lib/schemas/io/global-config.ts @@ -52,7 +52,7 @@ export async function updateGlobalConfig( configFile = GLOBAL_CONFIG_FILE ): Promise { try { - const existing = await readGlobalConfig(configFile); + const existing = await readGlobalConfigForUpdate(configFile); const merged: GlobalConfig = mergeConfig(existing, partial); await mkdir(configDir, { recursive: true }); @@ -63,6 +63,18 @@ export async function updateGlobalConfig( } } +async function readGlobalConfigForUpdate(configFile: string): Promise { + try { + const data = await readFile(configFile, 'utf-8'); + return GlobalConfigSchema.parse(JSON.parse(data)); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') { + return {}; + } + throw error; + } +} + function mergeConfig(target: GlobalConfig, source: GlobalConfig): GlobalConfig { return { ...target, From 325b50584529e2ec1f17061a7dcf1a379c72627b Mon Sep 17 00:00:00 2001 From: kiwigitops Date: Thu, 21 May 2026 21:33:05 -0400 Subject: [PATCH 2/3] Surface config read/parse errors from updateGlobalConfig Return a {success, error} result instead of a bare boolean so callers can distinguish a missing config (start fresh) from a malformed one (left unchanged, with the parse/read error surfaced). Telemetry enable/disable now report the actual failure reason rather than a generic write warning. --- src/cli/__tests__/global-config.test.ts | 17 ++++--- src/cli/commands/telemetry/actions.ts | 12 ++--- src/lib/schemas/io/global-config.ts | 65 ++++++++++++++++++++----- 3 files changed, 67 insertions(+), 27 deletions(-) diff --git a/src/cli/__tests__/global-config.test.ts b/src/cli/__tests__/global-config.test.ts index 425421561..a4dc1bf8c 100644 --- a/src/cli/__tests__/global-config.test.ts +++ b/src/cli/__tests__/global-config.test.ts @@ -82,9 +82,9 @@ describe('global-config', () => { it('creates directory and writes config when none exists', async () => { const fresh = createTempConfig('gc-fresh'); - const ok = await updateGlobalConfig({ telemetry: { enabled: false } }, fresh.configDir, fresh.configFile); + const result = await updateGlobalConfig({ telemetry: { enabled: false } }, fresh.configDir, fresh.configFile); - expect(ok).toBe(true); + expect(result.success).toBe(true); const written = JSON.parse(await readFile(fresh.configFile, 'utf-8')); expect(written).toEqual({ telemetry: { enabled: false } }); @@ -106,23 +106,24 @@ describe('global-config', () => { }); }); - it('does not overwrite malformed config', async () => { + it('does not overwrite malformed config and reports why', async () => { await writeFile(tmp.configFile, '{ invalid json'); - const ok = await updateGlobalConfig({ telemetry: { enabled: true } }, tmp.configDir, tmp.configFile); + const result = await updateGlobalConfig({ telemetry: { enabled: true } }, tmp.configDir, tmp.configFile); - expect(ok).toBe(false); + expect(result.success).toBe(false); + expect(result.success ? '' : result.error.message).toMatch(/malformed/); expect(await readFile(tmp.configFile, 'utf-8')).toBe('{ invalid json'); }); - it('returns false on write failures', async () => { - const ok = await updateGlobalConfig( + it('returns an error on write failures', async () => { + const result = await updateGlobalConfig( { telemetry: { enabled: true } }, tmp.testDir + '/\0invalid', tmp.testDir + '/\0invalid/config.json' ); - expect(ok).toBe(false); + expect(result.success).toBe(false); }); }); diff --git a/src/cli/commands/telemetry/actions.ts b/src/cli/commands/telemetry/actions.ts index c83a6f425..9c67df63e 100644 --- a/src/cli/commands/telemetry/actions.ts +++ b/src/cli/commands/telemetry/actions.ts @@ -10,18 +10,18 @@ export async function handleTelemetryDisable( configDir = GLOBAL_CONFIG_DIR, configFile = GLOBAL_CONFIG_FILE ): Promise { - const ok = await updateGlobalConfig({ telemetry: { enabled: false } }, configDir, configFile); - console.log(ok ? 'Telemetry has been disabled.' : `Warning: could not write config to ${configFile}`); - return ok; + const result = await updateGlobalConfig({ telemetry: { enabled: false } }, configDir, configFile); + console.log(result.success ? 'Telemetry has been disabled.' : `Warning: ${result.error.message}`); + return result.success; } export async function handleTelemetryEnable( configDir = GLOBAL_CONFIG_DIR, configFile = GLOBAL_CONFIG_FILE ): Promise { - const ok = await updateGlobalConfig({ telemetry: { enabled: true } }, configDir, configFile); - console.log(ok ? 'Telemetry has been enabled.' : `Warning: could not write config to ${configFile}`); - return ok; + const result = await updateGlobalConfig({ telemetry: { enabled: true } }, configDir, configFile); + console.log(result.success ? 'Telemetry has been enabled.' : `Warning: ${result.error.message}`); + return result.success; } export async function handleTelemetryStatus(configFile = GLOBAL_CONFIG_FILE): Promise { diff --git a/src/lib/schemas/io/global-config.ts b/src/lib/schemas/io/global-config.ts index 2e105d870..83916bef4 100644 --- a/src/lib/schemas/io/global-config.ts +++ b/src/lib/schemas/io/global-config.ts @@ -1,9 +1,10 @@ import { readFileSync } from 'fs'; -import { mkdir, readFile, writeFile } from 'fs/promises'; +import { mkdir, readFile, stat, writeFile } from 'fs/promises'; import { randomUUID } from 'node:crypto'; import { homedir } from 'os'; import { join } from 'path'; import { z } from 'zod'; +import { toError } from '../../errors/types.js'; export const GLOBAL_CONFIG_DIR = process.env.AGENTCORE_CONFIG_DIR ?? join(homedir(), '.agentcore'); export const GLOBAL_CONFIG_FILE = join(GLOBAL_CONFIG_DIR, 'config.json'); @@ -46,32 +47,70 @@ export function readGlobalConfigSync(configFile = GLOBAL_CONFIG_FILE): GlobalCon } } +export type UpdateGlobalConfigResult = { success: true } | { success: false; error: Error }; + export async function updateGlobalConfig( partial: GlobalConfig, configDir = GLOBAL_CONFIG_DIR, configFile = GLOBAL_CONFIG_FILE -): Promise { - try { - const existing = await readGlobalConfigForUpdate(configFile); - const merged: GlobalConfig = mergeConfig(existing, partial); +): Promise { + // Read the existing config strictly: a missing file is fine (start fresh), but a + // malformed file must not be silently overwritten with merged-in defaults. + const existing = await loadConfigForUpdate(configFile); + if (!existing.success) { + return existing; + } + try { + const merged: GlobalConfig = mergeConfig(existing.config, partial); await mkdir(configDir, { recursive: true }); await writeFile(configFile, JSON.stringify(merged, null, 2), 'utf-8'); - return true; - } catch { - return false; + return { success: true }; + } catch (error) { + return { success: false, error: new Error(`Failed to write config to ${configFile}: ${toError(error).message}`) }; } } -async function readGlobalConfigForUpdate(configFile: string): Promise { +type LoadConfigResult = { success: true; config: GlobalConfig } | { success: false; error: Error }; + +/** + * Reads the existing global config for an update. Distinguishes a missing file + * (treated as an empty config) from a malformed one (read/parse/schema failure), + * so the caller can avoid clobbering a config it could not understand. + */ +async function loadConfigForUpdate(configFile: string): Promise { + const existingFile = await configFileExists(configFile); + if (!existingFile.success) { + return existingFile; + } + if (!existingFile.exists) { + return { success: true, config: {} }; + } + try { const data = await readFile(configFile, 'utf-8'); - return GlobalConfigSchema.parse(JSON.parse(data)); + return { success: true, config: GlobalConfigSchema.parse(JSON.parse(data)) }; + } catch (error) { + const cause = toError(error); + return { + success: false, + error: new Error(`Config at ${configFile} is malformed: ${cause.message}`, { cause }), + }; + } +} + +type ConfigFileExistsResult = { success: true; exists: boolean } | { success: false; error: Error }; + +async function configFileExists(path: string): Promise { + try { + await stat(path); + return { success: true, exists: true }; } catch (error) { - if ((error as NodeJS.ErrnoException).code === 'ENOENT') { - return {}; + const cause = toError(error); + if ((cause as NodeJS.ErrnoException).code === 'ENOENT') { + return { success: true, exists: false }; } - throw error; + return { success: false, error: new Error(`Could not access config at ${path}: ${cause.message}`, { cause }) }; } } From 0e5cf3ad12ab56b6d8c83646ce0dfe3e3b1f827a Mon Sep 17 00:00:00 2001 From: kiwigitops Date: Tue, 26 May 2026 14:23:12 -0400 Subject: [PATCH 3/3] Propagate installation id config errors --- src/cli/__tests__/global-config.test.ts | 15 +++++++++- src/cli/cli.ts | 6 +++- .../telemetry/__tests__/telemetry.test.ts | 10 ++----- src/cli/commands/telemetry/command.ts | 8 +++-- src/cli/telemetry/config.ts | 10 +++++-- src/lib/schemas/io/global-config.ts | 29 +++++++++++++------ 6 files changed, 54 insertions(+), 24 deletions(-) diff --git a/src/cli/__tests__/global-config.test.ts b/src/cli/__tests__/global-config.test.ts index a4dc1bf8c..8d17c8598 100644 --- a/src/cli/__tests__/global-config.test.ts +++ b/src/cli/__tests__/global-config.test.ts @@ -131,6 +131,8 @@ describe('global-config', () => { it('generates installationId on first run and returns created: true', async () => { const result = await getOrCreateInstallationId(tmp.configDir, tmp.configFile); + expect(result.success).toBe(true); + if (!result.success) throw result.error; expect(result.created).toBe(true); expect(result.id).toMatch(/^[0-9a-f-]{36}$/); const config = await readGlobalConfig(tmp.configFile); @@ -142,7 +144,18 @@ describe('global-config', () => { const result = await getOrCreateInstallationId(tmp.configDir, tmp.configFile); - expect(result).toEqual({ id: 'existing-id', created: false }); + expect(result).toEqual({ success: true, id: 'existing-id', created: false }); + }); + + it('returns malformed config errors without generating an id', async () => { + await writeFile(tmp.configFile, '{ invalid json'); + + const result = await getOrCreateInstallationId(tmp.configDir, tmp.configFile); + + expect(result.success).toBe(false); + if (result.success) throw new Error('expected failure'); + expect(result.error.message).toMatch(/malformed/); + expect(await readFile(tmp.configFile, 'utf-8')).toBe('{ invalid json'); }); }); }); diff --git a/src/cli/cli.ts b/src/cli/cli.ts index e40732f52..6cf0e4c6d 100644 --- a/src/cli/cli.ts +++ b/src/cli/cli.ts @@ -215,7 +215,11 @@ export const main = async (argv: string[]) => { setupGlobalCleanup(); // Generate installationId on first run and show telemetry notice - const { created: isFirstRun } = await getOrCreateInstallationId(); + const installation = await getOrCreateInstallationId(); + if (!installation.success) { + throw installation.error; + } + const { created: isFirstRun } = installation; const program = createProgram(); diff --git a/src/cli/commands/telemetry/__tests__/telemetry.test.ts b/src/cli/commands/telemetry/__tests__/telemetry.test.ts index efdfd2f23..2f5b7b501 100644 --- a/src/cli/commands/telemetry/__tests__/telemetry.test.ts +++ b/src/cli/commands/telemetry/__tests__/telemetry.test.ts @@ -1,7 +1,7 @@ import { readGlobalConfig } from '../../../../lib/schemas/io/global-config'; import { createTempConfig } from '../../../__tests__/helpers/temp-config'; import { handleTelemetryDisable, handleTelemetryEnable, handleTelemetryStatus } from '../actions'; -import { chmod, mkdir, rm, writeFile } from 'fs/promises'; +import { writeFile } from 'fs/promises'; import { afterAll, afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; const tmp = createTempConfig('actions'); @@ -27,15 +27,9 @@ describe('telemetry actions', () => { }); it('returns false when config write fails', async () => { - await rm(tmp.testDir, { recursive: true, force: true }); - await mkdir(tmp.testDir, { recursive: true }); - await chmod(tmp.testDir, 0o444); - - const ok = await handleTelemetryDisable(tmp.configDir, tmp.configFile); + const ok = await handleTelemetryDisable(tmp.testDir + '/\0invalid', tmp.testDir + '/\0invalid/config.json'); expect(ok).toBe(false); - - await chmod(tmp.testDir, 0o755); }); }); diff --git a/src/cli/commands/telemetry/command.ts b/src/cli/commands/telemetry/command.ts index bc6033cfd..fac9daa38 100644 --- a/src/cli/commands/telemetry/command.ts +++ b/src/cli/commands/telemetry/command.ts @@ -14,14 +14,18 @@ export function registerTelemetry(program: Command) { .command('disable') .description('Disable anonymous usage analytics') .action(async () => { - await handleTelemetryDisable(); + if (!(await handleTelemetryDisable())) { + process.exitCode = 1; + } }); telemetry .command('enable') .description('Enable anonymous usage analytics') .action(async () => { - await handleTelemetryEnable(); + if (!(await handleTelemetryEnable())) { + process.exitCode = 1; + } }); telemetry diff --git a/src/cli/telemetry/config.ts b/src/cli/telemetry/config.ts index d14133484..e3ea9b954 100644 --- a/src/cli/telemetry/config.ts +++ b/src/cli/telemetry/config.ts @@ -44,15 +44,19 @@ export async function resolveTelemetryPreference(config?: GlobalConfig): Promise /** * Resolve and validate resource attributes for the current session. - * Called once at startup — the returned object is reused for every metric in the session. + * Called once at startup - the returned object is reused for every metric in the session. * Throws if any attribute fails validation (prevents PII leakage). */ export async function resolveResourceAttributes(mode: 'cli' | 'tui'): Promise { - const { id } = await getOrCreateInstallationId(); + const installation = await getOrCreateInstallationId(); + if (!installation.success) { + throw installation.error; + } + return ResourceAttributesSchema.parse({ 'service.name': 'agentcore-cli', 'service.version': PACKAGE_VERSION, - 'agentcore-cli.installation_id': id, + 'agentcore-cli.installation_id': installation.id, 'agentcore-cli.session_id': randomUUID(), 'agentcore-cli.mode': mode, 'os.type': os.type(), diff --git a/src/lib/schemas/io/global-config.ts b/src/lib/schemas/io/global-config.ts index 83916bef4..a0a5cac6c 100644 --- a/src/lib/schemas/io/global-config.ts +++ b/src/lib/schemas/io/global-config.ts @@ -1,10 +1,11 @@ +import { toError } from '../../errors/types.js'; +import type { Result } from '../../result.js'; import { readFileSync } from 'fs'; import { mkdir, readFile, stat, writeFile } from 'fs/promises'; import { randomUUID } from 'node:crypto'; import { homedir } from 'os'; import { join } from 'path'; import { z } from 'zod'; -import { toError } from '../../errors/types.js'; export const GLOBAL_CONFIG_DIR = process.env.AGENTCORE_CONFIG_DIR ?? join(homedir(), '.agentcore'); export const GLOBAL_CONFIG_FILE = join(GLOBAL_CONFIG_DIR, 'config.json'); @@ -47,7 +48,8 @@ export function readGlobalConfigSync(configFile = GLOBAL_CONFIG_FILE): GlobalCon } } -export type UpdateGlobalConfigResult = { success: true } | { success: false; error: Error }; +export type UpdateGlobalConfigResult = Result; +export type InstallationIdResult = Result<{ id: string; created: boolean }>; export async function updateGlobalConfig( partial: GlobalConfig, @@ -129,18 +131,27 @@ function mergeConfig(target: GlobalConfig, source: GlobalConfig): GlobalConfig { * `created: true` means this is the first run (ID was just generated). * * Note: concurrent first-run invocations may each generate a different ID; - * the last write wins. This is acceptable — the ID only needs to be stable + * the last write wins. This is acceptable - the ID only needs to be stable * after the first successful write, and CLI invocations are typically sequential. */ export async function getOrCreateInstallationId( configDir = GLOBAL_CONFIG_DIR, configFile = GLOBAL_CONFIG_FILE -): Promise<{ id: string; created: boolean }> { - const config = await readGlobalConfig(configFile); - if (config.installationId) { - return { id: config.installationId, created: false }; +): Promise { + const existing = await loadConfigForUpdate(configFile); + if (!existing.success) { + return existing; } + + if (existing.config.installationId) { + return { success: true, id: existing.config.installationId, created: false }; + } + const id = randomUUID(); - await updateGlobalConfig({ installationId: id }, configDir, configFile); - return { id, created: true }; + const updateResult = await updateGlobalConfig({ installationId: id }, configDir, configFile); + if (!updateResult.success) { + return updateResult; + } + + return { success: true, id, created: true }; }