Skip to content

Commit 7d735af

Browse files
theCodeDriftclaude
andcommitted
fix(vale): make detect survive the case it exists for
The detect job routed its cheap "is upstream ahead?" check through planManifestUpdate with an empty checksums payload. That function only ignores checksumsText on the NOT-ahead path, so the placeholder made it throw ("parsed to no entries") on exactly the runs with a release to propose: every real upstream bump failed before the checksums URL was ever resolved, and only the no-op path passed. Call isUpstreamAhead/parseReleaseTag directly for the cheap check and call planManifestUpdate once, with the real checksums. Every function involved was already green in isolation, so the bug lived purely in main()'s composition — vale-detect.test.cjs now covers that by running main() with both fetches stubbed. Also from review: - The publish loop no longer aborts at the first failure. Six sequential publishes are six chances at a transient registry error, and stopping midway leaves the set partially released, which is the one state the CLI's exact cross-package pins cannot tolerate. It now attempts all six, skips any already published at this stamped version (making a re-run idempotent rather than fatal), and fails at the end naming the stragglers. - assertManifest requires archiveMember to be a flat filename. unpackMember's containment checks run after extraction and cover the leaf entry only, so a nested member could have a symlinked intermediate directory followed by tar before there is a path to inspect. Removing the intermediate component is the guarantee; GNU tar's own refusal is not ours to rely on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jwc9FFroR3mTZ4hLiSkkX3
1 parent 43d2972 commit 7d735af

6 files changed

Lines changed: 249 additions & 21 deletions

File tree

‎.github/scripts/vale-detect.cjs‎

Lines changed: 34 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,8 @@ const { join } = require("node:path");
4040
const {
4141
applyTemplate,
4242
assertManifest,
43+
isUpstreamAhead,
44+
parseReleaseTag,
4345
planManifestUpdate,
4446
resolveChecksumsUrl,
4547
} = require("./vale-release.cjs");
@@ -86,36 +88,41 @@ async function fetchText(url) {
8688
return response.text();
8789
}
8890

