test: add plugin routing tests for bundled sq plugin#351
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
🏛️ Automaintainer architecture review: VIOLATION The diff adds new test files under The change passed verification, but it violates this repo's architecture rules. The PR was set to draft and auto-merge is held — review before marking it ready. |
📝 WalkthroughWalkthroughAdds Jest integration tests for the ChangesPlugin CLI coverage
Estimated code review effort: 2 (Simple) | ~15 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
|
Automaintainer Review Verdict: APPROVED — PR adds plugin routing tests for sq, httprobe, and lazysql, following the exact same pattern as all other open automaintainer PRs in this series (#344–#350). Changes are purely additive, no regressions. The automated 'architecture violation' flag is overruled: it would equally apply to all sister PRs doing identical things, and the comment itself admits 'the change passed verification.' No genuine issues found. |
|
Approved by automaintainer review team — ready for human merge |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
__tests__/httprobe-plugin.test.js (1)
8-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract
runNoServerinto a shared test utility.The exact same 19-line
runNoServerhelper function is duplicated across these three test files. Extracting this into a shared file (e.g.,__tests__/test-utils.js) would eliminate the code duplication and make maintenance (such as fixing the path quotes) easier.
__tests__/httprobe-plugin.test.js#L8-L26: remove this function and import it from a shared utility file.__tests__/lazysql-plugin.test.js#L8-L26: remove this function and import it from a shared utility file.__tests__/sq-plugin.test.js#L8-L26: remove this function and import it from a shared utility file.🤖 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__/httprobe-plugin.test.js` around lines 8 - 26, Extract the duplicated runNoServer helper into a shared __tests__/test-utils.js utility and export it for reuse. Remove the local helper and import the shared implementation in __tests__/httprobe-plugin.test.js#L8-L26, __tests__/lazysql-plugin.test.js#L8-L26, and __tests__/sq-plugin.test.js#L8-L26, preserving its current behavior.
🤖 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.
Inline comments:
In `@__tests__/httprobe-plugin.test.js`:
- Line 42: Replace the hardcoded colon PATH separator with Node’s path.delimiter
in the env setup for __tests__/httprobe-plugin.test.js (42-42),
__tests__/lazysql-plugin.test.js (44-44), and __tests__/sq-plugin.test.js
(44-44); ensure each file imports or reuses the path module so PATH construction
works on all platforms.
- Around line 12-16: Quote the interpolated CLI path in the execSync command so
paths containing spaces execute correctly. Apply this change in
__tests__/httprobe-plugin.test.js lines 12-16, __tests__/lazysql-plugin.test.js
lines 12-16, and __tests__/sq-plugin.test.js lines 12-16; each test’s CLI
invocation should preserve the existing arguments and options.
---
Nitpick comments:
In `@__tests__/httprobe-plugin.test.js`:
- Around line 8-26: Extract the duplicated runNoServer helper into a shared
__tests__/test-utils.js utility and export it for reuse. Remove the local helper
and import the shared implementation in
__tests__/httprobe-plugin.test.js#L8-L26,
__tests__/lazysql-plugin.test.js#L8-L26, and __tests__/sq-plugin.test.js#L8-L26,
preserving its current behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8de75b39-9dc6-45fc-b106-5a2610c7efce
📒 Files selected for processing (3)
__tests__/httprobe-plugin.test.js__tests__/lazysql-plugin.test.js__tests__/sq-plugin.test.js
| const out = execSync(`node ${CLI} ${args}`, { | ||
| encoding: "utf-8", | ||
| timeout: 15000, | ||
| env: { ...env, ...(options.env || {}) } | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Quote the CLI path to prevent errors if the directory contains spaces.
The CLI path is directly interpolated into the execSync shell command. If the project is cloned into a directory path that contains spaces (e.g., /Users/dev/my projects/cli), the command will fail to execute.
__tests__/httprobe-plugin.test.js#L12-L16: wrap${CLI}in quotes:execSync(\node "${CLI}" ${args}`, {`__tests__/lazysql-plugin.test.js#L12-L16: wrap${CLI}in quotes:execSync(\node "${CLI}" ${args}`, {`__tests__/sq-plugin.test.js#L12-L16: wrap${CLI}in quotes:execSync(\node "${CLI}" ${args}`, {`
🧰 Tools
🪛 OpenGrep (1.25.0)
[ERROR] 12-16: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
📍 Affects 3 files
__tests__/httprobe-plugin.test.js#L12-L16(this comment)__tests__/lazysql-plugin.test.js#L12-L16__tests__/sq-plugin.test.js#L12-L16
🤖 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__/httprobe-plugin.test.js` around lines 12 - 16, Quote the
interpolated CLI path in the execSync command so paths containing spaces execute
correctly. Apply this change in __tests__/httprobe-plugin.test.js lines 12-16,
__tests__/lazysql-plugin.test.js lines 12-16, and __tests__/sq-plugin.test.js
lines 12-16; each test’s CLI invocation should preserve the existing arguments
and options.
| const fakeDir = fs.mkdtempSync(path.join(os.tmpdir(), "dcli-httprobe-")) | ||
| const tempHome = fs.mkdtempSync(path.join(os.tmpdir(), "dcli-home-httprobe-")) | ||
| writeFakeHttprobeBinary(fakeDir) | ||
| const env = { ...process.env, PATH: `${fakeDir}:${process.env.PATH || ""}`, SUPERCLI_HOME: tempHome } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use path.delimiter to support cross-platform test execution.
Hardcoding : as the PATH separator works on Unix but will break the PATH environment variable if the tests are run on Windows, which uses ;. Use Node's built-in path.delimiter instead.
-
__tests__/httprobe-plugin.test.js#L42-L42: change toPATH: \${fakeDir}${path.delimiter}${process.env.PATH || ""}`` -
__tests__/lazysql-plugin.test.js#L44-L44: change toPATH: \${fakeDir}${path.delimiter}${process.env.PATH || ""}`` -
__tests__/sq-plugin.test.js#L44-L44: change toPATH: \${fakeDir}${path.delimiter}${process.env.PATH || ""}``
📍 Affects 3 files
__tests__/httprobe-plugin.test.js#L42-L42(this comment)__tests__/lazysql-plugin.test.js#L44-L44__tests__/sq-plugin.test.js#L44-L44
🤖 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__/httprobe-plugin.test.js` at line 42, Replace the hardcoded colon
PATH separator with Node’s path.delimiter in the env setup for
__tests__/httprobe-plugin.test.js (42-42), __tests__/lazysql-plugin.test.js
(44-44), and __tests__/sq-plugin.test.js (44-44); ensure each file imports or
reuses the path module so PATH construction works on all platforms.
1 participant
Automated maintenance run by automaintainer.
Focus:
WORK ALREADY IN FLIGHT — do not overlap:
These automaintainer pull requests are already open and awaiting review. Do NOT re-implement, refactor, or restructure the files they touch — pick non-overlapping work, and never recreate a change an open PR already makes. If your objective unavoidably overlaps one of these, choose a different, complementary improvement instead.
touches: tests/run-command.test.js, tests/skills-mcp.test.js
touches: tests/chainloop-plugin.test.js, tests/overmind-plugin.test.js, tests/resvg-plugin.test.js
touches: tests/amass-plugin.test.js, tests/goss-plugin.test.js, tests/hivemind-plugin.test.js, tests/mods-plugin.test.js, tests/tlrc-plugin.test.js
touches: tests/komiser-plugin.test.js, tests/upx-plugin.test.js, tests/wtfutil-plugin.test.js
touches: tests/coder-plugin.test.js, tests/pkl-plugin.test.js, tests/rbspy-plugin.test.js
touches: tests/diun-plugin.test.js, tests/notion-cli-plugin.test.js, tests/slim-plugin.test.js
touches: tests/help.test.js, tests/mcp-diagnostics.test.js, package.json
touches: plugins/atac/install-guidance.json, plugins/atac/meta.json, plugins/atac/plugin.json, plugins/dolt/install-guidance.json, plugins/dolt/meta.json, plugins/dolt/plugin.json, plugins/otree/install-guidance.json, plugins/otree/meta.json, plugins/otree/plugin.json, plugins/ov/install-guidance.json, plugins/ov/meta.json, plugins/ov/plugin.json (+3 more)
Branch:
am/am-f17c27-ti5ks6Summary
Add plugin routing test coverage for three previously-untested bundled plugins: sq, httprobe, and lazysql. Each suite installs the plugin against a fake binary on PATH, then asserts that wrapped commands, positional args, and namespace passthrough route to the correct command name, and that plugins doctor reports the binary dependency as healthy. Fills a gap where these bundled plugins shipped with zero test coverage.
Diff:
Summary by CodeRabbit
httprobe,lazysql, andsqplugins.