Nate Swenson

Your generator's output is code nobody reviewed

Skill Factory · No. 112 · 9 min read ·v0.4.0

Shipped

The scaffolder stopped emitting a release trigger this repo had already banned, an eighth registry was added to the list a new skill gets wired into, and the reference document that still described the old behaviour was rewritten. The assertions covering all of that went into their own file rather than into the baseline the tooling regenerates, for reasons that turn out to be the most useful part of the story.

The transferable piece: if you generate code, the check that governs your hand-written files has to run against what your generator emits, and the test proving it has to live somewhere your own tooling cannot quietly delete.

One policy, applied to generated files too

The banned pattern was a release job that accepted push as well as workflow_dispatch. In this repo a dev to main promotion merge fires a push event, so a release job wired that way releases on merge rather than on a deliberate decision. That had already cost two real releases: one skill tagged with stale notes before a planned changelog edit landed, and another tagged and published to npm seconds after a merge, with no dispatch and nobody choosing to release it.

The trigger was banned. Every hand-written caller was fixed. And the scaffolder kept handing the old one to every new skill, because generated files are not where people look.

That is not a metaphor. GitHub will hide generated files from diffs by default when you mark them linguist-generated in .gitattributes, which is a sensible feature and also an accurate description of what happens to generated code without the attribute: people scroll past it. A policy enforced by review does not reach anything nobody reads.

So write the policy as code, once, and let it govern everything:

// The policy that governs EVERY workflow in the repo. There is exactly one of
// these, and generated files are not exempt from it. Reusing the checker you
// already have is the point; writing a second one for generated output is how
// the two drift apart.
const BANNED_RELEASE_TRIGGERS = [
  {
    id: 'release-on-push',
    // A promotion merge fires a push event, so a release job that accepts
    // `push` releases on merge instead of on a deliberate dispatch.
    test: /event_name\s*==\s*'push'/,
    why: "a release job must be workflow_dispatch-only; `push` makes a merge cut the tag",
  },
];

export function checkWorkflow(name, yaml) {
  const findings = [];
  for (const rule of BANNED_RELEASE_TRIGGERS) {
    if (rule.test.test(yaml)) findings.push({ file: name, rule: rule.id, why: rule.why });
  }
  return { ok: findings.length === 0, findings };
}

In a real repo you would not write this; you would point the linter you already run at the generated path. The important property is that there is one checker, not two. A second checker written specially for generated output starts as a copy and ends as a different rule.

Run the generator inside the test

Here is the generator, in both the shape that predated the ban and the shape that respects it:

// The scaffolder. `emitCaller` is the template that predates the policy;
// `emitCallerFixed` is the same template after it was brought back in line.
export function emitCaller(skill) {
  return `name: ${skill}
on:
  pull_request:
  push:
    branches: [main]
    paths: ['skills/${skill}/**']
  workflow_dispatch:
jobs:
  ci:
    uses: ./.github/workflows/_ci.yml
  release:
    needs: ci
    if: (github.event_name == 'push' || github.event_name == 'workflow_dispatch')
    uses: ./.github/workflows/_release.yml
`;
}

export function emitCallerFixed(skill) {
  return `name: ${skill}
on:
  pull_request:
  push:
    branches: [main]
    paths: ['skills/${skill}/**']
  workflow_dispatch:
jobs:
  ci:
    uses: ./.github/workflows/_ci.yml
  release:
    needs: ci
    # NEVER add \`push\` here. A promotion merge fires it, which turns every
    # merge into a release.
    if: github.event_name == 'workflow_dispatch'
    uses: ./.github/workflows/_release.yml
`;
}

// Every registry a new skill has to appear in for the rest of the toolchain to
// see it. The list is the contract; a scaffolder that wires six of seven
// produces a skill that looks complete and is invisible to one tool.
export const REGISTRIES = [
  'marketplace.json',
  'repo-settings.sh',
  'plugin.json',
  'workflow-caller',
  'changelog',
  'invariants',
  'shipflow.json',
];

export function scaffold(skill, { fixed = true } = {}) {
  return {
    workflow: (fixed ? emitCallerFixed : emitCaller)(skill),
    wired: REGISTRIES.map((r) => ({ registry: r, entry: skill })),
  };
}

Note the push trigger survives at the top of both templates. That one is correct and load-bearing: it runs the tests when the branch changes. workflow_dispatch is required for the manual trigger, which the docs describe as how you run a workflow from the API, CLI or UI. The banned thing is narrower than either: it is the release job’s if: accepting push. A rule stated as “no push triggers” would have been wrong and would have been removed within a week.

The registry list is the other half of what shipped. This repo’s scaffolder wires eight registries now; it wired seven, and the missing one meant a newly created skill was invisible to the release tooling entirely, which reported on every other component and looked complete while doing it.

Now the test that ties the two together:

// The test that closes the loop: RUN the generator, then hand its output to the
// same policy checker your hand-written files go through.
//
// This file is deliberately separate from any golden/baseline file the tooling
// regenerates. A guard living inside a regenerated file is deleted by the
// command someone runs when a golden goes red, which is the exact moment the
// guard was supposed to speak up.
import assert from 'node:assert/strict';
import { checkWorkflow } from './policy.mjs';
import { emitCaller, scaffold, REGISTRIES } from './generate.mjs';

export function run() {
  const results = [];

  // 1. The generator's output obeys the policy.
  const out = scaffold('newskill');
  const checked = checkWorkflow('newskill.yml', out.workflow);
  assert.equal(checked.ok, true, `scaffolded workflow violates policy: ${JSON.stringify(checked.findings)}`);
  results.push('scaffolded workflow passes the repo policy');

  // 2. Non-vacuity. The check must FAIL against the template it replaced,
  //    otherwise it is asserting nothing and will stay green through a
  //    regression.
  const old = checkWorkflow('newskill.yml', emitCaller('newskill'));
  assert.equal(old.ok, false, 'the policy check does not catch the banned trigger');
  results.push(`policy still catches the old template: ${old.findings[0].rule}`);

  // 3. Every registry gets an entry. Counting is the assertion, because a
  //    missing one is invisible in a diff that only shows what was added.
  assert.equal(out.wired.length, REGISTRIES.length);
  for (const r of REGISTRIES) {
    assert.ok(out.wired.some((w) => w.registry === r && w.entry === 'newskill'), `not wired into ${r}`);
  }
  results.push(`wired into all ${REGISTRIES.length} registries`);

  return results;
}

Assertion 2 is the one people skip and the one that does the work. A test asserting only that today’s output is clean passes just as happily against a checker that has stopped checking. Asserting the check still fails against the input it was written for is what keeps it honest, and it costs one extra line.

Run it

import assert from 'node:assert/strict';
import { checkWorkflow } from './policy.mjs';
import { emitCaller, emitCallerFixed } from './generate.mjs';
import { run } from './wiring.test.mjs';

// What the policy says about each template.
for (const [label, yaml] of [
  ['old template', emitCaller('newskill')],
  ['fixed template', emitCallerFixed('newskill')],
]) {
  const r = checkWorkflow('newskill.yml', yaml);
  console.log(`${label.padEnd(15)} ok=${r.ok}` + (r.ok ? '' : `  ${r.findings[0].rule}: ${r.findings[0].why}`));
}

console.log();
for (const line of run()) console.log('PASS', line);

// ── the part that bites: a regenerator eating its own guard ──────────────────
// `freeze` rewrites the baseline file from a template. Anything hand-written in
// it is gone, silently, and the run that deletes it is the run someone starts
// because a golden went red.
const BASELINE_TEMPLATE = `import assert from 'node:assert/strict';\n// generated by freeze\n`;
let baselineFile = BASELINE_TEMPLATE + `assert.ok(scaffoldOutputPassesPolicy(), 'hand-written guard');\n`;

console.log('\nbefore freeze, baseline file guards:',
  baselineFile.includes('hand-written guard') ? 1 : 0);
baselineFile = BASELINE_TEMPLATE;   // freeze regenerates from the template
console.log('after  freeze, baseline file guards:',
  baselineFile.includes('hand-written guard') ? 1 : 0);

// The separate file survives, because nothing regenerates it.
assert.equal(run().length, 3, 'the standalone wiring test still runs');
console.log('after  freeze, wiring.test.mjs assertions:', run().length);

console.log('\nall checks passed');

Save the three blocks as policy.mjs, generate.mjs and wiring.test.mjs, this one as check.mjs, then run node check.mjs. It needs nothing but Node. Here is what it printed for me:

old template    ok=false  release-on-push: a release job must be workflow_dispatch-only; `push` makes a merge cut the tag
fixed template  ok=true

PASS scaffolded workflow passes the repo policy
PASS policy still catches the old template: release-on-push
PASS wired into all 7 registries

before freeze, baseline file guards: 1
after  freeze, baseline file guards: 0
after  freeze, wiring.test.mjs assertions: 3

all checks passed

The last three lines are the reason the test file sits where it does. A guard written into a regenerated file counts one assertion before the regenerator runs and zero after, with nothing in the output saying so.

Gotchas

A generated defect is found one project too late. This one was caught by CI, which sounds fine until you notice where: on a brand new skill’s first pull request. By that point the bad file has already been written into a new directory, so the fix is manual, and it will be manual again for the next skill until the template itself changes. CI on the consumer tells you the generator is broken; only a test on the generator stops it producing more. Both are worth having, and they are not substitutes.

Regenerators eat hand-written guards, silently. The command that rewrites a baseline from a template does not diff what it is replacing, and the moment somebody runs it is the moment a golden has gone red, which is the moment the guard was trying to say something. Keep regression assertions in a file nothing regenerates. This is also why golden files alone are thin protection: change-detector tests that mirror the implementation get updated rather than investigated whenever they fail. Verify the split works by reverting the template, re-running the regenerator, and confirming your separate test still fails.

A stale reference doc undoes the fix later. The reference this scaffolder ships still carried a section describing releases as publish-on-merge, which is exactly the behaviour that was reversed. A generator and a document that both predate a change will reintroduce it between them, one confidently and one politely. When you ban a pattern, grep the prose too.

State the ban narrowly enough to survive. push is correct in the workflow’s top-level trigger list and wrong in the release job’s condition. A rule written as “no push” would be deleted the first time it blocked something legitimate. Aim the check at the exact construct that causes the harm.

Count registry entries; do not eyeball them. A missing wiring point is invisible in a diff, because a diff shows what changed and this defect is something absent. Assert the length against the declared list so adding a new registry to the contract fails until the generator writes it.

Sources

Changelog

  • fix(skillfactory): stop scaffolding the release trigger this repo banned (0.4.0) (#170) (8bc5386)