From b529a9073eb61665a4540f7b13243366c2208559 Mon Sep 17 00:00:00 2001 From: Hal Eisen Date: Thu, 27 Aug 2026 10:19:08 -0700 Subject: [PATCH 1/9] ADFA-5317: Advance the linked Jira ticket to QA when a PR is approved A reviewer approves a PR and the ADFA ticket stays where it was, because moving it depends on someone remembering. The board then stops matching reality, which makes standups and planning unreliable. This adds a workflow that moves the linked ticket to QA on an approving review. Testing happens on the feature branch, not on stage: every push to a non-main branch already builds an APK, ships it to the Firebase testers group, and posts a Slack notification. The build QA needs therefore exists the moment the PR is approved, which is when the ticket should enter QA. Jira's transitions are gated and linear, so a ticket left behind in To Do or In Progress cannot jump straight to QA. The job walks it forward one hop at a time, resolving each hop by target status name from the live transitions endpoint rather than hardcoding transition IDs. Tickets already at or past QA are left alone; nothing ever moves backwards. Three guards are specific to pull_request_review, which runs in the base repo context with full access to secrets even for pull requests from forks: - No actions/checkout, so no pull request code ever runs on the runner. - Every payload field is read through github-script's context rather than interpolated into a shell, so a crafted branch name or PR title is never parsed as source. - The repo is public and any user may submit an approving review, which fires this event without satisfying branch protection. The job requires an author_association of OWNER, MEMBER, or COLLABORATOR. A Jira outage or auth failure produces a warning, never a red check. The workflow deliberately does not gate on the build being green: if the build were red at approval time and went green later, no review event would fire again and the ticket would silently never move, reproducing the failure this is meant to eliminate. Verified against live Jira using a throwaway ticket, since deleted: a ticket in To Do walked three hops to QA, a second approval was a no-op, and both community/ and keyless branches were skipped. --- .github/workflows/jira-advance-to-qa.yml | 157 +++++++++++++++++++++++ 1 file changed, 157 insertions(+) create mode 100644 .github/workflows/jira-advance-to-qa.yml diff --git a/.github/workflows/jira-advance-to-qa.yml b/.github/workflows/jira-advance-to-qa.yml new file mode 100644 index 0000000000..f39097b296 --- /dev/null +++ b/.github/workflows/jira-advance-to-qa.yml @@ -0,0 +1,157 @@ +name: Advance Jira Ticket to QA + +# Runs on pull_request_review so it can reach the Jira secrets, including for +# pull requests from forks. It must never check out or execute code from the +# pull request. All pull request data is read through github-script's `context` +# rather than `${{ }}` interpolation, so a crafted branch name or title is never +# parsed as source. +on: + pull_request_review: + types: [ submitted ] + +permissions: { } + +jobs: + advance_to_qa: + name: Move linked Jira ticket to QA + runs-on: ubuntu-latest + # Anyone with read access to a public repo can submit an approving review. + # Such a review does not satisfy branch protection, but it does fire this + # event, so restrict the automation to people who can actually merge. + if: >- + github.event.review.state == 'approved' && + contains(fromJSON('["OWNER", "MEMBER", "COLLABORATOR"]'), github.event.review.author_association) + steps: + - name: Walk the ticket forward to QA + uses: actions/github-script@v7 + env: + JIRA_EMAIL: ${{ secrets.JIRA_EMAIL }} + JIRA_API_TOKEN: ${{ secrets.JIRA_API_TOKEN }} + with: + script: | + const JIRA_BASE = 'https://appdevforall.atlassian.net'; + + // Case-sensitive, in board order. Note the lowercase "review" and + // "merge" -- Jira matches these exactly. + const STATUSES = ['To Do', 'In Progress', 'Code review', 'QA', 'Ready to merge', 'Done']; + const TARGET = 'QA'; + const TARGET_INDEX = STATUSES.indexOf(TARGET); + + const pr = context.payload.pull_request; + const review = context.payload.review; + + const branch = pr.head.ref; + if (branch.startsWith('community/')) { + core.info(`Branch "${branch}" is a community contribution; no Jira ticket to advance.`); + return; + } + + // The key appears in the branch name and the PR title with equal + // reliability and never disagrees between them, so either source + // works; the branch is the more structured of the two. + const match = branch.match(/ADFA-\d+/i) || pr.title.match(/ADFA-\d+/i); + if (!match) { + core.info(`No ADFA ticket referenced by branch "${branch}" or the pull request title; nothing to do.`); + return; + } + const key = match[0].toUpperCase(); + + const email = process.env.JIRA_EMAIL; + const token = process.env.JIRA_API_TOKEN; + if (!email || !token) { + core.warning(`Jira credentials are not configured; leaving ${key} untouched.`); + return; + } + const auth = 'Basic ' + Buffer.from(`${email}:${token}`).toString('base64'); + + const jira = async (path, init = {}) => { + const response = await fetch(`${JIRA_BASE}/rest/api/3${path}`, { + ...init, + headers: { + Authorization: auth, + Accept: 'application/json', + ...(init.body ? { 'Content-Type': 'application/json' } : {}), + }, + }); + if (!response.ok) { + throw new Error(`${init.method || 'GET'} ${path} returned HTTP ${response.status}`); + } + return response.status === 204 ? null : response.json(); + }; + + try { + const issue = await jira(`/issue/${key}?fields=status`); + const startedAt = issue.fields.status.name; + let current = startedAt; + let index = STATUSES.indexOf(current); + + if (index === -1) { + core.warning(`${key} is in unrecognized status "${current}"; leaving it untouched.`); + return; + } + if (index >= TARGET_INDEX) { + core.info(`${key} is already at "${current}", which is at or past ${TARGET}; nothing to do.`); + return; + } + + // Jira's transitions are gated and linear, so a ticket left behind + // in "To Do" or "In Progress" cannot jump straight to QA. Walking + // it forward one hop at a time is the whole point of this job: + // the ticket that is behind belongs to the person who forgot. + const walked = [current]; + while (index < TARGET_INDEX) { + const next = STATUSES[index + 1]; + const { transitions } = await jira(`/issue/${key}/transitions`); + const hop = transitions.find(transition => transition.to && transition.to.name === next); + + if (!hop) { + const offered = transitions.map(transition => transition.to && transition.to.name).join(', '); + core.warning( + `${key} is in "${current}" and offers no transition to "${next}" (available: ${offered}). ` + + `Stopping here; the ticket needs to be moved by hand.` + ); + return; + } + + await jira(`/issue/${key}/transitions`, { + method: 'POST', + body: JSON.stringify({ transition: { id: hop.id } }), + }); + + current = next; + index += 1; + walked.push(current); + } + + const trail = walked.join(' -> '); + const reviewer = review.user.login; + + await jira(`/issue/${key}/comment`, { + method: 'POST', + body: JSON.stringify({ + body: { + type: 'doc', + version: 1, + content: [{ + type: 'paragraph', + content: [ + { type: 'text', text: `Automatically moved to ${TARGET} (${trail}): ` }, + { + type: 'text', + text: `pull request #${pr.number}`, + marks: [{ type: 'link', attrs: { href: pr.html_url } }], + }, + { type: 'text', text: ` was approved by ${reviewer}.` }, + ], + }], + }, + }), + }); + + core.info(`${key}: ${trail}`); + core.summary.addRaw(`Moved [${key}](${JIRA_BASE}/browse/${key}) to ${TARGET}: ${trail}`); + await core.summary.write(); + } catch (error) { + // A Jira outage or auth problem must never turn a pull request red. + core.warning(`Could not advance ${key} to ${TARGET}: ${error.message}`); + } From 9a17bb8deeb529a6e6b2172d940b87289ac5041b Mon Sep 17 00:00:00 2001 From: Hal Eisen Date: Thu, 27 Aug 2026 18:24:47 -0700 Subject: [PATCH 2/9] ADFA-5317: Bound Jira requests and authorize reviewers by permission Two review findings. Node's fetch imposes no deadline on a response, so a hung or half-delivered reply from Jira would stall the job rather than fail it. Every request now carries AbortSignal.timeout(30s), which routes the abort into the existing catch and keeps it a warning. author_association does not prove write access: an org member may have no access to this repository at all, and a collaborator may be read-only. Both would have passed the old guard. The job now asks for the reviewer's effective permission and continues only for admin or write -- the legacy permission field reports maintain as write, so those two values cover admin, maintain, and write. The association test stays only as a cheap pre-filter that avoids starting a runner for a drive-by approval; it is no longer the authorization decision. The lookup fails closed. GitHub does not document which GITHUB_TOKEN permission this endpoint needs, so the job requests contents: read and, if the lookup fails anyway, warns and leaves the ticket untouched rather than falling back to the weaker signal. The first approval after merge will show in the Actions log whether the grant is sufficient. Verified by running the script extracted from the YAML against live Jira with the github-script globals stubbed, using a throwaway ticket since deleted: read permission skipped, a failing lookup warned and made no change, a 1 ms timeout aborted into the catch without throwing, write walked To Do to QA in three hops, and a second approval was a no-op. --- .github/workflows/jira-advance-to-qa.yml | 39 ++++++++++++++++++++++-- 1 file changed, 36 insertions(+), 3 deletions(-) diff --git a/.github/workflows/jira-advance-to-qa.yml b/.github/workflows/jira-advance-to-qa.yml index f39097b296..01e67b9583 100644 --- a/.github/workflows/jira-advance-to-qa.yml +++ b/.github/workflows/jira-advance-to-qa.yml @@ -9,7 +9,8 @@ on: pull_request_review: types: [ submitted ] -permissions: { } +permissions: + contents: read jobs: advance_to_qa: @@ -17,7 +18,9 @@ jobs: runs-on: ubuntu-latest # Anyone with read access to a public repo can submit an approving review. # Such a review does not satisfy branch protection, but it does fire this - # event, so restrict the automation to people who can actually merge. + # event. This condition is only a cheap pre-filter to avoid starting a + # runner for a drive-by approval; the authorization decision is the + # effective-permission check in the script, not the association. if: >- github.event.review.state == 'approved' && contains(fromJSON('["OWNER", "MEMBER", "COLLABORATOR"]'), github.event.review.author_association) @@ -31,6 +34,11 @@ jobs: script: | const JIRA_BASE = 'https://appdevforall.atlassian.net'; + // Node's fetch imposes no deadline on a response, so a hung or + // half-delivered reply would stall the job rather than fail it. + // Aborting routes it into the catch below, where it stays a warning. + const JIRA_REQUEST_TIMEOUT_MS = 30000; + // Case-sensitive, in board order. Note the lowercase "review" and // "merge" -- Jira matches these exactly. const STATUSES = ['To Do', 'In Progress', 'Code review', 'QA', 'Ready to merge', 'Done']; @@ -56,6 +64,31 @@ jobs: } const key = match[0].toUpperCase(); + // author_association does not prove write access: an org member may + // have no access to this repo, and a collaborator may be read-only. + // Ask for the reviewer's effective permission instead. The legacy + // `permission` field reports "maintain" as "write", so those two + // values cover admin, maintain, and write. + const reviewer = review.user.login; + let permission; + try { + const { data } = await github.rest.repos.getCollaboratorPermissionLevel({ + owner: context.repo.owner, + repo: context.repo.repo, + username: reviewer, + }); + permission = data.permission; + } catch (error) { + // Fail closed: without a permission answer, do not touch the board. + core.warning(`Could not read ${reviewer}'s permission on this repository (${error.message}); leaving ${key} untouched.`); + return; + } + + if (!['admin', 'write'].includes(permission)) { + core.info(`${reviewer} has "${permission}" permission and cannot merge; leaving ${key} untouched.`); + return; + } + const email = process.env.JIRA_EMAIL; const token = process.env.JIRA_API_TOKEN; if (!email || !token) { @@ -72,6 +105,7 @@ jobs: Accept: 'application/json', ...(init.body ? { 'Content-Type': 'application/json' } : {}), }, + signal: AbortSignal.timeout(JIRA_REQUEST_TIMEOUT_MS), }); if (!response.ok) { throw new Error(`${init.method || 'GET'} ${path} returned HTTP ${response.status}`); @@ -124,7 +158,6 @@ jobs: } const trail = walked.join(' -> '); - const reviewer = review.user.login; await jira(`/issue/${key}/comment`, { method: 'POST', From 9cbf27e6c483d7b6520eedf1db6ad151f186fa51 Mon Sep 17 00:00:00 2001 From: itsaky-adfa Date: Tue, 1 Sep 2026 20:23:22 +0530 Subject: [PATCH 3/9] Move the ticket only when the whole stack is approved, and serialize the walk --- .github/workflows/jira-advance-to-qa.yml | 197 ++++++++++++++++++----- 1 file changed, 155 insertions(+), 42 deletions(-) diff --git a/.github/workflows/jira-advance-to-qa.yml b/.github/workflows/jira-advance-to-qa.yml index 01e67b9583..f3cabbc09b 100644 --- a/.github/workflows/jira-advance-to-qa.yml +++ b/.github/workflows/jira-advance-to-qa.yml @@ -11,55 +11,49 @@ on: permissions: contents: read + pull-requests: read jobs: - advance_to_qa: - name: Move linked Jira ticket to QA + resolve: + name: Resolve the ticket and check the rest of its pull requests runs-on: ubuntu-latest # Anyone with read access to a public repo can submit an approving review. # Such a review does not satisfy branch protection, but it does fire this - # event. This condition is only a cheap pre-filter to avoid starting a + # event. The association test is only a cheap pre-filter to avoid starting a # runner for a drive-by approval; the authorization decision is the - # effective-permission check in the script, not the association. + # effective-permission check in the script. A review can also be submitted + # against a closed, merged or draft pull request, none of which mean the + # work is ready for QA. if: >- github.event.review.state == 'approved' && + github.event.pull_request.state == 'open' && + !github.event.pull_request.draft && contains(fromJSON('["OWNER", "MEMBER", "COLLABORATOR"]'), github.event.review.author_association) + outputs: + key: ${{ steps.resolve.outputs.key }} steps: - - name: Walk the ticket forward to QA - uses: actions/github-script@v7 - env: - JIRA_EMAIL: ${{ secrets.JIRA_EMAIL }} - JIRA_API_TOKEN: ${{ secrets.JIRA_API_TOKEN }} + - name: Resolve the ticket key + id: resolve + uses: actions/github-script@f28e40c7f34bde8b3046d885e986cb6290c5673b # v7.1.0 with: script: | - const JIRA_BASE = 'https://appdevforall.atlassian.net'; - - // Node's fetch imposes no deadline on a response, so a hung or - // half-delivered reply would stall the job rather than fail it. - // Aborting routes it into the catch below, where it stays a warning. - const JIRA_REQUEST_TIMEOUT_MS = 30000; - - // Case-sensitive, in board order. Note the lowercase "review" and - // "merge" -- Jira matches these exactly. - const STATUSES = ['To Do', 'In Progress', 'Code review', 'QA', 'Ready to merge', 'Done']; - const TARGET = 'QA'; - const TARGET_INDEX = STATUSES.indexOf(TARGET); + const KEY_PATTERN = /ADFA-\d+/i; const pr = context.payload.pull_request; - const review = context.payload.review; - const branch = pr.head.ref; + if (branch.startsWith('community/')) { core.info(`Branch "${branch}" is a community contribution; no Jira ticket to advance.`); return; } - // The key appears in the branch name and the PR title with equal - // reliability and never disagrees between them, so either source - // works; the branch is the more structured of the two. - const match = branch.match(/ADFA-\d+/i) || pr.title.match(/ADFA-\d+/i); + // The branch is the only source of the key that the author of an + // untrusted pull request cannot choose freely; a title reading + // "fixes the ADFA-1234 crash" would otherwise pick the ticket that + // moves. debug.yml extracts from the branch alone for the same reason. + const match = branch.match(KEY_PATTERN); if (!match) { - core.info(`No ADFA ticket referenced by branch "${branch}" or the pull request title; nothing to do.`); + core.info(`No ADFA ticket referenced by branch "${branch}"; nothing to do.`); return; } const key = match[0].toUpperCase(); @@ -69,7 +63,7 @@ jobs: // Ask for the reviewer's effective permission instead. The legacy // `permission` field reports "maintain" as "write", so those two // values cover admin, maintain, and write. - const reviewer = review.user.login; + const reviewer = context.payload.review.user.login; let permission; try { const { data } = await github.rest.repos.getCollaboratorPermissionLevel({ @@ -79,8 +73,13 @@ jobs: }); permission = data.permission; } catch (error) { - // Fail closed: without a permission answer, do not touch the board. - core.warning(`Could not read ${reviewer}'s permission on this repository (${error.message}); leaving ${key} untouched.`); + /* + * Fail closed, and fail loudly. A permission lookup that cannot run + * is a broken automation, not a transient outage: every later + * approval would end as a warning under a green check, which is the + * same silent drift this job exists to remove. + */ + core.setFailed(`Could not read ${reviewer}'s permission on this repository (${error.message}); leaving ${key} untouched.`); return; } @@ -89,6 +88,88 @@ jobs: return; } + /* + * One ticket can own a stack of pull requests. Moving it on the first + * approval hands QA a ticket whose code is four branches from landing, + * and the later approvals then find it already at QA and say nothing. + * Advance only once every open pull request carrying this key has an + * approval of its own. + */ + const open = await github.paginate(github.rest.pulls.list, { + owner: context.repo.owner, + repo: context.repo.repo, + state: 'open', + per_page: 100, + }); + const siblings = open.filter(candidate => { + if (candidate.number === pr.number) return false; + const key2 = candidate.head.ref.match(KEY_PATTERN); + return key2 && key2[0].toUpperCase() === key; + }); + + for (const sibling of siblings) { + if (sibling.draft) { + core.info(`${key} also has draft pull request #${sibling.number}; leaving the ticket where it is.`); + return; + } + const reviews = await github.paginate(github.rest.pulls.listReviews, { + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: sibling.number, + per_page: 100, + }); + // A reviewer's verdict is their latest non-comment review. + const verdicts = new Map(); + for (const submitted of reviews) { + if (submitted.state === 'COMMENTED' || submitted.state === 'PENDING') continue; + verdicts.set(submitted.user.login, submitted.state); + } + if (![...verdicts.values()].includes('APPROVED')) { + core.info(`${key} also has open pull request #${sibling.number}, which is not approved; leaving the ticket where it is.`); + return; + } + } + + core.setOutput('key', key); + + advance: + name: Move linked Jira ticket to QA + runs-on: ubuntu-latest + needs: resolve + if: needs.resolve.outputs.key != '' + # Two approvals landing seconds apart otherwise run two walks over one + # ticket. Backward transitions are global in this Jira workflow, so a stale + # walk can pull a ticket back out of QA. The group has to key on the ticket, + # not the pull request: a stack shares one ticket across several of them. + concurrency: + group: jira-advance-${{ needs.resolve.outputs.key }} + cancel-in-progress: false + steps: + - name: Walk the ticket forward to QA + uses: actions/github-script@f28e40c7f34bde8b3046d885e986cb6290c5673b # v7.1.0 + env: + JIRA_EMAIL: ${{ secrets.JIRA_EMAIL }} + JIRA_API_TOKEN: ${{ secrets.JIRA_API_TOKEN }} + TICKET_KEY: ${{ needs.resolve.outputs.key }} + with: + script: | + const JIRA_BASE = 'https://appdevforall.atlassian.net'; + + // Node's fetch imposes no deadline on a response, so a hung or + // half-delivered reply would stall the job rather than fail it. + // Aborting routes it into the catch below, where it stays a warning. + const JIRA_REQUEST_TIMEOUT_MS = 30000; + + // Case-sensitive, in board order. Note the lowercase "review" and + // "merge" -- Jira matches these exactly. + const STATUSES = ['To Do', 'In Progress', 'Code review', 'QA', 'Ready to merge', 'Done']; + const TARGET = 'QA'; + const TARGET_INDEX = STATUSES.indexOf(TARGET); + + const pr = context.payload.pull_request; + const reviewer = context.payload.review.user.login; + const key = process.env.TICKET_KEY; + const email = process.env.JIRA_EMAIL; const token = process.env.JIRA_API_TOKEN; if (!email || !token) { @@ -113,10 +194,26 @@ jobs: return response.status === 204 ? null : response.json(); }; + const statusOf = async () => (await jira(`/issue/${key}?fields=status`)).fields.status.name; + + /* + * Declared outside the try so the catch can report how far the walk + * got. A hop is committed as it is made and there is no rollback, so + * a failure part-way leaves the ticket somewhere new, and a log line + * claiming it did not move sends whoever repairs the board to the + * wrong place. + */ + let current = null; + const walked = []; + + const trailSuffix = () => { + if (walked.length <= 1) return ' The ticket was not moved.'; + return ` It was moved ${walked.join(' -> ')} and is now at "${current}"; finish it by hand.`; + }; + try { - const issue = await jira(`/issue/${key}?fields=status`); - const startedAt = issue.fields.status.name; - let current = startedAt; + current = await statusOf(); + walked.push(current); let index = STATUSES.indexOf(current); if (index === -1) { @@ -128,21 +225,37 @@ jobs: return; } - // Jira's transitions are gated and linear, so a ticket left behind - // in "To Do" or "In Progress" cannot jump straight to QA. Walking - // it forward one hop at a time is the whole point of this job: - // the ticket that is behind belongs to the person who forgot. - const walked = [current]; + /* + * There is no direct edge from "To Do" or "In Progress" to QA, so a + * ticket left behind has to be walked forward one hop at a time -- + * the ticket that is behind belongs to the person who forgot. + * Backward transitions, by contrast, are global from every status, + * which is why each hop re-reads the ticket first: a walk that + * blindly asks for the next status can drag a ticket that someone + * else already advanced back down the board. + */ while (index < TARGET_INDEX) { + const observed = await statusOf(); + if (observed !== current) { + core.warning( + `${key} moved to "${observed}" while this run was walking it from "${current}"; ` + + `something else is moving the same ticket. Stopping.${trailSuffix()}` + ); + return; + } + const next = STATUSES[index + 1]; const { transitions } = await jira(`/issue/${key}/transitions`); const hop = transitions.find(transition => transition.to && transition.to.name === next); if (!hop) { - const offered = transitions.map(transition => transition.to && transition.to.name).join(', '); + const offered = transitions + .map(transition => transition.to && transition.to.name) + .filter(Boolean) + .join(', '); core.warning( `${key} is in "${current}" and offers no transition to "${next}" (available: ${offered}). ` + - `Stopping here; the ticket needs to be moved by hand.` + `Stopping here.${trailSuffix()}` ); return; } @@ -186,5 +299,5 @@ jobs: await core.summary.write(); } catch (error) { // A Jira outage or auth problem must never turn a pull request red. - core.warning(`Could not advance ${key} to ${TARGET}: ${error.message}`); + core.warning(`Could not advance ${key} to ${TARGET}: ${error.message}.${trailSuffix()}`); } From 85ed7d6134e9ef49369ebdb60c8423f6cb62c992 Mon Sep 17 00:00:00 2001 From: itsaky-adfa Date: Tue, 1 Sep 2026 21:05:12 +0530 Subject: [PATCH 4/9] Refactor Jira advancement logic in workflow Refactor Jira ticket advancement logic to improve clarity and error handling. --- .github/workflows/jira-advance-to-qa.yml | 251 +++++++++++++++-------- 1 file changed, 166 insertions(+), 85 deletions(-) diff --git a/.github/workflows/jira-advance-to-qa.yml b/.github/workflows/jira-advance-to-qa.yml index f3cabbc09b..4978ed7db8 100644 --- a/.github/workflows/jira-advance-to-qa.yml +++ b/.github/workflows/jira-advance-to-qa.yml @@ -41,16 +41,30 @@ jobs: const pr = context.payload.pull_request; const branch = pr.head.ref; + const BASE_REPO_ID = context.payload.repository.id; if (branch.startsWith('community/')) { core.info(`Branch "${branch}" is a community contribution; no Jira ticket to advance.`); return; } - // The branch is the only source of the key that the author of an - // untrusted pull request cannot choose freely; a title reading - // "fixes the ADFA-1234 crash" would otherwise pick the ticket that - // moves. debug.yml extracts from the branch alone for the same reason. + /* + * A fork's head branch is named inside the author's own repository, + * so its ADFA key is evidence of nothing: a fork branch called + * "ADFA-1234-crash" would bind an unrelated ticket that a + * maintainer's approval then walks to QA. External contributions are + * expected to carry the `community/` prefix and have no ticket of + * their own in any case. + */ + if (pr.head.repo?.id !== BASE_REPO_ID) { + core.info(`Pull request #${pr.number} is from a fork; no Jira ticket to advance.`); + return; + } + + // The key comes from the branch rather than the title: a title + // reading "fixes the ADFA-1234 crash" would otherwise pick the ticket + // that moves. debug.yml extracts from the branch alone for the same + // reason. const match = branch.match(KEY_PATTERN); if (!match) { core.info(`No ADFA ticket referenced by branch "${branch}"; nothing to do.`); @@ -58,79 +72,136 @@ jobs: } const key = match[0].toUpperCase(); - // author_association does not prove write access: an org member may - // have no access to this repo, and a collaborator may be read-only. - // Ask for the reviewer's effective permission instead. The legacy - // `permission` field reports "maintain" as "write", so those two - // values cover admin, maintain, and write. - const reviewer = context.payload.review.user.login; - let permission; - try { - const { data } = await github.rest.repos.getCollaboratorPermissionLevel({ - owner: context.repo.owner, - repo: context.repo.repo, - username: reviewer, - }); - permission = data.permission; - } catch (error) { + /* + * author_association does not prove write access: an org member may + * have no access to this repo, and a collaborator may be read-only. + * Ask for the effective permission instead, once per login, since the + * same reviewer usually appears on every branch of a stack. + */ + const permissions = new Map(); + const canMerge = async login => { + if (!permissions.has(login)) { + let permission; + try { + const { data } = await github.rest.repos.getCollaboratorPermissionLevel({ + owner: context.repo.owner, + repo: context.repo.repo, + username: login, + }); + permission = data.permission; + } catch (error) { + /* + * A 404 answers the question with "no access" -- App bots review + * too, and a "name[bot]" login is not a username. Anything else + * is a broken automation rather than a verdict: the token cannot + * read collaborators, and every later approval would end as a + * warning under a green check, which is the silent drift this + * job exists to remove. Let it out to the catch below and turn + * the run red. + */ + if (error.status !== 404) { + throw new Error(`could not read ${login}'s permission on this repository (${error.message})`); + } + permission = 'none'; + } + permissions.set(login, permission); + } /* - * Fail closed, and fail loudly. A permission lookup that cannot run - * is a broken automation, not a transient outage: every later - * approval would end as a warning under a green check, which is the - * same silent drift this job exists to remove. + * The legacy `permission` field reports "maintain" as "write", so + * those two values cover admin, maintain and write. */ - core.setFailed(`Could not read ${reviewer}'s permission on this repository (${error.message}); leaving ${key} untouched.`); - return; - } + return ['admin', 'write'].includes(permissions.get(login)); + }; - if (!['admin', 'write'].includes(permission)) { - core.info(`${reviewer} has "${permission}" permission and cannot merge; leaving ${key} untouched.`); - return; - } + const reviewer = context.payload.review.user.login; - /* - * One ticket can own a stack of pull requests. Moving it on the first - * approval hands QA a ticket whose code is four branches from landing, - * and the later approvals then find it already at QA and say nothing. - * Advance only once every open pull request carrying this key has an - * approval of its own. - */ - const open = await github.paginate(github.rest.pulls.list, { - owner: context.repo.owner, - repo: context.repo.repo, - state: 'open', - per_page: 100, - }); - const siblings = open.filter(candidate => { - if (candidate.number === pr.number) return false; - const key2 = candidate.head.ref.match(KEY_PATTERN); - return key2 && key2[0].toUpperCase() === key; - }); - - for (const sibling of siblings) { - if (sibling.draft) { - core.info(`${key} also has draft pull request #${sibling.number}; leaving the ticket where it is.`); + try { + if (!await canMerge(reviewer)) { + core.info(`${reviewer} has "${permissions.get(reviewer)}" permission and cannot merge; leaving ${key} untouched.`); return; } - const reviews = await github.paginate(github.rest.pulls.listReviews, { + + /* + * One ticket can own a stack of pull requests. Moving it on the + * first approval hands QA a ticket whose code is four branches from + * landing, and the later approvals then find it already at QA and + * say nothing. Advance only once every open pull request carrying + * this key -- the one that fired this event included -- is approved + * with no change request outstanding. + */ + const open = await github.paginate(github.rest.pulls.list, { owner: context.repo.owner, repo: context.repo.repo, - pull_number: sibling.number, + state: 'open', per_page: 100, }); - // A reviewer's verdict is their latest non-comment review. - const verdicts = new Map(); - for (const submitted of reviews) { - if (submitted.state === 'COMMENTED' || submitted.state === 'PENDING') continue; - verdicts.set(submitted.user.login, submitted.state); + /* + * Fork heads are excluded for the reason given above, and because a + * stranger could otherwise hold the board still by opening a fork + * branch named after someone else's ticket. + */ + const stack = open.filter(candidate => { + if (candidate.head.repo?.id !== BASE_REPO_ID) return false; + const candidateKey = candidate.head.ref.match(KEY_PATTERN); + return candidateKey && candidateKey[0].toUpperCase() === key; + }); + if (!stack.some(member => member.number === pr.number)) { + stack.push(pr); } - if (![...verdicts.values()].includes('APPROVED')) { - core.info(`${key} also has open pull request #${sibling.number}, which is not approved; leaving the ticket where it is.`); - return; + + for (const member of stack) { + const label = member.number === pr.number + ? `this pull request (#${member.number})` + : `pull request #${member.number}`; + + if (member.draft) { + core.info(`${key}: ${label} is a draft; leaving the ticket where it is.`); + return; + } + + const reviews = await github.paginate(github.rest.pulls.listReviews, { + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: member.number, + per_page: 100, + }); + + /* + * A reviewer's verdict is their latest non-comment review, and + * only the verdicts of people who could merge count: anyone can + * review a public repo, so counting everyone would let a drive-by + * approval satisfy this gate and a drive-by change request hold + * the board still. + */ + const verdicts = new Map(); + for (const submitted of reviews) { + if (submitted.state === 'COMMENTED' || submitted.state === 'PENDING') continue; + if (!submitted.user || !await canMerge(submitted.user.login)) continue; + verdicts.set(submitted.user.login, submitted.state); + } + /* + * The review that fired this event is the newest verdict of the + * reviewer who submitted it; listReviews need not show it yet. + */ + if (member.number === pr.number) { + verdicts.set(reviewer, 'APPROVED'); + } + + const states = [...verdicts.values()]; + if (states.includes('CHANGES_REQUESTED')) { + core.info(`${key}: ${label} has an outstanding change request; leaving the ticket where it is.`); + return; + } + if (!states.includes('APPROVED')) { + core.info(`${key}: ${label} is not approved; leaving the ticket where it is.`); + return; + } } - } - core.setOutput('key', key); + core.setOutput('key', key); + } catch (error) { + core.setFailed(`Cannot decide whether ${key} may advance: ${error.message}. Leaving it untouched.`); + } advance: name: Move linked Jira ticket to QA @@ -272,27 +343,37 @@ jobs: const trail = walked.join(' -> '); - await jira(`/issue/${key}/comment`, { - method: 'POST', - body: JSON.stringify({ - body: { - type: 'doc', - version: 1, - content: [{ - type: 'paragraph', - content: [ - { type: 'text', text: `Automatically moved to ${TARGET} (${trail}): ` }, - { - type: 'text', - text: `pull request #${pr.number}`, - marks: [{ type: 'link', attrs: { href: pr.html_url } }], - }, - { type: 'text', text: ` was approved by ${reviewer}.` }, - ], - }], - }, - }), - }); + /* + * The walk is already committed by this point, so a comment that + * fails to post must not be reported as a failure to advance: that + * sends whoever reads the log to a ticket already sitting where it + * belongs. + */ + try { + await jira(`/issue/${key}/comment`, { + method: 'POST', + body: JSON.stringify({ + body: { + type: 'doc', + version: 1, + content: [{ + type: 'paragraph', + content: [ + { type: 'text', text: `Automatically moved to ${TARGET} (${trail}): ` }, + { + type: 'text', + text: `pull request #${pr.number}`, + marks: [{ type: 'link', attrs: { href: pr.html_url } }], + }, + { type: 'text', text: ` was approved by ${reviewer}.` }, + ], + }], + }, + }), + }); + } catch (error) { + core.warning(`${key} reached ${TARGET} (${trail}), but the Jira comment recording why could not be posted: ${error.message}.`); + } core.info(`${key}: ${trail}`); core.summary.addRaw(`Moved [${key}](${JIRA_BASE}/browse/${key}) to ${TARGET}: ${trail}`); From 194d33969e565a5cd3b3fc77c5c43c6ab4b4f0bc Mon Sep 17 00:00:00 2001 From: itsaky-adfa Date: Tue, 1 Sep 2026 23:03:10 +0530 Subject: [PATCH 5/9] Fix review comments and simplify workflow --- .github/workflows/jira-advance-to-qa.yml | 361 +++++++++++------------ 1 file changed, 170 insertions(+), 191 deletions(-) diff --git a/.github/workflows/jira-advance-to-qa.yml b/.github/workflows/jira-advance-to-qa.yml index 4978ed7db8..f6fb4c7dad 100644 --- a/.github/workflows/jira-advance-to-qa.yml +++ b/.github/workflows/jira-advance-to-qa.yml @@ -1,62 +1,73 @@ name: Advance Jira Ticket to QA -# Runs on pull_request_review so it can reach the Jira secrets, including for -# pull requests from forks. It must never check out or execute code from the -# pull request. All pull request data is read through github-script's `context` +# The event is only a wake-up signal: every decision is taken from the review +# state this workflow reads back from GitHub, never from the payload that woke +# it. It must never check out or execute code from a pull request. All pull +# request data is read through github-script's `context` and the GraphQL API # rather than `${{ }}` interpolation, so a crafted branch name or title is never # parsed as source. on: pull_request_review: - types: [ submitted ] - + types: [ submitted, dismissed ] + # The events that can *unblock* a ticket without anyone submitting a review: a + # sibling closing as abandoned, and a sibling leaving draft. Without these the + # gate below can hold a ticket for good, since nothing else re-runs this job. + # `converted_to_draft` and `reopened` are absent on purpose - both can only add + # a blocker, so re-running for them could never advance anything. + pull_request: + types: [ closed, ready_for_review ] + +# No `contents` grant: this workflow never reads the repository, only its pull +# requests. It runs in the base-repo context with access to the Jira secrets even +# for pull requests from forks, so it holds the narrowest set that works. permissions: - contents: read pull-requests: read jobs: resolve: - name: Resolve the ticket and check the rest of its pull requests + name: Decide whether the ticket may advance runs-on: ubuntu-latest - # Anyone with read access to a public repo can submit an approving review. - # Such a review does not satisfy branch protection, but it does fire this - # event. The association test is only a cheap pre-filter to avoid starting a - # runner for a drive-by approval; the authorization decision is the - # effective-permission check in the script. A review can also be submitted - # against a closed, merged or draft pull request, none of which mean the - # work is ready for QA. - if: >- - github.event.review.state == 'approved' && - github.event.pull_request.state == 'open' && - !github.event.pull_request.draft && - contains(fromJSON('["OWNER", "MEMBER", "COLLABORATOR"]'), github.event.review.author_association) + # A comment-only review is the one event that cannot change any pull + # request's review decision, so it is worth dropping before a runner starts. + # Nothing else is pre-filtered: a drive-by approval from a stranger can start + # a runner, but it cannot move a ticket, because the gate below asks GitHub + # for its own review decision rather than counting reviews itself. + if: github.event_name == 'pull_request' || github.event.review.state != 'commented' outputs: key: ${{ steps.resolve.outputs.key }} + pulls: ${{ steps.resolve.outputs.pulls }} steps: - name: Resolve the ticket key id: resolve uses: actions/github-script@f28e40c7f34bde8b3046d885e986cb6290c5673b # v7.1.0 with: + # The catch below is for a broken automation, and it fails the run to + # say so. A transient 5xx is not that, and without retries it would + # land in the same place and turn an unrelated pull request red. + retries: 3 script: | const KEY_PATTERN = /ADFA-\d+/i; - const pr = context.payload.pull_request; - const branch = pr.head.ref; - const BASE_REPO_ID = context.payload.repository.id; + /* + * A `community/` branch owns no ticket of its own, so it neither + * names the ticket that moves nor counts towards one that does. + */ + const keyOf = ref => { + if (ref.startsWith('community/')) return null; + const match = ref.match(KEY_PATTERN); + return match ? match[0].toUpperCase() : null; + }; - if (branch.startsWith('community/')) { - core.info(`Branch "${branch}" is a community contribution; no Jira ticket to advance.`); - return; - } + const pr = context.payload.pull_request; /* * A fork's head branch is named inside the author's own repository, * so its ADFA key is evidence of nothing: a fork branch called * "ADFA-1234-crash" would bind an unrelated ticket that a - * maintainer's approval then walks to QA. External contributions are - * expected to carry the `community/` prefix and have no ticket of - * their own in any case. + * maintainer's approval then walks to QA. lint-branch-name.yml + * already refuses the ADFA- prefix on an external branch. */ - if (pr.head.repo?.id !== BASE_REPO_ID) { + if (pr.head.repo?.id !== context.payload.repository.id) { core.info(`Pull request #${pr.number} is from a fork; no Jira ticket to advance.`); return; } @@ -65,153 +76,100 @@ jobs: // reading "fixes the ADFA-1234 crash" would otherwise pick the ticket // that moves. debug.yml extracts from the branch alone for the same // reason. - const match = branch.match(KEY_PATTERN); - if (!match) { - core.info(`No ADFA ticket referenced by branch "${branch}"; nothing to do.`); + const key = keyOf(pr.head.ref); + if (!key) { + core.info(`No ADFA ticket referenced by branch "${pr.head.ref}"; nothing to do.`); return; } - const key = match[0].toUpperCase(); /* - * author_association does not prove write access: an org member may - * have no access to this repo, and a collaborator may be read-only. - * Ask for the effective permission instead, once per login, since the - * same reviewer usually appears on every branch of a stack. + * One ticket can own a stack of pull requests. Moving it on the first + * approval hands QA a ticket whose code is several branches from + * landing, so it advances only once every open pull request carrying + * the key is ready. + * + * `reviewDecision` is GitHub's own verdict on a pull request. It + * counts only opinionated reviews from people who can write to the + * repository and it honours dismissals, which is what this job would + * otherwise have to reconstruct by reading every review and looking + * up every reviewer's permission. */ - const permissions = new Map(); - const canMerge = async login => { - if (!permissions.has(login)) { - let permission; - try { - const { data } = await github.rest.repos.getCollaboratorPermissionLevel({ - owner: context.repo.owner, - repo: context.repo.repo, - username: login, - }); - permission = data.permission; - } catch (error) { - /* - * A 404 answers the question with "no access" -- App bots review - * too, and a "name[bot]" login is not a username. Anything else - * is a broken automation rather than a verdict: the token cannot - * read collaborators, and every later approval would end as a - * warning under a green check, which is the silent drift this - * job exists to remove. Let it out to the catch below and turn - * the run red. - */ - if (error.status !== 404) { - throw new Error(`could not read ${login}'s permission on this repository (${error.message})`); + const query = ` + query($owner: String!, $repo: String!, $cursor: String) { + repository(owner: $owner, name: $repo) { + pullRequests(states: OPEN, first: 100, after: $cursor) { + pageInfo { hasNextPage endCursor } + nodes { number isDraft isCrossRepository headRefName reviewDecision } } - permission = 'none'; } - permissions.set(login, permission); - } - /* - * The legacy `permission` field reports "maintain" as "write", so - * those two values cover admin, maintain and write. - */ - return ['admin', 'write'].includes(permissions.get(login)); - }; - - const reviewer = context.payload.review.user.login; + }`; + const stack = []; try { - if (!await canMerge(reviewer)) { - core.info(`${reviewer} has "${permissions.get(reviewer)}" permission and cannot merge; leaving ${key} untouched.`); - return; - } - - /* - * One ticket can own a stack of pull requests. Moving it on the - * first approval hands QA a ticket whose code is four branches from - * landing, and the later approvals then find it already at QA and - * say nothing. Advance only once every open pull request carrying - * this key -- the one that fired this event included -- is approved - * with no change request outstanding. - */ - const open = await github.paginate(github.rest.pulls.list, { - owner: context.repo.owner, - repo: context.repo.repo, - state: 'open', - per_page: 100, - }); - /* - * Fork heads are excluded for the reason given above, and because a - * stranger could otherwise hold the board still by opening a fork - * branch named after someone else's ticket. - */ - const stack = open.filter(candidate => { - if (candidate.head.repo?.id !== BASE_REPO_ID) return false; - const candidateKey = candidate.head.ref.match(KEY_PATTERN); - return candidateKey && candidateKey[0].toUpperCase() === key; - }); - if (!stack.some(member => member.number === pr.number)) { - stack.push(pr); - } - - for (const member of stack) { - const label = member.number === pr.number - ? `this pull request (#${member.number})` - : `pull request #${member.number}`; - - if (member.draft) { - core.info(`${key}: ${label} is a draft; leaving the ticket where it is.`); - return; - } - - const reviews = await github.paginate(github.rest.pulls.listReviews, { + let cursor = null; + for (;;) { + const { repository } = await github.graphql(query, { owner: context.repo.owner, repo: context.repo.repo, - pull_number: member.number, - per_page: 100, + cursor, }); - - /* - * A reviewer's verdict is their latest non-comment review, and - * only the verdicts of people who could merge count: anyone can - * review a public repo, so counting everyone would let a drive-by - * approval satisfy this gate and a drive-by change request hold - * the board still. - */ - const verdicts = new Map(); - for (const submitted of reviews) { - if (submitted.state === 'COMMENTED' || submitted.state === 'PENDING') continue; - if (!submitted.user || !await canMerge(submitted.user.login)) continue; - verdicts.set(submitted.user.login, submitted.state); - } + const { nodes, pageInfo } = repository.pullRequests; /* - * The review that fired this event is the newest verdict of the - * reviewer who submitted it; listReviews need not show it yet. + * Fork heads are excluded for the reason given above, and because + * a stranger could otherwise hold the board still by opening a + * fork branch named after someone else's ticket. */ - if (member.number === pr.number) { - verdicts.set(reviewer, 'APPROVED'); - } - - const states = [...verdicts.values()]; - if (states.includes('CHANGES_REQUESTED')) { - core.info(`${key}: ${label} has an outstanding change request; leaving the ticket where it is.`); - return; - } - if (!states.includes('APPROVED')) { - core.info(`${key}: ${label} is not approved; leaving the ticket where it is.`); - return; - } + stack.push(...nodes.filter(node => + !node.isCrossRepository && keyOf(node.headRefName) === key)); + if (!pageInfo.hasNextPage) break; + cursor = pageInfo.endCursor; } - - core.setOutput('key', key); } catch (error) { - core.setFailed(`Cannot decide whether ${key} may advance: ${error.message}. Leaving it untouched.`); + core.setFailed(`Cannot tell whether ${key} may advance: ${error.message}. Leaving it untouched.`); + return; + } + + /* + * Nothing open means nothing to hand QA: every pull request for the + * ticket has already been merged or closed. A late approval on one of + * them must not walk a ticket that QA deliberately bounced back. + */ + if (!stack.length) { + core.info(`${key} has no open pull request; leaving the ticket where it is.`); + return; } + const blocker = member => member.isDraft ? 'a draft' + : member.reviewDecision === 'CHANGES_REQUESTED' ? 'awaiting changes' + : 'not approved'; + const blocked = stack.filter(member => + member.isDraft || member.reviewDecision !== 'APPROVED'); + + if (blocked.length) { + /* + * A warning, not info: a ticket held back by a sibling is the one + * outcome nobody is watching for, and a hold that says nothing is + * the same silent drift this job exists to remove. + */ + core.warning( + `${key} stays where it is: ` + + blocked.map(member => `#${member.number} is ${blocker(member)}`).join(', ') + '.' + ); + return; + } + + core.setOutput('key', key); + core.setOutput('pulls', stack.map(member => member.number).join(',')); + advance: name: Move linked Jira ticket to QA runs-on: ubuntu-latest needs: resolve if: needs.resolve.outputs.key != '' - # Two approvals landing seconds apart otherwise run two walks over one - # ticket. Backward transitions are global in this Jira workflow, so a stale - # walk can pull a ticket back out of QA. The group has to key on the ticket, - # not the pull request: a stack shares one ticket across several of them. + # Two events landing seconds apart otherwise run two walks over one ticket. + # Backward transitions are global in this Jira workflow, so a stale walk can + # pull a ticket back out of QA. The group has to key on the ticket, not the + # pull request: a stack shares one ticket across several of them. concurrency: group: jira-advance-${{ needs.resolve.outputs.key }} cancel-in-progress: false @@ -221,7 +179,10 @@ jobs: env: JIRA_EMAIL: ${{ secrets.JIRA_EMAIL }} JIRA_API_TOKEN: ${{ secrets.JIRA_API_TOKEN }} + # Both are derived values, not payload: the key matched /ADFA-\d+/ and + # the list is pull request numbers. TICKET_KEY: ${{ needs.resolve.outputs.key }} + TICKET_PULLS: ${{ needs.resolve.outputs.pulls }} with: script: | const JIRA_BASE = 'https://appdevforall.atlassian.net'; @@ -237,9 +198,9 @@ jobs: const TARGET = 'QA'; const TARGET_INDEX = STATUSES.indexOf(TARGET); - const pr = context.payload.pull_request; - const reviewer = context.payload.review.user.login; const key = process.env.TICKET_KEY; + const pulls = process.env.TICKET_PULLS.split(','); + const repoUrl = context.payload.repository.html_url; const email = process.env.JIRA_EMAIL; const token = process.env.JIRA_API_TOKEN; @@ -277,9 +238,10 @@ jobs: let current = null; const walked = []; + const trail = () => walked.join(' -> '); const trailSuffix = () => { if (walked.length <= 1) return ' The ticket was not moved.'; - return ` It was moved ${walked.join(' -> ')} and is now at "${current}"; finish it by hand.`; + return ` It was moved ${trail()} and is now at "${current}"; finish it by hand.`; }; try { @@ -298,23 +260,13 @@ jobs: /* * There is no direct edge from "To Do" or "In Progress" to QA, so a - * ticket left behind has to be walked forward one hop at a time -- - * the ticket that is behind belongs to the person who forgot. - * Backward transitions, by contrast, are global from every status, - * which is why each hop re-reads the ticket first: a walk that - * blindly asks for the next status can drag a ticket that someone - * else already advanced back down the board. + * ticket left behind has to be walked forward one hop at a time. + * Each hop resolves its transition by target status name from the + * live endpoint, which doubles as the freshness check: if someone + * else moved the ticket, the transition this run wants is no longer + * on offer and the walk stops instead of guessing. */ while (index < TARGET_INDEX) { - const observed = await statusOf(); - if (observed !== current) { - core.warning( - `${key} moved to "${observed}" while this run was walking it from "${current}"; ` + - `something else is moving the same ticket. Stopping.${trailSuffix()}` - ); - return; - } - const next = STATUSES[index + 1]; const { transitions } = await jira(`/issue/${key}/transitions`); const hop = transitions.find(transition => transition.to && transition.to.name === next); @@ -339,9 +291,25 @@ jobs: current = next; index += 1; walked.push(current); - } - const trail = walked.join(' -> '); + /* + * Backward transitions are global from every status, so another + * run or a person can pull the ticket back down the board mid + * walk. Re-read after each hop, and report where the ticket + * actually is rather than where this run left it. + */ + if (index < TARGET_INDEX) { + const observed = await statusOf(); + if (observed !== current) { + current = observed; + core.warning( + `${key} is at "${observed}", not where this run left it; something else is ` + + `moving the same ticket. Stopping.${trailSuffix()}` + ); + return; + } + } + } /* * The walk is already committed by this point, so a comment that @@ -349,6 +317,22 @@ jobs: * sends whoever reads the log to a ticket already sitting where it * belongs. */ + const link = number => ({ + type: 'text', + text: `#${number}`, + marks: [{ type: 'link', attrs: { href: `${repoUrl}/pull/${number}` } }], + }); + const sentence = [{ type: 'text', text: `Automatically moved to ${TARGET} (${trail()}): ` }]; + pulls.forEach((number, position) => { + sentence.push({ type: 'text', text: position === 0 ? '' : ', ' }, link(number)); + }); + sentence.push({ + type: 'text', + text: pulls.length > 1 + ? ' are all approved, with nothing else open for this ticket.' + : ' is approved, and is the only pull request open for this ticket.', + }); + try { await jira(`/issue/${key}/comment`, { method: 'POST', @@ -356,29 +340,24 @@ jobs: body: { type: 'doc', version: 1, - content: [{ - type: 'paragraph', - content: [ - { type: 'text', text: `Automatically moved to ${TARGET} (${trail}): ` }, - { - type: 'text', - text: `pull request #${pr.number}`, - marks: [{ type: 'link', attrs: { href: pr.html_url } }], - }, - { type: 'text', text: ` was approved by ${reviewer}.` }, - ], - }], + content: [{ type: 'paragraph', content: sentence }], }, }), }); } catch (error) { - core.warning(`${key} reached ${TARGET} (${trail}), but the Jira comment recording why could not be posted: ${error.message}.`); + core.warning(`${key} reached ${TARGET} (${trail()}), but the Jira comment recording why could not be posted: ${error.message}.`); } - core.info(`${key}: ${trail}`); - core.summary.addRaw(`Moved [${key}](${JIRA_BASE}/browse/${key}) to ${TARGET}: ${trail}`); + core.info(`${key}: ${trail()}`); + core.summary.addRaw(`Moved [${key}](${JIRA_BASE}/browse/${key}) to ${TARGET}: ${trail()}`); await core.summary.write(); } catch (error) { - // A Jira outage or auth problem must never turn a pull request red. - core.warning(`Could not advance ${key} to ${TARGET}: ${error.message}.${trailSuffix()}`); + /* + * A Jira outage or auth problem must never turn a pull request red. + * Anything thrown once the ticket has reached QA is a reporting + * failure, not a failure to advance, and must not say otherwise. + */ + core.warning(current === TARGET + ? `${key} reached ${TARGET} (${trail()}), but recording it failed: ${error.message}.` + : `Could not advance ${key} to ${TARGET}: ${error.message}.${trailSuffix()}`); } From ccf5636a2ba220d63c5146beaf27faa18a4753f4 Mon Sep 17 00:00:00 2001 From: Hal Eisen Date: Wed, 2 Sep 2026 19:40:01 -0700 Subject: [PATCH 6/9] Cut the workflow down to a single curl script The github-script version reconstructed a stack-wide approval gate, a multi-hop status walk, and an ADF comment. Replaced with the trick it was meant to be: approved PR, ADFA key from the branch, one transition to QA. Claude-Session: https://claude.ai/code/session_01FbfecgcGA8RPDJY3Bu21FY --- .github/workflows/jira-advance-to-qa.yml | 368 ++--------------------- 1 file changed, 17 insertions(+), 351 deletions(-) diff --git a/.github/workflows/jira-advance-to-qa.yml b/.github/workflows/jira-advance-to-qa.yml index f6fb4c7dad..22f8aef134 100644 --- a/.github/workflows/jira-advance-to-qa.yml +++ b/.github/workflows/jira-advance-to-qa.yml @@ -1,363 +1,29 @@ name: Advance Jira Ticket to QA -# The event is only a wake-up signal: every decision is taken from the review -# state this workflow reads back from GitHub, never from the payload that woke -# it. It must never check out or execute code from a pull request. All pull -# request data is read through github-script's `context` and the GraphQL API -# rather than `${{ }}` interpolation, so a crafted branch name or title is never -# parsed as source. on: pull_request_review: - types: [ submitted, dismissed ] - # The events that can *unblock* a ticket without anyone submitting a review: a - # sibling closing as abandoned, and a sibling leaving draft. Without these the - # gate below can hold a ticket for good, since nothing else re-runs this job. - # `converted_to_draft` and `reopened` are absent on purpose - both can only add - # a blocker, so re-running for them could never advance anything. - pull_request: - types: [ closed, ready_for_review ] + types: [ submitted ] -# No `contents` grant: this workflow never reads the repository, only its pull -# requests. It runs in the base-repo context with access to the Jira secrets even -# for pull requests from forks, so it holds the narrowest set that works. -permissions: - pull-requests: read +permissions: {} jobs: - resolve: - name: Decide whether the ticket may advance - runs-on: ubuntu-latest - # A comment-only review is the one event that cannot change any pull - # request's review decision, so it is worth dropping before a runner starts. - # Nothing else is pre-filtered: a drive-by approval from a stranger can start - # a runner, but it cannot move a ticket, because the gate below asks GitHub - # for its own review decision rather than counting reviews itself. - if: github.event_name == 'pull_request' || github.event.review.state != 'commented' - outputs: - key: ${{ steps.resolve.outputs.key }} - pulls: ${{ steps.resolve.outputs.pulls }} - steps: - - name: Resolve the ticket key - id: resolve - uses: actions/github-script@f28e40c7f34bde8b3046d885e986cb6290c5673b # v7.1.0 - with: - # The catch below is for a broken automation, and it fails the run to - # say so. A transient 5xx is not that, and without retries it would - # land in the same place and turn an unrelated pull request red. - retries: 3 - script: | - const KEY_PATTERN = /ADFA-\d+/i; - - /* - * A `community/` branch owns no ticket of its own, so it neither - * names the ticket that moves nor counts towards one that does. - */ - const keyOf = ref => { - if (ref.startsWith('community/')) return null; - const match = ref.match(KEY_PATTERN); - return match ? match[0].toUpperCase() : null; - }; - - const pr = context.payload.pull_request; - - /* - * A fork's head branch is named inside the author's own repository, - * so its ADFA key is evidence of nothing: a fork branch called - * "ADFA-1234-crash" would bind an unrelated ticket that a - * maintainer's approval then walks to QA. lint-branch-name.yml - * already refuses the ADFA- prefix on an external branch. - */ - if (pr.head.repo?.id !== context.payload.repository.id) { - core.info(`Pull request #${pr.number} is from a fork; no Jira ticket to advance.`); - return; - } - - // The key comes from the branch rather than the title: a title - // reading "fixes the ADFA-1234 crash" would otherwise pick the ticket - // that moves. debug.yml extracts from the branch alone for the same - // reason. - const key = keyOf(pr.head.ref); - if (!key) { - core.info(`No ADFA ticket referenced by branch "${pr.head.ref}"; nothing to do.`); - return; - } - - /* - * One ticket can own a stack of pull requests. Moving it on the first - * approval hands QA a ticket whose code is several branches from - * landing, so it advances only once every open pull request carrying - * the key is ready. - * - * `reviewDecision` is GitHub's own verdict on a pull request. It - * counts only opinionated reviews from people who can write to the - * repository and it honours dismissals, which is what this job would - * otherwise have to reconstruct by reading every review and looking - * up every reviewer's permission. - */ - const query = ` - query($owner: String!, $repo: String!, $cursor: String) { - repository(owner: $owner, name: $repo) { - pullRequests(states: OPEN, first: 100, after: $cursor) { - pageInfo { hasNextPage endCursor } - nodes { number isDraft isCrossRepository headRefName reviewDecision } - } - } - }`; - - const stack = []; - try { - let cursor = null; - for (;;) { - const { repository } = await github.graphql(query, { - owner: context.repo.owner, - repo: context.repo.repo, - cursor, - }); - const { nodes, pageInfo } = repository.pullRequests; - /* - * Fork heads are excluded for the reason given above, and because - * a stranger could otherwise hold the board still by opening a - * fork branch named after someone else's ticket. - */ - stack.push(...nodes.filter(node => - !node.isCrossRepository && keyOf(node.headRefName) === key)); - if (!pageInfo.hasNextPage) break; - cursor = pageInfo.endCursor; - } - } catch (error) { - core.setFailed(`Cannot tell whether ${key} may advance: ${error.message}. Leaving it untouched.`); - return; - } - - /* - * Nothing open means nothing to hand QA: every pull request for the - * ticket has already been merged or closed. A late approval on one of - * them must not walk a ticket that QA deliberately bounced back. - */ - if (!stack.length) { - core.info(`${key} has no open pull request; leaving the ticket where it is.`); - return; - } - - const blocker = member => member.isDraft ? 'a draft' - : member.reviewDecision === 'CHANGES_REQUESTED' ? 'awaiting changes' - : 'not approved'; - const blocked = stack.filter(member => - member.isDraft || member.reviewDecision !== 'APPROVED'); - - if (blocked.length) { - /* - * A warning, not info: a ticket held back by a sibling is the one - * outcome nobody is watching for, and a hold that says nothing is - * the same silent drift this job exists to remove. - */ - core.warning( - `${key} stays where it is: ` + - blocked.map(member => `#${member.number} is ${blocker(member)}`).join(', ') + '.' - ); - return; - } - - core.setOutput('key', key); - core.setOutput('pulls', stack.map(member => member.number).join(',')); - advance: name: Move linked Jira ticket to QA runs-on: ubuntu-latest - needs: resolve - if: needs.resolve.outputs.key != '' - # Two events landing seconds apart otherwise run two walks over one ticket. - # Backward transitions are global in this Jira workflow, so a stale walk can - # pull a ticket back out of QA. The group has to key on the ticket, not the - # pull request: a stack shares one ticket across several of them. - concurrency: - group: jira-advance-${{ needs.resolve.outputs.key }} - cancel-in-progress: false + # A fork branch can be named after anyone's ticket, so only our own branches count. + if: >- + github.event.review.state == 'approved' && + github.event.pull_request.head.repo.full_name == github.repository steps: - - name: Walk the ticket forward to QA - uses: actions/github-script@f28e40c7f34bde8b3046d885e986cb6290c5673b # v7.1.0 + - name: Move the ticket to QA env: - JIRA_EMAIL: ${{ secrets.JIRA_EMAIL }} - JIRA_API_TOKEN: ${{ secrets.JIRA_API_TOKEN }} - # Both are derived values, not payload: the key matched /ADFA-\d+/ and - # the list is pull request numbers. - TICKET_KEY: ${{ needs.resolve.outputs.key }} - TICKET_PULLS: ${{ needs.resolve.outputs.pulls }} - with: - script: | - const JIRA_BASE = 'https://appdevforall.atlassian.net'; - - // Node's fetch imposes no deadline on a response, so a hung or - // half-delivered reply would stall the job rather than fail it. - // Aborting routes it into the catch below, where it stays a warning. - const JIRA_REQUEST_TIMEOUT_MS = 30000; - - // Case-sensitive, in board order. Note the lowercase "review" and - // "merge" -- Jira matches these exactly. - const STATUSES = ['To Do', 'In Progress', 'Code review', 'QA', 'Ready to merge', 'Done']; - const TARGET = 'QA'; - const TARGET_INDEX = STATUSES.indexOf(TARGET); - - const key = process.env.TICKET_KEY; - const pulls = process.env.TICKET_PULLS.split(','); - const repoUrl = context.payload.repository.html_url; - - const email = process.env.JIRA_EMAIL; - const token = process.env.JIRA_API_TOKEN; - if (!email || !token) { - core.warning(`Jira credentials are not configured; leaving ${key} untouched.`); - return; - } - const auth = 'Basic ' + Buffer.from(`${email}:${token}`).toString('base64'); - - const jira = async (path, init = {}) => { - const response = await fetch(`${JIRA_BASE}/rest/api/3${path}`, { - ...init, - headers: { - Authorization: auth, - Accept: 'application/json', - ...(init.body ? { 'Content-Type': 'application/json' } : {}), - }, - signal: AbortSignal.timeout(JIRA_REQUEST_TIMEOUT_MS), - }); - if (!response.ok) { - throw new Error(`${init.method || 'GET'} ${path} returned HTTP ${response.status}`); - } - return response.status === 204 ? null : response.json(); - }; - - const statusOf = async () => (await jira(`/issue/${key}?fields=status`)).fields.status.name; - - /* - * Declared outside the try so the catch can report how far the walk - * got. A hop is committed as it is made and there is no rollback, so - * a failure part-way leaves the ticket somewhere new, and a log line - * claiming it did not move sends whoever repairs the board to the - * wrong place. - */ - let current = null; - const walked = []; - - const trail = () => walked.join(' -> '); - const trailSuffix = () => { - if (walked.length <= 1) return ' The ticket was not moved.'; - return ` It was moved ${trail()} and is now at "${current}"; finish it by hand.`; - }; - - try { - current = await statusOf(); - walked.push(current); - let index = STATUSES.indexOf(current); - - if (index === -1) { - core.warning(`${key} is in unrecognized status "${current}"; leaving it untouched.`); - return; - } - if (index >= TARGET_INDEX) { - core.info(`${key} is already at "${current}", which is at or past ${TARGET}; nothing to do.`); - return; - } - - /* - * There is no direct edge from "To Do" or "In Progress" to QA, so a - * ticket left behind has to be walked forward one hop at a time. - * Each hop resolves its transition by target status name from the - * live endpoint, which doubles as the freshness check: if someone - * else moved the ticket, the transition this run wants is no longer - * on offer and the walk stops instead of guessing. - */ - while (index < TARGET_INDEX) { - const next = STATUSES[index + 1]; - const { transitions } = await jira(`/issue/${key}/transitions`); - const hop = transitions.find(transition => transition.to && transition.to.name === next); - - if (!hop) { - const offered = transitions - .map(transition => transition.to && transition.to.name) - .filter(Boolean) - .join(', '); - core.warning( - `${key} is in "${current}" and offers no transition to "${next}" (available: ${offered}). ` + - `Stopping here.${trailSuffix()}` - ); - return; - } - - await jira(`/issue/${key}/transitions`, { - method: 'POST', - body: JSON.stringify({ transition: { id: hop.id } }), - }); - - current = next; - index += 1; - walked.push(current); - - /* - * Backward transitions are global from every status, so another - * run or a person can pull the ticket back down the board mid - * walk. Re-read after each hop, and report where the ticket - * actually is rather than where this run left it. - */ - if (index < TARGET_INDEX) { - const observed = await statusOf(); - if (observed !== current) { - current = observed; - core.warning( - `${key} is at "${observed}", not where this run left it; something else is ` + - `moving the same ticket. Stopping.${trailSuffix()}` - ); - return; - } - } - } - - /* - * The walk is already committed by this point, so a comment that - * fails to post must not be reported as a failure to advance: that - * sends whoever reads the log to a ticket already sitting where it - * belongs. - */ - const link = number => ({ - type: 'text', - text: `#${number}`, - marks: [{ type: 'link', attrs: { href: `${repoUrl}/pull/${number}` } }], - }); - const sentence = [{ type: 'text', text: `Automatically moved to ${TARGET} (${trail()}): ` }]; - pulls.forEach((number, position) => { - sentence.push({ type: 'text', text: position === 0 ? '' : ', ' }, link(number)); - }); - sentence.push({ - type: 'text', - text: pulls.length > 1 - ? ' are all approved, with nothing else open for this ticket.' - : ' is approved, and is the only pull request open for this ticket.', - }); - - try { - await jira(`/issue/${key}/comment`, { - method: 'POST', - body: JSON.stringify({ - body: { - type: 'doc', - version: 1, - content: [{ type: 'paragraph', content: sentence }], - }, - }), - }); - } catch (error) { - core.warning(`${key} reached ${TARGET} (${trail()}), but the Jira comment recording why could not be posted: ${error.message}.`); - } - - core.info(`${key}: ${trail()}`); - core.summary.addRaw(`Moved [${key}](${JIRA_BASE}/browse/${key}) to ${TARGET}: ${trail()}`); - await core.summary.write(); - } catch (error) { - /* - * A Jira outage or auth problem must never turn a pull request red. - * Anything thrown once the ticket has reached QA is a reporting - * failure, not a failure to advance, and must not say otherwise. - */ - core.warning(current === TARGET - ? `${key} reached ${TARGET} (${trail()}), but recording it failed: ${error.message}.` - : `Could not advance ${key} to ${TARGET}: ${error.message}.${trailSuffix()}`); - } + BRANCH: ${{ github.event.pull_request.head.ref }} + AUTH: ${{ secrets.JIRA_EMAIL }}:${{ secrets.JIRA_API_TOKEN }} + run: | + KEY=$(echo "$BRANCH" | grep -oE 'ADFA-[0-9]+' | head -1) + [ -n "$KEY" ] || { echo "No ADFA key in $BRANCH"; exit 0; } + API="https://appdevforall.atlassian.net/rest/api/3/issue/$KEY/transitions" + ID=$(curl -sf -u "$AUTH" "$API" | jq -r '.transitions[] | select(.to.name == "QA") | .id') + [ -n "$ID" ] || { echo "$KEY offers no transition to QA"; exit 0; } + curl -sf -u "$AUTH" -X POST -H 'Content-Type: application/json' \ + -d "{\"transition\":{\"id\":\"$ID\"}}" "$API" && echo "$KEY -> QA" From 13b9045c9ae6e99f1728f8a0536829676aefc0c5 Mon Sep 17 00:00:00 2001 From: Hal Eisen Date: Wed, 2 Sep 2026 19:44:48 -0700 Subject: [PATCH 7/9] Only trust approvals from people with write access The repo is public, so any GitHub user can submit an approving review. The fork check guarded the branch side but not the reviewer side. Claude-Session: https://claude.ai/code/session_01FbfecgcGA8RPDJY3Bu21FY --- .github/workflows/jira-advance-to-qa.yml | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/.github/workflows/jira-advance-to-qa.yml b/.github/workflows/jira-advance-to-qa.yml index 22f8aef134..7a958de48a 100644 --- a/.github/workflows/jira-advance-to-qa.yml +++ b/.github/workflows/jira-advance-to-qa.yml @@ -10,10 +10,12 @@ jobs: advance: name: Move linked Jira ticket to QA runs-on: ubuntu-latest - # A fork branch can be named after anyone's ticket, so only our own branches count. + # Public repo: anyone can approve, and a fork branch can be named after + # anyone's ticket. Only our own branches, approved by someone with write access. if: >- github.event.review.state == 'approved' && - github.event.pull_request.head.repo.full_name == github.repository + github.event.pull_request.head.repo.full_name == github.repository && + contains(fromJSON('["OWNER", "MEMBER", "COLLABORATOR"]'), github.event.review.author_association) steps: - name: Move the ticket to QA env: From 9b0fe971f2feacf1f916f26286338022ace06d4a Mon Sep 17 00:00:00 2001 From: Hal Eisen Date: Thu, 3 Sep 2026 07:37:54 -0700 Subject: [PATCH 8/9] Walk In Progress -> Code review -> QA, and drop the pipefail trap Jira only offers the QA edge from Code review, so a ticket still sitting in In Progress when its PR was approved never moved. Walk the two named forward transitions instead. Both are non-global, so they are only ever offered from the one status that owns them -- which makes the walk self-guarding and idempotent without reading the ticket's status. Key extraction moved from a grep pipeline to a bash regex: under 'shell: bash' (-eo pipefail) a branch with no ADFA key failed the step and turned the PR red. Claude-Session: https://claude.ai/code/session_01FbfecgcGA8RPDJY3Bu21FY --- .github/workflows/jira-advance-to-qa.yml | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/.github/workflows/jira-advance-to-qa.yml b/.github/workflows/jira-advance-to-qa.yml index 7a958de48a..90e8f3409a 100644 --- a/.github/workflows/jira-advance-to-qa.yml +++ b/.github/workflows/jira-advance-to-qa.yml @@ -22,10 +22,19 @@ jobs: BRANCH: ${{ github.event.pull_request.head.ref }} AUTH: ${{ secrets.JIRA_EMAIL }}:${{ secrets.JIRA_API_TOKEN }} run: | - KEY=$(echo "$BRANCH" | grep -oE 'ADFA-[0-9]+' | head -1) - [ -n "$KEY" ] || { echo "No ADFA key in $BRANCH"; exit 0; } + [[ "$BRANCH" =~ ADFA-[0-9]+ ]] || { echo "No ADFA key in $BRANCH"; exit 0; } + KEY="${BASH_REMATCH[0]}" API="https://appdevforall.atlassian.net/rest/api/3/issue/$KEY/transitions" - ID=$(curl -sf -u "$AUTH" "$API" | jq -r '.transitions[] | select(.to.name == "QA") | .id') - [ -n "$ID" ] || { echo "$KEY offers no transition to QA"; exit 0; } - curl -sf -u "$AUTH" -X POST -H 'Content-Type: application/json' \ - -d "{\"transition\":{\"id\":\"$ID\"}}" "$API" && echo "$KEY -> QA" + + # QA is reachable only via these two named transitions, and Jira offers + # each from exactly one status (In Progress, then Code review). Asking + # for one from anywhere else simply finds nothing, so a ticket already + # in QA or beyond is left alone without having to read its status. + # Never name the backward transitions (Done/To Do/In Progress); those + # are global, so a blind name match could drag a finished ticket back. + for STEP in "To code review" "Passed code review"; do + ID=$(curl -sf -u "$AUTH" "$API" | jq -r --arg S "$STEP" '.transitions[] | select(.name == $S) | .id') + [ -n "$ID" ] || continue + curl -sf -u "$AUTH" -X POST -H 'Content-Type: application/json' \ + -d "{\"transition\":{\"id\":\"$ID\"}}" "$API" && echo "$KEY: $STEP" + done From beddb9a5316bc11a4d7c95ed687c43750ee7476f Mon Sep 17 00:00:00 2001 From: Hal Eisen Date: Thu, 3 Sep 2026 09:45:34 -0700 Subject: [PATCH 9/9] Stop the script from failing silently, and bound it Review found the failure paths were the weak part, not the walk: - A failed transitions GET yielded an empty ID, so both hops hit `continue` and the step passed green with no output. That is the silent no-op the automation exists to remove. Both requests now warn and stop. - `curl ... && echo` tied the step's exit code to whichever loop iteration ran last, so a POST failing on the second hop reddened the PR. The earlier claim that no path could turn a PR red was wrong. - `jq` output went into the JSON body unchecked. A duplicate transition name gave a multi-line ID and a transition with no `.id` gave the string "null". `first(...) // empty` handles both. - No request deadline. Added `--max-time 30` and `timeout-minutes: 5`. - An approval on a draft PR promoted the ticket. Now gated. - The `author_association` comment claimed the check proves write access. It does not; it is a proxy. Comment corrected rather than the mechanism, which would need a token this workflow deliberately does not hold. Simulated happy path, GET failure, POST failure, already-in-QA, duplicate transition names and a missing id under `bash -eo pipefail`. All exit 0. Claude-Session: https://claude.ai/code/session_01FbfecgcGA8RPDJY3Bu21FY --- .github/workflows/jira-advance-to-qa.yml | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/.github/workflows/jira-advance-to-qa.yml b/.github/workflows/jira-advance-to-qa.yml index 90e8f3409a..1ec63ca3cc 100644 --- a/.github/workflows/jira-advance-to-qa.yml +++ b/.github/workflows/jira-advance-to-qa.yml @@ -11,13 +11,17 @@ jobs: name: Move linked Jira ticket to QA runs-on: ubuntu-latest # Public repo: anyone can approve, and a fork branch can be named after - # anyone's ticket. Only our own branches, approved by someone with write access. + # anyone's ticket. Only our own branches, and only an approval from an org + # member or a repo collaborator. That is a proxy for write access, not proof + # of it -- proving it needs a token this workflow deliberately does not hold. if: >- github.event.review.state == 'approved' && + github.event.pull_request.draft == false && github.event.pull_request.head.repo.full_name == github.repository && contains(fromJSON('["OWNER", "MEMBER", "COLLABORATOR"]'), github.event.review.author_association) steps: - name: Move the ticket to QA + timeout-minutes: 5 env: BRANCH: ${{ github.event.pull_request.head.ref }} AUTH: ${{ secrets.JIRA_EMAIL }}:${{ secrets.JIRA_API_TOKEN }} @@ -25,6 +29,7 @@ jobs: [[ "$BRANCH" =~ ADFA-[0-9]+ ]] || { echo "No ADFA key in $BRANCH"; exit 0; } KEY="${BASH_REMATCH[0]}" API="https://appdevforall.atlassian.net/rest/api/3/issue/$KEY/transitions" + jira() { curl -sS -f --max-time 30 -u "$AUTH" "$@"; } # QA is reachable only via these two named transitions, and Jira offers # each from exactly one status (In Progress, then Code review). Asking @@ -33,8 +38,13 @@ jobs: # Never name the backward transitions (Done/To Do/In Progress); those # are global, so a blind name match could drag a finished ticket back. for STEP in "To code review" "Passed code review"; do - ID=$(curl -sf -u "$AUTH" "$API" | jq -r --arg S "$STEP" '.transitions[] | select(.name == $S) | .id') + # A Jira outage must not redden an unrelated PR, but it must not pass + # for "no transition on offer" either -- that is the silent no-op. + LIST=$(jira "$API") || { echo "::warning::$KEY: cannot read transitions, ticket untouched"; exit 0; } + ID=$(jq -r --arg S "$STEP" 'first(.transitions[] | select(.name == $S) | .id) // empty' <<<"$LIST") [ -n "$ID" ] || continue - curl -sf -u "$AUTH" -X POST -H 'Content-Type: application/json' \ - -d "{\"transition\":{\"id\":\"$ID\"}}" "$API" && echo "$KEY: $STEP" + jira -X POST -H 'Content-Type: application/json' \ + -d "{\"transition\":{\"id\":\"$ID\"}}" "$API" \ + || { echo "::warning::$KEY: transition \"$STEP\" failed"; exit 0; } + echo "$KEY: $STEP" done