From 86cff73eabe8e215b972fb179fe07d82fb92969c Mon Sep 17 00:00:00 2001 From: James Wiesebron Date: Mon, 3 Aug 2026 18:07:15 -0700 Subject: [PATCH 1/2] [fix-workflows-skip-locks] fix-workflows: stop reformatting generated workflows The YAML fixer already skipped *.lock.yml, but the separate oxfmt step in action.yml (and its cli.ts equivalent) globbed .github/workflows/**/*.yml with no exclusions, so it reformatted the compiled agentic workflows the fixer had deliberately left alone. The 'this file needs updating' annotation that follows then points a maintainer at compiler output, and applying it desyncs a committed lock from gh aw compile. Both invocations now exclude the generator-owned workflows, and the fixer's skip list becomes a named, tested predicate (isGeneratedWorkflow) so the three call sites cannot drift apart again. agentics-maintenance.yml joins *.lock.yml in that set: gh-aw regenerates it unconditionally and it is not named *.lock.yml, so nothing skipped it, and it took 6 checkout-followed-by-setup violations and 12 runs-on rewrites on a file no human can fix. Negated globs rather than ignorePatterns in .oxfmtrc.json: that config lives in the action's own directory rather than the repo being formatted, and its patterns do not resolve against the working directory (verified -- the lock was still rewritten). Observed on Khan/agent-settings#48, the first repo to run this action over a directory containing compiled agentic workflows. --- .changeset/fix-workflows-skip-generated.md | 27 +++++++++++++ actions/fix-workflows/action.yml | 11 +++++- actions/fix-workflows/cli.ts | 15 +++++++- .../fix-workflows/generated-workflows.test.ts | 38 +++++++++++++++++++ actions/fix-workflows/index.ts | 27 ++++++++++++- 5 files changed, 114 insertions(+), 4 deletions(-) create mode 100644 .changeset/fix-workflows-skip-generated.md create mode 100644 actions/fix-workflows/generated-workflows.test.ts diff --git a/.changeset/fix-workflows-skip-generated.md b/.changeset/fix-workflows-skip-generated.md new file mode 100644 index 00000000..c2fd8859 --- /dev/null +++ b/.changeset/fix-workflows-skip-generated.md @@ -0,0 +1,27 @@ +--- +"fix-workflows": minor +--- + +fix-workflows: stop reformatting generated workflows + +The YAML fixer already skipped `*.lock.yml`, but the separate `oxfmt` step in +`action.yml` (and its `cli.ts` equivalent) globbed `.github/workflows/**/*.yml` +with no exclusions, so it reformatted the compiled agentic workflows the fixer had +deliberately left alone. The "this file needs updating" annotation that follows +then points a maintainer at compiler output, and applying it desyncs a committed +lock from `gh aw compile`. + +Both invocations now exclude the generator-owned workflows via negated globs, and +the fixer's own skip list is a named, tested predicate (`isGeneratedWorkflow`) so +the three call sites cannot drift apart again. `agentics-maintenance.yml` joins +`*.lock.yml` in that set: gh-aw regenerates it unconditionally and it is not named +`*.lock.yml`, so nothing was skipping it — it took 6 checkout-followed-by-setup +violations and 12 `runs-on` rewrites on a file no human can fix. + +Negated globs rather than `ignorePatterns` in `.oxfmtrc.json`, because that config +lives in the action's own directory rather than the repo being formatted and its +patterns do not resolve against the working directory (verified: the lock was still +rewritten). + +Observed on Khan/agent-settings#48, which is the first repo to run this action over +a directory containing compiled agentic workflows. diff --git a/actions/fix-workflows/action.yml b/actions/fix-workflows/action.yml index 8759ac88..a3eef778 100644 --- a/actions/fix-workflows/action.yml +++ b/actions/fix-workflows/action.yml @@ -36,12 +36,21 @@ runs: core.setFailed(err.message) } + # The negations must match `isGeneratedWorkflow` in index.ts. Without them + # this step reformats compiler output that the YAML fixer above correctly + # skips, which desyncs a committed lock from `gh aw compile` and makes the + # next step annotate a generated file with a fix nobody should apply. + # `ignorePatterns` in .oxfmtrc.json cannot do this: the config lives in the + # action's directory, not the repo being formatted, and its patterns do not + # resolve against the working directory. - name: Format YAML files with oxfmt shell: bash run: | npx --yes oxfmt@0.44.0 --write \ --config "${{ github.action_path }}/.oxfmtrc.json" \ - ".github/workflows/**/*.yml" + ".github/workflows/**/*.yml" \ + '!.github/workflows/*.lock.yml' \ + '!.github/workflows/agentics-maintenance.yml' - name: Show fix instructions shell: bash diff --git a/actions/fix-workflows/cli.ts b/actions/fix-workflows/cli.ts index e961b630..bbb23e09 100644 --- a/actions/fix-workflows/cli.ts +++ b/actions/fix-workflows/cli.ts @@ -44,13 +44,24 @@ const core = { fixWorkflows({core, fixRunsOn, setupAction}) .then(() => { - // Format workflow files with oxfmt after fixing lint violations. + // Format workflow files with oxfmt after fixing lint violations, skipping + // the generator-owned ones for the same reason the fixer does (see + // isGeneratedWorkflow in index.ts, and keep these globs in step with it and + // with action.yml). Negated globs rather than .oxfmtrc.json ignorePatterns: + // the config lives beside this script, not in the repo being formatted, and + // its patterns do not resolve against the working directory. const configPath = path.join(__dirname, ".oxfmtrc.json"); + const skip = [ + "!.github/workflows/*.lock.yml", + "!.github/workflows/agentics-maintenance.yml", + ] + .map((glob) => JSON.stringify(glob)) + .join(" "); console.log("Formatting workflow files with oxfmt..."); // eslint-disable-line no-console execSync( `npx --yes oxfmt@0.44.0 --write --config ${JSON.stringify( configPath, - )} ".github/workflows/**/*.yml"`, + )} ".github/workflows/**/*.yml" ${skip}`, {stdio: "inherit"}, ); }) diff --git a/actions/fix-workflows/generated-workflows.test.ts b/actions/fix-workflows/generated-workflows.test.ts new file mode 100644 index 00000000..f1d0fb57 --- /dev/null +++ b/actions/fix-workflows/generated-workflows.test.ts @@ -0,0 +1,38 @@ +import {describe, expect, it} from "vitest"; + +import {isGeneratedWorkflow} from "./index.ts"; + +/** + * Which `.github/workflows` entries a generator owns, split from index.test.ts to + * keep that file inside its 1000-line cap. + * + * This predicate is the single source of truth for a set that is duplicated as + * negated globs in action.yml and cli.ts. It exists because those three places + * diverged once: the YAML fixer skipped `*.lock.yml` while the oxfmt step did not. + */ + +describe("isGeneratedWorkflow", () => { + // These files are compiler output. Fixing them is worse than useless: the fix + // is overwritten on the next `gh aw compile`, and the "this file needs + // updating" annotation invites a maintainer to desync a committed lock from + // its compiler. The oxfmt globs in action.yml and cli.ts must exclude exactly + // this set -- they diverged once, and only the formatter step touched locks. + it("skips compiled agentic workflow locks", () => { + expect(isGeneratedWorkflow("review.lock.yml")).toBe(true); + expect(isGeneratedWorkflow("autofix.lock.yml")).toBe(true); + }); + + it("skips gh-aw's maintenance workflow, which is not named *.lock.yml", () => { + expect(isGeneratedWorkflow("agentics-maintenance.yml")).toBe(true); + }); + + it("still checks hand-written workflows, including lookalikes", () => { + expect(isGeneratedWorkflow("node-ci.yml")).toBe(false); + expect(isGeneratedWorkflow("validate-workflows.yml")).toBe(false); + // Not a lock: the suffix test is `.lock.yml`, not `lock` anywhere. + expect(isGeneratedWorkflow("lock-threads.yml")).toBe(false); + expect(isGeneratedWorkflow("agentics-maintenance-notes.yml")).toBe( + false, + ); + }); +}); diff --git a/actions/fix-workflows/index.ts b/actions/fix-workflows/index.ts index 7a45c40d..2654653c 100644 --- a/actions/fix-workflows/index.ts +++ b/actions/fix-workflows/index.ts @@ -178,6 +178,31 @@ export function stepIgnoresSetup(step: YAMLMap): boolean { // File discovery // --------------------------------------------------------------------------- +/** + * Workflow files this action must not touch because a generator owns them: the + * fix would be overwritten on the next `gh aw compile`, and worse, a maintainer + * who follows our own "this file needs updating" hint desyncs the committed file + * from its compiler. + * + * `*.lock.yml` is the compiled agentic workflow; `agentics-maintenance.yml` is + * gh-aw's scheduled housekeeping workflow, which is regenerated unconditionally + * and (unhelpfully) is not named `*.lock.yml`. + * + * The general rule would be "skip anything `.gitattributes` marks + * `linguist-generated`", which would need no updating as generators change. + * That is a larger change than this list and is left for when a third generated + * workflow shows up. + */ +export const GENERATED_WORKFLOWS = { + suffixes: [".lock.yml"], + names: ["agentics-maintenance.yml"], +}; + +/** Whether a `.github/workflows` entry is generator-owned (see above). */ +export const isGeneratedWorkflow = (name: string): boolean => + GENERATED_WORKFLOWS.suffixes.some((suffix) => name.endsWith(suffix)) || + GENERATED_WORKFLOWS.names.includes(name); + function getFilesToCheck(): string[] { const files: string[] = []; @@ -186,7 +211,7 @@ function getFilesToCheck(): string[] { if ( entry.isFile() && (entry.name.endsWith(".yml") || entry.name.endsWith(".yaml")) && - !entry.name.endsWith(".lock.yml") + !isGeneratedWorkflow(entry.name) ) { files.push(path.join(".github", "workflows", entry.name)); } From 7246edc13f0c9da4af9bac560a7fbc52b539d299 Mon Sep 17 00:00:00 2001 From: James Wiesebron Date: Tue, 4 Aug 2026 10:34:37 -0700 Subject: [PATCH 2/2] [fix-workflows-skip-locks] fix-workflows: derive the oxfmt skip globs from GENERATED_WORKFLOWS Review feedback on #321. The predicate governed only the fixer; both oxfmt invocations still hand-copied the set, so the "these three cannot drift apart" claim rested on the same diligence that let them drift in the first place. cli.ts now builds its negations from generatedWorkflowSkipGlobs(), so it cannot fall behind a fourth entry. action.yml genuinely cannot import the module, so a test pins its literals to that function instead; both drift shapes (a dropped negation, a stale non-recursive one) were checked to fail it. The negations are recursive now, matching the positive glob they subtract from. Verified against the pinned oxfmt@0.44.0 that `**/` also matches files directly in .github/workflows, so one pattern covers both depths: over a fixture with a top-level lock, a nested lock, agentics-maintenance.yml and one hand-written workflow, oxfmt reports 1 file and no generated checksum changes, against 4 files and 3 rewrites with the negations removed. cli.ts end-to-end on the same fixture leaves all three generated files byte-identical. isGeneratedWorkflow becomes a function declaration, matching every other exported callable in the module. The GENERATED_WORKFLOWS comment no longer points at .gitattributes parsing as the deferred general rule: gh-aw stamps DO NOT EDIT into the first line of both files it emits, which is far cheaper to detect. It records why that is not a drop-in improvement either -- a glob cannot read a header, so teaching only the fixer to sniff would put fixer and formatter back out of step, which is the bug this list exists to close. A content rule has to reach both sites at once. --- .changeset/fix-workflows-skip-generated.md | 17 ++++-- actions/fix-workflows/action.yml | 21 ++++--- actions/fix-workflows/cli.ts | 18 +++--- .../fix-workflows/generated-workflows.test.ts | 55 +++++++++++++++++-- actions/fix-workflows/index.ts | 50 ++++++++++++++--- 5 files changed, 124 insertions(+), 37 deletions(-) diff --git a/.changeset/fix-workflows-skip-generated.md b/.changeset/fix-workflows-skip-generated.md index c2fd8859..2b031f1e 100644 --- a/.changeset/fix-workflows-skip-generated.md +++ b/.changeset/fix-workflows-skip-generated.md @@ -11,12 +11,17 @@ deliberately left alone. The "this file needs updating" annotation that follows then points a maintainer at compiler output, and applying it desyncs a committed lock from `gh aw compile`. -Both invocations now exclude the generator-owned workflows via negated globs, and -the fixer's own skip list is a named, tested predicate (`isGeneratedWorkflow`) so -the three call sites cannot drift apart again. `agentics-maintenance.yml` joins -`*.lock.yml` in that set: gh-aw regenerates it unconditionally and it is not named -`*.lock.yml`, so nothing was skipping it — it took 6 checkout-followed-by-setup -violations and 12 `runs-on` rewrites on a file no human can fix. +Both invocations now exclude the generator-owned workflows via negated globs. One +list (`GENERATED_WORKFLOWS`) drives all three sites: the fixer calls +`isGeneratedWorkflow`, `cli.ts` derives its globs from +`generatedWorkflowSkipGlobs()`, and `action.yml`, whose bash step cannot import +either, has its hand-copied literals pinned to that function by a test. The +negations are recursive, matching the positive glob they subtract from. + +`agentics-maintenance.yml` joins `*.lock.yml` in that set: gh-aw regenerates it +unconditionally and it is not named `*.lock.yml`, so nothing was skipping it; it +took 6 checkout-followed-by-setup violations and 12 `runs-on` rewrites on a file no +human can fix. Negated globs rather than `ignorePatterns` in `.oxfmtrc.json`, because that config lives in the action's own directory rather than the repo being formatted and its diff --git a/actions/fix-workflows/action.yml b/actions/fix-workflows/action.yml index a3eef778..56fac936 100644 --- a/actions/fix-workflows/action.yml +++ b/actions/fix-workflows/action.yml @@ -36,21 +36,24 @@ runs: core.setFailed(err.message) } - # The negations must match `isGeneratedWorkflow` in index.ts. Without them - # this step reformats compiler output that the YAML fixer above correctly - # skips, which desyncs a committed lock from `gh aw compile` and makes the - # next step annotate a generated file with a fix nobody should apply. - # `ignorePatterns` in .oxfmtrc.json cannot do this: the config lives in the - # action's directory, not the repo being formatted, and its patterns do not - # resolve against the working directory. + # Without the negations this step reformats compiler output that the YAML + # fixer above correctly skips, which desyncs a committed lock from `gh aw + # compile` and makes the next step annotate a generated file with a fix + # nobody should apply. `ignorePatterns` in .oxfmtrc.json cannot do this: the + # config lives in the action's directory, not the repo being formatted, and + # its patterns do not resolve against the working directory. + # + # A bash step cannot import the TypeScript, so these literals are a copy of + # `generatedWorkflowSkipGlobs()` in index.ts; generated-workflows.test.ts + # fails if they drift from it. - name: Format YAML files with oxfmt shell: bash run: | npx --yes oxfmt@0.44.0 --write \ --config "${{ github.action_path }}/.oxfmtrc.json" \ ".github/workflows/**/*.yml" \ - '!.github/workflows/*.lock.yml' \ - '!.github/workflows/agentics-maintenance.yml' + '!.github/workflows/**/*.lock.yml' \ + '!.github/workflows/**/agentics-maintenance.yml' - name: Show fix instructions shell: bash diff --git a/actions/fix-workflows/cli.ts b/actions/fix-workflows/cli.ts index bbb23e09..1ee80a0d 100644 --- a/actions/fix-workflows/cli.ts +++ b/actions/fix-workflows/cli.ts @@ -12,7 +12,10 @@ import {execSync} from "node:child_process"; import * as path from "node:path"; -import fixWorkflows, {DEFAULT_SETUP_ACTION} from "./index"; +import fixWorkflows, { + DEFAULT_SETUP_ACTION, + generatedWorkflowSkipGlobs, +} from "./index"; function parseArgs(argv: string[]): { fixRunsOn: boolean; @@ -46,15 +49,12 @@ fixWorkflows({core, fixRunsOn, setupAction}) .then(() => { // Format workflow files with oxfmt after fixing lint violations, skipping // the generator-owned ones for the same reason the fixer does (see - // isGeneratedWorkflow in index.ts, and keep these globs in step with it and - // with action.yml). Negated globs rather than .oxfmtrc.json ignorePatterns: - // the config lives beside this script, not in the repo being formatted, and - // its patterns do not resolve against the working directory. + // GENERATED_WORKFLOWS in index.ts). Negated globs rather than + // .oxfmtrc.json ignorePatterns: the config lives beside this script, not in + // the repo being formatted, and its patterns do not resolve against the + // working directory. const configPath = path.join(__dirname, ".oxfmtrc.json"); - const skip = [ - "!.github/workflows/*.lock.yml", - "!.github/workflows/agentics-maintenance.yml", - ] + const skip = generatedWorkflowSkipGlobs() .map((glob) => JSON.stringify(glob)) .join(" "); console.log("Formatting workflow files with oxfmt..."); // eslint-disable-line no-console diff --git a/actions/fix-workflows/generated-workflows.test.ts b/actions/fix-workflows/generated-workflows.test.ts index f1d0fb57..7b799f29 100644 --- a/actions/fix-workflows/generated-workflows.test.ts +++ b/actions/fix-workflows/generated-workflows.test.ts @@ -1,22 +1,46 @@ +import * as fs from "fs"; import {describe, expect, it} from "vitest"; -import {isGeneratedWorkflow} from "./index.ts"; +import {generatedWorkflowSkipGlobs, isGeneratedWorkflow} from "./index.ts"; /** * Which `.github/workflows` entries a generator owns, split from index.test.ts to * keep that file inside its 1000-line cap. * - * This predicate is the single source of truth for a set that is duplicated as - * negated globs in action.yml and cli.ts. It exists because those three places - * diverged once: the YAML fixer skipped `*.lock.yml` while the oxfmt step did not. + * `GENERATED_WORKFLOWS` is the single source of truth for a set that three places + * act on: the fixer calls `isGeneratedWorkflow`, cli.ts derives its oxfmt globs + * from `generatedWorkflowSkipGlobs()`, and action.yml's bash step -- which cannot + * import either -- hand-copies those globs. The last of those is what this file's + * second describe block pins, because the three diverged once already: the YAML + * fixer skipped `*.lock.yml` while the oxfmt step reformatted it. */ +const actionYml = fs.readFileSync( + new URL("./action.yml", import.meta.url), + "utf-8", +); + +/** + * The oxfmt step's `run:` body alone, so that a quoted `!...` added to some other + * step cannot pass for one of the skip globs. + */ +function oxfmtStep(): string { + const afterName = actionYml.split( + "- name: Format YAML files with oxfmt", + )[1]; + if (afterName == null) { + throw new Error( + "action.yml has no oxfmt step; rename the test with it", + ); + } + return afterName.split("\n - name:")[0]!; +} + describe("isGeneratedWorkflow", () => { // These files are compiler output. Fixing them is worse than useless: the fix // is overwritten on the next `gh aw compile`, and the "this file needs // updating" annotation invites a maintainer to desync a committed lock from - // its compiler. The oxfmt globs in action.yml and cli.ts must exclude exactly - // this set -- they diverged once, and only the formatter step touched locks. + // its compiler. it("skips compiled agentic workflow locks", () => { expect(isGeneratedWorkflow("review.lock.yml")).toBe(true); expect(isGeneratedWorkflow("autofix.lock.yml")).toBe(true); @@ -36,3 +60,22 @@ describe("isGeneratedWorkflow", () => { ); }); }); + +describe("action.yml's copy of the skip globs", () => { + // The only site that cannot derive its skip set from GENERATED_WORKFLOWS, so + // it is the only one that can silently fall behind a fourth entry. This test + // is the substitute for the import it cannot do. + it("matches generatedWorkflowSkipGlobs()", () => { + const literals = [...oxfmtStep().matchAll(/'(![^']+)'/g)].map( + (match) => match[1], + ); + expect(literals).toEqual(generatedWorkflowSkipGlobs()); + }); + + // Negating patterns the positive glob never selects would leave the locks + // unformatted for the wrong reason, and would quietly stop the negations + // being load-bearing if that glob were narrowed. + it("still formats workflows recursively, so the negations subtract from that set", () => { + expect(oxfmtStep()).toContain('".github/workflows/**/*.yml"'); + }); +}); diff --git a/actions/fix-workflows/index.ts b/actions/fix-workflows/index.ts index 2654653c..c5342e29 100644 --- a/actions/fix-workflows/index.ts +++ b/actions/fix-workflows/index.ts @@ -188,10 +188,17 @@ export function stepIgnoresSetup(step: YAMLMap): boolean { * gh-aw's scheduled housekeeping workflow, which is regenerated unconditionally * and (unhelpfully) is not named `*.lock.yml`. * - * The general rule would be "skip anything `.gitattributes` marks - * `linguist-generated`", which would need no updating as generators change. - * That is a larger change than this list and is left for when a third generated - * workflow shows up. + * Matching by name rather than by content is deliberate, and the cheap + * content check is not the upgrade it looks like. Both files gh-aw emits do + * carry a `DO NOT EDIT.` marker in their first line, so the *fixer* could + * sniff for it and stop needing this list. The oxfmt step cannot: it selects + * files by glob, and a glob cannot read a header. Teaching only the fixer to + * sniff would put the two back out of step -- a marked-but-unlisted file would + * be skipped by the fixer and still reformatted by the formatter, which is + * exactly the bug this list exists to close. A content rule has to arrive at + * both sites at once (the formatter taking an explicit file list computed here + * instead of a glob), so it is left for when a third generated workflow makes + * that refactor worth it. */ export const GENERATED_WORKFLOWS = { suffixes: [".lock.yml"], @@ -199,9 +206,38 @@ export const GENERATED_WORKFLOWS = { }; /** Whether a `.github/workflows` entry is generator-owned (see above). */ -export const isGeneratedWorkflow = (name: string): boolean => - GENERATED_WORKFLOWS.suffixes.some((suffix) => name.endsWith(suffix)) || - GENERATED_WORKFLOWS.names.includes(name); +export function isGeneratedWorkflow(name: string): boolean { + return ( + GENERATED_WORKFLOWS.suffixes.some((suffix) => name.endsWith(suffix)) || + GENERATED_WORKFLOWS.names.includes(name) + ); +} + +/** + * `GENERATED_WORKFLOWS` as negated oxfmt globs, for the formatting step that + * runs after the fixer. + * + * The formatter selects files by glob rather than by `isGeneratedWorkflow`, so + * the skip set has to be expressible as globs too; deriving them here keeps + * cli.ts from carrying a second copy of the set. action.yml still spells them + * out (a composite action's bash step cannot import this module), and + * generated-workflows.test.ts asserts its literals against this function so + * that copy cannot drift. + * + * The `**` mirrors the recursive positive glob both call sites pass. It also + * matches files directly in `.github/workflows`, so one pattern covers both + * depths (verified against the pinned oxfmt@0.44.0). + */ +export function generatedWorkflowSkipGlobs(): string[] { + return [ + ...GENERATED_WORKFLOWS.suffixes.map( + (suffix) => `!.github/workflows/**/*${suffix}`, + ), + ...GENERATED_WORKFLOWS.names.map( + (name) => `!.github/workflows/**/${name}`, + ), + ]; +} function getFilesToCheck(): string[] { const files: string[] = [];