Skip to content

Commit b332dbe

Browse files
cliffhallclaude
andcommitted
fix: pin jobs whose outputs a credentialed job reads; serialize release runs
- verify:action-pins counts a job as credentialed when a credentialed job reads its outputs. release.yml's detect-packages supplies the name and version an artifact must match, so it is now held to the pin too. - release.yml runs one release at a time and never cancels one, so two runs cannot each read the old latest and publish out of order. - The release skill's Python audit counts a server as audited only when pip-audit's output holds a verdict, and lists changesets recursively. - The guard's failure message reads as sentences. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]> Signed-off-by: cliffhall <[email protected]>
1 parent c966532 commit b332dbe

5 files changed

Lines changed: 130 additions & 14 deletions

File tree

‎.claude/skills/release/SKILL.md‎

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -83,22 +83,28 @@ npm audit --audit-level=high
8383
# Python: each server has its own lockfile. `uv` has no audit of its own in the
8484
# version pinned here, so the locked requirements go through pip-audit.
8585
# Exported to a file first: piped straight in, a failed `uv export` hands
86-
# pip-audit empty input, which it reports as clean. The block runs in a
87-
# subshell that exits non-zero if any server could not be exported, so an
88-
# unaudited server cannot pass as an audited one.
86+
# pip-audit empty input, which it reports as clean. And pip-audit exits
87+
# non-zero both when it finds something and when it could not run, so a
88+
# server counts as audited only when its output holds a verdict. The block
89+
# runs in a subshell that exits non-zero if any server was not audited.
8990
(
9091
unaudited=
9192
for s in fetch git time; do
9293
echo "== $s"
93-
REQ=$(mktemp)
94+
REQ=$(mktemp) OUT=$(mktemp)
9495
if (cd "src/$s" && uv export --frozen --no-emit-project --format requirements-txt) > "$REQ" \
9596
&& [ -s "$REQ" ]; then
96-
uvx pip-audit --require-hashes --disable-pip -r "$REQ"
97+
uvx pip-audit --require-hashes --disable-pip -r "$REQ" > "$OUT" 2>&1
98+
cat "$OUT"
99+
grep -qE 'No known vulnerabilities found|Found [0-9]+ known vulnerabilit' "$OUT" || {
100+
echo "AUDIT DID NOT RUN for $s: no verdict in its output" >&2
101+
unaudited="$unaudited $s"
102+
}
97103
else
98104
echo "EXPORT FAILED for $s: nothing was audited" >&2
99105
unaudited="$unaudited $s"
100106
fi
101-
rm -f "$REQ"
107+
rm -f "$REQ" "$OUT"
102108
done
103109
[ -z "$unaudited" ] || { echo "NOT AUDITED:$unaudited" >&2; exit 1; }
104110
); echo "python audit complete: EXIT=$?"
@@ -146,7 +152,7 @@ releases nothing:
146152
# Refresh first, and read the REMOTE tree: the checkout may be a preparation
147153
# branch cut before another PR merged.
148154
git fetch origin main v2/main --tags
149-
git ls-tree --name-only origin/v2/main .changeset/ | grep '\.md$' | grep -v README.md # the pending changesets
155+
git ls-tree -r --name-only origin/v2/main .changeset/ | grep '\.md$' | grep -vx '.changeset/README.md' # the pending changesets
150156
# Since the latest published Release, asked of GitHub: this repo's tags do not
151157
# sort into release order (see 5a).
152158
PREV=$(gh release view --repo modelcontextprotocol/servers --json tagName --jq .tagName)

‎.github/workflows/release.yml‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,16 @@ on:
5757
release:
5858
types: [published]
5959

