Skip to content

Commit f7c957e

Browse files
arul28arul28
committed
ship: tighten review follow-up
1 parent 5dca2ec commit f7c957e

8 files changed

Lines changed: 106 additions & 19 deletions

File tree

‎apps/desktop/src/main/services/ai/apiKeyStore.test.ts‎

Lines changed: 39 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,10 @@ function securityArg(args: string[], flag: string): string {
4444
return index >= 0 ? args[index + 1] ?? "" : "";
4545
}
4646

47-
function installSecurityMock(keychain: Map<string, string>): void {
47+
function installSecurityMock(
48+
keychain: Map<string, string>,
49+
options: { failProviderIndexWrites?: boolean } = {},
50+
): void {
4851
spawnSyncMock.mockImplementation((_command: string, rawArgs: string[]) => {
4952
const args = rawArgs.map(String);
5053
const command = args[0];
@@ -64,6 +67,9 @@ function installSecurityMock(keychain: Map<string, string>): void {
6467
};
6568
}
6669
if (command === "add-generic-password") {
70+
if (options.failProviderIndexWrites && account === "__ade_provider_index__") {
71+
return { status: 1, stdout: "", stderr: "provider index write failed" };
72+
}
6773
keychain.set(account, securityArg(args, "-w"));
6874
return { status: 0, stdout: "", stderr: "" };
6975
}
@@ -131,6 +137,38 @@ describe("apiKeyStore", () => {
131137
expect(fs.existsSync(path.join(tempRoot, ".ade", "secrets", "api-keys.v1.bin"))).toBe(false);
132138
});
133139

140+
it("keeps a stored Keychain key usable when the provider index write fails", async () => {
141+
installSecurityMock(keychain, { failProviderIndexWrites: true });
142+
const store = await loadStoreModule();
143+
store.initApiKeyStore(tempRoot);
144+
145+
store.storeApiKey("cursor", "crsr_test_key");
146+
147+
expect(store.getApiKey("cursor")).toBe("crsr_test_key");
148+
expect(store.listStoredProviders()).toContain("cursor");
149+
expect(keychain.get("cursor")).toBe("crsr_test_key");
150+
expect(keychain.has("__ade_provider_index__")).toBe(false);
151+
expect(store.getApiKeyStoreStatus().macosKeychainError).toContain("provider index write failed");
152+
});
153+
154+
it("removes a deleted Keychain key from memory when the provider index write fails", async () => {
155+
keychain.set("__ade_provider_index__", JSON.stringify(["cursor"]));
156+
keychain.set("cursor", "crsr_test_key");
157+
installSecurityMock(keychain, { failProviderIndexWrites: true });
158+
const store = await loadStoreModule();
159+
store.initApiKeyStore(tempRoot);
160+
161+
expect(store.getApiKey("cursor")).toBe("crsr_test_key");
162+
163+
store.deleteApiKey("cursor");
164+
165+
expect(store.getApiKey("cursor")).toBeNull();
166+
expect(store.listStoredProviders()).not.toContain("cursor");
167+
expect(keychain.has("cursor")).toBe(false);
168+
expect(keychain.get("__ade_provider_index__")).toContain("cursor");
169+
expect(store.getApiKeyStoreStatus().macosKeychainError).toContain("provider index write failed");
170+
});
171+
134172
it("migrates a decryptable legacy safeStorage blob into macOS Keychain", async () => {
135173
const secretsDir = path.join(tempRoot, ".ade", "secrets");
136174
fs.mkdirSync(secretsDir, { recursive: true });

‎apps/desktop/src/main/services/ai/apiKeyStore.ts‎

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -229,6 +229,15 @@ function writeMacosKeychainProviderIndex(providers: Iterable<string>): void {
229229
);
230230
}
231231

232+
function tryWriteMacosKeychainProviderIndex(providers: Iterable<string>): void {
233+
try {
234+
writeMacosKeychainProviderIndex(providers);
235+
} catch {
236+
// The provider index only powers discovery/listing. Keep the just-written
237+
// secret usable in this process even if the secondary index write fails.
238+
}
239+
}
240+
232241
function knownProviderCandidates(extraProviders?: Iterable<string>): string[] {
233242
const providers = new Set<string>(Object.keys(ENV_KEY_PROVIDERS));
234243
for (const provider of extraProviders ?? []) {
@@ -390,9 +399,9 @@ export function storeApiKey(provider: string, key: string): void {
390399
const store = ensureStore();
391400
if (isMacosKeychainAvailable()) {
392401
writeMacosKeychainSecret(normalizedProvider, normalizedKey);
393-
const index = readMacosKeychainProviderIndex();
394-
writeMacosKeychainProviderIndex(new Set([...index.providers, normalizedProvider]));
395402
store[normalizedProvider] = normalizedKey;
403+
const index = readMacosKeychainProviderIndex();
404+
tryWriteMacosKeychainProviderIndex(new Set([...index.providers, normalizedProvider]));
396405
return;
397406
}
398407
const nextStore = { ...store, [normalizedProvider]: normalizedKey };
@@ -420,9 +429,9 @@ export function deleteApiKey(provider: string): void {
420429
const store = ensureStore();
421430
if (isMacosKeychainAvailable()) {
422431
deleteMacosKeychainSecret(normalizedProvider);
423-
const index = readMacosKeychainProviderIndex();
424-
writeMacosKeychainProviderIndex(index.providers.filter((entry) => entry !== normalizedProvider));
425432
delete store[normalizedProvider];
433+
const index = readMacosKeychainProviderIndex();
434+
tryWriteMacosKeychainProviderIndex(index.providers.filter((entry) => entry !== normalizedProvider));
426435
return;
427436
}
428437
const nextStore = { ...store };

‎apps/desktop/src/main/services/chat/agentChatService.ts‎

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1292,6 +1292,14 @@ function normalizeReasoningEffort(value: unknown): string | null {
12921292
return normalized.length > 0 ? normalized : null;
12931293
}
12941294

1295+
function catalogDescriptorInfoKey(
1296+
group: ModelProviderGroup,
1297+
providerKey: string,
1298+
descriptorId: string,
1299+
): string {
1300+
return `${group}:${providerKey}:${descriptorId}`;
1301+
}
1302+
12951303
function resolveSessionModelDescriptor(session: AgentChatSession): ModelDescriptor | null {
12961304
if (session.modelId) {
12971305
return getModelById(session.modelId) ?? resolveModelAlias(session.modelId) ?? null;
@@ -16898,11 +16906,6 @@ export function createAgentChatService(args: {
1689816906

1689916907
const getModelCatalog = async (): Promise<AgentChatModelCatalog> => {
1690016908
const catalogProviders: ModelProviderGroup[] = ["claude", "codex", "cursor", "droid", "opencode"];
16901-
const descriptorInfoKey = (
16902-
group: ModelProviderGroup,
16903-
providerKey: string,
16904-
descriptorId: string,
16905-
): string => `${group}:${providerKey}:${descriptorId}`;
1690616909
const modelsByProvider = await Promise.all(
1690716910
catalogProviders.map(async (provider) => {
1690816911
try {
@@ -16939,7 +16942,7 @@ export function createAgentChatService(args: {
1693916942
...(runtimeTiers?.length ? { reasoningTiers: runtimeTiers } : {}),
1694016943
};
1694116944
descriptors.push(patched);
16942-
descriptorInfo.set(descriptorInfoKey(provider, patched.family, patched.id), { provider, info });
16945+
descriptorInfo.set(catalogDescriptorInfoKey(provider, patched.family, patched.id), { provider, info });
1694316946
}
1694416947
}
1694516948

@@ -16963,7 +16966,7 @@ export function createAgentChatService(args: {
1696316966
key: subsection.key,
1696416967
label: subsection.label,
1696516968
models: subsection.models.map((descriptor) => {
16966-
const entry = descriptorInfo.get(descriptorInfoKey(group.key, provider.key, descriptor.id));
16969+
const entry = descriptorInfo.get(catalogDescriptorInfoKey(group.key, provider.key, descriptor.id));
1696716970
const runtimeProvider = entry?.provider ?? resolveProviderGroupForModel(descriptor);
1696816971
const runtimeModelId = entry?.info.id ?? getRuntimeModelRefForDescriptor(descriptor, runtimeProvider);
1696916972
const reasoningEfforts = entry?.info.reasoningEfforts

‎apps/desktop/src/main/services/chat/cursorSdkHooks.test.ts‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import os from "node:os";
55
import path from "node:path";
66
import { describe, expect, it } from "vitest";
77
import {
8+
__buildCursorSdkHookCommandForTests,
89
cursorSdkHookScriptPath,
910
cursorSdkHooksJsonPath,
1011
ensureCursorSdkUserHook,
@@ -162,4 +163,28 @@ describe("Cursor SDK hook installation", () => {
162163
fs.rmSync(home, { recursive: true, force: true });
163164
}
164165
});
166+
167+
it("uses a direct cmd set prefix for packaged Electron hooks on Windows", () => {
168+
const originalPlatform = process.platform;
169+
const originalElectron = process.versions.electron;
170+
Object.defineProperty(process, "platform", { value: "win32", configurable: true });
171+
Object.defineProperty(process.versions, "electron", { value: "37.0.0", configurable: true });
172+
try {
173+
const command = __buildCursorSdkHookCommandForTests(
174+
undefined,
175+
String.raw`C:\Users\Ada Lovelace\.cursor\hooks\ade-tool-gate.cjs`,
176+
);
177+
expect(command).toBe(
178+
`set ELECTRON_RUN_AS_NODE=1&& "${process.execPath}" "C:\\Users\\Ada Lovelace\\.cursor\\hooks\\ade-tool-gate.cjs"`,
179+
);
180+
expect(command).not.toContain("cmd /d /s /c");
181+
} finally {
182+
Object.defineProperty(process, "platform", { value: originalPlatform, configurable: true });
183+
if (originalElectron === undefined) {
184+
Reflect.deleteProperty(process.versions, "electron");
185+
} else {
186+
Object.defineProperty(process.versions, "electron", { value: originalElectron, configurable: true });
187+
}
188+
}
189+
});
165190
});

‎apps/desktop/src/main/services/chat/cursorSdkHooks.ts‎

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -26,22 +26,26 @@ function shellQuote(value: string): string {
2626
}
2727

2828
function windowsCmdQuote(value: string): string {
29-
if (/[\0\r\n]/.test(value)) {
30-
throw new Error("Cursor hook command paths cannot contain control characters.");
29+
if (/[\0\r\n"%]/.test(value)) {
30+
throw new Error("Cursor hook command paths cannot contain control characters or Windows cmd expansion characters.");
3131
}
32-
return `"${value.replace(/(["^&|<>%])/g, "^$1")}"`;
32+
return `"${value}"`;
33+
}
34+
35+
function commandPathQuote(value: string): string {
36+
return process.platform === "win32" ? windowsCmdQuote(value) : shellQuote(value);
3337
}
3438

3539
function buildHookCommand(nodePath: string | undefined, scriptPath: string): string {
3640
const explicitNode = nodePath?.trim();
37-
if (explicitNode) return `${shellQuote(explicitNode)} ${shellQuote(scriptPath)}`;
41+
if (explicitNode) return `${commandPathQuote(explicitNode)} ${commandPathQuote(scriptPath)}`;
3842
if (process.versions.electron) {
3943
if (process.platform === "win32") {
40-
return `cmd /d /s /c "set ELECTRON_RUN_AS_NODE=1&& ${windowsCmdQuote(process.execPath)} ${windowsCmdQuote(scriptPath)}"`;
44+
return `set ELECTRON_RUN_AS_NODE=1&& ${windowsCmdQuote(process.execPath)} ${windowsCmdQuote(scriptPath)}`;
4145
}
4246
return `ELECTRON_RUN_AS_NODE=1 ${shellQuote(process.execPath)} ${shellQuote(scriptPath)}`;
4347
}
44-
return `${shellQuote(process.execPath)} ${shellQuote(scriptPath)}`;
48+
return `${commandPathQuote(process.execPath)} ${commandPathQuote(scriptPath)}`;
4549
}
4650

4751
function readObject(value: unknown): Record<string, unknown> | null {
@@ -217,6 +221,8 @@ main().catch((error) => {
217221
fs.writeFileSync(scriptPath, source, { mode: 0o755 });
218222
}
219223

224+
export const __buildCursorSdkHookCommandForTests = buildHookCommand;
225+
220226
export function ensureCursorSdkUserHook(args: {
221227
userHomeDir: string;
222228
nodePath?: string;

‎apps/desktop/src/main/services/chat/cursorSdkPool.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,8 @@ function ensurePrivateDirectory(dir: string): void {
104104
fs.mkdirSync(dir, { recursive: true, mode: 0o700 });
105105
let fd: number | null = null;
106106
try {
107+
// Older Node/platform pairs can omit these open flags; fstat below still
108+
// verifies the directory shape when the constants are unavailable.
107109
fd = fs.openSync(
108110
dir,
109111
fs.constants.O_RDONLY

‎apps/desktop/src/main/services/ipc/registerIpc.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3229,6 +3229,8 @@ export function registerIpc({
32293229
const { storeApiKey } = await import("../ai/apiKeyStore");
32303230
storeApiKey(arg.provider, arg.key);
32313231
try {
3232+
// The key store mutation already succeeded; invalidation is a freshness
3233+
// step so settings save/delete should not fail if a runtime cache is gone.
32323234
ctx.aiIntegrationService.invalidateProviderReadinessCaches();
32333235
} catch (error) {
32343236
ctx.logger.warn("ai.api_key_cache_invalidation_failed", {
@@ -3243,6 +3245,8 @@ export function registerIpc({
32433245
const { deleteApiKey } = await import("../ai/apiKeyStore");
32443246
deleteApiKey(arg.provider);
32453247
try {
3248+
// The key store mutation already succeeded; invalidation is a freshness
3249+
// step so settings save/delete should not fail if a runtime cache is gone.
32463250
ctx.aiIntegrationService.invalidateProviderReadinessCaches();
32473251
} catch (error) {
32483252
ctx.logger.warn("ai.api_key_cache_invalidation_failed", {

‎apps/ios/ADE/Views/Work/WorkModelCatalog.swift‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -472,6 +472,7 @@ private func workModelLookupKeys(_ raw: String?) -> [String] {
472472
}
473473
}
474474

475+
append(trimmed)
475476
if let registryId = workCanonicalClaudeRegistryId(for: trimmed) {
476477
append(registryId)
477478
}
@@ -484,7 +485,6 @@ private func workModelLookupKeys(_ raw: String?) -> [String] {
484485
if let runtimeId = workCodexRuntimeModelId(for: trimmed) {
485486
append(runtimeId)
486487
}
487-
append(trimmed)
488488
if trimmed.lowercased().hasPrefix("openai/") {
489489
append(String(trimmed.dropFirst("openai/".count)))
490490
}

0 commit comments

Comments
 (0)