Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 58 additions & 7 deletions .github/workflows/publish-internal-personas.yml
Original file line number Diff line number Diff line change
Expand Up @@ -62,10 +62,23 @@ jobs:
name: Publish persona packs
runs-on: ubuntu-latest
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."
exit 1

- name: Checkout
uses: actions/checkout@v6
with:
fetch-depth: 0
# `workflow_dispatch` pins `github.sha` at dispatch time; a queued run
# would otherwise bump from a stale commit. See the same note in
# publish.yml.
ref: ${{ github.ref_name }}

- name: Setup pnpm
uses: pnpm/action-setup@v5
Expand Down Expand Up @@ -219,19 +232,57 @@ jobs:
echo "==> Publishing $TARBALL $COMMON_FLAGS"
npm publish "$TARBALL" $COMMON_FLAGS

# Staged, not committed: the run makes ONE release commit after the
# loop. A commit per persona cannot be reconciled onto a branch that
# moved mid-run, and tagging here would strand tags whenever the
# push is rejected — see publish.yml and scripts/push-release-commit.sh.
if [ "$INPUT_DRY_RUN" != "true" ] && [ "$INPUT_VERSION" != "none" ]; then
git add "$DIR/package.json"
if ! git diff --cached --quiet; then
git commit -m "chore(release): $NAME@$VERSION"
fi
SLUG="$(echo "${NAME#@}" | tr '/' '-')"
git tag -a "${SLUG}-v${VERSION}" -m "$NAME@$VERSION"
fi

echo "$NAME@$VERSION published (dry_run=$INPUT_DRY_RUN)" >> "$GITHUB_STEP_SUMMARY"
echo "::endgroup::"
done < /tmp/persona-publish-targets.tsv

- name: Push commits + tags
# The packs are on npm by the time this runs, so the push reconciles onto
# whatever landed on the branch mid-run rather than failing and leaving
# the registry ahead of git.
- name: Commit + push release
if: ${{ github.event.inputs.dry_run != 'true' && github.event.inputs.version != 'none' }}
env:
BRANCH: ${{ github.ref_name }}
Comment on lines +252 to +253

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Reject tag refs before mapping them to a branch

When this workflow is dispatched with a tag ref, github.ref_name is the tag name, but push-release-commit.sh always pushes HEAD to refs/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 --help describes --ref as 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 as BRANCH.

Useful? React with 👍 / 👎.

Comment thread
cubic-dev-ai[bot] marked this conversation as resolved.
run: |
set -euo pipefail
if git diff --cached --quiet; then
echo "No version changes to commit."
exit 0
fi

MSG="chore(release):"
while IFS=$'\t' read -r NAME DIR VERSION; do
MSG="$MSG $NAME@$VERSION"
done < /tmp/persona-publish-targets.tsv
git commit -m "$MSG"

scripts/push-release-commit.sh

- name: Tag + push tags
if: ${{ github.event.inputs.dry_run != 'true' && github.event.inputs.version != 'none' }}
run: git push origin HEAD --follow-tags
run: |
set -euo pipefail
CREATED=""
while IFS=$'\t' read -r NAME DIR VERSION; do
SLUG="$(echo "${NAME#@}" | tr '/' '-')"
TAG="${SLUG}-v${VERSION}"
if git rev-parse -q --verify "refs/tags/$TAG" >/dev/null; then
echo "::warning::tag $TAG already exists - leaving it as is"
continue
fi
git tag -a "$TAG" -m "$NAME@$VERSION"
CREATED="$CREATED refs/tags/$TAG"
done < /tmp/persona-publish-targets.tsv
if [ -n "$CREATED" ]; then
set -f
git push origin $CREATED
set +f
fi
9 changes: 9 additions & 0 deletions .github/workflows/publish-persona.yml
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,15 @@ jobs:
npm_name: ${{ steps.package.outputs.npm_name }}
tag_name: personas-core-v${{ steps.bump.outputs.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."
exit 1

- name: Checkout
uses: actions/checkout@v6
with:
Expand Down
9 changes: 9 additions & 0 deletions .github/workflows/publish.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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."

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.

P2: A branch name containing shell syntax such as $(...) executes during this diagnostic because GitHub interpolates github.ref_name into the Bash script. Read GITHUB_REF_NAME and GITHUB_REF_TYPE from the environment instead of embedding the context values in run.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/publish.yml, line 64:

<comment>A branch name containing shell syntax such as `$(...)` executes during this diagnostic because GitHub interpolates `github.ref_name` into the Bash script. Read `GITHUB_REF_NAME` and `GITHUB_REF_TYPE` from the environment instead of embedding the context values in `run`.</comment>

<file context>
@@ -55,6 +55,15 @@ jobs:
+      - 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."
+          exit 1
+
</file context>
Suggested change
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."
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."

exit 1

- name: Checkout
uses: actions/checkout@v6
with:
Expand Down
9 changes: 9 additions & 0 deletions scripts/push-release-commit.sh
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,15 @@ set -euo pipefail
BRANCH="${BRANCH:-main}"
ATTEMPTS="${PUSH_ATTEMPTS:-5}"

# Callers pass `github.ref_name`, which is a tag name on a tag dispatch. Pushing
# HEAD to refs/heads/<tag> would invent a branch named after the tag and leave
# the real branch stale while npm already has the new versions. The workflows
# reject a non-branch dispatch before publishing; this is the backstop.
if ! git show-ref --verify --quiet "refs/remotes/origin/$BRANCH"; then
echo "::error title=Release commit not pushed::'$BRANCH' is not an existing branch on origin. Publish from a branch." >&2
exit 1
fi

for attempt in $(seq 1 "$ATTEMPTS"); do
if git push origin "HEAD:refs/heads/$BRANCH"; then
echo "Pushed the release commit to $BRANCH on attempt $attempt."
Expand Down
122 changes: 113 additions & 9 deletions scripts/release-workflows.test.mjs
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=([^"]+)"/);
Expand Down Expand Up @@ -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',
Expand All @@ -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 });
Expand Down Expand Up @@ -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();

Expand All @@ -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,
Expand All @@ -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-'));

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.

P3: The test creates a temp repo via mkdtempSync and writes the fabricated TSV to the global /tmp/persona-publish-targets.tsv, but never removes either, leaving them behind after every run (and the fixed /tmp path can collide if a stale copy from an aborted run lingers). Wrap the body in try/finally and rmSync(root, { recursive: true, force: true }) (plus the /tmp TSV) after the assertions.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/release-workflows.test.mjs, line 302:

<comment>The test creates a temp repo via `mkdtempSync` and writes the fabricated TSV to the global `/tmp/persona-publish-targets.tsv`, but never removes either, leaving them behind after every run (and the fixed /tmp path can collide if a stale copy from an aborted run lingers). Wrap the body in try/finally and `rmSync(root, { recursive: true, force: true })` (plus the /tmp TSV) after the assertions.</comment>

<file context>
@@ -267,3 +291,44 @@ for (const [name, workflow] of [
+ * 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-'));
+  git(root, 'init', '-q', '-b', 'main', root);
+
</file context>

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');
});
Loading