Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion docs/PERFTRACE.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
11 changes: 11 additions & 0 deletions src/config.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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() });
Expand All @@ -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();
Expand Down
58 changes: 25 additions & 33 deletions src/config/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.`,
Expand Down Expand Up @@ -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 <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("--")) {
Expand Down
45 changes: 43 additions & 2 deletions src/perf/otel-config.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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,
);
Expand Down
36 changes: 19 additions & 17 deletions src/perf/otel-config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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>;
}
Expand Down Expand Up @@ -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.
*/
Expand Down Expand Up @@ -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,
Expand All @@ -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.
Expand Down
Loading