Skip to content

Commit 13f49cf

Browse files
thecodedriftclaude
andcommitted
test(ci): guard the quoting of single-quoted step outputs
The review workflow writes its three `focus=` strings single-quoted because they contain backticks and `$`, which double quotes would hand to the shell. Single quoting has exactly one failure mode and it is silent: an apostrophe inside the value closes the string early and the rest of the line becomes shell words. A future contraction — "don't", "doesn't", "won't" — would break the step for all three modes at once, and nothing would catch it. The strings are hundreds of characters of prose on one line, and a reviewer reading prose is not reading quoting. `workflow-outputs.cjs` checks every file in `.github/workflows/`: - A single-quoted echo must be `echo 'key=value' >> "$GITHUB_OUTPUT"`, whole, on one line. An odd quote count is reported as unterminated, which covers both the apostrophe and a value wrapped onto the next line. Detection keys off `echo '` rather than off `$GITHUB_OUTPUT`: a wrapped value leaves the redirect on the FOLLOWING line, so keying off the redirect would skip exactly the broken line. My first draft did that and its own test caught it. - Where a file both writes a key and compares it against a literal (`steps.prep.outputs.mode != 'full'`), the literal must be a value the file actually writes. That is the drift the mode/focus split invites. Keys the file never writes — an action's own outputs — are skipped, so there is no ground truth to get wrong. Node builtins only, like the other scripts here, and no YAML parser: the invariant is textual, about what the shell sees on one line, and parsing to a structure would discard the quoting the check is about. Verified by injecting `don't` into the real focus string, which the guard rejects with the file and line. `validate.yml` already globs `.github/scripts/*.test.cjs`, so this runs in CI with no change there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 60ffb75 commit 13f49cf

2 files changed

Lines changed: 300 additions & 0 deletions

File tree

Lines changed: 167 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,167 @@
1+
// SPDX-License-Identifier: MIT
2+
"use strict";
3+
4+
/**
5+
* Guard the shell quoting of workflow step outputs.
6+
*
7+
* `echo 'key=value' >> "$GITHUB_OUTPUT"` is single-quoted for a reason: the
8+
* values it writes here contain backticks and `$`, which double quotes would
9+
* hand to the shell. Single quotes have exactly one failure mode, and it is
10+
* silent — an apostrophe inside the value CLOSES the string early, and the rest
11+
* of the line becomes shell words. In the review workflow's `focus=` strings
12+
* that would mean a future contraction ("don't", "doesn't", "won't") breaking
13+
* the step for every mode at once, with nothing in review to catch it: the
14+
* strings are hundreds of characters of prose on one line, and a reviewer
15+
* reading prose is not reading quoting.
16+
*
17+
* This is a textual property of hand-written YAML — the invariant IS "what the
18+
* shell sees on this line" — so it is checked by reading the lines, not by
19+
* parsing the YAML into a structure that has already discarded the quoting.
20+
* Node builtins only, like every other script here.
21+
*
22+
* Two rules, both applied to every file in `.github/workflows/`:
23+
*
24+
* 1. A single-quoted `echo` that writes to `$GITHUB_OUTPUT` must be exactly
25+
* `echo 'key=value' >> "$GITHUB_OUTPUT"`, with no apostrophe inside the
26+
* value and nothing after the closing quote but the redirect. This catches
27+
* the apostrophe, an unterminated string, and a value wrapped onto a
28+
* second line ($GITHUB_OUTPUT is line-oriented; a multi-line value needs
29+
* heredoc syntax and is a different thing entirely).
30+
*
31+
* 2. Where a file both WRITES a key's values and COMPARES that key against a
32+
* literal — `steps.prep.outputs.mode != 'full'` — the literal must be one
33+
* of the values written. That is the drift the mode/focus split invites:
34+
* two independently-maintained places encoding the same fact, where
35+
* renaming one leaves a gate that silently never matches. Keys the file
36+
* does not write with a single-quoted echo (an action's own outputs, say)
37+
* are skipped — there is no ground truth for those here.
38+
*/
39+
40+
const { readFileSync, readdirSync } = require("node:fs");
41+
const { join } = require("node:path");
42+
43+
/**
44+
* `echo 'key=value' >> "$GITHUB_OUTPUT"`, whole and correctly quoted.
45+
*
46+
* `[^']*` is what does the work: an apostrophe in the value ends the capture
47+
* early, and the literal `' >> "$GITHUB_OUTPUT"` that must follow then fails to
48+
* match, so the line is reported rather than silently accepted.
49+
*/
50+
const OUTPUT_LINE =
51+
/^echo '([A-Za-z_][A-Za-z0-9_]*)=([^']*)' >> "\$GITHUB_OUTPUT"$/;
52+
53+
/**
54+
* A single-quoted echo, correct or not.
55+
*
56+
* Deliberately NOT "…and mentions $GITHUB_OUTPUT": a value wrapped onto a
57+
* second line leaves the redirect on the line below, so keying off the redirect
58+
* would skip exactly the malformed line it needs to see.
59+
*/
60+
const OUTPUT_LINE_CANDIDATE = /^echo '/;
61+
62+
/** `steps.<id>.outputs.<key> == 'literal'` (or `!=`), in an `if:` or anywhere. */
63+
const OUTPUT_COMPARISON =
64+
/steps\.[A-Za-z0-9_-]+\.outputs\.([A-Za-z0-9_]+)\s*[!=]=\s*'([^']*)'/g;
65+
66+
/**
67+
* Check one workflow file's source. Returns a list of human-readable problems;
68+
* an empty list means the file is fine.
69+
*/
70+
function checkWorkflowSource(source, name) {
71+
const errors = [];
72+
/** @type {Map<string, Set<string>>} key → every value written for it */
73+
const written = new Map();
74+
75+
const lines = source.split("\n");
76+
for (const [index, raw] of lines.entries()) {
77+
const line = raw.trim();
78+
if (!OUTPUT_LINE_CANDIDATE.test(line)) continue;
79+
80+
const where = `${name}:${String(index + 1)}`;
81+
const expected = `Expected echo 'key=value' >> "$GITHUB_OUTPUT" on one line, with no apostrophe in the value.`;
82+
83+
// An odd number of quotes means the string never closed on this line —
84+
// either an apostrophe inside the value (which closed it early, leaving the
85+
// rest as shell words) or a value wrapped onto the next line.
86+
const quotes = (line.match(/'/g) ?? []).length;
87+
if (quotes % 2 === 1) {
88+
errors.push(
89+
`${where}: unterminated single-quoted string — an apostrophe in the ` +
90+
`value, or a value continued on the next line. ${expected} Got: ${line}`
91+
);
92+
continue;
93+
}
94+
95+
// A balanced line that is not writing an output is none of our business:
96+
// `echo 'threads: 3'` into the log is fine.
97+
if (!line.includes("$GITHUB_OUTPUT")) continue;
98+
99+
const match = OUTPUT_LINE.exec(line);
100+
if (match === null) {
101+
errors.push(`${where}: malformed step output. ${expected} Got: ${line}`);
102+
continue;
103+
}
104+
105+
const [, key, value] = match;
106+
const values = written.get(key) ?? new Set();
107+
values.add(value);
108+
written.set(key, values);
109+
}
110+
111+
for (const match of source.matchAll(OUTPUT_COMPARISON)) {
112+
const [, key, literal] = match;
113+
const values = written.get(key);
114+
// No ground truth for a key this file never writes — an action's own
115+
// output, or one written by a script rather than an inline echo.
116+
if (values === undefined) continue;
117+
if (values.has(literal)) continue;
118+
errors.push(
119+
`${name}: compares steps output '${key}' against '${literal}', which ` +
120+
`this file never writes. Written: ${[...values].sort().join(", ")}.`
121+
);
122+
}
123+
124+
return errors;
125+
}
126+
127+
/** Check every workflow in `directory`. */
128+
function checkWorkflows(directory) {
129+
const errors = [];
130+
const files = readdirSync(directory)
131+
.filter((f) => f.endsWith(".yml") || f.endsWith(".yaml"))
132+
.sort();
133+
134+
for (const file of files) {
135+
const source = readFileSync(join(directory, file), "utf8");
136+
errors.push(...checkWorkflowSource(source, `.github/workflows/${file}`));
137+
}
138+
139+
return { errors, checked: files.length };
140+
}
141+
142+
function main(directory = ".github/workflows") {
143+
const { errors, checked } = checkWorkflows(directory);
144+
145+
for (const error of errors) {
146+
console.error(`::error::${error}`);
147+
}
148+
149+
if (errors.length > 0) {
150+
console.error("");
151+
console.error(
152+
`${String(errors.length)} step-output problem(s) in ${String(checked)} workflow file(s).`
153+
);
154+
return 1;
155+
}
156+
157+
console.log(
158+
`Step outputs are correctly quoted in ${String(checked)} workflow file(s).`
159+
);
160+
return 0;
161+
}
162+
163+
module.exports = { checkWorkflowSource, checkWorkflows, main };
164+
165+
if (require.main === module) {
166+
process.exit(main(process.argv[2]));
167+
}
Lines changed: 133 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,133 @@
1+
// SPDX-License-Identifier: MIT
2+
"use strict";
3+
4+
/**
5+
* Tests for workflow-outputs.cjs — the guard on single-quoted step outputs.
6+
*
7+
* Most cases run the checker over an inline workflow fragment. The last one
8+
* runs it over the committed workflows, which is the point of the check: it is
9+
* the repository's own `focus=` strings it exists to protect.
10+
*/
11+
12+
const test = require("node:test");
13+
const assert = require("node:assert/strict");
14+
15+
const {
16+
checkWorkflowSource,
17+
checkWorkflows,
18+
} = require("./workflow-outputs.cjs");
19+
20+
const wrap = (...lines) =>
21+
[
22+
"jobs:",
23+
" a:",
24+
" steps:",
25+
" - run: |",
26+
...lines.map((l) => ` ${l}`),
27+
].join("\n");
28+
29+
test("accepts a correctly quoted output", () => {
30+
const errors = checkWorkflowSource(
31+
wrap("echo 'mode=full' >> \"$GITHUB_OUTPUT\""),
32+
"w.yml"
33+
);
34+
assert.deepEqual(errors, []);
35+
});
36+
37+
test("accepts a value containing backticks and dollars", () => {
38+
// The reason these are single-quoted in the first place.
39+
const errors = checkWorkflowSource(
40+
wrap(
41+
"echo 'focus=Run `gh pr diff` and read $HOME first.' >> \"$GITHUB_OUTPUT\""
42+
),
43+
"w.yml"
44+
);
45+
assert.deepEqual(errors, []);
46+
});
47+
48+
test("rejects an apostrophe inside the value", () => {
49+
// The failure this guard exists for: the quote closes at "don" and the rest
50+
// of the line becomes shell words.
51+
const errors = checkWorkflowSource(
52+
wrap(
53+
"echo 'focus=Review this PR, but don't run the tests.' >> \"$GITHUB_OUTPUT\""
54+
),
55+
"w.yml"
56+
);
57+
assert.equal(errors.length, 1);
58+
assert.match(errors[0], /unterminated single-quoted string/);
59+
assert.match(errors[0], /w\.yml:5/);
60+
});
61+
62+
test("rejects a value wrapped onto a second line", () => {
63+
// $GITHUB_OUTPUT is line-oriented; a multi-line value needs heredoc syntax.
64+
// The redirect ends up on the FOLLOWING line, so a check keyed off
65+
// `$GITHUB_OUTPUT` would never look at the line that is actually broken.
66+
const errors = checkWorkflowSource(
67+
wrap("echo 'focus=First half", 'second half\' >> "$GITHUB_OUTPUT"'),
68+
"w.yml"
69+
);
70+
assert.equal(errors.length, 1);
71+
assert.match(errors[0], /unterminated single-quoted string/);
72+
assert.match(errors[0], /w\.yml:5/);
73+
});
74+
75+
test("rejects trailing content after the redirect", () => {
76+
const errors = checkWorkflowSource(
77+
wrap("echo 'mode=full' >> \"$GITHUB_OUTPUT\" && echo done"),
78+
"w.yml"
79+
);
80+
assert.equal(errors.length, 1);
81+
assert.match(errors[0], /malformed step output/);
82+
});
83+
84+
test("ignores double-quoted echoes, which may legitimately hold apostrophes", () => {
85+
const errors = checkWorkflowSource(
86+
wrap('echo "pr=${{ github.event.issue.number }}" >> "$GITHUB_OUTPUT"'),
87+
"w.yml"
88+
);
89+
assert.deepEqual(errors, []);
90+
});
91+
92+
test("ignores a single-quoted echo that is not a step output", () => {
93+
const errors = checkWorkflowSource(wrap("echo 'threads: none'"), "w.yml");
94+
assert.deepEqual(errors, []);
95+
});
96+
97+
test("accepts a comparison against a value the file writes", () => {
98+
const source = [
99+
wrap(
100+
"echo 'mode=full' >> \"$GITHUB_OUTPUT\"",
101+
"echo 'mode=incremental' >> \"$GITHUB_OUTPUT\""
102+
),
103+
" - if: steps.prep.outputs.mode != 'full'",
104+
].join("\n");
105+
assert.deepEqual(checkWorkflowSource(source, "w.yml"), []);
106+
});
107+
108+
test("rejects a comparison against a value nothing writes", () => {
109+
// The drift case: the gate was left behind when the written value changed.
110+
const source = [
111+
wrap(
112+
"echo 'mode=full-review' >> \"$GITHUB_OUTPUT\"",
113+
"echo 'mode=incremental' >> \"$GITHUB_OUTPUT\""
114+
),
115+
" - if: steps.prep.outputs.mode != 'full'",
116+
].join("\n");
117+
const errors = checkWorkflowSource(source, "w.yml");
118+
assert.equal(errors.length, 1);
119+
assert.match(errors[0], /compares steps output 'mode' against 'full'/);
120+
assert.match(errors[0], /Written: full-review, incremental/);
121+
});
122+
123+
test("skips a comparison for a key the file never writes", () => {
124+
// An action's own output — no ground truth here, so no opinion.
125+
const source = " - if: steps.detect.outputs.needed == 'true'\n";
126+
assert.deepEqual(checkWorkflowSource(source, "w.yml"), []);
127+
});
128+
129+
test("the committed workflows pass", () => {
130+
const { errors, checked } = checkWorkflows(".github/workflows");
131+
assert.deepEqual(errors, []);
132+
assert.ok(checked > 0, "expected to find workflow files");
133+
});

0 commit comments

Comments
 (0)