feat: activity monitoring hooks with CodeRabbit review fixes

[ellisjr]

Summary

• Activity monitoring hooks (Phase 1): PostToolUse and PostToolUseFailure hooks that collect structured events to session-specific JSONL files for future Phase 2 analysis. BMAD artifact trigger fires additionalContext when Write/Edit targets _bmad-output/.
• Auto-skills framework: User-configurable auto-skills (auto-review, auto-unblock, auto-security, auto-bmad-method-check) with master/per-skill enable/disable controls, CLI management via sidecar auto-skills, and config persistence.
• CodeRabbit review fixes: Addressed 20+ findings across security, bugs, tests, and docs — including making remote debugging opt-in, fixing TOCTOU race conditions in hooks, adding request timeouts, fixing backwards-compat alias signatures, and hardening error handling in IPC handlers.

Key changes

Security

• Remote debugging port now opt-in only via SIDECAR_DEBUG_PORT (was always-on port 9222)
• TOCTOU race conditions fixed in hook event file creation (noclobber + ownership verification)
• API key validation: 10s request timeout, Anthropic 5xx/429 treated as errors
• Publish workflow: curl timeout added, first-release edge case handled

Bug fixes

• validateOpenRouterKey backwards-compat alias now correctly wraps with (key) => validateApiKey('openrouter', key)
• handleAutoSkills now properly awaited in CLI dispatch
• IPC get-api-keys handler: try-catch added, saveApiKey result checked before reporting success
• filterRelevantModels guards against null m.name
• Unknown skill names default to disabled (was enabled)
• Thinking level fallback finds highest supported level for high/xhigh requests
• Pre-push hook now blocks push on test failure (|| exit 1)

Tests

• Fixed process.env restoration, nested model ID test case, silent-pass risk in UI test
• Updated tests for new opt-in debug port and wrapper alias behaviors

Test plan

• npm test — 1558 tests pass (70 suites)
• npm run lint — clean
• Pre-push hook runs full test suite successfully

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Auto-Skills: contextual automated workflows (auto-review, auto-security, auto-unblock, auto-BMAD) with CLI management and activity-monitoring hooks for pre/post tool events.
  • CLI help/usage improvements and a new auto-skills command.

• Documentation

  • Added extensive docs for auto-skills, BMAD workflow, invocation proposals, and skill-specific guides.

• Bug Fixes / Reliability

  • Safer hook scripts and auth/import handling; installer now registers/unregisters hooks cleanly; pre-push enforces test failures.

• Chores

  • Release flow: skip autogenerated notes when no prior tag and tightened external request timeouts.

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds an auto-skills system: config and CLI surfaces, activity-monitoring hooks and shell hook scripts, postinstall/preuninstall hook registration, several auto-skill specifications, supporting utilities/tests, and robustness fixes across sidecar components and tooling. (50 words)

Changes

