From 1c4a8d17008539b10d7978338a9e8a2aee05f417 Mon Sep 17 00:00:00 2001 From: Vivswan Shah <58091053+Vivswan@users.noreply.github.com> Date: Wed, 11 Nov 2026 20:33:51 -0500 Subject: [PATCH] fix(engine): name a notice's section once, and report what a refused entry left unattempted A section's own lines already lead with its key, and the section loop prefixed them again, so a run printed "teams: teams: ...", "repository: repository.enable_git_lfs: ...", and "environments: environments[alpha]: not applied - ...". The rule that prefixes a line once was private to the snapshot flow. It now sits beside the section-outcome functions as underKey, and every site that prefixes a section's line calls it: notes, change lines, and failure lines, in apply, check, and snapshot. The first failed operation ends its section's plan, since a later operation can depend on an earlier one, and nothing said what that cost: a refused environment entry dropped every later entry's writes without a word. The stop stays. The executor now returns the operations it never reached, each named by its own description, else by the drift it would have resolved, else by its role. The failed outcome prints the count as a warning, then one log line and one summary row per operation. A section skipped under on-missing-permission: warn lists nothing, because the skip covers every operation of the section. The embedded-reference error quoted the text it matched, which in a literal such as "Pa$$W0RD_SECRET9" is part of the secret. It now names the entry and the rule and echoes no part of the value. --- src/engine/execute.ts | 35 ++++-- src/engine/orchestrate.ts | 76 +++++++++--- src/engine/secret-refs.ts | 9 +- src/engine/snapshot.ts | 12 +- test/engine/execute.test.ts | 54 ++++++++ test/engine/orchestrate.test.ts | 158 ++++++++++++++++++++++-- test/engine/secret-refs.test.ts | 12 +- test/engine/snapshot.test.ts | 23 +++- test/engine/validate.test.ts | 2 +- test/sections/branches/branches.test.ts | 4 + 10 files changed, 328 insertions(+), 57 deletions(-) diff --git a/src/engine/execute.ts b/src/engine/execute.ts index 81741078a..8ad683515 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 c7d9ba273..15d372aff 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 1c58f607a..81fc386f1 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 85a2cb6c7..7a5585f8c 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 8d3bd5182..997c3ea54 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 2f207a6ca..c4a537299 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 f7a784558..176e8f9e6 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 e6e44fe8e..1f1fe54fe 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 d7e39eaed..02445b72d 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 bfd5887e3..c9649c245 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); });