60+
# One release at a time, and never cancelled. Each publish leg reads the
61+
# registry's `latest`, chooses a dist-tag from it and publishes; two runs doing
62+
# that at once could each see the old `latest` and then publish out of order,
63+
# leaving the lower version tagged `latest`. Queuing the second run behind the
64+
# first makes its read see what the first published. Never cancelled, because
65+
# a run stopped between packages leaves a release half published.
66+
concurrency:
67+
group: release
68+
cancel-in-progress: false
69+
6070
# Nothing in this workflow needs more than read access by default; the two
6171
# publish jobs add `id-token: write` for themselves.
6272
permissions:

‎AGENTS.md‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -162,7 +162,8 @@ than `GITHUB_TOKEN`. Today that is `release.yml`'s publish jobs and
162162
`uses: actions/checkout@<40-hex sha> # v6.1.0`. A tag can be moved, and a
163163
moved tag runs new code next to the credential with no change in this
164164
repository. The same holds for **a job whose artifact a credentialed job
165-
downloads**, since what it builds is what gets published.
165+
downloads**, since what it builds is what gets published, and for **a job
166+
whose outputs a credentialed job reads**, since the reader acts on them.
166167
`verify:action-pins` enforces both. Other jobs keep moving major tags.
167168
- **A job that holds a publish credential does not install dependencies or
168169
build.** `release.yml` builds, tests and verifies each package in a job

‎scripts/verify-action-pins.mjs‎

