Repository navigation
fix(standards-sync-stuck-automerge-alert): name the tracking-issue repository instead of assuming the caller - #346
Conversation
…pository instead of assuming the caller The tracking-issue token was minted with `owner` and `repositories` both unset, which scopes it to the calling repository. The caller is melodic-software/standards — the sync SOURCE, deliberately not a manifest target — so the sync App is not installed on it and the mint 404s. Every scheduled run has failed since 2026-07-22, so the watchdog has never reported a stuck PR. A required `tracking-issue-repository` input now names the destination: a bare repository name under the caller's own owner that the App is installed on. Required rather than defaulted, because a default would keep the reachable case the exception and turn an unreachable destination back into a 404 discovered only when the alert first has something to report. All four issue-touching surfaces move onto it — the mint's `repositories`, the lookup's and close's `listForRepo`/`createComment`/ `update`, and create-issue-from-file's `repository`, which otherwise defaults to the caller. The input reaches the two `script:` bodies through `env:`, never interpolated. The mint stays unconditional. Its three consumers partition the outcome space: the lookup and open/update run when a count is non-zero, and the recovery close runs when BOTH are zero, so guarding the mint on the alerting condition would strand the close branch on exactly the runs it exists for. A test pins that partition. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…racking-issue repository An `owner/repo` value mints a token successfully — create-github-app-token parses an owner-qualified entry and checks the owner half — so the run gets past the credential and dies later and less legibly: `listForRepo` 404s on a name containing a slash, and the issue write concatenates a second owner onto it. A first-step guard rejects anything that is not a bare repository name, before the standards checkout and the scan. Its `run:` block is executed by the tests, not pattern-matched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
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 19s —— View job Review of PR #346
Scope: SummaryThis is a well-scoped, well-documented fix. I read the full diff and the surrounding unchanged code for context; findings below are my own read, not a restatement of the PR's claimed test plan. Correctness:
One minor test-coverage gap (posted inline on Verification notes:
Nothing else stood out — the README update accurately reflects the new contract, and the architectural reasoning in the header comments matches what the code actually does. |
|
Claude finished @kyle-sexton's task in 2m 13s —— View job Security review
No security issues found. This PR only touches What I specifically checked and why it holds up:
No |
…om the test comments comment-hygiene flags an issue-tracker reference in a code comment, and the two here carry no meaning a future reader loses: the comments state the credential mismatch the fixtures exist to catch, which stands without a number to look up. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 1m 31s —— View job Security review
No security issues found. The only commit added since the last review on this PR ( What I checked, scoped to
No |
|
Claude has reviewed this PR 2 times. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
…cking-issue repository names Both match the repository-name character class the guard already enforces, and GitHub forbids both as repository names, so either would reach the REST client as a path segment rather than a name. Rejecting them at the guard keeps its claim exact rather than approximate — the same reason the guard rejects `owner/repo` instead of letting the mint accept it and something downstream fail less legibly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 50s —— View job Security review
No security issues found. Since the last security review on this PR ( What changed: workflow#L130-L139 extends the first-step guard to explicitly reject This is a strict tightening, not a new surface:
No |
|
Closing unmerged: authorization for this fix shape was withdrawn. The fix shape for this defect is a human-reserved decision on melodic-software/standards#274 (status: needs-decision, needs-human), and it is returning there rather than being picked here. No change is being proposed. The branch |
|
Claude finished @kyle-sexton's task in 1m 59s —— View job Review of PR #346 (re-triggered on reopen)
SummaryNo code has changed since the prior review round on this PR — HEAD is still What I re-checked directly (not restating the PR's own claims):
Still outstanding — the one finding from the prior review round has not been addressed: the mutation-tested assertions at No new correctness or CLAUDE.md-alignment issues found beyond that one. |
|
Claude finished @kyle-sexton's task in 39s —— View job Security review
No security issues found. This run was triggered by the PR being reopened, not by a new commit — That review's conclusions stand:
No Separately, for context: the PR author's reopen/close history on this thread indicates the fix shape here was later withdrawn pending a human decision on |
…nt's absent owner The absence of `owner` on that step is what makes a bare repository name resolve under the calling repository's owner, and it was the one property of the step nothing asserted. The sibling every-target mint pins `owner: melodic-software` deliberately, so both halves are pinned: the tracking mint must have no `owner` key and the every-target mint must keep its own, which fails a harmonization of the two in either direction rather than letting one silently undo the other. Mutation-checked both ways: adding `owner:` to the tracking mint, and removing it from the every-target mint, each turn the test red. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 38s —— View job Security review (re-triggered on new commit)
No security issues found. The only commit added since the last security review on this PR (
Since no workflow logic changed, the trust-boundary analysis from the prior security reviews on this PR (
No |
|
Closing the loop on the assert.doesNotMatch(
mintStep,
/^\s*owner:/mu,
"`owner` must stay unset so a bare repository name resolves under the caller's own owner",
);This is stronger than the gap required. It pins both mints — the tracking-issue mint must have no Also confirmed independently on this head: the mint at line 500 carries no 38 checks green. One thing to carry forward, not a change to this PR: merging it does not stop the hourly failures. |
Summary
The watchdog's tracking-issue token is minted with
ownerandrepositoriesboth unset, which
actions/create-github-app-tokenresolves to the callingrepository. The caller is
melodic-software/standards— the sync SOURCE, anddeliberately not a manifest target — so the sync App is not installed on it and
the mint 404s:
254 of 254 scheduled runs have failed since 2026-07-22. The stuck-PR alarm has
never fired, including while a real instance of the condition was in flight
(
melodic-software/dotfiles#328satBLOCKEDfor ~90 minutes with the watchdogrunning hourly throughout).
A required
tracking-issue-repositoryinput now names the destination: a barerepository name under the caller's own owner that the App is installed on.
Why required, not defaulted
A default reproduces the defect's shape. The App is what authors the issue, so
the destination must be a repository the App is installed on — and the caller
need not be one. Defaulting to the caller makes the reachable case the exception
and turns an unreachable destination back into a 404 discovered only when the
alert first has something to report, which is precisely when it must not fail.
Every consumer of this repository pins by 40-char SHA, so the contract change
strands no existing caller; a re-pinned caller that omits the input is rejected
for the missing input it can see and fix, rather than 404-ing at the mint. (The
docs state
requiredmeans "the input must be supplied" but do not specifywhether that is detected at workflow validation or at invocation, and this was
not tested live — either way the failure names the input.)
Why a bare name, not
owner/repoThe mint leaves
ownerunset, which resolves a repository entry's owner to thecaller's own owner — the same owner that owns the App installation the caller's
secrets belong to. A destination under a different owner needs a different
installation and therefore different secrets: a different caller, not a
different input value. A first-step guard rejects anything that is not a bare
name, because
owner/repomints successfully and then fails later and lesslegibly —
listForRepo404s on a name containing a slash, and the issue writeconcatenates a second owner onto it.
Every issue-touching surface moves
owner/repositoriesunset, so the callerrepositories: <input>context.repo<input>create-issue-from-filerepositorydefault${{ github.repository }}<owner>/<input>context.repo<input>The input reaches both
script:bodies throughenv:, never interpolated intoscript text.
The mint stays unconditional — deliberately
Its three consumers partition the outcome space rather than sharing one
condition: the lookup and the open/update run when a count is non-zero, and the
recovery close runs when both are zero. Guarding the mint on the alerting
condition would strand the close branch on exactly the runs it exists for, so a
clean-recovery run would try to close the issue with no token. A test now pins
that partition, so the guard cannot be added later by someone reading the
unconditional mint as an oversight.
Scope: this PR does not make the watchdog work on its own
The caller pins by SHA, so landing this changes nothing until
melodic-software/standardsre-pins and passes the input. That follow-up isin a different repository and is not attempted here. It must:
tracking-issue-repository: <a manifest target>, andthis repo — the calling repo".
The destination must be one of the 8 manifest targets — the installation's
selected set must EQUAL the derived target set for the sync's attest step, so
installing the App on
standardsis closed on design grounds. That is a realconsequence to accept knowingly: the standards-fleet alert will live in a sync
target rather than in
standards. Two axes to weigh when picking one:medley,dotfiles,github-iac,provisioning) keepthe alert body — which embeds PR links for private repositories — unpublished.
Strictly better on disclosure than today, where it would land in the public
standards.ci-workflows,.github,ci-runner,claude-code-plugins) republish those links, which is no worse than thecurrent design but no better.
Recommendation: a private target, and
medleyif the operator's watch list isalready there.
Test plan
node --test .github/scripts/*.test.cjs— 504 pass, 0 fail (was 501).actionlint— clean.zizmor --persona=regular— no findings.biome ci --error-on-warningsat the repo's own config — clean.editorconfig-checker,typos,markdownlint-cli2— clean on every changedfile.
to
context.repo.repo, droppingrepositories:from the mint, droppingrepository:from the issue write, adding adefault:, droppingrequired: true, guarding the mint, and widening the guard regex each fail atleast one new test. The caller and destination fixtures are deliberately
different values, so a regression to
context.repo.repocannot stay green.run:block is executed underbashby the tests (thepattern
claude-security-review-fail-closed.test.cjsalready uses), notpattern-matched, and the test refuses a block containing a
${{ }}expression, so what runs locally is what runs in CI.
Verified against sources rather than assumed
ownerunset +repositoriesset resolves the owner toGITHUB_REPOSITORY_OWNERand looks upGET /repos/{owner}/{repo}/installationagainst the named repository —read from
lib/main.jsat the pinned SHAbcd2ba4, not from the inputdescriptions. The observed failure log matches that source's both-unset
branch verbatim.
peter-evans/create-issue-from-filedefaultsrepositoryto${{ github.repository }}— read fromaction.ymlat the pinned SHAfca9117.melodic-standards-syncApp holdsissues: writeandrepository_selection: selected— confirmed live viagh api orgs/melodic-software/installations.An independent fresh-context verifier reviewed the diff with the reasoning
withheld and confirmed the four judgement calls above. Its one substantive
finding — that the bare-name shape was documented but unenforced — is the
second commit.
Related
No linked issue: this repository holds no tracking issue for the defect, and
this change does not on its own make the watchdog work (see Scope above), so
it would be wrong to auto-close one. It relates to
melodic-software/standards#273andmelodic-software/standards#274, whichtrack the defect and stay open until the caller-side follow-up lands there.
🤖 Generated with Claude Code
https://claude.ai/code/session_01TFFtZLysg75gm7hviQM5wU