Cohort / File(s)	Summary
Auto-Skills Core & CLI src/utils/auto-skills-config.js, src/cli-handlers.js, src/cli-usage.js, bin/sidecar.js, src/cli.js	New auto-skills config module, CLI usage module, and handleAutoSkills command wiring; src/cli.js exposes getUsage/validateStartArgs/DEFAULTS.
Activity Monitoring Hooks & Scripts hooks/hooks.json, hooks/pre-bash.sh, hooks/post-tool-use.sh, hooks/post-failure.sh	Adds hook manifest and three shell hooks for pre-tool, post-tool, and failure events that emit JSONL events and optionally trigger BMAD logic.
Install / Uninstall Integration scripts/postinstall.js, scripts/preuninstall.js, package.json	Auto-skill discovery and install (top-level), hook merge/registration into user settings, and cleanup on uninstall; new lifecycle scripts and exported helpers.
Skill Specs & Docs skill/.../SKILL.md, skill/SKILL.md, docs/*, CLAUDE.md	Adds four auto-skill spec documents (auto-review, auto-security, auto-unblock, auto-bmad-method-check) plus research/proposal/BMAD docs and README updates describing triggers and workflows.
Electron / Sidecar / Runtime Hardening electron/ipc-setup.js, src/sidecar/progress.js, src/sidecar/setup-window.js	Adds defensive error handling around auth.json ops, refines stall detection using progress.json, and makes remote-debugging port opt-in via SIDECAR_DEBUG_PORT.
Validation, Model & Thinking Helpers src/utils/api-key-validation.js, src/utils/model-validator.js, src/utils/thinking-validators.js, src/utils/start-helpers.js	Adds API request timeouts and server-error handling, safer model field coercion, nuanced thinking-level fallback logic, and logger-based error reporting.
Docs / Build / Tooling .github/workflows/publish.yml, .husky/pre-push, site/social-card-render.html, .eslintrc.js, scripts/generate-docs-helpers.js	Publish workflow guard and curl timeouts, stricter pre-push failure propagation, HTML metadata, ESLint override addition for CLI file, and minor docs-generator formatting change.
Tests tests/* (e.g., tests/auto-skills-config.test.js, tests/cli-handlers.test.js, tests/postinstall.test.js, tests/start-helpers.test.js, others)	New and updated tests covering auto-skills config, CLI handler, postinstall hook merging, start helpers, and related unit test adjustments.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant User
  participant Tool as Dev Tool (git/gh/other)
  participant Hook as Hook Script
  participant Config as ~/.config/sidecar/config.json
  participant Sidecar as Sidecar CLI/Agent
  participant Skill as Auto-Skill

  User->>Tool: run command (commit/push/edit/other)
  Tool->>Hook: emits JSON event (stdin)
  Hook->>Config: read settings (monitoring/autoSkills)
  alt monitoring enabled
    Hook->>Hook: write event to session JSONL
    Hook->>Sidecar: return additionalContext / trigger payload
    Sidecar->>Skill: handleAutoSkills / spawn sidecar models
    Skill-->>Sidecar: results
    Sidecar-->>User: display suggestions / apply changes
  else monitoring disabled
    Hook-->>Tool: exit quietly
  end

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Possibly related issues

• Proposal: Improving auto-skill invocation reliability #8: Implements auto-skill installation, hook registration, and CLI surfaces which directly address the auto-skill discovery and reliable triggering described.

Possibly related PRs

• fix: post-merge follow-ups for direct API fallback #3: Overlaps changes to CLI handling and start-helper/model-validator logic; likely related to argument/model resolution adjustments.
• fix: fall back to direct provider API when OpenRouter key is missing #2: Related to CLI argument handling for validate-model and sidecar CLI wiring.

Suggested reviewers

• jrenaldi79

Poem

🐰 I stitched some hooks and learned to spy,

events that whisper as commands go by,
auto-skills wake and give a helpful wink,
security, review, and BMAD in sync,
a rabbit hops — the sidecar hums — let's try!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 77.50% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 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 feature being implemented: activity monitoring hooks with additional code review fixes applied.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

📝 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]

Actionable comments posted: 11

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/utils/model-validator.js (1)

93-101: ⚠️ Potential issue | 🟠 Major

Guard sort key normalization too; current fallback is only partial.

Line 94-95 hardens filtering, but Line 100 still assumes name is always a string. If a provider model has missing/non-string name, localeCompare will throw and abort validation.

Suggested defensive fix

 function filterRelevantModels(models, alias) {
   const term = (ALIAS_SEARCH_TERMS[alias] || alias).toLowerCase();

-  let filtered = models.filter(m =>
-    (m.id || '').toLowerCase().includes(term) ||
-    (m.name || '').toLowerCase().includes(term)
+  let filtered = models.filter((m) =>
+    String(m?.id ?? '').toLowerCase().includes(term) ||
+    String(m?.name ?? '').toLowerCase().includes(term)
   );

   if (filtered.length === 0) { filtered = models; }

-  filtered.sort((a, b) => a.name.localeCompare(b.name));
+  filtered.sort((a, b) =>
+    String(a?.name ?? a?.id ?? '').localeCompare(String(b?.name ?? b?.id ?? ''))
+  );
   return filtered.slice(0, 15);
 }

🤖 Prompt for AI Agents

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

In `@src/utils/model-validator.js` around lines 93 - 101, The sort assumes every
model has a string name and calls a.name.localeCompare(b.name), which can throw
for missing/non-string names; update the sorting in the models filtering block
to normalize the sort keys to strings (e.g., coerce a.name and b.name to '' or
String(...) and optionally lower-case) before calling localeCompare so
filtered.sort(...) never receives undefined/non-string values and remains stable
when model.name is absent or not a string.

🧹 Nitpick comments (6)

tests/sidecar/setup-window.test.js (1)

57-58: Avoid unit-testing Electron window configuration details in this test.

This assertion checks launch argument wiring (main.js placement), which should be covered by manual CDP verification instead of unit tests.

As per coding guidelines, tests/**/*.js: “In tests, do not write unit tests for ... Electron window configuration - these require manual CDP verification.”

🤖 Prompt for AI Agents

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

In `@tests/sidecar/setup-window.test.js` around lines 57 - 58, Remove the unit
assertion that inspects Electron launch args for window configuration (the
expect(spawnArgs[1][0]).toContain('main.js') check) from
tests/sidecar/setup-window.test.js; instead either delete that line or replace
it with a non-Electron-window-related assertion (e.g., verifying spawn was
called or that a fallback path exists) so the test no longer asserts on Electron
window/launch arguments like main.js which must be validated manually via CDP.

src/sidecar/setup-window.js (1)

18-81: Refactor launchSetupWindow into smaller helpers to meet function-size limits.

This function is over the 50-line limit; extracting arg construction and stdout parsing would keep it within policy and improve readability.

As per coding guidelines, src/**/*.js: “Any function must not exceed 50 lines.”

🤖 Prompt for AI Agents

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

