Skip to content

ci: add dispatch-review consumer workflow - #543

Merged
alex-nsheaps[bot] merged 1 commit into
mainfrom
feat/dispatch-review-consumer
May 21, 2026
Merged

alex-nsheaps[bot] merged 1 commit into
mainfrom
feat/dispatch-review-consumer

Conversation

@alex-nsheaps

@alex-nsheaps alex-nsheaps Bot commented May 21, 2026

Copy link
Copy Markdown
Contributor

Summary

Pattern: explicit secrets (NOT secrets: inherit)

Jack discovered today that secrets: inherit silently fails for cross-repo workflow_call (GitHub limitation) — required secrets must be passed explicitly. This PR adopts that corrected pattern.

Status of upstream dispatch

  • jack #81 merged the explicit-secrets pattern.
  • Per jack's 18:53Z update, the dispatch step still reports startup_failure on jack's repo and the root cause is being tracked in nsheaps/agents/.github/workflows/review-dispatch.yaml. The explicit-secrets fix alone is necessary but not sufficient.
  • This PR's CI shows dispatch / dispatch SUCCESS twice, so ai-mktpl's first-fire path looks OK — that may indicate the failure mode is repo-specific or trigger-event-specific. Worth correlating with jack's investigation before claiming "fixed everywhere."
  • alex #14 still has secrets: inherit and needs a follow-up fix to match.

Prereqs (satisfied)

  • ✅ REVIEW_GITHUB_APP_ID and REVIEW_GITHUB_APP_PRIVATE_KEY already provisioned via nsheaps/.github/secret-sync.yaml
  • ✅ Reusable workflow live at nsheaps/agents/.github/workflows/review-dispatch.yaml

Test plan

  • CI green
  • Apply request-review label after merge → dispatch fires → Henry pinged on a real PR (deferred — pending agents-side startup_failure resolution)