89-
async function main() {
90-
const write = process.argv.slice(2).includes("--write");
91+
async function main({
92+
argv = process.argv.slice(2),
93+
latestTag = fetchLatestTag,
94+
text = fetchText,
95+
} = {}) {
96+
const write = argv.includes("--write");
9197
const manifest = assertManifest(
9298
JSON.parse(readFileSync(MANIFEST_PATH, "utf8"))
9399
);
94100

95-
const upstreamTag = await fetchLatestTag(manifest.upstream.repository);
101+
const upstreamTag = await latestTag(manifest.upstream.repository);
96102
console.log(
97103
`pinned: ${manifest.valeVersion} upstream latest: ${upstreamTag}`
98104
);
99105

100-
// Probe first with an empty checksums file. When upstream is not ahead the
101-
// plan short-circuits and never looks at it, so the checksums file is only
102-
// downloaded for a release we are actually going to propose.
103-
const probe = planManifestUpdate({
104-
manifest,
105-
upstreamTag,
106-
checksumsText: "",
107-
});
108-
if (!probe.update) {
106+
// Decide whether to go on with the two pure predicates directly, rather than
107+
// by calling planManifestUpdate with a placeholder checksums payload. That
108+
// shortcut looks equivalent but inverts the script: planManifestUpdate only
109+
// ignores `checksumsText` on the NOT-ahead path, so a stand-in empty string
110+
// makes it throw ("parsed to no entries") on exactly the runs that have
111+
// something to propose. The cheap check has to be the cheap check.
112+
const upstreamVersion = parseReleaseTag(upstreamTag);
113+
if (!isUpstreamAhead(manifest.valeVersion, upstreamVersion)) {
109114
console.log("Upstream is not ahead of the pinned version. Nothing to do.");
110115
setOutput("update", "false");
111-
setOutput("vale_version", probe.upstreamVersion);
116+
setOutput("vale_version", upstreamVersion);
112117
setOutput("pinned_version", manifest.valeVersion);
113118
return;
114119
}
115120

116-
const checksumsUrl = resolveChecksumsUrl(manifest, probe.upstreamVersion);
121+
// Only now is the checksums file worth downloading: it belongs to a release
122+
// we are actually going to propose.
123+
const checksumsUrl = resolveChecksumsUrl(manifest, upstreamVersion);
117124
console.log(`fetching ${checksumsUrl}`);
118-
const checksumsText = await fetchText(checksumsUrl);
125+
const checksumsText = await text(checksumsUrl);
119126

120127
const plan = planManifestUpdate({ manifest, upstreamTag, checksumsText });
121128
console.log(
@@ -139,7 +146,15 @@ async function main() {
139146
setOutput("pinned_version", plan.pinnedVersion);
140147
}
141148

142-
main().catch((error) => {
143-
console.error(`\nvale-detect failed: ${error.message}`);
144-
process.exitCode = 1;
145-
});
149+
// Exported (and only self-invoking as a script) so vale-detect.test.cjs can run
150+
// main() with the two fetches stubbed. The bug that motivated this was in the
151+
// composition — how main() sequences pure functions that were each already
152+
// tested — which is reachable no other way.
153+
module.exports = { main };
154+
155+
if (require.main === module) {
156+
main().catch((error) => {
157+
console.error(`\nvale-detect failed: ${error.message}`);
158+
process.exitCode = 1;
159+
});
160+
}
Lines changed: 136 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,136 @@
1+
// SPDX-License-Identifier: MIT
2+
"use strict";
3+
4+
/**
5+
* Composition tests for vale-detect.cjs.
6+
*
7+
* vale-release.test.cjs covers the pure functions individually. This file
8+
* covers the one thing that cannot: how main() sequences them, with both
9+
* network calls stubbed. That gap is not hypothetical — the detect job once
10+
* routed its cheap "is upstream ahead?" check through planManifestUpdate with
11+
* an empty checksums payload, which throws on precisely the ahead path, so the
12+
* job failed on every run that had a release to propose while the no-op path
13+
* kept passing. Every function involved was green in isolation.
14+
*
15+
* Nothing here writes: main() is called without `--write`, so the committed
16+
* manifest is only read.
17+
*/
18+
19+
const test = require("node:test");
20+
const assert = require("node:assert/strict");
21+
const { mkdtempSync, readFileSync, rmSync } = require("node:fs");
22+
const { tmpdir } = require("node:os");
23+
const { join } = require("node:path");
24+
25+
const { main } = require("./vale-detect.cjs");
26+
const { applyTemplate } = require("./vale-release.cjs");
27+
28+
const MANIFEST = JSON.parse(
29+
readFileSync(join(__dirname, "vale-manifest.json"), "utf8")
30+
);
31+
32+
/** A digest that is syntactically valid and obviously synthetic. */
33+
const digestFor = (index) =>
34+
String(index + 1)
35+
.repeat(64)
36+
.slice(0, 64);
37+
38+
/** Upstream's sha256sum-format checksums file for a given Vale version. */
39+
function checksumsFor(version) {
40+
return `${MANIFEST.platforms
41+
.map(
42+
(platform, index) =>
43+
`${digestFor(index)} ${applyTemplate(platform.asset, { version })}`
44+
)
45+
.join("\n")}\n`;
46+
}
47+
48+
/**
49+
* Run main() with both fetches stubbed and $GITHUB_OUTPUT pointed at a temp
50+
* file, then return the parsed step outputs plus which URLs were fetched.
51+
*/
52+
async function runDetect({ upstreamTag, checksums }) {
53+
const directory = mkdtempSync(join(tmpdir(), "vale-detect-test-"));
54+
const outputPath = join(directory, "github-output");
55+
const previous = process.env.GITHUB_OUTPUT;
56+
const fetched = [];
57+
process.env.GITHUB_OUTPUT = outputPath;
58+
try {
59+
await main({
60+
argv: [],
61+
latestTag: async () => upstreamTag,
62+
text: async (url) => {
63+
fetched.push(url);
64+
return checksums;
65+
},
66+
});
67+
const outputs = Object.fromEntries(
68+
readFileSync(outputPath, "utf8")
69+
.split("\n")
70+
.filter(Boolean)
71+
.map((line) => {
72+
const at = line.indexOf("=");
73+
return [line.slice(0, at), line.slice(at + 1)];
74+
})
75+
);
76+
return { outputs, fetched };
77+
} finally {
78+
if (previous === undefined) {
79+
delete process.env.GITHUB_OUTPUT;
80+
} else {
81+
process.env.GITHUB_OUTPUT = previous;
82+
}
83+
rmSync(directory, { recursive: true, force: true });
84+
}
85+
}
86+
87+
test("detect: an upstream release ahead of the pin plans an update", async () => {
88+
// The regression case. Before the fix this rejected with "upstream checksums
89+
// file for 3.99.0 parsed to no entries" — the detect job's failure mode on
90+
// every real upstream bump.
91+
const { outputs, fetched } = await runDetect({
92+
upstreamTag: "v3.99.0",
93+
checksums: checksumsFor("3.99.0"),
94+
});
95+
96+
assert.equal(outputs.update, "true");
97+
assert.equal(outputs.vale_version, "3.99.0");
98+
assert.equal(outputs.pinned_version, MANIFEST.valeVersion);
99+
assert.deepEqual(fetched, [
100+
`https://github.com/${MANIFEST.upstream.repository}/releases/download/v3.99.0/vale_3.99.0_checksums.txt`,
101+
]);
102+
});
103+
104+
test("detect: the pinned version being current is a no-op", async () => {
105+
const { outputs, fetched } = await runDetect({
106+
upstreamTag: `v${MANIFEST.valeVersion}`,
107+
checksums: "",
108+
});
109+
110+
assert.equal(outputs.update, "false");
111+
assert.equal(outputs.vale_version, MANIFEST.valeVersion);
112+
assert.equal(outputs.pinned_version, MANIFEST.valeVersion);
113+
// The whole reason the check is cheap: no checksums file is downloaded for a
114+
// release we are not going to propose.
115+
assert.deepEqual(fetched, []);
116+
});
117+
118+
test("detect: an upstream tag behind the pin is also a no-op", async () => {
119+
const { outputs, fetched } = await runDetect({
120+
upstreamTag: "v0.1.0",
121+
checksums: "",
122+
});
123+
124+
assert.equal(outputs.update, "false");
125+
assert.deepEqual(fetched, []);
126+
});
127+
128+
test("detect: a checksums file missing a platform aborts", async () => {
129+
await assert.rejects(
130+
runDetect({
131+
upstreamTag: "v3.99.0",
132+
checksums: checksumsFor("3.99.0").split("\n").slice(1).join("\n"),
133+
}),
134+
/publishes no asset named/
135+
);
136+
});

‎.github/scripts/vale-prepare.cjs‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -165,6 +165,18 @@ async function download(url, destinationPath) {
165165
* is a regular file rather than a symlink or directory. Without those, an
166166
* archive carrying a symlink at the member's name would put a dangling link, or
167167
* a link to a host path, into the published tarball.
168+
*
169+
* Both checks run after extraction, so they cover the member's LEAF entry and
170+
* nothing above it. For a nested member the escape to worry about is an
171+
* intermediate directory component that is itself a symlink out of the temp
172+
* directory, which `tar` would follow while writing — before this function sees
173+
* a path to inspect. What rules that out is upstream of here: `assertManifest`
174+
* requires every `archiveMember` to be a flat single-segment filename, so there
175+
* is no intermediate component to subvert. (GNU tar also refuses by default to
176+
* follow a symlink when creating an implied directory, but that is the
177+
* extractor's behavior, not this code's guarantee.) A future manifest entry
178+
* needing a nested member would have to relax that assertion, and the checks
179+
* below will not substitute for it.
168180
*/
169181
function unpackMember(archivePath, member, destinationPath) {
170182
const workDirectory = mkdtempSync(join(tmpdir(), "vale-unpack-"));

‎.github/scripts/vale-release.cjs‎

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -251,7 +251,9 @@ function rangeMatches(range, version) {
251251
// pins the minor, and `^0.0.z` desugars to `>=0.0.z <0.0.(z+1)`, which is
252252
// the single version itself.
253253
if (rangeParts.minor === 0) {
254-
return versionParts.minor === 0 && versionParts.patch === rangeParts.patch;
254+
return (
255+
versionParts.minor === 0 && versionParts.patch === rangeParts.patch
256+
);
255257
}
256258
return versionParts.minor === rangeParts.minor;
257259
}
@@ -313,6 +315,22 @@ function assertManifest(manifest) {
313315
);
314316
}
315317
}
318+
// The containment checks in vale-prepare.cjs's unpackMember run after
319+
// extraction, so they cover the member's LEAF entry only: an intermediate
320+
// directory component that is itself a symlink pointing out of the unpack
321+
// directory would be followed by tar while writing, before there is any
322+
// path to inspect. Requiring a flat filename deletes that case rather than
323+
// leaving it to the extractor's own symlink refusal — and keeps the
324+
// requirement here, where a hand-edited manifest is rejected up front,
325+
// instead of as a comment someone has to notice.
326+
if (
327+
/[/\\]/.test(platform.archiveMember) ||
328+
platform.archiveMember === ".."
329+
) {
330+
throw new Error(
331+
`manifest platform ${platform.package} has an archiveMember that is not a flat filename: ${platform.archiveMember}`
332+
);
333+
}
316334
if (!SHA256_PATTERN.test(platform.sha256)) {
317335
throw new Error(
318336
`manifest platform ${platform.package} has a sha256 that is not 64 lowercase hex characters`

‎.github/scripts/vale-release.test.cjs‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -320,6 +320,22 @@ test("assertManifest: rejects a missing field, a bad digest, and a duplicate", (
320320
assert.throws(() => assertManifest(noUpstream), /upstream\.downloadUrl/);
321321
});
322322

323+
test("assertManifest: an archiveMember must be a flat filename", () => {
324+
// unpackMember's containment checks run after extraction and only cover the
325+
// leaf entry, so a nested member would let a symlinked intermediate
326+
// directory be followed by tar before there is a path to inspect. Keeping
327+
// members flat is what removes that case, so it is asserted, not assumed.
328+
for (const member of ["bin/vale", "../vale", "..", "a\\vale.exe"]) {
329+
const nested = fixtureManifest();
330+
nested.platforms[0].archiveMember = member;
331+
assert.throws(() => assertManifest(nested), /not a flat filename/);
332+
}
333+
334+
const flat = fixtureManifest();
335+
flat.platforms[0].archiveMember = "vale.exe";
336+
assert.equal(assertManifest(flat), flat);
337+
});
338+
323339
test("resolve*: templates expand against the pinned version", () => {
324340
const manifest = fixtureManifest();
325341
const [linux] = manifest.platforms;

‎.github/workflows/vale-binaries.yml‎

Lines changed: 32 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -260,8 +260,39 @@ jobs:
260260
exit 1
261261
fi
262262
echo "Publishing ${#tarballs[@]} package(s) at ${STAMPED_VERSION}."
263+
264+
# The loop deliberately does not stop at the first failure, and the
265+
# step deliberately does not lean on `set -e` here. Six sequential
266+
# publishes are six chances for a transient registry error, and
267+
# aborting midway leaves the set partially released — some platforms
268+
# resolvable at this version, others not, which is the one state the
269+
# CLI's exact cross-package pins cannot tolerate. Attempting all six
270+
# and failing at the end means one re-run has at most the stragglers
271+
# left to do.
272+
#
273+
# That re-run is what the `npm view` guard is for: a tarball is
274+
# immutable and its version is stamped once, in prepare, so a package
275+
# already at this version was published by an earlier attempt of this
276+
# same release. Skipping it is idempotent, where re-publishing would
277+
# fail with "cannot publish over the previously published version"
278+
# and strand every package after it.
279+
failed=()
263280
for tarball in "${tarballs[@]}"; do
264281
echo "::group::$tarball"
265-
npm publish --provenance --access public --tag latest "./$tarball"
282+
name="$(tar -xzOf "$tarball" package/package.json | node -e 'let raw = ""; process.stdin.on("data", (chunk) => { raw += chunk; }).on("end", () => { console.log(JSON.parse(raw).name); });')"
283+
if npm view "${name}@${STAMPED_VERSION}" version >/dev/null 2>&1; then
284+
echo "${name}@${STAMPED_VERSION} is already published — skipping."
285+
elif ! npm publish --provenance --access public --tag latest "./$tarball"; then
286+
echo "::error::failed to publish ${name}@${STAMPED_VERSION}"
287+
failed+=("$name")
288+
fi
266289
echo "::endgroup::"
267290
done
291+
292+
if [ "${#failed[@]}" -ne 0 ]; then
293+
echo "Failed to publish ${#failed[@]} of ${#tarballs[@]} package(s) at ${STAMPED_VERSION}:" >&2
294+
printf ' %s\n' "${failed[@]}" >&2
295+
echo "Re-run this job to retry them; already-published packages are skipped." >&2
296+
exit 1
297+
fi
298+
echo "All ${#tarballs[@]} package(s) are published at ${STAMPED_VERSION}."

0 commit comments

Comments
 (0)