Lines changed: 36 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,12 @@
3636
// under provenance, so a moved tag in the build job publishes as surely
3737
// as one in the publish job would. (The build was split out of the
3838
// publish job to keep the OIDC token away from dependency installs; this
39-
// keeps a moving tag out of what gets published.)
39+
// keeps a moving tag out of what gets published.); or
40+
// 4. has outputs that a credentialed job reads (`needs.<job>.outputs.…`).
41+
// The reader acts on them next to its credential. Here that is
42+
// `release.yml`'s `detect-packages`: the publish jobs take from it the
43+
// name and version an artifact must match before it is published, so a
44+
// moved tag there could approve a tampered artifact.
4045
//
4146
// `GITHUB_TOKEN` alone does not count: every job holds one, so counting it
4247
// would turn this into "pin everything", which #4873 does not ask. That is why
@@ -199,10 +204,35 @@ export function credentialedJobs(yaml, file) {
199204
}
200205
return seen;
201206
};
207+
// The jobs whose outputs a job reads: every `needs.<job>.outputs` anywhere
208+
// in it. `needs.*.outputs` and a dynamic index name no job, so they count
209+
// every job it needs.
210+
const outputsReadBy = (job) => {
211+
const read = new Set();
212+
for (const text of stringsIn(job)) {
213+
if (!text.includes(EXPRESSION_OPEN)) continue;
214+
for (const m of text.matchAll(
215+
/\bneeds\s*(?:\.\s*([\w-]+|\*)|\[\s*(?:'([^']*)'|"([^"]*)"|[^\]]*)\s*\])\s*(?:\.\s*outputs\b|\[\s*['"]outputs['"]\s*\])/g,
216+
)) {
217+
const named = m[1] ?? m[2] ?? m[3];
218+
if (named === undefined || named === "*")
219+
for (const n of needsOf(job)) read.add(n);
220+
else read.add(named);
221+
}
222+
}
223+
return read;
224+
};
202225
for (let grew = true; grew; ) {
203226
grew = false;
204227
for (const [name, job] of jobs) {
205-
if (!held.has(name) || !mayDownloadArtifact(job)) continue;
228+
if (!held.has(name)) continue;
229+
for (const source of outputsReadBy(job)) {
230+
if (workflow.jobs[source] && !held.has(source)) {
231+
held.add(source);
232+
grew = true;
233+
}
234+
}
235+
if (!mayDownloadArtifact(job)) continue;
206236
for (const producer of upstreamOf(name)) {
207237
if (!held.has(producer) && mayUploadArtifact(workflow.jobs[producer])) {
208238
held.add(producer);
@@ -284,10 +314,10 @@ export function main(root = repoRoot) {
284314
"\n\nA job holding `id-token`/`packages: write`, a non-default secret, or building an" +
285315
"\nartifact such a job consumes runs only immutable refs. Pin each as" +
286316
"\n uses: owner/repo@<40-hex sha> # vX.Y.Z" +
287-
"\nA local `./…` action or workflow is refused there as well: this guard does not" +
288-
"\nread what it runs. Inline its steps, or extend the guard to follow it." +
289-
"\nresolving the SHA and the exact release from the same tag lookup. The comment" +
290-
"\nnames the release the SHA came from, for a reviewer and for the dependency sweep.",
317+
"\nResolve the SHA and the exact release from the same tag lookup. The comment" +
318+
"\nnames the release the SHA came from, for a reviewer and for the dependency sweep." +
319+
"\n\nA local `./…` action or workflow is refused in such a job as well: this guard" +
320+
"\ndoes not read what it runs. Inline its steps, or extend the guard to follow it.",
291321
);
292322
return 1;
293323
}

‎scripts/verify-action-pins.test.mjs‎

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -281,6 +281,75 @@ test("a credentialed reusable-workflow call counts as an artifact downloader", (
281281
]);
282282
});
283283

284+
test("a job whose outputs a credentialed job reads is credentialed", () => {
285+
const yaml = wf(
286+
"jobs:",
287+
" detect:",
288+
" steps:",
289+
" - uses: actions/checkout@v7",
290+
" lint:",
291+
" steps:",
292+
" - uses: actions/checkout@v7",
293+
" publish:",
294+
" needs: [detect, lint]",
295+
" permissions:",
296+
" id-token: write",
297+
" steps:",
298+
" - env:",
299+
" EXPECTED: ${{ fromJson(needs.detect.outputs.expected)[matrix.package].name }}",
300+
" run: publish",
301+
);
302+
// `lint` is needed but nothing of its is read, so it gates the job without
303+
// feeding it.
304+
assert.deepEqual([...credentialedJobs(yaml)].sort(), ["detect", "publish"]);
305+
assert.deepEqual(unpinnedRefs(yaml), [
306+
{ job: "detect", uses: "actions/checkout@v7" },
307+
]);
308+
});
309+
310+
test("outputs read in a job's if, matrix or bracket form count too", () => {
311+
const yaml = wf(
312+
"jobs:",
313+
" a:",
314+
" runs-on: x",
315+
" b:",
316+
" runs-on: x",
317+
" c:",
318+
" runs-on: x",
319+
" publish:",
320+
" needs: [a, b, c]",
321+
" if: ${{ needs.a.outputs.go == 'true' }}",
322+
" permissions:",
323+
" id-token: write",
324+
" strategy:",
325+
" matrix:",
326+
" package: ${{ fromJson(needs['b'].outputs.packages) }}",
327+
" unrelated:",
328+
" needs: [c]",
329+
" steps:",
330+
" - run: echo ${{ needs.c.outputs.x }}",
331+
);
332+
// `c` is read only by a job that holds no credential.
333+
assert.deepEqual([...credentialedJobs(yaml)].sort(), ["a", "b", "publish"]);
334+
});
335+
336+
test("needs.*.outputs counts every job the reader needs", () => {
337+
const yaml = wf(
338+
"jobs:",
339+
" a:",
340+
" runs-on: x",
341+
" b:",
342+
" runs-on: x",
343+
" publish:",
344+
" needs: [a, b]",
345+
" permissions:",
346+
" id-token: write",
347+
" steps:",
348+
" - run: echo ${{ join(needs.*.outputs.v) }}",
349+
);
350+
assert.deepEqual([...credentialedJobs(yaml)].sort(), ["a", "b", "publish"]);
351+
});
352+
284353
test("an upload is not credentialed when the consumer downloads nothing", () => {
285354
const yaml = wf(
286355
"jobs:",

0 commit comments

Comments
 (0)