From ddadd2ff7f36dfe0aab1b490d6392d72e0f962a3 Mon Sep 17 00:00:00 2001 From: Dan Barr <6922515+danbarr@users.noreply.github.com> Date: Mon, 2 Nov 2026 19:52:03 -0500 Subject: [PATCH] Prefer release initiator for docs ownership --- .github/workflows/upstream-release-docs.yml | 103 +++++++------- scripts/upstream-release/workflow.test.mjs | 141 ++++++++++++++++++++ 2 files changed, 199 insertions(+), 45 deletions(-) diff --git a/.github/workflows/upstream-release-docs.yml b/.github/workflows/upstream-release-docs.yml index bc4c00d2..f00545cb 100644 --- a/.github/workflows/upstream-release-docs.yml +++ b/.github/workflows/upstream-release-docs.yml @@ -485,14 +485,12 @@ jobs: # # OWNER RESOLUTION, in order. The owner is whoever cut the # release and owns driving this PR to merge. - # 1. The release author, when it is a human. Applies to - # projects where a person publishes the release - # (toolhive-studio). - # 2. The merger of the upstream `Release ` PR that the - # tag points at. Applies to projects released by a GitHub - # App, where `release.author` is the App and carries no - # human (toolhive, toolhive-registry-server). - # 3. The tagged commit's author. + # 1. The initiator recorded in the tagged commit's + # Release-Triggered-By trailer by releaseo. + # 2. The release author, when it is a human (toolhive-studio). + # 3. The merger of the upstream release PR, for bot-published + # releases without initiator metadata. + # 4. The tagged commit's author. # Every candidate is bot-filtered, so a source that yields only # a bot falls through to the next one. An empty owner is a # supported outcome: the PR body and Slack message both say the @@ -505,7 +503,7 @@ jobs: NEW: ${{ steps.detect.outputs.new_tag }} run: | # Shared bot filter, the single definition used for every - # login this step resolves: all three owner sources and the + # login this step resolves: all owner sources and the # contributor compare. Two bot spellings are in play and both # must be caught: the REST compare endpoint returns # `renovate[bot]`, while `gh pr view`/`gh release view` @@ -544,47 +542,62 @@ jobs: OWNER="" OWNER_SOURCE="" - # (1) Release author, if human. - RELEASE_AUTHOR=$(gh release view "$NEW" --repo "$REPO" \ - --json author --jq '.author.login // ""' 2>/dev/null || true) - if [ -n "$RELEASE_AUTHOR" ] && ! is_bot "$RELEASE_AUTHOR"; then - OWNER="$RELEASE_AUTHOR" - OWNER_SOURCE="release author" + # (1) Release initiator. Resolve the tag to its commit; + # annotated tags need one extra dereference. + REF_JSON=$(gh api "repos/$REPO/git/ref/tags/$NEW" 2>/dev/null || true) + OBJ_TYPE=$(printf '%s' "$REF_JSON" | jq -r '.object.type // ""' 2>/dev/null || true) + TAG_SHA=$(printf '%s' "$REF_JSON" | jq -r '.object.sha // ""' 2>/dev/null || true) + if [ "$OBJ_TYPE" = "tag" ] && [ -n "$TAG_SHA" ]; then + TAG_SHA=$(gh api "repos/$REPO/git/tags/$TAG_SHA" \ + --jq '.object.sha // ""' 2>/dev/null || true) + fi + COMMIT_JSON="" + if [ -n "$TAG_SHA" ]; then + COMMIT_JSON=$(gh api "repos/$REPO/commits/$TAG_SHA" 2>/dev/null || true) + COMMIT_MESSAGE=$(printf '%s' "$COMMIT_JSON" | jq -r '.commit.message // ""' 2>/dev/null || true) + # Squash merges can separate this trailer from Co-authored-by + # with a blank line, so inspect the complete commit message. + INITIATOR=$(printf '%s\n' "$COMMIT_MESSAGE" \ + | sed -n 's/^Release-Triggered-By:[[:blank:]]*//p') + # Accept a single GitHub login. Missing, malformed, duplicate, + # or bot trailers fall through to the existing owner sources. + if [[ "$INITIATOR" =~ ^[a-zA-Z0-9]([a-zA-Z0-9-]{0,37}[a-zA-Z0-9])?$ ]] && ! is_bot "$INITIATOR"; then + OWNER="$INITIATOR" + OWNER_SOURCE="Release-Triggered-By trailer on the tagged commit" + fi fi - # (2) Merger of the release PR the tag points at. Deref an - # annotated tag object to its commit first; a lightweight - # tag already points straight at the commit. + # (2) Release author, if human. if [ -z "$OWNER" ]; then - REF_JSON=$(gh api "repos/$REPO/git/ref/tags/$NEW" 2>/dev/null || true) - OBJ_TYPE=$(printf '%s' "$REF_JSON" | jq -r '.object.type // ""' 2>/dev/null || true) - TAG_SHA=$(printf '%s' "$REF_JSON" | jq -r '.object.sha // ""' 2>/dev/null || true) - if [ "$OBJ_TYPE" = "tag" ] && [ -n "$TAG_SHA" ]; then - TAG_SHA=$(gh api "repos/$REPO/git/tags/$TAG_SHA" \ - --jq '.object.sha // ""' 2>/dev/null || true) + RELEASE_AUTHOR=$(gh release view "$NEW" --repo "$REPO" \ + --json author --jq '.author.login // ""' 2>/dev/null || true) + if [ -n "$RELEASE_AUTHOR" ] && ! is_bot "$RELEASE_AUTHOR"; then + OWNER="$RELEASE_AUTHOR" + OWNER_SOURCE="release author" fi - if [ -n "$TAG_SHA" ]; then - # The commits//pulls list endpoint does not populate - # merged_by, so take the PR number from it and read - # mergedBy off the PR itself. - PR_NUM=$(gh api "repos/$REPO/commits/$TAG_SHA/pulls" \ - --jq 'map(select(.merged_at != null)) | .[0].number // ""' 2>/dev/null || true) - if [ -n "$PR_NUM" ]; then - MERGER=$(gh pr view "$PR_NUM" --repo "$REPO" \ - --json mergedBy --jq '.mergedBy.login // ""' 2>/dev/null || true) - if [ -n "$MERGER" ] && ! is_bot "$MERGER"; then - OWNER="$MERGER" - OWNER_SOURCE="merged release PR $REPO#$PR_NUM" - fi + fi + + # (3) Merger of the release PR the tag points at. + if [ -z "$OWNER" ] && [ -n "$TAG_SHA" ]; then + # The commits//pulls list endpoint does not populate + # merged_by, so take the PR number from it and read + # mergedBy off the PR itself. + PR_NUM=$(gh api "repos/$REPO/commits/$TAG_SHA/pulls" \ + --jq 'map(select(.merged_at != null)) | .[0].number // ""' 2>/dev/null || true) + if [ -n "$PR_NUM" ]; then + MERGER=$(gh pr view "$PR_NUM" --repo "$REPO" \ + --json mergedBy --jq '.mergedBy.login // ""' 2>/dev/null || true) + if [ -n "$MERGER" ] && ! is_bot "$MERGER"; then + OWNER="$MERGER" + OWNER_SOURCE="merged release PR $REPO#$PR_NUM" fi - # (3) Tagged commit author, last resort. - if [ -z "$OWNER" ]; then - COMMIT_AUTHOR=$(gh api "repos/$REPO/commits/$TAG_SHA" \ - --jq '.author.login // ""' 2>/dev/null || true) - if [ -n "$COMMIT_AUTHOR" ] && ! is_bot "$COMMIT_AUTHOR"; then - OWNER="$COMMIT_AUTHOR" - OWNER_SOURCE="author of the tagged commit" - fi + fi + # (4) Tagged commit author, last resort. + if [ -z "$OWNER" ]; then + COMMIT_AUTHOR=$(printf '%s' "$COMMIT_JSON" | jq -r '.author.login // ""' 2>/dev/null || true) + if [ -n "$COMMIT_AUTHOR" ] && ! is_bot "$COMMIT_AUTHOR"; then + OWNER="$COMMIT_AUTHOR" + OWNER_SOURCE="author of the tagged commit" fi fi fi diff --git a/scripts/upstream-release/workflow.test.mjs b/scripts/upstream-release/workflow.test.mjs index 79fb8ec3..6f17a36a 100644 --- a/scripts/upstream-release/workflow.test.mjs +++ b/scripts/upstream-release/workflow.test.mjs @@ -103,3 +103,144 @@ test('stops augmentation when draft conversion fails', (t) => { assert.match(result.stderr, /convertPullRequestToDraft/); assert.equal(mutation, null); }); + +const ownerStep = job.steps.find((step) => step.id === 'release_meta'); + +function resolveOwner( + t, + { + message = 'Release v0.60.0 (#7013)\n\nRelease-Triggered-By: jhrozek\n\nCo-authored-by: release bot ', + annotated = true, + releaseAuthor = 'app/toolhive-release-app', + merger = 'ChrisJBurns', + commitAuthor = 'toolhive-release-app[bot]', + commitUnavailable = false, + tagUnavailable = false, + } = {} +) { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'release-owner-')); + t.after(() => fs.rmSync(root, { recursive: true, force: true })); + const bin = path.join(root, 'bin'); + fs.mkdirSync(bin); + fs.writeFileSync( + path.join(bin, 'gh'), + `#!/bin/sh +case "$1 $2" in + 'api repos/stacklok/toolhive/compare/v0.59.0...v0.60.0') + printf '%s\\n' '{"commits":[],"total_commits":0}' ;; + 'api repos/stacklok/toolhive/git/ref/tags/v0.60.0') + [ "$TAG_UNAVAILABLE" = false ] || exit 1 + printf '%s\\n' "$TAG_JSON" ;; + 'api repos/stacklok/toolhive/git/tags/tag-object') + printf '%s\\n' 'release-commit' ;; + 'api repos/stacklok/toolhive/commits/release-commit') + [ "$COMMIT_UNAVAILABLE" = false ] || exit 1 + printf '%s\\n' "$FIXTURE_COMMIT_JSON" ;; + 'api repos/stacklok/toolhive/commits/release-commit/pulls') + printf '%s\\n' '7013' ;; + 'release view') printf '%s\\n' "$RELEASE_AUTHOR" ;; + 'pr view') printf '%s\\n' "$MERGER" ;; + *) echo "Unexpected gh call: $*" >&2; exit 1 ;; +esac +`, + { mode: 0o755 } + ); + const outputFile = path.join(root, 'output'); + const result = spawnSync('bash', ['-eo', 'pipefail', '-c', ownerStep.run], { + cwd: root, + encoding: 'utf8', + env: { + ...process.env, + PATH: `${bin}${path.delimiter}${process.env.PATH}`, + GITHUB_OUTPUT: outputFile, + REPO: 'stacklok/toolhive', + PREV: 'v0.59.0', + NEW: 'v0.60.0', + TAG_JSON: JSON.stringify({ + object: { + type: annotated ? 'tag' : 'commit', + sha: annotated ? 'tag-object' : 'release-commit', + }, + }), + FIXTURE_COMMIT_JSON: JSON.stringify({ + commit: { message }, + author: { login: commitAuthor }, + }), + RELEASE_AUTHOR: releaseAuthor, + MERGER: merger, + COMMIT_UNAVAILABLE: String(commitUnavailable), + TAG_UNAVAILABLE: String(tagUnavailable), + }, + }); + assert.equal(result.status, 0, result.stderr); + assert.doesNotMatch(result.stderr, /Unexpected gh call/); + const meta = JSON.parse( + fs.readFileSync(path.join(root, '.release-meta.json'), 'utf8') + ); + const output = fs.readFileSync(outputFile, 'utf8'); + assert.ok(output.includes(`owner=${meta.owner || ''}\n`)); + assert.ok(output.includes(`owner_source=${meta.owner_source || ''}\n`)); + return meta; +} + +for (const annotated of [true, false]) { + test(`prefers the release initiator for ${annotated ? 'annotated' : 'lightweight'} tags`, (t) => { + const meta = resolveOwner(t, { annotated, releaseAuthor: 'publisher' }); + assert.equal(meta.owner, 'jhrozek'); + assert.equal( + meta.owner_source, + 'Release-Triggered-By trailer on the tagged commit' + ); + }); +} + +for (const message of [ + 'Release v0.60.0', + 'Release v0.60.0\n\nRelease-Triggered-By: renovate', + 'Release v0.60.0\n\nRelease-Triggered-By: toolhive-release-app[bot]', + 'Release v0.60.0\n\nRelease-Triggered-By: app/toolhive-release-app', + 'Release v0.60.0\n\nRelease-Triggered-By: invalid login', + 'Release v0.60.0\n\nRelease-Triggered-By: alice\nRelease-Triggered-By: bob', + 'Release v0.60.0\n\nDocumentation mentions Release-Triggered-By: alice', +]) { + test(`falls back to the release PR merger for unusable metadata: ${JSON.stringify(message)}`, (t) => { + const meta = resolveOwner(t, { message }); + assert.equal(meta.owner, 'ChrisJBurns'); + assert.equal(meta.owner_source, 'merged release PR stacklok/toolhive#7013'); + }); +} + +test('retains human release author fallback when initiator is absent', (t) => { + const meta = resolveOwner(t, { message: '', releaseAuthor: 'publisher' }); + assert.equal(meta.owner, 'publisher'); + assert.equal(meta.owner_source, 'release author'); +}); + +test('falls back to the merger when tagged commit metadata is unavailable', (t) => { + const meta = resolveOwner(t, { commitUnavailable: true }); + assert.equal(meta.owner, 'ChrisJBurns'); +}); + +test('retains human release author fallback when tag lookup fails', (t) => { + const meta = resolveOwner(t, { + tagUnavailable: true, + releaseAuthor: 'publisher', + }); + assert.equal(meta.owner, 'publisher'); +}); + +test('retains human tagged commit author fallback when the merger is a bot', (t) => { + const meta = resolveOwner(t, { + message: '', + merger: 'app/release-bot', + commitAuthor: 'commit-author', + }); + assert.equal(meta.owner, 'commit-author'); + assert.equal(meta.owner_source, 'author of the tagged commit'); +}); + +test('leaves the owner unresolved when all sources are bots', (t) => { + const meta = resolveOwner(t, { message: '', merger: 'app/release-bot' }); + assert.equal(meta.owner, null); + assert.equal(meta.owner_source, null); +});