diff --git a/docs/PERFTRACE.md b/docs/PERFTRACE.md index 984ace052..f631cb673 100644 --- a/docs/PERFTRACE.md +++ b/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. diff --git a/src/config.test.ts b/src/config.test.ts index 06b3bb79a..72a92d167 100644 --- a/src/config.test.ts +++ b/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 reopens a known session and skips the initial task", async () => { const cwd = await emptyCwd(); const home = await tempHome(); diff --git a/src/config/index.ts b/src/config/index.ts index 6d14de5a5..79d91d2b4 100644 --- a/src/config/index.ts +++ b/src/config/index.ts @@ -837,16 +837,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.`, @@ -947,41 +943,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 ` - // reopens that session (alias for `corbits resume `); 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("--")) { diff --git a/src/perf/otel-config.test.ts b/src/perf/otel-config.test.ts index aadd1095a..60db9604c 100644 --- a/src/perf/otel-config.test.ts +++ b/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, ); diff --git a/src/perf/otel-config.ts b/src/perf/otel-config.ts index 2b3114d93..25370d765 100644 --- a/src/perf/otel-config.ts +++ b/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; } @@ -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 = {}; - 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.