fix(coding-agent): treat uppercase config values as literals
closes #5661
This commit is contained in:
@@ -37,7 +37,7 @@ describe("config value env var syntax migration", () => {
|
||||
}
|
||||
}
|
||||
|
||||
it("rewrites legacy uppercase auth.json API key values to explicit env references", () => {
|
||||
it("leaves uppercase auth.json API key values unchanged", () => {
|
||||
const agentDir = createAgentDir();
|
||||
fs.writeFileSync(
|
||||
path.join(agentDir, "auth.json"),
|
||||
@@ -61,19 +61,17 @@ describe("config value env var syntax migration", () => {
|
||||
string,
|
||||
Record<string, unknown>
|
||||
>;
|
||||
expect(migrated.anthropic.key).toBe("$ANTHROPIC_API_KEY");
|
||||
expect(migrated.anthropic.key).toBe("ANTHROPIC_API_KEY");
|
||||
expect(migrated.openai.key).toBe("$OPENAI_API_KEY");
|
||||
expect(migrated.opencode.key).toBe("public");
|
||||
expect(migrated.github.access).toBe("ACCESS_TOKEN");
|
||||
const logMessage = String(logSpy.mock.calls[0]?.[0] ?? "");
|
||||
expect(logMessage).toContain("explicit $ENV_VAR syntax");
|
||||
expect(logMessage).toContain('auth.json["anthropic"].key: ANTHROPIC_API_KEY -> $ANTHROPIC_API_KEY');
|
||||
expect(logSpy).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it.each([
|
||||
["malformed", '{\n "providers": {\n'],
|
||||
["blank", ""],
|
||||
])("does not throw on %s models.json during config migration", (_name, content) => {
|
||||
])("does not throw on %s models.json during migrations", (_name, content) => {
|
||||
const agentDir = createAgentDir();
|
||||
const modelsPath = path.join(agentDir, "models.json");
|
||||
fs.writeFileSync(modelsPath, content, "utf-8");
|
||||
@@ -87,71 +85,93 @@ describe("config value env var syntax migration", () => {
|
||||
expect(loadError).toContain(`File: ${modelsPath}`);
|
||||
});
|
||||
|
||||
it("rewrites legacy uppercase models.json API key and header values", () => {
|
||||
it("leaves uppercase models.json API key and header values unchanged", async () => {
|
||||
const agentDir = createAgentDir();
|
||||
fs.writeFileSync(
|
||||
path.join(agentDir, "models.json"),
|
||||
`${JSON.stringify(
|
||||
{
|
||||
providers: {
|
||||
"custom-provider": {
|
||||
baseUrl: "https://example.com/v1",
|
||||
apiKey: "CUSTOM_API_KEY",
|
||||
api: "openai-completions",
|
||||
headers: {
|
||||
"x-api-key": "HEADER_API_KEY",
|
||||
"x-literal": "literal",
|
||||
},
|
||||
models: [
|
||||
{
|
||||
id: "model-a",
|
||||
headers: { "x-model-key": "MODEL_API_KEY" },
|
||||
const envKeys = ["CUSTOM_API_KEY", "HEADER_API_KEY", "MODEL_API_KEY", "OVERRIDE_API_KEY"];
|
||||
const savedEnv: Record<string, string | undefined> = {};
|
||||
for (const key of envKeys) {
|
||||
savedEnv[key] = process.env[key];
|
||||
process.env[key] = `env-${key}`;
|
||||
}
|
||||
|
||||
try {
|
||||
fs.writeFileSync(
|
||||
path.join(agentDir, "models.json"),
|
||||
`${JSON.stringify(
|
||||
{
|
||||
providers: {
|
||||
"custom-provider": {
|
||||
baseUrl: "https://example.com/v1",
|
||||
apiKey: "CUSTOM_API_KEY",
|
||||
api: "openai-completions",
|
||||
headers: {
|
||||
"x-api-key": "HEADER_API_KEY",
|
||||
"x-literal": "literal",
|
||||
},
|
||||
models: [
|
||||
{
|
||||
id: "model-a",
|
||||
headers: { "x-model-key": "MODEL_API_KEY" },
|
||||
},
|
||||
],
|
||||
modelOverrides: {
|
||||
"model-b": { headers: { "x-override-key": "OVERRIDE_API_KEY" } },
|
||||
},
|
||||
],
|
||||
modelOverrides: {
|
||||
"model-b": { headers: { "x-override-key": "OVERRIDE_API_KEY" } },
|
||||
},
|
||||
},
|
||||
},
|
||||
null,
|
||||
2,
|
||||
)}\n`,
|
||||
"utf-8",
|
||||
);
|
||||
const logSpy = vi.spyOn(console, "log").mockImplementation(() => {});
|
||||
|
||||
withAgentDir(agentDir, () => runMigrations(agentDir));
|
||||
|
||||
const migrated = JSON.parse(fs.readFileSync(path.join(agentDir, "models.json"), "utf-8")) as {
|
||||
providers: Record<
|
||||
string,
|
||||
{
|
||||
apiKey?: string;
|
||||
headers?: Record<string, string>;
|
||||
models?: Array<{ headers?: Record<string, string> }>;
|
||||
modelOverrides?: Record<string, { headers?: Record<string, string> }>;
|
||||
}
|
||||
>;
|
||||
};
|
||||
const provider = migrated.providers["custom-provider"]!;
|
||||
expect(provider.apiKey).toBe("CUSTOM_API_KEY");
|
||||
expect(provider.headers?.["x-api-key"]).toBe("HEADER_API_KEY");
|
||||
expect(provider.headers?.["x-literal"]).toBe("literal");
|
||||
expect(provider.models?.[0]?.headers?.["x-model-key"]).toBe("MODEL_API_KEY");
|
||||
expect(provider.modelOverrides?.["model-b"]?.headers?.["x-override-key"]).toBe("OVERRIDE_API_KEY");
|
||||
expect(logSpy).not.toHaveBeenCalled();
|
||||
|
||||
const registry = ModelRegistry.create(
|
||||
AuthStorage.create(path.join(agentDir, "auth.json")),
|
||||
path.join(agentDir, "models.json"),
|
||||
);
|
||||
const model = registry.find("custom-provider", "model-a");
|
||||
expect(model).toBeDefined();
|
||||
expect(await registry.getApiKeyForProvider("custom-provider")).toBe("CUSTOM_API_KEY");
|
||||
expect(await registry.getApiKeyAndHeaders(model!)).toMatchObject({
|
||||
ok: true,
|
||||
apiKey: "CUSTOM_API_KEY",
|
||||
headers: {
|
||||
"x-api-key": "HEADER_API_KEY",
|
||||
"x-literal": "literal",
|
||||
"x-model-key": "MODEL_API_KEY",
|
||||
},
|
||||
null,
|
||||
2,
|
||||
)}\n`,
|
||||
"utf-8",
|
||||
);
|
||||
const logSpy = vi.spyOn(console, "log").mockImplementation(() => {});
|
||||
|
||||
withAgentDir(agentDir, () => runMigrations(agentDir));
|
||||
|
||||
const migrated = JSON.parse(fs.readFileSync(path.join(agentDir, "models.json"), "utf-8")) as {
|
||||
providers: Record<
|
||||
string,
|
||||
{
|
||||
apiKey?: string;
|
||||
headers?: Record<string, string>;
|
||||
models?: Array<{ headers?: Record<string, string> }>;
|
||||
modelOverrides?: Record<string, { headers?: Record<string, string> }>;
|
||||
});
|
||||
} finally {
|
||||
for (const key of envKeys) {
|
||||
if (savedEnv[key] === undefined) {
|
||||
delete process.env[key];
|
||||
} else {
|
||||
process.env[key] = savedEnv[key];
|
||||
}
|
||||
>;
|
||||
};
|
||||
const provider = migrated.providers["custom-provider"]!;
|
||||
expect(provider.apiKey).toBe("$CUSTOM_API_KEY");
|
||||
expect(provider.headers?.["x-api-key"]).toBe("$HEADER_API_KEY");
|
||||
expect(provider.headers?.["x-literal"]).toBe("literal");
|
||||
expect(provider.models?.[0]?.headers?.["x-model-key"]).toBe("$MODEL_API_KEY");
|
||||
expect(provider.modelOverrides?.["model-b"]?.headers?.["x-override-key"]).toBe("$OVERRIDE_API_KEY");
|
||||
const logMessage = String(logSpy.mock.calls[0]?.[0] ?? "");
|
||||
expect(logMessage).toContain(
|
||||
'models.json.providers["custom-provider"].apiKey: CUSTOM_API_KEY -> $CUSTOM_API_KEY',
|
||||
);
|
||||
expect(logMessage).toContain(
|
||||
'models.json.providers["custom-provider"].headers["x-api-key"]: HEADER_API_KEY -> $HEADER_API_KEY',
|
||||
);
|
||||
expect(logMessage).toContain(
|
||||
'models.json.providers["custom-provider"].models["model-a"].headers["x-model-key"]: MODEL_API_KEY -> $MODEL_API_KEY',
|
||||
);
|
||||
expect(logMessage).toContain(
|
||||
'models.json.providers["custom-provider"].modelOverrides["model-b"].headers["x-override-key"]: OVERRIDE_API_KEY -> $OVERRIDE_API_KEY',
|
||||
);
|
||||
}
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
@@ -7,7 +7,6 @@ import { getOAuthProvider } from "@earendil-works/pi-ai/oauth";
|
||||
import { afterEach, beforeEach, describe, expect, test, vi } from "vitest";
|
||||
import { AuthStorage } from "../src/core/auth-storage.ts";
|
||||
import { clearApiKeyCache, ModelRegistry, type ProviderConfigInput } from "../src/core/model-registry.ts";
|
||||
import { clearDeprecationWarningsForTests } from "../src/utils/deprecation.ts";
|
||||
|
||||
describe("ModelRegistry", () => {
|
||||
let tempDir: string;
|
||||
@@ -19,7 +18,6 @@ describe("ModelRegistry", () => {
|
||||
mkdirSync(tempDir, { recursive: true });
|
||||
modelsJsonPath = join(tempDir, "models.json");
|
||||
authStorage = AuthStorage.create(join(tempDir, "auth.json"));
|
||||
clearDeprecationWarningsForTests();
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
@@ -27,7 +25,6 @@ describe("ModelRegistry", () => {
|
||||
rmSync(tempDir, { recursive: true });
|
||||
}
|
||||
clearApiKeyCache();
|
||||
clearDeprecationWarningsForTests();
|
||||
vi.restoreAllMocks();
|
||||
});
|
||||
|
||||
@@ -896,26 +893,55 @@ describe("ModelRegistry", () => {
|
||||
expect(registry.getProviderDisplayName("oauth-provider")).toBe("OAuth Provider");
|
||||
});
|
||||
|
||||
test("registerProvider warns and temporarily treats uppercase apiKey as an env reference", async () => {
|
||||
const originalEnv = process.env.CUSTOM_NAME;
|
||||
process.env.CUSTOM_NAME = "legacy-env-key";
|
||||
test("registerProvider treats uppercase apiKey and headers as literals", async () => {
|
||||
const envKeys = ["CUSTOM_NAME", "BEARER", "MODEL_TOKEN"];
|
||||
const savedEnv: Record<string, string | undefined> = {};
|
||||
for (const key of envKeys) {
|
||||
savedEnv[key] = process.env[key];
|
||||
process.env[key] = `env-${key}`;
|
||||
}
|
||||
const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {});
|
||||
|
||||
try {
|
||||
const registry = ModelRegistry.create(authStorage, modelsJsonPath);
|
||||
|
||||
registry.registerProvider("legacy-provider", {
|
||||
registry.registerProvider("literal-provider", {
|
||||
...providerConfig("https://provider.test/v1", [{ id: "demo-model" }], "openai-completions"),
|
||||
apiKey: "CUSTOM_NAME",
|
||||
headers: { Authorization: "BEARER" },
|
||||
models: [
|
||||
{
|
||||
id: "demo-model",
|
||||
name: "demo-model",
|
||||
reasoning: false,
|
||||
input: ["text"],
|
||||
cost: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0 },
|
||||
contextWindow: 100000,
|
||||
maxTokens: 8000,
|
||||
headers: { "x-model-token": "MODEL_TOKEN" },
|
||||
},
|
||||
],
|
||||
});
|
||||
|
||||
expect(await registry.getApiKeyForProvider("legacy-provider")).toBe("legacy-env-key");
|
||||
expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('Pass "$CUSTOM_NAME" instead'));
|
||||
expect(await registry.getApiKeyForProvider("literal-provider")).toBe("CUSTOM_NAME");
|
||||
const model = registry.find("literal-provider", "demo-model");
|
||||
expect(model).toBeDefined();
|
||||
expect(await registry.getApiKeyAndHeaders(model!)).toMatchObject({
|
||||
ok: true,
|
||||
apiKey: "CUSTOM_NAME",
|
||||
headers: {
|
||||
Authorization: "BEARER",
|
||||
"x-model-token": "MODEL_TOKEN",
|
||||
},
|
||||
});
|
||||
expect(warnSpy).not.toHaveBeenCalled();
|
||||
} finally {
|
||||
if (originalEnv === undefined) {
|
||||
delete process.env.CUSTOM_NAME;
|
||||
} else {
|
||||
process.env.CUSTOM_NAME = originalEnv;
|
||||
for (const key of envKeys) {
|
||||
if (savedEnv[key] === undefined) {
|
||||
delete process.env[key];
|
||||
} else {
|
||||
process.env[key] = savedEnv[key];
|
||||
}
|
||||
}
|
||||
}
|
||||
});
|
||||
|
||||
@@ -0,0 +1,91 @@
|
||||
import { readFileSync, writeFileSync } from "node:fs";
|
||||
import { join } from "node:path";
|
||||
import { afterEach, describe, expect, it } from "vitest";
|
||||
import { ENV_AGENT_DIR } from "../../../src/config.ts";
|
||||
import { AuthStorage } from "../../../src/core/auth-storage.ts";
|
||||
import { ModelRegistry } from "../../../src/core/model-registry.ts";
|
||||
import { runMigrations } from "../../../src/migrations.ts";
|
||||
import { createHarness } from "../harness.ts";
|
||||
|
||||
describe("regression #5661: uppercase models.json header values", () => {
|
||||
const cleanups: Array<() => void> = [];
|
||||
|
||||
afterEach(() => {
|
||||
while (cleanups.length > 0) {
|
||||
cleanups.pop()?.();
|
||||
}
|
||||
});
|
||||
|
||||
function withAgentDir(agentDir: string, fn: () => void): void {
|
||||
const previousAgentDir = process.env[ENV_AGENT_DIR];
|
||||
process.env[ENV_AGENT_DIR] = agentDir;
|
||||
try {
|
||||
fn();
|
||||
} finally {
|
||||
if (previousAgentDir === undefined) {
|
||||
delete process.env[ENV_AGENT_DIR];
|
||||
} else {
|
||||
process.env[ENV_AGENT_DIR] = previousAgentDir;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
it("keeps uppercase header strings as literals during startup migrations", async () => {
|
||||
const harness = await createHarness({ withConfiguredAuth: false });
|
||||
cleanups.push(harness.cleanup);
|
||||
|
||||
const envKeys = ["CUSTOM_API_KEY", "BEARER"];
|
||||
const savedEnv: Record<string, string | undefined> = {};
|
||||
for (const key of envKeys) {
|
||||
savedEnv[key] = process.env[key];
|
||||
process.env[key] = `env-${key}`;
|
||||
}
|
||||
cleanups.push(() => {
|
||||
for (const key of envKeys) {
|
||||
if (savedEnv[key] === undefined) {
|
||||
delete process.env[key];
|
||||
} else {
|
||||
process.env[key] = savedEnv[key];
|
||||
}
|
||||
}
|
||||
});
|
||||
|
||||
const modelsPath = join(harness.tempDir, "models.json");
|
||||
writeFileSync(
|
||||
modelsPath,
|
||||
`${JSON.stringify(
|
||||
{
|
||||
providers: {
|
||||
"my-provider": {
|
||||
baseUrl: "https://example.com/v1",
|
||||
apiKey: "CUSTOM_API_KEY",
|
||||
api: "openai-completions",
|
||||
headers: { Authorization: "BEARER" },
|
||||
models: [{ id: "my-model" }],
|
||||
},
|
||||
},
|
||||
},
|
||||
null,
|
||||
2,
|
||||
)}\n`,
|
||||
"utf-8",
|
||||
);
|
||||
|
||||
withAgentDir(harness.tempDir, () => runMigrations(harness.tempDir));
|
||||
|
||||
const migrated = JSON.parse(readFileSync(modelsPath, "utf-8")) as {
|
||||
providers: Record<string, { apiKey?: string; headers?: Record<string, string> }>;
|
||||
};
|
||||
expect(migrated.providers["my-provider"]?.apiKey).toBe("CUSTOM_API_KEY");
|
||||
expect(migrated.providers["my-provider"]?.headers?.Authorization).toBe("BEARER");
|
||||
|
||||
const registry = ModelRegistry.create(AuthStorage.create(join(harness.tempDir, "auth.json")), modelsPath);
|
||||
const model = registry.find("my-provider", "my-model");
|
||||
expect(model).toBeDefined();
|
||||
expect(await registry.getApiKeyAndHeaders(model!)).toMatchObject({
|
||||
ok: true,
|
||||
apiKey: "CUSTOM_API_KEY",
|
||||
headers: { Authorization: "BEARER" },
|
||||
});
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user