-
Notifications
You must be signed in to change notification settings - Fork 0
fix(ci): give internal persona publishes one reconcilable release commit #328
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -55,6 +55,15 @@ jobs: | |||||
| versions: ${{ steps.bump.outputs.versions }} | ||||||
| release_version: ${{ steps.bump.outputs.release_version }} | ||||||
| steps: | ||||||
| # `github.ref_name` is the branch this run pushes its release commit to. | ||||||
| # On a tag dispatch it would be the tag name, and the push would create a | ||||||
| # branch named after the tag while npm already has the new versions. | ||||||
| - name: Require a branch dispatch | ||||||
| if: ${{ github.ref_type != 'branch' }} | ||||||
| run: | | ||||||
| echo "::error title=Dispatch from a branch::This workflow publishes and then pushes a release commit to '${{ github.ref_name }}', which is a ${{ github.ref_type }}. Re-run it from a branch." | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: A branch name containing shell syntax such as Prompt for AI agents
Suggested change
|
||||||
| exit 1 | ||||||
|
|
||||||
| - name: Checkout | ||||||
| uses: actions/checkout@v6 | ||||||
| with: | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,13 +1,17 @@ | ||
| import assert from 'node:assert/strict'; | ||
| import { execFileSync } from 'node:child_process'; | ||
| import { mkdirSync, mkdtempSync, readFileSync, writeFileSync } from 'node:fs'; | ||
| import { mkdirSync, mkdtempSync, readFileSync, readdirSync, rmSync, writeFileSync } from 'node:fs'; | ||
| import { tmpdir } from 'node:os'; | ||
| import { join, resolve } from 'node:path'; | ||
| import test from 'node:test'; | ||
|
|
||
| const publishWorkflow = readFileSync('.github/workflows/publish.yml', 'utf8'); | ||
| const verifyWorkflow = readFileSync('.github/workflows/verify-publish.yml', 'utf8'); | ||
| const personaWorkflow = readFileSync('.github/workflows/publish-persona.yml', 'utf8'); | ||
| const internalPersonaWorkflow = readFileSync( | ||
| '.github/workflows/publish-internal-personas.yml', | ||
| 'utf8' | ||
| ); | ||
|
|
||
| function publishTargetDirectories(workflow) { | ||
| const match = workflow.match(/echo "packages=([^"]+)"/); | ||
|
|
@@ -85,6 +89,21 @@ test('scoped CLI verification checks only the supported thin-entry contract', () | |
| */ | ||
| const pushScript = 'scripts/push-release-commit.sh'; | ||
|
|
||
| function stepScript(workflow, name) { | ||
| const lines = workflow.replaceAll('\r\n', '\n').split('\n'); | ||
| const start = lines.findIndex((line) => line.trim() === `- name: ${name}`); | ||
| assert.notEqual(start, -1, `workflow must define a "${name}" step`); | ||
|
|
||
| const next = lines.findIndex((line, index) => index > start && /^\s*- name: /.test(line)); | ||
| const stepLines = lines.slice(start, next === -1 ? lines.length : next); | ||
| const runIndex = stepLines.findIndex((line) => /^\s+run: \|\s*$/.test(line)); | ||
| assert.notEqual(runIndex, -1, `"${name}" must carry a literal run block`); | ||
|
|
||
| const body = stepLines.slice(runIndex + 1); | ||
| const indent = body.find((line) => line.trim())?.match(/^\s*/)[0] ?? ''; | ||
| return body.map((line) => (line.startsWith(indent) ? line.slice(indent.length) : line)).join('\n'); | ||
| } | ||
|
|
||
| const GIT_ENV = { | ||
| ...process.env, | ||
| GIT_AUTHOR_NAME: 'release-test', | ||
|
|
@@ -97,6 +116,11 @@ function git(cwd, ...args) { | |
| return execFileSync('git', args, { cwd, encoding: 'utf8', env: GIT_ENV }); | ||
| } | ||
|
|
||
| /** Temp repos are throwaway, but leaving a pile of them in tmpdir is rude. */ | ||
| function cleanup(...paths) { | ||
| for (const path of paths) rmSync(path, { recursive: true, force: true }); | ||
| } | ||
|
|
||
| function writeVersions(dir, version) { | ||
| for (const pkg of ['cli', 'deploy']) { | ||
| mkdirSync(join(dir, 'packages', pkg), { recursive: true }); | ||
|
|
@@ -227,6 +251,29 @@ test('exhausted attempts fail loudly instead of stranding a rebuilt commit', () | |
| assert.equal(git(run, 'rev-parse', 'HEAD').trim(), releaseBefore); | ||
| }); | ||
|
|
||
| test('a non-branch target is refused before anything is pushed', () => { | ||
| const { root, seed, run } = stageRelease(); | ||
| try { | ||
| // What a tag dispatch produces: github.ref_name is the tag, not a branch. | ||
| git(seed, 'tag', '-a', 'v9.9.9', '-m', 'a tag'); | ||
| git(seed, 'push', '-q', 'origin', 'refs/tags/v9.9.9'); | ||
| git(run, 'fetch', '-q', 'origin'); | ||
|
|
||
| assert.throws( | ||
| () => runPushStep(run, { BRANCH: 'v9.9.9' }), | ||
| /not an existing branch on origin/, | ||
| 'pushing HEAD to refs/heads/<tag> would invent a branch named after the tag' | ||
| ); | ||
| git(seed, 'fetch', '-q', 'origin'); | ||
| assert.throws( | ||
| () => git(seed, 'rev-parse', '--verify', 'origin/v9.9.9'), | ||
| 'no branch may be created for the tag' | ||
| ); | ||
| } finally { | ||
| cleanup(root); | ||
| } | ||
| }); | ||
|
|
||
| test('release commit is a no-op when the branch already carries its files', () => { | ||
| const { seed, run } = stageRelease(); | ||
|
|
||
|
|
@@ -242,23 +289,37 @@ test('release commit is a no-op when the branch already carries its files', () = | |
| assert.equal(versionOnMain(seed, 'cli'), '4.1.49'); | ||
| }); | ||
|
|
||
| for (const [name, workflow] of [ | ||
| ['publish.yml', publishWorkflow], | ||
| ['publish-persona.yml', personaWorkflow], | ||
| // Every workflow that publishes to npm and then updates git. `push` names the | ||
| // step that lands the release commit; each must reconcile, and must tag only | ||
| // after that commit is on the branch. | ||
| for (const [name, workflow, push] of [ | ||
| ['publish.yml', publishWorkflow, 'Push release commit'], | ||
| ['publish-persona.yml', personaWorkflow, 'Push release commit'], | ||
| ['publish-internal-personas.yml', internalPersonaWorkflow, 'Commit + push release'], | ||
| ]) { | ||
| test(`${name} pushes the release commit before tagging it`, () => { | ||
| const lines = workflow.split('\n'); | ||
| const push = lines.findIndex((line) => line.trim() === '- name: Push release commit'); | ||
| const pushStep = lines.findIndex((line) => line.trim() === `- name: ${push}`); | ||
| const tag = lines.findIndex((line) => line.trim() === '- name: Tag + push tags'); | ||
| assert.notEqual(push, -1, 'must reconcile its push'); | ||
| assert.notEqual(pushStep, -1, 'must reconcile its push'); | ||
| assert.notEqual(tag, -1, 'must tag in its own step'); | ||
| assert.ok(push < tag, 'tagging before the push can strand tags on an unreachable commit'); | ||
| assert.ok(pushStep < tag, 'tagging before the push can strand tags on an unreachable commit'); | ||
| assert.ok(workflow.includes(pushScript), 'must use the shared reconciling push script'); | ||
| assert.ok( | ||
| workflow.includes(`run: ${pushScript}`), | ||
| 'must use the shared reconciling push script' | ||
| !/git push origin HEAD --follow-tags/.test(workflow), | ||
| 'the unreconciled push is what left npm ahead of git on 2026-08-24' | ||
| ); | ||
| }); | ||
|
|
||
| test(`${name} refuses a dispatch that is not from a branch`, () => { | ||
| const lines = workflow.split('\n'); | ||
| const guard = lines.findIndex((line) => line.trim() === '- name: Require a branch dispatch'); | ||
| assert.notEqual(guard, -1, 'a tag dispatch would push the release commit to refs/heads/<tag>'); | ||
| const firstStep = lines.findIndex((line) => /^\s*- name: /.test(line)); | ||
| assert.equal(guard, firstStep, 'the guard must run before anything is published'); | ||
| assert.match(workflow, /if: \$\{\{ github\.ref_type != 'branch' \}\}/); | ||
| }); | ||
|
|
||
| test(`${name} checks out the branch tip, not the dispatch SHA`, () => { | ||
| assert.match( | ||
| workflow, | ||
|
|
@@ -267,3 +328,46 @@ for (const [name, workflow] of [ | |
| ); | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * publish-internal-personas.yml used to commit and tag once per persona inside | ||
| * its publish loop, which cannot be reconciled onto a branch that moved. It now | ||
| * stages every bump and makes one release commit after the loop; this exercises | ||
| * that step against a staged index rather than trusting the YAML to read right. | ||
| */ | ||
| test('internal personas make a single release commit for every pack', () => { | ||
| const root = mkdtempSync(join(tmpdir(), 'persona-release-')); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3: The test creates a temp repo via Prompt for AI agents |
||
| git(root, 'init', '-q', '-b', 'main', root); | ||
|
|
||
| for (const [dir, version] of [['persona-a', '1.2.3'], ['persona-b', '4.5.6']]) { | ||
| mkdirSync(join(root, 'packages', dir), { recursive: true }); | ||
| writeFileSync(join(root, 'packages', dir, 'package.json'), `{"version":"${version}"}\n`); | ||
| } | ||
| git(root, 'add', '-A'); | ||
| git(root, 'commit', '-qm', 'base'); | ||
|
|
||
| // What the publish loop leaves behind: bumped manifests, staged, uncommitted. | ||
| writeFileSync(join(root, 'packages', 'persona-a', 'package.json'), '{"version":"1.2.4"}\n'); | ||
| writeFileSync(join(root, 'packages', 'persona-b', 'package.json'), '{"version":"4.5.7"}\n'); | ||
| git(root, 'add', '-A'); | ||
| writeFileSync( | ||
| '/tmp/persona-publish-targets.tsv', | ||
| '@scope/persona-a\tpackages/persona-a\t1.2.4\n@scope/persona-b\tpackages/persona-b\t4.5.7\n' | ||
| ); | ||
|
|
||
| // The step ends by delegating the push; stub it so the test stays local. | ||
| mkdirSync(join(root, 'scripts'), { recursive: true }); | ||
| writeFileSync(join(root, 'scripts', 'push-release-commit.sh'), '#!/bin/sh\ntouch pushed.marker\n'); | ||
| execFileSync('chmod', ['+x', join(root, 'scripts', 'push-release-commit.sh')]); | ||
|
|
||
| const script = join(root, 'commit-step.sh'); | ||
| writeFileSync(script, stepScript(internalPersonaWorkflow, 'Commit + push release')); | ||
| execFileSync('/bin/bash', [script], { cwd: root, encoding: 'utf8', env: GIT_ENV }); | ||
|
|
||
| const subjects = git(root, 'log', '--format=%s').trim().split('\n'); | ||
| assert.equal(subjects.length, 2, 'one release commit on top of the base, not one per pack'); | ||
| assert.equal(subjects[0], 'chore(release): @scope/persona-a@1.2.4 @scope/persona-b@4.5.7'); | ||
| assert.ok(readdirSync(root).includes('pushed.marker'), 'must delegate to the push script'); | ||
|
|
||
| cleanup(root, '/tmp/persona-publish-targets.tsv'); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When this workflow is dispatched with a tag ref,
github.ref_nameis the tag name, butpush-release-commit.shalways pushesHEADtorefs/heads/$BRANCH; the newly published versions therefore create/update a branch named after the tag instead of reconciling onto an intended release branch, leaving that branch's manifests stale after npm has already changed. This is a supported invocation—gh workflow run --helpdescribes--refas a “Branch or tag name”—so the inspected workflow should reject tag dispatches before publishing or require an explicit target branch rather than passing the tag name asBRANCH.Useful? React with 👍 / 👎.