Skip to content

fix(cli): stabilization batch + CLI test suite #234

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pi0 merged 2 commits into from
Jul 14, 2026
Merged

fix(cli): stabilization batch + CLI test suite #234

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1e73c66
fix(cli): stabilization batch and add CLI test suite
pi0 Jul 14, 2026
084599b
Merge branch 'main' into fix/cli-batch
pi0 Jul 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions src/cli/_utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,16 @@
// Colors support for terminal output
// F58: honor the NO_COLOR convention (ANY non-empty value disables) and only
// emit escape sequences to an interactive TTY (unless FORCE_COLOR is set).
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;
Comment on lines 4 to +13

@coderabbitai coderabbitai Bot Jul 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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 . || true

Repository: 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 . || true

Repository: 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:

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.

})();

const _c =
Expand Down
19 changes: 13 additions & 6 deletions src/cli/fetch.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,10 @@ export async function cliFetch(
fetchHandler = loaded.fetch;
} else {
stderr.write(`* Fetching remote URL: ${inputURL}\n`);
if (!URL?.canParse(inputURL)) {
// F44: `URL.canParse("localhost:3000/api")` is true (scheme "localhost:"),
// so only treat inputs with an explicit `scheme://` as absolute URLs;
// everything else (host, host:port, host/path) is a schemeless http host.
if (!/^[a-zA-Z][a-zA-Z\d+\-.]*:\/\//.test(inputURL)) {
inputURL = `http${cliOpts.tls ? "s" : ""}://${inputURL}`;
}
fetchHandler = globalThis.fetch;
Expand Down Expand Up @@ -85,9 +88,11 @@ export async function cliFetch(

// Build body
let body: BodyInit | undefined;
let isStream = false;
if (cliOpts.data !== undefined) {
if (cliOpts.data === "@-") {
// Read from stdin
isStream = true;
body = new ReadableStream({
async start(controller) {
for await (const chunk of stdin) {
Expand All @@ -98,6 +103,7 @@ export async function cliFetch(
});
} else if (cliOpts.data.startsWith("@")) {
// Read from file as stream
isStream = true;
body = Readable.toWeb(createReadStream(cliOpts.data.slice(1))) as unknown as ReadableStream;
} else {
body = cliOpts.data;
Expand All @@ -110,11 +116,12 @@ export async function cliFetch(
inputURL,
`http${cliOpts.tls ? "s" : ""}://${cliOpts.host || cliOpts.hostname || "localhost"}`,
);
const req = new Request(url, {
method,
headers,
body,
});
// F39: a streamed body (`-d @-` / `-d @file`) requires `duplex: "half"` on Node.
const reqInit: RequestInit & { duplex?: "half" } = { method, headers, body };
if (isStream) {
reqInit.duplex = "half";
}
const req = new Request(url, reqInit);

// Verbose: print request info
if (cliOpts.verbose) {
Expand Down
143 changes: 76 additions & 67 deletions src/cli/main.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,19 @@ import { usage } from "./usage.ts";
import { srvxMeta } from "./_meta.ts";

export async function main(mainOpts: MainOptions): Promise<void> {
const args = process.argv.slice(2);
const cliOpts = parseArgs(args);
// F60: honor an explicit `args` array (used by tests); fall back to process.argv
const args = mainOpts.args ?? process.argv.slice(2);

let cliOpts: CLIOptions;
try {
cliOpts = parseArgs(args);
} catch (error) {
// F44: surface parse/entry-resolution problems as a one-line message + hint
const command = mainOpts.usage?.command || "srvx";
console.error(c.red((error as Error).message || String(error)));
console.error(c.gray(`Run \`${command} --help\` for usage.`));
process.exit(1);
}

// Handle version flag
if (cliOpts.version) {
Expand All @@ -22,11 +33,20 @@ export async function main(mainOpts: MainOptions): Promise<void> {
// Handle help flag
if (cliOpts.help) {
console.log(usage(mainOpts));
process.exit(cliOpts.help ? 0 : 1);
process.exit(0);
}

// Resolve .env files (used by both serve and fetch modes)
const envFiles = [".env", cliOpts.prod ? ".env.production" : ".env.local"].filter((f) =>
existsSync(f),
);

// Fetch mode
if (cliOpts.mode === "fetch") {
// F44: load env before fetching (the entry/handler may rely on env vars)
for (const envFile of [...envFiles].reverse() /* overrides first */) {
process.loadEnvFile?.(envFile);
}
try {
const res = await cliFetch(cliOpts);
process.exit(res.ok ? 0 : 22);
Expand All @@ -44,10 +64,6 @@ export async function main(mainOpts: MainOptions): Promise<void> {
// Log versions
console.log(c.gray([...versions(mainOpts), cliOpts.prod ? "prod" : "dev"].join(" · ")));

// Resolve .env files
const envFiles = [".env", cliOpts.prod ? ".env.production" : ".env.local"].filter((f) =>
existsSync(f),
);
if (envFiles.length > 0) {
console.log(
`${c.gray(`Loading environment variables from ${c.magenta(envFiles.join(", "))}`)}`,
Expand Down Expand Up @@ -81,78 +97,69 @@ export async function main(mainOpts: MainOptions): Promise<void> {
}

function parseArgs(args: string[]): CLIOptions {
const pArg0 = args.find((a) => !a.startsWith("-"));
const mode = pArg0 === "fetch" || pArg0 === "curl" ? "fetch" : "serve";

const commonArgs = {
help: { type: "boolean" },
// Parse with a combined schema so option VALUES are never mistaken for the
// subcommand (F40). serve/fetch short flags don't collide, so one pass is safe.
const options = {
// --- Common flags ---
help: { type: "boolean", short: "h" }, // F41: -h is documented in usage
version: { type: "boolean" },
dir: { type: "string" },
entry: { type: "string" },
host: { type: "string" },
hostname: { type: "string" },
tls: { type: "boolean" },
url: { type: "string" },
// --- Serve mode ---
prod: { type: "boolean" },
port: { type: "string", short: "p" },
static: { type: "string", short: "s" },
import: { type: "string" },
cert: { type: "string" },
key: { type: "string" },
// --- Fetch mode ---
method: { type: "string", short: "X" },
request: { type: "string" }, // curl compatibility
header: { type: "string", multiple: true, short: "H" },
verbose: { type: "boolean", short: "v" },
data: { type: "string", short: "d" },
} as const;

if (mode === "serve") {
// Serve mode
const { values, positionals } = parseNodeArgs({
args,
allowPositionals: true,
options: {
...commonArgs,
url: { type: "string" },
prod: { type: "boolean" },
port: { type: "string", short: "p" },
static: { type: "string", short: "s" },
import: { type: "string" },
cert: { type: "string" },
key: { type: "string" },
},
});
if (positionals[0] === "serve") {
positionals.shift();
}
const { values, positionals } = parseNodeArgs({ args, allowPositionals: true, options });

// Backward compatibility: allow entry or dir as positional argument
const maybeEntryOrDir = positionals[0];
if (maybeEntryOrDir) {
if (values.entry || values.dir) {
throw new Error(
"Cannot specify entry or dir as positional argument when --entry or --dir is used!",
);
}
const stat = statSync(maybeEntryOrDir);
if (stat.isDirectory()) {
values.dir = maybeEntryOrDir;
} else {
values.entry = maybeEntryOrDir;
}
}
// Detect mode from the first real positional (the subcommand), then drop it.
let mode: "serve" | "fetch" = "serve";
const sub = positionals[0];
if (sub === "fetch" || sub === "curl") {
mode = "fetch";
positionals.shift();
} else if (sub === "serve") {
positionals.shift();
}

return { mode, ...values };
if (mode === "fetch") {
const method = values.method || values.request;
const url = values.url || positionals[0] || "/";
return { mode, ...values, url, method };
}

// Fetch mode
const { values, positionals } = parseNodeArgs({
args,
allowPositionals: true,
options: {
...commonArgs,
url: { type: "string" },
method: { type: "string", short: "X" },
request: { type: "string" }, // curl compatibility
header: { type: "string", multiple: true, short: "H" },
verbose: { type: "boolean", short: "v" },
data: { type: "string", short: "d" },
},
});
if (positionals[0] === "fetch" || positionals[0] === "curl") {
positionals.shift();
// Serve mode: allow entry or dir as a positional argument
const maybeEntryOrDir = positionals[0];
if (maybeEntryOrDir) {
if (values.entry || values.dir) {
throw new Error("Cannot use a positional path together with --entry or --dir.");
}
// F44: turn a raw statSync ENOENT into a friendly message
if (!existsSync(maybeEntryOrDir)) {
throw new Error(`No such file or directory: ${maybeEntryOrDir}`);
}
if (statSync(maybeEntryOrDir).isDirectory()) {
values.dir = maybeEntryOrDir;
} else {
values.entry = maybeEntryOrDir;
}
}
const method = values.method || values.request;
const url = values.url || positionals[0] || "/";
return { mode, ...values, url, method };

return { mode, ...values };
}

async function startServer(cliOpts: CLIOptions) {
Expand Down Expand Up @@ -199,7 +206,9 @@ async function forkCLI(args: string[], runtimeArgs: string[]) {
};
process.on("exit", () => cleanup("SIGTERM"));
process.on("SIGTERM", () => cleanup("SIGTERM", 143));
if (args.includes("--watch")) {
// F38: watch mode pushes `--watch` into runtimeArgs (not args), so check there;
// otherwise the SIGINT handler never installs and the child is orphaned.
if (runtimeArgs.includes("--watch")) {
process.on("SIGINT" /* ctrl+c */, () => cleanup("SIGINT", 130));
}
}
Expand Down
45 changes: 40 additions & 5 deletions src/cli/serve.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,20 @@ export async function cliServe(cliOpts: CLIOptions): Promise<void> {
const { serveStatic } = await import("srvx/static");
const { log } = await import("srvx/log");

// 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 = "";
}
Comment on lines +35 to +48

@coderabbitai coderabbitai Bot Jul 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.

Suggested change
// 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.


if (loaded.notFound && !cliOpts.static) {
process.send?.({ error: "no-entry" });
Expand All @@ -49,13 +58,23 @@ export async function cliServe(cliOpts: CLIOptions): Promise<void> {
...loaded.module,
} as Partial<ServerOptions>;

// F42: only override the entry module's `tls` when CLI flags actually supply
// TLS. A bare `--tls` (no cert/key) must error rather than silently downgrade.
let tls = serverOptions.tls;
if (cliOpts.tls) {
if (!cliOpts.cert || !cliOpts.key) {
throw new Error("--tls requires both --cert and --key.");
}
tls = { cert: cliOpts.cert, key: cliOpts.key };
}

printInfo(cliOpts, loaded);
server = srvxServe({
...serverOptions,
gracefulShutdown: !!cliOpts.prod,
port: cliOpts.port ?? serverOptions.port,
hostname: cliOpts.hostname ?? cliOpts.host ?? serverOptions.hostname,
tls: cliOpts.tls ? { cert: cliOpts.cert, key: cliOpts.key } : undefined,
tls,
error: (error) => {
console.error(error);
return renderError(cliOpts, error);
Expand Down Expand Up @@ -91,9 +110,10 @@ function renderError(
status = 500,
title = "Server Error",
): Response {
let html = `<!DOCTYPE html><html><head><title>${title}</title></head><body>`;
const safeTitle = escapeHtml(title);
let html = `<!DOCTYPE html><html><head><title>${safeTitle}</title></head><body>`;
if (cliOpts.prod) {
html += `<h1>${title}</h1><p>Something went wrong while processing your request.</p>`;
html += `<h1>${safeTitle}</h1><p>Something went wrong while processing your request.</p>`;
} else {
html += /* html */ `
<style>
Expand All @@ -103,7 +123,9 @@ function renderError(
code { font-family: monospace; }
#error { display: flex; flex-direction: column; justify-content: center; align-items: center; height: 100vh; }
</style>
<div id="error"><h1>${title}</h1><pre>${error instanceof Error ? error.stack || error.message : String(error)}</pre></div>
<div id="error"><h1>${safeTitle}</h1><pre>${escapeHtml(
error instanceof Error ? error.stack || error.message : String(error),
)}</pre></div>
`;
}

Expand All @@ -113,6 +135,19 @@ function renderError(
});
}

const HTML_ESCAPES: Record<string, string> = {
"&": "&amp;",
"<": "&lt;",
">": "&gt;",
'"': "&quot;",
"'": "&#39;",
};

// F59: escape untrusted text before interpolating into the dev error page HTML.
function escapeHtml(str: string): string {
return str.replace(/[&<>"']/g, (ch) => HTML_ESCAPES[ch]);
}

function printInfo(cliOpts: CLIOptions, loaded: Awaited<ReturnType<typeof loadServerEntry>>) {
let entryInfo: string;
if (loaded.notFound) {
Expand Down
2 changes: 1 addition & 1 deletion src/loader.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ export async function loadServerEntry(opts: LoadOptions): Promise<LoadedServerEn
);
} else if (/"\.(m|c)?tsx"/g.test(message)) {
throw new Error(
`You need a compatible loader for JSX support (Deno, Bun or srvx --register jiti/register)`,
`You need a compatible loader for JSX support (Deno, Bun or srvx --import jiti/register)`,
{ cause: error },
);
}
Expand Down
14 changes: 14 additions & 0 deletions test/_cli-run.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// Test runner: invokes the srvx CLI from source (no dist build required).
// Node strips TypeScript types on the fly (Node >= 22.18 / 24).
import { main } from "../src/cli.ts";

// Point any forked child at this same source runner instead of ./bin/srvx.mjs.
(globalThis as any).__SRVX_BIN__ = new URL("./_cli-run.ts", import.meta.url).href;

await main({
usage: {
command: "srvx",
docs: "https://srvx.h3.dev",
issues: "https://github.com/h3js/srvx/issues",
},
});
Loading
Loading