Repository navigation
fix(standards-sync-stuck-automerge-alert)!: author the tracking issue with the ambient token - #347
kyle-sexton wants to merge 2 commits into
Conversation
…with the ambient token Every run of this workflow has failed since it was first scheduled. The `issue-token` step minted a second App token with `owner`/`repositories` omitted, which `actions/create-github-app-token` resolves to the calling repository. The only caller is melodic-software/standards, the sync SOURCE, which is deliberately not a sync target and so is outside the App's selected installation — the mint 404s on `GET /repos/melodic-software/standards/installation`. Converge on the ambient-`GITHUB_TOKEN` authorship its sibling rolling-report workflows already use (link-check.yml, queue-monitor-liveness.yml, pulumi-version-drift-check.yml). Ambient `GITHUB_TOKEN` is scoped to the calling repository by construction, which is exactly what the 404 says the App identity cannot be. The App secrets stay required and unrenamed: they still mint the cross-repository read token that scans the targets. Fold in the sibling's guard shape while here: `Find existing tracking issue` now runs unconditionally and `Close recovered tracking issue` consumes its `issue-number` behind an `!= ''` guard, replacing a duplicate lookup that carried its own author filter. BREAKING CHANGE: the caller's job must now grant `issues: write`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TFFtZLysg75gm7hviQM5wU
…contract The close step no longer resolves an issue of its own, so the two tests that drove it with a list of open issues asserted semantics it no longer has. Its decoy protection now lives entirely in its `if:` — the author-filtered lookup result plus the `issue-number != ''` conjunct — and no mock can reach an `if:` expression, so assert it structurally, together with the lookup running unconditionally. The two halves are only correct together: guarding the lookup on the alert condition would starve the close step on exactly the healthy runs it serves. Also record in the workflow header what the reusable's kebab-case secret input names cost a governed consumer: a runner-policy contract can pin the caller's `issues: write` grant only when every mapped secret's input name equals the repository secret it reads. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TFFtZLysg75gm7hviQM5wU
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Claude finished @kyle-sexton's task in 3m 43s —— View job Review:
|
|
Claude finished @kyle-sexton's task in 2m 8s —— View job Security review
This PR only touches SUGGESTION — decoy-authorship surface widens from one App identity to every workflow with
|
|
Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
|
Closing in favor of #346 (shape 2). Shape 1' authors the tracking issue with the ambient GITHUB_TOKEN, which is scoped to the repo where the workflow runs — Branch |
No linked issue in this repository. The two issues that track this defect live
in
melodic-software/standards(#273, #274) and are deliberately not closed bya native keyword here: this PR is only the ci-workflows half, and cross-repo
closing keywords fire on merge.
What was broken
standards-sync-stuck-automerge-alert.ymlhas failed every run since it wasfirst scheduled — 254 of 254 at the time of investigation, 100 of the last
100 confirmed by
gh run list. Zero successes, ever.Detection was never the problem. The
Mint App token for this repository's tracking issuestep minted a second App token withownerandrepositoriesomitted, whichactions/create-github-app-tokenresolves to thecalling repository. The only caller is
melodic-software/standards— thesync source, which
distribution/sync-manifest.ymlstates is "manifestsource, not a target" and is therefore outside the sync App's 8-repo selected
installation. Live evidence from run
30862951664:
The first token mint —
owner: melodic-software,repositories: <manifest targets>— completed fine two seconds earlier. Only the caller-scoped mint404s.
The fix
Converge this workflow onto the ambient-
GITHUB_TOKENauthorship its siblingrolling-report workflows already use, and retire the App identity for
tracking-issue authorship.
Ambient
GITHUB_TOKENis scoped to the calling repository by construction,which is precisely what a 404 on the caller's installation says the App
identity cannot be. This workflow was the sole outlier among the repo's
tracking-issue workflows, and the outlier is the broken one:
link-check.ymlGITHUB_TOKEN(link-check.yml:129)queue-monitor-liveness.ymlGITHUB_TOKEN(queue-monitor-liveness.yml:157)tool-version-drift-check.ymlGITHUB_TOKEN(tool-version-drift-check.yml:446)pulumi-version-drift-check.ymlGITHUB_TOKEN, no token step at allREADME.md:337-342already describes the ambient shape ("a scheduled callerthat grants
issues: write; the tracking issue lands in the caller's ownrepository"). It needed no edit — the App-token detour is what had drifted from
the documented contract.
Changes:
issue-tokenstep. The three consumers run on the ambient token(
github-scriptdefaults togithub.token;peter-evans/create-issue-from-filedefaults to${{ github.token }}).ISSUE_AUTHOR_LOGIN→github-actions[bot]. It is declared once at workflowlevel, so the single change covers both the find and the close filters.
issues: write.Find existing tracking issuenow runsunconditionally, and
Close recovered tracking issueconsumes itsissue-numberbehind an!= ''guard — matchinglink-check.yml:246-247—replacing a duplicated lookup that carried its own author filter.
The App secrets stay
required: trueand keep their names. They still mint thecross-repository read token that scans the 8 targets; only the second,
caller-scoped mint is gone.
BREAKING:
workflow_callconsumer contractThe caller's job must now grant
issues: write. Previously it needed nowrite permission at all, because the App token supplied the write capability. A
called workflow can only downgrade the caller's grant, never elevate it, so a
caller that does not grant it gets a hard failure on the issue write.
Affected callers — full org sweep (
gh search code --owner melodic-softwarefor
standards-sync-stuck-automerge-alert.yml@):melodic-software/standards.github/workflows/standards-sync-stuck-automerge-alert.ymlworkflow_callcallermelodic-software/standardscomponents/runner-policy/policy.jsonmelodic-software/{claude-code-plugins,github-iac,dotfiles,provisioning,medley}.github/standards/runner-policy/policy.jsonmelodic-software/ci-runnerreferences this workflow only as the subject itsqueue monitor watches, not as a caller.
Merging this PR is inert for that caller: it pins
@8202e03, so nothingchanges in production until the companion
standardsPR re-pins. There is nointerim breakage window.
Required companion change in
melodic-software/standardsOne PR, and all three parts must land together — a re-pin alone yields a
403 on the issue write:
.github/workflows/standards-sync-stuck-automerge-alert.yml'suses:to this PR's merge SHA, with the matching short-SHA trailing comment.
permissions: {contents: read, issues: write}to thealertjob (it currently inherits workflow-level
contents: read). Same shape asthat repo's own
link-check.ymlcaller.components/runner-policy/policy.json, keeping the current shape:Auto-approval cannot admit this entry — a secret-capable runner-input contract
declines it by design — so it is a human contract review either way.
On the secret rename this PR deliberately does not make
The brief for this work carried a premise that granting the caller
issues: writeforces anallowedCallerPermissionsentry in the contract, which inturn forces the kebab-case secret inputs (
app-client-id,app-private-key)to be renamed to identity-passthrough form. That premise does not hold, and
the rename is therefore not made.
Verified by executing the real validator
(
components/runner-policy/runner-policy.mjs) against the real repo config,four variants plus a negative control:
issues: writeallowedCallerPermissionsallowedCallerPermissionsallowedCallerPermissionsallowedCallerPermissionsrunner-target-contract) — the probe genuinely exercises the callerThe passthrough requirement (
runner-policy.mjs:204-213) fires if and onlyif the contract declares
allowedCallerPermissions. Nothing forces thatfield:
standardsisvisibility: public,selfHostedCi: false, so theprivileged-hosted machinery that would demand the waiver never engages, and
reusableWorkflowStatuschecks caller permissions only when the contractdeclares them.
The comment this PR removes (old lines 78-88) was not false — it correctly
stated that a contract with
allowedCallerPermissionsneeds passthroughsecrets. It was a non-sequitur: nothing required adding that field. The real
tradeoff is now recorded in the workflow header instead of being deleted
silently —
link-check.ymlandpulumi-version-drift-check.ymlpin theircallers' grants via
allowedCallerPermissionsbecause they take no secrets;this workflow's contract can pin the secrets or the grant, not both, until the
inputs are renamed. Renaming them is available later at the cost of a genuinely
breaking secret-interface change; it is not needed to fix this.
Semantic changes a diff reader would not notice
Stated rather than slipped in:
close path still acts only on an issue matching
ISSUE_AUTHOR_LOGINwithuser.type === 'Bot'. What widens is who can author a qualifying decoy:from "only the sync App" to "any workflow in the caller repo". Both require
write access to that repo. This is exactly the posture
queue-monitor-liveness.ymlalready ships (ambient identity, marker + authorfilter, no label filter), so no label filter is added here.
link-check.yml's label filter exists because it is a multi-invocationreusable whose marker is keyed by a title hash — a different problem.
matches.length > 1fail-closed guard previously ran only on alert runs; theclose path used
.find()and silently took the first match. Now aduplicate-marker state turns a healthy run red. Stricter, and identical to
link-check.yml.required the marker in the body; it now closes whatever the lookup resolved,
which includes the pre-marker title fallback. Still constrained to
github-actions[bot]-authored issues with the exact title. No such issueexists today — the workflow has never had a successful run, so it has never
authored an issue at all.
Verification
actionlint— clean.zizmorv1.28.0 — no findings.typos— clean.node --test .github/scripts/*.test.cjs— 496 pass, 0 fail.biome ci@2.5.4 with the repo's config, exactly asci.ymlinvokes it —clean.
comment-hygienescan — 18 findings, byte-identical to theorigin/mainbaseline; this change adds none.
Two close-path tests asserted the old semantics and are rewritten. Because the
close step's decoy protection now lives in an
if:expression — which no mockcan reach — it is asserted structurally, together with the lookup running
unconditionally, since the two halves are only correct together. The new
assertion was mutation-tested: re-adding an
if:to the lookup step fails it.A fresh-context verifier (rationale withheld) audited the change independently
and found the test-suite breakage above, which is fixed in the second commit.
Its remaining verdicts: the 404 is genuinely eliminated rather than relocated
(no surviving path mints or consumes a caller-scoped App token); both outcome
branches receive a usable token and issue reference; every issue mutation stays
behind the author filter; and no silently-green failure mode exists — every
partial-landing state it traced fails red, and an alert run always ends red via
the
Fail so the scheduled run notifiesstep.Residue — stated plainly
The 404 is fully eliminated from this reusable. The watchdog is not
functional until the
standardscompanion change lands. That is not residuein this PR; it is the cross-repo shape of the fix. Nothing here defers the
failure or converts it into a quieter one.
One item deferred with a trigger: a private, selector-routed consumer is
the only case that would need
allowedCallerPermissionson this contract, andno current caller is selector-routed. Should one appear, the resolution is not
necessarily the secret rename — splitting the cross-repository scan off the App
secrets is the likelier fix — so nothing is pre-committed.
Not done, deliberately, per the constraint that the attest step requires the
App installation's selected set to equal the derived target set:
standardsis not added to the sync App's installation or as a manifest target. Any
addition would fail the sync for every target.
Related
melodic-software/standardsissues docs(claude-security-review): stop recommending merge_group and track the carve-out #273 and feat(actions): extract the Claude lane composite trio (PR-A1) #274 — the tracking issues forthis defect. Closed by the companion PR there, not by this one.
.github/workflows/link-check.yml,.github/workflows/queue-monitor-liveness.yml,.github/workflows/pulumi-version-drift-check.yml.standards:.github/workflows/claude-lanes-repin.ymlcarries a comment citing "the sibling standards-sync-stuck-automerge-alert
reusable uses for its own single-repository token" — the step this PR deletes.