Skip to content

feat(vale): fetch, verify, and publish the Vale platform packages - #86

Merged
thecodedrift merged 3 commits into
mainfrom
openspec/add-vale-binary-packages-2-release
Aug 9, 2026
Merged

feat(vale): fetch, verify, and publish the Vale platform packages#86
thecodedrift merged 3 commits into
mainfrom
openspec/add-vale-binary-packages-2-release

Conversation

@thecodedrift

@thecodedrift thecodedrift commented Aug 6, 2026

Copy link
Copy Markdown
Member

Stack (root → tip):

Unit 2 of the add-vale-binary-packages stack. 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's optionalDependencies pin, is not here and cannot be written until these names exist on npm.

Two phases, because the trust boundary is code review

detect runs 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 own vale_<version>_checksums.txt. It publishes nothing.

publish runs on the push to main that 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.yml uses 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 prepare and publish are separate jobs

prepare downloads third-party bytes off the internet. It holds contents: read, no environment, and no id-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 over npm pack tarballs.

The credentialed publish job 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-artifact does not preserve file modes, and the Vale executable has to reach npm with its executable bit set. npm pack records modes inside the .tgz, so packing first and shipping the tarball through the artifact keeps 0755 intact end to end.

Before this can merge

  1. The six package names do not exist on npm yet. An npm trusted publisher is registered per package, and there is nothing to bind until the name exists, so the first publish of each name is a deliberate one-time manual step by a maintainer. The exact procedure is in the workflow file's header comment. Publish the packed tarball, not the package directory: the committed package.json carries the placeholder version 0.0.0 and no binary, so a bare npm publish from a package directory would burn the name on an empty 0.0.0.
  2. Register the trusted publisher for each of the six names on npmjs.com, bound to the npm-production environment. There is no fallback token path in this workflow on purpose.
  3. After merge, run the workflow manually with phase: publish to exercise the OIDC path end to end.

Merging neither PR in this stack publishes anything

The publish phase triggers on a push to main that 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 a workflow_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. So Validate will 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: Validate on #65 ran with run_attempt: 2, re-run manually before it merged. It is a limitation inherited from how GitHub scopes GITHUB_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-changeset is correct here for the same reason it is on #72: the six packages are in the changesets ignore list, and packages/cli is untouched. Unit 3 is where a changeset belongs, since that is where a published CLI actually changes.

Refs OSS-22

Built on top of #72

Publishes the Vale binary as per-platform npm packages from this repo, so a first-class engine isn't a host prerequisite.

