diff --git a/.github/scripts/standards-sync-stuck-automerge-alert.test.cjs b/.github/scripts/standards-sync-stuck-automerge-alert.test.cjs index a4bf76f2..2cd9ec07 100644 --- a/.github/scripts/standards-sync-stuck-automerge-alert.test.cjs +++ b/.github/scripts/standards-sync-stuck-automerge-alert.test.cjs @@ -799,24 +799,19 @@ async function runLookup({ openIssues = [], envOverrides = {} } = {}) { } } -async function runClose({ openIssues = [] } = {}) { +async function runClose({ issueNumber } = {}) { const script = extractStepScript("Close recovered tracking issue"); - const keys = ["MARKER", "ISSUE_AUTHOR_LOGIN"]; + const keys = ["ISSUE_NUMBER"]; const originalValues = Object.fromEntries( keys.map((key) => [key, process.env[key]]), ); - Object.assign(process.env, { - MARKER, - ISSUE_AUTHOR_LOGIN: "github-actions[bot]", - }); + Object.assign(process.env, { ISSUE_NUMBER: String(issueNumber) }); const comments = []; const updates = []; try { const github = { - paginate: async (_fn, _params) => openIssues, rest: { issues: { - listForRepo: () => {}, createComment: async (params) => comments.push(params), update: async (params) => updates.push(params), }, @@ -913,25 +908,39 @@ test("the lookup still fails closed on a genuine ambiguity between two bot-autho assert.match(failedWith, /found 2 issues carrying marker/u); }); -test("close ignores a decoy issue and takes no action when none of the candidates are bot-authored", async () => { - const { comments, updates } = await runClose({ - openIssues: [ - issue({ - number: 5, - login: "some-random-user", - type: "User", - body: `${MARKER}\ndecoy`, - }), - ], - }); - assert.equal(comments.length, 0); - assert.equal(updates.length, 0); +// The close step resolves no issue of its own — it acts on the number the +// lookup above already author-filtered. That makes its `if:` the whole of its +// decoy protection: without the `issue-number != ''` conjunct it would run +// with an empty number on every healthy run, and without the lookup running +// unconditionally the number would always be empty on exactly those runs. The +// two halves are only correct together, so both are asserted structurally — +// no mock can reach an `if:` expression. +test("the close step acts only on the author-filtered lookup result, and only when there is one", () => { + const closeStep = workflow.slice( + workflow.indexOf("- name: Close recovered tracking issue"), + ); + const closeCondition = /\n\s+if: ([^\n]*)/u.exec(closeStep)[1]; + assert.match(closeCondition, /steps\.scan\.outputs\.stuck-count == '0'/u); + assert.match(closeCondition, /steps\.scan\.outputs\.unarmed-count == '0'/u); + assert.match( + closeCondition, + /steps\.tracking\.outputs\.issue-number != ''/u, + "the close step must be guarded on the lookup having resolved an issue", + ); + + const lookupStep = workflow.slice( + workflow.indexOf("- name: Find existing tracking issue"), + ); + const lookupHeader = lookupStep.slice(0, lookupStep.indexOf("script: |")); + assert.doesNotMatch( + lookupHeader, + /\n\s+if:/u, + "the lookup must run unconditionally, or the close step is starved on every healthy run", + ); }); -test("close comments on and closes the genuine bot-authored tracking issue", async () => { - const { comments, updates } = await runClose({ - openIssues: [issue({ number: 5, body: `${MARKER}\nreport` })], - }); +test("close comments on and closes the tracking issue the lookup resolved", async () => { + const { comments, updates } = await runClose({ issueNumber: 5 }); assert.equal(comments.length, 1); assert.equal(comments[0].issue_number, 5); assert.equal(updates.length, 1); diff --git a/.github/workflows/standards-sync-stuck-automerge-alert.yml b/.github/workflows/standards-sync-stuck-automerge-alert.yml index 1c98cbf6..aa92e88f 100644 --- a/.github/workflows/standards-sync-stuck-automerge-alert.yml +++ b/.github/workflows/standards-sync-stuck-automerge-alert.yml @@ -28,11 +28,24 @@ name: standards-sync-stuck-automerge-alert # The target repo list is derived by checking out standards and running its # own sync-manifest.sh interpreter, the same way standards-sync.yml's plan job # does — never a second, hand-maintained inventory. The tracking issue is -# opened in the CALLING repository, authored by a second App token this -# workflow mints for itself (scoped to issues:write on that repo only — see -# the "Mint App token for this repository's tracking issue" step), not in any -# scanned target: a scheduled caller in melodic-software/medley gets the -# alert filed in medley, not scattered across the 8 targets. +# opened in the CALLING repository, not in any scanned target: a scheduled +# caller in melodic-software/medley gets the alert filed in medley, not +# scattered across the 8 targets. +# +# CONSUMER CONTRACT: the caller's job must grant `issues: write` (a called +# workflow can only downgrade the caller's grant, never elevate it), matching +# link-check.yml's and pulumi-version-drift-check.yml's callers. The App +# secrets below remain required — they mint the cross-repository read token +# that scans the targets — but they no longer author the tracking issue. +# +# One asymmetry with those two, for a consumer governed by +# melodic-software/standards' runner-policy: a reviewed contract may pin the +# caller's grant through `allowedCallerPermissions`, but only when every +# mapped secret's input name equals the repository secret it reads +# (`NAME: ${{ secrets.NAME }}`) — which the kebab-case inputs below cannot +# satisfy. Those two workflows take no secrets, so they pin the grant freely. +# A contract for this workflow either omits that pin or renames these inputs; +# nothing forces the pin, and granting `issues: write` does not. on: workflow_call: inputs: @@ -70,23 +83,23 @@ env: # `author.login` omits the REST-only `[bot]` suffix; `__typename` is the # authoritative bot-vs-human discriminator, so both are checked together. BOT_LOGIN: melodic-standards-sync - # The identity that opens/updates/closes this workflow's own tracking - # issue: a second App token (minted below, scoped to issues:write on only - # the calling repository) authors every REST write this workflow makes, so - # its bot login is the adoption filter — not the ambient GITHUB_TOKEN's - # shared github-actions[bot] identity every workflow in a repo can author - # as. App authorship is strictly tighter decoy resistance: a decoy from any - # other workflow in the caller repo no longer qualifies. It also drops the - # caller's need to grant issues:write to the ambient token at all, which - # resolves a runner-policy contract conflict in melodic-software/standards - # (components/runner-policy): a contract with a write allowedCallerPermissions - # entry must map its secrets as an exact identity passthrough - # `${{ secrets. }}`, which this reusable's kebab-case App-secret - # inputs (app-client-id/app-private-key) can never satisfy. Login format - # (`[bot]`, type Bot) is empirically verified: `gh api - # users/melodic-standards-sync%5Bbot%5D` returns login - # "melodic-standards-sync[bot]", type "Bot". - ISSUE_AUTHOR_LOGIN: melodic-standards-sync[bot] + # The identity that opens/updates/closes this workflow's own tracking issue. + # Every REST write below runs on the job's ambient GITHUB_TOKEN, so the + # issue's author.login is always github-actions[bot] (type Bot) — the same + # authorship link-check.yml, queue-monitor-liveness.yml, and + # tool-version-drift-check.yml already use, and the adoption filter the + # lookup below applies before any marker or title matching. + # + # An App identity is narrower — github-actions[bot] is shared by every + # workflow in the caller repo, so a decoy another workflow there authored + # qualifies where an App-only filter would reject it; both still require + # write access to that repo, and this is the posture + # queue-monitor-liveness.yml already ships. It is also not available here: + # a caller-scoped App token can only be minted where the App is installed, + # and the caller need not be in that installation at all — the App this + # workflow's secrets name is scoped to the sync TARGETS, and the sync + # source is deliberately not one of them. + ISSUE_AUTHOR_LOGIN: 'github-actions[bot]' jobs: detect-stuck-armed-prs: @@ -94,7 +107,8 @@ jobs: runs-on: ${{ inputs.runner }} timeout-minutes: 15 permissions: - contents: read # The ambient token no longer authors any issue write; see ISSUE_AUTHOR_LOGIN above. + contents: read + issues: write # The ambient token authors the rolling tracking issue; see ISSUE_AUTHOR_LOGIN above. steps: - name: Check out standards uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 @@ -438,19 +452,6 @@ jobs: require('node:fs').writeFileSync(process.env.GITHUB_WORKSPACE + '/.stuck-automerge-report.md', reportBody); - # owner/repositories intentionally omitted: create-github-app-token - # defaults both to the current (calling) repository when unset — the - # exact narrow scope this token needs, distinct from the read-only - # every-target token minted above. Runs unconditionally: both the - # open/update and close branches below need it. - - name: Mint App token for this repository's tracking issue - id: issue-token - uses: actions/create-github-app-token@bcd2ba49218906704ab6c1aa796996da409d3eb1 # v3.2.0 - with: - client-id: ${{ secrets.app-client-id }} - private-key: ${{ secrets.app-private-key }} - permission-issues: write - # A marker embedded in the body (not a title search) is exact and # survives a retitle, and matches the mechanism pulumi-version-drift- # check.yml, link-check.yml, and queue-monitor-liveness.yml already use. @@ -461,15 +462,19 @@ jobs: # was ported off the shared find-tracking-issue.sh bash block — that # block stays a generated consumer only for the two workflows that run # on a fixed hosted runner). + # + # Unconditional, matching link-check.yml: the open/update branch and the + # recovery-close branch below partition the outcome space, and both need + # the resolved issue number, so resolving it once here is what lets the + # close branch carry the same `issue-number != ''` guard link-check.yml + # does instead of repeating the lookup with its own author filter. - name: Find existing tracking issue id: tracking - if: steps.scan.outputs.stuck-count != '0' || steps.scan.outputs.unarmed-count != '0' uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 env: MARKER: '' ISSUE_TITLE: '[Alert] standards-sync auto-merge PR(s) needing attention' with: - github-token: ${{ steps.issue-token.outputs.token }} script: | const marker = process.env.MARKER; const issueTitle = process.env.ISSUE_TITLE; @@ -484,9 +489,11 @@ jobs: // The marker and title strings are public (embedded verbatim in // this workflow file), so matching on them alone would let anyone // with issue-creation permission in the caller repo craft a decoy - // this automation adopts. Restricting to issues this workflow's - // own identity authored closes that: an attacker's issue is never - // a candidate at all, no matter what text it carries. + // this automation adopts — then overwrites or closes. Restricting + // to issues authored by the caller's Actions identity narrows + // candidates to what a workflow in that repo wrote, so an issue + // opened by a human or a third-party App is never a candidate at + // all, whatever text it carries. const candidates = openIssues.filter( (issue) => !issue.pull_request && issue.user?.login === issueAuthorLogin && issue.user?.type === 'Bot', ); @@ -516,7 +523,6 @@ jobs: if: steps.scan.outputs.stuck-count != '0' || steps.scan.outputs.unarmed-count != '0' uses: peter-evans/create-issue-from-file@fca9117c27cdc29c6c4db3b86c48e4115a786710 # v6.0.0 with: - token: ${{ steps.issue-token.outputs.token }} title: '[Alert] standards-sync auto-merge PR(s) needing attention' issue-number: ${{ steps.tracking.outputs.issue-number }} content-filepath: ${{ github.workspace }}/.stuck-automerge-report.md @@ -524,41 +530,23 @@ jobs: # actions/github-script rather than a `gh issue close` run step, for # the same runner-tool-availability reason as the lookup step above. - name: Close recovered tracking issue - if: steps.scan.outputs.stuck-count == '0' && steps.scan.outputs.unarmed-count == '0' + if: steps.scan.outputs.stuck-count == '0' && steps.scan.outputs.unarmed-count == '0' && steps.tracking.outputs.issue-number != '' uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 env: - MARKER: '' + ISSUE_NUMBER: ${{ steps.tracking.outputs.issue-number }} with: - github-token: ${{ steps.issue-token.outputs.token }} script: | - const marker = process.env.MARKER; - const issueAuthorLogin = process.env.ISSUE_AUTHOR_LOGIN; - const openIssues = await github.paginate(github.rest.issues.listForRepo, { - owner: context.repo.owner, - repo: context.repo.repo, - state: 'open', - per_page: 100, - }); - // Same author restriction as the lookup step: only ever act on an - // issue this workflow's own identity opened, never a decoy. - const match = openIssues.find( - (issue) => - !issue.pull_request && - issue.user?.login === issueAuthorLogin && - issue.user?.type === 'Bot' && - (issue.body ?? '').includes(marker), - ); - if (!match) return; + const issueNumber = Number(process.env.ISSUE_NUMBER); await github.rest.issues.createComment({ owner: context.repo.owner, repo: context.repo.repo, - issue_number: match.number, + issue_number: issueNumber, body: 'No standards-sync pull request is currently armed and blocked, or unarmed past the threshold; closing this rolling report.', }); await github.rest.issues.update({ owner: context.repo.owner, repo: context.repo.repo, - issue_number: match.number, + issue_number: issueNumber, state: 'closed', state_reason: 'completed', });