feat(vale): fetch, verify, and publish the Vale platform packages - #86
Conversation
Unit 2 of a forward-merging stack. Unit 1 added six empty packages; this adds the pipeline that fills and publishes them, plus the scripts it runs and their tests. Nothing is consumed yet: the CLI pin is unit 3. Two phases, because the trust boundary is code review. `detect` runs on a weekly schedule with no npm credential and no OIDC identity, compares upstream Vale against the pinned version, and opens a pull request carrying the new version and all six digests taken from upstream's own checksums file. `publish` runs on the push to main that merges it. Splitting them is what makes the automation worth trusting: a single job that discovered a digest and then verified downloads against the digest it had just discovered would verify nothing. What bounds a run is the upstream comparison alone. The "is this version already on npm?" check release.yml uses cannot work here, since every publish stamps <valeVersion>-<yyyymmddhhmmss>, a version npm has never seen, so that check would answer "not published" every time and could never suppress anything. The publish phase is split again into prepare and publish. `prepare` downloads third-party bytes off the internet, so it holds contents: read, no environment, and no id-token, and cannot publish or mint a token no matter what it downloads. It verifies every archive against the committed digest and aborts before anything is unpacked on a mismatch. The credentialed `publish` job only ever sees bytes that already matched a reviewed digest, and it does not even check out the repository. Packing happens before the artifact upload because actions/upload-artifact does not preserve file modes and the Vale executable has to reach npm executable. `npm pack` records modes inside the .tgz, so shipping the tarball through the artifact keeps 0755 intact end to end. Merging this publishes nothing. The publish trigger is a path filter on the manifest, which this branch does not touch, and unit 1 added the manifest without a workflow to fire. The first publish is always deliberate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
thecodedrift
added
the
skip-changeset
…-packages-2-release * openspec/add-vale-binary-packages: chore: grow the changeset with the committed sg config docs(openspec): make cli-check internally consistent; correct the generateSgConfig note test(cli): isolate the missing-binary test from the host environment docs(openspec): archive partition-rules-by-engine ref(cli): declare COMMITTED_SG_CONFIG beside the layout it derives from docs(cli): describe the engine-partitioned layout in help and scaffolding feat(cli): run ast-grep against the committed sg config chore: grow the changeset with the runtime discovery path docs(openspec): correct the group-1 note, which no longer describes reality ref(cli): retire layout.ts now that ENGINE_LAYOUTS supersedes it feat(cli): discover runtime rules under the runtime engine directory
There was a problem hiding this comment.
Pull request overview
Adds the automated “detect upstream → review manifest diff → publish verified binaries” pipeline for the six @taskless/vale-<platform> packages, including the zero-dependency release logic + tests, and a new GitHub Actions workflow that implements the two-phase trust boundary described in the PR.
Changes:
- Adds
.github/workflows/vale-binaries.ymlwith separatedetect, credential-freeprepare, and credentialedpublishjobs. - Introduces pure release logic (
vale-release.cjs) plus unit tests, and I/O entrypoints (vale-detect.cjs,vale-prepare.cjs) to fetch/verify/stamp/pack. - Updates
.gitignoreto exclude local pack output in.vale-dist/.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| openspec/changes/add-vale-binary-packages/tasks.md | Marks tasks for fetch/verify/stamp/workflow and quality gates as completed. |
| .gitignore | Ignores .vale-dist/ output from local npm pack runs. |
| .github/workflows/vale-binaries.yml | New two-phase workflow: detect upstream release → prepare verified tarballs → publish via OIDC trusted publishing. |
| .github/scripts/vale-release.test.cjs | Unit tests covering stamping/versioning, manifest rewrite planning, checksum parsing, and digest assertion logic. |
| .github/scripts/vale-release.cjs | Pure, dependency-free release logic (version parsing/stamping, manifest validation, digest comparison, detect planning). |
| .github/scripts/vale-prepare.cjs | Fetch/verify/unpack/stamp/pack script used by the credential-free prepare job (and for manual bootstrap). |
| .github/scripts/vale-detect.cjs | Detect script that reads upstream latest release + checksums and rewrites the pinned manifest for review. |
…-packages-2-release * openspec/add-vale-binary-packages: fix(cli): size the vitest timeout to what the suite actually does
Three review findings from the platform-package release scripts: - rangeMatches treated every `^0.0.x` as in-range for any `0.0.y`. Semver desugars `^0.0.1` to `>=0.0.1 <0.0.2`, so the patch is pinned too. - vale-prepare defaulted a missing `--out` / `--only` value to the empty string, which resolved `--out` to the current working directory instead of failing on the typo. - unpackMember copied whatever landed at the member path. It now requires the resolved path to stay inside the temp directory and to be a regular file, so a symlink or traversal entry in third-party archive bytes cannot reach the published tarball. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@claude /review |
|
Claude finished @thecodedrift's task in 19m 8s —— View job Claude finished reviewing @thecodedrift's PRTasks
Overall assessmentThe two-phase trust-boundary design (credential-free However, there is one bug that breaks the feature's core purpose, found while tracing
Two lower-severity, non-blocking observations posted inline:
Nothing else stood out on security, performance, or clarity — the shell/Action-expression injection surface is well-handled (all interpolated values are validated by pure functions before hitting |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| const probe = planManifestUpdate({ | ||
| manifest, | ||
| upstreamTag, | ||
| checksumsText: "", | ||
| }); |
There was a problem hiding this comment.
Bug: the detect job throws (fails) on every run where upstream actually is ahead — the one case this workflow exists to handle.
planManifestUpdate (.github/scripts/vale-release.cjs:120-150 ish, planManifestUpdate) only skips looking at checksumsText when upstream is not ahead. When upstream is ahead, it falls through to parseChecksumsFile(checksumsText), and when that parses to zero entries it throws:
const digests = parseChecksumsFile(checksumsText);
if (digests.size === 0) {
throw new Error(
`upstream checksums file for ${upstreamVersion} parsed to no entries`
);
}This probe call passes checksumsText: "" unconditionally. parseChecksumsFile("") returns an empty Map, so whenever isUpstreamAhead is true, this probe throws instead of returning { update: true, ... }. The throw is uncaught here and propagates to the top-level main().catch(), which sets process.exitCode = 1 — before line 116's resolveChecksumsUrl(manifest, probe.upstreamVersion) and the real checksums fetch ever run.
Net effect: the scheduled detect job fails with a caught top-level error on every real upstream bump, no update/vale_version outputs are set, and the "Open the manifest update PR" step (vale-binaries.yml:132-133, gated on steps.detect.outputs.update == 'true') never fires. Only the no-op path (upstream already pinned) succeeds — the inverse of what the feature needs to do.
This is also why it slipped past vale-release.test.cjs: that suite exercises planManifestUpdate directly and even has a test proving it throws on ahead + unparseable checksums ("an unparseable checksums file aborts"), but there's no test for vale-detect.cjs's own main() composition, so the probe's checksumsText: "" sentinel was never exercised against a real "upstream ahead" case.
Suggested fix: don't route the "is upstream ahead" check through planManifestUpdate with a fake checksums payload. Call isUpstreamAhead(manifest.valeVersion, parseReleaseTag(upstreamTag)) directly (both already exported from vale-release.cjs) to decide whether to fetch checksums, then call planManifestUpdate exactly once with the real checksumsText.
| shopt -s nullglob | ||
| tarballs=(*.tgz) | ||
| if [ "${#tarballs[@]}" -eq 0 ]; then | ||
| echo "No tarballs in the artifact — refusing to report success." >&2 | ||
| exit 1 | ||
| fi | ||
| echo "Publishing ${#tarballs[@]} package(s) at ${STAMPED_VERSION}." | ||
| for tarball in "${tarballs[@]}"; do | ||
| echo "::group::$tarball" | ||
| npm publish --provenance --access public --tag latest "./$tarball" |
There was a problem hiding this comment.
Minor operational-resilience gap: this loop runs under the default bash -e -o pipefail step shell, so if npm publish fails partway through the six tarballs (network blip, npm registry hiccup, one trusted-publisher not yet registered), the loop aborts immediately and the remaining packages are never attempted — a partial release across the six platform packages.
Re-running the workflow doesn't cleanly recover either: prepare re-downloads/re-verifies/re-stamps/re-packs (same stamped version, since it's derived from the manifest + a fresh timestamp — actually a new timestamp each run, so this specific retry concern is more about the same invocation's tarballs being reused via the artifact than a rerun producing new ones). If the same artifact/run is retried, the already-published package(s) will fail with npm's "cannot publish over the previously published version," aborting again before reaching any packages that still need it.
Not blocking — six sequential npm publish calls is a reasonable, auditable default, and the PR is explicit that a human is in the loop for review already — but worth a continue-on-error + summary of failures, or per-package idempotency (skip if npm view <pkg>@<version> already resolves), so a partial failure doesn't require manual npm publish for the stragglers.
| function unpackMember(archivePath, member, destinationPath) { | ||
| const workDirectory = mkdtempSync(join(tmpdir(), "vale-unpack-")); | ||
| try { | ||
| if (archivePath.endsWith(".zip")) { | ||
| run("unzip", ["-o", "-q", archivePath, member, "-d", workDirectory]); | ||
| } else { | ||
| run("tar", ["-xzf", archivePath, "-C", workDirectory, member]); | ||
| } | ||
| const root = resolve(workDirectory); | ||
| const extracted = resolve(root, member); | ||
| if (extracted === root || !extracted.startsWith(`${root}${sep}`)) { | ||
| throw new Error(`member ${member} resolves outside the unpack directory`); | ||
| } | ||
| const stats = lstatSync(extracted, { throwIfNoEntry: false }); | ||
| if (!stats) { | ||
| throw new Error( | ||
| `${basename(archivePath)} contains no member named ${member}` | ||
| ); | ||
| } | ||
| if (!stats.isFile()) { | ||
| throw new Error( | ||
| `member ${member} of ${basename(archivePath)} is not a regular file` | ||
| ); | ||
| } | ||
| copyFileSync(extracted, destinationPath); |
There was a problem hiding this comment.
Follow-up on the symlink/traversal hardening added in ba16968: the containment check (extracted.startsWith(root + sep)) and the lstatSync(...).isFile() check both run after tar/unzip has already extracted the archive to workDirectory. That's fine for catching a malicious leaf entry (a symlink or ../ name at the final path component, which is what the new check targets), but it can't prevent — only detect after the fact — a malicious intermediate path segment: e.g. an archive whose archiveMember directory component is itself a symlink pointing outside workDirectory, with tar following it while writing the nested file, before this function ever inspects extracted.
Not exploitable today since every archiveMember in vale-manifest.json is a flat single-segment filename, and the checksum gate means only archives a human has already reviewed the digest for reach this code — so this is defense-in-depth commentary rather than a live bug. But unpackMember is written generically enough (accepts any member path) that if a future manifest entry ever used a nested archiveMember, this check would not catch a compromised-but-checksum-matching upstream archive that exploits the intermediate-symlink case. Modern GNU tar (ubuntu-latest) does refuse to follow a pre-existing symlink when creating an implied directory by default, which is the main practical mitigation here — worth a one-line comment noting that's what's actually doing the protecting for nested members, since the code comment above currently implies the lstatSync check alone provides full containment.
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
Stack (root → tip):
Unit 2 of the
add-vale-binary-packagesstack. Unit 1 (#72) added six empty packages and the pinned manifest; this adds the pipeline that fills and publishes them, along with the scripts it runs and their tests. Unit 3, the CLI'soptionalDependenciespin, is not here and cannot be written until these names exist on npm.Two phases, because the trust boundary is code review
detectruns on a weekly schedule with no npm credential and no OIDC identity. It compares the latest upstream Vale release against the version pinned in.github/scripts/vale-manifest.json, and when upstream is ahead it opens a pull request updating that version and all six SHA256 digests, taken from upstream's ownvale_<version>_checksums.txt. It publishes nothing.publishruns on the push tomainthat merges that pull request, once a human has read the digests.The split is what makes the automation trustworthy. A single job that discovered a digest and then verified its downloads against the digest it had just discovered would verify nothing at all: whatever it downloaded would match, because the digest came from the same fetch. Separating discovery from verification puts a review in between, so nothing publishes on bytes nobody signed off on, and nobody has to notice a Vale release for the process to run.
What bounds a run
The upstream-version comparison, and only that. The "is this version already on npm?" check
release.ymluses cannot work here. Every publish stamps<valeVersion>-<yyyymmddhhmmss>, a version npm has never seen, so such a check would answer "not published" on every single run and could never suppress anything. The comparison against upstream is the only thing that can say "nothing to do."Why
prepareandpublishare separate jobspreparedownloads third-party bytes off the internet. It holdscontents: read, no environment, and noid-token, so it cannot publish or mint a token regardless of what it downloads. It verifies every archive against the committed digest and aborts the run on a mismatch before anything is unpacked, then hands overnpm packtarballs.The credentialed
publishjob therefore only ever handles bytes that already matched a reviewed digest and are already sealed into a tarball. It does not even check out the repository.Why packing comes before the artifact upload
actions/upload-artifactdoes not preserve file modes, and the Vale executable has to reach npm with its executable bit set.npm packrecords modes inside the.tgz, so packing first and shipping the tarball through the artifact keeps0755intact end to end.Before this can merge
package.jsoncarries the placeholder version0.0.0and no binary, so a barenpm publishfrom a package directory would burn the name on an empty0.0.0.npm-productionenvironment. There is no fallback token path in this workflow on purpose.phase: publishto exercise the OIDC path end to end.Merging neither PR in this stack publishes anything
The publish phase triggers on a push to
mainthat touches.github/scripts/vale-manifest.json. Unit 1 adds that manifest but no workflow to fire on it, and this PR adds the workflow but does not touch the manifest. The path filter never matches on either merge. The first publish is always deliberate, whether that is the manual bootstrap or aworkflow_dispatch.Known inherited limitation: the detect PR needs a manual check re-run
The detect phase opens its pull request with
GITHUB_TOKEN, and GitHub does not fire workflows on events raised by that token. SoValidatewill not start on a detect PR, and a maintainer has to re-run checks by hand before merging.This is the same step the changesets "Version Packages" PR already needs. Verified:
Validateon #65 ran withrun_attempt: 2, re-run manually before it merged. It is a limitation inherited from how GitHub scopesGITHUB_TOKEN, not a defect in this workflow.Stack
Forward-merging, per the proposal's delivery table. Unit 1 is repository-only and publishes nothing. Unit 2 publishes packages no consumer references yet. Unit 3 pins packages that by then exist.
This PR is a draft because it is the tip of the stack until unit 3 exists, and the OpenSpec archive gate would otherwise ask it to archive a change that is not finished. It also must not merge before the npm bootstrap above.
skip-changesetis correct here for the same reason it is on #72: the six packages are in the changesetsignorelist, andpackages/cliis untouched. Unit 3 is where a changeset belongs, since that is where a published CLI actually changes.Refs OSS-22