Merge pull request 'fix: config validation and agent name normalization (#531, #532, #533)' (#534) from fix/531-532-533 into main

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

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

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

packages/cli-workflow/src/commands/setup.ts

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