Skip to content

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

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

automation-nsheaps[bot] wants to merge 1 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.

### ⚠️ Looks good overall — one P1 doc footgun worth fixing before this gets copy-pasted into more consumer repos

  • ⚠️ P1: commented event-type: pr-review example is not a valid upstream input; uncommenting it breaks the workflow with "Invalid input".
  • ❔ P2: comment claims @main = rolling updates, but the uses: ref is pinned to a SHA — mechanism mismatch.
  • ✅ Permissions correctly scoped at both top-level and job-level (satisfies checkov CKV2_GHA_1).
  • ✅ Reusable workflow pinned to a commit SHA — good supply-chain practice.
  • ✅ Secrets forwarded explicitly (correct — secrets: inherit does not cross-repo for uses: owner/repo/...@sha reusable calls).
  • ✅ Gate if: logic matches its documentation (open-and-not-draft, or labeled with request-review).
    Click to expand for full details

What this PR does

Adds the consumer-side gate .github/workflows/dispatch-review.yaml for the AI code review flow. On relevant PR events it delegates to the reusable workflow nsheaps/agents/.github/workflows/review-dispatch.yaml pinned at SHA 31622503be5de83437594476b86d3c500b4af7c2, which posts a pending check and fires a repository_dispatch to nsheaps/.ai-agent-henry where the review actually runs under the reviewer-identity App.

How I evaluated

  • Fetched the upstream reusable workflow at the exact pinned SHA via gh api repos/nsheaps/agents/contents/.github/workflows/review-dispatch.yaml?ref=31622503… and verified the workflow_call.inputs and workflow_call.secrets contracts.
  • Cross-checked permission scopes, secret names, and the uses: ref against upstream.
  • Traced the gate if: boolean for the five trigger types to confirm it matches the comment's claims.

Findings

⚠️ P1 — Commented example breaks the workflow if literally followed (lines 73–76)

The bottom-of-file override example references event-type: pr-review:

# with:
#   target-repo: nsheaps/.ai-agent-henry  # default
#   event-type: pr-review                 # default repository_dispatch event_type

But the upstream reusable workflow at 31622503 only declares one input:

# upstream workflow_call.inputs
target-repo:
  description: 'Target agent repo that runs the review (owner/name).'
  required: false
  default: 'nsheaps/.ai-agent-henry'
  type: string

The event-type sent on the repository_dispatch is hardcoded upstream to ${{ github.event_name }}/${{ github.event.action }} (e.g. pull_request/opened). It is not a configurable input. Uncommenting this block to tweak the event type will trip validation: "Invalid input, 'event-type' is not defined in the referenced workflow".

This matters because this file is a template meant to be copy-pasted into other consumer repos — the misleading example is likely to be acted on.

❔ P2 — "@main = rolling updates" comment contradicts the pinned SHA (lines 67–71)

# @main = rolling updates: any change merged to nsheaps/agents takes effect
# on the next PR event in repos using this template. This is intentional —
# operators who need pinned stability should replace @main with a commit SHA
# and update it in lock-step with plugin version bumps.
uses: nsheaps/agents/.github/workflows/review-dispatch.yaml@31622503be5de83437594476b86d3c500b4af7c2 # main

The ref is pinned to a SHA; @main only appears as a trailing comment. If org-sync rewrites this SHA on every nsheaps/agents main push, the practical effect is "rolling" but the mechanism is SHA bumping — not @main. If sync doesn't rewrite, this is just pinned, and the comment is wrong.

Either way, describe the actual mechanism so operators know how to opt into stability vs. rolling.

Checked / correct

  • Permissions: top-level contents: read / pull-requests: write / checks: write + identical job-level grant. Matches upstream job permissions. Satisfies checkov CKV2_GHA_1.
  • Secret forwarding: explicit (not secrets: inherit), which is required because reusable workflows called with owner/repo/...@sha cannot inherit cross-repo. Names match upstream workflow_call.secrets.
  • Supply-chain pinning: reusable workflow pinned to a commit SHA (not a floating ref).
  • Gate if: logic: state == 'open' AND (draft != true OR (action == 'labeled' AND label.name == 'request-review')). Correctly causes the review to fire on opened/reopened/synchronize/ready_for_review for non-drafts, and only labeled-with-request-review for drafts. Matches the inline explanation.
  • Separation of creds: comment correctly justifies using automation creds (routing) vs. reviewer-identity creds (receiver). No LLM-auth secrets leaked here.
  • File extension: .yaml matches the referenced path and the convention in the comment header.

Follow-ups:

  • P0: (none)
  • P1: Remove or correct the event-type: pr-review line in the commented override example (lines 73–76). Suggestion inline.
  • P2: Reconcile the "@main = rolling updates" comment with the actual pinned-SHA ref (lines 67–71).

References

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.

⚠️ P1 — commented example breaks the workflow if uncommented.

event-type is not a workflow_call input in the pinned reusable workflow (nsheaps/agents/.github/workflows/review-dispatch.yaml@31622503). Its only input is target-repo. The dispatched event-type is hardcoded upstream to ${{ github.event_name }}/${{ github.event.action }} (e.g. pull_request/opened), so a value like pr-review is never what gets sent on the wire.

If an operator follows this template literally and uncomments the block, the workflow fails validation with:

Invalid input, 'event-type' is not defined in the referenced workflow

Suggest dropping that line (or converting it to a prose note that the event-type is derived upstream, not configurable here):

Suggested change
# Optional overrides (uncomment to use):
# with:
# target-repo: nsheaps/.ai-agent-henry # default
# event-type: pr-review # default repository_dispatch event_type
# Optional overrides (uncomment to use):
# with:
# target-repo: nsheaps/.ai-agent-henry # default
# Note: the repository_dispatch event-type is derived upstream from
# `${{ github.event_name }}/${{ github.event.action }}` and is not
# a configurable input of this reusable workflow.

Comment on lines +67 to +71
# and update it in lock-step with plugin version bumps.
uses: nsheaps/agents/.github/workflows/review-dispatch.yaml@31622503be5de83437594476b86d3c500b4af7c2 # main
# secrets: inherit doesn't pass cross-repo (GitHub limitation).
secrets:
AUTOMATION_GITHUB_APP_ID: ${{ secrets.AUTOMATION_GITHUB_APP_ID }}

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 describes "@main = rolling updates" but the uses: line is pinned to a specific SHA (31622503…) with just a trailing # main marker. The two don't match: if org-sync bumps the SHA automatically on every nsheaps/agents main push, the behavior is effectively rolling but implemented via SHA rewrites — not @main. If sync does not rewrite the SHA, the comment is simply wrong and this is pinned stability.

Either way, the "@main" in the explanatory text no longer appears in the ref. Suggest describing the actual mechanism (e.g. "pinned to a SHA; org-sync bumps on every nsheaps/agents main push — override by replacing the SHA and dropping the # main marker"). Not blocking — doc-only.

This branch has not been deployed

No deployments
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