chore(distribution): re-disarm fleet automerge until the watchdog is fixed - #315
Merged
Merged
Conversation
…fixed Restores `automerge: false` on all eight sync targets, reversing #299. The stuck-automerge watchdog that would catch an armed sync PR wedging has never once succeeded: `standards-sync-stuck-automerge-alert` has failed every run since 2026-07-22 — 256 of 256 as of 2026-08-03 — on the App token mint, because `standards` is the sync source, not a target, and the App's selected access covers only the eight targets (standards#273, standards#274). The Phase 3d rollout window was the only thing masking that, and #299 closed the window on 2026-07-30 — arming auto-merge fleet-wide against an alarm that has never fired. This puts the gate back where the #274 triage placed it: the watchdog fix gates the restore, not the other way round. Mechanism is the manifest's own opt-out idiom, the exact inverse of #299: explicit `automerge: false` per target, since absent means true (`distribution/sync-manifest.sh:428`, `distribution/README.md:41`). The comment states the current reason rather than restoring #299's stale Phase 3d prose. Temporary. Reverts when standards#274 ships the watchdog fix. 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 2m 2s —— View job Todo List
ReviewScope: code-review lane (correctness, maintainability, docs, test coverage). No Change is correct and minimal. Verified mechanically against the diff (
I wasn't able to execute No correctness, maintainability, or test-coverage issues found. This is a clean, narrowly-scoped, well-documented policy revert. |
|
Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd4dc2bb97
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
Author
|
Addressing the P1 on already-armed sync PRs. The finding is correct: this manifest change governs future arming only, and per Taking the second branch the review offers — establishing and recording that none are currently armed. Swept every one of the 8 manifest targets for open Zero open sync PRs fleet-wide, therefore zero armed ones. There is no PR for a transition step to act on, so adding one would be unreachable code. The hazard is real in general and has no live instance at this moment. One consequence worth stating plainly, since it bounds how long this record stays true: the window is open until this merges. If a sync run fires in between, it will arm a PR under the current opt-out default, and that PR would need disarming by hand. Re-running the sweep immediately before merge is the cheap way to close that. Context for why this PR exists: the stuck-automerge watchdog (standards#273) has failed 254 of 254 runs since 2026-07-22 — it has never once succeeded. The 2026-07-29 triage on standards#274 recorded that the watchdog fix should gate restoring fleet automerge; PR #299 restored it on 2026-07-30 anyway. This restores the gate until #274 ships. |
Contributor
Author
|
Re-swept immediately before merge, since the earlier record was point-in-time. 2026-08-04 02:05Z — all 8 manifest targets checked for open Unchanged from the first sweep. No sync run fired in the interval, so there is still no already-armed PR for a transition step to act on, and the P1's second branch — establish and record that none are armed — holds at merge time rather than only at authoring time. |
kyle-sexton
added a commit
that referenced
this pull request
Aug 4, 2026
) ## Summary `standards-sync-stuck-automerge-alert` has failed **254 of 254 runs** since 2026-07-22. Not mostly — every one. Detection was never the problem; the issue-filing identity was. The reusable's second token mint carried no `repositories` input, so `actions/create-github-app-token` defaulted to the calling repository and 404'd on `GET /repos/melodic-software/standards/installation`. `standards` is the sync *source*, deliberately **not** a target, and therefore outside the sync App's 8-repo selected access. A watchdog that dies before it can report is worse than no watchdog: its silence reads as "all clear". ci-workflows#346 merged the fix — a required `tracking-issue-repository` input — at `ed6d410c1fe10d9deb40241bbe46428b31fafd57`. **That merge changed nothing here on its own**, because this caller pins by SHA. This PR is what actually closes the defect. ### The destination is `medley`, and the reason is disclosure, not convenience The alert body builds a per-PR table embedding each reported PR's repository name, number, and URL. `standards` is **PUBLIC**; four of the eight scanned targets are **PRIVATE** (`dotfiles`, `github-iac`, `medley`, `provisioning`). Filing the issue here would republish private repository names and PR URLs in public, every hour, for as long as the condition held. `medley` is private, is inside the App's selected access, and is where the reusable's own header already imagines this alert living. `medley` is *also* one of the scanned targets — it may end up reporting on its own sync PRs. That is incidental, not the reason it was chosen, and the file header now says so rather than implying the destination sits outside the scan. ### Why the App installation is not widened instead Adding `standards` to the sync App's installation would "fix" the 404 and break everything else: `sync.yml`'s `attest` job requires the installation's selected set to **equal** the derived target set, so any addition fails the sync for *every* target. `distribution/sync-manifest.yml` states the position directly — `standards` is "manifest source, not a target". Closed on design grounds. This is the reason issue #274's suggested fix is not the one taken; see **Related**. ### Three files, and why the second two are not optional | File | Change | |---|---| | `.github/workflows/standards-sync-stuck-automerge-alert.yml` | Re-pin `8202e03f` → `ed6d410c`; pass `tracking-issue-repository: medley`; correct the now-false header prose. | | `components/runner-policy/policy.json` | New approved-contract entry for `ed6d410c`, `allowedInputs` grown by `tracking-issue-repository`. | | `components/runner-policy/README.md` | The claim "the caller passes only `runner`" becomes false with this change. | The runner-policy contract entry is a **hard gate**, not bookkeeping: without it, `npm run lint:runner-policy` rejects both the unknown SHA and the input the caller now passes. The entry is **added**, not replaced — this map retains superseded revisions (six `standards-sync.yml` SHAs, and the alert's own `43bc8d0f` and `8202e03f`, are all still present) so consumers still pinning an older SHA keep a contract. `8202e03f` additionally remains live for `sync.yml` under a different workflow key. `allowedInputs` is exactly the reusable's five `workflow_call` inputs at `ed6d410c` — no input the caller passes is missing, and no input the reusable does not define is claimed. ### The value must be a bare name `tracking-issue-repository: medley`, never `melodic-software/medley`. The reusable's *first* step — before the checkout, so a malformed destination fails early rather than confusingly late — rejects anything outside `^[A-Za-z0-9._-]+$`, plus the literal `.` and `..`. An `owner/repo` value mints a token fine and then breaks downstream, which is precisely the failure the guard exists to pre-empt. No caller-side permission change is needed: the reusable mints its own issue-scoped token, so this workflow's ambient `GITHUB_TOKEN` still needs only `contents: read`. ## Test plan Actual output from this branch, not a description of what would run: ```text bash distribution/sync-manifest.sh validate -> Manifest valid: 34 components, 8 targets npm run lint:runner-policy -> Runner policy passed. npm run test:runner-policy -> tests 242 | pass 242 | fail 0 npm run lint:md -> 105 files | 0 issues in 0 files npm run lint:hooks -> All good npm run lint:concurrency-policy -> Concurrency policy passed. npm run lint:dependabot-policy -> Dependabot policy passed. npm run test:concurrency-policy -> pass 24 | fail 0 npm run test:dependabot-policy -> pass 35 | fail 0 npm run test:packages -> pass 14 | fail 0 components/pin-comment-convention/scan-workflow-files.sh -> exit 0 harness/shell/run-tests.sh .../pin-comment-convention.test.sh -> 1 passed, 0 failed actionlint <the changed workflow> -> exit 0, no output ``` Verified from primary sources rather than assumed: - **The pinned SHA is #346's merge commit.** `gh pr view 346 --repo melodic-software/ci-workflows` → `state MERGED`, `mergeCommit.oid ed6d410c1fe10d9deb40241bbe46428b31fafd57`, `mergedAt 2026-08-04T02:07:30Z`; reachable from `ci-workflows` `origin/main`. - **`medley` is in the App's selected access.** `sync.yml` run `30870837177`, `attest` job: `EXPECTED_REPOSITORIES` lists all eight targets including `melodic-software/medley`, and the job passed — attest requires *equality*, so this is proof of the whole set, and proof `standards` is outside it. - **The App can author issues.** `gh api orgs/melodic-software/installations` → `melodic-standards-sync` carries `issues: write`, `repository_selection: selected`, unsuspended. The mint is satisfiable. - **The input contract.** Read from the reusable at `ed6d410c`, not from a summary: `tracking-issue-repository` is `type: string`, `required: true`, and the first-step guard's accepted/rejected forms are as described above. - **Repository visibility.** `gh repo view` per repo: `standards` PUBLIC; `dotfiles`, `github-iac`, `medley`, `provisioning` PRIVATE. An independent fresh-context agent re-derived all of the above with the author's reasoning withheld, and **found a defect that is fixed in this branch**: the header first claimed the destination was "neither this repo nor any scanned target", which is false of `medley`. Commit `9d3fced` corrects it. Its one remaining note is recorded below. ### Known limitation of the pin-comment date The comment reads `# ed6d410 2026-08-04`, the commit's **UTC** date, matching `mergedAt`. The convention in `components/pin-comment-convention/` fixes the *form* (`YYYY-MM-DD`) but never says whose date, and the repo's own precedent is genuinely split — `31a5b76 2026-07-21` used UTC, `f2d5e06 2026-07-14` used the commit's local-offset date. The machine check accepts either, so no claim of "matches precedent" would be honest in either direction. UTC is chosen as the observer-independent timestamp. Pinning this down belongs in the convention component, not in this PR. ## Related Closes #273. **#274 is deliberately left open**, not closed by this PR, and that is a judgement call worth stating rather than burying: - It reports the *same* failure as #273 from a second angle, and this PR does resolve that symptom. - Its suggested fix — "re-add `standards` to the App's selected-repositories list" (items 1 and 2) — is **rejected on design grounds**, not merely unimplemented. The `attest` equality check makes that change break the sync for all eight targets. Closing #274 with a keyword would record it as "fixed as suggested", which is the opposite of what happened. - Its item 3 — *make the alert fail loudly on a token-minting error, so the next occurrence is visible without someone reading run history* — is a genuine, still-unaddressed residual. This PR removes the specific cause of the current 404; it does not add alerting on the alarm's own failure. That residual is exactly the reason the defect went 254 runs unnoticed, and it deserves to survive as tracked work rather than being auto-closed. Recommended follow-up for a human: rescope #274 to its item 3, and close it as a duplicate of #273 only if that residual is retracted or refiled elsewhere. Delivers ci-workflows#346 to this consumer. Does not touch `distribution/sync-manifest.yml` — standards#315 landed there at 2026-08-04T02:07:15Z and this branch is built on that main. After merge, the next `17 * * * *` run is the first that can succeed. Nothing else is required to stop the hourly failures. ### What to expect after merge — green, and quiet Two effects, worth stating so neither reads as a regression: - **The watchdog will run green and report nothing.** #315 set `automerge: false` on all eight targets, so the never-armed half is inert by design, and the armed-but-BLOCKED half has nothing to find — `gh pr list --author app/melodic-standards-sync --state open` returns no open sync PR anywhere in the fleet right now. A green empty scan is the correct outcome, not evidence the fix did not take. The first real exercise of the issue-filing path comes when `automerge: true` is restored and a PR actually wedges. - **The `runner-policy` component is sync-managed**, so merging this cascades `policy.json` to all eight targets as ordinary sync PRs. Adding a contract entry (rather than replacing one) is what keeps that cascade safe: a target still pinning `8202e03f` keeps its contract and its lint stays green. Because the empty-scan case cannot distinguish "fixed" from "still broken by some later step", the honest verification after merge is a `workflow_dispatch` run confirming the **token mint step itself** now succeeds — the step that has been 404ing — rather than inferring it from a green run. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Restores
automerge: falseon all eight sync targets indistribution/sync-manifest.yml— the exact inverse of #299, which removedthose keys on 2026-07-30.
This is temporary. It reverts when #274 ships the watchdog fix.
Why
The stuck-automerge watchdog that would catch an armed sync PR wedging has
never once succeeded.
standards-sync-stuck-automerge-alerthas failedevery run since 2026-07-22 — 256 of 256 as of 2026-08-03 — on the App token
mint (
GET /repos/melodic-software/standards/installation→ 404), becausestandardsis the sync source, not a target, and the sync App's selectedaccess covers only the eight targets. That is #273; the fix is #274.
#274's own triage stated the ordering: "the Phase 3d window is the only thing
currently masking a dead watchdog. When
automerge: falseis reverted, syncPRs start arming again against an alarm that has never once succeeded. The
watchdog fix should gate that revert." #299 closed the window before that fix
landed, so for the last four days all eight targets have armed auto-merge on
sync PRs with no working alarm behind them. This puts the gate back where the
triage placed it.
Mechanism
The manifest's own opt-out idiom, not a new one: an explicit
automerge: falseper target, because absent means true — "automerge is opt-out policy-as-data,
so a target that never mentions the key keeps the fleet-default armed
behavior" (
distribution/sync-manifest.sh:428, alsodistribution/README.md:41). #299 disarmed by removing the keys; thisre-arms the opt-out by restoring them, in the same position in each target
block.
The comment above
targets:states the current reason (dead watchdog, gated on#274) rather than restoring #299's Phase 3d rollout-window prose, which is now
stale — Phase 3d closed. Faithful reversal applies to the mechanism, not to
prose that is no longer true.
The target set is untouched. No target added, none removed, no
managedorlocally-ownedassignment changed, and no GitHub App installationconfiguration touched — the attest step requires the installation's selected
set to equal the derived target set, so any addition would fail the sync for
every target.
Test plan
bash distribution/sync-manifest.sh validate→Manifest valid: 34 components, 8 targets, exit 0.bash distribution/sync-manifest.sh matrix→ all 8 targets emit"automerge":falseas a JSON boolean (the behavioral proof, not justtext).
diff <(yq '.targets | map_values(del(.automerge))' -o=json)betweenorigin/mainand this branch → empty. Same for.components.git diff --name-onlyvs merge base → exactly one file,distribution/sync-manifest.yml.bash harness/shell/run-tests.sh distribution/sync-manifest.test.sh→ 1passed, 0 failed.
npm run test:runner-policy→ 242 pass, 0 fail.npm run lint:runner-policy→
Runner policy passed.(disarms all 8 per the manifest's own semantics; target set and App access
provably untouched; faithful inverse of chore: restore fleet automerge — close the Phase 3d rollout window #299 with the comment-text divergence
disclosed and justified; no other
automergeconsumer broken or falsified —it separately confirmed the
runner-policyREADME and the watchdog workflowheader describe the disarmed state as expected behavior, so neither becomes
false).
Related
No linked issue: this PR closes no GitHub issue, and deliberately leaves both
of the following open.