test: add plugin routing tests for bundled chainloop plugin#349
test: add plugin routing tests for bundled chainloop plugin#349javimosch wants to merge 3 commits into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Let me reconsider — I have no stated single objective beyond the PR-overlap constraint. The verification gate passed, but this change may not address the stated objective. Review before merging. |
|
🏛️ Automaintainer architecture review: VIOLATION The change 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. |
|
Automaintainer Review Verdict: APPROVED — Changes add plugin routing tests for chainloop, overmind, and resvg following the established eza/goose test conventions (isolated temp dirs, fake binary on PATH, passthrough + arg-forwarding + doctor checks, proper afterAll cleanup). Code quality is good. The external comments raising an 'architecture violation' (no changes outside plugin folder) and 'off-objective' concerns are not supported by evidence: all other open automaintainer PRs (#345–#348) also add files to |
|
Approved by automaintainer review team — ready for human merge |
📝 WalkthroughWalkthroughAdds Jest integration suites for chainloop, overmind, and resvg, covering CLI passthrough routing, argument forwarding, and plugin dependency health checks using temporary fake binaries. ChangesPlugin integration test coverage
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
__tests__/chainloop-plugin.test.js (2)
8-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared test helpers to reduce duplication across the three plugin test files.
runNoServerand the overall test scaffold (temp dir setup, fake binary creation, install/remove lifecycle) are copy-pasted acrosschainloop-plugin.test.js,overmind-plugin.test.js, andresvg-plugin.test.js. A shared helper module (e.g.,__tests__/helpers/plugin-test-utils.js) would centralizerunNoServer, fake-binary creation, and thedescribelifecycle, so fixes like theexecFileSyncchange above only need to be applied once.🤖 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__/chainloop-plugin.test.js` around lines 8 - 26, Extract the duplicated plugin test scaffold from chainloop-plugin.test.js, overmind-plugin.test.js, and resvg-plugin.test.js into a shared helper module such as plugin-test-utils.js. Centralize runNoServer, temporary-directory setup, fake-binary creation, and install/remove lifecycle handling, then update each test file to reuse the shared helpers while preserving its existing test behavior.
12-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
execSyncwith string interpolation across all three plugin test files. All threerunNoServerhelpers build a shell command string (node ${CLI} ${args}), which breaks if the project path contains spaces and triggers shell interpretation of arguments. The shared root cause is one function duplicated across three files; switching toexecFileSyncwith an argument array (or extracting a shared helper that does so) fixes all sites at once.
__tests__/chainloop-plugin.test.js#L12-L16: replaceexecSync(node ${CLI} ${args}, ...)withexecFileSync("node", [CLI, ...args], ...)and update call sites to pass arrays.__tests__/overmind-plugin.test.js#L12-L16: same change.__tests__/resvg-plugin.test.js#L12-L16: same change.🤖 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__/chainloop-plugin.test.js` around lines 12 - 16, Replace the duplicated runNoServer helpers’ shell-based execSync invocation with execFileSync using node and an argument array, and update callers to provide args as arrays rather than interpolated strings. Apply this in __tests__/chainloop-plugin.test.js lines 12-16, __tests__/overmind-plugin.test.js lines 12-16, and __tests__/resvg-plugin.test.js lines 12-16.Source: Linters/SAST tools
🤖 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__/chainloop-plugin.test.js`:
- Around line 46-48: Unchecked plugin installation results obscure setup
failures. In the beforeAll install setup, capture the result of runNoServer and
assert r.ok is true in __tests__/chainloop-plugin.test.js lines 46-48,
__tests__/overmind-plugin.test.js lines 46-48, and
__tests__/resvg-plugin.test.js lines 46-48; apply the same change in all three
suites.
In `@__tests__/overmind-plugin.test.js`:
- Around line 12-16: Replace the interpolated execSync invocation in the test
helper with execFileSync, passing CLI and the command arguments as separate
array entries while preserving encoding, timeout, and merged environment
options.
In `@__tests__/resvg-plugin.test.js`:
- Around line 12-16: Replace the interpolated command invocation in the test
helper with execFileSync, passing CLI as the executable and args as an argument
array. Preserve the existing encoding, timeout, and merged environment options
while avoiding shell interpretation of the CLI path.
---
Nitpick comments:
In `@__tests__/chainloop-plugin.test.js`:
- Around line 8-26: Extract the duplicated plugin test scaffold from
chainloop-plugin.test.js, overmind-plugin.test.js, and resvg-plugin.test.js into
a shared helper module such as plugin-test-utils.js. Centralize runNoServer,
temporary-directory setup, fake-binary creation, and install/remove lifecycle
handling, then update each test file to reuse the shared helpers while
preserving its existing test behavior.
- Around line 12-16: Replace the duplicated runNoServer helpers’ shell-based
execSync invocation with execFileSync using node and an argument array, and
update callers to provide args as arrays rather than interpolated strings. Apply
this in __tests__/chainloop-plugin.test.js lines 12-16,
__tests__/overmind-plugin.test.js lines 12-16, and
__tests__/resvg-plugin.test.js lines 12-16.
🪄 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: 21967be6-fe9a-4d8b-992d-df60caac4d53
📒 Files selected for processing (3)
__tests__/chainloop-plugin.test.js__tests__/overmind-plugin.test.js__tests__/resvg-plugin.test.js
| beforeAll(() => { | ||
| runNoServer("plugins install ./plugins/chainloop --on-conflict replace --json", { env }) | ||
| }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
beforeAll does not assert plugin install success across all three test files. None of the three suites check the return value of runNoServer("plugins install ..."); if installation fails, every downstream test fails with cryptic JSON.parse errors instead of a clear failure message. The shared root cause is the same unchecked install call in each file.
__tests__/chainloop-plugin.test.js#L46-L48: capture the result and addexpect(r.ok).toBe(true).__tests__/overmind-plugin.test.js#L46-L48: same change.__tests__/resvg-plugin.test.js#L46-L48: same change.
📍 Affects 3 files
__tests__/chainloop-plugin.test.js#L46-L48(this comment)__tests__/overmind-plugin.test.js#L46-L48__tests__/resvg-plugin.test.js#L46-L48
🤖 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__/chainloop-plugin.test.js` around lines 46 - 48, Unchecked plugin
installation results obscure setup failures. In the beforeAll install setup,
capture the result of runNoServer and assert r.ok is true in
__tests__/chainloop-plugin.test.js lines 46-48,
__tests__/overmind-plugin.test.js lines 46-48, and
__tests__/resvg-plugin.test.js lines 46-48; apply the same change in all three
suites.
| const out = execSync(`node ${CLI} ${args}`, { | ||
| encoding: "utf-8", | ||
| timeout: 15000, | ||
| env: { ...env, ...(options.env || {}) } | ||
| }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
execSync with string interpolation — same issue as chainloop-plugin.test.js.
Use execFileSync with an argument array to avoid shell interpretation of the CLI path.
🧰 Tools
🪛 OpenGrep (1.23.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)
🤖 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__/overmind-plugin.test.js` around lines 12 - 16, Replace the
interpolated execSync invocation in the test helper with execFileSync, passing
CLI and the command arguments as separate array entries while preserving
encoding, timeout, and merged environment options.
Source: Linters/SAST tools
| const out = execSync(`node ${CLI} ${args}`, { | ||
| encoding: "utf-8", | ||
| timeout: 15000, | ||
| env: { ...env, ...(options.env || {}) } | ||
| }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
execSync with string interpolation — same issue as chainloop-plugin.test.js.
Use execFileSync with an argument array to avoid shell interpretation of the CLI path.
🧰 Tools
🪛 OpenGrep (1.23.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)
🤖 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__/resvg-plugin.test.js` around lines 12 - 16, Replace the
interpolated command invocation in the test helper with execFileSync, passing
CLI as the executable and args as an argument array. Preserve the existing
encoding, timeout, and merged environment options while avoiding shell
interpretation of the CLI path.
Source: Linters/SAST tools
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/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-ti47ofSummary
Add plugin routing tests for three bundled passthrough plugins: chainloop, overmind, and resvg. Each suite spins up a fake binary on PATH, installs the plugin into an isolated SUPERCLI_HOME, and verifies namespace passthrough routing (command name + raw output), argument forwarding, and that 'plugins doctor' reports the binary dependency as healthy. These plugins previously had no test coverage; the tests follow the existing eza/goose plugin-test conventions.
Diff:
Summary by CodeRabbit