Skip to content

fix(vale): gate publishing on the pinned Vale version, not the manifest file - #92

Merged
thecodedrift merged 2 commits into
mainfrom
fix/vale-publish-gate
Aug 10, 2026
Merged

fix(vale): gate publishing on the pinned Vale version, not the manifest file#92
thecodedrift merged 2 commits into
mainfrom
fix/vale-publish-gate

Conversation

@thecodedrift

Copy link
Copy Markdown
Member

The problem

The publish path fires on a push to main touching .github/scripts/vale-manifest.json:

push:
  branches: [main]
  paths: [".github/scripts/vale-manifest.json"]

A paths: filter cannot see why the file changed. Rewording a comment, reformatting it, or correcting a digest is indistinguishable from a version bump — and each one publishes six packages at a fresh <valeVersion>-<yyyymmddhhmmss>.

There was no second line of defense, because publishing here is not idempotent: every stamp is novel by construction, so nothing downstream could recognize the run as redundant.

To be clear about what was not wrong: the weekly schedule only reaches detect, which holds no npm credential and publishes nothing. The exposure was the manifest-file push trigger alone.

The fix

A credential-free gate job ahead of prepare:

gate → prepare → publish

It asks whether the pinned Vale version is already published and skips when it is. It runs before prepare downloads ~60 MB of upstream archives, so a skip is cheap.

Two properties are deliberate:

  • An explicit workflow_dispatch passes --force and is never suppressed. A human asking for a publish gets one. Only the automatic push path is gated.
  • A skip requires all six packages to carry the pinned version. Checking one would silently skip a half-published set, so the gate doubles as partial-release repair — it re-runs exactly the case the publish loop's failure aggregation exists to report.

A package that 404s reads as "nothing published", not as an error. That is the ordinary pre-bootstrap state, and failing there would wedge the gate closed precisely when the packages most need publishing.

Why this doesn't contradict D5

design.md D5 argued that an already-published check cannot bound this workflow. That is true of the stamped version and false of the base version, and the distinction is the whole design:

Question Answer
Is 3.17.1-20260810000724 published? Always no — novel by construction
Has anything been published for Vale 3.17.1? Answerable: a published 3.17.1 or any 3.17.1-* stamp

D5 is amended to draw that line explicitly. Without it, the next reader removes this gate as contradicting the design.

