Skip to content

[sync] Create .github/workflows/dispatch-review.yaml - #153

Closed
automation-nsheaps[bot] wants to merge 0 commit into
mainfrom
org-sync/-github-workflows-dispatch-review-yaml
Closed

automation-nsheaps[bot] wants to merge 0 commit into
mainfrom
org-sync/-github-workflows-dispatch-review-yaml

Conversation

@automation-nsheaps

Copy link
Copy Markdown
Contributor

Org-sync from nsheaps/.github

Direct push was blocked by branch protection. This PR applies the
central file update via the standard PR flow.

Path: .github/workflows/dispatch-review.yaml
Action: Create

Branch org-sync/-github-workflows-dispatch-review-yaml is stable — subsequent runs update this PR.

@henry-nsheaps henry-nsheaps Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

### ✅ Approve — routine org-sync of a vetted template; three P2 comment nits for the upstream template source

  • ✅ Byte-identical to sister sync PRs already merged in public-scratch, homebrew-devsetup, agents, ai-mktpl, brew-meta-formula, renovate-config, dotfiles — blob SHA f3cfd356….
  • ✅ Reusable workflow pin (nsheaps/agents@f54467d…) is a real commit (release v0.3.157), the called file exists and matches the input/secret contract this template calls into.
  • ✅ Minimal explicit permissions at both top-level and job-level — satisfies CKV2_GHA_1.
  • ✅ No conflict with existing workflows in this repo (check.yaml, pages.yaml, sync-main-to-edge.yaml, sync-plugin-specs.yaml).
  • ⚠️ Comment on the uses: line advertises @main rolling-ref semantics, but the ref is a frozen SHA — contradictory; see inline.
  • ⚠️ secrets: inherit is described as "GitHub limitation", but it works for same-org reusable workflows — see inline.
  • ❔ labeled trigger fires a dispatch on any non-draft label event — intentional per the design comment, worth confirming amplification is acceptable; see inline.

Click to expand for full details.

What this PR does

Adds .github/workflows/dispatch-review.yaml — a thin "gate" workflow that forwards PR events to the shared decider at nsheaps/agents/.github/workflows/review-dispatch.yaml@f54467d…. The decider posts a queued check-run on this repo's PR head SHA and fires a repository_dispatch to nsheaps/.ai-agent-henry, where the review actually runs under the reviewer App identity.

Verification I ran

  • Compared blob SHA on this branch (f3cfd356…) against synced copies in sister repos — identical. This is a vetted template rolled out org-wide, not a bespoke change.
  • Fetched the reusable workflow at the pinned SHA and confirmed:
    • Secret contract matches (AUTOMATION_GITHUB_APP_ID + AUTOMATION_GITHUB_APP_PRIVATE_KEY, both passed explicitly).
    • Optional target-repo input defaults to nsheaps/.ai-agent-henry (private but accessible), which has both dispatch-review.yaml and dispatch-receiver-review.yaml present — the receiver side is wired up.
  • Confirmed nsheaps/agents HEAD on main (3162250…, 2026-10-07) is newer than the pinned SHA (f54467d…, 2026-10-01, release v0.3.157), so the "rolling" claim in the comment is demonstrably not what's happening.
  • Checked for prior reviews / threads on this PR — none.

Why the three findings are all P2 and all upstream

Every synced copy of this template carries the same two comment issues (@main rolling-ref claim, secrets: inherit cross-repo claim) and the same labeled-trigger behavior. Fixing them here only would make this file drift from the other sync targets, which would then be overwritten by the next sync. The right place to fix is wherever the template source lives (likely nsheaps/.github CI automation or an internal template directory).

Score rationale

  • Quality 88 — well-commented, correct wiring, right permissions scope. Dinged for two comments that misrepresent the behavior they describe.
  • Security 92 — pinned action SHA, pinned reusable-workflow SHA, explicit (not inherited) secrets, minimal permissions, no run: steps that interpolate event payload. Nothing to flag; stays below 100 only because the review gate relies entirely on automation-nsheaps[bot] scope not being over-provisioned on the installed repos (out of scope for this PR to verify).
  • Simplicity 90 — ~30 lines of real config, rest is contextual comments explaining the design. Appropriate for a boilerplate dispatcher.
  • Confidence 92 — I verified the called workflow, the pinned SHA, the receiver file, and the blob equivalence with other synced copies. Not 100 because I did not run the gate end-to-end in a disposable test repo.

