Skip to content

Commit ede5b60

Browse files
claudeclaude
committed
fix(ci): a gate earns credit by running, so if: must be ruled on
Round 9 put `if:` in the step digest, which makes an unapproved `if: false` fail against the old baseline. Round 10 is that this is not enough: the baseline is GENERATED, so `--update` records the new digest and the gate is credited again. Where credit is decided cannot be a generated file. `GATE_CONDITIONS` is hand-written and says, per exact condition, whether a step guarded by it counts as running. Undeclared earns nothing, so `if: false` unowns whatever it guarded no matter how many times the baseline is regenerated. Six conditions cover the 19 conditional gate steps in the tree; the other 51 are unconditional and need nothing. Two of the six are declared as NOT crediting, which is the honest reading: `failure()` guards the mutation lanes' envelope recorders, which run only after the lane has already failed, and `env.IOS_UDID != ''` guards a suite that runs only when a repository variable names an attached device. The first costs nothing — every id it names is also invoked unconditionally. The second leaves `replay-ios-device` with no owner this tree can prove, so it is named in `UNPROVABLE_OWNERS` with its reason and reported separately from the wired count instead of being folded into it. A conditional `uses:` guards everything the action reaches, and a `uses:` step runs no shell of its own, so no digest can carry it; the caller's condition is attached to the gate sighting directly. Verified by running the reported attack end to end against the real tree: set the Apple runner build step to `if: false`, run `--update`, and the audit still fails four ways — `owned`, `path-coverage`, `condition` and `inert`. Four checked-in regressions, including the post-regeneration `covered()` assertion round 10 asked for, and an ablation proving every crediting condition is load-bearing. 70 gate tests. Manifest, lint, typecheck, format, layering, depgraph, production-exports and fallow green locally; baseline unchanged at 90 entries.
1 parent efb9d22 commit ede5b60

7 files changed

Lines changed: 390 additions & 28 deletions

File tree

‎CONTEXT.md‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -469,8 +469,10 @@ when landing it, satisfy the gate where one exists.
469469
generated baseline (`scripts/gate/baseline.json`). The construction is narrowed so most of that
470470
cannot arise: a local action may not interpolate an input into a `run:` block, an action that
471471
builds through a gate takes the gate ID rather than a command, and environment is checked against
472-
an allowlist by name instead of by digest. The workflow→check mapping is a shape check
473-
rather than an interpretation of shell, and a gate that is not in the registry
472+
an allowlist by name instead of by digest. A lane earns credit for a gate only if the step
473+
runs, so an `if:` guarding one is ruled on in `GATE_CONDITIONS` and undeclared earns nothing —
474+
hand-written precisely because `--update` regenerates the baseline. The workflow→check mapping
475+
is a shape check rather than an interpretation of shell, and a gate that is not in the registry
474476
(`scripts/check-affected/checks.ts`) cannot run at all — the failure
475477
mode is a red build, not an invisible one. `check:gate-manifest` fails when a registered check
476478
is run by no `pull_request`/`schedule` lane, when a path category's selected checks are not run

‎docs/agents/testing.md‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -237,8 +237,18 @@ clever. Three seams that used to accept arbitrary commands no longer can:
237237
`NODE_OPTIONS`, `BASH_ENV`, `PERL5OPT`, `LD_PRELOAD` and `PATH` cannot arrive by accident,
238238
while ordinary device configuration needs no waiver at all.
239239

240+
A gate step earns its lane ownership credit only if it **runs**, so an `if:` on one — or on
241+
the `uses:` that reaches it — must be ruled on in `GATE_CONDITIONS`, with whether it counts.
242+
Undeclared earns nothing, which is why `if: false` unowns whatever it guarded. This
243+
deliberately does not live in the baseline: that file is generated, so `--update` would
244+
record the new digest and hand the credit back. Six conditions guard the 19 conditional gate
245+
steps in the tree. A check whose owner genuinely cannot be proved from the tree —
246+
`replay-ios-device`, which needs a repository variable naming an attached device — is named in
247+
`UNPROVABLE_OWNERS` with its reason, and reported separately from the wired count rather than
248+
folded into it.
249+
240250
The residue — everything CI genuinely runs outside the runner — is a generated baseline
241-
(`scripts/gate/baseline.json`, 81 entries of file + step + digest), refreshed with
251+
(`scripts/gate/baseline.json`, 90 entries of file + step + digest), refreshed with
242252
`pnpm check:gate-manifest --update` so a reviewer reads the diff. The prose lives once per
243253
declaring file in `REASONS`. It fails in both directions: an unrecorded step fails as a
244254
bypass, and an entry matching no live step fails as inert. Two execution surfaces the loader

‎scripts/gate/audit.test.ts‎

Lines changed: 126 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -16,11 +16,11 @@ import fs from 'node:fs';
1616
import os from 'node:os';
1717
import path from 'node:path';
1818
import test from 'node:test';
19-
import { audit, formatFailures } from './audit.ts';
20-
import { loadBaseline } from './baseline.ts';
21-
import { ALLOWED_ENV, EXTERNAL_ACTIONS } from './declarations.ts';
19+
import { audit, formatFailures, plainGateStep } from './audit.ts';
20+
import { census, loadBaseline } from './baseline.ts';
21+
import { ALLOWED_ENV, EXTERNAL_ACTIONS, GATE_CONDITIONS } from './declarations.ts';
2222
import { loadModel, type Model } from './model.ts';
23-
import { loadLanes, stepDigest, type Lane } from './workflows.ts';
23+
import { creditsUnder, loadLanes, stepDigest, type Lane } from './workflows.ts';
2424

2525
const repoRoot = path.resolve(import.meta.dirname, '../..');
2626
const tracked = execFileSync('git', ['ls-files'], { cwd: repoRoot, encoding: 'utf8' })
@@ -181,7 +181,9 @@ jobs:
181181
if: false
182182
run: pnpm gate layering
183183
`),
184-
['bypass'],
184+
// `bypass` against the shipped baseline, `condition` regardless of it — round 10 is that
185+
// only the second survives `--update`.
186+
['bypass', 'condition'],
185187
],
186188
[
187189
'r6: an undeclared third-party action',
@@ -303,7 +305,10 @@ test('a gate-valued action must be proven to run its gate, not trusted to', () =
303305
audit(
304306
{
305307
...base,
306-
gateActionBodies: { ...base.gateActionBodies, [GATE_ACTION]: { run, boundTo: 'gate' } },
308+
gateActionBodies: {
309+
...base.gateActionBodies,
310+
[GATE_ACTION]: { run, boundTo: 'gate', condition: null },
311+
},
307312
},
308313
baseline,
309314
).filter((failure) => failure.assertion === 'gate');
@@ -314,6 +319,121 @@ test('a gate-valued action must be proven to run its gate, not trusted to', () =
314319
assert.match(noop[0]?.message ?? '', /does not invoke/);
315320
});
316321

322+
test('r10: regenerating the baseline cannot make `if: false` credit a gate', () => {
323+
// Round 9 put `if:` in the step digest, so an unapproved `if: false` fails as `bypass`
324+
// against the OLD baseline. Round 10's point is that this is not enough: the baseline is
325+
// GENERATED, and `--update` records the new digest. What must survive regeneration is
326+
// ownership, so this asserts `covered()` and not just the pre-update finding.
327+
const disabled = plantWorkflow(
328+
workflow(
329+
' - name: Run the parser fuzzer\n if: false\n run: pnpm gate fuzz-parsers\n',
330+
),
331+
);
332+
// Drop the real owner, so the only lane offering the gate is the disabled one.
333+
const soleOwner: Model = {
334+
...disabled,
335+
lanes: disabled.lanes.filter((lane) => !lane.gates.includes('fuzz-parsers')),
336+
};
337+
const regenerated = census(soleOwner.lanes, (step) => plainGateStep(step) !== null);
338+
const found = audit(soleOwner, regenerated);
339+
340+
assert.deepEqual(
341+
found.filter((failure) => failure.assertion === 'bypass'),
342+
[],
343+
'the regenerated baseline accepts the step — which is exactly why credit cannot rest on it',
344+
);
345+
assert.ok(
346+
found.some((f) => f.assertion === 'owned' && /"fuzz-parsers"/.test(f.message)),
347+
'a gate behind `if: false` must leave its check unowned even after regeneration',
348+
);
349+
assert.ok(
350+
found.some((f) => f.assertion === 'condition' && /if: false/.test(f.message)),
351+
'and the condition itself must be named, since "unowned" does not say how to fix it',
352+
);
353+
});
354+
355+
test('r10: a condition declared as not counting earns no credit either', () => {
356+
// `failure()` is declared, so it raises no `condition` finding — but it runs only after
357+
// the lane has already failed, so it must not own anything. Undeclared and declared-false
358+
// differ in how loudly they report, never in what they credit.
359+
const onlyOnFailure = plantWorkflow(
360+
workflow(
361+
' - name: Record an envelope\n if: failure()\n run: pnpm gate fuzz-parsers\n',
362+
),
363+
);
364+
const soleOwner: Model = {
365+
...onlyOnFailure,
366+
lanes: onlyOnFailure.lanes.filter(
367+
(lane) => !lane.gates.includes('fuzz-parsers') || lane.workflow === 'planted.yml',
368+
),
369+
};
370+
const found = audit(
371+
soleOwner,
372+
census(soleOwner.lanes, (step) => plainGateStep(step) !== null),
373+
);
374+
assert.deepEqual(
375+
found.filter((f) => f.assertion === 'condition'),
376+
[],
377+
'`failure()` is declared, so it is not an unruled condition',
378+
);
379+
assert.ok(
380+
found.some((f) => f.assertion === 'owned' && /"fuzz-parsers"/.test(f.message)),
381+
'a declared non-crediting condition must still leave the check unowned',
382+
);
383+
});
384+
385+
test('r10: a conditional `uses:` cannot credit the gate the action would have run', () => {
386+
// The caller's `if:` guards everything the action reaches, and a `uses:` step runs no
387+
// shell of its own, so no digest can carry it. It has to reach the sighting directly.
388+
const planted = plantWorkflow(
389+
workflow(` - uses: ./.github/actions/setup-apple-runner-build
390+
if: false
391+
with:
392+
derived-path: /tmp/derived
393+
cache-key-prefix: planted
394+
gate: swift-runner-ios
395+
`),
396+
);
397+
const lane = planted.lanes.find((candidate) => candidate.workflow === 'planted.yml');
398+
assert.ok(lane, 'the planted lane loaded');
399+
assert.deepEqual(lane.gates, [], 'a disabled `uses:` earns nothing');
400+
assert.deepEqual(
401+
lane.gateSightings.map((sighting) => sighting.conditions),
402+
[['false', "steps.restore-runner-build.outputs.cache-hit != 'true'"]],
403+
'both guards reach the sighting: the caller`s and the one inside the action',
404+
);
405+
});
406+
407+
test('every declared gate condition that credits is load-bearing', () => {
408+
// `gates` is `gateSightings` filtered by `creditsUnder`, so recomputing it here is the
409+
// real rule, not a copy — the identity case asserts exactly that before anything is denied.
410+
const denying = (denied: string | null): Model => ({
411+
...base,
412+
lanes: base.lanes.map((lane) => ({
413+
...lane,
414+
gates: [
415+
...new Set(
416+
lane.gateSightings
417+
.filter((s) => s.conditions.every((c) => c !== denied && creditsUnder(c)))
418+
.map((s) => s.id),
419+
),
420+
],
421+
})),
422+
});
423+
assert.deepEqual(
424+
denying(null).lanes.map((lane) => [...lane.gates].sort()),
425+
base.lanes.map((lane) => [...lane.gates].sort()),
426+
'recomputing credit from sightings reproduces what the loader stored',
427+
);
428+
for (const [condition, { credits }] of Object.entries(GATE_CONDITIONS)) {
429+
if (!credits) continue;
430+
assert.ok(
431+
audit(denying(condition), baseline).length > 0,
432+
`GATE_CONDITIONS \`${condition}\` credits nothing that needs it; it is inert`,
433+
);
434+
}
435+
});
436+
317437
test('a baseline entry describing no live step is inert', () => {
318438
const invented = [
319439
...baseline,

‎scripts/gate/audit.ts‎

Lines changed: 75 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,8 @@ import {
1212
ALLOWED_ENV,
1313
EXTERNAL_ACTIONS,
1414
GATE_ACTIONS,
15+
GATE_CONDITIONS,
16+
UNPROVABLE_OWNERS,
1517
type ExternalAction,
1618
} from './declarations.ts';
1719
import { categories, checkUnits, covered, scriptUnits, type Model } from './model.ts';
@@ -28,6 +30,7 @@ const HEADINGS: Readonly<Record<string, string>> = {
2830
external: 'Third-party actions with no declaration',
2931
surface: 'Execution surfaces the manifest does not model',
3032
'path-coverage': 'Paths whose selected checks no triggered lane runs',
33+
condition: 'Gate steps behind an `if:` nobody has ruled on',
3134
inert: 'Declarations that no longer apply',
3235
registered: 'Suites and projects no registered check covers',
3336
};
@@ -66,10 +69,13 @@ function fail(assertion: string, message: string): Failure {
6669
}
6770

6871
// 1. Every registered check is run by some qualifying lane, unit by unit.
69-
function unowned(model: Model): Failure[] {
72+
function unowned(
73+
model: Model,
74+
unprovable: Readonly<Record<string, string>> = UNPROVABLE_OWNERS,
75+
): Failure[] {
7076
return CHECK_CATALOG.flatMap((spec) => {
7177
const result = covered(spec, null, model);
72-
if (result.covered) return [];
78+
if (result.covered || spec.id in unprovable) return [];
7379
const missing = result.missing.length > 0 ? result.missing.join(', ') : '(no units resolved)';
7480
return [
7581
fail(
@@ -186,6 +192,35 @@ function gateIds(model: Model): Failure[] {
186192
);
187193
}
188194

195+
// 3a-iii. A conditional gate step must have its condition ruled on.
196+
//
197+
// Ownership means the gate RUNS, and `if:` decides that. Round 9 put `if:` in the step
198+
// digest, which makes an unapproved edit fail against the old baseline — but the baseline
199+
// is GENERATED, so `--update` records the new digest and `if: false` credits a gate that
200+
// cannot run. Where credit is decided therefore cannot be a generated file. It is
201+
// `GATE_CONDITIONS`, hand-written, and an undeclared condition earns nothing.
202+
//
203+
// Reported here as well as through the resulting unowned check, because "this condition
204+
// is not ruled on" names the fix and "check x is unowned" does not.
205+
function gateConditions(model: Model): Failure[] {
206+
return model.lanes
207+
.filter((lane) => lane.qualifying)
208+
.flatMap((lane) =>
209+
lane.gateSightings.flatMap((sighting) =>
210+
[...new Set(sighting.conditions)]
211+
.filter((condition) => !(condition in GATE_CONDITIONS))
212+
.map((condition) =>
213+
fail(
214+
'condition',
215+
`${lane.workflow} / ${lane.label}: \`pnpm gate ${sighting.id}\` is guarded by ` +
216+
`\`if: ${condition}\`, which GATE_CONDITIONS does not rule on, so the lane earns ` +
217+
`no credit for it. Declare the condition with whether it counts as running.`,
218+
),
219+
),
220+
),
221+
);
222+
}
223+
189224
// 3a-ii. A gate-valued action must be PROVEN to run the gate, not trusted to.
190225
//
191226
// Listing an action in GATE_ACTIONS credits its callers' lanes with whatever id they
@@ -355,9 +390,45 @@ function inertOpaque(model: Model): Failure[] {
355390
});
356391
}
357392

393+
function seenConditions(model: Model): Set<string> {
394+
return new Set(
395+
model.lanes
396+
.filter((lane) => lane.qualifying)
397+
.flatMap((lane) => lane.gateSightings)
398+
.flatMap((sighting) => sighting.conditions),
399+
);
400+
}
401+
402+
function inertConditions(model: Model): Failure[] {
403+
const seen = seenConditions(model);
404+
return [
405+
...Object.keys(GATE_CONDITIONS)
406+
.filter((condition) => !seen.has(condition))
407+
.map((condition) =>
408+
fail('inert', `gate-condition declaration \`${condition}\` guards no gate step.`),
409+
),
410+
...Object.keys(UNPROVABLE_OWNERS)
411+
.filter((id) => {
412+
const spec = CHECK_CATALOG.find((entry) => entry.id === id);
413+
return spec === undefined || covered(spec, null, model).covered;
414+
})
415+
.map((id) =>
416+
fail(
417+
'inert',
418+
`unprovable-owner declaration "${id}" is not needed; the check is either unregistered ` +
419+
`or provably owned. Delete it.`,
420+
),
421+
),
422+
];
423+
}
424+
358425
function inert(model: Model, declared: readonly BaselineEntry[]): Failure[] {
359426
const qualifying = model.lanes.filter((lane) => lane.qualifying);
360-
return [...declared.flatMap((entry) => inertStep(entry, qualifying)), ...inertOpaque(model)];
427+
return [
428+
...declared.flatMap((entry) => inertStep(entry, qualifying)),
429+
...inertOpaque(model),
430+
...inertConditions(model),
431+
];
361432
}
362433

363434
/** Compared as a set, not a count: removing a waiver can trade one failure for another. */
@@ -421,6 +492,7 @@ export function audit(
421492
...unowned(model),
422493
...bypass(model, declared),
423494
...gateIds(model),
495+
...gateConditions(model),
424496
...gateActionBodies(model, GATE_ACTIONS),
425497
...environment(model, allowedEnv),
426498
...laneSurfaces(model),

‎scripts/gate/check.ts‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import { pathToFileURL } from 'node:url';
77
import { runCmdSync } from '../../src/utils/exec.ts';
88
import { audit, formatFailures, plainGateStep } from './audit.ts';
99
import { census, writeBaseline } from './baseline.ts';
10+
import { UNPROVABLE_OWNERS } from './declarations.ts';
1011
import { loadModel } from './model.ts';
1112

1213
function main(): number {
@@ -24,9 +25,13 @@ function main(): number {
2425
const failures = audit(model);
2526
if (failures.length === 0) {
2627
const gates = model.lanes.filter((lane) => lane.qualifying).flatMap((lane) => lane.gates);
28+
// Unprovable owners are reported rather than folded into the count: a check whose lane
29+
// this tree cannot show running should not read the same as one it can.
30+
const unprovable = Object.keys(UNPROVABLE_OWNERS).length;
2731
process.stdout.write(
2832
`gate manifest: ok — ${new Set(gates).size} checks wired across ` +
29-
`${model.lanes.filter((lane) => lane.gates.length > 0).length} lanes.\n`,
33+
`${model.lanes.filter((lane) => lane.gates.length > 0).length} lanes` +
34+
`${unprovable > 0 ? `, ${unprovable} declared unprovable` : ''}.\n`,
3035
);
3136
return 0;
3237
}

0 commit comments

Comments
 (0)