Verification

  • node --test .github/scripts/*.test.cjs113/113 pass (9 new)
  • New tests cover: bare-version and stamp matching, the - separator (so pinned 3.1 is not satisfied by 3.1.1), pre-bootstrap, fully published, partial set, forced dispatch, and should_publish output in both directions
  • Run against the live registry: all six report MISSING for 3.17.1 → would publish, which is correct today
  • pnpm lint, pnpm typecheck, prettier all clean

Blocking

Blocks #91. That PR archives this change, so its design.md needs the amended D5 before it lands — it will need a merge from main after this one.

Refs #91

…st file

The publish path fires on a push to main touching vale-manifest.json, and a
`paths:` filter cannot see WHY the file changed. A reworded comment, a
reformat, or a digest correction is indistinguishable from a version bump, and
each one published six packages at a fresh <valeVersion>-<timestamp>. Nothing
downstream absorbs that: every stamp is novel by construction, so there was no
second line of defense.

Add a credential-free `gate` job ahead of `prepare` that asks whether the
pinned Vale version is already published, and skips when it is. It runs before
prepare downloads ~60 MB, so a skip is cheap.

design.md D5 argued an already-published check cannot work here. That is true
of the STAMPED version and false of the BASE version, and the difference is the
whole design: "is 3.17.1-20260810000724 published?" is always no, while "has
anything been published for Vale 3.17.1?" is answered by a published 3.17.1 or
any 3.17.1-* stamp. D5 is amended to draw that line, so the gate does not read
as contradicting it.

Two properties kept deliberately:

- An explicit workflow_dispatch passes --force and is never suppressed. A human
  asking for a publish gets one.
- A skip requires ALL six packages to carry the pinned version. Checking one
  would silently skip a half-published set, so the gate doubles as
  partial-release repair.

Blocks #91: that PR archives this change, so its design.md must carry the
amended D5 before it lands.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Jwc9FFroR3mTZ4hLiSkkX3
Copilot AI lite review requested due to automatic review settings August 10, 2026 04:19
@thecodedrift thecodedrift added the skip-changeset PR intentionally ships no release note (bypasses the changeset requirement) label Aug 10, 2026
@thecodedrift

Copy link
Copy Markdown
Member Author

@claude /review

@claude

claude Bot commented Aug 10, 2026

Copy link
Copy Markdown

Claude finished @thecodedrift's task in 4m 24s —— View job


Review complete

  • Gather context (diff, changed files)
  • Review .github/scripts/vale-gate.cjs
  • Review .github/scripts/vale-gate.test.cjs
  • Review .github/workflows/vale-binaries.yml
  • Review openspec/changes/add-vale-binary-packages/design.md
  • Post inline comments for concrete issues
  • Post final summary

Overall assessment

This is a well-reasoned, well-tested fix. The gate → prepare → publish chain, the base-version-vs-stamped-version distinction, the "all six must be covered" skip condition, and the 404-as-unpublished handling all match what's described in the PR body and are backed by good unit tests (vale-gate.test.cjs, 9 new cases covering the boundary cases that matter: - separator, pre-bootstrap, partial set, forced dispatch). I didn't find any correctness bugs in vale-gate.cjs or the workflow wiring — the needs: gate / if: needs.gate.outputs.should_publish == 'true' gating on prepare is correct, and skip propagation for non-publish trigger events (schedule, workflow_dispatch with phase: detect) works the same as before since a skipped gate job auto-skips its dependents.

Two non-blocking items posted as inline comments:

  1. .github/workflows/vale-binaries.yml (lines 27–31, unchanged by this diff — GitHub wouldn't let me anchor a comment there since it's outside the hunk, so noting it here instead): the file's own header comment still says an "is this version already on npm?" check "cannot work here... (design D5)," which is now contradicted by the gate job added a few dozen lines below it. The PR carefully amends design.md D5 to draw the base-vs-stamped-version distinction, but the near-identical paragraph embedded in this workflow file's header wasn't updated to match — the exact "next reader removes this gate as contradicting the design" risk the PR calls out for design.md applies here too. The "TWO PHASES" summary near the top of the file also doesn't mention the new gate phase.
  2. .github/scripts/vale-gate.cjs:117-119: the six registry lookups in main() are independent but awaited sequentially in a for loop. Not a correctness issue, but worth using Promise.all given the job's explicit goal of being cheap and fast ahead of prepare's ~60 MB download.

Nothing else stood out — the --force handling for workflow_dispatch, the contents: read / no-checkout-persist permissions on the new gate job, and the pure/testable split between planPublish (decision) and fetchPublishedVersions (I/O) all look right. Per instructions I did not run lint/typecheck/tests locally; CI will report those on the PR.

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

This PR prevents redundant Vale platform-package publishes that were previously triggered by any edit to .github/scripts/vale-manifest.json (including non-version changes). It introduces a credential-free “gate” that checks whether the pinned base Vale version already exists on npm (across all six platform packages) and only proceeds to prepare → publish when publishing is actually needed (or when explicitly forced via workflow_dispatch).

Changes:

  • Add a new gate job ahead of prepare in the Vale binaries workflow and condition prepare on gate’s should_publish output.
  • Introduce .github/scripts/vale-gate.cjs to decide “publish vs skip” based on npm registry state, with a --force override for manual dispatch.
  • Amend the design doc’s D5 rationale to clarify why base-version publish checks are valid while stamped-version checks are not.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
openspec/changes/add-vale-binary-packages/design.md Updates D5 to explicitly distinguish stamped vs base version checks and justify gating publish on the base version.
.github/workflows/vale-binaries.yml Adds a credential-free gate job and gates prepare (and thus publish) on should_publish.
.github/scripts/vale-gate.cjs Implements the npm-registry-based decision logic and writes should_publish to $GITHUB_OUTPUT.
.github/scripts/vale-gate.test.cjs Adds unit tests covering matching behavior, pre-bootstrap, partial/complete publish sets, and forced dispatch.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread .github/scripts/vale-gate.cjs Outdated
The PR that added the gate amended design.md D5 to distinguish a stamped-version
check (impossible) from a base-version check (the gate), specifically so nobody
would later delete the gate for disagreeing with the design — and left the
near-identical paragraph in the workflow's own header, a few dozen lines above
the job it describes, still asserting that such a check "could never suppress
anything". Fix the header to draw the same distinction, and list `gate` in the
TWO PHASES summary alongside prepare and publish.

Also run the six registry lookups concurrently. They are independent, and the
job's whole justification is deciding cheaply before prepare downloads ~60 MB;
sequential awaits made the gate six round trips deep for no reason. Order is
unaffected — `missing` is built by filtering manifest.platforms, not by
completion order.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Jwc9FFroR3mTZ4hLiSkkX3
@thecodedrift

Copy link
Copy Markdown
Member Author

Re: @claude[bot] — "Claude finished @thecodedrift's task in 4m 24s — Review complete / Overall assessment…"
#92 (comment)

Both items addressed in cd1da85.

1 — the workflow header. You caught a real miss, and it is the exact failure this PR exists to prevent. I amended design.md D5 so nobody would later delete the gate for contradicting the design, then left the near-identical paragraph in vale-binaries.yml — a few dozen lines above the job it describes — still asserting such a check "could never suppress anything." The header now draws the same stamped-vs-base distinction, explains that the gate exists because paths: cannot see why the manifest changed, and notes that an explicit dispatch passes --force. gate is also listed in the TWO PHASES summary. Thanks for anchoring it here when GitHub would not let you comment outside the hunk — it would have shipped otherwise.

2 — sequential lookups. Now Promise.all. Order is unaffected, since missing is built by filtering manifest.platforms rather than by completion order.

Verified after both: 113/113 script tests, pnpm lint and prettier clean, workflow still parses as detect → gate → prepare → publish, and the gate against the live registry still correctly reports all six missing for 3.17.1.

— AI Coding Agent

@thecodedrift
thecodedrift merged commit d95f92c into main Aug 10, 2026
4 of 5 checks passed
@thecodedrift
thecodedrift deleted the fix/vale-publish-gate branch August 10, 2026 04:33
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