In `@src/sidecar/setup-window.js` around lines 18 - 81, The launchSetupWindow
function exceeds the 50-line limit and should be split into smaller helpers;
extract the argument construction and stdout parsing into two functions (e.g.,
buildElectronArgs(debugPort, mainPath) to return the args array and
parseSetupStdout(stdout) to return the same result object currently produced
inside the proc.on('close') handler), then update launchSetupWindow to call
those helpers and keep only process setup, logging, and wiring; make sure
launchSetupWindow still resolves the same objects ({success, error, default,
keyCount}) and preserves the existing debugPort handling, env assembly, and
stdio listeners.

scripts/preuninstall.js (1)

23-23: Add JSDoc for exported removeHooks() API.

This is a public export and should be documented like other public functions in the codebase.

As per coding guidelines **/*.js: Write JSDoc comments for all public APIs.

Also applies to: 65-65

🤖 Prompt for AI Agents

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

In `@scripts/preuninstall.js` at line 23, Add a JSDoc comment block above the
exported function removeHooks() describing its purpose, parameters (if any) and
return value, matching the project's public API style; include a short
description, `@returns` annotation (or `@returns` {void} if it returns nothing), and
any relevant `@example` or `@throws` notes used elsewhere in the codebase so the
function is documented like other public APIs (ensure the comment is placed
immediately above the removeHooks function declaration).

src/utils/auto-skills-config.js (2)

44-49: Minor: Redundant check on line 48.

When skill is truthy (not null/undefined), skill.enabled !== false is sufficient. The current logic is correct but the ternary could be simplified.

♻️ Simplified check

 function isSkillEnabled(skillName, config) {
   const as = getAutoSkillsConfig(config);
   if (!as.enabled) { return false; }
   const skill = as[skillName];
-  return skill ? skill.enabled !== false : false;
+  return skill?.enabled !== false;
 }

🤖 Prompt for AI Agents

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

In `@src/utils/auto-skills-config.js` around lines 44 - 49, In isSkillEnabled,
simplify the redundant ternary by returning the boolean expression directly:
after calling getAutoSkillsConfig(config) and verifying as.enabled, compute
const skill = as[skillName]; then return skill && skill.enabled !== false;
update the function (isSkillEnabled) to use this simplified check referencing
getAutoSkillsConfig, as and skill.

---

75-78: Dynamic require of logger inside function.

The require('./logger') inside the function body avoids potential circular dependency issues but adds overhead on each call. If circular dependencies aren't a concern, consider hoisting to module level.

♻️ Hoist logger import if no circular dependency

 const { loadConfig, saveConfig } = require('./config');
+const { logger } = require('./logger');
 
 // ... later in setAutoSkillsEnabled and error handling ...
-      const { logger } = require('./logger');
       logger.warn(`Ignoring invalid skill name: ${name}`);

🤖 Prompt for AI Agents

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

In `@src/utils/auto-skills-config.js` around lines 75 - 78, The code dynamically
requires the logger inside the loop (in the for...of over skillNames) which adds
repeated overhead; move the logger import out of the loop to module scope by
adding const { logger } = require('./logger'); (or an equivalent top-level
import) and replace the inline require in the block so the loop uses the hoisted
logger; if a circular dependency actually exists, instead create a single cached
require before the loop (e.g., let logger; if (!logger) logger =
require('./logger').logger) so the require happens once.

src/utils/api-key-validation.js (1)

34-35: Consider adding JSDoc for the public API.

The validateApiKey function is exported and used externally but lacks JSDoc documentation describing parameters and return type.

📝 Suggested JSDoc

+/**
+ * Validate an API key by making a test request to the provider's API
+ * `@param` {string} provider - Provider name (openrouter, openai, anthropic, google, deepseek)
+ * `@param` {string} key - API key to validate
+ * `@returns` {Promise<{valid: boolean, error?: string}>} Validation result
+ */
 function validateApiKey(provider, key) {

As per coding guidelines: "Write JSDoc comments for all public APIs"

🤖 Prompt for AI Agents

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

In `@src/utils/api-key-validation.js` around lines 34 - 35, Add a JSDoc block
above the exported validateApiKey function describing its purpose, parameters
and return value: document the provider parameter (type and expected shape or
allowed strings), the key parameter (string), the async return type
(Promise<boolean> indicating whether the key is valid) and any errors thrown
(use `@throws` if the function can reject with an error). Ensure tags include
`@param` for provider and key, `@returns` {Promise<boolean>} and a short description
of behavior so external callers and editors get proper tooling/type hints for
validateApiKey.

🤖 Prompt for all review comments with AI agents

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

Inline comments:
In `@hooks/post-failure.sh`:
- Around line 57-64: Add a size-cap check before appending to EVENT_FILE: define
or use a MAX_EVENT_FILE_BYTES (env or constant) and, after verifying FILE_OWNER
equals "$(id -u)", determine the current file size using a portable stat
fallback (e.g., stat -c '%s' || stat -f '%z') and only perform echo "$EVENT" >>
"$EVENT_FILE" if the file size is less than MAX_EVENT_FILE_BYTES; if the cap is
reached, skip the append (or log/handle per policy). Keep the existing atomic
creation and ownership checks (EVENT, EVENT_FILE, FILE_OWNER) and ensure the
size check is done immediately before the append to avoid race windows.

In `@hooks/post-tool-use.sh`:
- Line 69: Replace the current telemetry value that stores the raw Bash command
(the expression using .tool_name and .tool_input.command) so it never persists
full command lines; instead when .tool_name == "Bash" extract and record only
the executable name (e.g., basename of the first token) or a deterministic
redacted placeholder, and for non-Bash tools keep existing behavior, ensuring
the stored value never contains arguments, environment exports, or URLs/tokens;
update the expression referencing .tool_name and .tool_input.command accordingly
so telemetry writes a safe executable name or redacted string to the session
file.
- Around line 74-77: The FILE_OWNER lookup uses stat -f first (BSD) then stat -c
(GNU), which yields wrong output on Linux; update the fallback order in the
FILE_OWNER assignment so it attempts GNU-style stat -c '%u' first, then
BSD-style stat -f '%u' as a fallback (preserving the existing 2>/dev/null error
suppression and final echo ""), ensuring the subsequent ownership comparison
against $(id -u) in the if condition works correctly across GNU and BSD systems.

In `@hooks/pre-bash.sh`:
- Around line 49-55: The grep -E patterns that search COMMAND for 'git commit',
'git push', and 'gh pr create' use \s which isn't portable; update each grep
invocation that references COMMAND (the lines setting IS_COMMIT=true) to replace
\s and \s+ with POSIX whitespace classes like [[:space:]] and [[:space:]]+ (e.g.
change '(^|[;&|]+\s*)git\s+commit(\s|$)' to
'(^|[;&|]+[[:space:]]*)git[[:space:]]+commit([[:space:]]|$)' and do the
analogous replacements for the git push and gh pr create patterns) so the hook
works on BSD/macOS.

In `@package.json`:
- Line 49: The preuninstall lifecycle script ("preuninstall": "node
scripts/preuninstall.js") is not executed by npm v7+, so replace the reliance on
that lifecycle by exposing the cleanup logic as an explicit npm script and
documenting its use: remove or stop relying on "preuninstall" and add a script
entry like "cleanup-hooks": "node scripts/preuninstall.js" (or rename the script
file to scripts/cleanup.js and reference it) and update user-facing docs to
instruct users to run npm run cleanup-hooks (or the package CLI equivalent)
before uninstalling; optionally keep a legacy "preuninstall" entry for older npm
versions but ensure the canonical mechanism is the explicit "cleanup-hooks"
script and any package uninstall docs reference it.

In `@scripts/postinstall.js`:
- Around line 175-188: The current filter on settings.hooks[event] removes
entire matcher objects when any contained hook is a sidecar; instead iterate
over each matcher (existing) and, if existing.hooks is an array, replace
existing.hooks with a filtered array that removes only hooks where typeof
h.command === 'string' && (h.command === cmd || (path.basename(h.command) ===
basename && h.command.includes('claude-sidecar'))); then keep the matcher only
if it still has at least one hook (existing.hooks.length > 0) or if it
originally had no hooks array; use the same symbols (basename, cmd, existing, h,
matcher, settings.hooks[event]) so you only drop sidecar-owned hooks and
preserve user hooks in the same matcher object.

In `@scripts/preuninstall.js`:
- Around line 17-21: isSidecarHookCommand currently uses only path.basename to
detect sidecar-owned hooks, which can falsely match user hooks elsewhere; update
it to check the command's full path against the sidecar install locations
instead of only basename. Replace the basename-based check in
isSidecarHookCommand (and the duplicate logic around the 37-42 block) with a
test that the absolute command path is inside the known sidecar hook directories
(or startsWith the sidecar install prefix) and then verify the file name is one
of SIDECAR_HOOK_SCRIPTS; ensure you resolve symlinks/absolute paths before
comparing so only files under sidecar-managed directories are considered owned.

In `@skill/auto-security/SKILL.md`:
- Around line 69-71: The docs currently instruct combining `git diff` and `git
diff --cached` for commit audits; change the default behavior so commits audit
only staged changes by default (use `git diff --cached`) and make the combined
unstaged+staged audit (the `git diff` + `git diff --cached` approach) opt-in;
update the "Choose the right diff for the operation" section text to explicitly
state the default is `git diff --cached` for commits, describe how to opt into
broader working-tree scans, and keep the PR/push guidance (the `git merge-base
...` diff resolution) unchanged.

In `@src/cli-usage.js`:
- Around line 11-91: The getUsage() function is too large; split its static
sections into separate constants/functions and have getUsage() assemble them.
Extract the multi-line string into smaller pieces like USAGE_HEADER,
USAGE_COMMANDS, OPTIONS_START, OPTIONS_LIST, OPTIONS_READ, AGENT_TYPES, EXAMPLES
(or helper functions buildStartOptions(), buildCommonOptions(), etc.), ensure
each extracted constant/function is under 50 lines, then return a single
concatenation/template literal in getUsage() that composes those pieces
(referencing the existing getUsage identifier to wire them together).

In `@src/cli.js`:
- Around line 59-63: The parser currently treats --flag=value as true in the
boolean-flag branch (isBooleanFlag), which silently accepts inline values;
update the logic in the CLI parsing function so that when isBooleanFlag(key) is
true you check whether a value was provided (i.e., the token contained '=' or
next token is a non-flag) and, if so, throw or return a clear error/message
instead of setting result[key]=true; only set result[key]=true when no inline
value is present. Locate the boolean handling around isBooleanFlag and
result[key] to implement this validation and error path.

In `@src/sidecar/progress.js`:
- Around line 177-186: The code uses progress.updatedAt without validating it,
which can produce an invalid Date and NaN progressMs; update the logic in the
block handling progress.updatedAt (references: progress.updatedAt, progressTime,
computeLastActivity, progressMs, lastActivityMs, convStat) to first construct
progressTime and guard that progressTime.getTime() is a finite number (e.g.,
!isNaN(progressTime.getTime())) before calling computeLastActivity or computing
progressMs; if the date is invalid, skip updating lastActivity and
lastActivityMs so they cannot become NaN.

---

Outside diff comments:
In `@src/utils/model-validator.js`:
- Around line 93-101: The sort assumes every model has a string name and calls
a.name.localeCompare(b.name), which can throw for missing/non-string names;
update the sorting in the models filtering block to normalize the sort keys to
strings (e.g., coerce a.name and b.name to '' or String(...) and optionally
lower-case) before calling localeCompare so filtered.sort(...) never receives
undefined/non-string values and remains stable when model.name is absent or not
a string.

---

Nitpick comments:
In `@scripts/preuninstall.js`:
- Line 23: Add a JSDoc comment block above the exported function removeHooks()
describing its purpose, parameters (if any) and return value, matching the
project's public API style; include a short description, `@returns` annotation (or
`@returns` {void} if it returns nothing), and any relevant `@example` or `@throws`
notes used elsewhere in the codebase so the function is documented like other
public APIs (ensure the comment is placed immediately above the removeHooks
function declaration).

In `@src/sidecar/setup-window.js`:
- Around line 18-81: The launchSetupWindow function exceeds the 50-line limit
and should be split into smaller helpers; extract the argument construction and
stdout parsing into two functions (e.g., buildElectronArgs(debugPort, mainPath)
to return the args array and parseSetupStdout(stdout) to return the same result
object currently produced inside the proc.on('close') handler), then update
launchSetupWindow to call those helpers and keep only process setup, logging,
and wiring; make sure launchSetupWindow still resolves the same objects
({success, error, default, keyCount}) and preserves the existing debugPort
handling, env assembly, and stdio listeners.

In `@src/utils/api-key-validation.js`:
- Around line 34-35: Add a JSDoc block above the exported validateApiKey
function describing its purpose, parameters and return value: document the
provider parameter (type and expected shape or allowed strings), the key
parameter (string), the async return type (Promise<boolean> indicating whether
the key is valid) and any errors thrown (use `@throws` if the function can reject
with an error). Ensure tags include `@param` for provider and key, `@returns`
{Promise<boolean>} and a short description of behavior so external callers and
editors get proper tooling/type hints for validateApiKey.

In `@src/utils/auto-skills-config.js`:
- Around line 44-49: In isSkillEnabled, simplify the redundant ternary by
returning the boolean expression directly: after calling
getAutoSkillsConfig(config) and verifying as.enabled, compute const skill =
as[skillName]; then return skill && skill.enabled !== false; update the function
(isSkillEnabled) to use this simplified check referencing getAutoSkillsConfig,
as and skill.
- Around line 75-78: The code dynamically requires the logger inside the loop
(in the for...of over skillNames) which adds repeated overhead; move the logger
import out of the loop to module scope by adding const { logger } =
require('./logger'); (or an equivalent top-level import) and replace the inline
require in the block so the loop uses the hoisted logger; if a circular
dependency actually exists, instead create a single cached require before the
loop (e.g., let logger; if (!logger) logger = require('./logger').logger) so the
require happens once.

In `@tests/sidecar/setup-window.test.js`:
- Around line 57-58: Remove the unit assertion that inspects Electron launch
args for window configuration (the expect(spawnArgs[1][0]).toContain('main.js')
check) from tests/sidecar/setup-window.test.js; instead either delete that line
or replace it with a non-Electron-window-related assertion (e.g., verifying
spawn was called or that a fallback path exists) so the test no longer asserts
on Electron window/launch arguments like main.js which must be validated
manually via CDP.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: a97bfdb6-1ef6-4f96-b78f-83b2d4bb53bb

📥 Commits

Reviewing files that changed from the base of the PR and between 251358e and cb4e059.

📒 Files selected for processing (43)

• .eslintrc.js
• .github/workflows/publish.yml
• .husky/pre-push
• CLAUDE.md
• bin/sidecar.js
• docs/activity-monitoring-research.md
• docs/auto-skill-invocation-proposal.md
• docs/bmad-workflow.md
• docs/publishing.md
• electron/ipc-setup.js
• hooks/hooks.json
• hooks/post-failure.sh
• hooks/post-tool-use.sh
• hooks/pre-bash.sh
• package.json
• scripts/generate-docs-helpers.js
• scripts/postinstall.js
• scripts/preuninstall.js
• site/social-card-render.html
• skill/SKILL.md
• skill/auto-bmad-method-check/SKILL.md
• skill/auto-review/SKILL.md
• skill/auto-security/SKILL.md
• skill/auto-unblock/SKILL.md
• src/cli-handlers.js
• src/cli-usage.js
• src/cli.js
• src/sidecar/progress.js
• src/sidecar/setup-window.js
• src/utils/api-key-validation.js
• src/utils/auto-skills-config.js
• src/utils/model-validator.js
• src/utils/start-helpers.js
• src/utils/thinking-validators.js
• tests/api-key-store-validation.test.js
• tests/auto-skills-config.test.js
• tests/cli-handlers.test.js
• tests/config-null-alias.test.js
• tests/model-validator-normalize.test.js
• tests/postinstall.test.js
• tests/setup-ui-model.test.js
• tests/sidecar/setup-window.test.js
• tests/start-helpers.test.js

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

Does npm v10 (and npm v7+) execute package.json "preuninstall" scripts during npm uninstall? Please cite the official npm lifecycle docs.

💡 Result:

No. In npm v7 and later (including npm v10), npm uninstall does not run uninstall lifecycle scripts—so preuninstall (and uninstall / postuninstall) will not execute.

The official npm lifecycle docs state: “While npm v6 had uninstall lifecycle scripts, npm v7 does not… uninstall lifecycle scripts are not implemented and will not function.” [1] (same note is present in the v8+ scripts docs as well). [2]

Sources (official npm docs):
[1] (docs.npmjs.com)
[2] (docs.npmjs.com)

Citations:

• 1: https://docs.npmjs.com/cli/v10/using-npm/scripts/?v=true
• 2: https://docs.npmjs.com/cli/v8/using-npm/scripts/

---

preuninstall script will not execute in npm v7+, leaving cleanup logic unrun.

The preuninstall script on line 49 relies on uninstall lifecycle execution, which is not supported in npm v7 and later. According to official npm documentation, uninstall lifecycle scripts (preuninstall, uninstall, postuninstall) are not implemented in npm v7+. This means user hook entries may remain after package removal.

🤖 Prompt for AI Agents

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

In `@package.json` at line 49, The preuninstall lifecycle script ("preuninstall":
"node scripts/preuninstall.js") is not executed by npm v7+, so replace the
reliance on that lifecycle by exposing the cleanup logic as an explicit npm
script and documenting its use: remove or stop relying on "preuninstall" and add
a script entry like "cleanup-hooks": "node scripts/preuninstall.js" (or rename
the script file to scripts/cleanup.js and reference it) and update user-facing
docs to instruct users to run npm run cleanup-hooks (or the package CLI
equivalent) before uninstalling; optionally keep a legacy "preuninstall" entry
for older npm versions but ensure the canonical mechanism is the explicit
"cleanup-hooks" script and any package uninstall docs reference it.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Avoid basename-only hook ownership detection; it can delete user hooks.

Current matching treats any pre-bash.sh/post-tool-use.sh/post-failure.sh as sidecar-owned, even outside sidecar paths, which can remove unrelated user hooks during uninstall.

🔧 Proposed fix

 function isSidecarHookCommand(command) {
   if (!command || typeof command !== 'string') { return false; }
   const basename = path.basename(command);
-  return SIDECAR_HOOK_SCRIPTS.includes(basename);
+  return SIDECAR_HOOK_SCRIPTS.includes(basename) && command.includes('claude-sidecar');
 }

Also applies to: 37-42

🤖 Prompt for AI Agents

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

In `@scripts/preuninstall.js` around lines 17 - 21, isSidecarHookCommand currently
uses only path.basename to detect sidecar-owned hooks, which can falsely match
user hooks elsewhere; update it to check the command's full path against the
sidecar install locations instead of only basename. Replace the basename-based
check in isSidecarHookCommand (and the duplicate logic around the 37-42 block)
with a test that the absolute command path is inside the known sidecar hook
directories (or startsWith the sidecar install prefix) and then verify the file
name is one of SIDECAR_HOOK_SCRIPTS; ensure you resolve symlinks/absolute paths
before comparing so only files under sidecar-managed directories are considered
owned.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Don’t silently ignore inline values on boolean flags.

Lines [59-63] treat --flag=value as just true. For --on=security, that can unexpectedly activate master enable behavior instead of targeting a skill.

🤖 Prompt for AI Agents

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

In `@src/cli.js` around lines 59 - 63, The parser currently treats --flag=value as
true in the boolean-flag branch (isBooleanFlag), which silently accepts inline
values; update the logic in the CLI parsing function so that when
isBooleanFlag(key) is true you check whether a value was provided (i.e., the
token contained '=' or next token is a non-flag) and, if so, throw or return a
clear error/message instead of setting result[key]=true; only set
result[key]=true when no inline value is present. Locate the boolean handling
around isBooleanFlag and result[key] to implement this validation and error
path.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

src/cli.js (2)

40-92: 🛠️ Refactor suggestion | 🟠 Major

Split parseArgs to meet max function-length guideline.

parseArgs exceeds the 50-line cap; please extract boolean/value/array flag handlers into helpers.

As per coding guidelines, src/**/*.js: "Any function must not exceed 50 lines".

🤖 Prompt for AI Agents

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

In `@src/cli.js` around lines 40 - 92, The parseArgs function is over the 50-line
limit; extract the inline handlers into small helpers to reduce its length while
preserving behavior: create e.g. handleBooleanFlag(key, inlineValue, result,
logger) to set boolean flags and warn on inline values (used where
isBooleanFlag(key) is true), handleArrayFlag(key, val, result) to accumulate
values for the 'exclude-mcp' case (ensure it increments the caller's index when
consumed), and handleOptionWithValue(key, inlineValue, next, result) to parse
and assign values via parseValue or set true when no value is present; update
parseArgs to call these helpers from the main loop and keep DEFAULTS merging and
the positional-arguments push logic unchanged so semantics and i increments
remain identical.

---

78-85: ⚠️ Potential issue | 🟠 Major

Reject missing values for value-flags instead of coercing to true.

At Line [84], non-boolean flags without values become true. That can break downstream validation (e.g., --model with no value can reach isValidModelFormat and fail when calling .split on a boolean).

🛠️ Proposed fix

+function requiresValueFlag(key) {
+  const valueFlags = new Set([
+    'prompt', 'model', 'cwd', 'session-id', 'context-turns',
+    'context-max-tokens', 'timeout', 'opencode-port', 'summary-length',
+    'context-since', 'thinking', 'mcp', 'mcp-config', 'client', 'session-dir'
+  ]);
+  return valueFlags.has(key);
+}
+
 function parseArgs(argv) {
   const result = {
     _: [],
     ...DEFAULTS
   };
@@
       if (inlineValue !== undefined) {
         result[key] = parseValue(key, inlineValue);
       } else if (next && !next.startsWith('--')) {
         result[key] = parseValue(key, next);
         i++;
+      } else if (requiresValueFlag(key)) {
+        logger.warn(`Flag --${key} requires a value`);
       } else {
         result[key] = true;
       }

🤖 Prompt for AI Agents

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

In `@src/cli.js` around lines 78 - 85, The current CLI parsing loop sets
result[key] = true when a non-boolean flag has no inline value and the next
token is absent or is another flag, which causes downstream errors (e.g.,
parseValue expecting a string). Update the final else branch in the loop (the
block handling inlineValue/next) to reject missing values for value-flags:
instead of assigning true, detect a missing value for the flag identified by key
and throw or return a clear error like "Missing value for flag --<key>" (or call
your CLI error handler), so callers of parseValue(key, ...) never receive a
boolean for a value-flag; keep parseValue, inlineValue, next and result as the
referenced symbols to locate and modify the logic.

🧹 Nitpick comments (3)

skill/auto-security/SKILL.md (1)

141-142: Minor: Consider hyphenating "medium-severity" as a compound modifier.

Static analysis suggests using "medium-severity issues" for grammatical correctness.

📝 Suggested wording

-- **If medium severity issues found:** Surface them as warnings
+- **If medium-severity issues found:** Surface them as warnings

🤖 Prompt for AI Agents

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

In `@skill/auto-security/SKILL.md` around lines 141 - 142, Replace the
unhyphenated phrase "medium severity" with the compound modifier
"medium-severity" in the SKILL.md text (specifically where the lines mention "If
medium severity issues found" and "Security scan flagged some medium-severity
items" / "medium severity items") so the compound modifier is grammatically
correct; update both occurrences to "medium-severity issues" or "medium-severity
items" as appropriate, keeping the surrounding wording intact.

src/cli-usage.js (1)

48-49: Minor: Inconsistent indentation in options list.

Lines 48-49 have extra leading spaces compared to other options:

-   --context-max-tokens <N>     Max context tokens (default: 80000)
-   --summary-length <length>    Summary verbosity: brief, normal (default), verbose
+  --context-max-tokens <N>     Max context tokens (default: 80000)
+  --summary-length <length>    Summary verbosity: brief, normal (default), verbose

🤖 Prompt for AI Agents

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

In `@src/cli-usage.js` around lines 48 - 49, Fix the inconsistent indentation in
the CLI usage options list by aligning the two options shown to the same
indentation as the other entries; specifically update the lines defining the
"--context-max-tokens" and "--summary-length" options in the usage text so they
do not have the extra leading spaces and match the indentation style used
elsewhere in the options output.

src/sidecar/progress.js (1)

114-199: Consider splitting readProgress() to meet the 50-line function limit.

The readProgress function spans ~85 lines, exceeding the 50-line guideline. Consider extracting helpers such as:

• readConversationEntries(convPath) for lines 121-136
• processProgressJson(progressPath, ...) for lines 156-193

This would improve maintainability and align with the coding standards.

As per coding guidelines src/**/*.js: "Any function must not exceed 50 lines".

🤖 Prompt for AI Agents

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

In `@src/sidecar/progress.js` around lines 114 - 199, readProgress is >50 lines;
split it into helpers to meet the guideline: extract the conversation-reading
block into readConversationEntries(convPath) which returns {entries, convStat}
and preserves skipping blank/malformed lines and setting convStat via
fs.statSync, and extract the progress handling into
processProgressJson(progressPath, {entries, convStat, messages, latest,
lastActivity, lastActivityMs}) which reads progress.json safely, applies stage,
progress.stageLabel, progress.latestTool fallback logic, messagesReceived
override, updatedAt-based lastActivity/lastActivityMs adjustments, and ignores
malformed files; then refactor readProgress to call these two helpers and return
the same result object (including conditionally adding stage), while still using
computeLastActivity and extractLatest as before.

🤖 Prompt for all review comments with AI agents

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

Outside diff comments:
In `@src/cli.js`:
- Around line 40-92: The parseArgs function is over the 50-line limit; extract
the inline handlers into small helpers to reduce its length while preserving
behavior: create e.g. handleBooleanFlag(key, inlineValue, result, logger) to set
boolean flags and warn on inline values (used where isBooleanFlag(key) is true),
handleArrayFlag(key, val, result) to accumulate values for the 'exclude-mcp'
case (ensure it increments the caller's index when consumed), and
handleOptionWithValue(key, inlineValue, next, result) to parse and assign values
via parseValue or set true when no value is present; update parseArgs to call
these helpers from the main loop and keep DEFAULTS merging and the
positional-arguments push logic unchanged so semantics and i increments remain
identical.
- Around line 78-85: The current CLI parsing loop sets result[key] = true when a
non-boolean flag has no inline value and the next token is absent or is another
flag, which causes downstream errors (e.g., parseValue expecting a string).
Update the final else branch in the loop (the block handling inlineValue/next)
to reject missing values for value-flags: instead of assigning true, detect a
missing value for the flag identified by key and throw or return a clear error
like "Missing value for flag --<key>" (or call your CLI error handler), so
callers of parseValue(key, ...) never receive a boolean for a value-flag; keep
parseValue, inlineValue, next and result as the referenced symbols to locate and
modify the logic.

---

Nitpick comments:
In `@skill/auto-security/SKILL.md`:
- Around line 141-142: Replace the unhyphenated phrase "medium severity" with
the compound modifier "medium-severity" in the SKILL.md text (specifically where
the lines mention "If medium severity issues found" and "Security scan flagged
some medium-severity items" / "medium severity items") so the compound modifier
is grammatically correct; update both occurrences to "medium-severity issues" or
"medium-severity items" as appropriate, keeping the surrounding wording intact.

In `@src/cli-usage.js`:
- Around line 48-49: Fix the inconsistent indentation in the CLI usage options
list by aligning the two options shown to the same indentation as the other
entries; specifically update the lines defining the "--context-max-tokens" and
"--summary-length" options in the usage text so they do not have the extra
leading spaces and match the indentation style used elsewhere in the options
output.

In `@src/sidecar/progress.js`:
- Around line 114-199: readProgress is >50 lines; split it into helpers to meet
the guideline: extract the conversation-reading block into
readConversationEntries(convPath) which returns {entries, convStat} and
preserves skipping blank/malformed lines and setting convStat via fs.statSync,
and extract the progress handling into processProgressJson(progressPath,
{entries, convStat, messages, latest, lastActivity, lastActivityMs}) which reads
progress.json safely, applies stage, progress.stageLabel, progress.latestTool
fallback logic, messagesReceived override, updatedAt-based
lastActivity/lastActivityMs adjustments, and ignores malformed files; then
refactor readProgress to call these two helpers and return the same result
object (including conditionally adding stage), while still using
computeLastActivity and extractLatest as before.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 837ef997-59a8-4c76-8aa2-de7d44c60f42

📥 Commits

Reviewing files that changed from the base of the PR and between cb4e059 and 75a10de.

📒 Files selected for processing (12)

• CLAUDE.md
• hooks/post-failure.sh
• hooks/post-tool-use.sh
• hooks/pre-bash.sh
• package.json
• scripts/postinstall.js
• scripts/preuninstall.js
• skill/auto-security/SKILL.md
• src/cli-usage.js
• src/cli.js
• src/sidecar/progress.js
• src/utils/model-validator.js

🚧 Files skipped from review as they are similar to previous changes (3)

• hooks/pre-bash.sh
• hooks/post-failure.sh
• hooks/post-tool-use.sh