mirror of
https://github.com/mvschwarz/openrig.git
synced 2026-11-07 05:56:21 -10:00
fix(cli): finish a default rig export beside an existing rig.yaml (#2356)
`rig export <rig>` with no -o no longer replaces an existing rig.yaml. It claims the next unused sibling (rig-export.yaml, then rig-export-2.yaml and so on) by creating it exclusively, writes the export there, and prints the path only after the write succeeds. An explicit -o keeps its replacement behaviour, including -o rig.yaml, and filesystem or export failures still exit nonzero. Command +39/-5, tests +79/-1.
This commit is contained in:
1 parent
b78894736d
commit
978e95fbff
4 files changed
+118
-6
No files matched your search
@@ -62,3 +62,16 @@ export function writeTextAtomically(path: string, content: string, label = "file
|
||||
if (owned) try { unlinkSync(temporary); } catch { /* Renamed or already removed. */ }
|
||||
}
|
||||
}
|
||||
|
||||
/** Create a file, failing with EEXIST rather than replacing anything already at the path, even a dangling link. */
|
||||
export function writeNewText(path: string, content: string): void {
|
||||
const fd = openSync(path, "wx", 0o666);
|
||||
try {
|
||||
writeFileSync(fd, content, "utf-8");
|
||||
} catch (error) {
|
||||
try { unlinkSync(path); } catch { /* Report the write failure. */ }
|
||||
throw error;
|
||||
} finally {
|
||||
closeSync(fd);
|
||||
}
|
||||
}
|
||||
@@ -1,7 +1,7 @@
|
||||
import nodePath from "node:path";
|
||||
import fs from "node:fs";
|
||||
import { Command } from "commander";
|
||||
import { writeTextAtomically } from "../atomic-text-write.js";
|
||||
import { writeNewText, writeTextAtomically } from "../atomic-text-write.js";
|
||||
import { DaemonClient } from "../client.js";
|
||||
import { getDaemonStatus, getDaemonUrl , daemonStatusGuard} from "../daemon-lifecycle.js";
|
||||
import { realDeps } from "./daemon.js";
|
||||
@@ -10,6 +10,8 @@ import { rigIdForHandle } from "../rig-handle.js";
|
||||
|
||||
export interface ExportDeps extends StatusDeps {
|
||||
writeFile: (path: string, content: string) => void;
|
||||
/** Must fail with EEXIST when the path is taken. */
|
||||
writeNewFile?: (path: string, content: string) => void;
|
||||
exists?: (path: string) => boolean;
|
||||
}
|
||||
|
||||
@@ -37,18 +39,35 @@ function copyCulture(file: string, root: string, outDir: string): void {
|
||||
console.log(`Copied culture to ${target}`);
|
||||
}
|
||||
|
||||
/** rig.yaml is also the name of an authored spec, and an export doesn't carry all of one (comments, a summary, startup
|
||||
* it couldn't read back). So a default export takes the first free name, created so that it can't replace a file that
|
||||
* appeared after the daemon was asked. Only a path the person named with -o may replace a file. */
|
||||
function writeDefaultExport(write: (path: string, content: string) => void, first: string, content: string): string {
|
||||
for (let attempt = 1; ; attempt++) {
|
||||
const path = attempt === 1 ? first : attempt === 2 ? "rig-export.yaml" : `rig-export-${attempt - 1}.yaml`;
|
||||
try {
|
||||
write(path, content);
|
||||
if (attempt > 1) console.warn(`${first} already exists and was left unchanged. Use -o ${first} to replace it.`);
|
||||
return path;
|
||||
} catch (error) {
|
||||
if ((error as NodeJS.ErrnoException).code !== "EEXIST") throw error;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
export function exportCommand(depsOverride?: ExportDeps): Command {
|
||||
const cmd = new Command("export").description("Export a rig spec as YAML");
|
||||
const getDeps = (): ExportDeps => depsOverride ?? {
|
||||
lifecycleDeps: realDeps(),
|
||||
clientFactory: (url: string) => new DaemonClient(url),
|
||||
writeFile: (p, content) => writeTextAtomically(p, content, "export"),
|
||||
writeNewFile: writeNewText,
|
||||
exists: (p) => fs.existsSync(p),
|
||||
};
|
||||
|
||||
cmd
|
||||
.argument("<rig>", "Rig name or ID to export")
|
||||
.option("-o, --output <path>", "Output file path", "rig.yaml")
|
||||
.option("-o, --output <path>", "Output file path, replacing any file there. Without it, an existing rig.yaml is kept and the export goes to an unused rig-export*.yaml", "rig.yaml")
|
||||
.action(async (rigId: string, opts: { output: string }) => {
|
||||
const deps = getDeps();
|
||||
const status = await getDaemonStatus(deps.lifecycleDeps);
|
||||
@@ -67,8 +86,10 @@ export function exportCommand(depsOverride?: ExportDeps): Command {
|
||||
console.error(`Export failed: ${res.data}`);
|
||||
process.exitCode = 1;
|
||||
} else {
|
||||
deps.writeFile(opts.output, res.data);
|
||||
console.log(`Exported to ${opts.output}`);
|
||||
const written = cmd.getOptionValueSource("output") === "default"
|
||||
? writeDefaultExport(deps.writeNewFile ?? deps.writeFile, opts.output, res.data)
|
||||
: (deps.writeFile(opts.output, res.data), opts.output);
|
||||
console.log(`Exported to ${written}`);
|
||||
for (const line of String(res.data).split("\n")) {
|
||||
if (line.startsWith("# Not carried: ")) console.log(line.slice(2));
|
||||
if (line.startsWith("# Member startup not carried for ")) console.warn(`Warning: ${line.slice(2)}`);
|
||||
@@ -92,7 +113,7 @@ export function exportCommand(depsOverride?: ExportDeps): Command {
|
||||
const parsed = parse(res.data) as Record<string, unknown> | null;
|
||||
if (parsed && typeof parsed === "object") {
|
||||
if (parsed["name"] === "starter") {
|
||||
console.warn("Warning: this export still installs as 'starter'. Before sharing, rename the team's name in rig.yaml; changing the bundle name alone does not rename the installed rig.");
|
||||
console.warn(`Warning: this export still installs as 'starter'. Before sharing, rename the team's name in ${written}; changing the bundle name alone does not rename the installed rig.`);
|
||||
}
|
||||
const pods = parsed["pods"] as Array<{
|
||||
id?: string;
|
||||
|
||||
@@ -5,7 +5,7 @@ import { tmpdir } from "node:os";
|
||||
import { execFileSync, spawn } from "node:child_process";
|
||||
import http from "node:http";
|
||||
import { Command } from "commander";
|
||||
import { writeTextAtomically } from "../src/atomic-text-write.js";
|
||||
import { writeNewText, writeTextAtomically } from "../src/atomic-text-write.js";
|
||||
import { exportCommand } from "../src/commands/export.js";
|
||||
import { STATE_FILE } from "../src/daemon-lifecycle.js";
|
||||
import { allowFetchTarget, resetFetchAllowlist } from "./fetch-guard.js";
|
||||
@@ -35,6 +35,23 @@ it("preserves an existing export and removes staging after a partial failed writ
|
||||
expect(fs.readFileSync(target, "utf8")).toBe("name: original\n");
|
||||
expect(fs.readdirSync(dir)).toEqual(["rig.yaml"]);
|
||||
});
|
||||
it("writeNewText creates a file but never replaces one, even a dangling link", () => {
|
||||
dir = fs.mkdtempSync(join(tmpdir(), "export-atomic-"));
|
||||
const target = join(dir, "rig.yaml"), link = join(dir, "link.yaml");
|
||||
writeNewText(target, "name: first\n");
|
||||
expect(fs.readFileSync(target, "utf8")).toBe("name: first\n");
|
||||
expect(() => writeNewText(target, "name: second\n")).toThrow(expect.objectContaining({ code: "EEXIST" }));
|
||||
expect(fs.readFileSync(target, "utf8")).toBe("name: first\n");
|
||||
fs.symlinkSync(join(dir, "missing.yaml"), link);
|
||||
expect(() => writeNewText(link, "name: second\n")).toThrow(expect.objectContaining({ code: "EEXIST" }));
|
||||
expect(fs.existsSync(join(dir, "missing.yaml"))).toBe(false);
|
||||
});
|
||||
it("writeNewText leaves nothing behind after a failed write", () => {
|
||||
dir = fs.mkdtempSync(join(tmpdir(), "export-atomic-"));
|
||||
failure.active = true;
|
||||
expect(() => writeNewText(join(dir, "rig.yaml"), "name: new\n")).toThrow(/no space left/);
|
||||
expect(fs.readdirSync(dir)).toEqual([]);
|
||||
});
|
||||
it("retains export symlinks and target permissions on successful replacement", () => {
|
||||
dir = fs.mkdtempSync(join(tmpdir(), "export-atomic-"));
|
||||
const target = join(dir, "saved.yaml"), link = join(dir, "rig.yaml");
|
||||
|
||||
@@ -262,6 +262,67 @@ describe("rig export + import", () => {
|
||||
expect(writeFile).toHaveBeenCalledWith("rig.yaml", expect.any(String));
|
||||
});
|
||||
|
||||
describe("export: a default export never replaces a file", () => {
|
||||
const occupied = (...taken: string[]) => vi.fn((p: string) => {
|
||||
if (taken.includes(p)) throw Object.assign(new Error(`EEXIST: ${p}`), { code: "EEXIST" });
|
||||
});
|
||||
const run = async (deps: Partial<ExportDeps>, ...args: string[]) => {
|
||||
const program = new Command();
|
||||
program.addCommand(exportCommand(exportDeps(deps)));
|
||||
return captureLogs(() => program.parseAsync(["node", "rig", "export", "rig-1", ...args]));
|
||||
};
|
||||
|
||||
it("the first export writes rig.yaml", async () => {
|
||||
const writeNewFile = occupied();
|
||||
const logs = await run({ writeNewFile });
|
||||
expect(writeNewFile.mock.calls.map((c) => c[0])).toEqual(["rig.yaml"]);
|
||||
expect(logs).toContain("Exported to rig.yaml");
|
||||
expect(logs.join("\n")).not.toContain("left unchanged");
|
||||
});
|
||||
|
||||
it("an occupied rig.yaml sends the export to a sibling, and says which", async () => {
|
||||
const writeFile = vi.fn();
|
||||
const writeNewFile = occupied("rig.yaml");
|
||||
const exitCode = process.exitCode;
|
||||
const logs = await run({ writeFile, writeNewFile });
|
||||
expect(writeFile).not.toHaveBeenCalled();
|
||||
expect(writeNewFile).toHaveBeenLastCalledWith("rig-export.yaml", expect.any(String));
|
||||
expect(logs).toContain("Exported to rig-export.yaml");
|
||||
expect(logs.join("\n")).toContain("rig.yaml already exists and was left unchanged");
|
||||
expect(process.exitCode).toBe(exitCode);
|
||||
});
|
||||
|
||||
it("occupied siblings are kept too", async () => {
|
||||
const writeNewFile = occupied("rig.yaml", "rig-export.yaml", "rig-export-2.yaml");
|
||||
const logs = await run({ writeNewFile });
|
||||
expect(writeNewFile.mock.calls.map((c) => c[0])).toEqual(["rig.yaml", "rig-export.yaml", "rig-export-2.yaml", "rig-export-3.yaml"]);
|
||||
expect(logs).toContain("Exported to rig-export-3.yaml");
|
||||
});
|
||||
|
||||
it("-o replaces the file it names, including rig.yaml", async () => {
|
||||
const writeFile = vi.fn();
|
||||
const writeNewFile = occupied("rig.yaml");
|
||||
const logs = await run({ writeFile, writeNewFile }, "-o", "rig.yaml");
|
||||
expect(writeFile).toHaveBeenCalledWith("rig.yaml", expect.any(String));
|
||||
expect(writeNewFile).not.toHaveBeenCalled();
|
||||
expect(logs).toContain("Exported to rig.yaml");
|
||||
});
|
||||
|
||||
it("a real write failure stays a failure, with no success reported", async () => {
|
||||
const writeNewFile = vi.fn(() => { throw Object.assign(new Error("no space"), { code: "ENOSPC" }); });
|
||||
const log = vi.spyOn(console, "log").mockImplementation(() => {});
|
||||
try {
|
||||
const program = new Command();
|
||||
program.addCommand(exportCommand(exportDeps({ writeNewFile })));
|
||||
await expect(program.parseAsync(["node", "rig", "export", "rig-1"])).rejects.toThrow("no space");
|
||||
expect(writeNewFile).toHaveBeenCalledTimes(1);
|
||||
expect(log).not.toHaveBeenCalledWith(expect.stringContaining("Exported to"));
|
||||
} finally {
|
||||
log.mockRestore();
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
it("export: warns when culture_file or startup files do not resolve relative to output directory", async () => {
|
||||
const writeFile = vi.fn();
|
||||
const exists = vi.fn((p: string) => !p.includes("missing-culture.md") && !p.includes("missing-start.md") && !p.includes("optional-start.md"));
|
||||
|
||||
Reference in new issue
Block a user