mirror of
https://github.com/software-mansion/argent.git
synced 2026-09-14 19:27:14 +08:00
a6f1bef632
Fixes #638. ## The report understated it The issue says `argent mcp --help` "does not hang and does not start the server — it simply prints nothing". Reproducing it showed the opposite: ``` $ argent mcp --help < /dev/null ; echo $? 0 # 0 bytes ← what the reporter saw $ argent mcp --help # stdin open (a pipe, an editor, a CI step) # HANGS — I hit a 300s timeout ``` `--help` **starts the MCP stdio server**, which blocks reading JSON-RPC from stdin. The "prints nothing, exit 0" is just the server getting an immediate EOF from a closed stdin. So this is the same "`--help` runs the real command" bug that was fixed for the installers — `mcp` was missed. `startMcpServer` takes no argv at all (`mcp-server.ts:96` — its only parameter is `{ paths }`), so a help flag was *structurally* unreachable; there was nowhere downstream that could have handled it. ## Fix `mcp` joins the existing guarded set rather than growing a second help path. That set's defining property is "the handler never inspects its argv", and `mcp` qualifies more strongly than the installers do — it isn't merely ignoring `rest`, it is never handed it. **It cannot fire on a real launch.** The guard tests `rest`, which is `[]` for every generated editor config (all four shapes end at `mcp`, and `isArgentManagedEntry` requires exactly that), and `[].some(...)` evaluates no predicate. This is pinned in both directions, because a guard that *did* fire here would print usage onto the JSON-RPC channel and break every editor at handshake — the highest-cost regression in this file, and it was untested. The top-level table's `mcp` row now reads the same metadata as its own help, so the two cannot drift. **No rename.** An earlier draft renamed the module and six identifiers (`installer-help` → `command-help`, etc.) on the grounds that its comments had become false. They hadn't quite: fixing `mcp` *and* `tools` makes "every other subcommand parses `--help` itself" true again — I checked all ten siblings. Since #530 and #568 both edit `cli.ts`, a rename would have forced them through churn for no behavioural gain. Prose-only. ## Second commit: `argent tools --help` The issue mentions it in passing; it's a different mechanism and a genuinely dead branch. `argv.filter((a) => !a.startsWith("--"))` dropped `--help` before the subcommand was read, so it fell through to listing all 77 tools — **contacting the tool-server, starting one if none was running**, to do it. Only `-h` ever reached the existing help branch. Scoped to the subcommand position on purpose: a global check would have broken `argent tools describe <name> --help`, which today prints *that tool's* flags. It still does — there is a test. Separate commit so it can be reverted alone. ## Verification Live, against the built CLI: | | before | after | |---|---|---| | `mcp --help`, stdin open | hangs indefinitely | usage, returns immediately | | `mcp --help`, stdin closed | 0 bytes, exit 0 | 849 bytes of help, exit 0 | | `mcp` (real launch) | works | works — verified with an actual `initialize` handshake, server replies with its capabilities | | `mcp --metro-port 8082` | starts server | starts server (guard keys on help, not on "any argv") | | `tools --help` | 77-line catalogue | usage | | `tools describe gesture-tap --help` | tool's flags | tool's flags | | `argent --help` `mcp` row | — | byte-identical | argent 80 passed, argent-cli 284 passed. ## Note on the help text Every claim in it is sourced (stdio/JSON-RPC transport, stdout ownership, the `~/.argent/mcp-calls.log` default and `ARGENT_MCP_LOG` override, auto-spawn vs `ARGENT_TOOLS_URL`/`argent link`). It deliberately does **not** suggest running `argent mcp` by hand to check an install — that is the reported hang — and instead says it will look like it has hung and why. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
271 lines
11 KiB
TypeScript
271 lines
11 KiB
TypeScript
import { afterEach, describe, expect, it, vi } from "vitest";
|
||
import {
|
||
INSTALLER_COMMAND_META,
|
||
INSTALLER_COMMANDS,
|
||
installerHelpRequested,
|
||
isInstallerCommand,
|
||
printInstallerHelp,
|
||
} from "../src/installer-help.js";
|
||
|
||
// Guards the fix for #451: `argent <installer-cmd> --help` must short-circuit
|
||
// to help instead of forwarding `--help` to the side-effecting installer
|
||
// functions (which don't handle it and would run the real command — e.g.
|
||
// `uninstall --help` opening the destructive removal prompt).
|
||
|
||
describe("installerHelpRequested", () => {
|
||
it("is true for every installer subcommand with --help", () => {
|
||
for (const command of INSTALLER_COMMANDS) {
|
||
expect(installerHelpRequested(command, ["--help"])).toBe(true);
|
||
}
|
||
});
|
||
|
||
it("is true for the -h short flag", () => {
|
||
expect(installerHelpRequested("uninstall", ["-h"])).toBe(true);
|
||
expect(installerHelpRequested("remove", ["-h"])).toBe(true);
|
||
});
|
||
|
||
it("is true when the help flag trails other arguments", () => {
|
||
expect(installerHelpRequested("init", ["--scope", "project", "--help"])).toBe(true);
|
||
expect(installerHelpRequested("update", ["--foo", "-h"])).toBe(true);
|
||
});
|
||
|
||
// On destructive commands the match is deliberately lenient: a fat-fingered
|
||
// help request must never fall through to the real uninstall (which, with no
|
||
// `--yes`, opens the "Remove argent configuration…" confirm with Yes
|
||
// pre-selected).
|
||
it("is true for --help=<value> (a plausible fat-finger)", () => {
|
||
expect(installerHelpRequested("uninstall", ["--help=foo"])).toBe(true);
|
||
expect(installerHelpRequested("init", ["--help="])).toBe(true);
|
||
});
|
||
|
||
it("is true for case-insensitive help flags", () => {
|
||
expect(installerHelpRequested("uninstall", ["--HELP"])).toBe(true);
|
||
expect(installerHelpRequested("remove", ["-H"])).toBe(true);
|
||
expect(installerHelpRequested("update", ["--Help=x"])).toBe(true);
|
||
});
|
||
|
||
it("is true for single-dash and smart-dash help flags", () => {
|
||
// `-help` is the Go-style spelling; `—help`/`–help` are what smart-dash
|
||
// editors rewrite `--help` into when it is pasted through them.
|
||
expect(installerHelpRequested("uninstall", ["-help"])).toBe(true);
|
||
expect(installerHelpRequested("uninstall", ["—help"])).toBe(true);
|
||
expect(installerHelpRequested("init", ["–help"])).toBe(true);
|
||
expect(installerHelpRequested("update", ["—help=x"])).toBe(true);
|
||
});
|
||
|
||
it("is true for the bareword `help` as the first argument", () => {
|
||
expect(installerHelpRequested("uninstall", ["help"])).toBe(true);
|
||
expect(installerHelpRequested("init", ["HELP"])).toBe(true);
|
||
});
|
||
|
||
it("is true for a bareword `help` after a non-value flag (the --yes footgun)", () => {
|
||
// `uninstall --yes help`: `--yes` takes no value, so `help` is a help
|
||
// request. Before this was honoured, the argv fell through to the REAL
|
||
// uninstall with `--yes` set — which skips the confirmation prompt and
|
||
// prunes the workspace config with no help and no prompt.
|
||
expect(installerHelpRequested("uninstall", ["--yes", "help"])).toBe(true);
|
||
expect(installerHelpRequested("uninstall", ["-y", "help"])).toBe(true);
|
||
expect(installerHelpRequested("update", ["--no-telemetry", "help"])).toBe(true);
|
||
// uninstall parses no value-taking flags at all, so even `--from help`
|
||
// (a flag it does not know) reads as a help request there.
|
||
expect(installerHelpRequested("uninstall", ["--from", "help"])).toBe(true);
|
||
});
|
||
|
||
it("does not treat a bareword `help` after a value-taking flag as help", () => {
|
||
// `--from help` names a package/tarball literally called `help`, and
|
||
// `--version help` / `--project-root help` are those flags' values; none
|
||
// may be swallowed as a help request.
|
||
expect(installerHelpRequested("init", ["--from", "help"])).toBe(false);
|
||
expect(installerHelpRequested("install", ["--from", "help"])).toBe(false);
|
||
expect(installerHelpRequested("update", ["--version", "help"])).toBe(false);
|
||
expect(installerHelpRequested("update", ["--project-root", "help"])).toBe(false);
|
||
});
|
||
|
||
it("is false for a near-miss flag that only starts with --help", () => {
|
||
// `--helpme` is not a help request (no `=`, not exactly `--help`).
|
||
expect(installerHelpRequested("uninstall", ["--helpme"])).toBe(false);
|
||
// `/help` is not a recognised spelling either — it falls through to the
|
||
// command, where the interactive confirmation still guards uninstall.
|
||
expect(installerHelpRequested("uninstall", ["/help"])).toBe(false);
|
||
});
|
||
|
||
it("is false for an installer subcommand without a help flag", () => {
|
||
expect(installerHelpRequested("init", [])).toBe(false);
|
||
expect(installerHelpRequested("uninstall", ["--yes"])).toBe(false);
|
||
expect(installerHelpRequested("update", ["--check"])).toBe(false);
|
||
});
|
||
|
||
it("is false for non-installer commands even with --help (they handle it themselves)", () => {
|
||
// `argent run <tool> --help` must still print the tool's own flags, and
|
||
// likewise for tools/server/etc. — never intercepted here.
|
||
expect(installerHelpRequested("run", ["--help"])).toBe(false);
|
||
expect(installerHelpRequested("run", ["gesture-tap", "--help"])).toBe(false);
|
||
expect(installerHelpRequested("tools", ["--help"])).toBe(false);
|
||
expect(installerHelpRequested("server", ["-h"])).toBe(false);
|
||
// A non-installer command must not be intercepted via the bareword either.
|
||
expect(installerHelpRequested("run", ["help"])).toBe(false);
|
||
});
|
||
|
||
it("is false for an undefined command", () => {
|
||
expect(installerHelpRequested(undefined, ["--help"])).toBe(false);
|
||
});
|
||
});
|
||
|
||
describe("isInstallerCommand", () => {
|
||
it("recognizes exactly the installer command set", () => {
|
||
expect([...INSTALLER_COMMANDS].sort()).toEqual(
|
||
["init", "install", "mcp", "remove", "uninstall", "update"].sort()
|
||
);
|
||
for (const command of INSTALLER_COMMANDS) {
|
||
expect(isInstallerCommand(command)).toBe(true);
|
||
}
|
||
});
|
||
|
||
it("rejects non-installer and undefined commands", () => {
|
||
expect(isInstallerCommand("run")).toBe(false);
|
||
expect(isInstallerCommand(undefined)).toBe(false);
|
||
});
|
||
});
|
||
|
||
describe("printInstallerHelp", () => {
|
||
let logSpy: ReturnType<typeof vi.spyOn>;
|
||
|
||
function render(command: (typeof INSTALLER_COMMANDS)[number]): string {
|
||
logSpy = vi.spyOn(console, "log").mockImplementation(() => {});
|
||
printInstallerHelp(command);
|
||
return logSpy.mock.calls.map((call: unknown[]) => String(call[0])).join("\n");
|
||
}
|
||
|
||
afterEach(() => {
|
||
logSpy?.mockRestore();
|
||
});
|
||
|
||
it("prints the usage line and summary for every installer command", () => {
|
||
for (const command of INSTALLER_COMMANDS) {
|
||
const out = render(command);
|
||
const meta = INSTALLER_COMMAND_META[command];
|
||
expect(out).toContain(`Usage: ${meta.usage}`);
|
||
expect(out).toContain(meta.summary);
|
||
expect(out).toContain("Run `argent --help` for the full list of commands.");
|
||
}
|
||
});
|
||
|
||
it("lists the real flags each command accepts", () => {
|
||
// The point of `--help` is to tell the user which flags exist.
|
||
// (installer-flags-sync.test.ts asserts these lists against the parsers.)
|
||
const init = render("init");
|
||
expect(init).toContain("Options:");
|
||
expect(init).toContain("--yes, -y");
|
||
expect(init).toContain("--no-telemetry");
|
||
expect(init).toContain("--from <path>");
|
||
expect(init).toContain("--global");
|
||
expect(init).toContain("--local");
|
||
|
||
const update = render("update");
|
||
expect(update).toContain("--version <version>");
|
||
expect(update).toContain("--no-telemetry");
|
||
expect(update).toContain("--global");
|
||
expect(update).toContain("--local");
|
||
|
||
const uninstall = render("uninstall");
|
||
expect(uninstall).toContain("Options:");
|
||
expect(uninstall).toContain("--yes, -y");
|
||
expect(uninstall).toContain("--global");
|
||
expect(uninstall).toContain("--local");
|
||
});
|
||
|
||
it("points aliases at their target command instead of duplicating options", () => {
|
||
const install = render("install");
|
||
expect(install).toContain("Run `argent init --help` to see its options.");
|
||
expect(install).not.toContain("Options:");
|
||
|
||
const remove = render("remove");
|
||
expect(remove).toContain("Run `argent uninstall --help` to see its options.");
|
||
expect(remove).not.toContain("Options:");
|
||
});
|
||
|
||
it("only writes to stdout (no wizard / prompt / network)", () => {
|
||
// The whole contract of the help path is that it is side-effect-free. This
|
||
// asserts the function writes to console.log and returns nothing — not how
|
||
// many calls it batches the output into, which is an implementation
|
||
// detail. The end-to-end guarantee (config left untouched) is covered in
|
||
// cli-dispatch.test.ts.
|
||
const spy = vi.spyOn(console, "log").mockImplementation(() => {});
|
||
const result = printInstallerHelp("uninstall");
|
||
expect(result).toBeUndefined();
|
||
expect(spy).toHaveBeenCalled();
|
||
spy.mockRestore();
|
||
});
|
||
});
|
||
|
||
describe("INSTALLER_COMMAND_META", () => {
|
||
it("covers exactly the installer command set", () => {
|
||
expect(Object.keys(INSTALLER_COMMAND_META).sort()).toEqual([...INSTALLER_COMMANDS].sort());
|
||
});
|
||
|
||
it("summaries have no trailing period (they are rendered inline in the top-level table)", () => {
|
||
for (const command of INSTALLER_COMMANDS) {
|
||
expect(INSTALLER_COMMAND_META[command].summary.endsWith(".")).toBe(false);
|
||
}
|
||
});
|
||
|
||
it("aliases carry no own options and non-aliases carry at least one", () => {
|
||
for (const command of INSTALLER_COMMANDS) {
|
||
const meta = INSTALLER_COMMAND_META[command];
|
||
if (meta.aliasOf) {
|
||
expect(meta.options).toHaveLength(0);
|
||
expect(isInstallerCommand(meta.aliasOf)).toBe(true);
|
||
} else {
|
||
expect(meta.options.length).toBeGreaterThan(0);
|
||
}
|
||
}
|
||
});
|
||
});
|
||
|
||
describe("mcp is intercepted like the installers", () => {
|
||
// `argent mcp --help` used to start the stdio server, which then blocked
|
||
// reading JSON-RPC from stdin — the reason this command joined the set.
|
||
it.each([
|
||
["--help"],
|
||
["-h"],
|
||
["-H"],
|
||
["--HELP"],
|
||
["-help"],
|
||
["—help"],
|
||
["–help"],
|
||
["--help=x"],
|
||
["help"],
|
||
["HELP"],
|
||
["--foo", "--help"],
|
||
["--foo", "help"],
|
||
])("treats %j as a help request", (...rest) => {
|
||
expect(installerHelpRequested("mcp", rest)).toBe(true);
|
||
});
|
||
|
||
it("leaves a real server launch alone", () => {
|
||
// Every generated editor config invokes a bare `argent mcp`.
|
||
expect(installerHelpRequested("mcp", [])).toBe(false);
|
||
});
|
||
|
||
it.each([["--helpme"], ["/help"], ["--metro-port", "8082"]])(
|
||
"does not treat %j as a help request",
|
||
(...rest) => {
|
||
expect(installerHelpRequested("mcp", rest)).toBe(false);
|
||
}
|
||
);
|
||
|
||
it("documents itself without inviting the reader to run it blind", () => {
|
||
const logs: string[] = [];
|
||
const spy = vi.spyOn(console, "log").mockImplementation((m) => void logs.push(String(m)));
|
||
printInstallerHelp("mcp");
|
||
spy.mockRestore();
|
||
const out = logs.join("\n");
|
||
|
||
expect(out).toContain("Usage: argent mcp");
|
||
expect(out).toContain("JSON-RPC");
|
||
// The whole point: say why running it by hand looks like a hang.
|
||
expect(out).toContain("hung");
|
||
expect(out).toContain("--help, -h");
|
||
});
|
||
});
|