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 <noreply@anthropic.com>
2026-05-26 05:57:55 +00:00
parent bbbe4651c2
commit b0c73b5439
4 changed files with 265 additions and 16 deletions
@@ -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<string, unknown>;
        const dashscope = providers.dashscope as Record<string, unknown>;
        const openai = providers.openai as Record<string, unknown>;
-        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 });
+      }
+    });
+  });
 });
@@ -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();
+  });
 });
@@ -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<string, { nested: boolean; knownFields?: string[] }> = {
+  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}.<name>.<field>). 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<string, unknown>, 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<string, unknown>): Record<string, unknown> {
  // Deep clone
  const cloned = JSON.parse(JSON.stringify(config)) as Record<string, unknown>;

-  // Mask apiKeyEnv values in providers
+  // Mask apiKey values in providers
  if (cloned.providers && typeof cloned.providers === "object") {
    const providers = cloned.providers as Record<string, unknown>;
    for (const providerName of Object.keys(providers)) {
      const provider = providers[providerName];
      if (provider && typeof provider === "object") {
        const providerObj = provider as Record<string, unknown>;
-        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)
@@ -377,7 +377,7 @@ function mergeConfig(existing: Record<string, unknown>, args: SetupArgs): Record
      : {}
  ) as Record<string, unknown>;

-  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: [] };