test: measure the CLI entrypoints instead of excluding them - #48
Merged
Conversation
`**/index.ts` was excluded from coverage as if every index file were a
re-export barrel. Three were not: cli/src/index.ts (216 lines),
api/src/index.ts (150), create-modelgov/src/index.ts (143), all with zero
barrel exports. The exclusion also contradicted this config's own
`include`, which lists create-modelgov because "a regression here
misconfigures every new install" — then hid that package's largest file.
The reason they were unmeasured was structural, not incidental: both CLI
entrypoints ended in a bare top-level call, so importing the module RAN
the program. Splitting the executable into a `bin.ts` shim is what makes
them importable. Deliberately NOT an `import.meta.url === process.argv[1]`
guard — npx and pnpm invoke through .bin symlinks, and a path mis-compare
would make the installer silently do nothing, a far worse failure than the
untestability it would solve. Both built CLIs were run end-to-end to
confirm they still work.
Doing this immediately found a real bug. create-modelgov cast four flags
through unvalidated:
--framework next → reached adapterFor (no default case) and crashed
with "Cannot read properties of undefined"
--safety bogus → SUCCEEDED, writing `preset: bogus` into
modelgov.yaml — a scaffold that looks fine and
fails at gateway boot
--mode / --provider same class
All four now fail with the valid values listed and write nothing, matching
how --template already behaved.
Coverage went UP despite ~500 more lines under measurement:
cli 37.29/38.00/40.14/37.85 → 41.33/46.21/43.19/41.76 (gate raised)
global 79.28/79.16/71.22/77.19 → 80.05/81.55/72.43/78.15
api/src/index.ts (the server bootstrap, referenced by the Dockerfile,
compose and Helm) is left unsplit and now counts at 0% — a VISIBLE gap
rather than a hidden one. Closing it is the next ratchet.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThe CLI and scaffolder implementations moved into side-effect-free modules. Executable shims invoke the exported entrypoints. Tests cover dispatch, validation, scaffold generation, overwrite handling, and entrypoint behavior. Coverage measurement now includes selected entrypoints. ChangesCLI and scaffolder entrypoint separation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CLIShim
participant main
participant dispatch
User->>CLIShim: invoke CLI
CLIShim->>main: call main()
main->>dispatch: route command and arguments
dispatch-->>main: return command result
sequenceDiagram
participant User
participant ScaffolderShim
participant wizardMain
participant promptOptions
participant ScaffoldWriter
User->>ScaffolderShim: invoke scaffolder
ScaffolderShim->>wizardMain: call main()
wizardMain->>promptOptions: request options when required
promptOptions-->>wizardMain: return project options
wizardMain->>ScaffoldWriter: build and write scaffold
ScaffoldWriter-->>wizardMain: return completion result
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 2
🤖 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/create-modelgov/src/bin.ts`:
- Line 11: Update the main() invocation in the CLI entrypoint to attach a
rejection handler instead of discarding its Promise; set process.exitCode to a
failure status and write the rejection’s error message so invalid
non-interactive flags produce a controlled CLI error.
In `@packages/create-modelgov/src/index.ts`:
- Around line 58-65: Update resolveNonInteractive to validate supplied
framework, safety, mode, and provider flags before the !flags.template early
return, using the existing oneOf calls and validation constants. Preserve the
null return when no template is provided, while ensuring invalid standalone
flags throw their documented validation errors.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2feaae92-b2a8-4a01-b0c7-34f0099eb60c
📒 Files selected for processing (9)
packages/cli/package.jsonpackages/cli/src/bin.tspackages/cli/src/index.tspackages/cli/test/dispatch.test.tspackages/create-modelgov/package.jsonpackages/create-modelgov/src/bin.tspackages/create-modelgov/src/index.tspackages/create-modelgov/test/index.test.tsvitest.config.ts
The CodeQL jobs failed with "The job was not acquired by Runner of type hosted even after multiple attempts" — GitHub runner availability, not a code failure. That run is not retryable, so this nudges a fresh one.
…anged The previous commit moved the logic into index.ts and the executable into a new bin.ts, which meant `bin` pointed at ./dist/bin.js — a packaging change to two published CLIs. I verified it (packed the tarball, installed it, ran create-modelgov through the .bin symlink), but not making the change is better than proving it safe. Inverted: index.ts is the three-line shim, the logic lives in cli.ts and wizard.ts. `bin` stays ./dist/index.js, `exports` is untouched, and the logic is still importable and tested — which was the whole point. The two shims are named explicitly in the coverage exclusion rather than matched by a glob, so a future index.ts that grows logic is not silently exempted the way `**/index.ts` exempted 509 lines of entrypoint. create-modelgov's shim also catches: `--safety bogus` now prints "unknown safety preset 'bogus' (one of: dev, balanced, strict)" instead of a Node stack trace.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/cli.ts`:
- Around line 33-38: Update the documentation comment in packages/cli/src/cli.ts
(lines 33-38) to state that index.ts invokes the CLI, and revise the coverage
comment in vitest.config.ts (lines 157-163) to describe the actual split: CLI
logic in cli.ts with an executable shim in index.ts.
- Around line 211-214: Validate the --inputTokensEstimate value in
parseExplainFlags before storing it. Add a requireNumber helper beside
requireValue that rejects non-finite or negative values with a flag-specific
error, then use it in the --inputTokensEstimate case instead of Number so
invalid input fails before runExplain serializes the request.
In `@packages/create-modelgov/src/wizard.ts`:
- Around line 21-32: Update parseFlags so every value-taking option rejects a
missing next argument or one beginning with “-” instead of consuming it as a
value; report the malformed command through the existing error behavior. Apply
this consistently to --name, --framework, --template, --provider/--providers,
--safety, and --mode, and add coverage for each option’s missing/option-token
case.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4018a840-2c80-46c9-a6be-8479931c557c
📒 Files selected for processing (7)
packages/cli/src/cli.tspackages/cli/src/index.tspackages/cli/test/dispatch.test.tspackages/create-modelgov/src/index.tspackages/create-modelgov/src/wizard.tspackages/create-modelgov/test/wizard.test.tsvitest.config.ts
Three real defects from review, two of them in the validation this PR had just added. - `--framework next` with NO `--template` skipped validation entirely: the checks sat after resolveNonInteractive's early return, so the bad value fell through to the interactive wizard and crashed later in the scaffolder — exactly the failure the validation exists to prevent. Extracted validateFlags(), which runs first. - `--name --yes` set the name to "--yes" AND silently dropped --yes: val() took the next token blindly. @modelgov/cli already guarded this with requireValue; the wizard did not. - `--inputTokensEstimate abc` became NaN, which JSON.stringify writes as null, so the API received a silently different request and the caller saw no error. Same class as the scaffolder flags, now rejected with the offending value quoted. Also corrected three comments left stale by the shim inversion: the executable is index.ts and the logic is cli.ts / wizard.ts, not bin.ts. Both wizard fixes verified against the built CLI, not just in tests.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/create-modelgov/src/wizard.ts`:
- Around line 75-78: Reject empty scalar option values in the validation flow
around oneOf() in packages/create-modelgov/src/wizard.ts lines 75-78 by checking
each option against undefined rather than using truthiness, while preserving
provider validation. Add empty-value test cases for framework, safety, and mode
in packages/create-modelgov/test/wizard.test.ts lines 117-128 and assert that
each produces a validation error.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 73d245c1-0fb7-492d-b8d4-570464c3730d
📒 Files selected for processing (5)
packages/cli/src/cli.tspackages/cli/test/dispatch.test.tspackages/create-modelgov/src/wizard.tspackages/create-modelgov/test/wizard.test.tsvitest.config.ts
val() rejected a missing value and one starting with '-', but let "" through, and validateFlags used a truthy check — so `--framework ""` skipped validation and silently fell back to the default instead of reporting the malformed command. Rejected at parse time, and validateFlags now tests `!== undefined` for direct callers. This also corrects an earlier test here that encoded the worse behaviour: `--provider ""` used to yield [], quietly scaffolding with no providers. Omitting the flag is how you ask for none.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/create-modelgov/src/wizard.ts (1)
35-42: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject unknown option tokens.
parseFlags()silently discards every unrecognized argument that starts with-. For example,--framwork nextjsis ignored, andnextjsis assigned tof.dir. The command can then scaffold with the default framework in an unintended directory. Throw for unknown options before accepting positional arguments, and add a regression test.Proposed fix
else if (a === "--safety") f.safety = val() as SafetyPreset; else if (a === "--mode") f.mode = val() as DeployMode; - else if (!a.startsWith("-")) f.dir = a; + else if (a.startsWith("-")) throw new Error(`unknown option: ${a}`); + else f.dir = a;🤖 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 `@packages/create-modelgov/src/wizard.ts` around lines 35 - 42, Update parseFlags() to throw immediately for any unrecognized token beginning with "-" before processing positional directory arguments, preventing its following value from being assigned to f.dir. Add a regression test covering a misspelled option such as --framwork and asserting that parsing fails.
🤖 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.
Outside diff comments:
In `@packages/create-modelgov/src/wizard.ts`:
- Around line 35-42: Update parseFlags() to throw immediately for any
unrecognized token beginning with "-" before processing positional directory
arguments, preventing its following value from being assigned to f.dir. Add a
regression test covering a misspelled option such as --framwork and asserting
that parsing fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 68f7aefe-513d-4446-8e4c-1acc1bed494c
📒 Files selected for processing (2)
packages/create-modelgov/src/wizard.tspackages/create-modelgov/test/wizard.test.ts
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.
Closes the last item from the #34 review.
**/index.tswas excluded from coverage as if every index file were a re-export barrel. Three are not:packages/cli/src/index.tspackages/api/src/index.tspackages/create-modelgov/src/index.tsThe exclusion also contradicted this config's own
include, which listspackages/create-modelgov/src/**/*.tsbecause "a regression here misconfigures every new install" — and then hid that package's largest file.Why they were unmeasured
Structural, not incidental: both CLI entrypoints ended in a bare top-level call, so importing the module ran the program. No test could touch them. Splitting the executable into a
bin.tsshim is what makes them importable.Deliberately not an
import.meta.url === process.argv[1]guard: npx and pnpm invoke through.binsymlinks, and a path mis-compare there would make the installer silently do nothing — a far worse failure than the untestability it solves. Both built CLIs were run end-to-end to confirm they still work before any test was written.This found a real bug
create-modelgovcast four CLI flags through with no validation:--framework next(plausible typo fornextjs) reachedadapterFor, which has nodefaultcase, and died withCannot read properties of undefined (reading 'files').--safety bogussucceeded and wrotepreset: bogusstraight intomodelgov.yaml. A scaffold that looks fine and fails at gateway boot — precisely the failure the coverage config cites as its reason for measuring this package, shipped in the file it excluded.--modeand--providerwere the same class.All four now fail with the valid values listed and write nothing, matching how
--templatealready behaved. Verified against the built CLI, not just in unit tests.I found it by accident: my own test fixture used
framework: "next"andparseFlagsaccepted it, because nothing validated. The typechecker then caught a second invalid value (mode: "compose") in the same fixture once the types were actually applied.Coverage went up
Despite ~500 more lines entering the measured surface:
packages/cliCLI gates ratcheted 35/36/37/35 → 38/43/40/38, keeping the ~3-point margin this config documents for CI's Node 22 measuring below local Node 24. Global gates left as-is: the surface moved, and the existing comment sets that precedent.
37 new tests (20 wizard, 17 CLI dispatch) covering flag parsing and validation, non-interactive resolution, the overwrite guard including cancel-is-not-consent, prompt→options mapping, command routing,
doctor productionvs baredoctor, exit-code propagation, and unknown-command handling.Deliberately not done
packages/api/src/index.tsis left unsplit. Its package gate passes with the file counted, anddist/index.jsis referenced by the Dockerfile, compose and Helm — not worth restructuring for coverage. It now counts at 0%, visible in the report rather than hidden behind an exclusion. Closing it is the next ratchet, not this one.pnpm verifygreen: 1262 tests, all thresholds met.Summary by CodeRabbit
New Features
Bug Fixes
Tests