fix: 6 verified bugs cherry-picked from PR #9

[jrenaldi79]

Summary

Cherry-picked and independently verified 6 bug fixes identified during review of #9. Each bug was reproduced before fixing.

• api-key-validation: Anthropic 429/5xx incorrectly resolved as {valid: true}
• api-key-validation: No HTTP timeout on validation requests (hangs forever)
• 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 silently ignored
• cli: --key=value syntax parsed as {"model=gemini": true} instead of {"model": "gemini"}
• pre-push hook: Test failures don't block push (script exits 0 due to npm audit || echo fallback)

What was NOT included

The auto-skills framework, activity monitoring hooks, and other feature work from #9 were not included. This PR is strictly bug fixes for existing code.

Test plan

• All 6 bugs independently reproduced before fixing
• Full test suite passes (1645 tests, 0 failures)
• Lint clean (no new errors)
• New test coverage for --key=value CLI syntax (4 tests)
• New test coverage for Anthropic 429/5xx rejection (2 tests)
• Updated setup-window tests for opt-in debug port behavior
• Code review via superpowers:code-reviewer (no critical issues)

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • CLI argument parser now supports inline key-value syntax (e.g., --key=value)
  • Remote debugging port is now configurable and optional

• Bug Fixes

  • API key validation now handles server errors and request timeouts
  • API key handler includes error recovery and returns safe defaults on failure

• Chores

  • Pre-push hook updated to enforce test suite passing

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR introduces defensive error handling for API key validation, adds support for inline CLI arguments (--key=value syntax), makes sidecar debugging port optional, and enforces stricter test failures in the pre-push hook. Changes span multiple modules with corresponding test updates.

Changes

Cohort / File(s)	Summary
CLI Argument Parser src/cli.js, tests/cli.test.js	Adds support for inline value syntax (--key=value) by detecting and extracting equals signs within arguments. Tests validate numeric parsing, model flags, and values containing equals signs.
API Key Validation & IPC electron/ipc-setup.js, src/utils/api-key-validation.js, tests/api-key-store-validation.test.js	Wraps get-api-keys IPC handler in try/catch with defensive fallback. Adds server error handling (500+, 429) and 10-second request timeout for anthropic provider. Tests mocked with setTimeout/destroy methods and new cases for rate-limiting and server errors.
Sidecar Debug Port src/sidecar/setup-window.js, tests/sidecar/setup-window.test.js	Makes remote debugging port optional via SIDECAR_DEBUG_PORT environment variable. Adds test coverage for port presence and verifies default behavior excludes debugging flags.
Pre-push Test Enforcement .husky/pre-push	Adds exit code propagation (|| exit 1) to npm run test:all, causing push to abort on test failures.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Possibly related PRs

• fix: shell-independent API key resolution (#11) #13: Refactors API key loading and exports (removes saveApiKey, adds env-loader), directly overlapping with this PR's changes to IPC get-api-keys handler and API key validation logic.

Poem

🐰 The rabbit hops through code so fine,
With error guards and timeouts divine,
Inline arguments now take their place,
Debug ports dance with graceful space,
And tests stand firm—no push gets through,
When failures rise, we bid adieu! 🛡️

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title accurately describes the main change: cherry-picking and applying 6 verified bug fixes from PR #9. It is concise, specific, and directly related to the changeset across all modified files.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/bug-fixes-from-pr9

📝 Coding Plan

• Generate coding plan for human review comments

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

src/cli.js (1)

1-369: File exceeds 300 line limit.

The file is currently ~369 lines, exceeding the 300-line limit specified in coding guidelines for {src,bin,scripts}/**/*.js. Consider extracting validation functions (validateStartArgs, validators) into a separate module like src/cli-validators.js.

As per coding guidelines: "File size limit of 300 lines per file in src/, bin/, or scripts/ directories".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/cli.js` around lines 1 - 369, This file exceeds the 300-line limit;
extract the argument validation logic into a new module: create
src/cli-validators.js and move validateStartArgs, isValidModelFormat, and
isValidDurationFormat into it (preserve their behavior and any calls to
validate* helpers like validatePromptContent, validateCwdPath,
validateAgentMode, validateHeadlessAgent, validateMcpSpec,
validateMcpConfigFile, validateApiKey, validateThinkingLevel and logger), export
them, and update src/cli.js to require/import those functions and remove the
moved definitions so parseArgs, parseValue, isBooleanFlag, getUsage, and
DEFAULTS remain in cli.js; ensure module.exports still exposes parseArgs,
getUsage, DEFAULTS and the validateStartArgs is re-exported from the new module.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/cli.js`:
- Around line 1-369: This file exceeds the 300-line limit; extract the argument
validation logic into a new module: create src/cli-validators.js and move
validateStartArgs, isValidModelFormat, and isValidDurationFormat into it
(preserve their behavior and any calls to validate* helpers like
validatePromptContent, validateCwdPath, validateAgentMode,
validateHeadlessAgent, validateMcpSpec, validateMcpConfigFile, validateApiKey,
validateThinkingLevel and logger), export them, and update src/cli.js to
require/import those functions and remove the moved definitions so parseArgs,
parseValue, isBooleanFlag, getUsage, and DEFAULTS remain in cli.js; ensure
module.exports still exposes parseArgs, getUsage, DEFAULTS and the
validateStartArgs is re-exported from the new module.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 82462b8f-8e0f-48b2-b389-6103c582fa8e

📥 Commits

Reviewing files that changed from the base of the PR and between ad433c5 and 1d05e1f.

📒 Files selected for processing (8)

• .husky/pre-push
• electron/ipc-setup.js
• src/cli.js
• src/sidecar/setup-window.js
• src/utils/api-key-validation.js
• tests/api-key-store-validation.test.js
• tests/cli.test.js
• tests/sidecar/setup-window.test.js