This PR now carries unit 1 of a forward-merging stack: the six packages/vale-<platform>/ workspace packages, the committed checksum manifest that pins what goes into them, and the changesets ignore entries that keep release.yml out of their versions. It publishes nothing and no consumer references it. Unit 2 (#86) adds the fetch, verify, stamp, and two-phase release workflow. Unit 3, the CLI's optionalDependencies pin, cannot be written until these names exist on npm.

Why binary-in-tarball

The only existing npm distribution, @vvago/vale, is third-party and downloads at postinstall. That script runs during a consumer's install under a policy we don't set — pnpm 10 blocks dependency build scripts by default — producing no binary and no error. The objection is mechanism, not provenance: it would stand if the Vale project published it. Binary-in-tarball is integrity-hashed, lockfile-pinned, resolves offline, and needs no lifecycle script. So packages/vale-<platform>/ carries the binary with os/cpu declared and no bin, no code, no scripts — ast-grep's packaging without ast-grep's installation, whose hardlink step already fails here under pnpm dlx.

Versioning

An all-prerelease timestamp, <valeVersion>-<yyyymmddhhmmss>, with a plain <valeVersion> never published. That keeps the Vale version legible, means a packaging fix is a new timestamp on the same base rather than a spent version, and — because a prerelease only satisfies a range naming the same major.minor.patch — makes ^3.17.1 provably unable to resolve. Exact pinning stops being a convention someone can drift from.

Binaries are not committed

Six platforms at 10–20 MB each would live in git history permanently, so a published tarball is not reproducible from a plain clone. SHA256 checksums are committed and reviewed, and the pipeline refuses a mismatch, keeping "what can merge to main" as the trust boundary. The workflow that consumes them (#86) runs in two phases — detect upstream on a schedule and open a PR with the new version and checksums, then publish on merge — so nobody has to notice a Vale release and nothing publishes on bytes nobody signed off on. Safe to automate because publishing is inert: the CLI pins a literal exact version, so a new package reaches nobody until that pin is deliberately bumped.

Resolved open questions

The proposal left three open. All three are answered, and the reasoning is written up in design.md under Resolved Questions.

The matrix is six packages, not ast-grep's seven. Vale 3.17.1 publishes exactly six binary assets, and the packages are those six: darwin-arm64, darwin-x64, linux-arm64, linux-x64, win32-arm64, win32-x64. ast-grep's seventh is win32-ia32, and Vale ships no 32-bit Windows asset, so there is nothing to package.

No libc or toolchain suffix in the namesvale-linux-x64, not vale-linux-x64-gnu; vale-win32-x64, not -msvc. ast-grep carries those suffixes because Rust target triples disambiguate several builds per platform. Vale publishes exactly one build per os/cpu pair, so a suffix would disambiguate nothing while asserting a toolchain nobody verified.

musl stays on the PATH fallback. Upstream publishes no musl asset, so there is nothing to package for Alpine. That is not only a packaging gap: Vale's Linux build is dynamically linked against glibc (verified as dynamically linked, interpreter /lib64/ld-linux-x86-64.so.2, for GNU/Linux 3.2.0), so it is not a static Go binary and would not run on musl even if it were installed there. The linux packages' READMEs say so plainly rather than leaving a user to discover it as a loader error. This matches the existing gap rather than widening it: findSgBinary() maps every Linux to -gnu today, so Alpine already falls through for ast-grep.

Vale 3.17.1 is the pinned version, recorded in .github/scripts/vale-manifest.json beside the scripts that consume it. The manifest holds the version once, and per platform the asset-name template, the archive member to unpack, and the SHA256 of the release archive — upstream's vale_<version>_checksums.txt covers the archives rather than the executables inside them, so a committed digest is independently checkable against upstream and the archive is verified before anything is unpacked from it. Tracking is the detect phase in #86: a weekly schedule opens a PR whenever upstream is ahead, and a security release takes a manual detect dispatch rather than waiting for the cadence.

Which Vale version the CLI pins is a separate decision, made when the CLI's optionalDependencies land in unit 3.

Also carries two CLAUDE.md fixes

Unrelated to Vale but too small to spend PRs on:

  • The PR-reference table documented only TSKL-, reading as though it's the only bare identifier the Linear integration resolves. It isn't — OSS-23 linked and moved to In Review on PR creation for ref(cli): resolve ast-grep without an install-time step #69.
  • A warning to check for a shallow clone before rebasing or force-pushing. git clone --depth=N implies --single-branch, which breaks --force-with-lease on every branch (it fails stale info, so people fall back to a bare --force) and, more quietly, makes git rebase main correct only while the merge base sits inside the shallow window.

Where this sits

This change is the one exception to "one change, one PR": it is stacked, merging forward, with the archive landing on the last unit. The archive gate skips a PR that is not the tip, so stack: openspec-archived is not expected on this PR at all — #86 is the tip, and the change is archived on unit 3.

PR Change Prerequisites
#72 (this PR) Vale binary packages, unit 1 — packages, manifest, changesets ignore none
#86 Vale binary packages, unit 2 — fetch, verify, stamp, release workflow #72
Vale binary packages, unit 3 — the CLI's optionalDependencies pin #86 published
#70 knowledge prompt export none
#71 Vale engine + engine-selection topic a published Vale binary to resolve

partition-rules-by-engine has landed and is archived, so #71's only remaining prerequisite is a published binary from this stack.

#70 and #71 are coupled by exactly one line: whichever lands second adds the engine-selection topic to TOPICS. Ordering between them doesn't matter.

Downstream, the generator's decision router (TSKL-279) needs a published release containing #70 and #71. It consumes a normal release — no prerelease, no path dependency — so it waits without blocking anything here.

skip-changeset stays on this PR. The six packages are in the changesets ignore list and packages/cli is untouched, so there is nothing here for changesets to version or release. Unit 3 is where a changeset belongs, since that is where a published CLI actually changes.

Fixes OSS-22

@thecodedrift thecodedrift added the skip-changeset PR intentionally ships no release note (bypasses the changeset requirement) label Aug 6, 2026
@thecodedrift
thecodedrift marked this pull request as ready for review August 7, 2026 00:29
Copilot AI lite review requested due to automatic review settings August 7, 2026 00:29

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.yml with separate detect, credential-free prepare, and credentialed publish jobs.
  • 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 .gitignore to 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.

Comment thread .github/scripts/vale-release.cjs
Comment thread .github/scripts/vale-prepare.cjs
Comment thread .github/scripts/vale-prepare.cjs Outdated
@thecodedrift

Copy link
Copy Markdown
Member Author

@claude /review

@claude

claude Bot commented Aug 7, 2026

Copy link
Copy Markdown

Claude finished @thecodedrift's task in 19m 8s —— View job


Claude finished reviewing @thecodedrift's PR

Tasks

  • Gather PR context and diff (git diff origin/openspec/add-vale-binary-packages...HEAD)
  • Read all changed files in full (vale-detect.cjs, vale-prepare.cjs, vale-release.cjs, vale-release.test.cjs, vale-binaries.yml, .gitignore, tasks.md)
  • Review for correctness, security, performance, tests, clarity
  • Post inline comments for concrete issues
  • Post final summary

Overall assessment

The two-phase trust-boundary design (credential-free detect/prepare vs. OIDC-credentialed publish) is sound and well-documented, and the hardening added in ba16968 (sub-1.0 caret ranges, requireValue for --out/--only, the unpackMember containment/regular-file check) correctly addresses all three issues Copilot raised. vale-release.cjs is genuinely pure and its test suite (vale-release.test.cjs) is thorough for the logic it covers — version stamping, manifest validation, checksum parsing, digest assertion.

However, there is one bug that breaks the feature's core purpose, found while tracing vale-detect.cjs's calls into planManifestUpdate:

  • vale-detect.cjs:103-107 — the detect job throws on every run where upstream actually has a new release. The "probe" call passes checksumsText: "" to cheaply check isUpstreamAhead. But planManifestUpdate only skips looking at checksumsText when upstream is not ahead; when it is ahead, it falls through to parseChecksumsFile("") → an empty Map → throws "upstream checksums file for X parsed to no entries". That's uncaught, so main() exits 1 before the real checksums URL is ever fetched, no outputs are set, and the "Open the manifest update PR" step never runs. Only the no-op path (nothing to detect) currently succeeds — the inverse of the intended behavior. This gap exists because vale-release.test.cjs only unit-tests planManifestUpdate in isolation (and even has a test proving this exact throw is intentional for that function's contract); there's no test exercising vale-detect.cjs's own main() composition, so the probe's empty-string sentinel was never run against a real "ahead" case. Details and a suggested fix (call isUpstreamAhead/parseReleaseTag directly for the cheap check instead of routing through planManifestUpdate with a fake payload) are in the inline comment.

Two lower-severity, non-blocking observations posted inline:

  • vale-binaries.yml's publish loop runs six sequential npm publish calls under bash -e; a mid-loop failure leaves a partial release, and a naive retry would immediately fail again on the already-published package before reaching the rest. Worth considering idempotency or continue-on-error with a summary, not blocking.
  • vale-prepare.cjs's unpackMember containment check (added in ba16968) runs after extraction and only catches a malicious leaf entry, not a symlinked intermediate path segment. Not exploitable today (every archiveMember in the manifest is a flat filename, and the checksum gate means only reviewed archives reach this code), but worth a comment noting GNU tar's own default symlink protection is what's actually covering the nested case, since the current code comment implies the lstatSync check alone provides full containment.

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 run: blocks, spawnSync calls use argv arrays not shell strings), and the workflow's SHA-pinned actions, scoped permissions, and no-checkout publish job all match the hardening conventions this repo already follows elsewhere.

