Skip to content

Commit a5ce3c4

Browse files
ralyodioclaude
andauthored
fix(site): actually run the plan's commands, instead of skipping them silently (#250)
`serveCommand` defaulted `runner` to null, and the execution loop was: else if (step.kind === "run" && runner) { ... } The CLI calls `serveCommand(rest)` with no deps, so every `run` step was skipped — in silence — and the command then printed "is live on this machine". `nginx -t` never validated anything and `systemctl reload` never reloaded, so the file just written was not the config being served. That stays invisible until somebody reloads nginx for an unrelated reason, hours or a reboot later, at which point the failure has no obvious cause. --tls turned it from misleading into broken: the openssl step is what creates the key, so the config was written referencing a certificate that never existed. nginx then refuses to load the whole config — every site on the box, not just the new one: [emerg] cannot load certificate "/etc/ssl/moshpit/2600.crt" Three fixes: - a real default runner, so the plan runs - a missing runner is a failure, not a skip: nothing takes effect, and saying otherwise is the bug above - a failed command prints the first line of its stderr, which is the whole diagnosis — "nginx -t failed" without it costs an hour Also fixes the display printing `publish-pin undefined`: that step has no `path`, so it shows the ending instead. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent 48796b4 commit a5ce3c4

2 files changed

Lines changed: 82 additions & 3 deletions

File tree

‎src/serve.mjs‎

Lines changed: 33 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -351,8 +351,27 @@ server already on this machine; nginx or Caddy does the serving.
351351
The name still has to point at this machine — set that in the Pit, and check
352352
it with \`moshcode dns resolve <name>\`. This only makes the box answer to it.`;
353353

354+
/**
355+
* Run one of the plan's commands.
356+
*
357+
* A real default, because the alternative was worse than it looked: with no
358+
* runner the execution loop skipped every `run` step in silence and then
359+
* printed "is live on this machine". `nginx -t` never validated, `systemctl
360+
* reload` never reloaded, and the config that had just been written was not
361+
* the config being served — a lie that is invisible until someone reloads
362+
* nginx for an unrelated reason, hours or reboots later.
363+
*/
364+
async function defaultRunner(command, args) {
365+
const { execFile } = await import("node:child_process");
366+
return new Promise((resolve) => {
367+
execFile(command, args, { timeout: 60_000 }, (error, _stdout, stderr) => {
368+
resolve({ ok: !error, code: error?.code ?? 0, stderr: String(stderr || "").trim() });
369+
});
370+
});
371+
}
372+
354373
export async function serveCommand(args = [], out = console.log, deps = {}) {
355-
const { detect = detectServer, write = fs.writeFile, mkdir = fs.mkdir, copy = fs.cp, runner = null } = deps;
374+
const { detect = detectServer, write = fs.writeFile, mkdir = fs.mkdir, copy = fs.cp, runner = defaultRunner } = deps;
356375
const [name, ...rest] = args;
357376

358377
if (!name || name === "help" || name === "--help" || name === "-h") {
@@ -406,7 +425,11 @@ export async function serveCommand(args = [], out = console.log, deps = {}) {
406425
out(`${name} → ${plan.proxy ? `127.0.0.1:${plan.proxy}` : plan.root} (${plan.server}, ${ports})`);
407426
out("");
408427
for (const step of plan.steps) {
409-
const what = step.kind === "run" ? `${step.command} ${step.args.join(" ")}` : step.path;
428+
const what = step.kind === "run"
429+
? `${step.command} ${step.args.join(" ")}`
430+
: step.kind === "publish-pin"
431+
? `.${step.tld}`
432+
: step.path;
410433
out(` ${step.kind.padEnd(6)} ${what}`);
411434
out(` ${step.why}`);
412435
}
@@ -424,10 +447,17 @@ export async function serveCommand(args = [], out = console.log, deps = {}) {
424447
if (step.kind === "write") await write(step.path, step.content);
425448
else if (step.kind === "mkdir") await mkdir(step.path, { recursive: true });
426449
else if (step.kind === "seed") await copy(step.from, step.path, { recursive: true, force: false });
427-
else if (step.kind === "run" && runner) {
450+
else if (step.kind === "run") {
451+
if (!runner) {
452+
// Silence here is how a config gets written referencing a key that
453+
// was never generated, after reporting success.
454+
out(`! cannot run ${step.command} — nothing would validate or take effect`);
455+
return 1;
456+
}
428457
const result = await runner(step.command, step.args);
429458
if (!result?.ok) {
430459
out(`! ${step.command} failed — stopping before anything else changes`);
460+
if (result?.stderr) out(` ${result.stderr.split("\n")[0]}`);
431461
return 1;
432462
}
433463
} else if (step.kind === "publish-pin") {

‎test/serve.test.mjs‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -243,6 +243,8 @@ test("a bad --template stops the install before anything is written", async () =
243243
write: async () => {},
244244
mkdir: async () => {},
245245
copy: async (from) => { seeded = from; },
246+
// `nginx -t` really runs now, and there is no nginx to run it against here.
247+
runner: async () => ({ ok: true }),
246248
});
247249
assert.equal(code, 0);
248250
assert.match(String(seeded), /examples\/templates\/caddy-static\/site$/);
@@ -331,3 +333,50 @@ test("a second name under the same ending reuses the key and its pin", async ()
331333
assert.equal(tls.create, null, "no new key when the ending already has one");
332334
assert.equal(tls.pin, pinFromCertificate(cert));
333335
});
336+
337+
test("a run step that cannot execute fails loudly instead of being skipped", async () => {
338+
// This was silent, and it is how a config gets written referencing a key
339+
// that was never generated — then reported as live. `nginx -t` never
340+
// validated and `systemctl reload` never reloaded, so the file on disk was
341+
// not the config being served, which stays invisible until somebody
342+
// reloads nginx for an unrelated reason hours later.
343+
const lines = [];
344+
const code = await serveCommand(["blue.eggs", "--install"], (l) => lines.push(l), {
345+
detect: async () => "nginx",
346+
write: async () => {},
347+
mkdir: async () => {},
348+
copy: async () => {},
349+
runner: null,
350+
});
351+
352+
assert.equal(code, 1, "no runner means nothing would take effect — that is a failure, not a success");
353+
assert.match(lines.join("\n"), /cannot run/);
354+
});
355+
356+
test("a failing command stops the install and says which one", async () => {
357+
const lines = [];
358+
const code = await serveCommand(["blue.eggs", "--install"], (l) => lines.push(l), {
359+
detect: async () => "nginx",
360+
write: async () => {},
361+
mkdir: async () => {},
362+
copy: async () => {},
363+
runner: async () => ({ ok: false, code: 1, stderr: "nginx: [emerg] cannot load certificate" }),
364+
});
365+
366+
assert.equal(code, 1);
367+
// The reason nginx gave, not just that something failed — that line is the
368+
// whole diagnosis and hiding it costs an hour.
369+
assert.match(lines.join("\n"), /cannot load certificate/);
370+
});
371+
372+
test("the pin step prints the ending rather than undefined", async () => {
373+
const plan = servePlan({
374+
name: "demo.hacker",
375+
server: "nginx",
376+
root: "/srv/demo.hacker",
377+
tls: { tld: "hacker", dir: "/etc/ssl/moshpit", key: "/k", cert: "/c", pin: "x", create: null },
378+
});
379+
const step = plan.steps.find((s) => s.kind === "publish-pin");
380+
assert.equal(step.path, undefined, "it has no path, which is what printed `undefined`");
381+
assert.equal(step.tld, "hacker", "so the display has to use this instead");
382+
});

0 commit comments

Comments
 (0)