Skip to content

review: keep the pinned review.md ref in sync with the released version#242

Open
jwbron wants to merge 1 commit into
mainfrom
jwies/review-version-sync
Open

review: keep the pinned review.md ref in sync with the released version#242
jwbron wants to merge 1 commit into
mainfrom
jwies/review-version-sync

Conversation

@jwbron

@jwbron jwbron commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Problem: the ref: lag

workflows/review/review.md checks out Khan/actions at a pinned tag in its pre-agent-steps to fetch the deterministic lib the prompt invokes at runtime. The file's own comment calls that ref: "the single version surface for prompt + code", but nothing updated it during a release: changesets bumps package.json and changelogs, not prose in a markdown file, so every tag since review-v1.2.2 (v1.3.0, v1.3.1, v1.4.0) shipped with the ref still reading review-v1.2.2.

The skew is not cosmetic: the v1.4.0 prompt invokes lib/provenance.ts, which does not exist in a v1.2.2 checkout, so an un-overridden consumer gets runtime failures or silently missing gates. It also mis-stamps attribution, since the posted pr-reviewer:version marker reads the version from the pinned checkout's package.json. (This is the "operational floor" row from the round-two plan, re-confirmed on the review-v1.4.0 tag on 2026-07-09.)

Fix: automation, not vigilance

  • utils/sync-review-version.ts (+ lib, tests): rewrites every review-v<semver> literal in review.md (today, only the checkout ref: line) to the current review package version, and fails if it finds none to rewrite.
  • release.yml / package.json: the changesets action now runs pnpm run version-packages (changeset version followed by the sync) instead of the default changeset version, so the ref bump lands in the same Version Packages commit that gets tagged. If a Version Packages PR does not bump review, the sync is a no-op.
  • workflows/review/version-sync.test.ts (CI backstop): fails any PR, including the Version Packages PR, where review.md's literals do not match the review package version. Runs with the existing pnpm run test --run step in node-ci.
  • Bumps the currently stale ref to review-v1.4.0 so the invariant holds on main now; the next release rewrites it automatically.
  • Doc updates: review.md's version-surface comment now describes the sync mechanism, and the README's install/versioning sections state that each tag's review.md pins its own release (repo-wide audit of review-v literals found no other embeds outside changelogs and test fixtures).

Changeset included (patch bump for review; release plumbing and comments only).

Verification

  • Simulated the release end to end by running pnpm run version-packages against this PR's real changeset: changeset version bumped review to 1.4.1 and the sync rewrote the ref to review-v1.4.1 in the same working state the changesets action commits (then reset).
  • Negative check: with the ref set back to review-v1.2.2, the backstop test fails as intended.
  • vitest suite, eslint, and tsc all green on the real tree.

Once a release ships with the sync in place, webapp's hand-pinned override (its imported review.md carries a "the tag's own ref line lags the release" comment and hand-bumps the ref on every re-import) can be dropped on its next re-import.

cc @jeresig

Human steps (non-review)

  • Verify on the next release: after the next review package release merges, confirm the tagged review.md carries ref: review-v<released version> (the sync step) and that CI would have failed without it (the assert).
  • Retire the consumer workaround: on webapp's next re-import after a synced release, drop the hand-pinned ref: override comment ("the tag's own ref line lags the release") from its review.md.

…sync with the released version

The pre-agent-steps checkout ref in review.md is supposed to name the
release the file ships in, but changesets only bumps package.json, so
every tag since review-v1.2.2 shipped with a stale ref (and v1.4.0's
prompt invokes lib/provenance.ts, which a v1.2.2 checkout lacks).

The release flow's version command now runs utils/sync-review-version.ts
after changeset version, rewriting every review-v<semver> literal in
review.md to the version being released so the bump lands in the same
Version Packages commit that gets tagged. version-sync.test.ts is the CI
backstop: it fails any PR where the ref diverges from the review package
version. Also bumps the currently stale ref to review-v1.4.0.
@changeset-bot

changeset-bot Bot commented Jul 9, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: bfd3cea

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
review Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@khan-actions-bot khan-actions-bot requested review from a team, kevinb-khan and somewhatabstract and removed request for a team July 9, 2026 21:23

@jeresig jeresig left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for solving this issue - one request inline to make this generic for any possible workflow that we add!

if: steps.has-changesets.outputs.result == 'true'
uses: changesets/action@6a0a831ff30acef54f2c6aa1cbbc1096b066edaf # v1
with:
# Not the default `changeset version`: version-packages also syncs

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
# Not the default `changeset version`: version-packages also syncs
# Note the default `changeset version`: version-packages also syncs

@@ -0,0 +1,79 @@
import * as fs from "fs";

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could we make this generic so that it updates any workflow? We're bound to have more shared workflows so we might as well handle this in a single way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants