diff --git a/docs/playbooks/teams-not-collaborators.md b/docs/playbooks/teams-not-collaborators.md index 5f0ee527..0859a717 100644 --- a/docs/playbooks/teams-not-collaborators.md +++ b/docs/playbooks/teams-not-collaborators.md @@ -18,4 +18,4 @@ teams: permission: pull ``` -An empty `collaborators` list is authoritative: apply removes every direct collaborator (the repository owner is never touched), and team-derived access is unaffected because the section manages direct grants only. In check mode the same file emits one drift line per unauthorized direct grant, which is the access review report. Two caveats complete the picture. Undeclared collaborators are deleted under this file's plain-array declaration (the section's default policy) while undeclared teams are left untouched (see the [Sections table](../reference/sections.md)), so a team you stop declaring keeps its access until removed by hand. And pending invitations are reconciled alongside the collaborators: an undeclared pending invitation is cancelled under the same policy (kept as a note under `_undeclared: keep`), a declared user's pending invitation counts as converged at the declared permission, and only an invitation sent by email - which no username can declare - is left untouched, each surfaced as a note. +An empty `collaborators` list is authoritative: apply removes every direct collaborator (the repository owner is never touched), and team-derived access is unaffected because the section manages direct grants only. In check mode the same file emits one drift line per unauthorized direct grant, which is the access review report. Two caveats complete the picture. Undeclared collaborators are deleted under this file's plain-array declaration (the section's default policy) while undeclared teams holding a direct grant are kept and noted under the section's default (see the [Sections table](../reference/sections.md)): a team you stop declaring keeps its access until removed by hand, or until you set the wrapped `teams: {_undeclared: delete, entries: [...]}` form, which revokes the direct grants the file does not name. And pending invitations are reconciled alongside the collaborators: an undeclared pending invitation is cancelled under the same policy (kept as a note under `_undeclared: keep`), a declared user's pending invitation counts as converged at the declared permission, and only an invitation sent by email - which no username can declare - is left untouched, each surfaced as a note. diff --git a/docs/reference/library.md b/docs/reference/library.md index 1ba973be..ffd3e384 100644 --- a/docs/reference/library.md +++ b/docs/reference/library.md @@ -75,8 +75,8 @@ The examples continue from one another and form one program (the docs tests comp | `OptOutNotice` | type | A `null` that deleted what a lower layer declared: the layer and the path | | `describeOptOut` | function | One notice as the line the action prints | | `readLayerFiles` | function | Read paths into `Layer`s in order; the first unreadable file is the problem | -| `readSettingsFile` | function | Read and parse one YAML file, given its role (`settings-file`, `defaults-file`, `layer`), so the problem's advice fits | -| `SettingsFileRole` | type | The three roles | +| `readSettingsFile` | function | Read and parse one YAML file, given its role (`settings-file`, `defaults-file`, `layer`, `central-file`), so the problem's advice fits | +| `SettingsFileRole` | type | The four roles | | `parseSettingsDoc` | function | Parse YAML text into an unknown document | | `Problem` | type | Every typed failure a call can return, keyed by `code` | | `describeProblem` | function | A `Problem` as the message the action prints | @@ -191,10 +191,12 @@ console.log(snapshot.result, snapshot.yaml ?? "(failed: no document)"); | Name | Kind | Says | |---|---|---| -| `executeRun` | function | The executor the action and the CLI share: a `RunConfig` plus a face's `RunDeps` runs to its exit code | -| `RunDeps` | type | What a face hands in: the `io`, the client factory, the artifact `uploader` only the Actions runner has, and the problem wording | +| `executeRun` | function | The executor the action and the CLI share: a `RunConfig` plus a face's `RunDeps` runs to its `RunEnd` | +| `RunDeps` | type | What a face hands in: the `io`, the client factory, and the artifact `uploader` only the Actions runner has | +| `RunEnd` | type | How the run ended: its `exitCode`, and the fatal `Problem` when it never reached a target | | `RunConfig` | type | The action's parsed inputs: a `SingleConfig`, `MultiConfig`, `MergeConfig`, or `SnapshotConfig`, discriminated by `kind` | -| `parseConfig` | function | Build a `RunConfig` from an input reader and the environment the way the action does | +| `parseConfig` | function | Build a `RunConfig` from an input reader, the environment, and the face's `RunCapabilities`, the way the action does; a face without an artifact upload is refused `private-report: artifact` here | +| `RunCapabilities` | type | What the face can do: `artifactUpload`, whether it hands the run a workflow-artifact uploader | | `InputReader` | type | `(name) => string`: how `parseConfig` reads an input | | `ConfigEnv` | type | The environment `parseConfig` reads | | `runSingle` | function | One repository from a local settings file, in check or apply | diff --git a/docs/upgrading/v2-to-v3.md b/docs/upgrading/v2-to-v3.md index 781a73ae..446992be 100644 --- a/docs/upgrading/v2-to-v3.md +++ b/docs/upgrading/v2-to-v3.md @@ -4,7 +4,7 @@ order: 20 # Upgrading from v2 to v3 -Twenty-one breaks (the ninth is for library consumers, the last for anyone pinning a sha). One is silent (the fallback), so run `mode: check` before the first v3 apply. The changelog entry for 3.0.0 will carry the release-please footers in the [CHANGELOG](https://github.com/Vivswan/github-settings-as-code/blob/main/CHANGELOG.md). +Twenty-three breaks (the ninth and twenty-third are for library consumers, the twenty-first for anyone pinning a sha). The silent ones include the fallback and the renamed `GSAC_RETRY_BASE_MS`, so run `mode: check` before the first v3 apply. The changelog entry for 3.0.0 will carry the release-please footers in the [CHANGELOG](https://github.com/Vivswan/github-settings-as-code/blob/main/CHANGELOG.md). | Break | v2 | v3 | What the old form does now | |---|---|---|---| @@ -24,11 +24,13 @@ Twenty-one breaks (the ninth is for library consumers, the last for anyone pinni | Webhooks manage web hooks only | A service hook was matched and deleted like any other | A service hook or url-less hook is outside the section | It is left alone and noted by snapshot; [section 14](#14-webhooks-manage-web-hooks-only) | | A ruleset without `source_type` is repository-owned | Kept with a note under `_undeclared: delete` | Deleted like any other undeclared repository ruleset | [section 15](#15-a-ruleset-without-source_type-is-repository-owned) | | Underscore keys are directives, never notes | An unknown `_note: ...` at the top level was dropped silently | Only `_layering` and `_undeclared` exist; any other underscore key fails validation, on a wrapper and at the top level alike | Validation fails before any section runs, naming the two directives; [section 16](#16-underscore-keys-are-directives-never-notes) | -| `teams` takes the `_undeclared` knob | A plain array; an undeclared team was never listed or touched | `teams: {_undeclared: keep, entries: [...]}` accepted, default `keep`; `delete` revokes undeclared direct grants | No error. Every run now lists the repository's teams and notes each undeclared one; snapshots write the wrapper form; [section 17](#17-teams-takes-the-_undeclared-knob) | +| `teams` takes the `_undeclared` knob | A plain array; an undeclared team was never listed or touched | `teams: {_undeclared: keep, entries: [...]}` accepted, default `keep`; `delete` revokes undeclared direct grants | No error. Every run now lists the repository's teams and notes each undeclared direct grant (access granted at the organization level is noted only under `delete`); snapshots write the wrapper form; [section 17](#17-teams-takes-the-_undeclared-knob) | | `GSAC_RETRY_BASE_MS` | `RETRY_BASE_MS`, undocumented | `GSAC_RETRY_BASE_MS`, in the inputs reference | No error: an unknown environment variable is ignored, so a harness setting the old name waits real seconds; [section 18](#18-gsac_retry_base_ms) | | The sealing key is read at apply time | Check mode read `GET .../secrets/public-key` and failed on a malformed key | The first sealed PUT reads it at apply | Check mode issues one request fewer per secret family; a malformed key fails at apply; [section 19](#19-the-sealing-key-is-read-at-apply-time) | | Environment secrets and variables plan through the shared engines | Their own wording | The engines' wording | Only a grep over the output notices; [section 20](#20-environment-secrets-and-variables-plan-through-the-shared-engines) | | The `build` branch retires | Every green push appended a packaged commit to the `build` branch; `latest` and the release tags pointed into it | One packaged commit per `main` commit under the tag `build/.`, the ten newest kept; `latest`, `@v3`, and `vX.Y.Z` point at them; the branch is deleted once every consumer has repinned | A sha pin into the branch stops resolving when the branch goes; a pin taken from a build tag goes when ten newer commits have been packaged; [section 21](#21-the-build-branch-retires) says what to pin instead | +| One wording for every face | The command line reworded two remedies and refused `private-report: artifact` on its own; other remedies said "re-run the workflow" | One problem renderer; remedies name the input and its flag; `parseConfig` refuses the artifact channel for a face without an upload (`input-artifact-unsupported`) | No error. A step or script matching an error line by its old text stops matching; [section 22](#22-one-wording-for-every-face) | +| Library: one owner per refusal, one selection type | `parseConfig(read, env)`; `executeRun` returned a number and took `describe`; `artifact-uploader-missing`; `validateSettings` took a `Set` | `parseConfig(read, env, capabilities)`; `executeRun` returns `{exitCode, fatal?}`; `input-artifact-unsupported`; `SectionSelection` everywhere; `writeReplacing`, `snapshotFileDestination`, the `central-file` role | The call fails to compile, naming the missing argument or member; [section 23](#23-library-one-owner-per-refusal-one-selection-type) | ## 1. The defaults-file fallback @@ -300,7 +302,6 @@ Their lines are the engines' lines now: | The secrets cannot-verify note | once per environment that exists | once per environment with declared secrets, the missing environment included, under the `environments[prod].secrets` label | Repository variable operations (`actions_variables`, `agents_variables`) gain a describe line in failure prose (`creating Actions variable "X"`); nothing else moves. - ## 21. The build branch retires Nothing lands on the `build` branch any more, and the owner deletes it once the migration is complete: the first `build/.` tags exist, `latest` points at a tagged packaged commit, and every known consumer of a sha on the branch has repinned to a release tag, `@v3`, or an npm version. Its ruleset is declared disabled in `.github/settings.local.yml` (the apply leaves an undeclared ruleset alone, so dropping the entry would keep the deletion rule live); after `git push origin --delete build` the owner removes that entry. Repin now: a `Vivswan/github-settings-as-code@` pin into the branch stops resolving when it goes. @@ -314,6 +315,34 @@ Every green push to `main` now mints one packaged commit, the main commit's chil | A sha from the `build` branch | Until the owner deletes the branch | Repin to `@v3` or a release tag's commit now | | A sha from a `build/*` tag | Until ten newer commits are packaged (the next merge, for an old tag) | Pin the release tag's commit or an npm version instead | +## 22. One wording for every face + +The action and the command line print the same line for the same problem; the remedy names the input and, where one exists, its flag. + +| Problem | v2 (action) | v2 (command line) | v3 (both) | +|---|---|---|---| +| No token | `Set the "token" input on the action step (or export GITHUB_TOKEN)` | `Pass --token, or export GITHUB_TOKEN` | `Set the "token" input (--token on the command line), or export GITHUB_TOKEN` | +| No repository | `Set the "repository" input (or GITHUB_REPOSITORY) to a value like "octocat/hello-world"` | `Pass --repository owner/name (inside GitHub Actions, GITHUB_REPOSITORY supplies it)` | `Set the "repository" input (--repository on the command line) to a value like "octocat/hello-world"; inside GitHub Actions, GITHUB_REPOSITORY supplies it` | +| Unknown section in `sections` | `Fix the name in the workflow's input list` | same | `Fix the section name` | +| A transient API failure | `... re-run the workflow, and retry later if it persists` | same | `... re-run, and retry later if it persists` (the report channels say `Re-run, or set private-report: none if it persists`) | +| `private-report: artifact` off the runner | accepted at the parse (the action has the upload; a library caller without an uploader failed later with `artifact-uploader-missing`) | `the "private-report" input is "artifact", which is not a supported private-report channel from the command line ...` | `private-report: artifact uploads the reports as a workflow artifact, which only the GitHub Actions runner can do, and this run has no artifact upload ... Set private-report to "issue", "issue-on-failure", or "none"` | +| `gsac init --settings-file a,b` | `the --settings-file value "a,b" contains a comma or a newline, which check and apply read as a list separator ...` | | `the "settings-file" input is "a,b", which contains a list separator: init writes exactly one settings file ... Name one file` | +| An unreadable `repos-dir` file | `cannot read settings from : ... Fix the file, or delete it to stop managing this repository` | | `cannot read the central settings file : ... Fix the file, or delete it to stop managing this repository` (a YAML syntax error in it reads the same way, where v2 said `cannot parse`) | + +## 23. Library: one owner per refusal, one selection type + +For `@vivswan/github-settings-as-code` consumers. + +| v2 | v3 | +|---|---| +| `parseConfig(read, env)`; the command line refused `private-report: artifact` before calling it, and the flows checked for the uploader again (`artifact-uploader-missing`) | `parseConfig(read, env, { artifactUpload })`: the one refusal, `input-artifact-unsupported`; `runSingle` and `runMulti` no longer check | +| `executeRun(cfg, deps): Promise`, with `deps.describe` rewording a fatal problem | `executeRun(cfg, deps): Promise`, `{ exitCode, fatal? }`; `failRun(io, problem)` takes no wording hook | +| `validateSettingsDoc(doc, source, onlySections: ReadonlySet, io)`; `validateSettings(doc, { sections?: ReadonlySet })` | Both take a `SectionSelection` (`SectionSelection.ALL` for no allowlist) | +| `parseSnapshotFileConfig(read, env, snapshotFile)` | `parseSnapshotFileConfig(read, env, "settings-file")` reads the destination itself; `snapshotFileDestination(read, "settings-file")` names it before parsing | +| Every flow wrote its file in place with its own try/catch | `writeReplacing(path, text): Result` stages and renames, for the snapshot, the merged file, and init alike | +| `SettingsFileRole`: `settings-file`, `defaults-file`, `layer` | plus `central-file`, the repos-dir file a multi-repo target is read from | + + ## Order of operations 1. Rename any settings file whose path contains a comma, rename `undeclared` to `_undeclared` in every settings file, and move every other underscore key into a YAML comment; the v2 line accepts the old spellings only, so do all three together with the pin move. diff --git a/src/action/inputs.ts b/src/action/inputs.ts index aa0f4d98..a6c996e4 100644 --- a/src/action/inputs.ts +++ b/src/action/inputs.ts @@ -4,5 +4,6 @@ import { type Problem, parseConfig, type RunConfig } from "../index.js"; export function parseActionConfig(): Result { // @actions/core reads INPUT_ (uppercased, spaces to underscores, dashes kept: `settings-file` -> INPUT_SETTINGS-FILE) and trims. - return parseConfig((name) => core.getInput(name), process.env); + // The runner has the Actions artifact service, so the artifact report channel is admitted here alone. + return parseConfig((name) => core.getInput(name), process.env, { artifactUpload: true }); } diff --git a/src/action/run.ts b/src/action/run.ts index d99ac513..7ab5749b 100644 --- a/src/action/run.ts +++ b/src/action/run.ts @@ -24,7 +24,7 @@ export async function run(overrides?: { uploader: overrides?.uploader ?? actionsArtifactUploader, }; return parseActionConfig().match( - (cfg) => executeRun(cfg, deps), + async (cfg) => (await executeRun(cfg, deps)).exitCode, async (problem) => failRun(io, problem), ); } diff --git a/src/cli/commands.ts b/src/cli/commands.ts index a0363b80..750c1b58 100644 --- a/src/cli/commands.ts +++ b/src/cli/commands.ts @@ -1,14 +1,13 @@ /** - * What the file-only subcommands and init render for the program to print, - * and the CLI's own wording of a problem; check, apply, merge, and snapshot run - * through the library's executor from the program. + * What the file-only subcommands and init render for the program to print; + * check, apply, merge, and snapshot run through the library's executor from + * the program. */ import { describeProblem, type GitHubClient, type Io, - type Problem, type RunOutcome, readSettingsFile, SECTIONS, @@ -17,7 +16,6 @@ import { type ValidatedSettings, validateSettings, } from "../index.js"; -import { INPUT_DECLS, PRIVATE_REPORT_CHANNELS } from "../internal.js"; /** What the CLI needs from its process: the environment and a client factory tests can stub. */ export interface CliHost { @@ -110,31 +108,3 @@ export function permissionsFor(file: string, io: Io, bold: (text: string) => str }, ); } - -/** - * The action's wording for a problem, except where the remedy names the - * workflow step: from a terminal the fix is a flag or an environment variable. - */ -export function describeCliProblem(problem: Problem): string { - switch (problem.code) { - case "input-token-missing": - return "cannot call the GitHub API: no token was provided. Pass --token, or export GITHUB_TOKEN"; - case "input-repository-not-slug": - return `cannot target a repository: "${problem.value}" is not an owner/name slug. Pass --repository owner/name (inside GitHub Actions, GITHUB_REPOSITORY supplies it)`; - default: - return describeProblem(problem); - } -} - -/** - * The one report channel a terminal cannot serve: the artifact upload needs - * the Actions runner. Worded as the unsupported value it is from here. - */ -export const ARTIFACT_REFUSED: Problem = { - code: "input-unsupported-value", - input: "private-report", - value: "artifact", - noun: "private-report channel from the command line (the artifact upload needs the Actions runner)", - allowed: PRIVATE_REPORT_CHANNELS.filter((channel) => channel !== "artifact"), - fallback: INPUT_DECLS["private-report"].default, -}; diff --git a/src/cli/init.ts b/src/cli/init.ts index 7b38d54d..355792e4 100644 --- a/src/cli/init.ts +++ b/src/cli/init.ts @@ -8,11 +8,11 @@ * Problem. */ -import { existsSync, mkdirSync, writeFileSync } from "node:fs"; -import { dirname } from "node:path"; -import { err, ok, type Result, ResultAsync } from "neverthrow"; +import { existsSync } from "node:fs"; +import { err, type Result, ResultAsync } from "neverthrow"; import { type ConfigEnv, + describeProblem, type InputReader, type Io, type Problem, @@ -21,14 +21,8 @@ import { type SnapshotReport, snapshotRepository, } from "../index.js"; -import { DEFAULT_SETTINGS_FILE, parseSnapshotFileConfig, skippedSectionKeys } from "../internal.js"; -import { - type CliHost, - describeCliProblem, - failedEnvelope, - grantTable, - type Rendered, -} from "./commands.js"; +import { parseSnapshotFileConfig, skippedSectionKeys, writeReplacing } from "../internal.js"; +import { type CliHost, failedEnvelope, grantTable, type Rendered } from "./commands.js"; export interface InitConfig { readonly kind: "init"; @@ -46,7 +40,6 @@ export interface InitConfig { /** The library's problems plus the ones only init raises; describeInitProblem words every one. */ export type InitProblem = | Problem - | { readonly code: "init-settings-file-is-list"; readonly value: string } | { readonly code: "init-settings-file-exists"; readonly settingsFile: string } | { readonly code: "init-settings-file-unwritable"; @@ -66,31 +59,19 @@ export type InitProblem = readonly reasons: ReadonlyArray; }; -/** The separators every mode reads as a list, which one path can therefore never contain. */ -const LIST_SEPARATOR = /[\n,]/; - -/** The file init writes: the one named, else the one apply and check read. Known before any parsing, so a failure names it. */ -export function initSettingsFile(read: InputReader): string { - return read("settings-file") || DEFAULT_SETTINGS_FILE; -} - -/** The init flags are the snapshot inputs of one repository, so every problem is the snapshot subcommand's. */ +/** The init flags are the snapshot inputs of one repository with settings-file as the destination, so every problem is the library's. */ export function parseInitConfig( read: InputReader, force: boolean, env: ConfigEnv, ): Result { - const settingsFile = initSettingsFile(read); - if (LIST_SEPARATOR.test(settingsFile)) { - return err({ code: "init-settings-file-is-list", value: settingsFile }); - } - return parseSnapshotFileConfig(read, env, settingsFile).map( + return parseSnapshotFileConfig(read, env, "settings-file").map( (cfg): InitConfig => ({ kind: "init", token: cfg.token, apiVersion: cfg.apiVersion, repo: cfg.repo, - settingsFile, + settingsFile: cfg.snapshotFile, sections: cfg.sections, onMissingPermission: cfg.onMissingPermission, force, @@ -98,11 +79,9 @@ export function parseInitConfig( ); } -/** The wording for every problem init can end in: its own here, the library's through the CLI's rewording. */ +/** The wording for every problem init can end in: its own here, the library's through the one renderer. */ function describeInitProblem(problem: InitProblem): string { switch (problem.code) { - case "init-settings-file-is-list": - return `the --settings-file value "${problem.value}" contains a comma or a newline, which check and apply read as a list separator. Name one path without them`; case "init-settings-file-exists": return `${problem.settingsFile} already exists: init writes the starting settings file and does not replace the one you author. Pass --force to replace it, or --settings-file to write elsewhere`; case "init-settings-file-unwritable": @@ -117,7 +96,7 @@ function describeInitProblem(problem: InitProblem): string { return `the snapshot of ${problem.repository} declares no section (${why}), so ${problem.settingsFile} was not written. Choose sections init can read back, or drop --sections to read every section`; } default: - return describeCliProblem(problem); + return describeProblem(problem); } } @@ -129,17 +108,13 @@ export function failInit(io: Io, problem: InitProblem, file?: string): Rendered } function writeSettingsFile(cfg: InitConfig, yaml: string): Result { - try { - mkdirSync(dirname(cfg.settingsFile), { recursive: true }); - writeFileSync(cfg.settingsFile, yaml); - return ok(); - } catch (error) { - return err({ + return writeReplacing(cfg.settingsFile, yaml).mapErr( + (reason): InitProblem => ({ code: "init-settings-file-unwritable", settingsFile: cfg.settingsFile, - reason: String(error), - }); - } + reason, + }), + ); } /** The outcome keys in one status, for the lines that name them. */ diff --git a/src/cli/program.ts b/src/cli/program.ts index b87b33e0..0651eaea 100644 --- a/src/cli/program.ts +++ b/src/cli/program.ts @@ -9,25 +9,24 @@ import { Command, CommanderError, Option } from "commander"; import pc from "picocolors"; import { + describeProblem, executeRun, failRun, GitHubApi, type Io, - type Problem, parseConfig, type RunConfig, + type RunEnd, } from "../index.js"; -import { INPUT_DECLS, type Mode, type MustBeNever } from "../internal.js"; +import { INPUT_DECLS, type Mode, type MustBeNever, snapshotFileDestination } from "../internal.js"; import { - ARTIFACT_REFUSED, type CliHost, - describeCliProblem, failedEnvelope, permissionsFor, type Rendered, validateFile, } from "./commands.js"; -import { failInit, type InitConfig, initSettingsFile, parseInitConfig, runInit } from "./init.js"; +import { failInit, type InitConfig, parseInitConfig, runInit } from "./init.js"; import { argvReader, INIT_INPUTS, @@ -88,12 +87,8 @@ export interface ProgramOptions { readonly streams: MaskedStreams; /** Color the level labels and the permissions table; defaults to picocolors' detection. */ readonly colors?: boolean; - /** Run a parsed config, wording a fatal problem with `describe`; tests capture the config here instead of running it. */ - readonly execute?: ( - cfg: RunConfig, - io: Io, - describe: (problem: Problem) => string, - ) => Promise; + /** Run a parsed config to its end; tests capture the config here instead of running it. */ + readonly execute?: (cfg: RunConfig, io: Io) => Promise; /** Run a parsed init config to its rendering; tests capture the config here instead of running it. */ readonly executeInit?: (cfg: InitConfig, io: Io) => Promise; } @@ -106,6 +101,9 @@ export function processHost(): CliHost { }; } +/** A terminal has no Actions artifact service, so the artifact report channel is refused at the parse. */ +const CLI_CAPABILITIES = { artifactUpload: false } as const; + /** * The whole command tree, wired to `options`; the exit code lands in the * returned holder. The environment's token is masked here, before any writer @@ -120,11 +118,10 @@ export function buildProgram(options: ProgramOptions): { const paint = pc.createColors(colors); const execute = options.execute ?? - ((cfg, io, describe) => + ((cfg, io) => executeRun(cfg, { io, createClient: (token, io, apiVersion) => host.createClient(token, io, apiVersion), - describe, })); const executeInit = options.executeInit ?? ((cfg, io) => runInit(cfg, io, host, paint.bold)); const envToken = host.env.GITHUB_TOKEN?.trim(); @@ -186,20 +183,19 @@ export function buildProgram(options: ProgramOptions): { const values = this.optsWithGlobals>(); const { io, flush } = openIo(values); const read = argvReader(mode, values); - // The face words the fatal problem AND keeps the words: the envelope carries them beside the outputs. + // The envelope carries the fatal problem's text beside the outputs; the line itself is already on stderr. let fatal: string | undefined; - const describe = (problem: Problem): string => { - fatal = describeCliProblem(problem); - return fatal; - }; - // Refused before parseConfig, which would otherwise ask for the channel's age key first. - exitCode = - read("private-report") === "artifact" - ? failRun(io, ARTIFACT_REFUSED, describe) - : await parseConfig(read, host.env).match( - (cfg) => execute(cfg, io, describe), - async (problem) => failRun(io, problem, describe), - ); + exitCode = await parseConfig(read, host.env, CLI_CAPABILITIES).match( + async (cfg) => { + const end = await execute(cfg, io); + fatal = end.fatal === undefined ? undefined : describeProblem(end.fatal); + return end.exitCode; + }, + async (problem) => { + fatal = describeProblem(problem); + return failRun(io, problem); + }, + ); flush(fatal); }); } @@ -222,7 +218,7 @@ export function buildProgram(options: ProgramOptions): { const read = argvReader("snapshot", values); const rendered = await parseInitConfig(read, values.force === true, host.env).match( (cfg) => executeInit(cfg, io), - async (problem) => failInit(io, problem, initSettingsFile(read)), + async (problem) => failInit(io, problem, snapshotFileDestination(read, "settings-file")), ); present(rendered, values); exitCode = rendered.code; @@ -293,14 +289,21 @@ export async function main( } return error.exitCode; } - const verbose = program.opts().verbose === true; + const globals = program.opts(); + const verbose = globals.verbose === true; const detail = verbose && error instanceof Error && error.stack ? error.stack : String(error); // Under --verbose the stack is already printed, so asking for it again would loop. const remedy = verbose ? "The stack above is the report: if it recurs, file a bug with it attached" : "Re-run with --verbose for the stack; if it recurs, file a bug with that output attached"; const message = `github-settings-as-code stopped unexpectedly: ${detail}. ${remedy}`; - streams.stderr.write(`error: ${message}\n`); + // The crash line goes through the same renderer as every other error line, so its label and color match. + cliIo({ + streams, + json: globals.json === true, + verbose, + colors: options.colors ?? pc.isColorSupported, + }).io.annotate("error", message); failedJson(message); return 1; } diff --git a/src/engine/orchestrate.ts b/src/engine/orchestrate.ts index 58e8582b..a1dd1bbc 100644 --- a/src/engine/orchestrate.ts +++ b/src/engine/orchestrate.ts @@ -96,7 +96,7 @@ export function skippedSectionKeys( export function validateSettingsDoc( settings: unknown, sourceLabel: string, - onlySections: ReadonlySet, + sections: SectionSelection, io: Io, ): Result { if (typeof settings !== "object" || settings === null || Array.isArray(settings)) { @@ -114,7 +114,7 @@ export function validateSettingsDoc( } const knownSections = new Set(SECTION_KEYS); const directives = new Set(DOCUMENT_DIRECTIVE_KEYS); - const allowed: ReadonlySet = onlySections; + const allowed: ReadonlySet = sections.only; // A misspelled section silently doing nothing would break the loud-failure promise, and so would a misspelled // directive: `_layerin: replace` dropped as a private note would merge a layer the author meant to replace. const strangers = Object.keys(settings).filter( @@ -130,7 +130,7 @@ export function validateSettingsDoc( } const unknownKeys = strangers.filter((key) => !key.startsWith("_")); if (unknownKeys.length > 0) { - if (onlySections.size === 0 || unknownKeys.some((key) => allowed.has(key))) { + if (allowed.size === 0 || unknownKeys.some((key) => allowed.has(key))) { return err({ code: "settings-unknown-sections", source: sourceLabel, diff --git a/src/engine/snapshot.ts b/src/engine/snapshot.ts index 1f101986..15d69cc8 100644 --- a/src/engine/snapshot.ts +++ b/src/engine/snapshot.ts @@ -26,7 +26,7 @@ import { SECTIONS } from "../sections/registry.js"; import type { MustBeNever } from "../types.js"; import { type ValidatedSettings, validateSettingsDoc } from "./orchestrate.js"; import type { RunOutcome } from "./outcome.js"; -import type { SectionSelection } from "./section-selection.js"; +import { SectionSelection } from "./section-selection.js"; export interface SnapshotRunOptions { /** The target repository, parsed at the caller's validated boundary. */ @@ -190,7 +190,7 @@ export async function snapshotRepository( const verdict = validateSettingsDoc( { [section.key]: snapshot.value }, `the ${section.key} snapshot of ${opts.repo.slug}`, - new Set(), + SectionSelection.ALL, io, ); if (verdict.isErr()) { @@ -209,7 +209,12 @@ export async function snapshotRepository( } // The brand's one mint, over the already-parsed fragments: every section validated alone // above, so the whole cannot fail. - const verdict = validateSettingsDoc(document, `the snapshot of ${opts.repo.slug}`, new Set(), io); + const verdict = validateSettingsDoc( + document, + `the snapshot of ${opts.repo.slug}`, + SectionSelection.ALL, + io, + ); if (verdict.isErr()) { throw new Error( `BUG: the assembled snapshot of ${opts.repo.slug} failed validation after every section validated on its own: ${describeProblem(verdict.error)}`, diff --git a/src/flows/deliver.ts b/src/flows/deliver.ts index f6de4cc1..28537198 100644 --- a/src/flows/deliver.ts +++ b/src/flows/deliver.ts @@ -4,7 +4,6 @@ * what they report. */ -import { err, ok, type Result } from "neverthrow"; import type { RepoRef, Target } from "../discovery/targets.js"; import { type RepoRunResult, @@ -18,7 +17,7 @@ import type { GitHubClient } from "../github/api.js"; import type { RepoVisibility } from "../github/repo-visibility.js"; import type { Io } from "../io.js"; import { isPrivate, type Private } from "../private.js"; -import { describeProblem, type Problem, type ProblemOf } from "../problem.js"; +import { describeProblem, type Problem } from "../problem.js"; import type { ArtifactUploader } from "../report/artifact-report.js"; import { type ClosedOutcome, @@ -110,16 +109,6 @@ export interface RunFlowConfig extends DeliveryConfig { privateRepos: PrivateReposPolicy; } -/** Both flows check this before any API work; openReportChannel asserts the same rule as its backstop. */ -export function requireUploader( - cfg: Pick, - uploader: ArtifactUploader | undefined, -): Result> { - return cfg.privateReport === "artifact" && uploader === undefined - ? err({ code: "artifact-uploader-missing" }) - : ok(); -} - export type Exposure = { kind: "shown" } | { kind: "redacted"; visibility: RepoVisibility }; /** One target as it enters delivery; `repo` is null when the slug did not parse (the issue channel has nowhere to post). */ @@ -230,15 +219,11 @@ export function concludeRun(io: Io, run: FinishedRun): number { } /** - * A run that failed before any target ran gets a failed target's conclusion and no summary; the one place a fatal problem becomes text. - * `describe` is the action's wording unless the caller's face (the command line) words a remedy differently. + * A run that failed before any target ran gets a failed target's conclusion and no summary; the one place a fatal + * problem becomes text, in the one wording both faces print. */ -export function failRun( - io: Io, - problem: Problem, - describe: (problem: Problem) => string = describeProblem, -): number { - const message = describe(problem); +export function failRun(io: Io, problem: Problem): number { + const message = describeProblem(problem); io.annotate("error", message); // The mode may be unknown here (a config error); a failure exits 1 under either. return conclude(io, failedTarget(message), false); diff --git a/src/flows/execute.ts b/src/flows/execute.ts index 922c28e2..26e9618b 100644 --- a/src/flows/execute.ts +++ b/src/flows/execute.ts @@ -1,5 +1,5 @@ /** - * One run executor behind the action and the CLI: a parsed RunConfig runs to its exit code, with the outputs and the + * One run executor behind the action and the CLI: a parsed RunConfig runs to its end, with the outputs and the * summary through `deps.io`. The two faces differ only in what they hand in, so the arm dispatch cannot drift. */ @@ -19,30 +19,43 @@ export interface RunDeps { readonly io: Io; /** Opens the client a config's token authorizes; a merge never asks for one. */ readonly createClient: (token: string, io: Io, apiVersion: string) => GitHubClient; - /** The artifact channel's uploader: only the Actions runner has one, and without it that channel fails before any API call. */ + /** The artifact channel's uploader; parseConfig refuses that channel for a face that declares no upload capability. */ readonly uploader?: ArtifactUploader; - /** Words a fatal problem for the face; the action's wording unless the face words a remedy differently. */ - readonly describe?: (problem: Problem) => string; } -export function executeRun(cfg: RunConfig, deps: RunDeps): Promise { +/** + * How a run ended: its exit code, and the problem it ended in when it never reached a target (the outputs and the + * error line are already through `deps.io`; the command line's --json envelope carries the problem's text beside them). + */ +export interface RunEnd { + readonly exitCode: number; + readonly fatal?: Problem; +} + +export function executeRun(cfg: RunConfig, deps: RunDeps): Promise { const { io } = deps; - const fail = (problem: Problem): number => failRun(io, problem, deps.describe); + const fail = (problem: Problem): RunEnd => ({ exitCode: failRun(io, problem), fatal: problem }); + const end = (exitCode: number): RunEnd => ({ exitCode }); if (cfg.kind === "merge") { - return Promise.resolve(runMerge(cfg, io).match((merged) => concludeMerge(io, merged), fail)); + return Promise.resolve( + runMerge(cfg, io).match((merged) => end(concludeMerge(io, merged)), fail), + ); } const api = deps.createClient(cfg.token, io, cfg.apiVersion); switch (cfg.kind) { case "snapshot": - return runSnapshot(api, cfg, io).match((finished) => concludeSnapshot(io, finished), fail); + return runSnapshot(api, cfg, io).match( + (finished) => end(concludeSnapshot(io, finished)), + fail, + ); case "multi": return runMulti(api, cfg, io, deps.uploader).match( - (targets) => concludeRun(io, { kind: "multi", mode: cfg.mode, targets }), + (targets) => end(concludeRun(io, { kind: "multi", mode: cfg.mode, targets })), fail, ); case "single": return runSingle(api, cfg, io, deps.uploader).match( - (target) => concludeRun(io, { kind: "single", mode: cfg.mode, target }), + (target) => end(concludeRun(io, { kind: "single", mode: cfg.mode, target })), fail, ); } diff --git a/src/flows/inputs.ts b/src/flows/inputs.ts index 41d92964..97d43372 100644 --- a/src/flows/inputs.ts +++ b/src/flows/inputs.ts @@ -688,16 +688,29 @@ function parseSnapshotFileArm( } /** - * The file arm for a caller whose destination is fixed: the CLI's init writes - * the settings file, so it reads no snapshot-file input and can never be the - * dir form. + * The file a one-file destination input names, or its declared default: known before any parsing, so a failure can + * name it. The CLI's init reads `settings-file` this way, since it writes the file apply and check read. + */ +export function snapshotFileDestination(read: InputReader, destination: "settings-file"): string { + return inputs(read).orDefault(destination); +} + +/** + * The file arm for a caller whose destination is a one-file input of its own: + * the CLI's init writes the settings file, so it reads `settings-file` as the + * destination (refusing a list separator as apply and check do) and can never + * be the dir form. */ export function parseSnapshotFileConfig( read: InputReader, env: ConfigEnv, - snapshotFile: string, + destination: "settings-file", ): Result { - return parseSnapshotFileArm(inputs(read), env, snapshotFile); + const path = snapshotFileDestination(read, destination); + if (LIST_SEPARATOR.test(path)) { + return err({ code: "input-settings-file-is-list", value: path, mode: "init" }); + } + return parseSnapshotFileArm(inputs(read), env, path); } /** Read and validate the mode: snapshot inputs; the first problem wins. */ @@ -746,8 +759,21 @@ function parseSnapshotConfig( }); } +/** + * What the face running the config can do; parseConfig refuses an input that needs a capability the face lacks, so + * the refusal has one owner and the flows never re-check it. + */ +export interface RunCapabilities { + /** The face hands the run a workflow-artifact uploader (the Actions runner); without it `private-report: artifact` is refused. */ + readonly artifactUpload: boolean; +} + /** Read and validate every input through `read`; the first problem wins. */ -export function parseConfig(read: InputReader, env: ConfigEnv): Result { +export function parseConfig( + read: InputReader, + env: ConfigEnv, + capabilities: RunCapabilities, +): Result { const input = inputs(read); return safeTry(function* () { // The mode decides which inputs exist at all, so it is read first: a merge never needs the token. @@ -777,6 +803,10 @@ export function parseConfig(read: InputReader, env: ConfigEnv): Result = new Set(); +const EVERY_SECTION = SectionSelection.ALL; /** A layer must be a valid document before it may contribute, so the merge can never complete a broken declaration into a valid one. */ export function foldLayers( @@ -73,12 +74,12 @@ export function foldLayers( > { return Result.combine( layers.map((layer) => - validateSettingsDoc(standaloneView(layer.doc), layer.name, NO_ALLOWLIST, io), + validateSettingsDoc(standaloneView(layer.doc), layer.name, EVERY_SECTION, io), ), ) .andThen(() => mergeLayers(layers, { layering })) .andThen((merged) => - validateSettingsDoc(merged.settings, sourceLabel, NO_ALLOWLIST, io).map((settings) => ({ + validateSettingsDoc(merged.settings, sourceLabel, EVERY_SECTION, io).map((settings) => ({ settings, notices: merged.notices, })), diff --git a/src/flows/library.ts b/src/flows/library.ts index 44a63a58..0d40edc3 100644 --- a/src/flows/library.ts +++ b/src/flows/library.ts @@ -65,7 +65,7 @@ export function validateSettings( return validateSettingsDoc( doc, options.source ?? UNNAMED_SOURCE, - (options.sections ?? SectionSelection.ALL).only, + options.sections ?? SectionSelection.ALL, out.io, ).map((settings) => ({ settings, log: out.log() })); } diff --git a/src/flows/merge.ts b/src/flows/merge.ts index 28abc851..ab37ba13 100644 --- a/src/flows/merge.ts +++ b/src/flows/merge.ts @@ -3,8 +3,6 @@ * here touches GitHub, so the flow takes no client and needs no token. */ -import { mkdirSync, writeFileSync } from "node:fs"; -import { dirname, resolve } from "node:path"; import { err, ok, type Result } from "neverthrow"; import { describeOptOut, type Layering } from "../engine/layers.js"; import type { Io } from "../io.js"; @@ -12,6 +10,7 @@ import type { Problem, ProblemOf } from "../problem.js"; import type { FinishedMerge } from "./deliver.js"; import { foldLayers, readLayerFiles } from "./layers.js"; import { renderMergedYaml } from "./library.js"; +import { readEntries, renameEntry, writeReplacing } from "./settings-write.js"; export interface MergeConfig { settingsFiles: string[]; @@ -22,12 +21,19 @@ export interface MergeConfig { const MERGED_LABEL = "the merged settings document"; /** - * Paths are compared resolved, so "./a.yml" and "a.yml" collide. Guarded beside the write: the next run would fold the - * merged document as if it were a layer. + * An input layer is never the destination, under any name the read follows or the rename reaches: the entry the + * rename replaces (the leaf under its resolved parent, a link there unfollowed) against every entry each layer's read + * follows (each component, every link hop, the final file), compared by identity, so spellings, directory links, case + * aliases, and link chains all meet. `out.yml -> layer.yml` as the destination with `layer.yml` as the layer is + * admitted: the write replaces the link and leaves the layer intact. Guarded beside the write: the next run would + * fold the merged document as if it were a layer. */ function mergedFileCollision(cfg: MergeConfig): Result> { - const mergedPath = resolve(cfg.mergedFile); - const index = cfg.settingsFiles.findIndex((layer) => resolve(layer) === mergedPath); + const replaced = renameEntry(cfg.mergedFile); + if (replaced === null) { + return ok(); + } + const index = cfg.settingsFiles.findIndex((layer) => readEntries(layer).has(replaced)); const layer = cfg.settingsFiles[index]; return layer === undefined ? ok() @@ -42,16 +48,10 @@ export function runMerge(cfg: MergeConfig, io: Io): Result ({ code: "merged-file-unwritable", path: cfg.mergedFile, reason }), + ) + .map(() => ({ layers: cfg.settingsFiles, mergedFile: cfg.mergedFile })); }); } diff --git a/src/flows/multi.ts b/src/flows/multi.ts index 73bee995..32c875a6 100644 --- a/src/flows/multi.ts +++ b/src/flows/multi.ts @@ -10,7 +10,6 @@ * central warnings may name slugs the operator wrote into the workflow or the admin repo. */ -import { readFileSync } from "node:fs"; import { type Err, err, ok, type ResultAsync, safeTry } from "neverthrow"; import { resolveCentralTargets } from "../discovery/central.js"; import { type DiscoveryFilters, discoverRepos, formatSkipNotice } from "../discovery/discover.js"; @@ -38,7 +37,6 @@ import { failedTarget, type OpenedTarget, type RunFlowConfig, - requireUploader, type TargetResult, targetFailure, withDelivery, @@ -138,18 +136,11 @@ async function processTarget(ctx: { return run(defaults, "operator"); } - const parsed = parseSettingsDoc(read.raw); - if (parsed.isErr()) { - return fail( - `cannot parse ${read.sourceLabel}: ${parsed.error.reason}. Fix the YAML in that file`, - ); - } - // validateSettingsDoc names sourceLabel (the slug for remote targets) in its own warnings, so they go through the unprefixed sink. const validated = validateSettingsDoc( - parsed.value, + read.doc, read.sourceLabel, - cfg.sections.only, + cfg.sections, channel.unprefixed, ); if (validated.isErr()) { @@ -186,19 +177,15 @@ async function readTargetSettings( api: GitHubClient, target: Target, ): Promise< - | { raw: string; sourceLabel: string; source: SettingsSource } + | { doc: unknown; sourceLabel: string; source: SettingsSource } | { missing: true } | { error: string } > { if (target.source === "central") { - const sourceLabel = target.filePath; - try { - return { raw: readFileSync(target.filePath, "utf8"), sourceLabel, source: "operator" }; - } catch (error) { - return { - error: `cannot read settings from ${sourceLabel}: ${String(error)}. Fix the file, or delete it to stop managing this repository`, - }; - } + return readSettingsFile(target.filePath, "central-file").match( + (doc) => ({ doc, sourceLabel: target.filePath, source: "operator" }), + (problem) => ({ error: describeProblem(problem) }), + ); } const sourceLabel = `${target.slug}:${DEFAULT_SETTINGS_FILE}`; const file = await getRepoFile(api, target.slug, DEFAULT_SETTINGS_FILE); @@ -217,7 +204,12 @@ async function readTargetSettings( : `reading ${sourceLabel} failed: ${file.error.status} ${file.error.message}. ${RERUN_ADVICE}`, }; } - return { raw: file.content, sourceLabel, source: "target" }; + return parseSettingsDoc(file.content).match( + (doc) => ({ doc, sourceLabel, source: "target" }), + (parse) => ({ + error: `cannot parse ${sourceLabel}: ${parse.reason}. Fix the YAML in that file`, + }), + ); } /** The fleet a run acts on, with the redaction decision every target reports under. */ @@ -389,14 +381,12 @@ export function runMulti( uploader?: ArtifactUploader, ): ResultAsync { return safeTry(async function* () { - yield* requireUploader(cfg, uploader); - // The defaults document is read before the fleet is resolved: nothing about // a target has been emitted yet, so its failure names only the local file. let defaults: ValidatedSettings | null = null; if (cfg.defaultsFile) { const doc = yield* readSettingsFile(cfg.defaultsFile, "defaults-file"); - defaults = yield* validateSettingsDoc(doc, cfg.defaultsFile, cfg.sections.only, io); + defaults = yield* validateSettingsDoc(doc, cfg.defaultsFile, cfg.sections, io); } const { targets, plan, visibilityOf } = yield* resolveTargets(api, cfg, io); diff --git a/src/flows/redact.ts b/src/flows/redact.ts index cb6d5b30..baa6b94e 100644 --- a/src/flows/redact.ts +++ b/src/flows/redact.ts @@ -120,25 +120,41 @@ export function emitRedactedResult( detail: Private, ): void { const { outcomes } = revealPrivate(detail); - if (result === "failed") { - const failed = outcomes - .filter((o) => o.status === "failed") + // The keys in one status, each with its safe HTTP code when the row carries one (a denial's, never a body). + const keysWith = (status: ClosedOutcome["status"]): string => { + const keys = outcomes + .filter((o) => o.status === status) .map((o) => (o.httpStatus !== undefined ? `${o.key} (${o.httpStatus})` : o.key)); - const sections = failed.length > 0 ? ` - ${failed.join(", ")}` : ""; - io.annotate("error", `${display}: failed${sections}. ${REDACTED_NOTE}`); - return; - } - if (result === "drift") { - const drifted = outcomes.filter((o) => o.status === "drift").map((o) => o.key); - const sections = drifted.length > 0 ? ` - ${drifted.join(", ")}` : ""; - io.annotate("warning", `${display}: drift${sections}. ${REDACTED_NOTE}`); - return; - } - if (result === "skipped") { - io.annotate("notice", `${display}: skipped. ${REDACTED_NOTE}`); + return keys.length > 0 ? ` - ${keys.join(", ")}` : ""; + }; + switch (result) { + case "failed": + io.annotate("error", `${display}: failed${keysWith("failed")}. ${REDACTED_NOTE}`); + return; + case "drift": + io.annotate("warning", `${display}: drift${keysWith("drift")}. ${REDACTED_NOTE}`); + return; + case "partial": + io.annotate("warning", `${display}: partial${keysWith("skipped")}. ${REDACTED_NOTE}`); + return; + case "skipped": + io.annotate("notice", `${display}: skipped. ${REDACTED_NOTE}`); + return; + case "applied": + case "clean": + case "snapshot": + case "merged": + // A healthy result prints nothing: the summary row already says so in closed values. + return; + default: + unreachable(result); } } +function unreachable(result: never): never { + throw new Error(`BUG: emitRedactedResult has no arm for the run result ${String(result)}`); +} + export interface RedactionPlan { isRedacted(slug: string): boolean; display(slug: string): string; diff --git a/src/flows/settings-write.ts b/src/flows/settings-write.ts new file mode 100644 index 00000000..251eaeba --- /dev/null +++ b/src/flows/settings-write.ts @@ -0,0 +1,208 @@ +/** + * The one place a run writes a settings document (a snapshot, the merged file, init's starting file): staged beside + * the destination and renamed into place, so a write that fails partway (disk full, an interrupted run) leaves the + * previous file intact instead of a truncated one. The rename is atomic on POSIX and a single replace call on + * Windows. The staging name is unique to the write (pid and random bytes), so no file of the user's is ever unlinked + * or written through: an existing path there fails the write instead. A destination that is a symlink is replaced by + * the rename, the link itself, never its referent, so the written document is always a regular file at `path`. The + * guards over the writer hold one promise: an input layer is never the destination, under any name the read follows + * or the rename reaches; deliberate evasion (hardlinks, mounts, races) is out of scope. + */ + +import { randomBytes } from "node:crypto"; +import { + closeSync, + fchmodSync, + lstatSync, + mkdirSync, + openSync, + readlinkSync, + realpathSync, + renameSync, + rmSync, + statSync, + writeFileSync, +} from "node:fs"; +import { basename, dirname, join, parse, resolve, sep } from "node:path"; +import { err, ok, type Result } from "neverthrow"; + +/** + * `path` as the filesystem names it: the real path of what exists, the rest + * as spelled. Built one segment at a time, so ".." steps out of a symlink's + * TARGET as the write will: handed "link/../x" whole, bun's realpath collapses + * the ".." lexically before following the link and names a different file + * than the one the write reaches. Every step retries realpath, since + * "missing/../link" is back on existing ground after the "..". The flows + * compare a destination against the files they read through this name, so a + * spelling through a symlinked directory (macOS's /tmp for /private/tmp) or a + * case alias cannot slip a write onto an input. + */ +export function canonicalPath(path: string): string { + // The platform reads the root (a drive-relative "C:x" resolves on that drive); the walk reads the rest. + const { root } = parse(path); + let real = realOrSpelled(root === "" ? process.cwd() : resolve(root)); + for (const part of path.slice(root.length).split(sep === "\\" ? /[\\/]/ : sep)) { + if (part === "" || part === ".") { + continue; + } + real = part === ".." ? dirname(real) : realOrSpelled(join(real, part)); + } + return real; +} + +const SEGMENT = sep === "\\" ? /[\\/]/ : sep; + +/** An entry's identity on its filesystem, the same under every name it has. */ +function entryId(stat: { dev: number | bigint; ino: number | bigint }): string { + return `${stat.dev}:${stat.ino}`; +} + +function lstatOrNull(path: string) { + try { + return lstatSync(path); + } catch { + return null; + } +} + +/** + * Every entry a read of `path` follows, by identity: each component as the OS resolves it, every link along the way + * (hop by hop, so a chain's middle entries count), and the final file. Stops recording where nothing exists. + */ +export function readEntries(path: string): Set { + const ids = new Set(); + walkRecording(process.cwd(), path, ids, { hops: 64 }); + return ids; +} + +function walkRecording( + base: string, + path: string, + ids: Set, + budget: { hops: number }, +): string { + const { root } = parse(path); + let current = root === "" ? base : resolve(root); + for (const part of path.slice(root.length).split(SEGMENT)) { + if (part === "" || part === ".") { + continue; + } + current = part === ".." ? dirname(current) : join(current, part); + let stat = lstatOrNull(current); + while (stat?.isSymbolicLink() && budget.hops-- > 0) { + ids.add(entryId(stat)); + let target: string; + try { + target = readlinkSync(current); + } catch { + return current; + } + current = walkRecording(dirname(current), target, ids, budget); + stat = lstatOrNull(current); + } + if (stat !== null) { + ids.add(entryId(stat)); + } + } + return current; +} + +/** + * The identity of the entry a rename onto `path` replaces: the leaf under the parent as the OS resolves it, the leaf + * itself unfollowed (a link there is replaced, not its referent). Null when no entry exists yet, which no read can + * have followed. + */ +export function renameEntry(path: string): string | null { + const parent = walkRecording(process.cwd(), dirname(path), new Set(), { hops: 64 }); + const stat = lstatOrNull(join(parent, basename(path))); + return stat === null ? null : entryId(stat); +} + +/** `path`'s real path when it exists, else `path` itself. */ +function realOrSpelled(path: string): string { + try { + return realpathSync.native(path); + } catch { + return path; + } +} + +/** The bits a replaced regular file keeps; a link at `path` is stat-followed, since the write replaces the link with a regular file of its referent's mode. */ +function regularFileMode(path: string): number | undefined { + try { + const stat = statSync(path); + return stat.isFile() ? stat.mode & 0o7777 : undefined; + } catch { + return undefined; + } +} + +/** + * The name the write reaches: the directory as the filesystem names it plus the leaf as spelled, since a rename + * replaces a link at the leaf rather than following it. + */ +export function renameTarget(path: string): string { + return join(canonicalPath(dirname(path)), basename(path)); +} + +/** + * Every name a write to `path` lands on, for a flow comparing its destination against its inputs: the rename target, + * and, when the leaf is not a link (a link is replaced, its referent untouched), the referent as the filesystem names + * it now, which on a case-insensitive filesystem is the existing file's own spelling. + */ +export function landingNames(path: string): string[] { + const names = [renameTarget(path)]; + if (!isSymlink(path)) { + names.push(canonicalPath(path)); + } + return [...new Set(names)]; +} + +function isSymlink(path: string): boolean { + try { + return lstatSync(path).isSymbolicLink(); + } catch { + return false; + } +} + +/** + * The error is the filesystem's own reason; each caller names the input that chose the path. The staging file sits in + * the destination's directory, spelled as the caller spelled it (a `link/..` segment is the OS's to resolve, the same + * way for both names), under a short name of its own (the destination's leaf may already be at NAME_MAX), and takes + * an existing regular destination's mode, so a replaced 0600 file stays 0600. + */ +export function writeReplacing(path: string, text: string): Result { + const directory = path.slice(0, path.length - basename(path).length); + const staging = `${directory}.gsac-${process.pid}-${randomBytes(4).toString("hex")}.tmp`; + // Set once the exclusive open succeeded: only a staging file THIS write made is removed on failure, never one + // another writer got there first with (`wx` fails on it, and that failure is the one reported). + let created = false; + let fd: number | undefined; + try { + mkdirSync(dirname(path), { recursive: true }); + const mode = regularFileMode(path); + fd = openSync(staging, "wx"); + created = true; + if (mode !== undefined) { + fchmodSync(fd, mode); + } + writeFileSync(fd, text); + closeSync(fd); + fd = undefined; + renameSync(staging, path); + return ok(); + } catch (error) { + if (fd !== undefined) { + try { + closeSync(fd); + } catch {} + } + if (created) { + try { + rmSync(staging, { force: true }); + } catch {} + } + return err(String(error)); + } +} diff --git a/src/flows/single.ts b/src/flows/single.ts index f8e2490e..430651bd 100644 --- a/src/flows/single.ts +++ b/src/flows/single.ts @@ -18,7 +18,6 @@ import { engineOutcome, failedTarget, type RunFlowConfig, - requireUploader, withDelivery, } from "./deliver.js"; import { @@ -72,9 +71,8 @@ export function runSingle( io: Io, uploader?: ArtifactUploader, ): ResultAsync { - return requireUploader(cfg, uploader) - .andThen(() => readSettingsFile(cfg.settingsFile, "settings-file")) - .andThen((doc) => validateSettingsDoc(doc, cfg.settingsFile, cfg.sections.only, io)) + return readSettingsFile(cfg.settingsFile, "settings-file") + .andThen((doc) => validateSettingsDoc(doc, cfg.settingsFile, cfg.sections, io)) .asyncAndThen((settings) => ResultAsync.fromSafePromise(runTarget(api, cfg, io, settings, uploader)), ); diff --git a/src/flows/snapshot.ts b/src/flows/snapshot.ts index 6f62f300..e6804333 100644 --- a/src/flows/snapshot.ts +++ b/src/flows/snapshot.ts @@ -9,8 +9,7 @@ * lands on disk while nothing about it is printed. */ -import { mkdirSync, realpathSync, renameSync, rmSync, writeFileSync } from "node:fs"; -import { dirname, isAbsolute, join, parse, relative, resolve, sep } from "node:path"; +import { isAbsolute, join, relative, resolve, sep } from "node:path"; import { err, ok, type Result, ResultAsync } from "neverthrow"; import type { RepoRef } from "../discovery/targets.js"; import type { SectionSelection } from "../engine/section-selection.js"; @@ -34,6 +33,7 @@ import { type TargetOutcome, toPublicView, } from "./redact.js"; +import { canonicalPath, landingNames, renameTarget, writeReplacing } from "./settings-write.js"; import { openSingleRepoChannel } from "./single.js"; import { writeSnapshotDirSummary, writeSummary } from "./summary.js"; @@ -75,36 +75,6 @@ export type FinishedSnapshot = | { form: "file"; target: Omit } | { form: "dir"; snapshotDir: string; targets: TargetOutcome[] }; -/** - * `path` as the filesystem names it: the real path of what exists, the rest - * as spelled. Built one segment at a time, so ".." steps out of a symlink's - * TARGET as the write will: handed "link/../x" whole, bun's realpath collapses - * the ".." lexically before following the link and names a different file - * than the one the write reaches. Every step retries realpath, since - * "missing/../link" is back on existing ground after the "..". - */ -function canonicalPath(path: string): string { - // The platform reads the root (a drive-relative "C:x" resolves on that drive); the walk reads the rest. - const { root } = parse(path); - let real = realOrSpelled(root === "" ? process.cwd() : resolve(root)); - for (const part of path.slice(root.length).split(sep === "\\" ? /[\\/]/ : sep)) { - if (part === "" || part === ".") { - continue; - } - real = part === ".." ? dirname(real) : realOrSpelled(join(real, part)); - } - return real; -} - -/** `path`'s real path when it exists, else `path` itself. */ -function realOrSpelled(path: string): string { - try { - return realpathSync.native(path); - } catch { - return path; - } -} - /** Whether `path` is `dir` itself or lies under it; both already named the same way. */ function isWithin(path: string, dir: string): boolean { const rel = relative(dir, path); @@ -195,15 +165,14 @@ async function snapshotTarget(ctx: { note: "the snapshot failed, so no file was written", }; } - try { - writeReplacing( - path, - renderSnapshotYaml(result, { schemaUrl: SNAPSHOT_SCHEMA_URL, timestamp: ctx.timestamp }), - ); - } catch (error) { + const written = writeReplacing( + path, + renderSnapshotYaml(result, { schemaUrl: SNAPSHOT_SCHEMA_URL, timestamp: ctx.timestamp }), + ); + if (written.isErr()) { channel.io.annotate( "error", - `cannot write the snapshot to ${path}: ${String(error)}. Check that the "${ctx.pathInput}" input names a writable path`, + `cannot write the snapshot to ${path}: ${written.error}. Check that the "${ctx.pathInput}" input names a writable path`, ); return { result: "failed", @@ -220,41 +189,25 @@ async function snapshotTarget(ctx: { }; } -/** - * Write `text` to `path` through a sibling staging file renamed into place, so - * a write that fails partway (disk full, an interrupted run) leaves the - * previous snapshot at `path` intact instead of a truncated one; the rename is - * atomic on POSIX and a single replace call on Windows. A leftover staging - * file or link is unlinked first, never written through. - */ -function writeReplacing(path: string, text: string): void { - mkdirSync(dirname(path), { recursive: true }); - const staging = `${path}.tmp`; - try { - rmSync(staging, { force: true }); - writeFileSync(staging, text, { flag: "wx" }); - renameSync(staging, path); - } catch (error) { - // The write's error is the one reported: a directory at the staging path fails both the write and this rm. - try { - rmSync(staging, { force: true }); - } catch {} - throw error; - } -} - /** * A target's file under the snapshot directory, in the repos-dir layout so the * directory can later serve as one, or why the target has none. SLUG_RE admits * "." and "..", which GitHub never issues but a repos entry can spell; either * would leave the directory. And the filesystem may carry the file onto * authored ground in a way the two inputs cannot show: a link under either - * directory that leads into the other, or an owner spelled ".github". + * directory that leads into the other, or an owner spelled ".github". Two + * targets can land on ONE file the same way (a link `out/bob -> out/alice` + * with targets alice/r and bob/r), so every file this run writes is claimed + * in `claimed` and a second target reaching it is refused. The refusal names + * the earlier target through `display`, so a redacted one stays sealed, and + * never the landing, whose spelling is the operator's. */ function snapshotFilePath( cfg: Extract, repo: RepoRef, authored: ReadonlySet, + claimed: Map, + display: (slug: string) => string, ): { path: string } | { error: string } { if ([repo.owner, repo.name].some((part) => part === "." || part === "..")) { return { @@ -273,6 +226,22 @@ function snapshotFilePath( error: `cannot write the snapshot to ${path}: the filesystem carries it to ${landing}, inside the "repos-dir" input "${cfg.reposDir}". Write the snapshots to a directory that leads to no central file`, }; } + // The claim is the file the rename reaches; a later target is refused when any of its landing names is claimed + // (its own rename target for a leaf that was a link, its referent for a leaf spelled in another case on a + // case-insensitive filesystem once the first file exists). + const written = renameTarget(path); + const earlier = landingNames(path) + .map((name) => claimed.get(name)) + .find((slug) => slug !== undefined); + if (earlier !== undefined) { + return { + error: + `cannot write the snapshot to ${path}: the filesystem carries it to the file this run already claimed for ` + + `${display(earlier)}. Remove the link under the "snapshot-dir" input that folds the two owners together, so ` + + "each target has a file of its own", + }; + } + claimed.set(written, repo.slug); return { path }; } @@ -332,6 +301,8 @@ async function snapshotDir( canonicalPath(DEFAULT_SETTINGS_FILE), ...resolved.targets.flatMap((t) => (t.source === "central" ? [canonicalPath(t.filePath)] : [])), ]); + // Every landing this run writes, by the target that claimed it; the one writer's answer to two targets on one file. + const claimed = new Map(); const targets: TargetOutcome[] = []; for (const target of resolved.targets) { // The channel is opened BEFORE any processing so a failure lands in a @@ -348,7 +319,9 @@ async function snapshotDir( `the repository name "${target.slug}" from ${target.origin} is not an owner/name slug, so it cannot be snapshotted`, ); } else { - const located = snapshotFilePath(cfg, repo, authored); + const located = snapshotFilePath(cfg, repo, authored, claimed, (slug) => + resolved.plan.display(slug), + ); outcome = "error" in located ? fail(located.error) diff --git a/src/github/api.ts b/src/github/api.ts index 9ac08eae..72d48cfb 100644 --- a/src/github/api.ts +++ b/src/github/api.ts @@ -358,7 +358,7 @@ function apiErrorFromHttp(error: OctokitHttpError, carriesSecret: boolean): ApiE */ export function transportFailure(label: string, reason: string, target: string): Error { return new Error( - `${label} failed: ${reason}. Check network connectivity from the runner to ${target}, then re-run the workflow`, + `${label} failed: ${reason}. Check network connectivity from the runner to ${target}, then re-run`, ); } @@ -634,7 +634,7 @@ export class GitHubApi implements GitHubClient { // The GraphQL contract makes errors, when present, a NON-EMPTY list; a malformed value must not read as "no errors" // and turn a partial response into a success. The body is never quoted. throw new Error( - `GRAPHQL ${op.name} returned a malformed errors value (not a non-empty list); the GraphQL endpoint at ${this.baseUrl} is not answering the GraphQL wire contract. Re-run the workflow, and retry later if it persists`, + `GRAPHQL ${op.name} returned a malformed errors value (not a non-empty list); the GraphQL endpoint at ${this.baseUrl} is not answering the GraphQL wire contract. Re-run, and retry later if it persists`, ); } const errors = Array.isArray(body.errors) ? body.errors : []; @@ -645,7 +645,7 @@ export class GitHubApi implements GitHubClient { if (typeof data !== "object" || data === null || Array.isArray(data)) { // A 200 with neither errors nor a data map is outside the GraphQL contract; the body is not quoted, since it could carry private live state. throw new Error( - `GRAPHQL ${op.name} returned a response carrying neither errors nor a data object; the GraphQL endpoint at ${this.baseUrl} is not answering the GraphQL wire contract. Re-run the workflow, and retry later if it persists`, + `GRAPHQL ${op.name} returned a response carrying neither errors nor a data object; the GraphQL endpoint at ${this.baseUrl} is not answering the GraphQL wire contract. Re-run, and retry later if it persists`, ); } return { data: data as Record }; diff --git a/src/index.ts b/src/index.ts index 406cec08..f0c6aeb8 100644 --- a/src/index.ts +++ b/src/index.ts @@ -35,11 +35,12 @@ export { type FinishedMerge, failRun, } from "./flows/deliver.js"; -export { executeRun, type RunDeps } from "./flows/execute.js"; +export { executeRun, type RunDeps, type RunEnd } from "./flows/execute.js"; export { type ConfigEnv, type InputReader, parseConfig, + type RunCapabilities, type RunConfig, } from "./flows/inputs.js"; export { readLayerFiles } from "./flows/layers.js"; diff --git a/src/internal.ts b/src/internal.ts index cc177fda..2357fe91 100644 --- a/src/internal.ts +++ b/src/internal.ts @@ -45,6 +45,7 @@ export { SNAPSHOT_ONLY_INPUTS, SNAPSHOT_REJECTED_INPUTS, type SnapshotFileConfig, + snapshotFileDestination, } from "./flows/inputs.js"; export { foldLayers } from "./flows/layers.js"; export { renderMergedYaml } from "./flows/library.js"; @@ -63,6 +64,7 @@ export { publicDetail, toPublicView, } from "./flows/redact.js"; +export { writeReplacing } from "./flows/settings-write.js"; export { SNAPSHOT_SCHEMA_URL } from "./flows/snapshot.js"; export { SECRET_RESPONSE_WITHHELD, diff --git a/src/problem.ts b/src/problem.ts index 488664b9..eb0a425c 100644 --- a/src/problem.ts +++ b/src/problem.ts @@ -15,10 +15,11 @@ import type { SectionKey } from "./schema.js"; * The advice appended to a transient (non-permission) API failure: a network * blip or a 5xx that survived the retries. One source for the discovery * problems rendered here and multi.ts's remote-file read failure, so the "not - * a permission problem" wording cannot drift between them. + * a permission problem" wording cannot drift between them. Face-neutral: the + * action and the command line print the same line. */ export const RERUN_ADVICE = - "This is not a permission problem; re-run the workflow, and retry later if it persists"; + "This is not a permission problem; re-run, and retry later if it persists"; /** Names as an error message lists them: each quoted, comma-separated. */ export function quoteList(names: readonly string[]): string { @@ -41,7 +42,7 @@ export type TopLevelShape = | "function"; /** Which input named an unreadable settings file; each role's advice names its fix. */ -export type SettingsFileRole = "settings-file" | "defaults-file" | "layer"; +export type SettingsFileRole = "settings-file" | "defaults-file" | "layer" | "central-file"; /** One repos-dir file the central resolution cannot turn into a target. */ export type CentralFileProblem = @@ -121,9 +122,11 @@ export type Problem = | { readonly code: "input-settings-file-is-list"; readonly value: string; - readonly mode: EngineMode; + /** An engine mode reading its one file, or the command line's init, which writes one. */ + readonly mode: EngineMode | "init"; } | { readonly code: "input-repository-not-slug"; readonly value: string } + | { readonly code: "input-artifact-unsupported" } // The settings document | { readonly code: "settings-not-mapping"; @@ -189,7 +192,6 @@ export type Problem = // The section selection | { readonly code: "required-sections-excluded"; readonly excluded: readonly SectionKey[] } // The run flows - | { readonly code: "artifact-uploader-missing" } | { readonly code: "merged-file-is-layer"; readonly mergedFile: string; @@ -310,6 +312,8 @@ function describeUnreadable(problem: ProblemOf<"settings-file-unreadable">): str return `cannot read the defaults file ${problem.path}: ${problem.reason}. Check the "defaults-file" path and that the file is valid YAML`; case "layer": return `cannot read the settings layer ${problem.path}: ${problem.reason}. Check that every path in the "settings-file" input exists and is valid YAML`; + case "central-file": + return `cannot read the central settings file ${problem.path}: ${problem.reason}. Fix the file, or delete it to stop managing this repository`; } } @@ -335,8 +339,8 @@ function describeUnknownSectionInput( ): string { const quoted = quoteList(unknown.names); return unknown.names.length === 1 - ? `unknown section ${quoted} in the "${unknown.input}" input; it matches none of: ${known.join(", ")}. Fix the name in the workflow's input list` - : `unknown sections ${quoted} in the "${unknown.input}" input; each matches none of: ${known.join(", ")}. Fix the names in the workflow's input list`; + ? `unknown section ${quoted} in the "${unknown.input}" input; it matches none of: ${known.join(", ")}. Fix the section name` + : `unknown sections ${quoted} in the "${unknown.input}" input; each matches none of: ${known.join(", ")}. Fix the section names`; } function describeInvalidReposEntries(problem: ProblemOf<"repos-input-invalid-entries">): string { @@ -441,7 +445,7 @@ export function describeProblem(problem: Problem): string { case "input-snapshot-dir-without-targets": return 'the "snapshot-dir" input needs multi-repo targets: set "repos" (an owner/name list, or "*" to discover) or "repos-dir". To snapshot one repository, set "snapshot-file" instead'; case "input-token-missing": - return 'cannot call the GitHub API: no token was provided. Set the "token" input on the action step (or export GITHUB_TOKEN)'; + return 'cannot call the GitHub API: no token was provided. Set the "token" input (--token on the command line), or export GITHUB_TOKEN'; case "input-report-without-redaction": return 'the "private-report" input delivers reports only for redacted targets, but "private-repos" is "show", so nothing is redacted and no report would ever be sent. Set private-repos: redact, or set private-report: none'; case "input-affiliation-unsupported": @@ -461,14 +465,20 @@ export function describeProblem(problem: Problem): string { case "input-defaults-file-without-multi": return 'the "defaults-file" input only applies to multi-repo mode, but this run is in single-repo mode, so the defaults would never apply. Remove the input, or add "repos" or "repos-dir" to switch to multi-repo mode'; case "input-settings-file-is-list": + return problem.mode === "init" + ? `the "settings-file" input is "${problem.value}", which contains a list separator: init writes exactly one settings file, and only mode: merge takes a newline- or comma-separated list. Name one file` + : `the "settings-file" input is "${problem.value}", which contains a list separator: ` + + `${problem.mode} mode reads exactly one settings file, and only mode: merge takes a ` + + "newline- or comma-separated list. Name one file, or set mode: merge to fold the list into " + + "one document"; + case "input-repository-not-slug": + return `cannot target a repository: "${problem.value}" is not an owner/name slug. Set the "repository" input (--repository on the command line) to a value like "octocat/hello-world"; inside GitHub Actions, GITHUB_REPOSITORY supplies it`; + case "input-artifact-unsupported": return ( - `the "settings-file" input is "${problem.value}", which contains a list separator: ` + - `${problem.mode} mode reads exactly one settings file, and only mode: merge takes a ` + - "newline- or comma-separated list. Name one file, or set mode: merge to fold the list into " + - "one document" + "private-report: artifact uploads the reports as a workflow artifact, which only the GitHub " + + "Actions runner can do, and this run has no artifact upload (the command line, or a library " + + 'caller without an uploader). Set private-report to "issue", "issue-on-failure", or "none"' ); - case "input-repository-not-slug": - return `cannot target a repository: "${problem.value}" is not an owner/name slug. Set the "repository" input (or GITHUB_REPOSITORY) to a value like "octocat/hello-world"`; case "settings-not-mapping": return `${problem.source} must be a YAML mapping of section names to settings, but its top level parsed as a ${problem.shape}. Rewrite the top level as "section: ..." keys`; case "settings-not-plain-mapping": @@ -495,8 +505,6 @@ export function describeProblem(problem: Problem): string { return `${layerSite(problem)} carries no string ${quote(problem.keyField)}, which every entry needs to layer by`; case "layer-duplicate-key": return `${layerSite(problem)}[${problem.first}] and ${problem.site}[${problem.second}] both claim one ${problem.keyField}; each ${problem.keyField} belongs to one entry within a layer`; - case "artifact-uploader-missing": - return "private-report: artifact needs an artifact uploader, and none was supplied: the action supplies its own; a library caller passes one as the uploader argument, or picks another private-report channel"; case "merged-file-is-layer": return ( `the "merged-file" input "${problem.mergedFile}" is layer ${problem.index + 1} of the ` + diff --git a/src/report/artifact-report.ts b/src/report/artifact-report.ts index 0fd58efd..9111626f 100644 --- a/src/report/artifact-report.ts +++ b/src/report/artifact-report.ts @@ -58,7 +58,7 @@ export async function deliverArtifactReport( } catch (error) { const reason = error instanceof Error ? error.message : String(error); return { - warning: `could not upload the private report artifact: ${reason}. Re-run the workflow, or set private-report: none if it persists`, + warning: `could not upload the private report artifact: ${reason}. Re-run, or set private-report: none if it persists`, }; } } diff --git a/src/report/delivery.ts b/src/report/delivery.ts index 1c0898e6..3fa2cd57 100644 --- a/src/report/delivery.ts +++ b/src/report/delivery.ts @@ -12,7 +12,6 @@ import type { GitHubClient } from "../github/api.js"; import type { CollectedLine, Io } from "../io.js"; import type { Private } from "../private.js"; import { revealPrivate } from "../private-open.js"; -import { describeProblem } from "../problem.js"; import type { SectionKey } from "../schema.js"; import { type ArtifactUploader, deliverArtifactReport } from "./artifact-report.js"; import { composeReport } from "./composer.js"; @@ -140,8 +139,8 @@ export interface ReportChannel { } /** - * The flows check for the artifact uploader before any API work (requireUploader); the throw here is the backstop for - * a caller that skipped that check, an invariant violation and not a run outcome. + * parseConfig refuses the artifact channel for a face without an upload capability, so an artifact channel with no + * uploader here is a face that declared a capability it does not hand in: an invariant violation, not a run outcome. */ export function openReportChannel( api: GitHubClient, @@ -160,7 +159,9 @@ export function openReportChannel( return issueChannel(api, meta, "on-failure", io); case "artifact": if (uploader === undefined) { - throw new Error(describeProblem({ code: "artifact-uploader-missing" })); + throw new Error( + "BUG: the artifact report channel was opened without an uploader; parseConfig admits private-report: artifact only for a face with an artifact upload, which must hand its uploader to the run", + ); } return artifactChannel(meta, reportPublicKey, io, uploader); } diff --git a/src/report/issue-report.ts b/src/report/issue-report.ts index e74a04e7..8b713ef7 100644 --- a/src/report/issue-report.ts +++ b/src/report/issue-report.ts @@ -64,7 +64,7 @@ export type IssueDelivery = { url: string } | { skipped: true } | { warning: str function deliveryWarning(error: ApiError): { warning: string } { const advice = isPermissionError(error) ? `To fix, ${grantFor(ISSUE_REPORT_PERMISSION)} for the target repository, or set private-report: none` - : "Re-run the workflow, or set private-report: none if it persists"; + : "Re-run, or set private-report: none if it persists"; return { warning: `could not deliver the private report (HTTP ${error.status}). ${advice}` }; } @@ -306,7 +306,7 @@ export async function deliverIssueReport( // A throw is a network-level failure whose message embeds the request path (the private slug), so nothing from it may escape. return { warning: - "could not deliver the private report: the request failed before an HTTP response arrived. Re-run the workflow, or set private-report: none if it persists", + "could not deliver the private report: the request failed before an HTTP response arrived. Re-run, or set private-report: none if it persists", }; } } diff --git a/src/sections/actions_secrets/actions_secrets.test.ts b/src/sections/actions_secrets/actions_secrets.test.ts index 87a345ee..5b0078d6 100644 --- a/src/sections/actions_secrets/actions_secrets.test.ts +++ b/src/sections/actions_secrets/actions_secrets.test.ts @@ -389,7 +389,7 @@ describe("actions_secrets execution", () => { const validated = validateSettingsDoc( { actions_secrets: [{ name: "DEPLOY_TOKEN", value: "$DEPLOY_TOKEN" }] }, "settings.yml", - new Set(), + SectionSelection.ALL, io, ); if (validated.isErr()) { diff --git a/src/sections/secret_scanning_custom_patterns/secret_scanning_custom_patterns.test.ts b/src/sections/secret_scanning_custom_patterns/secret_scanning_custom_patterns.test.ts index 5f563a64..9f588584 100644 --- a/src/sections/secret_scanning_custom_patterns/secret_scanning_custom_patterns.test.ts +++ b/src/sections/secret_scanning_custom_patterns/secret_scanning_custom_patterns.test.ts @@ -4,6 +4,7 @@ import { fragmentFake } from "../../../test/sections/fragment-fake.js"; import { provePlanIdempotent } from "../../../test/sections/plan-idempotence.js"; import { REPO } from "../../../test/sections/section-run.js"; import { validateSettingsDoc } from "../../engine/orchestrate.js"; +import { SectionSelection } from "../../engine/section-selection.js"; import { silentIo } from "../../io.js"; import { describeProblem } from "../../problem.js"; import { type PlainData, planContext, type SectionPlan } from "../contract/plan.js"; @@ -268,7 +269,7 @@ describe("secret_scanning_custom_patterns", () => { // "" cannot mean "clear it": the PATCH updates provided fields only. for (const key of ["start_delimiter", "end_delimiter"] as const) { const doc = { secret_scanning_custom_patterns: [{ ...INTERNAL, [key]: "" }] }; - const invalid = validateSettingsDoc(doc, "test doc", new Set(), silentIo()); + const invalid = validateSettingsDoc(doc, "test doc", SectionSelection.ALL, silentIo()); expect(invalid.match(() => "", describeProblem)).toContain( "cannot be cleared with an empty string", ); @@ -339,7 +340,7 @@ describe("secret_scanning_custom_patterns closed surface", () => { const error = validateSettingsDoc( { secret_scanning_custom_patterns: [{ ...INTERNAL, [key]: true }] }, "settings.yml", - new Set(), + SectionSelection.ALL, silentIo(), ); expect(error.isErr(), `a declared "${key}" must be rejected`).toBe(true); diff --git a/test/action/artifact.test.ts b/test/action/artifact.test.ts index 9d379b04..f8060335 100644 --- a/test/action/artifact.test.ts +++ b/test/action/artifact.test.ts @@ -33,7 +33,7 @@ describe("the action's uploader without a runtime token", () => { "could not upload the private report artifact: the artifact service is unavailable: no " + "ACTIONS_RUNTIME_TOKEN in the environment. Artifact upload needs a GitHub-hosted or " + "self-hosted Actions runner (it is not available on GitHub Enterprise Server or outside " + - "Actions). Re-run the workflow, or set private-report: none if it persists", + "Actions). Re-run, or set private-report: none if it persists", }); expect(uploadSpy).toHaveBeenCalledTimes(0); } finally { diff --git a/test/cli/execution.test.ts b/test/cli/execution.test.ts index 1b2e010b..b967de64 100644 --- a/test/cli/execution.test.ts +++ b/test/cli/execution.test.ts @@ -352,7 +352,9 @@ describe("the action and the CLI run one arm to one result", () => { test("the table reaches every arm, both snapshot forms, both exit codes, and a partial result", () => { const arms = new Set(); for (const c of cases) { - const parsed = parseConfig((name) => c.inputs("")[name] ?? "", c.env)._unsafeUnwrap(); + const parsed = parseConfig((name) => c.inputs("")[name] ?? "", c.env, { + artifactUpload: true, + })._unsafeUnwrap(); arms.add(parsed.kind === "snapshot" ? `snapshot:${parsed.form}` : parsed.kind); } expect([...arms].sort()).toEqual(["merge", "multi", "single", "snapshot:dir", "snapshot:file"]); diff --git a/test/cli/init.test.ts b/test/cli/init.test.ts index 2225160c..fa586169 100644 --- a/test/cli/init.test.ts +++ b/test/cli/init.test.ts @@ -105,25 +105,25 @@ describe("init: argv -> config", () => { "no token", ["--repository", "o/r"], {}, - "cannot call the GitHub API: no token was provided. Pass --token, or export GITHUB_TOKEN", + 'cannot call the GitHub API: no token was provided. Set the "token" input (--token on the command line), or export GITHUB_TOKEN', ], [ "no repository outside Actions", [], { GITHUB_TOKEN: TOKEN }, - 'cannot target a repository: "" is not an owner/name slug. Pass --repository owner/name (inside GitHub Actions, GITHUB_REPOSITORY supplies it)', + 'cannot target a repository: "" is not an owner/name slug. Set the "repository" input (--repository on the command line) to a value like "octocat/hello-world"; inside GitHub Actions, GITHUB_REPOSITORY supplies it', ], [ "a settings file spelled as a list", ["--repository", "o/r", "--settings-file", "a.yml,b.yml"], { GITHUB_TOKEN: TOKEN }, - 'the --settings-file value "a.yml,b.yml" contains a comma or a newline, which check and apply read as a list separator. Name one path without them', + 'the "settings-file" input is "a.yml,b.yml", which contains a list separator: init writes exactly one settings file, and only mode: merge takes a newline- or comma-separated list. Name one file', ], [ "a repeated settings file", ["--repository", "o/r", "--settings-file", "a.yml", "--settings-file", "b.yml"], { GITHUB_TOKEN: TOKEN }, - 'the --settings-file value "a.yml\nb.yml" contains a comma or a newline, which check and apply read as a list separator. Name one path without them', + 'the "settings-file" input is "a.yml\nb.yml", which contains a list separator: init writes exactly one settings file, and only mode: merge takes a newline- or comma-separated list. Name one file', ], ])( "%s is refused before any config exists, with a remedy a terminal can follow", @@ -428,7 +428,7 @@ describe("init: an existing settings file", () => { new MockApi({}), ); const problem = - "cannot call the GitHub API: no token was provided. Pass --token, or export GITHUB_TOKEN"; + 'cannot call the GitHub API: no token was provided. Set the "token" input (--token on the command line), or export GITHUB_TOKEN'; expect(result).toEqual({ code: 1, stdout: `${JSON.stringify({ result: "failed", file, problem })}\n`, diff --git a/test/cli/inputs.test.ts b/test/cli/inputs.test.ts index 6c21058a..1d7fa63d 100644 --- a/test/cli/inputs.test.ts +++ b/test/cli/inputs.test.ts @@ -10,7 +10,6 @@ import { beforeAll, describe, expect, test } from "bun:test"; import { generateX25519Identity, identityToRecipient } from "age-encryption"; -import { ARTIFACT_REFUSED, describeCliProblem } from "../../src/cli/commands.js"; import { CLI_UNSUPPORTED_INPUTS, declared, @@ -233,7 +232,7 @@ function cases(): Case[] { "private-report": "artifact", }, env: { GITHUB_REPOSITORY: "o/admin" }, - cliRefuses: describeCliProblem(ARTIFACT_REFUSED), + cliRefuses: describeProblem({ code: "input-artifact-unsupported" }), }, { name: "merge, layers as repeated flags", @@ -315,19 +314,22 @@ async function throughArgv(argv: readonly string[], env: ConfigEnv) { colors: false, execute: async (cfg) => { parsed = cfg; - return 0; + return { exitCode: 0 }; }, }); return { parsed, code, stderr: stderr.text() }; } +/** The action's face: it hands the run an artifact uploader, so parseConfig admits the artifact channel. */ +const ACTION = { artifactUpload: true } as const; + describe("argv -> config equals env -> config", () => { test("for every RunConfig arm and every rejection", async () => { for (const { name, argv, inputs, env = {}, unknownFlag, cliRefuses } of cases()) { - const expected = parseConfig(recordReader(inputs), env); + const expected = parseConfig(recordReader(inputs), env, ACTION); const actual = await throughArgv(argv, env); if (cliRefuses !== undefined) { - // The action would ask for the channel's key next; the CLI refuses the channel itself. + // The action would ask for the channel's key next; the CLI's face has no artifact upload, so parseConfig refuses the channel for it. expect(actual, name).toEqual({ parsed: undefined, code: 1, @@ -343,7 +345,7 @@ describe("argv -> config equals env -> config", () => { expect(actual.parsed, name).toBeUndefined(); expect(actual.code, name).toBe(1); if (unknownFlag === undefined) { - expect(actual.stderr, name).toBe(`error: ${describeCliProblem(expected.error)}\n`); + expect(actual.stderr, name).toBe(`error: ${describeProblem(expected.error)}\n`); } else { expect(actual.stderr, name).toStartWith(`error: unknown option '--${unknownFlag}'`); } @@ -356,7 +358,7 @@ describe("argv -> config equals env -> config", () => { let envToken = 0; let rejected = 0; for (const { argv, inputs, env = {}, cliRefuses } of cases()) { - const result = parseConfig(recordReader(inputs), env); + const result = parseConfig(recordReader(inputs), env, ACTION); if (result.isErr() || cliRefuses !== undefined) { rejected++; continue; @@ -453,13 +455,14 @@ describe("the per-mode flag split", () => { const result = parseConfig( recordReader({ ...base(mode), ...companions(mode, name), [name]: value }), env, + ACTION, ); expect( result.isOk(), `${mode} --${name}: ${result.match(() => "", describeProblem)}`, ).toBe(true); } else { - const result = parseConfig(recordReader({ ...base(mode), [name]: value }), env); + const result = parseConfig(recordReader({ ...base(mode), [name]: value }), env, ACTION); expect(result.isErr(), `${mode} --${name} should be rejected by parseConfig`).toBe(true); expect( result.match( @@ -707,19 +710,17 @@ describe("the help text", () => { }); test("the private-report flag's help offers exactly the channels the CLI accepts", () => { - // The CLI's refusal is the source: its value leaves the list, its allowed channels stay. - if (ARTIFACT_REFUSED.code !== "input-unsupported-value") { - throw new Error("ARTIFACT_REFUSED no longer names the refused value"); - } + // parseConfig refuses the artifact channel for a face without an upload, so its value leaves the list and the other channels stay. + const accepted = PRIVATE_REPORT_CHANNELS.filter((channel) => channel !== "artifact"); const check = inputDescription("private-report", modeSubcommand("check")); const opening = check.slice(0, check.indexOf(". ") + 1); const named: string[] = PRIVATE_REPORT_CHANNELS.filter((channel) => new RegExp(`(? { expect(result.code).toBe(1); expect(api.calls).toHaveLength(0); expect(result.stderr).toBe( - 'error: cannot target a repository: "not-a-slug" is not an owner/name slug. Pass --repository owner/name (inside GitHub Actions, GITHUB_REPOSITORY supplies it)\n', + 'error: cannot target a repository: "not-a-slug" is not an owner/name slug. Set the "repository" input (--repository on the command line) to a value like "octocat/hello-world"; inside GitHub Actions, GITHUB_REPOSITORY supplies it\n', ); expect(result.stdout).toBe( "result: failed\nresult=failed\nskipped-sections=\nrepos-result={}\n", @@ -139,17 +139,19 @@ describe("check and apply", () => { [ "no token", ["check", "--repository", "o/r"], - "cannot call the GitHub API: no token was provided. Pass --token, or export GITHUB_TOKEN", + 'cannot call the GitHub API: no token was provided. Set the "token" input (--token on the command line), or export GITHUB_TOKEN', ], [ "no repository outside Actions", ["check", "--token", TOKEN], - 'cannot target a repository: "" is not an owner/name slug. Pass --repository owner/name (inside GitHub Actions, GITHUB_REPOSITORY supplies it)', + 'cannot target a repository: "" is not an owner/name slug. Set the "repository" input (--repository on the command line) to a value like "octocat/hello-world"; inside GitHub Actions, GITHUB_REPOSITORY supplies it', ], [ "the artifact channel", ["check", ...target, "--private-report", "artifact"], - 'the "private-report" input is "artifact", which is not a supported private-report channel from the command line (the artifact upload needs the Actions runner). Set it to "none" (default), "issue", "issue-on-failure"', + "private-report: artifact uploads the reports as a workflow artifact, which only the GitHub " + + "Actions runner can do, and this run has no artifact upload (the command line, or a library " + + 'caller without an uploader). Set private-report to "issue", "issue-on-failure", or "none"', ], ])("%s fails with a remedy a terminal can follow", async (_case, args, message) => { const api = new MockApi({}); diff --git a/test/docs/guides.test.ts b/test/docs/guides.test.ts index bf44a846..d68fd507 100644 --- a/test/docs/guides.test.ts +++ b/test/docs/guides.test.ts @@ -6,6 +6,7 @@ import { ok } from "neverthrow"; import { parse as parseYaml } from "yaml"; import { type Layer, mergeLayers, stripNulls } from "../../src/engine/layers.js"; import { validateSettingsDoc } from "../../src/engine/orchestrate.js"; +import { SectionSelection } from "../../src/engine/section-selection.js"; import { MERGE_REJECTED_INPUTS, SNAPSHOT_REJECTED_INPUTS } from "../../src/flows/inputs.js"; import { foldLayers } from "../../src/flows/layers.js"; import { silentIo } from "../../src/io.js"; @@ -709,7 +710,7 @@ describe("docs/ guide pages", () => { ).toBe(expected); if (row.gate === "validation") { // "with the same messages a standalone file gets" - const standalone = validateSettingsDoc(doc, layerName, new Set(), silentIo()); + const standalone = validateSettingsDoc(doc, layerName, SectionSelection.ALL, silentIo()); expect( standalone.match(() => null, describeProblem), `standalone ${input}`, @@ -725,7 +726,7 @@ describe("docs/ guide pages", () => { const result = validateSettingsDoc( { labels: { undeclared: "keep", entries: [{ name: "bug", color: "d73a4a" }] } }, ".github/settings.yml", - new Set(), + SectionSelection.ALL, silentIo(), ); if (result.isOk()) { diff --git a/test/docs/settings-examples.ts b/test/docs/settings-examples.ts index d0d31160..0d3bfe9e 100644 --- a/test/docs/settings-examples.ts +++ b/test/docs/settings-examples.ts @@ -4,12 +4,13 @@ import { expect } from "bun:test"; import { validateSettingsDoc } from "../../src/engine/orchestrate.js"; +import { SectionSelection } from "../../src/engine/section-selection.js"; import { silentIo } from "../../src/io.js"; import { SPECIAL_KEYS } from "../../src/sections/repository/index.js"; /** Assert `doc` validates and its repository special-looking keys are real. */ export function assertValidSettingsExample(doc: unknown, label: string): void { - const invalid = validateSettingsDoc(doc, label, new Set(), silentIo()); + const invalid = validateSettingsDoc(doc, label, SectionSelection.ALL, silentIo()); expect( "error" in invalid ? invalid.error : null, `${label} failed validation: ${"error" in invalid ? invalid.error : ""}`, diff --git a/test/e2e/generators.test.ts b/test/e2e/generators.test.ts index a6ef15da..e5e2c0a6 100644 --- a/test/e2e/generators.test.ts +++ b/test/e2e/generators.test.ts @@ -4,6 +4,7 @@ import { basename, join } from "node:path"; import { parse as parseYaml, stringify as stringifyYaml } from "yaml"; import { describeOptOut } from "../../src/engine/layers.js"; import { validateSettingsDoc } from "../../src/engine/orchestrate.js"; +import { SectionSelection } from "../../src/engine/section-selection.js"; import { silentIo } from "../../src/io.js"; import { describeProblem } from "../../src/problem.js"; import { SECTION_KEYS, type SectionKey } from "../../src/schema.js"; @@ -50,7 +51,7 @@ describe("three-way drift detection", () => { `${key} seed ${i}: published schema rejected the doc: ${error instanceof Error ? error.message : String(error)}`, ); } - const verdict = validateSettingsDoc(doc, "fuzz", new Set(), silentIo()); + const verdict = validateSettingsDoc(doc, "fuzz", SectionSelection.ALL, silentIo()); if ("error" in verdict) { offenders.push( `${key} seed ${i}: validateSettingsDoc rejected the doc: ${verdict.error}`, @@ -447,7 +448,7 @@ describe("genInvalidSettings", () => { for (const { name, build } of INVALID_SETTINGS_CASES) { for (let i = 0; i < 25; i++) { const { doc, offendingToken } = build(new Rng(i * 13 + 1)); - const verdict = validateSettingsDoc(doc, "settings.yml", new Set(), silentIo()); + const verdict = validateSettingsDoc(doc, "settings.yml", SectionSelection.ALL, silentIo()); if (verdict.isOk()) { throw new Error(`case "${name}" produced a doc the validator accepts`); } @@ -1080,7 +1081,12 @@ describe("genMergeScenario", () => { continue; } merged++; - const verdict = validateSettingsDoc(prediction.merged, "merged", new Set(), silentIo()); + const verdict = validateSettingsDoc( + prediction.merged, + "merged", + SectionSelection.ALL, + silentIo(), + ); expect("error" in verdict ? verdict.error : undefined, `seed ${seed}`).toBeUndefined(); expect(parseYaml(stringifyYaml(prediction.merged))).toEqual(prediction.merged); // The directives address the fold; none may reach the written document. diff --git a/test/e2e/generators.ts b/test/e2e/generators.ts index 3e71f673..d190d996 100644 --- a/test/e2e/generators.ts +++ b/test/e2e/generators.ts @@ -8,6 +8,7 @@ import { Ajv, type ValidateFunction } from "ajv"; import addFormats from "ajv-formats"; import settingsSchema from "../../lib/settings.schema.json" with { type: "json" }; import { validateSettingsDoc } from "../../src/engine/orchestrate.js"; +import { SectionSelection } from "../../src/engine/section-selection.js"; import { silentIo } from "../../src/io.js"; import { SECTION_KEYS, @@ -1515,7 +1516,10 @@ function markerNullsDropped(doc: Json): Json { * through the action's own validator. */ function standaloneValid(doc: Json): boolean { - return !("error" in validateSettingsDoc(markerNullsDropped(doc), "layer", new Set(), silentIo())); + return !( + "error" in + validateSettingsDoc(markerNullsDropped(doc), "layer", SectionSelection.ALL, silentIo()) + ); } /** The runner's file name for layer `index` of `count`: settings.yml is always the top. */ diff --git a/test/e2e/oracle.ts b/test/e2e/oracle.ts index 6f67a1cb..576cb66f 100644 --- a/test/e2e/oracle.ts +++ b/test/e2e/oracle.ts @@ -6,6 +6,7 @@ import type { OptOutNotice } from "../../src/engine/layers.js"; import { validateSettingsDoc } from "../../src/engine/orchestrate.js"; +import { SectionSelection } from "../../src/engine/section-selection.js"; import { silentIo } from "../../src/io.js"; import { describeProblem } from "../../src/problem.js"; import { @@ -1033,7 +1034,7 @@ export function predictMerge(meta: MergeScenarioMeta): MergePrediction { const folded = foldMergeLayers(meta.layers, meta.layering); // Whether the fold is a valid document is the validator's question, the same one the run asks: // cross-field rules the published schema cannot spell, so the generator cannot avoid them by construction. - const validated = validateSettingsDoc(folded.merged, "merged", new Set(), silentIo()); + const validated = validateSettingsDoc(folded.merged, "merged", SectionSelection.ALL, silentIo()); if (validated.isErr()) { return { kind: "invalid", error: describeProblem(validated.error) }; } diff --git a/test/engine/check-purity.test.ts b/test/engine/check-purity.test.ts index d20f5c48..713d1b03 100644 --- a/test/engine/check-purity.test.ts +++ b/test/engine/check-purity.test.ts @@ -209,7 +209,12 @@ describe("check-mode purity", () => { test("every registered section stays read-only in check mode, even on its drift paths", async () => { const api = new MockApi(ROUTES); // Brand the fixture document through the REAL boundary: an invalid fixture fails loudly here instead of riding a cast into runForRepo. - const verdict = validateSettingsDoc(FIXTURES, "purity fixtures", new Set(), silentIo()); + const verdict = validateSettingsDoc( + FIXTURES, + "purity fixtures", + SectionSelection.ALL, + silentIo(), + ); if (verdict.isErr()) { throw new Error(`purity fixtures failed validation: ${describeProblem(verdict.error)}`); } diff --git a/test/engine/layers.test.ts b/test/engine/layers.test.ts index 76034392..2e41c329 100644 --- a/test/engine/layers.test.ts +++ b/test/engine/layers.test.ts @@ -2,6 +2,7 @@ import { describe, expect, test } from "bun:test"; import { err, ok } from "neverthrow"; import { type Layer, type Layering, mergeLayers, stripNulls } from "../../src/engine/layers.js"; import { validateSettingsDoc } from "../../src/engine/orchestrate.js"; +import { SectionSelection } from "../../src/engine/section-selection.js"; import { silentIo } from "../../src/io.js"; import { describeProblem, type LayerProblem } from "../../src/problem.js"; import { planContext } from "../../src/sections/contract/plan.js"; @@ -985,7 +986,7 @@ describe("stripNulls", () => { const upper = deepFreeze({ rulesets: [{ name: "main", bypass_actors: null }] }); // Widened so the whole verdict can be pinned by value; the brand is opaque to toEqual. const validate = (doc: unknown): unknown => - validateSettingsDoc(doc, "repo", new Set(), silentIo()); + validateSettingsDoc(doc, "repo", SectionSelection.ALL, silentIo()); expect(validate(upper)).toEqual( err({ code: "settings-malformed-sections", diff --git a/test/engine/orchestrate.test.ts b/test/engine/orchestrate.test.ts index 45c3969c..792b27be 100644 --- a/test/engine/orchestrate.test.ts +++ b/test/engine/orchestrate.test.ts @@ -24,7 +24,7 @@ import { MockApi } from "../mock-api.js"; /** Brand fixtures through the REAL boundary: an invalid fixture fails here instead of riding a cast into runForRepo. */ function validated(doc: SettingsFile): ValidatedSettings { - return validateSettingsDoc(doc, "test fixture", new Set(), silentIo()).match( + return validateSettingsDoc(doc, "test fixture", SectionSelection.ALL, silentIo()).match( (settings) => settings, (problem) => { throw new Error(`test fixture failed validation: ${describeProblem(problem)}`); @@ -199,7 +199,7 @@ describe("runForRepo", () => { const polluted = JSON.parse( '{"rulesets":{"entries":[{"name":"r"}],"__proto__":{"planted":2}}}', ); - expect(validateSettingsDoc(polluted, "s.yml", new Set(), captureIo().io)).toEqual( + expect(validateSettingsDoc(polluted, "s.yml", SectionSelection.ALL, captureIo().io)).toEqual( err({ code: "settings-malformed-sections", source: "s.yml", @@ -330,7 +330,7 @@ describe("runForRepo secret references", () => { describe("validateSettingsDoc", () => { test("unknown top-level keys are a problem naming the source and the known sections", () => { const { io } = captureIo(); - expect(validateSettingsDoc({ labls: [] }, "repos/x.yml", new Set(), io)).toEqual( + expect(validateSettingsDoc({ labls: [] }, "repos/x.yml", SectionSelection.ALL, io)).toEqual( err({ code: "settings-unknown-sections", source: "repos/x.yml", @@ -348,12 +348,24 @@ describe("validateSettingsDoc", () => { source: "f.yml", unknown: ["_notes", "_layerin"], }); - expect(validateSettingsDoc(doc, "f.yml", new Set(), io)).toEqual(refused); + expect(validateSettingsDoc(doc, "f.yml", SectionSelection.ALL, io)).toEqual(refused); // Outside a `sections` allowlist an unknown SECTION only warns; the underscore rule has no such downgrade. - expect(validateSettingsDoc(doc, "f.yml", new Set(["repository"]), io)).toEqual(refused); + expect( + validateSettingsDoc( + doc, + "f.yml", + SectionSelection.of({ only: ["repository"] })._unsafeUnwrap(), + io, + ), + ).toEqual(refused); expect(annotations).toEqual([]); expect( - validateSettingsDoc({ _layering: "replace", repository: {} }, "f.yml", new Set(), io).isOk(), + validateSettingsDoc( + { _layering: "replace", repository: {} }, + "f.yml", + SectionSelection.ALL, + io, + ).isOk(), ).toBe(true); }); @@ -364,7 +376,7 @@ describe("validateSettingsDoc", () => { ["a number", 7, "number"], ])("a non-mapping document (%s) is rejected with its shape", (_what, doc, shape) => { const { io } = captureIo(); - expect(validateSettingsDoc(doc, "f.yml", new Set(), io)).toEqual( + expect(validateSettingsDoc(doc, "f.yml", SectionSelection.ALL, io)).toEqual( err({ code: "settings-not-mapping", source: "f.yml", shape }), ); }); @@ -373,15 +385,20 @@ describe("validateSettingsDoc", () => { // parse("!!timestamp ...") returns a Date, an object with no keys; branding it valid would turn the whole document into a silent green no-op. const { io } = captureIo(); const tagged = err({ code: "settings-not-plain-mapping" as const, source: "f.yml" }); - expect(validateSettingsDoc(new Date(0), "f.yml", new Set(), io)).toEqual(tagged); - expect(validateSettingsDoc(new Set(["a"]), "f.yml", new Set(), io)).toEqual(tagged); + expect(validateSettingsDoc(new Date(0), "f.yml", SectionSelection.ALL, io)).toEqual(tagged); + expect(validateSettingsDoc(new Set(["a"]), "f.yml", SectionSelection.ALL, io)).toEqual(tagged); }); test("a valid document comes back branded, ready for runForRepo", () => { const { io } = captureIo(); const doc = { repository: { has_wiki: false } }; // The brand is compile-time only; the value is zod's parsed copy. - const branded: unknown = validateSettingsDoc(doc, "s.yml", new Set(), io)._unsafeUnwrap(); + const branded: unknown = validateSettingsDoc( + doc, + "s.yml", + SectionSelection.ALL, + io, + )._unsafeUnwrap(); expect(branded).toEqual(doc); expect(branded).not.toBe(doc); }); diff --git a/test/engine/secrets.test.ts b/test/engine/secrets.test.ts index 2759dbf9..49e7b860 100644 --- a/test/engine/secrets.test.ts +++ b/test/engine/secrets.test.ts @@ -129,7 +129,7 @@ describe("secret provenance is one source per document", () => { describe("runForRepo provenance", () => { // Branded through the REAL boundary, so an invalid fixture fails here instead of riding a cast. const validated = (doc: unknown): ValidatedSettings => { - const verdict = validateSettingsDoc(doc, "fixture", new Set(), silentIo()); + const verdict = validateSettingsDoc(doc, "fixture", SectionSelection.ALL, silentIo()); if (verdict.isErr()) { throw new Error(`fixture failed validation: ${describeProblem(verdict.error)}`); } diff --git a/test/flows/deliver.test.ts b/test/flows/deliver.test.ts index d1224065..77978f30 100644 --- a/test/flows/deliver.test.ts +++ b/test/flows/deliver.test.ts @@ -406,7 +406,7 @@ describe("concludeRun", () => { expect(failRun(io, { code: "input-token-missing" })).toBe(1); expect(outputs).toEqual({ result: "failed", "skipped-sections": "", "repos-result": "{}" }); expect(events).toEqual([ - 'annotate error: cannot call the GitHub API: no token was provided. Set the "token" input on the action step (or export GITHUB_TOKEN)', + 'annotate error: cannot call the GitHub API: no token was provided. Set the "token" input (--token on the command line), or export GITHUB_TOKEN', "output result=failed", "output skipped-sections=", "output repos-result={}", diff --git a/test/flows/execute.test.ts b/test/flows/execute.test.ts index 2c0c30f4..e184fb50 100644 --- a/test/flows/execute.test.ts +++ b/test/flows/execute.test.ts @@ -9,7 +9,6 @@ import { generateX25519Identity, identityToRecipient } from "age-encryption"; import { type ArtifactUploader, collectingIo, - describeProblem, executeRun, parseRepoSlug, type RunConfig, @@ -71,7 +70,7 @@ describe("executeRun", () => { }, d.run, ); - expect(code).toBe(0); + expect(code).toEqual({ exitCode: 0 }); expect(d.opened()).toBe(0); expect(api.calls).toEqual([]); expect(d.collected.outputs).toEqual({ @@ -85,12 +84,20 @@ describe("executeRun", () => { ]); })); - test("a fatal problem raised after the parse is worded by the face's describe", () => + test("a fatal problem raised after the parse comes back beside the exit code, worded once on the Io", () => withTempDir("gsac-execute-", async (dir) => { const settingsFile = join(dir, "missing.yml"); const api = new MockApi({}); - const d = deps(api, { describe: (problem) => `worded: ${problem.code}` }); - expect(await executeRun(single({ settingsFile }), d.run)).toBe(1); + const d = deps(api); + expect(await executeRun(single({ settingsFile }), d.run)).toEqual({ + exitCode: 1, + fatal: { + code: "settings-file-unreadable", + role: "settings-file", + path: settingsFile, + reason: expect.stringContaining("ENOENT"), + }, + }); expect(d.opened()).toBe(1); expect(api.calls).toEqual([]); expect(d.collected.outputs).toEqual({ @@ -99,30 +106,15 @@ describe("executeRun", () => { "repos-result": "{}", }); expect(d.collected.lines).toEqual([ - { level: "error", line: "worded: settings-file-unreadable" }, + { level: "error", line: expect.stringMatching(/^cannot read settings from /) }, { line: "result: failed" }, ]); })); - test("the artifact channel is the uploader dep's: absent it fails before any API call, present the run reaches the API", async () => { + test("the artifact channel uploads through the uploader dep, and a target proven public gets no report", async () => { const reportPublicKey = await identityToRecipient(await generateX25519Identity()); const cfg = single({ privateRepos: "redact", privateReport: "artifact", reportPublicKey }); const routes = { "GET /repos/o/r": { data: { has_wiki: false, private: false } } }; - - const refused = new MockApi(routes); - const without = deps(refused); - expect(await executeRun(cfg, without.run)).toBe(1); - expect(refused.calls).toEqual([]); - expect(without.collected.outputs).toEqual({ - result: "failed", - "skipped-sections": "", - "repos-result": "{}", - }); - expect(without.collected.lines).toEqual([ - { level: "error", line: describeProblem({ code: "artifact-uploader-missing" }) }, - { line: "result: failed" }, - ]); - const uploads: string[] = []; const uploader: ArtifactUploader = { async upload(name) { @@ -131,14 +123,13 @@ describe("executeRun", () => { }; const reached = new MockApi(routes); const with_ = deps(reached, { uploader }); - expect(await executeRun(cfg, with_.run)).toBe(0); + expect(await executeRun(cfg, with_.run)).toEqual({ exitCode: 0 }); expect(reached.calls.map((c) => `${c.method} ${c.path}`)).toContain("GET /repos/o/r"); expect(with_.collected.outputs).toEqual({ result: "clean", "skipped-sections": "", "repos-result": "{}", }); - // A target proven public gets no private report, so nothing was uploaded. expect(uploads).toEqual([]); }); }); diff --git a/test/flows/inputs.test.ts b/test/flows/inputs.test.ts index 8941381b..c3004567 100644 --- a/test/flows/inputs.test.ts +++ b/test/flows/inputs.test.ts @@ -11,9 +11,9 @@ import { } from "../../src/index.js"; import { INPUT_DECLS, type InputName, SNAPSHOT_REJECTED_INPUTS } from "../../src/internal.js"; -/** Parse a step's inputs (unset ones read as empty, as the runner reports them) under `env`. */ +/** Parse a step's inputs (unset ones read as empty, as the runner reports them) under `env`, as the action's face does. */ function parse(inputs: Partial>, env: ConfigEnv = {}) { - return parseConfig((name) => inputs[name] ?? "", env); + return parseConfig((name) => inputs[name] ?? "", env, { artifactUpload: true }); } /** A single-repo apply run's inputs, with `inputs` on top. */ diff --git a/test/flows/library.test.ts b/test/flows/library.test.ts index d01d1559..371c9813 100644 --- a/test/flows/library.test.ts +++ b/test/flows/library.test.ts @@ -178,7 +178,7 @@ describe("a caller-supplied client that echoes a secret", () => { `fetch failed; body was ${JSON.stringify({ config: { secret: plaintext } })}`, ); }, - detail: `webhooks: POST /repos/o/r/hooks failed: ${SECRET_TRANSPORT_WITHHELD}. Check network connectivity from the runner to the GitHub API, then re-run the workflow`, + detail: `webhooks: POST /repos/o/r/hooks failed: ${SECRET_TRANSPORT_WITHHELD}. Check network connectivity from the runner to the GitHub API, then re-run`, }, ])("apply withholds the failure when the client $answer", async ({ create, detail }) => { const { client, marks } = echoing(create); diff --git a/test/flows/merge.test.ts b/test/flows/merge.test.ts index c6a91b5f..1a9e4941 100644 --- a/test/flows/merge.test.ts +++ b/test/flows/merge.test.ts @@ -1,5 +1,15 @@ import { describe, expect, test } from "bun:test"; -import { readFileSync, writeFileSync } from "node:fs"; +import { + chmodSync, + existsSync, + lstatSync, + mkdirSync, + readdirSync, + readFileSync, + statSync, + symlinkSync, + writeFileSync, +} from "node:fs"; import { join } from "node:path"; import { err, ok } from "neverthrow"; import { collectingIo, concludeMerge, type MergeConfig, runMerge } from "../../src/index.js"; @@ -60,4 +70,189 @@ describe("runMerge", () => { expect(readFileSync(join(dir, "repo.yml"), "utf8")).toBe(REPO); }), ); + + test("a sibling file beside the merged file is never touched, whether a layer or the user's own", () => + withTempDir("run-merge-", (dir) => { + // The write stages under a name unique to the run, so a `.tmp` the user owns, or one that IS a + // layer, survives the write untouched. + writeFileSync(join(dir, "fleet.yml"), FLEET); + const owned = join(dir, "merged.yml.tmp"); + writeFileSync(owned, REPO); + const mergedFile = join(dir, "merged.yml"); + const collected = collectingIo(); + const merged = runMerge( + { settingsFiles: [join(dir, "fleet.yml"), owned], mergedFile, layering: "merge" }, + collected.io, + ); + expect(merged).toEqual(ok({ layers: [join(dir, "fleet.yml"), owned], mergedFile })); + expect(readFileSync(owned, "utf8")).toBe(REPO); + expect(readFileSync(mergedFile, "utf8")).toBe( + "repository:\n has_wiki: false\n has_issues: true\n", + ); + expect(readdirSync(dir).sort()).toEqual(["fleet.yml", "merged.yml", "merged.yml.tmp"]); + })); +}); + +describe("runMerge writes through the shared writer", () => { + const twoLayers = (dir: string): string[] => { + writeFileSync(join(dir, "fleet.yml"), FLEET); + writeFileSync(join(dir, "repo.yml"), REPO); + return [join(dir, "fleet.yml"), join(dir, "repo.yml")]; + }; + const MERGED = "repository:\n has_wiki: false\n has_issues: true\n"; + const merge = (layers: string[], mergedFile: string) => + runMerge({ settingsFiles: layers, mergedFile, layering: "merge" }, collectingIo().io); + + test("a merged-file that is a link to a layer is admitted: the rename replaces the link and the layer stands", () => + withTempDir("run-merge-", (dir) => { + const layers = twoLayers(dir); + const mergedFile = join(dir, "out.yml"); + symlinkSync("repo.yml", mergedFile); + expect(merge(layers, mergedFile)).toEqual(ok({ layers, mergedFile })); + expect(lstatSync(mergedFile).isSymbolicLink()).toBe(false); + expect(readFileSync(mergedFile, "utf8")).toBe(MERGED); + expect(readFileSync(join(dir, "repo.yml"), "utf8")).toBe(REPO); + })); + + test("a layer read through a link to the destination is refused: the write would change what the next run reads", () => + withTempDir("run-merge-", (dir) => { + twoLayers(dir); + const alias = join(dir, "alias.yml"); + symlinkSync("repo.yml", alias); + const layers = [join(dir, "fleet.yml"), alias]; + const mergedFile = join(dir, "repo.yml"); + expect(merge(layers, mergedFile)).toEqual( + err({ code: "merged-file-is-layer" as const, mergedFile, index: 1, layer: alias }), + ); + expect(readFileSync(join(dir, "repo.yml"), "utf8")).toBe(REPO); + })); + + test.each<[string, (dir: string) => { layer: string; mergedFile: string }]>([ + [ + "a chain: the destination is a link in the middle of the layer's read", + (d) => { + symlinkSync("repo.yml", join(d, "out.yml")); + symlinkSync("out.yml", join(d, "alias.yml")); + return { layer: join(d, "alias.yml"), mergedFile: join(d, "out.yml") }; + }, + ], + [ + "a directory link the layer is read through", + (d) => { + symlinkSync(".", join(d, "folder")); + return { layer: join(d, "folder", "repo.yml"), mergedFile: join(d, "folder") }; + }, + ], + ])("%s is refused: the write would change what the next run reads", (_case, shape) => + withTempDir("run-merge-", (dir) => { + twoLayers(dir); + const { layer, mergedFile } = shape(dir); + const layers = [join(dir, "fleet.yml"), layer]; + expect(merge(layers, mergedFile)).toEqual( + err({ code: "merged-file-is-layer" as const, mergedFile, index: 1, layer }), + ); + expect(readFileSync(join(dir, "repo.yml"), "utf8")).toBe(REPO); + }), + ); + + test("a merged-file that spells a layer's link in another case is refused on a case-insensitive filesystem", () => + withTempDir("run-merge-", (dir) => { + writeFileSync(join(dir, "Probe"), ""); + const caseInsensitive = existsSync(join(dir, "probe")); + twoLayers(dir); + const alias = join(dir, "alias.yml"); + symlinkSync("repo.yml", alias); + const layers = [join(dir, "fleet.yml"), alias]; + const mergedFile = join(dir, "ALIAS.YML"); + const merged = merge(layers, mergedFile); + if (caseInsensitive) { + expect(merged).toEqual( + err({ code: "merged-file-is-layer" as const, mergedFile, index: 1, layer: alias }), + ); + expect(lstatSync(alias).isSymbolicLink()).toBe(true); + } else { + expect(merged).toEqual(ok({ layers, mergedFile })); + } + })); + + test("a layer that IS the link at the destination is refused: the rename would replace the layer's own entry", () => + withTempDir("run-merge-", (dir) => { + twoLayers(dir); + const link = join(dir, "out.yml"); + symlinkSync("repo.yml", link); + const layers = [join(dir, "fleet.yml"), link]; + expect(merge(layers, link)).toEqual( + err({ code: "merged-file-is-layer" as const, mergedFile: link, index: 1, layer: link }), + ); + expect(lstatSync(link).isSymbolicLink()).toBe(true); + })); + + test("an existing merged file keeps its permission bits through the replace", () => + withTempDir("run-merge-", (dir) => { + const layers = twoLayers(dir); + const mergedFile = join(dir, "merged.yml"); + writeFileSync(mergedFile, "stale\n"); + chmodSync(mergedFile, 0o600); + // A permissive umask, so a writer that forgot the mode would land at 0644 and fail the pin. + const previous = process.umask(0o022); + try { + expect(merge(layers, mergedFile)).toEqual(ok({ layers, mergedFile })); + } finally { + process.umask(previous); + } + expect(readFileSync(mergedFile, "utf8")).toBe(MERGED); + expect(statSync(mergedFile).mode & 0o777).toBe(0o600); + })); + + test("a merged-file spelled through a link and .. stages beside the destination the OS resolves, not beside cwd", () => + withTempDir("run-merge-", (dir) => { + // `link/../out.yml`: the OS follows the link and steps up, so the file lands beside `inner`; a staging name + // built with join() would collapse `link/..` lexically and stage under `dir`. `dir` is made read-only for the + // write, so a staging file placed there fails the run instead of quietly renaming across. + mkdirSync(join(dir, "elsewhere", "inner"), { recursive: true }); + symlinkSync(join("elsewhere", "inner"), join(dir, "link")); + const layers = twoLayers(dir); + const mergedFile = `${dir}/link/../out.yml`; + chmodSync(dir, 0o555); + try { + expect(merge(layers, mergedFile)).toEqual(ok({ layers, mergedFile })); + } finally { + chmodSync(dir, 0o755); + } + expect(readFileSync(join(dir, "elsewhere", "out.yml"), "utf8")).toBe(MERGED); + expect(readdirSync(join(dir, "elsewhere")).sort()).toEqual(["inner", "out.yml"]); + expect(readdirSync(dir).filter((name) => name.startsWith(".gsac-"))).toEqual([]); + })); + + test("a merged-file that spells an existing layer in another case is refused on a case-insensitive filesystem", () => + withTempDir("run-merge-", (dir) => { + writeFileSync(join(dir, "Probe"), ""); + const caseInsensitive = existsSync(join(dir, "probe")); + const layers = twoLayers(dir); + const mergedFile = join(dir, "REPO.YML"); + const merged = merge(layers, mergedFile); + if (caseInsensitive) { + expect(merged).toEqual( + err({ + code: "merged-file-is-layer" as const, + mergedFile, + index: 1, + layer: join(dir, "repo.yml"), + }), + ); + expect(readFileSync(join(dir, "repo.yml"), "utf8")).toBe(REPO); + } else { + expect(merged).toEqual(ok({ layers, mergedFile })); + expect(readFileSync(mergedFile, "utf8")).toBe(MERGED); + } + })); + + test("a merged-file leaf at the filesystem's name limit is written: the staging name is short and its own", () => + withTempDir("run-merge-", (dir) => { + const layers = twoLayers(dir); + const mergedFile = join(dir, `${"m".repeat(251)}.yml`); + expect(merge(layers, mergedFile)).toEqual(ok({ layers, mergedFile })); + expect(readFileSync(mergedFile, "utf8")).toBe(MERGED); + expect(readdirSync(dir).filter((name) => name.startsWith(".gsac-"))).toEqual([]); + })); }); diff --git a/test/flows/redact.test.ts b/test/flows/redact.test.ts index 85606f10..1825e6cd 100644 --- a/test/flows/redact.test.ts +++ b/test/flows/redact.test.ts @@ -300,6 +300,9 @@ describe("public projections", () => { test.each([ ["failed", "error: private repository #1: failed - labels (403). "], ["drift", "warning: private repository #1: drift - rulesets. "], + // No row is skipped in this fixture, so the partial line carries no section list; the flow test in + // test/flows/single.test.ts pins the `- labels (403)` form. + ["partial", "warning: private repository #1: partial. "], ["skipped", "notice: private repository #1: skipped. "], ] as const)("emitRedactedResult on %s names only closed values", (result, head) => { const { io, events } = captureIo(); diff --git a/test/flows/single.test.ts b/test/flows/single.test.ts index 479542a2..d666cafe 100644 --- a/test/flows/single.test.ts +++ b/test/flows/single.test.ts @@ -1,9 +1,9 @@ import { describe, expect, test } from "bun:test"; import { err, ok } from "neverthrow"; +import { REDACTED_NOTE } from "../../src/flows/redact.js"; import { collectingIo, parseRepoSlug, - runMulti, runSingle, SectionSelection, type SingleConfig, @@ -27,6 +27,44 @@ const cfg = (overrides: Partial = {}): SingleConfig => ({ }); describe("runSingle", () => { + test("a redacted target ending partial prints its one sealed line: the skipped sections and their codes, no detail", async () => { + // A private repository under on-missing-permission: warn with one denied section: the shown run prints the + // section's skipped line; the redacted one must still say partial, or a denied grant goes unnoticed. + const api = new MockApi({ + "GET /repos/o/r": { + data: { private: true, visibility: "private", has_wiki: false, has_projects: false }, + }, + "GET /repos/o/r/labels?per_page=100&page=1": { + error: { + status: 403, + message: "Resource not accessible by personal access token", + body: "", + }, + }, + }); + const collected = collectingIo(); + const outcome = await runSingle( + api, + cfg({ + settingsFile: "test/fixtures/layers/fleet.yml", + sections: SectionSelection.of({ only: ["repository", "labels"] })._unsafeUnwrap(), + onMissingPermission: "warn", + privateRepos: "redact", + selfSlug: "admin/fleet", + }), + collected.io, + ); + expect(outcome.map((target) => [target.result, target.display])).toEqual( + ok(["partial", "private repository #1"]), + ); + expect(collected.lines).toEqual([ + { + level: "warning", + line: `private repository #1: partial - labels (403). ${REDACTED_NOTE}`, + }, + ]); + }); + test("a clean check returns the one target's outcome and prints nothing", async () => { const api = new MockApi({ "GET /repos/o/r": { data: { has_wiki: false } } }); const collected = collectingIo(); @@ -44,41 +82,6 @@ describe("runSingle", () => { expect(collected.lines).toEqual([]); }); - test("the artifact channel without an uploader is fatal before any API call", async () => { - const api = new MockApi({}); - const fatal = err({ code: "artifact-uploader-missing" as const }); - expect( - await runSingle( - api, - cfg({ privateRepos: "redact", privateReport: "artifact", reportPublicKey: "age1x" }), - collectingIo().io, - ), - ).toEqual(fatal); - expect( - await runMulti( - api, - { - ...cfg({ privateRepos: "redact", privateReport: "artifact", reportPublicKey: "age1x" }), - reposDir: "", - reposInput: "o/a", - defaultsFile: "", - adminOwner: "o", - discoveryFilters: { - visibility: "all", - archived: "skip", - forks: "include", - affiliation: ["owner"], - topics: [], - exclude: [], - }, - discoveryFiltersSet: [], - }, - collectingIo().io, - ), - ).toEqual(fatal); - expect(api.calls).toEqual([]); - }); - test("an unreadable settings file is fatal, carrying the path under the settings-file role", async () => { const api = new MockApi({}); const result = await runSingle(api, cfg({ settingsFile: "missing.yml" }), collectingIo().io); diff --git a/test/flows/snapshot.test.ts b/test/flows/snapshot.test.ts index 3860abd7..fc5a6a27 100644 --- a/test/flows/snapshot.test.ts +++ b/test/flows/snapshot.test.ts @@ -10,9 +10,9 @@ import { describe, expect, test } from "bun:test"; import { existsSync, - lstatSync, mkdirSync, mkdtempSync, + readdirSync, readFileSync, realpathSync, rmSync, @@ -20,7 +20,7 @@ import { writeFileSync, } from "node:fs"; import { tmpdir } from "node:os"; -import { dirname, join, sep } from "node:path"; +import { basename, dirname, join, sep } from "node:path"; import { parse as parseYaml } from "yaml"; import { parseRepoSlug } from "../../src/discovery/targets.js"; import { SectionSelection } from "../../src/engine/section-selection.js"; @@ -40,6 +40,11 @@ import { withTempDir } from "../temp-dir.js"; const repo = parseRepoSlug("o/r")._unsafeUnwrap(); +/** `text` as a regex source matching itself literally. */ +function escapeRegExp(text: string): string { + return text.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); +} + /** Run a snapshot the way the action does: the finished run concludes, a problem fails the run. */ async function run(api: MockApi, cfg: SnapshotConfig, io: Io): Promise { return (await runSnapshot(api, cfg, io)).match( @@ -321,59 +326,29 @@ describe("runSnapshot writes through a staging file", () => { line: `cannot write the snapshot to ${path}: ${os}. Check that the "${input}" input names a writable path`, }); - test("a staging write that fails leaves the previous snapshot intact and reports the write's error", () => + test("the user's own `.tmp` sibling is never touched: the write stages under a name of its own", () => withTempDir("snapshot-flow-", async (dir) => { const api = new MockApi(labelsRoute("o/r", [BUG])); const cfg = fileCfg(dir); - const staging = `${cfg.snapshotFile}.tmp`; + const sibling = `${cfg.snapshotFile}.tmp`; mkdirSync(dirname(cfg.snapshotFile), { recursive: true }); writeFileSync(cfg.snapshotFile, "labels: []\n"); - // A directory at the staging path fails the run before the rename; the run must not remove it either. - mkdirSync(staging); - const collected = collectingIo(); - expect(await run(api, cfg, collected.io)).toBe(1); - expect(collected.outputs).toEqual({ - result: "failed", - "skipped-sections": "", - "repos-result": "{}", - }); - expect(collected.lines).toEqual([ - unwritable( - cfg.snapshotFile, - "snapshot-file", - `SystemError [ERR_FS_EISDIR]: Path is a directory: rm returned EISDIR (is a directory) ${staging}`, - ), - { line: "result: failed" }, - ]); - expect(readFileSync(cfg.snapshotFile, "utf8")).toBe("labels: []\n"); - rmSync(staging, { recursive: true }); - expect(await run(api, cfg, collectingIo().io)).toBe(0); - expect(parseYaml(readFileSync(cfg.snapshotFile, "utf8"))).toEqual(doc(BUG)); - expect(existsSync(staging)).toBe(false); - })); - - test("a leftover link at the staging path is unlinked, never written through: the destination becomes a regular file", () => - withTempDir("snapshot-flow-", async (dir) => { - const api = new MockApi(labelsRoute("o/r", [BUG])); - const cfg = fileCfg(dir); - const staging = `${cfg.snapshotFile}.tmp`; - mkdirSync(dirname(cfg.snapshotFile), { recursive: true }); - writeFileSync(cfg.snapshotFile, "labels: []\n"); - symlinkSync("snapshot.yml", staging); + // A directory with content at the `.tmp` sibling: a writer staging THERE would remove it or fail on it. + mkdirSync(sibling); + writeFileSync(join(sibling, "keep"), "user data\n"); const collected = collectingIo(); expect(await run(api, cfg, collected.io)).toBe(0); - expect(collected.outputs).toEqual({ - result: "snapshot", - "skipped-sections": "", - "repos-result": "{}", - }); expect(collected.lines).toEqual([ { line: `snapshot written to ${cfg.snapshotFile}` }, { line: "result: snapshot" }, ]); - expect(lstatSync(cfg.snapshotFile).isSymbolicLink()).toBe(false); expect(parseYaml(readFileSync(cfg.snapshotFile, "utf8"))).toEqual(doc(BUG)); - expect(existsSync(staging)).toBe(false); + expect(readFileSync(join(sibling, "keep"), "utf8")).toBe("user data\n"); + // No staging file of the run's own is left beside the destination. + expect(readdirSync(dirname(cfg.snapshotFile)).sort()).toEqual([ + basename(cfg.snapshotFile), + basename(sibling), + ]); })); test("a rename that fails removes the staging file, fails only that target, and leaves the destination as it was", () => @@ -403,17 +378,25 @@ describe("runSnapshot writes through a staging file", () => { const { level, line } = unwritable( fileA, "snapshot-dir", - `Error: EISDIR: illegal operation on a directory, rename '${fileA}.tmp' -> '${fileA}'`, + `Error: EISDIR: illegal operation on a directory, rename '' -> '${fileA}'`, ); + // The staging name carries the pid and random bytes, so the line is matched with that piece wild. + const [head = "", tail = ""] = line.split(""); + const stagingRe = `${escapeRegExp(join(cfg.snapshotDir, "o"))}/\\.gsac-\\d+-[0-9a-f]{8}\\.tmp`; expect(collected.lines).toEqual([ - { level, line: `o/a: ${line}` }, + { + level, + line: expect.stringMatching( + new RegExp(`^o/a: ${escapeRegExp(head)}${stagingRe}${escapeRegExp(tail)}$`), + ), + }, { line: `o/b: snapshot written to ${fileB}` }, { line: "result: failed" }, ]); - expect(existsSync(`${fileA}.tmp`)).toBe(false); + // No staging file of the run's own is left beside either destination. + expect(readdirSync(join(cfg.snapshotDir, "o")).sort()).toEqual(["a.yml", "b.yml"]); expect(readFileSync(join(fileA, "keep"), "utf8")).toBe("authored\n"); expect(parseYaml(readFileSync(fileB, "utf8"))).toEqual(doc(DOCS)); - expect(existsSync(`${fileB}.tmp`)).toBe(false); expect(collected.summary[0]?.split("\n").slice(0, 8)).toEqual([ "## github-settings-as-code (snapshot, 2 repositories)", "", @@ -822,6 +805,127 @@ describe("runSnapshot, dir form", () => { ]); })); + test("two targets carried to one file by an operator link fail the second, naming the first; the first's file stands", () => + withTempDir("snapshot-flow-", async (dir) => { + const api = new MockApi({ + "GET /repos/alice/r": { data: { private: false } }, + "GET /repos/bob/r": { data: { private: false } }, + ...labelsRoute("alice/r", [BUG]), + ...labelsRoute("bob/r", [DOCS]), + }); + const cfg = dirCfg(dir, { reposInput: "alice/r,bob/r" }); + // `snapshots/bob -> snapshots/alice`: bob/r's file lands on alice/r's, and a last-writer-wins would report both + // written. alice/r's destination is itself a link to an older file: the rename replaces the LINK, so the claim + // is the leaf under the real directory, not the old file the link pointed at, and old/r.yml stays as it was. + mkdirSync(join(cfg.snapshotDir, "alice"), { recursive: true }); + symlinkSync("alice", join(cfg.snapshotDir, "bob")); + mkdirSync(join(dir, "old")); + writeFileSync(join(dir, "old", "r.yml"), "labels: []\n"); + symlinkSync(join("..", "..", "old", "r.yml"), join(cfg.snapshotDir, "alice", "r.yml")); + const collected = collectingIo(); + expect(await run(api, cfg, collected.io)).toBe(1); + expect(collected.lines).toEqual([ + { line: `alice/r: snapshot written to ${join(cfg.snapshotDir, "alice", "r.yml")}` }, + { + level: "error", + line: + `bob/r: cannot write the snapshot to ${join(cfg.snapshotDir, "bob", "r.yml")}: the filesystem carries it to ` + + 'the file this run already claimed for alice/r. Remove the link under the "snapshot-dir" input that folds ' + + "the two owners together, so each target has a file of its own", + }, + { line: "result: failed" }, + ]); + expect(collected.outputs).toEqual({ + "skipped-sections": "", + result: "failed", + "repos-result": JSON.stringify({ + "alice/r": { result: "snapshot", source: "remote", "skipped-sections": [] }, + "bob/r": { result: "failed", source: "remote", "skipped-sections": [] }, + }), + }); + expect(parseYaml(readFileSync(join(cfg.snapshotDir, "alice", "r.yml"), "utf8"))).toEqual( + doc(BUG), + ); + expect(readdirSync(join(cfg.snapshotDir, "alice"))).toEqual(["r.yml"]); + expect(readFileSync(join(dir, "old", "r.yml"), "utf8")).toBe("labels: []\n"); + })); + + test("a destination that was a link to another owner's file claims only itself: that owner's own target still writes", () => + withTempDir("snapshot-flow-", async (dir) => { + // alice/r.yml -> ../old/r.yml before the run; the rename replaces the link, so old/r.yml is not alice's file and + // the target old/r keeps its own. + const api = new MockApi({ + "GET /repos/alice/r": { data: { private: false } }, + "GET /repos/old/r": { data: { private: false } }, + ...labelsRoute("alice/r", [BUG]), + ...labelsRoute("old/r", [DOCS]), + }); + const cfg = dirCfg(dir, { reposInput: "alice/r,old/r" }); + mkdirSync(join(cfg.snapshotDir, "alice"), { recursive: true }); + mkdirSync(join(cfg.snapshotDir, "old")); + writeFileSync(join(cfg.snapshotDir, "old", "r.yml"), "labels: []\n"); + symlinkSync(join("..", "old", "r.yml"), join(cfg.snapshotDir, "alice", "r.yml")); + const collected = collectingIo(); + expect(await run(api, cfg, collected.io)).toBe(0); + expect(collected.lines).toEqual([ + { line: `alice/r: snapshot written to ${join(cfg.snapshotDir, "alice", "r.yml")}` }, + { line: `old/r: snapshot written to ${join(cfg.snapshotDir, "old", "r.yml")}` }, + { line: "result: snapshot" }, + ]); + expect(parseYaml(readFileSync(join(cfg.snapshotDir, "alice", "r.yml"), "utf8"))).toEqual( + doc(BUG), + ); + expect(parseYaml(readFileSync(join(cfg.snapshotDir, "old", "r.yml"), "utf8"))).toEqual( + doc(DOCS), + ); + })); + + test("two targets whose leaves differ only in case are one file on a case-insensitive filesystem: the second is refused", () => + withTempDir("snapshot-flow-", async (dir) => { + // Slugs dedupe case-insensitively, so the alias needs two owners folded by a link and leaves spelled apart: + // alice/R writes alice/R.yml, bob/r reaches the same directory through the link with the leaf r.yml. The + // filesystem's own name for that path (realpath, on-disk case) is the first's file, so the referent name + // catches what the rename target cannot; in either order. + writeFileSync(join(dir, "Probe"), ""); + const caseInsensitive = existsSync(join(dir, "probe")); + for (const [first, second] of [ + ["alice/R", "bob/r"], + ["bob/r", "alice/R"], + ] as const) { + const api = new MockApi({ + "GET /repos/alice/R": { data: { private: false } }, + "GET /repos/bob/r": { data: { private: false } }, + ...labelsRoute("alice/R", [BUG]), + ...labelsRoute("bob/r", [DOCS]), + }); + const cfg = dirCfg(dir, { + reposInput: `${first},${second}`, + snapshotDir: join(dir, `snapshots-${first.replace("/", "-")}`), + }); + mkdirSync(join(cfg.snapshotDir, "alice"), { recursive: true }); + symlinkSync("alice", join(cfg.snapshotDir, "bob")); + const collected = collectingIo(); + expect(await run(api, cfg, collected.io)).toBe(caseInsensitive ? 1 : 0); + const written = readdirSync(join(cfg.snapshotDir, "alice")); + if (caseInsensitive) { + expect(collected.lines[1]).toEqual({ + level: "error", + line: expect.stringMatching( + new RegExp( + `^${escapeRegExp(second)}: cannot write the snapshot to .*: the filesystem carries it to the file this run already claimed for ${escapeRegExp(first)}\\. `, + ), + ), + }); + expect(written).toHaveLength(1); + expect( + parseYaml(readFileSync(join(cfg.snapshotDir, "alice", written[0] ?? ""), "utf8")), + ).toEqual(doc(first === "alice/R" ? BUG : DOCS)); + } else { + expect(written.sort()).toEqual(["R.yml", "r.yml"]); + } + } + })); + test("writes one /.yml per resolved target and publishes the per-target rollup", () => withTempDir("snapshot-flow-", async (dir) => { const api = new MockApi({ diff --git a/test/github/api.test.ts b/test/github/api.test.ts index 54931092..221cea29 100644 --- a/test/github/api.test.ts +++ b/test/github/api.test.ts @@ -812,7 +812,7 @@ describe("secret-field request redaction and fail-closed error responses", () => expect(thrown.message).toBe( "PATCH /repos/hookco/hookrepo/hooks/1/config failed: the transport failed before an HTTP " + "response arrived (details withheld: the request carried a secret field). Check network " + - "connectivity from the runner to https://api.test, then re-run the workflow", + "connectivity from the runner to https://api.test, then re-run", ); }); @@ -827,7 +827,7 @@ describe("secret-field request redaction and fail-closed error responses", () => thrown = error as Error; } expect(thrown?.message).toBe( - "GET /repos/hookco/hookrepo failed: socket hang up. Check network connectivity from the runner to https://api.test, then re-run the workflow", + "GET /repos/hookco/hookrepo failed: socket hang up. Check network connectivity from the runner to https://api.test, then re-run", ); }); @@ -876,7 +876,7 @@ describe("secret-field request redaction and fail-closed error responses", () => (error: unknown) => String(error), ); expect(thrown).toBe( - `Error: POST ${path} failed: the transport failed before an HTTP response arrived (details withheld: the request carried a secret field). Check network connectivity from the runner to https://api.test, then re-run the workflow`, + `Error: POST ${path} failed: the transport failed before an HTTP response arrived (details withheld: the request carried a secret field). Check network connectivity from the runner to https://api.test, then re-run`, ); }); diff --git a/test/github/graphql.test.ts b/test/github/graphql.test.ts index a26bd495..4cea918a 100644 --- a/test/github/graphql.test.ts +++ b/test/github/graphql.test.ts @@ -398,7 +398,7 @@ describe("tryGraphql tracing and redaction", () => { (error: unknown) => String(error), ); expect(thrown).toBe( - "Error: GRAPHQL RepoToggles failed: the transport failed before an HTTP response arrived (details withheld: the repository is redacted). Check network connectivity from the runner to https://api.test, then re-run the workflow", + "Error: GRAPHQL RepoToggles failed: the transport failed before an HTTP response arrived (details withheld: the repository is redacted). Check network connectivity from the runner to https://api.test, then re-run", ); }); diff --git a/test/problem.test.ts b/test/problem.test.ts index afdbe3d1..d0a79e6d 100644 --- a/test/problem.test.ts +++ b/test/problem.test.ts @@ -22,8 +22,7 @@ const PASSTHROUGH = "through, except in closed sections and strict nested objects like actions.cache, which reject " + "unrecognized keys)"; -const RERUN = - "This is not a permission problem; re-run the workflow, and retry later if it persists"; +const RERUN = "This is not a permission problem; re-run, and retry later if it persists"; const PAT = "Discovery needs a user PAT; the workflow GITHUB_TOKEN and GitHub App installation tokens " + @@ -51,8 +50,8 @@ const SPECIMENS = { ], known: SECTION_KEYS, }, - `unknown section "nope" in the "required-sections" input; it matches none of: ${KNOWN}. Fix the name in the workflow's input list; ` + - `unknown sections "typo", "nope" in the "sections" input; each matches none of: ${KNOWN}. Fix the names in the workflow's input list`, + `unknown section "nope" in the "required-sections" input; it matches none of: ${KNOWN}. Fix the section name; ` + + `unknown sections "typo", "nope" in the "sections" input; each matches none of: ${KNOWN}. Fix the section names`, ], "required-sections-excluded": [ { code: "required-sections-excluded", excluded: ["labels", "milestones"] }, @@ -124,7 +123,7 @@ const SPECIMENS = { ], "input-token-missing": [ { code: "input-token-missing" }, - 'cannot call the GitHub API: no token was provided. Set the "token" input on the action step (or export GITHUB_TOKEN)', + 'cannot call the GitHub API: no token was provided. Set the "token" input (--token on the command line), or export GITHUB_TOKEN', ], "input-report-without-redaction": [ { code: "input-report-without-redaction" }, @@ -166,7 +165,13 @@ const SPECIMENS = { ], "input-repository-not-slug": [ { code: "input-repository-not-slug", value: "nope" }, - 'cannot target a repository: "nope" is not an owner/name slug. Set the "repository" input (or GITHUB_REPOSITORY) to a value like "octocat/hello-world"', + 'cannot target a repository: "nope" is not an owner/name slug. Set the "repository" input (--repository on the command line) to a value like "octocat/hello-world"; inside GitHub Actions, GITHUB_REPOSITORY supplies it', + ], + "input-artifact-unsupported": [ + { code: "input-artifact-unsupported" }, + "private-report: artifact uploads the reports as a workflow artifact, which only the GitHub " + + "Actions runner can do, and this run has no artifact upload (the command line, or a library " + + 'caller without an uploader). Set private-report to "issue", "issue-on-failure", or "none"', ], "settings-not-mapping": [ { code: "settings-not-mapping", source: "f.yml", shape: "list" }, @@ -246,10 +251,6 @@ const SPECIMENS = { }, 'layer "repo": labels[0] and labels[2] both claim one name; each name belongs to one entry within a layer', ], - "artifact-uploader-missing": [ - { code: "artifact-uploader-missing" }, - "private-report: artifact needs an artifact uploader, and none was supplied: the action supplies its own; a library caller passes one as the uploader argument, or picks another private-report channel", - ], "merged-file-is-layer": [ { code: "merged-file-is-layer", mergedFile: "./repo.yml", index: 1, layer: "repo.yml" }, 'the "merged-file" input "./repo.yml" is layer 2 of the "settings-file" list ("repo.yml"): ' + @@ -380,6 +381,21 @@ describe("describeProblem", () => { { code: "settings-file-unreadable", role: "layer", path: "fleet.yml", reason: "ENOENT" }, 'cannot read the settings layer fleet.yml: ENOENT. Check that every path in the "settings-file" input exists and is valid YAML', ], + [ + "a central repos-dir file that cannot be read", + { + code: "settings-file-unreadable", + role: "central-file", + path: "repos/o/r.yml", + reason: "EACCES", + }, + "cannot read the central settings file repos/o/r.yml: EACCES. Fix the file, or delete it to stop managing this repository", + ], + [ + "init's settings-file spelled as a list", + { code: "input-settings-file-is-list", value: "a.yml,b.yml", mode: "init" }, + 'the "settings-file" input is "a.yml,b.yml", which contains a list separator: init writes exactly one settings file, and only mode: merge takes a newline- or comma-separated list. Name one file', + ], [ "no targets with nothing filtered", { code: "no-targets", filteredOut: 0 }, diff --git a/test/report/artifact-report.test.ts b/test/report/artifact-report.test.ts index a72a2bc8..cf857081 100644 --- a/test/report/artifact-report.test.ts +++ b/test/report/artifact-report.test.ts @@ -87,7 +87,7 @@ describe("deliverArtifactReport", () => { expect(await deliverArtifactReport(uploader, "doc", recipient)).toEqual({ warning: "could not upload the private report artifact: Unable to get the ACTIONS_RUNTIME_TOKEN " + - "env variable. Re-run the workflow, or set private-report: none if it persists", + "env variable. Re-run, or set private-report: none if it persists", }); }); @@ -97,7 +97,7 @@ describe("deliverArtifactReport", () => { // The middle of the warning is the age library's own wording, so only our prefix and advice are pinned. expect(result).toEqual({ warning: expect.stringMatching( - /^could not upload the private report artifact: .+\. Re-run the workflow, or set private-report: none if it persists$/, + /^could not upload the private report artifact: .+\. Re-run, or set private-report: none if it persists$/, ), }); expect(uploads).toEqual([]); diff --git a/test/report/delivery.test.ts b/test/report/delivery.test.ts index 0a4295dd..9a3a3d8f 100644 --- a/test/report/delivery.test.ts +++ b/test/report/delivery.test.ts @@ -6,6 +6,7 @@ import { type ValidatedSettings, validateSettingsDoc, } from "../../src/engine/orchestrate.js"; +import { SectionSelection } from "../../src/engine/section-selection.js"; import { runOutcome } from "../../src/flows/deliver.js"; import { redactedChannel } from "../../src/flows/redact.js"; import { type Io, silentIo } from "../../src/io.js"; @@ -213,7 +214,7 @@ describe("the artifact channel", () => { await channel?.flush(); expect(annotations).toEqual([ "warning: could not upload the private report artifact: Unable to get the " + - "ACTIONS_RUNTIME_TOKEN env variable. Re-run the workflow, or set private-report: none if " + + "ACTIONS_RUNTIME_TOKEN env variable. Re-run, or set private-report: none if " + "it persists", ]); }); @@ -222,7 +223,7 @@ describe("the artifact channel", () => { describe("applyMarkerInjection", () => { // Branded through the REAL boundary, so an invalid fixture fails here instead of riding a cast into the injection. const validated = (doc: SettingsFile): ValidatedSettings => { - const verdict = validateSettingsDoc(doc, "test fixture", new Set(), silentIo()); + const verdict = validateSettingsDoc(doc, "test fixture", SectionSelection.ALL, silentIo()); if (verdict.isErr()) { throw new Error(`test fixture failed validation: ${describeProblem(verdict.error)}`); } diff --git a/test/report/issue-report.test.ts b/test/report/issue-report.test.ts index f876dfbd..798a88b0 100644 --- a/test/report/issue-report.test.ts +++ b/test/report/issue-report.test.ts @@ -1,5 +1,6 @@ import { describe, expect, test } from "bun:test"; import { validateSettingsDoc } from "../../src/engine/orchestrate.js"; +import { SectionSelection } from "../../src/engine/section-selection.js"; import { silentIo } from "../../src/io.js"; import { describeProblem } from "../../src/problem.js"; import { REPORT_HEADING } from "../../src/report/composer.js"; @@ -345,7 +346,7 @@ describe("deliverIssueReport", () => { const result = await deliverIssueReport(api, SLUG, "body", true, "always"); expect(result).toEqual({ warning: - "could not deliver the private report (HTTP 500). Re-run the workflow, or set " + + "could not deliver the private report (HTTP 500). Re-run, or set " + "private-report: none if it persists", }); }); @@ -357,7 +358,7 @@ describe("deliverIssueReport", () => { expect(result).toEqual({ warning: "could not deliver the private report: the request failed before an HTTP response " + - "arrived. Re-run the workflow, or set private-report: none if it persists", + "arrived. Re-run, or set private-report: none if it persists", }); }); @@ -419,7 +420,7 @@ describe("deliverIssueReport under mode: on-failure", () => { const result = await deliverIssueReport(api, SLUG, "body", false, "on-failure"); expect(result).toEqual({ warning: - "could not deliver the private report (HTTP 500). Re-run the workflow, or set " + + "could not deliver the private report (HTTP 500). Re-run, or set " + "private-report: none if it persists", }); }); @@ -597,7 +598,12 @@ describe("injectMarkerLabel", () => { for (const { doc, expected } of cases) { const result = injectMarkerLabel(doc); expect(result.outcome).toBe(expected as typeof result.outcome); - const verdict = validateSettingsDoc(result.settings, "injected doc", new Set(), silentIo()); + const verdict = validateSettingsDoc( + result.settings, + "injected doc", + SectionSelection.ALL, + silentIo(), + ); expect( verdict.match(() => null, describeProblem), `outcome "${expected}" produced a document validation rejects`, diff --git a/test/sections/contract.test.ts b/test/sections/contract.test.ts index cc14ac0a..782f1d8e 100644 --- a/test/sections/contract.test.ts +++ b/test/sections/contract.test.ts @@ -1201,7 +1201,7 @@ describe("a marked request's failure is rebuilt on the engine's side of the clie { wire: "REST, the client throws", run: () => rest(throwing, true), - thrown: `PATCH /repos/o/r/code-quality/setup failed: ${SECRET_TRANSPORT_WITHHELD}. Check network connectivity from the runner to the GitHub API, then re-run the workflow`, + thrown: `PATCH /repos/o/r/code-quality/setup failed: ${SECRET_TRANSPORT_WITHHELD}. Check network connectivity from the runner to the GitHub API, then re-run`, }, { wire: "GraphQL, the client answers with errors", @@ -1211,7 +1211,7 @@ describe("a marked request's failure is rebuilt on the engine's side of the clie { wire: "GraphQL, the client throws", run: () => graphql(throwing, true), - thrown: `GRAPHQL MarkedWrite failed: ${SECRET_TRANSPORT_WITHHELD}. Check network connectivity from the runner to the GitHub API, then re-run the workflow`, + thrown: `GRAPHQL MarkedWrite failed: ${SECRET_TRANSPORT_WITHHELD}. Check network connectivity from the runner to the GitHub API, then re-run`, }, { wire: "REST, the client answers a rate limit signalled only by its message",