Re-sync dispatch-review.yaml to corrected canonical template - #318
Merged
Merged
Conversation
Contributor
Plugin Version PreviewPreview only — plugin versions and
|
Owner
Author
|
Checked both CI failures:
Generated by Claude Code |
nsheaps
changed the title
nsheaps
changed the title
Encodes the review-dispatch consumer pattern (dispatch-review.yaml + pr-status-dispatch.yaml, canonical source nsheaps/.github) as a skill for verifying and repairing per-repo drift between scheduled Ansible syncs. Also fixes the README's stale "Consumer setup" snippet, which still showed the pre-redesign REVIEW_GITHUB_APP_* secrets and on: trigger list.
… pr-status-dispatch pr-status-dispatch.yaml is a separate, unrelated mechanism (pings nsheaps/.org's PR digest) and doesn't belong in this skill's scope — it's now dispatch-review.yaml only. Rewrites the skill from "steps to manually audit/patch a repo" to explain and operate the actual distribution mechanism: nsheaps/.github's Ansible file-sync system (sync-files.yml + managed-repos.yml, sync-all.yaml). Covers how to change the template, add/remove consumer repos, and trigger a sync run (local mise task or workflow_dispatch) instead of hand-copying files into individual repos. Also updates sync-files.yml (companion PR on nsheaps/.github) so nsheaps/.github is no longer excluded from the dispatch-review.yaml target list -- it's a normal PR-taking repo and should get its own gate too.
…t note Independent verification flagged that docs/file-sync.md doesn't actually document the specific cron schedule or PR dry-run gating the skill attributed to it as "full reference" -- those facts only live in sync-all.yaml's own on: block and DRY_RUN gating. Re-attributed correctly and expanded the trigger-paths list to match the workflow file exactly. Also documents the failure mode just hit on nsheaps/homebrew-devsetup: a consumer repo's own formatter can silently re-diverge a synced file (prettier wrapping a long inline array) faster than anyone notices, so a synced path needs to be excluded from local formatting, not just fixed once.
…aphrase Re-verification found the trigger-paths list omitted ansible/playbooks/sync-all.yml (a distinct path, not covered by any glob already listed) despite claiming to match sync-all.yaml exactly. Replaced the paraphrase with the on.push.paths block's exact entries.
Companion fix on nsheaps/.github changed the review-dispatch trigger gate: reviews now fire automatically on any open, non-draft PR event instead of requiring the request-review label on every event (that label was never centrally provisioned before, so the gate was effectively inert almost everywhere). Re-syncs both copies here (the documentation mirror at templates/dispatch-review.yaml and this repo's own deployed .github/workflows/dispatch-review.yaml) to the corrected canonical content, and updates the skill to describe the actual gate logic plus the org-wide label-sync provisioning that now backs it.
Final verification round found the Non-goals secret-provisioning bullet omitted the github_auth role (which authenticates the sync run itself, not a secret provisioner) from its "ansible/roles/ only has..." list.
Per review feedback on the companion nsheaps/.github PR: converted_to_draft should not fire a review by itself. Re-syncs both copies here to the corrected canonical template (gate no longer has a converted_to_draft OR-clause, and it's dropped from on.types entirely), and updates the skill to match. Also documents a real regression just caught: the request-review label's original description was 141 chars, over GitHub's 100-char limit -- github-label-sync rejects the WHOLE org-labels.yaml file when that happens, breaking label sync org-wide (not just for the 7 repos this work touches). Fixed on the nsheaps/.github companion PR; documented here as a standing constraint.
…ithub The skill is fundamentally about nsheaps/.github's own Ansible file-sync mechanism, not this plugin's review pipeline -- moves it to nsheaps/.github/.claude/skills/sync-dispatch-workflows/ (companion commit there) rather than living in a marketplace plugin one repo removed from the thing it documents. Updates README's "What's in here" tree and the "Consumer setup" pointer, and plugin.json's description, accordingly.
…cture nsheaps/.github flattened its ansible/templates/ layout (no longer mirrors destination paths, e.g. ansible/templates/dispatch-review.yaml instead of ansible/templates/.github/workflows/dispatch-review.yaml) so it can safely be a normal sync target of its own templates. Updates this repo's synced copies' stale "Source:" comments and review-utils README's canonical-path reference to match.
nsheaps/.github restructured its sync source layout to mirror destination paths (ansible/templates/.github/workflows/dispatch-review.yaml) instead of the earlier flat layout. Update the reference here to match. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016wPHfMEiz9Bjau7twwZybf
nsheaps
force-pushed
the
claude/review-workflow-multi-repo-ljbuw0
branch
from
August 10, 2026 16:25
33a6d2f to
b02326a
Compare
…w-multi-repo-ljbuw0
apply-repo-settings.yaml and sync-main-to-edge.yaml had their Source: comment fixed on this branch back when nsheaps/.github's canonical templates/ layout was still flat. .github later restructured to nest templates under destination-mirroring paths (#198), and main here already picked up the corrected nested-path comment via the sync -- this branch's stale flat-path version would have reverted it on merge. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016wPHfMEiz9Bjau7twwZybf
…w-multi-repo-ljbuw0
…o-ljbuw0' into claude/review-workflow-multi-repo-ljbuw0
…w-multi-repo-ljbuw0 # Conflicts: # plugins/claude-code/review-utils/.claude-plugin/plugin.json
It wasn't covered by nsheaps/.github's sync-files.yml, so it silently went stale relative to the canonical template multiple times (most recently #324). Point docs directly at the nsheaps/.github source instead of maintaining a second copy. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016wPHfMEiz9Bjau7twwZybf
…w-multi-repo-ljbuw0 # Conflicts: # plugins/claude-code/review-utils/.claude-plugin/plugin.json
We pin the exact version in mise.toml for the native-binary postinstall fix, but renovate should still open bump PRs for it -- don't disable it at the renovate-config level. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016wPHfMEiz9Bjau7twwZybf
![henry-nsheaps[bot]](https://avatars.githubusercontent.com/in/2371206?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review summary
Doc-only cleanup that resolves a real drift-liability: this repo's templates/dispatch-review.yaml was a hand-maintained copy of a file whose canonical source lives (and is sync-distributed from) nsheaps/.github, and it wasn't covered by that repo's sync-files.yml. Removing the mirror + repointing docs at the canonical source is the right call, and the PR description is unusually clear about why the mirror kept re-diverging (four commits on this branch alone re-syncing stale Source: comments and gate expressions). The plugin.json bump to 0.2.0 is proportionate — README/spec pointers change externally, but no plugin-consumer API changes.
Confirmed on main after merge: templates/ no longer contains dispatch-review.yaml (README.md, agents/, dispatch-receiver-review.yaml, system-prompts/, teams/ remain). The Topology — receiver template row in specs/review-dispatch.md still points at templates/dispatch-receiver-review.yaml, which is correct — that file was not part of this PR's cleanup and still exists locally.
Findings are both P2 nits, non-blocking, and only relevant to a future doc pass since this is already merged.
Detail
Strengths
- Root-cause fix, not a patch. Prior branches on this line kept re-syncing the mirror. This PR deletes the mirror instead of fixing it again — removes the class of drift, not one instance. Matches the repo's "fix root causes" posture in
.claude/rules/. - Deletion is safe. The mirror was documentation-only; the actually-deployed
.github/workflows/dispatch-review.yamlon consumer repos is sync-managed fromnsheaps/.github, so no runtime path breaks. - Version bump is honest.
0.1.10 → 0.2.0— minor bump for a docs-plus-file-removal change to a 0.x plugin. Not major (no consumer API change), not patch (a file that consumers might have externally referenced is gone). - PR description carries the archaeology. Anyone auditing the branch later gets the full "why does this exist" without spelunking through 25 commits.
Improvements suggested (both P2)
README.md:28— the "Consumer setup" body is one ~120-word run-on paragraph doing four jobs. Suggested split into a short paragraph + bulleted breakdown so the "where's the template?" answer is scannable. inline suggestionspecs/review-dispatch.md:42—Trigger eventsrow references the same canonical file as the row above but as a bare code fragment. Hyperlink it for consistency. inline suggestion
Critical issues
None.
Follow-ups (out of scope for this PR)
- The
dispatch-receiver-review.yamltemplate still lives under this repo'stemplates/and isn't covered bynsheaps/.github's sync — same drift-liability shape as the file this PR just removed. Worth deciding whether to migrate it to the same sync pipeline or explicitly document why it stays local. - The
Topology — receiver templaterow in the spec table also links to the local copy — will need the same treatment if/when the receiver template moves.
Recommendation
COMMENT. No blocking issues, PR is already merged, findings are polish-only.
Scoring rationale
- Correctness: removes a documented drift source; deletion is a strict cleanup, no runtime impact. ✅
- Maintainability: improved — fewer places to keep in sync. ✅
- Documentation accuracy: improved (stale snippet gone) but the new prose is dense (see nit 1). 🟡
- PR description accuracy: verified against the diff and post-merge tree state — matches. ✅
- Convention adherence: aligns with
.claude/rules/*.mdposture on root-cause fixes. ✅
| pull-requests: write | ||
| ``` | ||
| Per-repo `.github/workflows/dispatch-review.yaml` is a thin consumer-side file distributed from `nsheaps/.github`'s `ansible/templates/.github/workflows/dispatch-review.yaml` (source layout there mirrors each file's destination, nested under `ansible/templates/` — see that repo's `docs/file-sync.md`) — see `ansible/config/sync-files.yml` there for the exact `managed_repos` membership (currently every managed repo, `nsheaps/.github` included — it's a normal sync target of its own template since it takes real PRs too). `nsheaps/.github` is the sole source of truth — there is no mirror of this file in `nsheaps/agents`; a prior copy under `templates/` drifted out of sync more than once and was removed for that reason. (A separate, unrelated file, `pr-status-dispatch.yaml`, is distributed by the same sync system but pings `nsheaps/.org`'s PR status digest — a different mechanism, not covered here.) | ||
|
|
Contributor
There was a problem hiding this comment.
🟡 nit · P2 · readability
This is a single ~120-word sentence-tangle with three parentheticals and an em-dash chain, doing four separate jobs (where the template lives, path-layout note, membership pointer, why the local mirror is gone, and an aside about pr-status-dispatch.yaml). Splitting into a short paragraph + bullets makes the operational info scannable and lets a reader who just wants "where's the template?" find it in one line:
Suggested change
| Per-repo `.github/workflows/dispatch-review.yaml` is a thin consumer-side file distributed from [`nsheaps/.github`'s `ansible/templates/.github/workflows/dispatch-review.yaml`](https://github.com/nsheaps/.github/blob/main/ansible/templates/.github/workflows/dispatch-review.yaml) — the sole source of truth (no mirror in this repo). | |
| - **Layout:** `nsheaps/.github` mirrors each synced file's destination path nested under `ansible/templates/` — see that repo's `docs/file-sync.md`. | |
| - **Membership:** see [`ansible/config/sync-files.yml`](https://github.com/nsheaps/.github/blob/main/ansible/config/sync-files.yml) there for the exact `managed_repos` list (currently every managed repo, `nsheaps/.github` included — it's a normal sync target of its own template since it takes real PRs too). | |
| - **Prior mirror:** a copy under this repo's `templates/` drifted out of sync more than once and was removed. | |
| - **Related but out of scope:** `pr-status-dispatch.yaml` is distributed by the same sync system but pings `nsheaps/.org`'s PR status digest — a different mechanism. |
Non-blocking (docs already merged); worth folding into the next README pass.1
Footnotes
-
🟡 P2 — nit / polish. Non-blocking style/readability issue. ↩
| | Topology — consumer template | [`nsheaps/.github` — `ansible/templates/.github/workflows/dispatch-review.yaml`](https://github.com/nsheaps/.github/blob/main/ansible/templates/.github/workflows/dispatch-review.yaml) (sole source of truth; no mirror in this repo) | | ||
| | Topology — receiver template | [`templates/dispatch-receiver-review.yaml`](../../../../templates/dispatch-receiver-review.yaml) | | ||
| | Topology — plugin composite | [`../actions/run-agent/action.yaml`](../actions/run-agent/action.yaml) | | ||
| | Topology — review-code skill | [`../skills/review-code/SKILL.md`](../skills/review-code/SKILL.md) | |
Contributor
There was a problem hiding this comment.
🟡 nit · P2 · consistency
The row above (Topology — consumer template) now links to the canonical file on nsheaps/.github — good. This Trigger events row still references the same file's on: block as a bare code fragment. Same treatment (hyperlink into nsheaps/.github) would make it navigable and keep the two rows consistent — reader clicks once, lands on the actual on: block.
Suggested change
| | Topology — review-code skill | [`../skills/review-code/SKILL.md`](../skills/review-code/SKILL.md) | | |
| | Trigger events | [`nsheaps/.github`'s `dispatch-review.yaml` template `on:` block](https://github.com/nsheaps/.github/blob/main/ansible/templates/.github/workflows/dispatch-review.yaml) | |
Non-blocking.1
Footnotes
-
🟡 P2 — nit / polish. Non-blocking consistency improvement. ↩
1 task
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
2 participants
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
nsheaps/.github#198(canonical template + gate fix + org-wide label provisioning + mirroredansible/templates/layout) merged and itssync-all.yamlautomation already pushed the correcteddispatch-review.yaml(and related synced workflow files) directly tomain. What's left in this PR is the content that sync doesn't cover:review-utils/README.md's stale "Consumer setup" snippet (pre-redesign secrets/trigger list), pointing it atnsheaps/.github'ssync-dispatch-workflowsskill and its current mirroredansible/templates/.github/workflows/dispatch-review.yamlsource path instead.templates/dispatch-review.yaml— this repo's own documentation-only mirror of the canonical template. It was never covered bynsheaps/.github'ssync-files.yml, so it drifted out of sync with the real gate multiple times over the course of this branch (staleSource:comments, missing gate fixes, and again via Add top-level permissions block to dispatch-review.yaml mirror #324 after this branch started). A manually-maintained copy outside the sync system's reach is a liability with no offsetting benefit, so it's gone; the spec and README now point straight atnsheaps/.github's canonical file instead of a second copy.Adds a— moved tosync-dispatch-workflowsskillnsheaps/.github/.claude/skills/instead, since it documents that repo's own mechanism rather than this plugin's review pipeline.plugin.jsonversion bump (0.2.0) kept for these doc fixes.Source:comment lines (apply-repo-settings.yaml,sync-main-to-edge.yaml) still referenced the old flatansible/templates/path from before.github's restructuring — merging as-is would have reverted those comments onmain. Re-synced them to matchmain's current (correct) nested-path comments.main's independent fix for the CI-wideclaude native binary not installedfailure (mise.tomlnow usesallow_buildsinstead of relying on install ordering, pinned at 2.1.128) — Renovate had bumped that pin from 2.1.128 to 2.1.205 across chore(deps): update dependency npm:@anthropic-ai/claude-code to v2.1.138 #229-chore(deps): update dependency npm:@anthropic-ai/claude-code to v2.1.204 #312, past the version where the native-binary postinstall breaks, and CI'stestjob had been red on every commit tomainsince. (Renovate keeps managing bumps to this pin going forward — that's intentional; only the broken pin itself needed fixing.)Diffed against
main: onlyplugin.json(version bump),README.md, andspecs/review-dispatch.mdremain —dispatch-review.yaml,mise.toml, and all other synced/shared files are byte-identical tomain(confirmed after eachmainmerge in this branch's history), andtemplates/dispatch-review.yamlis deleted rather than diverging.Context
Companion PRs
nsheaps/homebrew-devsetup(#686) andnsheaps/dotfiles(#37) merged;nsheaps/dotfiles(#35),nsheaps/claude-utils(#434),nsheaps/op-exec(#49),nsheaps/.org(#26) were closed as superseded once their branches showed zero diff againstmainpost-sync.Test plan
python3 -c "import yaml; yaml.safe_load(...)"on the modified workflow files — valid,if:expression reads correctlyREADME.md's corrected snippet no longer contradicts the canonical template or references the moved skill's old locationdispatch-review.yaml,apply-repo-settings.yaml,sync-main-to-edge.yaml,mise.toml,renovate.jsonare byte-identical tomainafter each rebase/mergetemplates/dispatch-review.yamloutside historical/archival docs (design plan doc left untouched — it's a dated snapshot, not live documentation)maincommit due to the claude-code native-binary issue above; expect green now thatmain's fix is merged in)