test(#495): 28 unit tests for config-commands.js (upsertCommand, removeCommandsByNamespace, showConfig)

[javimosch]

What

Direct unit test suite for cli/config-commands.js — the module that owns command upsert and removal logic in the local config cache.

Why

config.test.js exercises these functions via the config.js re-export but shares mocks with config-core tests, limiting isolation. A dedicated file mocks config-core and plugins-store directly so each exported function can be exercised independently with precise assertions.

Tests (28 assertions, all green)

upsertCommand

• Null cache → falls back to emptyConfig, pushes new command
• Empty commands array → appends
• Non-array commands field → treated as empty
• Appends when namespace+resource+action do not match any existing entry
• Updates in-place when all three key fields match
• Correct command updated when multiple commands are present
• Original cache array not mutated (slice isolation)
• Returns the commandDef reference
• writeCache called exactly once
• Null entries in commands array are preserved and do not cause crashes during match search

removeCommandsByNamespace

• Null cache → returns 0, still writes
• No match → returns 0
• Single match removed, returns 1
• Multiple matches removed, correct count returned
• Non-matching namespace commands kept intact
• Non-array commands field handled (treated as empty)
• Null entries skipped without throwing
• writeCache called exactly once
• Empty-string namespace removes commands where namespace === ""

showConfig

• No cache → {cached: false, message: ...}
• Full cache → correct counts for commands, plugins, server_plugins, mcp_servers, specs
• Missing fields → defaults to 0
• fetchedAt converted to ISO string
• server_plugins counts keys from installed sub-object
• installed absent → server_plugins is 0
• cacheFilePath() return value forwarded as cacheFile
• plugins count matches listInstalledPlugins length

Verification

npx jest __tests__/config-commands.test.js --no-coverage
# Test Suites: 1 passed, 1 total
# Tests:       28 passed, 28 total
# Time:        0.207 s

Closes #495 (mago task #495)

Summary by CodeRabbit

• Tests
  • Added broad automated coverage for CLI config, execution, and planning flows.
  • Improves confidence in cache updates, command removal, config display, and plan execution output.
  • Verifies human-friendly and JSON outputs, error handling, and stream behavior across multiple scenarios.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[coderabbitai]

📝 Walkthrough

Walkthrough

Two Jest test files are added. __tests__/config-commands.test.js tests upsertCommand, removeCommandsByNamespace, and showConfig by mocking config-core and plugins-store. __tests__/execute-handler.test.js tests handleExecute, handlePlan, and handleExecutePlan by mocking executor and plan-runtime.

Changes

CLI Test Coverage

Layer / File(s)	Summary
config-commands test suite __tests__/config-commands.test.js	Mocks config-core and plugins-store; tests upsertCommand (cache-null, append, replace, null-entry preservation), removeCommandsByNamespace (counts, filtering, edge cases), and showConfig (cache presence, field counts, ISO date conversion).
execute-handler test suite __tests__/execute-handler.test.js	Mocks executor and plan-runtime; tests handleExecute (argument forwarding, stream wiring, writeLog fields, human/envelope output, error propagation), handlePlan (local vs server paths, fetch failure with code 105), and handleExecutePlan (plan execute endpoint, error handling).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐇 Hoppy tests now guard the code,
Mocks and stubs line every road,
upsert, remove, show and plan,
execute as fast as I can!
No bug slips past this fuzzy clan. 🌿

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

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

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the new unit test suite for config-commands.js and names the main covered functions.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch mago/task-495

---

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.}

[coderabbitai]

🧹 Nitpick comments (2)

__tests__/execute-handler.test.js (2)

318-320: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Assert the JSON header on the server plan request.

These tests lock down the URL/body, but not the Content-Type: application/json header that cli/execute-handler.js:49-54 currently sends. A regression there would still pass here while breaking the server contract.

Suggested test tightening

 expect(global.fetch).toHaveBeenCalledWith(
   "https://srv.io/api/plans",
-  expect.objectContaining({ method: "POST" })
+  expect.objectContaining({
+    method: "POST",
+    headers: { "Content-Type": "application/json" },
+  })
 );

Also applies to: 352-356

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@__tests__/execute-handler.test.js` around lines 318 - 320, Tighten the
execute-handler tests so they also verify the server plan request sends
Content-Type: application/json in addition to the URL and POST method. Update
the fetch expectation in execute-handler.test.js for the request asserted around
the plan submission path, using the existing global.fetch check to validate the
headers object as well; mirror the same assertion in the other plan-request case
referenced by the review so both paths are covered.

---

259-267: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Restore the original fetch instead of deleting it

Both hooks delete Node 18’s built-in global.fetch. Save the original value once and restore it in afterEach so later tests in the same process don’t inherit a missing fetch.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@__tests__/execute-handler.test.js` around lines 259 - 267, The test hooks are
removing Node 18’s built-in global.fetch instead of restoring it, which can leak
state into later tests. In __tests__/execute-handler.test.js, update the
beforeEach/afterEach setup to save the original global.fetch once, mock or clear
it during the test, and restore that saved value in afterEach rather than
deleting it. Use the existing beforeEach/afterEach hooks and the global.fetch
reference to locate the fix.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@__tests__/execute-handler.test.js`:
- Around line 318-320: Tighten the execute-handler tests so they also verify the
server plan request sends Content-Type: application/json in addition to the URL
and POST method. Update the fetch expectation in execute-handler.test.js for the
request asserted around the plan submission path, using the existing
global.fetch check to validate the headers object as well; mirror the same
assertion in the other plan-request case referenced by the review so both paths
are covered.
- Around line 259-267: The test hooks are removing Node 18’s built-in
global.fetch instead of restoring it, which can leak state into later tests. In
__tests__/execute-handler.test.js, update the beforeEach/afterEach setup to save
the original global.fetch once, mock or clear it during the test, and restore
that saved value in afterEach rather than deleting it. Use the existing
beforeEach/afterEach hooks and the global.fetch reference to locate the fix.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: df97cc4f-6db8-476d-a4cc-8bc3669bb257

📥 Commits

Reviewing files that changed from the base of the PR and between afda2f7 and ed0e403.

📒 Files selected for processing (2)

• __tests__/config-commands.test.js
• __tests__/execute-handler.test.js

[javimosch]

Review — Head of Org Engineering: The diff adds new test files for config-commands and execute-handler modules, which aligns with the company's focus on unit/integration tests for CLI modules. The tests are syntactically valid, scoped only to new test files, and contain no secrets, destructive changes, or out-of-scope edits.

(approved — review-only mode; merge when ready.)