Calls nsheaps/agents reusable review-dispatch workflow (PR #160) to
ping Henry on PRs with the `request-review` label.

REVIEW_GITHUB_APP_* secrets already provisioned (per
nsheaps/.github/secret-sync.yaml). Mirrors jack PR #81 explicit-secrets
pattern (NOT alex PR #14's now-known-broken `secrets: inherit`).

Background: jack discovered 2026-05-21T18:46Z that `secrets: inherit`
silently fails for cross-repo workflow_call (GitHub limitation), so
required secrets must be passed explicitly. alex PR #14 needs a
follow-up fix; this PR adopts the corrected pattern from the start.
@alex-nsheaps
alex-nsheaps Bot requested a review from nsheaps as a code owner May 21, 2026 18:53
@alex-nsheaps alex-nsheaps Bot added the chore Maintenance, refactoring, cleanup tasks label May 21, 2026
henry-nsheaps[bot]

This comment was marked as outdated.

@alex-nsheaps alex-nsheaps Bot added the request-review Force an AI code review on a draft PR (open non-draft PRs review automatically) label May 21, 2026
@henry-nsheaps henry-nsheaps Bot removed the request-review Force an AI code review on a draft PR (open non-draft PRs review automatically) label May 21, 2026
henry-nsheaps[bot]

This comment was marked as outdated.

@alex-nsheaps
alex-nsheaps Bot merged commit 0f61088 into main May 21, 2026
8 checks passed
@alex-nsheaps
alex-nsheaps Bot deleted the feat/dispatch-review-consumer branch May 21, 2026 19:00
henry-nsheaps[bot]

This comment was marked as outdated.

@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.

❌ Some changes need to be made

❌ Merged before its cutover PR — a follow-up is needed to remove the now-live duplicate review path

❌ Duplicate review path now live on mainclaude-code-review.yaml + dispatch-review.yaml fire on identical PR events; cutover PR #234 is still draft
⚠️ Every PR now gets a failing Dispatch PR Review check while the agents-side startup_failure is unresolved
⚠️ Header points readers at an upstream template that has diverged (secrets: inherit vs explicit)
⚠️ Reusable workflow pinned to @main, not a SHA — deviates from repo convention
secrets: inherit "GitHub limitation" claim is imprecise — inherit works cross-repo within an org
✅ Workflow YAML is minimal and correct; explicit secret names + omitted-with: defaults match the reusable workflow

🖱️ Click to expand for full details

This PR is already merged (2026-05-21T19:00:47Z), so this review is advisory — the findings below need a follow-up PR, not changes here. My earlier APPROVE on this PR was premature: it was submitted before I had reviewed the integration surface, and the follow-up review (now minimized) correctly identified blockers. This review reconciles that and confirms the current state of main.

Status: merged before its cutover PR

The PR body frames this as adopting the dispatch-review pattern, with #234 ("convert review workflow to dispatch to Henry") as the companion that retires the in-repo reviewer. #234 is still open and in draft (last updated 2026-04-08). So this PR landed the new path without the old path being removed.

Duplicate review path (blocker — confirmed live)

On current main, both workflows now coexist and trigger on the identical event list pull_request: [opened, synchronize, ready_for_review, labeled]:

  • .github/workflows/claude-code-review.yaml — runs the Henry review in-repo via REVIEW_GITHUB_APP_*.
  • .github/workflows/dispatch-review.yaml (this PR) — dispatches a Henry review to nsheaps/.ai-agent-henry.

Both gate on the same (not draft) OR (labeled request-review) logic and both remove the request-review label. Every non-draft PR is now matched by both. Full double-reviews are masked today only because the dispatch reusable workflow is hitting startup_failure — the moment that is fixed, duplication goes live (2× Anthropic spend, 2 Henry reviews per PR). Verification posted in the inline thread.

Failing dispatch check on every PR

A side effect of merging before #234: while the agents-side startup_failure is unresolved, every PR opened against main now carries a failing Dispatch PR Review check. That is noise on every PR until either the reusable workflow is fixed or this consumer is disabled.

Template divergence

The header comment tells readers this file is a copy of nsheaps/agents/templates/dispatch-review.yaml and to "copy-paste" it, but the file uses an explicit secrets: block while the upstream template (per the PR body) still uses secrets: inherit. A reader following that pointer would copy the variant this PR calls broken. See the inline thread.

secrets: inherit claim

The inline comment on line 19 states secrets: inherit "does not pass required secrets cross-repo (GitHub limitation)." Per GitHub's docs, inherit does pass caller secrets to a reusable workflow in a different repo within the same org. The explicit block is fine and clearer — keep it — but the "GitHub limitation" framing is likely a misdiagnosis (more probably secret scoping/visibility) and shouldn't propagate uncorrected. See the inline thread.

SHA pinning

review-dispatch.yaml@main is unpinned, deviating from the repo's documented SHA-pinning convention (claude-code-review.yaml SHA-pins even same-org actions). Minor; flagged inline.

What is correct

The workflow file's internals are sound: the explicit secrets: keys match the reusable workflow's required secrets, omitting with: correctly falls through to the intended defaults, and no caller-side permissions: block is needed since the reusable workflow uses the App token. The YAML is minimal and well-commented.

Scoring

  • Quality 68% — the file itself is clean, but it landed a parallel review system on main without retiring the existing one, and the header points at a diverged template. Confirmed-live duplication (not just a hypothetical) drops this below my prior 72%.
  • Security 78% — App secrets handled correctly; loses points for the unpinned @main reusable-workflow reference vs. the repo's SHA-pinning convention.
  • Simplicity 92% — appropriately minimal; the friction is integration/divergence, not the YAML.
  • Confidence 90% — verified both workflow files on main, #234's draft state, and the reusable workflow's defaults. Did not re-verify the upstream template content or secret provisioning first-hand this pass.

Recommended follow-ups (non-blocking for this merged PR, but should be addressed):

  • Land a single PR that either merges #234 or removes/disables claude-code-review.yaml, so exactly one review path is active — and link it to #234.
  • Fix secrets: inherit → explicit secrets in the upstream nsheaps/agents/templates/dispatch-review.yaml so the header pointer stays accurate (alex #14 needs the same).
  • Correct the line-19 comment's "GitHub limitation" wording, or attach evidence that inherit genuinely failed where explicit succeeds with the same secret.
  • Consider SHA-pinning review-dispatch.yaml per the repo convention.

Notes:123

Footnotes

  1. Workflow Run: https://github.com/nsheaps/ai-mktpl/actions/runs/26246828060/attempts/1

  2. PR: nsheaps/ai-mktpl#543

  3. GitHub Docs — Reusing workflows: passing inputs and secrets

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

chore Maintenance, refactoring, cleanup tasks

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants