Skip to content

Commit d786865

Browse files
committed
fix(ci): audit the commands callers hand to third-party actions (#1429)
Review round 6. Round 5 taught the model to read a LOCAL composite action and treat the values a caller passes to inputs it interpolates into `run:` blocks as steps of the calling lane. A third-party action cannot be read, and the model therefore recorded nothing for it — neither the `uses:` step nor its `with:` values. `reactivecircus/android-emulator-runner`'s `script:` input is how the Android smoke, perf and replay lanes run their entire suites, so the largest command in CI moved no digest and produced no finding. Confirmed before fixing: injecting `node -e 'import("./scripts/layering/…")'` into android.yml's emulator script at the previous head reported "ok — 48 checks wired across 34 lanes". The same edit now fails twice, as an unlisted step and as an inert entry. Which inputs an action executes is now read for a local composite and DECLARED for a third-party one, per pinned sha, in EXTERNAL_ACTIONS. Pinning is what makes the claim checkable: `owner/repo@<sha>` names immutable content, so bumping a pin fails as undeclared until someone re-reads that version's action.yml. An action no entry names fails the audit rather than being assumed inert; an entry no qualifying lane uses fails as inert. Each of the 13 entries was checked against its action.yml at the pinned sha. Two more surfaces in the same pass: - The depth-3 composite-action cutoff returned an empty step list, which reads downstream as "this action executes nothing" — silence in the one place that must shout. Replaced with cycle-aware traversal: a cycle raises, and nesting deeper than three levels is now followed rather than dropped. - `check:gate-manifest:test` listed its test files by hand, so a new one was silently unrun. It globs `scripts/gate/*.test.ts` now. Extraction first, per the ≤500-LOC tripwire model.ts had crossed (624): the workflow/action surface derivation moved to workflows.ts, shell splitting to shell.ts, leaving model.ts at 277 for units and coverage. Tests follow the same seam, and audit.test.ts — which the new cases pushed to 567 — splits by the claim it asserts, the way scripts/layering/ names its test files. Inventory 104 -> 109 entries: the five live call sites that pass executed values to a third-party action. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01SkS4S8XXrfkJ8TD1VBKkvJ
1 parent e3e3edc commit d786865

13 files changed

Lines changed: 1286 additions & 566 deletions

‎CONTEXT.md‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -466,7 +466,9 @@ when landing it, satisfy the gate where one exists.
466466
- **A gate that stops running is worse than no gate.** Gate: the gate manifest
467467
(`scripts/gate/`, #1429). every shell block a qualifying lane reaches — a `run:` step, or a value
468468
passed to an action input the action executes — is either `pnpm gate <check-id>` or inventoried in
469-
`scripts/gate/declarations.ts`, so the workflow→check mapping is a shape check
469+
`scripts/gate/declarations.ts`. Which inputs an action executes is read for a local composite and
470+
declared per pinned sha for a third-party one, so an undeclared action is a failure rather than an
471+
assumption that it runs nothing. The workflow→check mapping is a shape check
470472
rather than an interpretation of shell, and a gate that is not in the registry
471473
(`scripts/check-affected/checks.ts`) cannot run at all — the failure
472474
mode is a red build, not an invisible one. `check:gate-manifest` fails when a registered check

‎docs/agents/testing.md‎

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -218,26 +218,31 @@ strings, and four suites (`check:tmpdir-leaks`, its model tests, `test:fixture-c
218218

219219
The rule: **every shell block a qualifying lane reaches is either `pnpm gate <check-id>`
220220
or listed in `NON_GATE_STEPS`.** A shell block is a `run:` step — in a workflow or in a local
221-
composite action — or a value passed to an action input that the action interpolates into a
222-
`run:`. That second kind matters because `setup-apple-runner-build` runs
223-
`${{ inputs.build-command }}`: its own step digest is constant, so the command is really written
224-
at the call site, and the caller's value is held to the rule instead. Nothing inspects what a
225-
command does. That is deliberate
221+
composite action — or a value passed to an action input that the action executes. That second
222+
kind matters because `setup-apple-runner-build` runs `${{ inputs.build-command }}`: its own step
223+
digest is constant, so the command is really written at the call site, and the caller's value is
224+
held to the rule instead. Which inputs an action executes is *read* for a local composite (any
225+
input it interpolates into a `run:` block) and *declared* for a third-party one, per pinned sha,
226+
in `EXTERNAL_ACTIONS` — an action nothing declares fails the audit rather than being assumed
227+
inert. The Android smoke suite runs entirely inside `android-emulator-runner`'s `script:` input,
228+
so that is not a hypothetical surface. Nothing inspects what a command does. That is deliberate
226229
and was learned the hard way: a content-based classifier was defeated by `pnpm exec`, then
227230
by `pnpm exec --` and `npx --yes`, then by `node -e 'import("./scripts/…")'` — and after
228231
those, `eval`, heredocs and base64 remain. "Does this text run project code?" is not
229232
answerable from text, so the question is not asked. A step is allowed because of its
230233
shape; everything else is written down by a human, which is the only boundary an unknown
231234
spelling cannot walk through.
232235

233-
The price is a long inventory (104 entries today, keyed on the file that declares them, so a
236+
The price is a long inventory (109 entries today, keyed on the file that declares them, so a
234237
composite action shared by eight lanes is listed once — but an executed input is listed per call
235238
site, because a new caller of the same action is a new command to review). That is the census of
236239
everything CI does outside the runner, and it fails in both directions — an unlisted step fails as
237240
a bypass, a listed step whose body is edited fails as both, since entries bind the step's digest
238241
(`run` plus `env`/`shell`/`working-directory`) rather than its name. Lane-level `env:` is
239242
fingerprinted the same way in `LANE_ENVIRONMENTS`, and the two execution surfaces the loader does
240-
not read — `defaults.run` and reusable-workflow jobs — fail closed rather than being ignored.
243+
not read — `defaults.run` and reusable-workflow jobs — fail closed rather than being ignored. A
244+
composite-action cycle raises rather than truncating: the depth cutoff it replaces returned an
245+
empty step list, which every assertion downstream would read as "this action executes nothing".
241246
Every gate is a `CheckId` in the registry (`scripts/check-affected/checks.ts`) — the same universe
242247
the affected-selector uses — so the workflow→check mapping is a token scan for `pnpm gate <id>`
243248
rather than an interpretation of shell. That is what makes a new gate impossible to hide: an

‎package.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -130,7 +130,7 @@
130130
"check:affected:test": "node --experimental-strip-types scripts/node-test-tmpdir.ts --experimental-strip-types --test scripts/check-affected/model.test.ts scripts/check-affected/platform-packages.test.ts scripts/check-affected/run.test.ts",
131131
"gate": "node --experimental-strip-types scripts/gate/run.ts",
132132
"check:gate-manifest": "node --experimental-strip-types scripts/gate/check.ts",
133-
"check:gate-manifest:test": "node --experimental-strip-types scripts/node-test-tmpdir.ts --experimental-strip-types --test scripts/gate/model.test.ts scripts/gate/audit.test.ts scripts/gate/run.test.ts",
133+
"check:gate-manifest:test": "node --experimental-strip-types scripts/node-test-tmpdir.ts --experimental-strip-types --test scripts/gate/*.test.ts",
134134
"check:coverage-changed": "node --experimental-strip-types scripts/coverage-changed/run.ts",
135135
"check:coverage-changed:test": "node --experimental-strip-types scripts/node-test-tmpdir.ts --experimental-strip-types --test scripts/coverage-changed/model.test.ts scripts/coverage-changed/run.test.ts",
136136
"check:layering": "node --experimental-strip-types scripts/node-test-tmpdir.ts --experimental-strip-types --test scripts/layering/model.test.ts scripts/layering/zone-policy.test.ts scripts/layering/daemon-modularity.test.ts scripts/layering/package-boundaries.test.ts scripts/layering/platform-package-policy.test.ts scripts/layering/platform-package-repository.test.ts scripts/layering/platform-package-source-policy.test.ts scripts/layering/device-inventory-cutover-policy.test.ts scripts/layering/logs-runtime-cutover-policy.test.ts scripts/layering/network-runtime-cutover-policy.test.ts scripts/layering/record-runtime-cutover-policy.test.ts scripts/layering/contracts-implementation-policy.test.ts scripts/layering/facade-exports.test.ts scripts/layering/bin-alias-fast-path.test.ts && node --experimental-strip-types scripts/layering/check.ts",
Lines changed: 174 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,174 @@
1+
// Assertion group two: is every registered check actually RUN by a lane a PR would start?
2+
//
3+
// Split from audit.test.ts, which asserts the other half — that nothing runs outside a
4+
// gate. The two use different machinery (a mutated model here, planted YAML through the
5+
// real loader there), which is the seam; `scripts/layering/` names its test files after
6+
// the claim they assert for the same reason.
7+
//
8+
// Every case mutates the REAL model and asserts the audit goes red for the right reason.
9+
// #1714's predecessor had 75 unit tests over home-grown parsers, and the two holes that
10+
// mattered — a suite outside the naming convention, and category samples naming files no
11+
// PR could touch — were green in every one of them.
12+
13+
import { execFileSync } from 'node:child_process';
14+
import assert from 'node:assert/strict';
15+
import path from 'node:path';
16+
import test from 'node:test';
17+
import { audit } from './audit.ts';
18+
import { NON_GATE_STEPS } from './declarations.ts';
19+
import { categories, covered, loadModel, type Model } from './model.ts';
20+
import type { Lane } from './workflows.ts';
21+
import { CHECK_CATALOG } from '../check-affected/checks.ts';
22+
23+
const repoRoot = path.resolve(import.meta.dirname, '../..');
24+
const tracked = execFileSync('git', ['ls-files'], {
25+
cwd: repoRoot,
26+
encoding: 'utf8',
27+
})
28+
.split('\n')
29+
.filter(Boolean);
30+
const base = loadModel(repoRoot, tracked);
31+
32+
function mutate(change: (model: Model) => Partial<Model>): Model {
33+
return { ...base, ...change(base) };
34+
}
35+
36+
function messages(model: Model, declared = NON_GATE_STEPS): string[] {
37+
return audit(model, declared).map((failure) => failure.message);
38+
}
39+
40+
function mapLane(
41+
model: Model,
42+
match: (lane: Lane) => boolean,
43+
change: (lane: Lane) => Lane,
44+
): Lane[] {
45+
return model.lanes.map((lane) => (match(lane) ? change(lane) : lane));
46+
}
47+
48+
test('the live tree is green — every planted failure below is a real difference', () => {
49+
assert.deepEqual(messages(base), []);
50+
});
51+
52+
test('deleting the lane that runs a gate reports exactly that gate, naming the runner', () => {
53+
const model = mutate((m) => ({
54+
lanes: mapLane(
55+
m,
56+
(lane) => lane.gates.includes('fuzz-parsers'),
57+
(lane) => ({
58+
...lane,
59+
gates: lane.gates.filter((id) => id !== 'fuzz-parsers'),
60+
}),
61+
),
62+
}));
63+
const found = messages(model);
64+
assert.equal(found.length, 1);
65+
assert.match(
66+
found[0] ?? '',
67+
/check "fuzz-parsers" is not run by any pull_request\/schedule lane/,
68+
);
69+
assert.match(found[0] ?? '', /pnpm gate fuzz-parsers/);
70+
});
71+
72+
test('a gate outside the test:/check: naming convention is still owned — the #1714 hole', () => {
73+
// `fuzz:parsers` resolves to an executable, not a Vitest project or a --test file.
74+
// Under the old design that made it invisible to the suite universe; here it is a
75+
// registered check like any other, so nothing has to recognise its shape.
76+
const spec = CHECK_CATALOG.find((entry) => entry.id === 'fuzz-parsers');
77+
assert.ok(spec);
78+
const result = covered(spec, null, base);
79+
assert.ok(result.covered);
80+
assert.deepEqual(result.lanes, ['Replay Nightly / Parser Fuzz Lane']);
81+
});
82+
83+
test('a docs-only change still reaches the command-reference gate (#1420)', () => {
84+
const model = mutate((m) => ({
85+
lanes: mapLane(
86+
m,
87+
(lane) => lane.workflow === 'pr-preview.yml',
88+
(lane) => ({ ...lane, paths: ['website/assets/**'] }),
89+
),
90+
}));
91+
const found = messages(model);
92+
assert.equal(found.length, 1);
93+
assert.match(found[0] ?? '', /website\/docs\/docs\/commands\.md/);
94+
assert.match(found[0] ?? '', /selects "command-docs"/);
95+
});
96+
97+
test('per-unit coverage catches a lane deletion that whole-check ownership would miss', () => {
98+
// command-docs-gate runs one unit-core FILE, so it must not stand in for the
99+
// project when Coverage disappears.
100+
const model = mutate((m) => ({
101+
lanes: m.lanes.filter((lane) => lane.label !== 'Coverage'),
102+
}));
103+
const found = messages(model);
104+
assert.ok(found.some((message) => /check "unit-ci" is not run/.test(message)));
105+
assert.ok(found.some((message) => /check "vitest-related" is not run/.test(message)));
106+
});
107+
108+
test('a path filter that excludes a category fails, though the check still runs somewhere', () => {
109+
// Take the category's path from the derivation rather than naming a file, so the
110+
// case keeps exercising the real classification as the tree changes.
111+
const category = categories(base).find((entry) => entry.rule === 'own:daemon-wire-compat');
112+
assert.ok(category, 'the wire ledger must still be a category');
113+
const model = mutate((m) => ({
114+
lanes: mapLane(
115+
m,
116+
(lane) => lane.workflow === 'ci.yml',
117+
(lane) => ({
118+
...lane,
119+
pathsIgnore: [...lane.pathsIgnore, category.path],
120+
}),
121+
),
122+
}));
123+
const found = messages(model);
124+
assert.ok(
125+
found.every((message) => !/is not run by any/.test(message)),
126+
'the checks still run somewhere — only this path stops reaching them',
127+
);
128+
assert.ok(found.some((message) => message.includes(category.path)));
129+
assert.ok(found.some((message) => /selects "daemon-wire-compat"/.test(message)));
130+
});
131+
132+
test('removing the opaque-runner declaration changes the audit, so it is not inert', () => {
133+
const model = mutate(() => ({ opaque: {} }));
134+
const found = messages(model);
135+
assert.ok(found.some((message) => /check "unit" is not run/.test(message)));
136+
assert.ok(found.some((message) => /vitest:unit-core/.test(message)));
137+
});
138+
139+
test('a Vitest project no check runs is reported, and so is a suite script', () => {
140+
const project = mutate((m) => ({
141+
vitestProjects: [...m.vitestProjects, 'new-lane'],
142+
}));
143+
assert.ok(
144+
messages(project).some((message) =>
145+
/Vitest project "new-lane" is run by no registered check/.test(message),
146+
),
147+
);
148+
149+
const script = mutate((m) => ({
150+
scripts: {
151+
...m.scripts,
152+
'test:orphan': 'vitest run --project unit-core --project orphan-only',
153+
},
154+
vitestProjects: [...m.vitestProjects, 'orphan-only'],
155+
}));
156+
assert.ok(
157+
messages(script).some((message) =>
158+
/package script "test:orphan" runs vitest:orphan-only/.test(message),
159+
),
160+
);
161+
});
162+
163+
test('categories are derived from the tracked tree, so no sample can be fictional', () => {
164+
const found = categories(base);
165+
assert.ok(found.length >= 12, `expected the selector's real categories, got ${found.length}`);
166+
for (const category of found) {
167+
assert.ok(tracked.includes(category.path), `${category.path} must be a tracked file`);
168+
assert.ok(category.checks.length > 0, `${category.rule} must select at least one check`);
169+
}
170+
// The #1420 category in particular has to be present, since ci.yml ignores website/**.
171+
const docs = found.find((category) => category.rule === 'own:command-docs');
172+
assert.equal(docs?.path, 'website/docs/docs/commands.md');
173+
assert.deepEqual(docs?.checks, ['command-docs']);
174+
});

0 commit comments

Comments
 (0)