test(cli): pin help, error, and exit-code contracts ahead of the gunshi migration - #449
Merged
Merged
Conversation
…hi migration Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe CLI entry point now delegates dispatch to ChangesCLI dispatch and coverage
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant bin_ts
participant runCli
participant subcommand
participant analyzer
bin_ts->>runCli: Pass argv and CLI I/O
alt Subcommand
runCli->>subcommand: Dispatch docs, explain, install, or ci
subcommand-->>runCli: Return command result
else Analysis
runCli->>analyzer: Run resolved analysis options
analyzer-->>runCli: Return analysis result
end
runCli-->>bin_ts: Return exit code and termination mode
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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 |
Owner
Author
|
@coderabbitai review |
|
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![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 (2)
scripts/cli-e2e.mjs (2)
25-42: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a
timeoutto the child process.If the built CLI hangs,
execFileSyncblocks until the 15-minute job timeout. A per-check timeout fails fast and reports the failing check by name.execFileSyncsetserr.signaltoSIGTERMon timeout, so the existingsignalassertions produce a clear message.♻️ Proposed timeout
function runCli(args, opts = {}) { try { const stdout = execFileSync(process.execPath, [cliBin, ...args], { stdio: ['ignore', 'pipe', 'pipe'], encoding: 'utf8', + timeout: 60_000, ...opts });🤖 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 `@scripts/cli-e2e.mjs` around lines 25 - 42, Update runCli to pass a per-process timeout option to execFileSync, while preserving any caller-provided options through the existing opts spread. Use the timeout value expected by the checks so hung CLI invocations terminate promptly and continue returning the resulting status and signal through the existing error handling.
89-148: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: extract the repeated exit-code assertion.
The
signal/codeassertion pair repeats in six checks. A single helper keeps the messages consistent and shortens each check.♻️ Sketch of the helper
function expectExit(args, expected, opts) { const { code, signal, stdout, stderr } = runCli(args, opts); assert.equal(signal, null, `killed by signal ${signal} (stderr: ${stderr})`); assert.equal( code, expected, `\`svelte-vitals ${args.join(' ')}\` expected exit ${expected}, got ${code}: ${stderr}` ); return { stdout, stderr }; }🤖 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 `@scripts/cli-e2e.mjs` around lines 89 - 148, Optionally add an expectExit helper near the existing CLI test utilities that wraps runCli, validates signal is null and code matches the expected exit status, and returns stdout/stderr for callers that need them. Replace the repeated signal/code assertions in the affected check blocks, including the tests around clean, warning-only, minimum-health, and non-project behavior, while preserving their existing cleanup and stderr-specific assertions.
🤖 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 `@packages/cli/src/bin.ts`:
- Around line 5-12: Invoke the defined main function from the executable entry
point in bin.ts so runCli executes for every CLI command. Preserve the existing
exit-code handling inside main, including immediate process exits and assigned
process.exitCode values.
- Around line 7-8: Update the immediate-exit branch in bin.ts to await
completion of both standard output and standard error drains before calling
process.exit(code). Ensure all callers returning exit: 'immediate', including
install, ci, and argument-validation paths, use this shared draining behavior
rather than analyzer-specific or stdout-only handling.
---
Nitpick comments:
In `@scripts/cli-e2e.mjs`:
- Around line 25-42: Update runCli to pass a per-process timeout option to
execFileSync, while preserving any caller-provided options through the existing
opts spread. Use the timeout value expected by the checks so hung CLI
invocations terminate promptly and continue returning the resulting status and
signal through the existing error handling.
- Around line 89-148: Optionally add an expectExit helper near the existing CLI
test utilities that wraps runCli, validates signal is null and code matches the
expected exit status, and returns stdout/stderr for callers that need them.
Replace the repeated signal/code assertions in the affected check blocks,
including the tests around clean, warning-only, minimum-health, and non-project
behavior, while preserving their existing cleanup and stderr-specific
assertions.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ef94b35b-8efc-42f0-80f6-049688bc761b
⛔ Files ignored due to path filters (1)
packages/cli/test/__snapshots__/help-golden.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (8)
.github/workflows/ci.ymlpackage.jsonpackages/cli/src/bin.tspackages/cli/src/cli.tspackages/cli/src/install/cli.tspackages/cli/test/cli-contract.test.tspackages/cli/test/help-golden.test.tsscripts/cli-e2e.mjs
This was referenced Aug 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Phase 0 of the gunshi migration plan (#448,
docs/superpowers/specs/2026-08-10-gunshi-cli-migration-design.md): record the CLI's user-visible contracts before any gunshi code exists, so migration diffs are judged against pinned behavior. Also pays down audit backlog 2608-TEST-01/03 (bin.ts in-process seam + built-dist gate-flag E2E) — the suite stands on its own merits regardless of the migration.The seam
bin.ts(139 lines) is now a 14-line entry; the full dispatch lives in a newsrc/cli.tsasrunCli(argv, io?) → { code, exit: 'natural' | 'immediate' }. Two deliberate shapes:bin.tsends invoid main(), so importing a seam from it would execute the CLI against vitest's argv; and animport.meta.urlentry guard was rejected because npm/pnpm bin shims are symlinks (a realpath check would silently break the published binary).exitdiscriminator instead of a bare number — today's code uses three exit mechanisms deliberately (natural drain fordocs/explain/help/version to avoid truncating pipe writes; immediate exit forinstall/ci/argv errors, which may hold prompt/timer handles; flush-then-immediate for the analyzer, whose report is the largest write). Unifying them is not provably safe, so the thin entry reproduces each path's exact mechanism.Behavior preservation verified two ways: floor-smoke 8/8 unchanged on the branch, and a coordinator byte-comparison of main's rebuilt dist vs this branch's dist across 12 command surfaces (help ×5, version, docs list/show/redirect, explain list/error, flag-guard errors, rules×category conflict) — exit codes, stdout, and stderr all byte-identical.
The pins
help-golden.test.ts, 6 snapshots): root/docs/explain/install/ci--help+--version(digits normalized). When gunshi's generated format lands in Phase 2, the snapshot diff IS the review surface.cli-contract.test.ts, 16 cells / 10 classes): the fix(cli): reject flag-shaped and empty values on string flags #397/cli: --out-file - in its space-separated form writes a file instead of stdout #383 flag-guard class,docs-vs-./docsdispatch,docs showredirect, sub-command error surfaces — bin-level representatives, withresolve-args.test.tsstill owning the exhaustive matrix.scripts/cli-e2e.mjs, Node builtins only, 7 checks): the builtbin.json generated fixtures — clean→0,--fail-on warning→1,--min-health→1, non-project→2,--reporter jsonstdout parses. Wired into CI'stestjob after the floor-smoke step (dist already built); thefloor-smokejob is untouched.Two Phase-2 inputs discovered and pinned as-is (reality, not the doc)
ci <unknown-subcommand>prints its help to stdout and exits 2 — contradicting the design doc's "stdout empty on every exit-2 path" invariant. Pinned with a comment; either the code or the doc moves in Phase 2.util.parseArgsstrict:false passthrough). gunshi will likely reject them — an explicit Phase 2 decision point, now impossible to change by accident.No changeset — tests, an internal behavior-preserving refactor, and CI wiring only. Full suite 2,549 green.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests