Merge pull request #1438 from corbitsdev/cl-10672-config-sprawl-cut-dead-options-merge-duplicate

refactor(config): fold duplicate flag spellings into single parse paths

Author: TheGreatAxios

docs/PERFTRACE.md

@@ -66,7 +66,8 @@ Precedence:
   `resourceAttributes["service.name"]` > `corbits-code`. After merge,
   `resourceAttributes["service.name"]` is always set to the resolved name so
   the two never diverge.
-- **resourceAttributes:** settings merged with env; env wins on key conflict
+- **resourceAttributes:** when `OTEL_RESOURCE_ATTRIBUTES` is set, it fully
+  replaces settings attributes (including set-but-empty)
 - **enabling export:** setting an endpoint (settings or env) enables export;
   `otel.enabled` need not be set. Service name or resource attributes alone do
   not enable export.

src/config.test.ts

@@ -572,6 +572,8 @@ describe("loadConfig", () => {
     { argv: ["resume", "--pick"] },
     { argv: ["resume"] },
     { argv: ["continue", "--list"] },
+    { argv: ["resume", "--list"] },
+    { argv: ["resume", "--pick", "--list"] },
   ] as const)("$argv opens the session picker", async ({ argv }) => {
     const cwd = await emptyCwd();
     const config = await loadFor(cwd, argv, { home: await tempHome() });
@@ -581,6 +583,15 @@ describe("loadConfig", () => {
     expect(config.skipInitialTask).toBe(true);
   });
 
+  test("CL-10672 keeps --pick/--list out of flow as unrecognized flags", async () => {
+    await expect(
+      loadConfig(["--pick"], { globalSettingsPath: NO_SETTINGS }),
+    ).rejects.toThrow("unrecognized flag: --pick");
+    await expect(
+      loadConfig(["--list"], { globalSettingsPath: NO_SETTINGS }),
+    ).rejects.toThrow("unrecognized flag: --list");
+  });
+
   test("resume <id> reopens a known session and skips the initial task", async () => {
     const cwd = await emptyCwd();
     const home = await tempHome();

src/config/index.ts

@@ -834,16 +834,12 @@ export async function loadConfig(
     command = "exec";
     args.shift();
   } else if (leading === "resume" || leading === "continue") {
-    command = "tui";
     args.shift();
     // Bare resume opens the list; a session id is the only direct-resume path.
     // Invalid non-flag positionals error instead of falling through (a
     // free-form token would otherwise become task while skipInitialTask is set).
     const next = args.slice()[0];
-    if (next === "--pick" || next === "--list") {
-      resumeMode = "pick";
-      args.shift();
-    } else if (next !== undefined && !isFlagToken(next)) {
+    if (next !== undefined && !isFlagToken(next)) {
       if (!isSessionId(next)) {
         throw new Error(
           `'${next}' is not a session id. Use a UUID session id or \`corbits resume\` to choose.`,
@@ -944,41 +940,37 @@ export async function loadConfig(
       continue;
     }
     if (arg === "-p") {
-      command = "exec";
       continue;
     }
-    if (arg === "--resume") {
-      if (resumeMode === "id") {
-        throw new Error("cannot combine a session id with --resume");
-      }
-      // Optional session id on the interactive path: `corbits --resume <uuid>`
-      // reopens that session (alias for `corbits resume <uuid>`); bare
-      // `--resume` opens the picker. Exec / `-p` require the id — headless has
-      // no picker. A non-id token errors instead of leaking into task text.
-      const next = args[i + 1];
-      if (next !== undefined && !isFlagToken(next)) {
-        if (!isSessionId(next)) {
+    if (arg === "--resume" || arg === "--pick" || arg === "--list") {
+      if (arg === "--resume") {
+        if (resumeMode === "id") {
+          throw new Error("cannot combine a session id with --resume");
+        }
+        const next = args[i + 1];
+        if (next !== undefined && !isFlagToken(next)) {
+          if (!isSessionId(next)) {
+            throw new Error(
+              `'${next}' is not a session id. Use a UUID session id or \`corbits resume\` to choose.`,
+            );
+          }
+          resumeMode = "id";
+          resumeSessionId = next;
+          i++;
+        } else if (command === "exec") {
           throw new Error(
-            `'${next}' is not a session id. Use a UUID session id or \`corbits resume\` to choose.`,
+            `--resume requires a session id in exec mode. Use \`${COMMAND_NAME} resume\` to choose a session.`,
           );
+        } else {
+          resumeMode = "pick";
         }
-        resumeMode = "id";
-        resumeSessionId = next;
-        i++;
-      } else if (command === "exec") {
-        throw new Error(
-          `--resume requires a session id in exec mode. Use \`${COMMAND_NAME} resume\` to choose a session.`,
-        );
-      } else {
-        resumeMode = "pick";
-      }
-      continue;
-    }
-    if ((arg === "--pick" || arg === "--list") && resumeMode !== undefined) {
-      if (resumeMode === "id") {
+      } else if (resumeMode === "id") {
         throw new Error("cannot combine a session id with --pick/--list");
+      } else if (resumeMode !== undefined) {
+        resumeMode = "pick";
+      } else {
+        throw new Error(`unrecognized flag: ${arg}`);
       }
-      resumeMode = "pick";
       continue;
     }
     if (arg.startsWith("--")) {

src/perf/otel-config.test.ts

@@ -197,7 +197,7 @@ describe("resolveOtelExportConfig", () => {
     }
   });
 
-  test("resource attributes merge with env winning on conflict", () => {
+  test("env resource attributes replace settings wholesale (CL-10672)", () => {
     const result = resolveOtelExportConfig(
       baseSettings({
         endpoint: "https://c.example",
@@ -213,7 +213,48 @@ describe("resolveOtelExportConfig", () => {
       expect(result.config.resourceAttributes["deployment.environment"]).toBe(
         "prod",
       );
-      expect(result.config.resourceAttributes.team).toBe("corbits");
+      expect(result.config.resourceAttributes.team).toBeUndefined();
+      expect(result.config.resourceAttributes["service.name"]).toBe(
+        DEFAULT_OTEL_SERVICE_NAME,
+      );
+    }
+  });
+
+  test("settings resource attributes can carry a service.name the env never overrides", () => {
+    const result = resolveOtelExportConfig(
+      baseSettings({
+        endpoint: "https://c.example",
+        resourceAttributes: {
+          "deployment.environment": "settings-only",
+          team: "corbits",
+        },
+      }),
+      { [OTEL_ENV.resourceAttributes]: "service.name=from-env" },
+    );
+    expect(result.ok).toBe(true);
+    if (result.ok && result.config.enabled) {
+      expect(result.config.resourceAttributes.team).toBeUndefined();
+      expect(result.config.serviceName).toBe("from-env");
+    }
+  });
+
+  test("set-but-empty env resource attributes replace with nothing (regression: no settings fallback)", () => {
+    const result = resolveOtelExportConfig(
+      baseSettings({
+        endpoint: "https://c.example",
+        resourceAttributes: {
+          team: "corbits",
+          "deployment.environment": "dev",
+        },
+      }),
+      { [OTEL_ENV.resourceAttributes]: "" },
+    );
+    expect(result.ok).toBe(true);
+    if (result.ok && result.config.enabled) {
+      expect(result.config.resourceAttributes.team).toBeUndefined();
+      expect(
+        result.config.resourceAttributes["deployment.environment"],
+      ).toBeUndefined();
       expect(result.config.resourceAttributes["service.name"]).toBe(
         DEFAULT_OTEL_SERVICE_NAME,
       );

src/perf/otel-config.ts

@@ -36,7 +36,8 @@ export interface OtelSettings {
   /** Resource service.name. Env OTEL_SERVICE_NAME overrides. */
   serviceName?: string;
   /**
-   * Extra resource attributes (merged; env keys win). Non-secret labels only.
+   * Extra resource attributes. Non-secret labels only. Replaced wholesale by
+   * OTEL_RESOURCE_ATTRIBUTES when the env var is set; never merged per-key.
    */
   resourceAttributes?: Record<string, string>;
 }
@@ -212,8 +213,9 @@ function validateStringMap(
  * - endpoint: env > settings
  * - headers: env list fully replaces settings when set; else settings
  * - serviceName: env > settings.serviceName > attrs["service.name"] > default
- * - resourceAttributes: settings then env (env wins on conflict); after merge,
- *   attrs["service.name"] is synced to the resolved serviceName
+ * - resourceAttributes: env list fully replaces settings when OTEL_RESOURCE_ATTRIBUTES
+ *   is set; else settings. After resolution, attrs["service.name"] is synced
+ *   to the resolved serviceName
  *
  * No endpoint → disabled (ok); invalid or malformed → not ok.
  */
@@ -294,19 +296,6 @@ export function resolveOtelExportConfig(
   }
 
   let resourceAttributes: Record<string, string> = {};
-  const settingsAttrs = validateStringMap(
-    otel?.resourceAttributes,
-    "settings.otel.resourceAttributes",
-  );
-  if (!settingsAttrs.ok) {
-    return {
-      ok: false,
-      code: OTEL_CONFIG_INVALID,
-      message: settingsAttrs.message,
-    };
-  }
-  resourceAttributes = { ...settingsAttrs.value };
-
   if (envResourceAttrsRaw !== undefined) {
     const parsed = parseOtelKeyValueList(
       envResourceAttrsRaw,
@@ -315,7 +304,20 @@ export function resolveOtelExportConfig(
     if (!parsed.ok) {
       return { ok: false, code: OTEL_CONFIG_INVALID, message: parsed.message };
     }
-    resourceAttributes = { ...resourceAttributes, ...parsed.value };
+    resourceAttributes = parsed.value;
+  } else {
+    const settingsAttrs = validateStringMap(
+      otel?.resourceAttributes,
+      "settings.otel.resourceAttributes",
+    );
+    if (!settingsAttrs.ok) {
+      return {
+        ok: false,
+        code: OTEL_CONFIG_INVALID,
+        message: settingsAttrs.message,
+      };
+    }
+    resourceAttributes = settingsAttrs.value;
   }
 
   // Sync attrs["service.name"] to the resolved name so the two never diverge.