fix: 6 verified bugs from PR #9 review

Each bug was independently reproduced before fixing:

- api-key-validation: Anthropic 429/5xx incorrectly resolved as valid key
- api-key-validation: no HTTP timeout, requests hang forever on unreachable providers
- setup-window: remote debug port 9222 always enabled (now opt-in via SIDECAR_DEBUG_PORT)
- ipc-setup: get-api-keys handler has no error handling, saveApiKey failures ignored
- cli: --key=value syntax parsed as {"model=gemini": true} instead of {"model": "gemini"}
- pre-push hook: test failures don't block push (exits 0 due to npm audit fallback)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Author: jrenaldi79

.husky/pre-push

@@ -15,7 +15,7 @@ if [ -n "$HEAD_SHA" ] && [ "$HEAD_SHA" = "$CACHED_SHA" ]; then
   echo "Tests already passed for $HEAD_SHA — skipping."
 else
   echo "Running full test suite (unit + integration) before push..."
-  npm run test:all
+  npm run test:all || exit 1
 fi
 
 echo "Checking for dependency vulnerabilities..."

electron/ipc-setup.js

@@ -99,21 +99,28 @@ function registerSetupHandlers(ipcMain, getMainWindow) {
   });
 
   ipcMain.handle('sidecar:get-api-keys', () => {
-    const { readApiKeys, readApiKeyHints, saveApiKey } = require('../src/utils/api-key-store');
-    const { importFromAuthJson } = require('../src/utils/auth-json');
-    const status = readApiKeys();
-    const hints = readApiKeyHints();
+    try {
+      const { readApiKeys, readApiKeyHints, saveApiKey } = require('../src/utils/api-key-store');
+      const { importFromAuthJson } = require('../src/utils/auth-json');
+      const status = readApiKeys();
+      const hints = readApiKeyHints();
 
-    // Auto-import keys from auth.json that sidecar doesn't have yet
-    const { imported } = importFromAuthJson(status);
-    for (const entry of imported) {
-      saveApiKey(entry.provider, entry.key);
-      status[entry.provider] = true;
-      const visible = entry.key.slice(0, 8);
-      hints[entry.provider] = visible + '\u2022'.repeat(Math.max(0, Math.min(entry.key.length - 8, 12)));
-    }
+      // Auto-import keys from auth.json that sidecar doesn't have yet
+      const { imported } = importFromAuthJson(status);
+      for (const entry of imported) {
+        const result = saveApiKey(entry.provider, entry.key);
+        if (result && result.success !== false) {
+          status[entry.provider] = true;
+          const visible = entry.key.slice(0, 8);
+          hints[entry.provider] = visible + '\u2022'.repeat(Math.max(0, Math.min(entry.key.length - 8, 12)));
+        }
+      }
 
-    return { status, hints, imported: imported.map(e => e.provider) };
+      return { status, hints, imported: imported.map(e => e.provider) };
+    } catch (err) {
+      logger.error('get-api-keys handler error', { error: err.message });
+      return { status: {}, hints: {}, imported: [] };
+    }
   });
 
   ipcMain.handle('sidecar:fetch-models', async () => {

src/cli.js

@@ -47,15 +47,29 @@ function parseArgs(argv) {
     const arg = argv[i];
 
     if (arg.startsWith('--')) {
-      const key = arg.slice(2);
+      let key = arg.slice(2);
       const next = argv[i + 1];
 
+      // Handle --key=value syntax
+      let inlineValue;
+      const eqIdx = key.indexOf('=');
+      if (eqIdx !== -1) {
+        inlineValue = key.slice(eqIdx + 1);
+        key = key.slice(0, eqIdx);
+      }
+
       // Boolean flags (no value expected)
       if (isBooleanFlag(key)) {
         result[key] = true;
         continue;
       }
 
+      // If --key=value was used, use the inline value directly
+      if (inlineValue !== undefined) {
+        result[key] = parseValue(key, inlineValue);
+        continue;
+      }
+
       // Array accumulation flags
       if (key === 'exclude-mcp' && next && !next.startsWith('--')) {
         result['exclude-mcp'] = result['exclude-mcp'] || [];

src/sidecar/setup-window.js

@@ -29,13 +29,13 @@ function launchSetupWindow() {
       SIDECAR_MODE: 'setup'
     };
 
-    const debugPort = process.env.SIDECAR_DEBUG_PORT || '9222';
-    logger.info('Launching setup window', { debugPort });
+    const debugPort = process.env.SIDECAR_DEBUG_PORT;
+    const args = debugPort
+      ? [`--remote-debugging-port=${debugPort}`, mainPath]
+      : [mainPath];
+    logger.info('Launching setup window', { debugPort: debugPort || 'disabled' });
 
-    const proc = spawn(electronPath, [
-      `--remote-debugging-port=${debugPort}`,
-      mainPath
-    ], {
+    const proc = spawn(electronPath, args, {
       env,
       stdio: ['ignore', 'pipe', 'pipe']
     });

src/utils/api-key-validation.js

@@ -60,6 +60,8 @@ function validateApiKey(provider, key) {
         if (provider === 'anthropic') {
           if (res.statusCode === 401) {
             resolve({ valid: false, error: 'Invalid API key (401)' });
+          } else if (res.statusCode >= 500 || res.statusCode === 429) {
+            resolve({ valid: false, error: `Server error (${res.statusCode})` });
           } else {
             resolve({ valid: true });
           }
@@ -75,6 +77,10 @@ function validateApiKey(provider, key) {
         }
       });
     });
+    req.setTimeout(10000, () => {
+      req.destroy();
+      resolve({ valid: false, error: 'Request timed out' });
+    });
     req.on('error', (err) => {
       resolve({ valid: false, error: err.message });
     });

tests/api-key-store-validation.test.js

@@ -52,7 +52,7 @@ describe('api-key-store validation', () => {
       };
       https.get.mockImplementation((_url, _opts, cb) => {
         cb(mockResponse);
-        return { on: jest.fn() };
+        return { on: jest.fn(), setTimeout: jest.fn(), destroy: jest.fn() };
       });
 
       const result = await validateApiKey('openrouter', 'sk-or-valid');
@@ -70,7 +70,7 @@ describe('api-key-store validation', () => {
       };
       https.get.mockImplementation((_url, _opts, cb) => {
         cb(mockResponse);
-        return { on: jest.fn() };
+        return { on: jest.fn(), setTimeout: jest.fn(), destroy: jest.fn() };
       });
 
       const result = await validateApiKey('openrouter', 'sk-or-bad');
@@ -79,7 +79,7 @@ describe('api-key-store validation', () => {
 
     it('should resolve invalid for network error', async () => {
       https.get.mockImplementation((_url, _opts, _cb) => {
-        const req = { on: jest.fn() };
+        const req = { on: jest.fn(), setTimeout: jest.fn(), destroy: jest.fn() };
         setTimeout(() => {
           const errCall = req.on.mock.calls.find(c => c[0] === 'error');
           if (errCall) { errCall[1](new Error('Network error')); }
@@ -122,7 +122,7 @@ describe('api-key-store validation', () => {
       };
       https.get.mockImplementation((_url, _opts, cb) => {
         cb(mockResponse);
-        return { on: jest.fn() };
+        return { on: jest.fn(), setTimeout: jest.fn(), destroy: jest.fn() };
       });
 
       const result = await validateApiKey('openrouter', 'sk-or-forbidden');
@@ -140,7 +140,7 @@ describe('api-key-store validation', () => {
       };
       https.get.mockImplementation((_url, _opts, cb) => {
         cb(mockResponse);
-        return { on: jest.fn() };
+        return { on: jest.fn(), setTimeout: jest.fn(), destroy: jest.fn() };
       });
 
       const result = await validateApiKey('openrouter', 'sk-or-500');
@@ -159,7 +159,7 @@ describe('api-key-store validation', () => {
       };
       https.get.mockImplementation((_url, _opts, cb) => {
         cb(mockResponse);
-        return { on: jest.fn() };
+        return { on: jest.fn(), setTimeout: jest.fn(), destroy: jest.fn() };
       });
 
       const result = await validateApiKey('anthropic', 'sk-ant-valid');
@@ -177,7 +177,7 @@ describe('api-key-store validation', () => {
       };
       https.get.mockImplementation((_url, _opts, cb) => {
         cb(mockResponse);
-        return { on: jest.fn() };
+        return { on: jest.fn(), setTimeout: jest.fn(), destroy: jest.fn() };
       });
 
       const result = await validateApiKey('anthropic', 'sk-ant-bad');
@@ -199,7 +199,7 @@ describe('api-key-store validation', () => {
         capturedUrl = url;
         capturedHeaders = opts.headers;
         cb(mockResponse);
-        return { on: jest.fn() };
+        return { on: jest.fn(), setTimeout: jest.fn(), destroy: jest.fn() };
       });
 
       const result = await validateApiKey('deepseek', 'sk-deepseek-valid');
@@ -221,7 +221,7 @@ describe('api-key-store validation', () => {
       https.get.mockImplementation((url, _opts, cb) => {
         capturedUrl = url;
         cb(mockResponse);
-        return { on: jest.fn() };
+        return { on: jest.fn(), setTimeout: jest.fn(), destroy: jest.fn() };
       });
 
       await validateApiKey('google', 'AIza-test-key');

tests/sidecar/setup-window.test.js

@@ -54,15 +54,36 @@ describe('setup-window', () => {
 
     expect(spawn).toHaveBeenCalled();
     const spawnArgs = spawn.mock.calls[0];
-    expect(spawnArgs[1][0]).toContain('--remote-debugging-port=');
-    expect(spawnArgs[1][1]).toContain('main.js');
+    // Debug port is opt-in: without SIDECAR_DEBUG_PORT, only main.js is passed
+    expect(spawnArgs[1][0]).toContain('main.js');
 
     const env = spawnArgs[2].env;
     expect(env.SIDECAR_MODE).toBe('setup');
 
     expect(result.success).toBe(true);
   });
 
+  it('should pass --remote-debugging-port when SIDECAR_DEBUG_PORT is set', async () => {
+    process.env.SIDECAR_DEBUG_PORT = '9333';
+    spawn.mockClear();
+    spawn.mockReturnValue(mockProcess);
+
+    const promise = launchSetupWindow();
+
+    const dataCallback = mockProcess.stdout.on.mock.calls.find(c => c[0] === 'data')[1];
+    dataCallback('{"status":"complete"}\n');
+
+    const closeCallback = mockProcess.on.mock.calls.find(c => c[0] === 'close')[1];
+    closeCallback(0);
+
+    await promise;
+
+    const spawnArgs = spawn.mock.calls[0];
+    expect(spawnArgs[1][0]).toBe('--remote-debugging-port=9333');
+    expect(spawnArgs[1][1]).toContain('main.js');
+    delete process.env.SIDECAR_DEBUG_PORT;
+  });
+
   it('should parse enriched JSON with default model and keyCount', async () => {
     const promise = launchSetupWindow();