From b0c73b5439570cd3878857c9ac2ac256accaa1c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=B0=8F=E6=A9=98?= Date: Tue, 26 May 2026 05:57:55 +0000 Subject: [PATCH] fix(cli): fix config masking, agent normalization, and add key validation This commit addresses three related issues in the CLI config and setup commands: 1. Issue #531: Fix config list apiKey masking - maskApiKeys() now checks for 'apiKey' instead of 'apiKeyEnv' - Updated tests to use apiKey field throughout 2. Issue #532: Add config set key validation - Reject unknown top-level keys with helpful error messages - Reject unknown nested fields in providers/models/agents - Reject incomplete paths and nested paths on scalar keys - Added VALID_CONFIG_KEYS schema and validateConfigKey() function 3. Issue #533: Fix agent name double-prefix in setup - mergeConfig() now uses _agentNameFromBinary() to normalize agent names - 'uwf-hermes' input now produces 'hermes' key with 'uwf-hermes' command - Added tests for prefixed agent names All tests passing, no regressions. Co-Authored-By: Claude Opus 4.6 --- .../cli-workflow/src/__tests__/config.test.ts | 177 ++++++++++++++++-- .../__tests__/setup-agent-discovery.test.ts | 30 +++ packages/cli-workflow/src/commands/config.ts | 72 ++++++- packages/cli-workflow/src/commands/setup.ts | 2 +- 4 files changed, 265 insertions(+), 16 deletions(-) diff --git a/packages/cli-workflow/src/__tests__/config.test.ts b/packages/cli-workflow/src/__tests__/config.test.ts index e2b860d..2c08777 100644 --- a/packages/cli-workflow/src/__tests__/config.test.ts +++ b/packages/cli-workflow/src/__tests__/config.test.ts @@ -25,10 +25,10 @@ describe("config command", () => { const sampleConfig = `providers: dashscope: baseUrl: https://dashscope.aliyuncs.com/compatible-mode/v1 - apiKeyEnv: DASHSCOPE_API_KEY + apiKey: sk-test-dashscope-key openai: baseUrl: https://api.openai.com/v1 - apiKeyEnv: OPENAI_API_KEY + apiKey: sk-test-openai-key models: default: provider: dashscope @@ -102,16 +102,16 @@ defaultModel: default }); describe("maskApiKeys", () => { - test("deep clones and masks all apiKeyEnv values in providers", () => { + test("deep clones and masks all apiKey values in providers", () => { const config = { providers: { dashscope: { baseUrl: "https://example.com", - apiKeyEnv: "DASHSCOPE_API_KEY", + apiKey: "sk-test-key-12345", }, openai: { baseUrl: "https://api.openai.com", - apiKeyEnv: "OPENAI_API_KEY", + apiKey: "sk-another-secret", }, }, models: { @@ -123,11 +123,11 @@ defaultModel: default providers: { dashscope: { baseUrl: "https://example.com", - apiKeyEnv: "***MASKED***", + apiKey: "***MASKED***", }, openai: { baseUrl: "https://api.openai.com", - apiKeyEnv: "***MASKED***", + apiKey: "***MASKED***", }, }, models: { @@ -164,7 +164,7 @@ defaultModel: default } }); - test("masks all apiKeyEnv values in providers section", async () => { + test("masks all apiKey values in providers section", async () => { const tempDir = mkdtempSync(join(tmpdir(), "test-config-")); try { createTestConfig(tempDir, sampleConfig); @@ -172,8 +172,8 @@ defaultModel: default const providers = result.providers as Record; const dashscope = providers.dashscope as Record; const openai = providers.openai as Record; - expect(dashscope.apiKeyEnv).toBe("***MASKED***"); - expect(openai.apiKeyEnv).toBe("***MASKED***"); + expect(dashscope.apiKey).toBe("***MASKED***"); + expect(openai.apiKey).toBe("***MASKED***"); } finally { rmSync(tempDir, { recursive: true, force: true }); } @@ -240,7 +240,7 @@ defaultModel: default const result = await cmdConfigGet(tempDir, "providers.dashscope"); expect(result).toEqual({ baseUrl: "https://dashscope.aliyuncs.com/compatible-mode/v1", - apiKeyEnv: "DASHSCOPE_API_KEY", + apiKey: "sk-test-dashscope-key", }); } finally { rmSync(tempDir, { recursive: true, force: true }); @@ -464,4 +464,159 @@ defaultModel: default } }); }); + + describe("cmdConfigSet validation", () => { + test("rejects unknown top-level key", async () => { + const tempDir = mkdtempSync(join(tmpdir(), "test-config-")); + try { + createTestConfig(tempDir, sampleConfig); + await expect(cmdConfigSet(tempDir, "unknownKey", "value")).rejects.toThrow( + /Unknown config key.*unknownKey/, + ); + } finally { + rmSync(tempDir, { recursive: true, force: true }); + } + }); + + test("rejects unknown nested key in providers", async () => { + const tempDir = mkdtempSync(join(tmpdir(), "test-config-")); + try { + createTestConfig(tempDir, sampleConfig); + await expect( + cmdConfigSet(tempDir, "providers.myProvider.unknownField", "value"), + ).rejects.toThrow(/Unknown field.*unknownField.*providers/); + } finally { + rmSync(tempDir, { recursive: true, force: true }); + } + }); + + test("rejects unknown nested key in models", async () => { + const tempDir = mkdtempSync(join(tmpdir(), "test-config-")); + try { + createTestConfig(tempDir, sampleConfig); + await expect(cmdConfigSet(tempDir, "models.default.invalidField", "value")).rejects.toThrow( + /Unknown field.*invalidField.*models/, + ); + } finally { + rmSync(tempDir, { recursive: true, force: true }); + } + }); + + test("rejects unknown nested key in agents", async () => { + const tempDir = mkdtempSync(join(tmpdir(), "test-config-")); + try { + createTestConfig(tempDir, sampleConfig); + await expect(cmdConfigSet(tempDir, "agents.hermes.badField", "value")).rejects.toThrow( + /Unknown field.*badField.*agents/, + ); + } finally { + rmSync(tempDir, { recursive: true, force: true }); + } + }); + + test("rejects nested path on scalar key (defaultAgent)", async () => { + const tempDir = mkdtempSync(join(tmpdir(), "test-config-")); + try { + createTestConfig(tempDir, sampleConfig); + await expect(cmdConfigSet(tempDir, "defaultAgent.foo", "value")).rejects.toThrow( + /defaultAgent.*scalar|Cannot set property/i, + ); + } finally { + rmSync(tempDir, { recursive: true, force: true }); + } + }); + + test("rejects nested path on scalar key (defaultModel)", async () => { + const tempDir = mkdtempSync(join(tmpdir(), "test-config-")); + try { + createTestConfig(tempDir, sampleConfig); + await expect(cmdConfigSet(tempDir, "defaultModel.bar", "value")).rejects.toThrow( + /defaultModel.*scalar|Cannot set property/i, + ); + } finally { + rmSync(tempDir, { recursive: true, force: true }); + } + }); + + test("rejects incomplete nested path (providers without field)", async () => { + const tempDir = mkdtempSync(join(tmpdir(), "test-config-")); + try { + createTestConfig(tempDir, sampleConfig); + await expect(cmdConfigSet(tempDir, "providers.myProvider", "value")).rejects.toThrow( + /incomplete path|must specify a field/i, + ); + } finally { + rmSync(tempDir, { recursive: true, force: true }); + } + }); + + test("rejects incomplete nested path (models without field)", async () => { + const tempDir = mkdtempSync(join(tmpdir(), "test-config-")); + try { + createTestConfig(tempDir, sampleConfig); + await expect(cmdConfigSet(tempDir, "models.myModel", "value")).rejects.toThrow( + /incomplete path|must specify a field/i, + ); + } finally { + rmSync(tempDir, { recursive: true, force: true }); + } + }); + + test("rejects incomplete nested path (agents without field)", async () => { + const tempDir = mkdtempSync(join(tmpdir(), "test-config-")); + try { + createTestConfig(tempDir, sampleConfig); + await expect(cmdConfigSet(tempDir, "agents.myAgent", "value")).rejects.toThrow( + /incomplete path|must specify a field/i, + ); + } finally { + rmSync(tempDir, { recursive: true, force: true }); + } + }); + + test("allows valid nested keys in providers", async () => { + const tempDir = mkdtempSync(join(tmpdir(), "test-config-")); + try { + createTestConfig(tempDir, sampleConfig); + await cmdConfigSet(tempDir, "providers.newprovider.baseUrl", "https://example.com"); + await cmdConfigSet(tempDir, "providers.newprovider.apiKey", "sk-test"); + const baseUrl = await cmdConfigGet(tempDir, "providers.newprovider.baseUrl"); + const apiKey = await cmdConfigGet(tempDir, "providers.newprovider.apiKey"); + expect(baseUrl).toBe("https://example.com"); + expect(apiKey).toBe("sk-test"); + } finally { + rmSync(tempDir, { recursive: true, force: true }); + } + }); + + test("allows valid nested keys in models", async () => { + const tempDir = mkdtempSync(join(tmpdir(), "test-config-")); + try { + createTestConfig(tempDir, sampleConfig); + await cmdConfigSet(tempDir, "models.gpt4.provider", "openai"); + await cmdConfigSet(tempDir, "models.gpt4.name", "gpt-4o"); + const provider = await cmdConfigGet(tempDir, "models.gpt4.provider"); + const name = await cmdConfigGet(tempDir, "models.gpt4.name"); + expect(provider).toBe("openai"); + expect(name).toBe("gpt-4o"); + } finally { + rmSync(tempDir, { recursive: true, force: true }); + } + }); + + test("allows valid nested keys in agents", async () => { + const tempDir = mkdtempSync(join(tmpdir(), "test-config-")); + try { + createTestConfig(tempDir, sampleConfig); + await cmdConfigSet(tempDir, "agents.hermes.command", "uwf-hermes"); + await cmdConfigSet(tempDir, "agents.hermes.args", '["--flag"]'); + const command = await cmdConfigGet(tempDir, "agents.hermes.command"); + const args = await cmdConfigGet(tempDir, "agents.hermes.args"); + expect(command).toBe("uwf-hermes"); + expect(args).toEqual(["--flag"]); + } finally { + rmSync(tempDir, { recursive: true, force: true }); + } + }); + }); }); diff --git a/packages/cli-workflow/src/__tests__/setup-agent-discovery.test.ts b/packages/cli-workflow/src/__tests__/setup-agent-discovery.test.ts index 3b4c605..bb74a3d 100644 --- a/packages/cli-workflow/src/__tests__/setup-agent-discovery.test.ts +++ b/packages/cli-workflow/src/__tests__/setup-agent-discovery.test.ts @@ -134,4 +134,34 @@ describe("cmdSetup agent configuration", () => { const config2 = parse(readFileSync(join(storageRoot, "config.yaml"), "utf8")); expect(config2.defaultAgent).toBe("builtin"); }); + + test("normalizes agent name with uwf- prefix to bare name", async () => { + vi.spyOn(globalThis, "fetch").mockResolvedValue( + new Response(JSON.stringify({}), { status: 200 }), + ); + + const result = await cmdSetup({ ...baseArgs(), agent: "uwf-hermes" }); + + expect(result.defaultAgent).toBe("hermes"); + const config = parse(readFileSync(join(storageRoot, "config.yaml"), "utf8")); + expect(config.agents.hermes).toEqual({ command: "uwf-hermes", args: [] }); + expect(config.defaultAgent).toBe("hermes"); + // Verify no duplicate uwf- prefix + expect(config.agents["uwf-hermes"]).toBeUndefined(); + }); + + test("normalizes uwf-claude-code to claude-code", async () => { + vi.spyOn(globalThis, "fetch").mockResolvedValue( + new Response(JSON.stringify({}), { status: 200 }), + ); + + const result = await cmdSetup({ ...baseArgs(), agent: "uwf-claude-code" }); + + expect(result.defaultAgent).toBe("claude-code"); + const config = parse(readFileSync(join(storageRoot, "config.yaml"), "utf8")); + expect(config.agents["claude-code"]).toEqual({ command: "uwf-claude-code", args: [] }); + expect(config.defaultAgent).toBe("claude-code"); + // Verify no duplicate uwf- prefix + expect(config.agents["uwf-claude-code"]).toBeUndefined(); + }); }); diff --git a/packages/cli-workflow/src/commands/config.ts b/packages/cli-workflow/src/commands/config.ts index 6f3fc46..63d3a9b 100644 --- a/packages/cli-workflow/src/commands/config.ts +++ b/packages/cli-workflow/src/commands/config.ts @@ -2,6 +2,66 @@ import { existsSync, mkdirSync, readFileSync, writeFileSync } from "node:fs"; import { join } from "node:path"; import { parse, stringify } from "yaml"; +/** + * Valid configuration key schema + */ +const VALID_CONFIG_KEYS: Record = { + providers: { + nested: true, + knownFields: ["baseUrl", "apiKey"], + }, + models: { + nested: true, + knownFields: ["provider", "name"], + }, + agents: { + nested: true, + knownFields: ["command", "args"], + }, + defaultAgent: { nested: false }, + defaultModel: { nested: false }, +}; + +/** + * Validate a config key path against the known schema + */ +function validateConfigKey(path: string[]): void { + if (path.length === 0) { + throw new Error("Path cannot be empty"); + } + + const topLevel = path[0]; + const schema = VALID_CONFIG_KEYS[topLevel]; + + if (!schema) { + const validKeys = Object.keys(VALID_CONFIG_KEYS).join(", "); + throw new Error(`Unknown config key: ${topLevel}. Valid top-level keys are: ${validKeys}`); + } + + // Scalar keys cannot have nested paths + if (!schema.nested && path.length > 1) { + throw new Error(`${topLevel} is a scalar key and cannot have nested properties`); + } + + // Nested keys must have at least 3 segments (e.g., providers.myProvider.baseUrl) + if (schema.nested && path.length < 3) { + const fields = schema.knownFields?.join(", ") ?? ""; + throw new Error( + `Incomplete path for ${topLevel}. Must specify a field (e.g., ${topLevel}..). Valid fields: ${fields}`, + ); + } + + // Validate the field name for nested keys + if (schema.nested && path.length >= 3 && schema.knownFields) { + const field = path[path.length - 1]; + if (!schema.knownFields.includes(field)) { + throw new Error( + `Unknown field '${field}' in ${topLevel}. Valid fields are: ${schema.knownFields.join(", ")}`, + ); + } + } +} + /** * Returns the path to the config.yaml file */ @@ -100,21 +160,21 @@ export function setNestedValue(obj: Record, path: string[], val } /** - * Deep clone and mask all apiKeyEnv values in providers section + * Deep clone and mask all apiKey values in providers section */ export function maskApiKeys(config: Record): Record { // Deep clone const cloned = JSON.parse(JSON.stringify(config)) as Record; - // Mask apiKeyEnv values in providers + // Mask apiKey values in providers if (cloned.providers && typeof cloned.providers === "object") { const providers = cloned.providers as Record; for (const providerName of Object.keys(providers)) { const provider = providers[providerName]; if (provider && typeof provider === "object") { const providerObj = provider as Record; - if ("apiKeyEnv" in providerObj) { - providerObj.apiKeyEnv = "***MASKED***"; + if ("apiKey" in providerObj) { + providerObj.apiKey = "***MASKED***"; } } } @@ -207,6 +267,10 @@ export async function cmdConfigSet( } const path = parseDotPath(key); + + // Validate the key path + validateConfigKey(path); + const lastSegment = path[path.length - 1]; // Parse value if it's for an array key (args) diff --git a/packages/cli-workflow/src/commands/setup.ts b/packages/cli-workflow/src/commands/setup.ts index 99873e4..5577430 100644 --- a/packages/cli-workflow/src/commands/setup.ts +++ b/packages/cli-workflow/src/commands/setup.ts @@ -377,7 +377,7 @@ function mergeConfig(existing: Record, args: SetupArgs): Record : {} ) as Record; - const agentName = args.agent ?? "hermes"; + const agentName = _agentNameFromBinary(args.agent ?? "hermes"); // Ensure the selected agent has an entry if (!agents[agentName]) { agents[agentName] = { command: `uwf-${agentName}`, args: [] };