Comment thread .github/scripts/vale-detect.cjs Outdated
Comment thread .github/workflows/vale-binaries.yml Outdated
Comment thread .github/scripts/vale-prepare.cjs
@thecodedrift

Copy link
Copy Markdown
Member Author

Re: @claude[bot] — "Claude finished reviewing @thecodedrift's PR … there is one bug that breaks the feature's core purpose…"
#86 (comment)

All three findings are addressed in 45f4bff, with replies on the inline threads:

  • vale-detect.cjs throwing on every real upstream bump — confirmed and fixed. main() now calls isUpstreamAhead/parseReleaseTag directly for the cheap check and calls planManifestUpdate exactly once with the real checksums. Your read of why it slipped through was the useful part: the bug was in main()'s composition, which nothing tested, so .github/scripts/vale-detect.test.cjs is new and exercises main() with both fetches stubbed — the first case being the "upstream ahead" path that previously threw.
  • Partial release from the publish loop — the loop now attempts all six, skips any package npm view already resolves at the stamped version (so a re-run is idempotent rather than fatal), and fails at the end naming the stragglers.
  • unpackMember's leaf-only containment — enforced rather than commented: assertManifest now rejects an archiveMember containing a path separator or .., so the flat-filename property the safety actually rests on is machine-checked instead of assumed.

— AI Coding Agent

@thecodedrift
thecodedrift force-pushed the openspec/add-vale-binary-packages-2-release branch 2 times, most recently from c5fc35a to 7d735af Compare August 9, 2026 23:16
Base automatically changed from openspec/add-vale-binary-packages to main August 9, 2026 23:35
thecodedrift and others added 3 commits August 9, 2026 16:35
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>
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>
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
@thecodedrift
thecodedrift force-pushed the openspec/add-vale-binary-packages-2-release branch from 7d735af to c439227 Compare August 9, 2026 23:35
@thecodedrift
thecodedrift merged commit 3a69ac8 into main Aug 9, 2026
4 checks passed
@thecodedrift
thecodedrift deleted the openspec/add-vale-binary-packages-2-release branch August 9, 2026 23:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-changeset PR intentionally ships no release note (bypasses the changeset requirement)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants