Skip to content

Commit d5a9e5d

Browse files
committed
fix(snapshot): never write the staging file through a leftover link, and pin the mode subcommands
A stale symlink at <path>.tmp (out/s.yml.tmp -> s.yml) was written through and then installed as the destination, so the run printed "snapshot written" while the destination became a self-pointing link and both the old and the new content were gone. The staging path is unlinked before the write (a directory there still fails loudly) and created exclusively, and a test runs the link case. The Windows note on the helper now says what Node's rename is there, a single replace call, rather than claiming atomicity. Beside it: the staging tests assert the full outputs record and lines array like the rest of the file; the CLI's mode subcommands gain the lockstep type that fails when a Mode has no subcommand; the snapshot fleet-input set in the CLI test spreads FILTER_INPUTS instead of hand-listing them; and the README's snapshot pointer moves from step 3's check-mode bullet to step 2's settings-file line, where starting from a snapshot belongs.
1 parent a5d6237 commit d5a9e5d

5 files changed

Lines changed: 54 additions & 20 deletions

File tree

‎README.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ Apply declarative repository settings from `.github/settings.yml`: a loud, state
66

77
1. Create a [fine-grained PAT](https://docs.github.com/en/authentication/keeping-your-account-and-data-secure/managing-your-personal-access-tokens#creating-a-fine-grained-personal-access-token) from the [pre-filled token form][pat-form] and save it as the `ADMIN_TOKEN` repository secret. The form starts with every repository permission the action can need (an organization owner adds Members: read by hand); the default `GITHUB_TOKEN` can never hold them.
88

9-
2. Add `.github/settings.yml`. The first line gives editor autocomplete and hover docs:
9+
2. Add `.github/settings.yml` (or start from a [snapshot](docs/operate/snapshot.md) of the live settings). The first line gives editor autocomplete and hover docs:
1010

1111
```yaml
1212
# yaml-language-server: $schema=https://raw.githubusercontent.com/Vivswan/github-settings-as-code/v2/lib/settings.schema.json # x-release-please-major
@@ -23,7 +23,7 @@ Apply declarative repository settings from `.github/settings.yml`: a loud, state
2323
3. Add the workflow and run it from the Actions tab.
2424
- Keep `mode: check` for the first run: the drift report lists everything an apply would change or delete, and nothing is written.
2525
- Read the report. An apply deletes undeclared labels, autolinks, collaborators, Actions variables, and Copilot agents variables.
26-
- Drop the `mode: check` line once the report says what you expect. The [getting started guide](docs/start/getting-started.md) explains the drift output; [Snapshot mode](docs/operate/snapshot.md) writes an existing repository's live settings as the file to start from.
26+
- Drop the `mode: check` line once the report says what you expect. The [getting started guide](docs/start/getting-started.md) explains the drift output.
2727

2828
```yaml
2929
# .github/workflows/settings.yml

‎src/cli/program.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import {
1313
INPUT_DECLS,
1414
type Io,
1515
type Mode,
16+
type MustBeNever,
1617
parseConfig,
1718
type RunConfig,
1819
} from "../index.js";
@@ -58,6 +59,9 @@ const MODE_COMMANDS = {
5859
snapshot: "snapshot",
5960
} as const satisfies Partial<Record<CliCommand, Mode>>;
6061

62+
/** Compile-time lockstep: a Mode without a subcommand fails here. */
63+
type _UnlistedMode = MustBeNever<Exclude<Mode, (typeof MODE_COMMANDS)[keyof typeof MODE_COMMANDS]>>;
64+
6165
interface Globals {
6266
readonly token?: string;
6367
readonly json?: boolean;

‎src/flows/snapshot.ts‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -256,14 +256,15 @@ async function snapshotTarget(ctx: {
256256
* Write `text` to `path` through a sibling staging file renamed into place, so
257257
* a write that fails partway (disk full, an interrupted run) leaves the
258258
* previous snapshot at `path` intact instead of a truncated one; the rename is
259-
* atomic for a regular file on POSIX and Windows alike. The staging file is
260-
* removed when the write fails.
259+
* atomic on POSIX and a single replace call on Windows. A leftover staging
260+
* file or link is unlinked first, never written through.
261261
*/
262262
function writeReplacing(path: string, text: string): void {
263263
mkdirSync(dirname(path), { recursive: true });
264264
const staging = `${path}.tmp`;
265265
try {
266-
writeFileSync(staging, text);
266+
rmSync(staging, { force: true });
267+
writeFileSync(staging, text, { flag: "wx" });
267268
renameSync(staging, path);
268269
} catch (error) {
269270
// The write's error is the one reported: a directory at the staging path fails both the write and this rm.

‎test/cli/inputs.test.ts‎

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import { buildProgram, CLI_COMMANDS, main } from "../../src/cli/program.js";
2323
import {
2424
type ConfigEnv,
2525
describeProblem,
26+
FILTER_INPUTS,
2627
INPUT_DECLS,
2728
type InputDecl,
2829
type InputName,
@@ -424,12 +425,7 @@ const SNAPSHOT_FLEET_INPUTS = new Set<InputName>([
424425
"snapshot-dir",
425426
"repos",
426427
"repos-dir",
427-
"visibility",
428-
"archived",
429-
"forks",
430-
"exclude",
431-
"topics",
432-
"affiliation",
428+
...FILTER_INPUTS,
433429
]);
434430

435431
/** The inputs a flag needs beside it under `mode`, on top of that mode's base. */

‎test/flows/snapshot.test.ts‎

Lines changed: 42 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,16 @@
88
*/
99

1010
import { afterEach, beforeEach, describe, expect, test } from "bun:test";
11-
import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs";
11+
import {
12+
existsSync,
13+
lstatSync,
14+
mkdirSync,
15+
mkdtempSync,
16+
readFileSync,
17+
rmSync,
18+
symlinkSync,
19+
writeFileSync,
20+
} from "node:fs";
1221
import { tmpdir } from "node:os";
1322
import { dirname, join } from "node:path";
1423
import { parse as parseYaml } from "yaml";
@@ -244,7 +253,7 @@ function disjointRefusal(snapshotDir: string, reposDir: string): string {
244253
describe("runSnapshot writes through a staging file", () => {
245254
const unwritable = (path: string, input: "snapshot-file" | "snapshot-dir", os: string) => ({
246255
level: "error" as const,
247-
line: `cannot write the snapshot to ${path}: Error: ${os}. Check that the "${input}" input names a writable path`,
256+
line: `cannot write the snapshot to ${path}: ${os}. Check that the "${input}" input names a writable path`,
248257
});
249258

250259
test("a staging write that fails leaves the previous snapshot intact and reports the write's error", async () => {
@@ -253,25 +262,45 @@ describe("runSnapshot writes through a staging file", () => {
253262
const staging = `${cfg.snapshotFile}.tmp`;
254263
mkdirSync(dirname(cfg.snapshotFile), { recursive: true });
255264
writeFileSync(cfg.snapshotFile, "labels: []\n");
256-
// A directory at the staging path fails the write before the rename; the run must not remove it either.
265+
// A directory at the staging path fails the run before the rename; the run must not remove it either.
257266
mkdirSync(staging);
258267
const collected = collectingIo();
259268
expect(await run(api, cfg, collected.io)).toBe(1);
260-
expect(collected.outputs.result).toBe("failed");
261-
expect(collected.lines[0]).toEqual(
269+
expect(collected.outputs).toEqual({ "skipped-sections": "", result: "failed" });
270+
expect(collected.lines).toEqual([
262271
unwritable(
263272
cfg.snapshotFile,
264273
"snapshot-file",
265-
`EISDIR: illegal operation on a directory, open '${staging}'`,
274+
`SystemError [ERR_FS_EISDIR]: Path is a directory: rm returned EISDIR (is a directory) ${staging}`,
266275
),
267-
);
276+
{ line: "result: failed" },
277+
]);
268278
expect(readFileSync(cfg.snapshotFile, "utf8")).toBe("labels: []\n");
269279
rmSync(staging, { recursive: true });
270280
expect(await run(api, cfg, collectingIo().io)).toBe(0);
271281
expect(parseYaml(readFileSync(cfg.snapshotFile, "utf8"))).toEqual(doc(BUG));
272282
expect(existsSync(staging)).toBe(false);
273283
});
274284

285+
test("a leftover link at the staging path is unlinked, never written through: the destination becomes a regular file", async () => {
286+
const api = new MockApi(labelsRoute("o/r", [BUG]));
287+
const cfg = fileCfg();
288+
const staging = `${cfg.snapshotFile}.tmp`;
289+
mkdirSync(dirname(cfg.snapshotFile), { recursive: true });
290+
writeFileSync(cfg.snapshotFile, "labels: []\n");
291+
symlinkSync("snapshot.yml", staging);
292+
const collected = collectingIo();
293+
expect(await run(api, cfg, collected.io)).toBe(0);
294+
expect(collected.outputs).toEqual({ "skipped-sections": "", result: "snapshot" });
295+
expect(collected.lines).toEqual([
296+
{ line: `snapshot written to ${cfg.snapshotFile}` },
297+
{ line: "result: snapshot" },
298+
]);
299+
expect(lstatSync(cfg.snapshotFile).isSymbolicLink()).toBe(false);
300+
expect(parseYaml(readFileSync(cfg.snapshotFile, "utf8"))).toEqual(doc(BUG));
301+
expect(existsSync(staging)).toBe(false);
302+
});
303+
275304
test("a rename that fails removes the staging file, fails only that target, and leaves the destination as it was", async () => {
276305
const api = new MockApi({
277306
"GET /repos/o/a": { data: { private: false } },
@@ -298,9 +327,13 @@ describe("runSnapshot writes through a staging file", () => {
298327
const { level, line } = unwritable(
299328
fileA,
300329
"snapshot-dir",
301-
`EISDIR: illegal operation on a directory, rename '${fileA}.tmp' -> '${fileA}'`,
330+
`Error: EISDIR: illegal operation on a directory, rename '${fileA}.tmp' -> '${fileA}'`,
302331
);
303-
expect(collected.lines[0]).toEqual({ level, line: `o/a: ${line}` });
332+
expect(collected.lines).toEqual([
333+
{ level, line: `o/a: ${line}` },
334+
{ line: `o/b: snapshot written to ${fileB}` },
335+
{ line: "result: failed" },
336+
]);
304337
expect(existsSync(`${fileA}.tmp`)).toBe(false);
305338
expect(readFileSync(join(fileA, "keep"), "utf8")).toBe("authored\n");
306339
expect(parseYaml(readFileSync(fileB, "utf8"))).toEqual(doc(DOCS));

0 commit comments

Comments
 (0)