fix-workflows: stop reformatting generated workflows

.changeset/fix-workflows-skip-generated.md

@@ -0,0 +1,32 @@
+---
+"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. 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
+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.

actions/fix-workflows/action.yml

@@ -36,12 +36,24 @@ runs:
                       core.setFailed(err.message)
                   }
 
+        # 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/**/*.yml" \
+                '!.github/workflows/**/*.lock.yml' \
+                '!.github/workflows/**/agentics-maintenance.yml'
 
         - name: Show fix instructions
           shell: bash

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;
@@ -44,13 +47,21 @@ 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
+        // 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 = generatedWorkflowSkipGlobs()
+            .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"},
         );
     })

actions/fix-workflows/generated-workflows.test.ts

@@ -0,0 +1,81 @@
+import * as fs from "fs";
+import {describe, expect, it} from "vitest";
+
+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.
+ *
+ * `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.
+    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,
+        );
+    });
+});
+
+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"');
+    });
+});

actions/fix-workflows/index.ts

@@ -178,6 +178,67 @@ 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`.
+ *
+ * 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"],
+    names: ["agentics-maintenance.yml"],
+};
+
+/** Whether a `.github/workflows` entry is generator-owned (see above). */
+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[] = [];
 
@@ -186,7 +247,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));
         }