Follow-ups:

  • P2: Fix the @main rolling-ref comment vs. frozen-SHA pin mismatch in the upstream template.
  • P2: Rewrite the secrets: inherit "GitHub limitation" comment — same-org reusable workflows do inherit; the real reason to pass explicitly is auditability.
  • P2: Decide whether labeled events on non-draft PRs should always dispatch, or only when the label is request-review. Current form is simpler but amplifies on label-noisy repos.

References

Comment thread .github/workflows/dispatch-review.yaml Outdated
Comment on lines +70 to +73
secrets:
AUTOMATION_GITHUB_APP_ID: ${{ secrets.AUTOMATION_GITHUB_APP_ID }}
AUTOMATION_GITHUB_APP_PRIVATE_KEY: ${{ secrets.AUTOMATION_GITHUB_APP_PRIVATE_KEY }}
# Optional overrides (uncomment to use):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ P2 — comment contradicts the actual pin.

The comment advertises rolling-ref semantics ("any change merged to nsheaps/agents takes effect on the next PR event"), but the uses: ref is a frozen commit SHA (f54467d…, release v0.3.157 from 2026-10-01). Current nsheaps/agents main is 3162250… and this consumer will NOT pick it up until the sync bot re-runs.

The behavior is actually "stable, pinned via sync automation" — the opposite of what the comment claims. Suggest rewording to describe the real contract (sync bot resolves @main → SHA at sync time, so consumers get the SHA that was main at sync time, not live main).

Belongs upstream in the template source — every synced copy carries this mismatch.

Comment thread .github/workflows/dispatch-review.yaml Outdated
Comment on lines +73 to +76
# Optional overrides (uncomment to use):
# with:
# target-repo: nsheaps/.ai-agent-henry # default
# event-type: pr-review # default repository_dispatch event_type

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ P2 — the "GitHub limitation" rationale is incorrect.

secrets: inherit does pass across repos when the caller and the reusable workflow are in the same organization — both nsheaps/github-actions and nsheaps/agents are. See GitHub's reusable-workflow docs on jobs.<job_id>.secrets.inherit.

Explicit passing is still a reasonable choice (keeps the secret contract visible, limits blast radius if more secrets are added later), but the stated reason is wrong. Suggest replacing the comment with the real justification — e.g. "pass explicitly so the required-secret contract is auditable from this file".

Belongs upstream in the template source.

Comment thread .github/workflows/dispatch-review.yaml Outdated
Comment on lines +42 to +55
jobs:
review:
# Gate: review fires automatically on any OPEN, non-draft PR event
# (opened, reopened, synchronize, ready_for_review) -- no label needed.
# The `request-review` label only matters to FORCE a review on a DRAFT
# PR (apply the label while it's still a draft). `converted_to_draft`
# does NOT fire a review by itself -- a PR converted to draft is simply
# not reviewed until it's marked ready again or explicitly labeled. If
# you change the request label name, update the literal in the `==`
# comparison below.
if: |
github.event.pull_request.state == 'open' &&
(
github.event.pull_request.draft != true ||

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❔ P2 (informational) — dispatch amplification on noisy label traffic.

The if: short-circuits on draft != true, so on a non-draft PR any labeled event dispatches a review — not just labeled with request-review. If other automations (release tooling, triage bots, humans) add labels, each addition fires a new review. The comment on line 50 ("request-review label only matters to FORCE a review on a DRAFT PR") describes the design, so this appears intentional, but worth confirming the dispatch amplification is acceptable in high-label-traffic repos.

If you want to narrow this, you could gate the labeled branch to only request-review:

Suggested change
jobs:
review:
# Gate: review fires automatically on any OPEN, non-draft PR event
# (opened, reopened, synchronize, ready_for_review) -- no label needed.
# The `request-review` label only matters to FORCE a review on a DRAFT
# PR (apply the label while it's still a draft). `converted_to_draft`
# does NOT fire a review by itself -- a PR converted to draft is simply
# not reviewed until it's marked ready again or explicitly labeled. If
# you change the request label name, update the literal in the `==`
# comparison below.
if: |
github.event.pull_request.state == 'open' &&
(
github.event.pull_request.draft != true ||
if: |
github.event.pull_request.state == 'open' &&
(
(github.event.pull_request.draft != true && github.event.action != 'labeled') ||
(github.event.action == 'labeled' && github.event.label.name == 'request-review')
)

(Only suggest this if the amplification is actually a problem — the current form is simpler and the receiver-side concurrency group should coalesce duplicates anyway. Belongs upstream in the template source.)

@automation-nsheaps
automation-nsheaps Bot force-pushed the org-sync/-github-workflows-dispatch-review-yaml branch from 8bcf0c0 to 1e12939 Compare October 8, 2026 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants