fix(cli): stabilization batch + CLI test suite#234
Conversation
- F60: honor `mainOpts.args` instead of always reading process.argv - F40: parse args first so option values aren't mistaken for the subcommand - F41: define `-h` short flag (documented but missing) - F38: install SIGINT cleanup in watch mode (checked runtimeArgs, not args) - F39: add `duplex: "half"` for streamed request bodies (`-d @-` / `-d @file`) - F42: only override entry `tls` from CLI flags; `--tls` w/o cert/key now errors - F43: explicit missing `--static` dir errors; implicit `public` stays silent - F44: friendly one-line errors for bad flags / missing entry + usage hint - F44: load `.env` before `fetch`; treat schemeless host:port as http - F59: escape error output in the dev error page HTML - F58: NO_COLOR any non-empty value disables; gate colors on TTY - loader: fix JSX hint to recommend `--import` (not `--register`) - F46: add test/cli.test.ts covering the above (execa + in-process main) Co-Authored-By: Claude Fable 5 <[email protected]>
📝 WalkthroughWalkthroughChangesThe CLI now has unified argument parsing, earlier environment loading, improved URL and streamed-body handling, stricter serve configuration validation, escaped error rendering, updated compatibility guidance, and expanded integration tests using a source-based runner. CLI behavior and integration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
commit: |
![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 `@src/cli/_utils.ts`:
- Around line 4-13: Update the noColor initializer to parse FORCE_COLOR rather
than checking its truthiness, ensuring the explicit value "0" disables colors
while preserving the existing handling for other FORCE_COLOR values and
NO_COLOR/TERM conditions.
In `@src/cli/serve.ts`:
- Around line 35-48: Update the static path validation in the serve flow to
require that staticDir exists and is a directory, using the same
statSync(...).isDirectory() pattern as parseArgs(). Preserve the implicit public
fallback behavior, while explicit --static paths that are missing or files must
throw the existing clear directory-not-found error.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ea19094d-32c6-4eaf-a3ba-ec81c66a9202
📒 Files selected for processing (9)
src/cli/_utils.tssrc/cli/fetch.tssrc/cli/main.tssrc/cli/serve.tssrc/loader.tstest/_cli-run.tstest/cli.test.tstest/fixtures/cli/data.txttest/fixtures/cli/server.ts
| const noColor = /* @__PURE__ */ (() => { | ||
| const env = globalThis.process?.env ?? {}; | ||
| return env.NO_COLOR === "1" || env.TERM === "dumb"; | ||
| const proc = globalThis.process; | ||
| const env = proc?.env ?? {}; | ||
| if (env.FORCE_COLOR) { | ||
| return false; | ||
| } | ||
| if (env.NO_COLOR || env.TERM === "dumb") { | ||
| return true; | ||
| } | ||
| return !proc?.stdout?.isTTY; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== src/cli/_utils.ts =="
nl -ba src/cli/_utils.ts | sed -n '1,80p'
echo
echo "== tests mentioning FORCE_COLOR / NO_COLOR / isTTY =="
rg -n "FORCE_COLOR|NO_COLOR|isTTY|noColor" src test tests . || trueRepository: h3js/srvx
Length of output: 211
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== src/cli/_utils.ts =="
cat -n src/cli/_utils.ts | sed -n '1,120p'
echo
echo "== matching tests / references =="
rg -n "FORCE_COLOR|NO_COLOR|isTTY|noColor" src test tests . || trueRepository: h3js/srvx
Length of output: 3373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== test/cli.test.ts =="
cat -n test/cli.test.ts | sed -n '1,220p'Repository: h3js/srvx
Length of output: 8162
🌐 Web query:
supports-color FORCE_COLOR 0 disable colors documentation
💡 Result:
To disable color output in command-line applications that utilize the supports-color library or follow its conventions, you can set the environment variable FORCE_COLOR to 0 [1][2][3]. When FORCE_COLOR=0 is set, it explicitly overrides other terminal color support detection mechanisms and forces the application to disable ANSI color output [1][2][4][5]. In addition to this, the NO_COLOR standard is widely supported across the industry as a way to disable color output [6][7]. According to the NO_COLOR specification, any non-empty value for the NO_COLOR environment variable (e.g., NO_COLOR=1) should signal applications to refrain from adding ANSI color to their output [6][8][5]. While both are used to disable colors, FORCE_COLOR=0 is specific to libraries and tools that implement the supports-color convention, whereas NO_COLOR is a broader, cross-platform standard intended for any CLI software [9][10][6]. If both are present, the specific application's implementation determines priority, though FORCE_COLOR is often treated as having higher precedence in libraries that support both [9][10].
Citations:
- 1: https://github.com/chalk/supports-color
- 2: https://www.npmjs.com/package/supports-color
- 3: https://www.npmjs.com/package/supports-color?activeTab=readme
- 4: fix: treat FORCE_COLOR=0 as disabled per force-color.org alexeyraspopov/picocolors#101
- 5: https://www.npmjs.com/package/@visulima/is-ansi-color-supported
- 6: https://no-color.org/
- 7: https://man7.org/linux/man-pages/man5/terminal-colors.d.5.html
- 8: https://terminfo.dev/fundamentals/color-detection
- 9: support for NO_COLOR env var chalk/supports-color#105
- 10: tty: add NO_COLOR and FORCE_COLOR support nodejs/node#26485
Handle FORCE_COLOR=0 explicitly. env.FORCE_COLOR is truthy for "0", so this branch enables colors when the convention expects them to be disabled. Parse the env value instead of checking truthiness.
🤖 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 `@src/cli/_utils.ts` around lines 4 - 13, Update the noColor initializer to
parse FORCE_COLOR rather than checking its truthiness, ensuring the explicit
value "0" disables colors while preserving the existing handling for other
FORCE_COLOR values and NO_COLOR/TERM conditions.
| // F43: an explicit `--static` pointing at a missing dir must error; the | ||
| // implicit `public` default may stay silent. | ||
| const explicitStatic = !!cliOpts.static; | ||
| const staticDir = resolve( | ||
| cliOpts.dir || (loaded.url ? dirname(fileURLToPath(loaded.url)) : "."), | ||
| cliOpts.static || "public", | ||
| ); | ||
| cliOpts.static = existsSync(staticDir) ? staticDir : ""; | ||
| if (existsSync(staticDir)) { | ||
| cliOpts.static = staticDir; | ||
| } else if (explicitStatic) { | ||
| throw new Error(`--static directory not found: ${staticDir}`); | ||
| } else { | ||
| cliOpts.static = ""; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
--static existence check doesn't verify it's a directory.
existsSync(staticDir) passes for files too. An explicit --static ./some-file would silently set cliOpts.static to a file path and only fail later inside serveStatic on each request, instead of failing fast with a clear message like the sibling "not found" branch. parseArgs() in src/cli/main.ts already uses statSync(...).isDirectory() for the analogous entry/dir check — worth mirroring here.
🐛 Suggested fix
- if (existsSync(staticDir)) {
+ if (existsSync(staticDir) && statSync(staticDir).isDirectory()) {
cliOpts.static = staticDir;
} else if (explicitStatic) {
throw new Error(`--static directory not found: ${staticDir}`);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // F43: an explicit `--static` pointing at a missing dir must error; the | |
| // implicit `public` default may stay silent. | |
| const explicitStatic = !!cliOpts.static; | |
| const staticDir = resolve( | |
| cliOpts.dir || (loaded.url ? dirname(fileURLToPath(loaded.url)) : "."), | |
| cliOpts.static || "public", | |
| ); | |
| cliOpts.static = existsSync(staticDir) ? staticDir : ""; | |
| if (existsSync(staticDir)) { | |
| cliOpts.static = staticDir; | |
| } else if (explicitStatic) { | |
| throw new Error(`--static directory not found: ${staticDir}`); | |
| } else { | |
| cliOpts.static = ""; | |
| } | |
| // F43: an explicit `--static` pointing at a missing dir must error; the | |
| // implicit `public` default may stay silent. | |
| const explicitStatic = !!cliOpts.static; | |
| const staticDir = resolve( | |
| cliOpts.dir || (loaded.url ? dirname(fileURLToPath(loaded.url)) : "."), | |
| cliOpts.static || "public", | |
| ); | |
| if (existsSync(staticDir) && statSync(staticDir).isDirectory()) { | |
| cliOpts.static = staticDir; | |
| } else if (explicitStatic) { | |
| throw new Error(`--static directory not found: ${staticDir}`); | |
| } else { | |
| cliOpts.static = ""; | |
| } |
🤖 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 `@src/cli/serve.ts` around lines 35 - 48, Update the static path validation in
the serve flow to require that staticDir exists and is a directory, using the
same statSync(...).isDirectory() pattern as parseArgs(). Preserve the implicit
public fallback behavior, while explicit --static paths that are missing or
files must throw the existing clear directory-not-found error.
2 participants
CLI batch from the v1 stabilization review.
main()now honorsmainOpts.args ?? process.argv.slice(2)(adds an in-process path for tests).-p 8080 fetch /api) is no longer mistaken for the subcommand.-hshort flag (documented in usage but previously threwERR_PARSE_ARGS_UNKNOWN_OPTION).runtimeArgs, notargs), so the dev-server child isn't orphaned onkill -INT.duplex: "half"when the request body is a stream, so-d @-and-d @filework on Node.tlswhen CLI flags supply it; a bare--tlswithout--cert/--keynow errors instead of silently downgrading to HTTP.--static ./missingnow errors; the implicitpublicdefault stays silent.--entryconflicts print a one-line message + usage hint and exit non-zero (no raw stack traces).srvx fetchloads.envbefore running; schemelesshost:portis treated ashttp(fixeslocalhost:3000/api).error.stack/title before interpolating into the dev error page HTML.NO_COLORnow disables on any non-empty value and colors are gated on a TTY (withFORCE_COLORoverride).--import jiti/register(was--register).test/cli.test.ts(14 tests, via execa + a source runner and one in-processmain({args})case):--version,--help/-h, serve+fetch, fetch exit codes (0/22), F40, F42, F43, F39 stdin+file, F44 unknown-flag/env/schemeless-URL. Watch-reload and SIGINT-orphan tests were left out to avoid flakiness.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
--helpguidance.Bug Fixes
NO_COLOR, terminal interactivity, andFORCE_COLOR.jiti/register.