Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 15 additions & 5 deletions src/cli/__tests__/global-config.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 } });

Expand All @@ -106,14 +106,24 @@ describe('global-config', () => {
});
});

it('returns false on write failures', async () => {
const ok = await updateGlobalConfig(
it('does not overwrite malformed config and reports why', async () => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice simple test!

await writeFile(tmp.configFile, '{ invalid json');

const result = await updateGlobalConfig({ telemetry: { enabled: true } }, tmp.configDir, tmp.configFile);

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 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);
});
});

Expand Down
12 changes: 6 additions & 6 deletions src/cli/commands/telemetry/actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,18 +10,18 @@ export async function handleTelemetryDisable(
configDir = GLOBAL_CONFIG_DIR,
configFile = GLOBAL_CONFIG_FILE
): Promise<boolean> {
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<boolean> {
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<void> {
Expand Down
67 changes: 59 additions & 8 deletions src/lib/schemas/io/global-config.ts
Original file line number Diff line number Diff line change
@@ -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');
Expand Down Expand Up @@ -46,20 +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<boolean> {
try {
const existing = await readGlobalConfig(configFile);
const merged: GlobalConfig = mergeConfig(existing, partial);
): Promise<UpdateGlobalConfigResult> {
// 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}`) };
}
}

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<LoadConfigResult> {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this logic feels a bit convoluted to me. Do you think it makes sense to directly check if the file exists (via fs.exists or something), then use that instead of the error?

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 { 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<ConfigFileExistsResult> {
try {
await stat(path);
return { success: true, exists: true };
} catch (error) {
const cause = toError(error);
if ((cause as NodeJS.ErrnoException).code === 'ENOENT') {
return { success: true, exists: false };
}
return { success: false, error: new Error(`Could not access config at ${path}: ${cause.message}`, { cause }) };
}
}

Expand Down