diff --git a/src/engine/execute.ts b/src/engine/execute.ts index 81741078..8ad68351 100644 --- a/src/engine/execute.ts +++ b/src/engine/execute.ts @@ -1,7 +1,7 @@ /** * The ONE place a section's operations touch the API. Operations go through the request helpers so error classification * (a denial vs a hard failure, the hints) matches the reads'; a failure comes back as the value the section loop - * classifies, beside what landed before it. + * classifies, beside what landed before it and the operations it left unattempted. */ import { err, ok, type Result } from "neverthrow"; @@ -10,7 +10,7 @@ import type { GitHubClient } from "../github/api.js"; import { type EndpointDecl, endpointMethod } from "../sections/contract/endpoints.js"; import { type SectionFailure, sectionFailure, thrown } from "../sections/contract/errors.js"; import type { SectionContext, SectionMeta } from "../sections/contract/module.js"; -import type { ExecTools, SectionPlan } from "../sections/contract/plan.js"; +import { driftOf, type ExecTools, type SectionPlan } from "../sections/contract/plan.js"; import { callDeclared, callGraphql, @@ -25,9 +25,14 @@ interface PlanExecutionBase { readonly landed: number; } +/** A failure ends the plan, since a later operation may depend on an earlier one; `unattempted` names each operation after it, in plan order. */ type PlanExecution = | (PlanExecutionBase & { readonly status: "applied" }) - | (PlanExecutionBase & { readonly status: "failed"; readonly failure: SectionFailure }); + | (PlanExecutionBase & { + readonly status: "failed"; + readonly failure: SectionFailure; + readonly unattempted: readonly string[]; + }); type PlannedOp = SectionPlan["ops"][number]; @@ -236,6 +241,19 @@ function changeLines( return ok(typeof lines === "string" ? [lines] : lines); } +/** + * An operation that never ran, in its section's own words: its description, else the drift it would have resolved, + * else (a write that recurs without drift and describes nothing) its role. + */ +function nameOf(op: PlannedOp): string { + if (op.describe !== undefined) { + return op.describe; + } + + const drift = driftOf(op); + return drift.length > 0 ? drift.join("; ") : `the "${op.role}" write`; +} + export async function executePlan( plan: SectionPlan, section: SectionMeta, @@ -250,19 +268,20 @@ export async function executePlan( const changes: string[] = []; const notes: string[] = []; let landed = 0; - const failed = (failure: SectionFailure): PlanExecution => ({ + const failed = (failure: SectionFailure, at: number): PlanExecution => ({ status: "failed", changes, notes, landed, failure, + unattempted: plan.ops.slice(at + 1).map(nameOf), }); - for (const op of plan.ops) { + for (const [at, op] of plan.ops.entries()) { const { exec, resolved } = secretRecorder(tools); try { const requested = await request(run, op, exec, resolved); if (requested.isErr()) { - return failed(requested.error); + return failed(requested.error, at); } if (requested.value.kind === "tolerated") { @@ -273,13 +292,13 @@ export async function executePlan( landed++; const rendered = changeLines(section, op, requested.value.response); if (rendered.isErr()) { - return failed(rendered.error); + return failed(rendered.error, at); } changes.push(...rendered.value); } catch (error) { // The client's own throw on an unmarked request, or a BUG invariant: reported beside what landed. - return failed(thrown(error)); + return failed(thrown(error), at); } } diff --git a/src/engine/orchestrate.ts b/src/engine/orchestrate.ts index c7d9ba27..15d372af 100644 --- a/src/engine/orchestrate.ts +++ b/src/engine/orchestrate.ts @@ -121,6 +121,7 @@ interface Produced { readonly notes: readonly string[]; readonly changes: readonly string[]; readonly landed: number; + readonly unattempted: readonly string[]; } /** A section's plan and execution as a value, beside what it produced on the way. */ @@ -383,7 +384,7 @@ async function runSection( ); } - let produced: Produced = { notes: [], changes: [], landed: 0 }; + let produced: Produced = { notes: [], changes: [], landed: 0, unattempted: [] }; try { const planned = await section.plan(planContext(section, run.api, run.repo), desired); if (planned.isErr()) { @@ -400,7 +401,12 @@ async function runSection( const execution = await executePlan(plan, section, run.api, run.repo, run.tools); const notes = [...plan.notes, ...plan.drift, ...execution.notes]; - produced = { notes, changes: execution.changes, landed: execution.landed }; + produced = { + notes, + changes: execution.changes, + landed: execution.landed, + unattempted: execution.status === "failed" ? execution.unattempted : [], + }; if (execution.status === "failed") { return { step: err(execution.failure), produced }; } @@ -411,6 +417,34 @@ async function runSection( } } +/** A section's line under its key, prefixed once: a section's own lines already lead with the key (`teams: ...`, `repository.enable_git_lfs: ...`, `environments[prod]: ...`). */ +export function underKey(key: SectionKey, line: string): string { + const own = line.startsWith(key) && /^[:.[]/.test(line.slice(key.length)); + return own ? line : `${key}: ${line}`; +} + +/** + * What the stop at a failed operation cost, said after the failure: the count as a warning, each operation the plan + * still held as a log line, and those lines as the rows they add. The stop itself stays, since a later operation can + * depend on the one that failed. + */ +function reportUnattempted(io: Io, key: SectionKey, unattempted: readonly string[]): string[] { + const count = unattempted.length; + if (count === 0) { + return []; + } + + io.annotate( + "warning", + `${key}: ${countNoun(count, "operation", "operations")} after this failure ${agree(count, "was", "were")} not attempted; re-run once it is fixed`, + ); + const rows = unattempted.map((name) => `not attempted - ${name}`); + for (const row of rows) { + io.log(`${key}: ${row}`); + } + return rows; +} + /** Why a denial fails the run whatever the policy: a request landed, or the section is required. */ function denialWhy(landed: number, required: boolean): string { if (landed > 0) { @@ -422,12 +456,15 @@ function denialWhy(landed: number, required: boolean): string { return ""; } -/** A denied section: skipped under warn when nothing landed and the section is not required, else failed, the line saying why the policy did not spare it. */ +/** + * A denied section: skipped under warn when nothing landed and the section is not required, else failed, the line + * saying why the policy did not spare it. A skip covers every operation of the section, so it lists none unattempted. + */ function deniedOutcome( run: SectionRun, key: SectionKey, failure: Denied, - landed: number, + { landed, unattempted }: Produced, before: readonly string[], ): SectionOutcome { const required = run.opts.sections.required.has(key); @@ -444,10 +481,16 @@ function deniedOutcome( const applied = landed > 0 ? "partially applied" : "not applied"; run.io.annotate("error", `${key}: ${applied}${denialWhy(landed, required)} - ${failure.detail}`); - return { key, status: "failed", detail: [...before, failure.detail], httpStatus: failure.status }; + const after = reportUnattempted(run.io, key, unattempted); + return { + key, + status: "failed", + detail: [...before, failure.detail, ...after], + httpStatus: failure.status, + }; } -/** A section that failed: what it produced first (its notes and change lines), then the failure as its row. */ +/** A section that failed: what it produced first (its notes and change lines), then the failure as its row, then what the failure left unattempted. */ function failedOutcome( run: SectionRun, key: SectionKey, @@ -455,28 +498,27 @@ function failedOutcome( produced: Produced, ): SectionOutcome { for (const note of produced.notes) { - run.io.annotate("notice", `${key}: ${note}`); + run.io.annotate("notice", underKey(key, note)); } for (const line of produced.changes) { - run.io.log(`${key}: ${line}`); + run.io.log(underKey(key, line)); } const before = [...produced.notes, ...produced.changes]; if (failure.kind === "permission-denied") { - return deniedOutcome(run, key, failure, produced.landed, before); + return deniedOutcome(run, key, failure, produced, before); } - // failureFor() messages already carry section, cause, and fix; anything else gets the section prefixed. A landed - // request is a real mutation with or without its line, so say so. - const prefixed = failure.message.startsWith(`${key}:`) - ? failure.message - : `${key}: ${failure.message}`; + // failureFor() messages already carry section, cause, and fix, and a refusal leads with its entry; anything else gets + // the section prefixed. A landed request is a real mutation with or without its line, so say so. + const prefixed = underKey(key, failure.message); const annotated = produced.landed > 0 ? `${prefixed} (${countNoun(produced.landed, "request", "requests")} landed before this failure, so the repository is partially applied)` : prefixed; run.io.annotate("error", annotated); - return { key, status: "failed", detail: [...before, annotated] }; + const after = reportUnattempted(run.io, key, produced.unattempted); + return { key, status: "failed", detail: [...before, annotated, ...after] }; } /** A section that changed nothing but left notes (a tolerated 409, a personal-account skip) is NOT "already in the desired state"; the notes are shown instead of claiming no changes were needed. */ @@ -493,7 +535,7 @@ function appliedDetail(changes: string[], notes: string[]): string[] { /** A section that ran: its notes, then its drift lines (check) or change lines (apply) as its row. */ function ranOutcome(io: Io, key: SectionKey, outcome: Outcome): SectionOutcome { for (const note of outcome.notes) { - io.annotate("notice", `${key}: ${note}`); + io.annotate("notice", underKey(key, note)); } if (outcome.check) { @@ -507,7 +549,7 @@ function ranOutcome(io: Io, key: SectionKey, outcome: Outcome): SectionOutcome { } for (const line of outcome.changes) { - io.log(`${key}: ${line}`); + io.log(underKey(key, line)); } return { key, status: "applied", detail: appliedDetail(outcome.changes, outcome.notes) }; } diff --git a/src/engine/secret-refs.ts b/src/engine/secret-refs.ts index 1c58f607..81fc386f 100644 --- a/src/engine/secret-refs.ts +++ b/src/engine/secret-refs.ts @@ -30,8 +30,8 @@ export const RESERVED_REF_PREFIXES = ["INPUT_", "GITHUB_", "ACTIONS_", "RUNNER_" /** * Reads no environment, so check mode and preflight run it without touching secrets; `label` names the owning entry - * (a secret name, a webhook url), never a value. Errors never echo a non-reference value either: a rejected literal, - * or the text around an embedded `$NAME`, may already be a secret. + * (a secret name, a webhook url), never a value. Errors never echo any part of a non-reference value either: a + * rejected literal may already be a secret, and so may what reads as an embedded `$NAME` (the tail of `Pa$$W0RD`). */ export function validateSecretRef( value: string, @@ -53,10 +53,9 @@ export function validateSecretRef( } return ok({ name }); } - const embedded = value.match(EMBEDDED_REFERENCE_RE)?.[0]; - if (embedded !== undefined) { + if (EMBEDDED_REFERENCE_RE.test(value)) { return err( - `${label} embeds ${embedded} without being a whole-value reference; it would otherwise ship verbatim as the secret. Make the entire value a single $NAME reference`, + `${label} embeds a $NAME reference without being a whole-value reference; it would otherwise ship verbatim as the secret. Make the entire value a single $NAME reference`, ); } return err( diff --git a/src/engine/snapshot.ts b/src/engine/snapshot.ts index 85a2cb6c..7a5585f8 100644 --- a/src/engine/snapshot.ts +++ b/src/engine/snapshot.ts @@ -25,7 +25,7 @@ import { type OnMissingPermission, snapshotContext } from "../sections/contract/ import { SECTIONS } from "../sections/registry.js"; import type { MustBeNever } from "../types.js"; import { canonicalDocument, compareByCodePoint, renderCanonicalYaml } from "./canonical.js"; -import { type ValidatedSettings, validateSettingsDoc } from "./orchestrate.js"; +import { underKey, type ValidatedSettings, validateSettingsDoc } from "./orchestrate.js"; import type { RunOutcome } from "./outcome.js"; import { SectionSelection } from "./section-selection.js"; @@ -77,12 +77,6 @@ type SectionRead = /** What a section without live state says for itself in the outcome and the file header. */ const NOTHING_TO_DECLARE = "nothing exists on the repository, so the section is omitted"; -/** A line under its section's key, prefixed once: a section's own notes already lead with the key. */ -function underKey(key: SectionKey, line: string): string { - const own = line.startsWith(key) && /^[:.[]/.test(line.slice(key.length)); - return own ? line : `${key}: ${line}`; -} - /** * The client as one section's snapshot sees it, reporting whether a GET on `read`'s route * answered 404 (the observation the concealed-absence note is keyed on). Null passes through. @@ -135,9 +129,7 @@ function failedSnapshot( return { key, status: "failed", detail: [failure.detail] }; } - const prefixed = failure.message.startsWith(`${key}:`) - ? failure.message - : `${key}: ${failure.message}`; + const prefixed = underKey(key, failure.message); io.annotate("error", prefixed); return { key, status: "failed", detail: [prefixed] }; } diff --git a/test/engine/execute.test.ts b/test/engine/execute.test.ts index 8d3bd518..997c3ea5 100644 --- a/test/engine/execute.test.ts +++ b/test/engine/execute.test.ts @@ -183,6 +183,52 @@ describe("executePlan", () => { ); }); + test("a failed execution names every operation the stop left unattempted, in plan order, and sends none of them", async () => { + // Without the names a refused entry ends the plan and the later entries' writes vanish without a word. Each is named by its description, else by the + // drift it would have resolved, else (a write that recurs without drift and describes nothing) by its role. + const api = new MockApi({}, { unroutedMutations: "succeed" }); + const plan: SectionPlan = { + ops: [ + { + role: "create", + before: () => err(sectionFailure("refused", "labels[first]: not applied")), + drift: ["labels[first]: missing"], + change: 'created label "first"', + }, + { + role: "create", + describe: 'creating label "second"', + drift: ["labels[second]: missing"], + change: 'created label "second"', + }, + { + role: "write", + variables: {}, + drift: ["toggle off", "toggle unarmed"], + change: "flipped the toggle", + }, + { + role: "resend", + params: { name: "x" }, + drift: { unverifiable: "GitHub never echoes the value back", lines: [] }, + change: "re-sent", + }, + ], + notes: [], + drift: [], + }; + const execution = await executePlan(plan, SECTION, api, REPO, TOOLS); + expect(execution).toEqual({ + status: "failed", + changes: [], + notes: [], + landed: 0, + failure: { kind: "refused", message: "labels[first]: not applied" }, + unattempted: ['creating label "second"', "toggle off; toggle unarmed", 'the "resend" write'], + }); + expect(api.calls).toEqual([]); + }); + test("an operation's describe label leads the failure message, REST and GraphQL alike", async () => { const api = new MockApi({ "POST /repos/o/r/labels": { error: REJECTED }, @@ -268,6 +314,7 @@ describe("executePlan", () => { /BUG: labels: operation "create" rendered no change line for a request that landed/, ), }, + unattempted: [], }); }); @@ -291,6 +338,7 @@ describe("executePlan", () => { notes: [], landed: 1, failure: { kind: "unverified", message: "the write did not take" }, + unattempted: [], }); expect(api.mutations().map((m) => `${m.method} ${m.path}`)).toEqual(["POST /repos/o/r/labels"]); expect(captured).toBe(false); @@ -333,6 +381,7 @@ describe("executePlan", () => { notes: [], landed: 2, failure: { kind: "live-shape", message: "the response carried no node id" }, + unattempted: ["labels[next]: missing"], }); expect(api.mutations().map((m) => `${m.method} ${m.path}`)).toEqual([ "POST /repos/o/r/labels", @@ -396,6 +445,7 @@ describe("executePlan", () => { kind: "thrown", message: `BUG: labels: the ${hook} of operation "create" returned a promise; it must be synchronous`, }, + unattempted: [], }); }, ); @@ -459,6 +509,7 @@ describe("executePlan", () => { notes: [], landed: 1, failure: { kind: "refused", message: "the actor does not exist" }, + unattempted: [], }); expect(seen).toEqual(['[{"name":"live"}]']); expect(api.calls.map((c) => `${c.method} ${c.path}`)).toEqual([ @@ -514,6 +565,7 @@ describe("executePlan", () => { message: "labels: POST answered 409 Conflict; a label run is in progress, re-run after it finishes", }, + unattempted: [], }); }); @@ -559,6 +611,7 @@ describe("executePlan", () => { /^labels: POST \/repos\/o\/r\/labels: 422 Validation Failed\. /, ), }, + unattempted: [], }); const denied = await executePlan( planOf({ role: "create", tolerate: { outcome } }), @@ -579,6 +632,7 @@ describe("executePlan", () => { ), status: 403, }), + unattempted: [], }); // The control: a tolerated operation that succeeds records its line. const succeeded = await executePlan( diff --git a/test/engine/orchestrate.test.ts b/test/engine/orchestrate.test.ts index 2f207a6c..c4a53729 100644 --- a/test/engine/orchestrate.test.ts +++ b/test/engine/orchestrate.test.ts @@ -10,6 +10,7 @@ import { validateSettingsDoc, } from "../../src/engine/orchestrate.js"; import { SectionSelection } from "../../src/engine/section-selection.js"; +import { publicDetail, redactedChannel } from "../../src/flows/redact.js"; import { silentIo } from "../../src/io.js"; import { describeProblem, @@ -19,6 +20,7 @@ import { unknownSectionsIssue, } from "../../src/problem.js"; import { SECTION_KEYS, type SettingsFile } from "../../src/schema.js"; +import { sectionFailure } from "../../src/sections/contract/errors.js"; import type { SectionModule } from "../../src/sections/contract/module.js"; import type { SectionPlan } from "../../src/sections/contract/plan.js"; import { interactionLimitsSection } from "../../src/sections/interaction_limits/index.js"; @@ -668,9 +670,7 @@ describe("runForRepo plan sections", () => { }, ]); expect(annotations).toEqual([ - expect.stringMatching( - /^notice: workflows: workflows\[missing\.yml\]: declared in the settings file/, - ), + expect.stringMatching(/^notice: workflows\[missing\.yml\]: declared in the settings file/), ]); }); @@ -713,7 +713,7 @@ describe("runForRepo plan sections", () => { expect(api.mutations()).toHaveLength(2); expect(logs).toEqual(['workflows: disabled workflow ".github/workflows/ci.yml"']); expect(annotations).toEqual([ - expect.stringMatching(/^notice: workflows: workflows\[missing\.yml\]/), + expect.stringMatching(/^notice: workflows\[missing\.yml\]: declared in the settings file/), expect.stringContaining("PUT /repos/o/r/actions/workflows/2/enable: 422"), ]); expect(result.outcomes).toEqual([ @@ -737,16 +737,13 @@ describe("runForRepo plan sections", () => { afterEach(() => { stubbed?.mockRestore(); }); - const stub = (section: SectionModule, ...ops: SectionPlan["ops"]) => { + const stubPlan = (section: SectionModule, plan: SectionPlan) => { // A restored spy no longer intercepts, so each test arms its own. - stubbed = spyOn(section, "plan").mockResolvedValue( - ok({ - ops: ops as never, - notes: [], - drift: [], - }), - ); + stubbed?.mockRestore(); + stubbed = spyOn(section, "plan").mockResolvedValue(ok(plan as never)); }; + const stub = (section: SectionModule, ...ops: SectionPlan["ops"]) => + stubPlan(section, { ops, notes: [], drift: [] }); const disabling = (workflowId: string): SectionPlan["ops"][number] => ({ role: "disable", params: { workflow_id: workflowId }, @@ -871,6 +868,143 @@ describe("runForRepo plan sections", () => { { key: "workflows", status: "failed", detail: [partial.slice("error: ".length)] }, ]); }); + + const refusing = (workflowId: string): SectionPlan["ops"][number] => ({ + ...disabling(workflowId), + before: () => + err(sectionFailure("refused", `workflows[${workflowId}]: not applied - refused`)), + }); + const accepting = () => + new MockApi({ [WORKFLOWS_LIST]: { data: live } }, { unroutedMutations: "succeed" }); + + test("a section's line that already leads with its key prints under that key once on every path that prefixes; any other line is prefixed", async () => { + // A section's own lines lead with its key (`teams: ...`, `repository.enable_git_lfs: ...`, `workflows[ci.yml]: ...`), so prefixing them again + // names the section twice. A longer word that merely starts with the key is not the key. + const NOTES = [ + "workflows: the whole section", + "workflows.state: a dotted key", + "workflows[ci.yml]: an entry", + "workflows_extra: another word", + "plain words", + ]; + const ONCE = [ + "annotate notice: workflows: the whole section", + "annotate notice: workflows.state: a dotted key", + "annotate notice: workflows[ci.yml]: an entry", + "annotate notice: workflows: workflows_extra: another word", + "annotate notice: workflows: plain words", + ]; + const keyed = { ...disabling("1"), change: "workflows[1]: disabled" }; + for (const [mode, lines] of [ + ["check", ["log: drift: workflows[1]: drifted"]], + ["apply", ["log: workflows[1]: disabled"]], + ] as const) { + stubPlan(workflowsSection, { ops: [keyed], notes: NOTES, drift: [] }); + const { io, events } = captureIo(); + await runForRepo(accepting(), opts({ mode, settings: drifting }), io); + expect(events).toEqual([...ONCE, ...lines]); + } + // Beside a failure: the notes, the change that landed first, and the failure's own line. + stubPlan(workflowsSection, { ops: [keyed, refusing("2")], notes: NOTES, drift: [] }); + const { io, events } = captureIo(); + const result = await runForRepo(accepting(), opts({ settings: drifting }), io); + expect(events).toEqual([ + ...ONCE, + "log: workflows[1]: disabled", + "annotate error: workflows[2]: not applied - refused (1 request landed before this failure, so the repository is partially applied)", + ]); + expect(result.outcomes).toEqual([ + { + key: "workflows", + status: "failed", + detail: [ + ...NOTES, + "workflows[1]: disabled", + "workflows[2]: not applied - refused (1 request landed before this failure, so the repository is partially applied)", + ], + }, + ]); + }); + + test("a failure that stops a plan says what it left unattempted: the count as a warning, each operation as a log line and a row", async () => { + // Without it a refused entry ends the section and every later entry's write is dropped without a word. + stub( + workflowsSection, + refusing("1"), + { ...disabling("2"), describe: "disabling workflow 2" }, + disabling("3"), + ); + const api = accepting(); + const { io, events } = captureIo(); + const result = await runForRepo(api, opts({ settings: drifting }), io); + expect(result.result).toBe("failed"); + // The stop stays: a later operation may depend on the one that failed, so none is sent. + expect(api.mutations()).toEqual([]); + expect(events).toEqual([ + "annotate error: workflows[1]: not applied - refused", + "annotate warning: workflows: 2 operations after this failure were not attempted; re-run once it is fixed", + "log: workflows: not attempted - disabling workflow 2", + "log: workflows: not attempted - workflows[3]: drifted", + ]); + expect(result.outcomes).toEqual([ + { + key: "workflows", + status: "failed", + detail: [ + "workflows[1]: not applied - refused", + "not attempted - disabling workflow 2", + "not attempted - workflows[3]: drifted", + ], + }, + ]); + }); + + test("a denial that fails the section says what it left unattempted too; a section skipped under warn lists nothing, since the skip covers every operation", async () => { + const denying = () => + new MockApi({ + [WORKFLOWS_LIST]: { data: live }, + "PUT /repos/o/r/actions/workflows/1/disable": { + error: { status: 403, message: "Resource not accessible", body: "" }, + }, + }); + stub(workflowsSection, disabling("1"), disabling("2")); + const failed = captureIo(); + const refused = await runForRepo(denying(), opts({ settings: drifting }), failed.io); + expect(refused.outcomes.map((o) => o.status)).toEqual(["failed"]); + expect(failed.events).toEqual([ + expect.stringMatching( + /^annotate error: workflows: not applied - the token was denied PUT /, + ), + "annotate warning: workflows: 1 operation after this failure was not attempted; re-run once it is fixed", + "log: workflows: not attempted - workflows[2]: drifted", + ]); + expect(refused.outcomes[0]?.detail.at(-1)).toBe("not attempted - workflows[2]: drifted"); + const skipped = captureIo(); + const softened = await runForRepo( + denying(), + opts({ onMissingPermission: "warn", settings: drifting }), + skipped.io, + ); + expect(softened.outcomes.map((o) => o.status)).toEqual(["skipped"]); + expect(skipped.events).toEqual([ + expect.stringMatching(/^annotate warning: workflows: skipped - the token was denied PUT /), + ]); + }); + + test("a redacted target prints no unattempted line in the clear: each goes through the run's own sink, and its rows seal with the rest", async () => { + stub(workflowsSection, refusing("1"), { + ...disabling("2"), + describe: "disabling workflow CANARY", + }); + const shown = captureIo(); + const channel = redactedChannel(shown.io, "o/r", "private repository #1"); + const result = await runForRepo(accepting(), opts({ settings: drifting }), channel.io); + // The control: the line exists, in the row and in the sealed transcript the private report is built from. + expect(result.outcomes[0]?.detail).toContain("not attempted - disabling workflow CANARY"); + const sealed = channel.close({ outcomes: result.outcomes }); + expect(shown.events).toEqual([]); + expect(JSON.stringify(publicDetail(sealed))).not.toMatch(/CANARY|not attempted/); + }); }); test("a denial after an operation landed fails the run even under the warn policy", async () => { diff --git a/test/engine/secret-refs.test.ts b/test/engine/secret-refs.test.ts index f7a78455..176e8f9e 100644 --- a/test/engine/secret-refs.test.ts +++ b/test/engine/secret-refs.test.ts @@ -50,12 +50,18 @@ describe("validateSecretRef (syntax phase, never reads the environment)", () => "operator", { name: "INPUTX" }, ], - // The surrounding text may itself be half a secret; it must not be echoed. + // What reads as an embedded reference may be part of a literal secret, so the error names the entry and the rule and echoes no part of the value. [ - "a value that embeds $NAME without being one is rejected, naming the fragment", + "a value that embeds $NAME without being one is rejected by the rule, echoing no part of the value", "prefix-$TOKEN", "operator", - { says: ["$TOKEN"], never: ["prefix-"] }, + { says: [LABEL, "a $NAME reference"], never: ["prefix-", "TOKEN"] }, + ], + [ + "a literal whose `$` run reads as a reference is rejected by the same rule, echoing no part of it", + "Pa$$W0RD_SECRET9", + "operator", + { says: [LABEL, "a $NAME reference"], never: ["W0RD", "SECRET9", "Pa$"] }, ], [ "a literal value is rejected and never echoed", diff --git a/test/engine/snapshot.test.ts b/test/engine/snapshot.test.ts index e6e44fe8..1f1fe54f 100644 --- a/test/engine/snapshot.test.ts +++ b/test/engine/snapshot.test.ts @@ -5,7 +5,7 @@ */ import { describe, expect, spyOn, test } from "bun:test"; -import { ok } from "neverthrow"; +import { err, ok } from "neverthrow"; import { parse as parseYaml } from "yaml"; import { z } from "zod"; import { SectionSelection } from "../../src/engine/section-selection.js"; @@ -16,6 +16,7 @@ import { snapshotRepository, } from "../../src/engine/snapshot.js"; import type { GitHubClient } from "../../src/github/api.js"; +import { sectionFailure } from "../../src/sections/contract/errors.js"; import type { SectionMeta } from "../../src/sections/contract/module.js"; import { endpointPermission, sectionOperations } from "../../src/sections/contract/operations.js"; import { labelsSection } from "../../src/sections/labels/index.js"; @@ -357,6 +358,26 @@ describe("snapshotRepository", () => { } }); + test("a failed read whose message already leads with the section's key is reported under that key once", async () => { + // A section names the entry it could not read (`labels[bug]: ...`); prefixing that names the section twice. + const message = "labels[bug]: GitHub reported no color, so it cannot be read back"; + const stubbed = spyOn(labelsSection, "snapshot").mockResolvedValue( + err(sectionFailure("live-shape", message)), + ); + try { + const { io, annotations } = captureIo(); + const result = await snapshotRepository(registryFake(LIVE), opts(), io); + expect(result.outcomes.find((o) => o.key === "labels")).toEqual({ + key: "labels", + status: "failed", + detail: [message], + }); + expect(annotations).toContain(`error: ${message}`); + } finally { + stubbed.mockRestore(); + } + }); + test("a section returning a value its own schema rejects fails the run with the validation error, never a document", async () => { const stubbed = spyOn(labelsSection, "snapshot").mockResolvedValue( ok({ diff --git a/test/engine/validate.test.ts b/test/engine/validate.test.ts index d7e39eae..02445b72 100644 --- a/test/engine/validate.test.ts +++ b/test/engine/validate.test.ts @@ -318,7 +318,7 @@ describe("secret values are judged at validation, under the document's provenanc expect( issuesOf({ webhooks: [{ config: { url: "https://x.test/h", secret: "prefix-$TOKEN" } }] }), ).toEqual([ - 'webhooks: the webhook "https://x.test/h" config.secret embeds $TOKEN without being a whole-value reference; it would otherwise ship verbatim as the secret. Make the entire value a single $NAME reference', + 'webhooks: the webhook "https://x.test/h" config.secret embeds a $NAME reference without being a whole-value reference; it would otherwise ship verbatim as the secret. Make the entire value a single $NAME reference', ]); }); diff --git a/test/sections/branches/branches.test.ts b/test/sections/branches/branches.test.ts index bfd5887e..c9649c24 100644 --- a/test/sections/branches/branches.test.ts +++ b/test/sections/branches/branches.test.ts @@ -1172,6 +1172,10 @@ describe("branches GraphQL-routed keys", () => { notes: [], landed: 0, failure: { kind: "live-shape", message: GHOST_ACTOR_ERROR }, + unattempted: [ + 'replacing protection for branch "dev"', + 'setting the GraphQL-only protection fields of branch "dev"', + ], }); expect(api.mutations()).toHaveLength(0); });