Skip to content

Fleet pr-review callers pin legacy bare pr-review/stable and are unmanaged — no pr-review release can reach the fleet #1125

Closed

Description

@don-petry

Summary

No pr-review release can reach the fleet through the normal path. Every fleet pr-review caller pins the legacy bare channel @pr-review/stable. Promotion moves only the major-scoped pr-review/v1-* channels. There is no standards/workflows/pr-review.yml template and pr-review.yml is not in DEPLOYABLE_WORKFLOWS, so the sweep never re-pins these callers.

This issue adds the template and makes the sweep migrate the existing callers.

Maintainer decision (2026-10-10): the canonical caller shape

The earlier version of this issue told dev-lead to stop if the callers differed. They do (dev-lead's report of 2026-09-13, in the comments). That question is now decided: do not stop, and do not re-derive a shape. Use the one below.

Why one shape is safe: most of the per-repo differences duplicate what the engine (petry-projects/.github-private/.github/workflows/pr-review.yml) already does itself.

Per-repo difference Decision Reason
Top-level scoped permissions vs {} plus job-level permissions: {} plus job-level Same three scopes either way; this is the org stub convention.
pr_url forwarded from client_payload (broodly) Not in the stub The engine reads client_payload.pr_url itself.
Skip check_suite with no PR (broodly) Not in the stub The engine's job guard already does it, and also requires the suite to have succeeded.
Stub-level concurrency: (TalkTerm, broodly) Not in the stub The engine owns concurrency: per PR and head SHA, cancel-in-progress: false on purpose (.github-private #534). broodly's cancel-in-progress: true cancels in-flight reviews, which the engine is designed not to do.
opened in the pull_request types (TalkTerm, bmad-bgreat-suite) Not in the stub The review runs when CI completes; opened only adds an early no-op run.
Dependabot skip guard (TalkTerm, bmad-bgreat-suite) In the stub, for every repo Dependabot events get no org secrets; skipping saves the runs. A person dispatching by hand on a Dependabot PR still runs.

Canonical content

Add the standard stub header above this (SOURCE OF TRUTH, AGENTS — READ BEFORE EDITING, as in standards/workflows/dev-lead.yml).

name: PR Review Agent

on:
  check_suite:
    types: [completed]
  pull_request_review:
    types: [submitted, dismissed]
  pull_request:
    types: [ready_for_review, reopened, synchronize]
  workflow_dispatch:
    inputs:
      pr_url:
        description: "Optional: review a single PR URL instead of enumerating"
        required: false
        type: string
      dry_run:
        description: "If true, never submit reviews or comments"
        required: false
        default: "false"
        type: string
      force_review:
        description: "If true, bypass idempotency and re-review at the same head SHA"
        required: false
        default: "false"
        type: string
  repository_dispatch:
    types: [pr-review-mention]

permissions: {}

jobs:
  review:
    if: ${{ github.actor != 'dependabot[bot]' }}
    permissions:
      contents: read
      pull-requests: write
      checks: read
    uses: petry-projects/.github-private/.github/workflows/pr-review.yml@pr-review/v1-stable  # NOSONAR(githubactions:S7637) first-party channel ref
    with:
      agent_ref: pr-review/v1-stable
      pr_url: ${{ inputs.pr_url || '' }}
      dry_run: ${{ inputs.dry_run || '' }}
      force_review: ${{ inputs.force_review || '' }}
    secrets: inherit  # NOSONAR(githubactions:S7635) first-party trusted reusable

Keep the workflow name: and the job id review exactly as shown; check names depend on them.

State on 2026-10-10

Repo Caller today Ring tier for pr-review
TalkTerm, bmad-bgreat-suite .github/workflows/pr-review.yml pinned pr-review/stable ring1
broodly, google-app-scripts, ContentTwin .github/workflows/pr-review.yml pinned pr-review/stable stable
markets No stub file. The pin lives in the pr-review job of .github/workflows/agent-ingress.yml. Out of scope here, see below
.github-private pr-review.yml there is the engine; its caller is pr-review-trigger.yml on pr-review/v1-next host
.github, incubator, broodminder-data, empower-personal-dashboard, repo-template No pr-review caller —

pr-review/stable, pr-review/v1-stable, pr-review/v1-ring1 and pr-review/v1-ring0 all resolve to commit f6f2e384, so the re-pin changes no build today.

Acceptance criteria

  1. Template. standards/workflows/pr-review.yml exists with the canonical content above and the standard header.

  2. Deploy, existing callers only. pr-review.yml is in DEPLOYABLE_WORKFLOWS in a replace-if-present mode with two explicit, tested guards in scripts/deploy-standard-workflows.sh:

    • never target petry-projects/.github-private (its pr-review.yml is the engine);
    • never add the file to a repo that has no .github/workflows/pr-review.yml today.

    Both pin lines (uses: and agent_ref:) are emitted in lockstep from the ring model: pr-review/v1-ring1 for TalkTerm and bmad-bgreat-suite, pr-review/v1-stable for the other three.

  3. Compliance looks at the body, not just the pin. A caller that already carries the right pin but a different body (for example a leftover concurrency: block) is drift and is replaced. A caller on the bare pr-review/stable is drift. The engine's filename is pr-review.yml, not *-reusable.yml, so today's is_pin_compliant would fall through to a substring match on the uses: line; see Standards sweep reports a drifted initiative-driver.yml as compliant — the template gained a uses: step, so the stub is no longer compared verbatim (repo-template still on ref: v1) #1277 for the same blind spot on initiative-driver.yml.

  4. Tests and lint. Tests cover: the host engine file is never targeted; a repo with no caller is never seeded; an existing caller is replaced with the template body and both pin lines on its tier channel; a second run is a no-op. shellcheck --severity=warning -x is clean and the tests run in the existing bats job.

Dev notes

Out of scope

  • markets. Its pr-review pin is inside agent-ingress.yml (ADR-0007 collapse), where a stub template cannot reach. It is handled as a pin-only edit in a separate issue in petry-projects/markets.
  • Promoting any pr-review release, or moving any tag.

References

Activity

  1. don-petry commented on Sep 13, 2026

    @don-petry
    ContributorAuthor

    Dev-Lead: blocked at AC #1 — fleet pr-review.yml callers differ materially (stop-and-list, not fabricate)

    Per the issue, AC #1 is a precondition ("First diff every existing fleet pr-review.yml caller … If they differ materially, stop and list the differences in the PR description instead of choosing one"). I ran the two precondition checks (AC #1 diff and AC #5 SHA check) before touching any source file. Result: AC #5 passes, but AC #1 blocks. No template was derived and no deploy/audit code was changed — see rationale at the end.

    AC #5 — behaviour-neutral check: PASS (both channels resolve to the same commit)

    Live on petry-projects/.github-private, 2026-09-13:

    channel tag tag object type resolves to commit
    pr-review/stable annotated tag (pr-review/v1.7.0, tag-object 8e00cde62d65b5be7df7f0b5a3d21be71dd27aef) ceab48a1c64d1e06a87d41ea5cf590c8e6a780bf
    pr-review/v1-stable lightweight tag ceab48a1c64d1e06a87d41ea5cf590c8e6a780bf

    Both dereference to ceab48a1 — migrating pins @pr-review/stable → @pr-review/v1-stable would be behaviour-neutral at deploy time, as the issue anticipated. (Note: git/ref/tags returns the annotated tag object SHA 8e00cde6… for the bare channel; that must be dereferenced through the tag object — it points at pr-review/v1.7.0 → commit ceab48a1. A naïve .object.sha comparison would falsely report divergence.)

    AC #1 — derive the template: BLOCKED (callers are not identical apart from pin lines)

    All six callers pin the legacy bare @pr-review/stable / agent_ref: pr-review/stable (confirming the issue's premise), but they diverge on multiple behavioural axes, not just pin lines or comment prose:

    axis TalkTerm bmad-bgreat-suite markets broodly google-app-scripts ContentTwin
    pull_request opened trigger yes yes no no no no
    top-level permissions: {} {} scoped r/w scoped r/w {} scoped r/w
    concurrency: block per-head-SHA, cancel-in-progress: false absent per-PR-number, cancel-in-progress: true per-PR-number, cancel-in-progress: true absent absent
    Dependabot/no-PR skip guard (if:) github.actor != 'dependabot[bot]' !startsWith(head.ref,'dependabot/') check_suite null-PR guard check_suite null-PR guard none none
    pr_url forward inputs.pr_url inputs.pr_url inputs.pr_url inputs.pr_url || client_payload.pr_url inputs.pr_url inputs.pr_url

    No two callers are identical, and the divergences are behavioural (which events trigger a review, how concurrent runs collapse, whether/how Dependabot runs are skipped, and the effective permission surface). There is no shared content "apart from the pin lines" from which to derive a single canonical standards/workflows/pr-review.yml. Choosing one variant would silently overwrite five repos' tuned callers — a breaking change requiring human approval per AGENTS.md.

    Other premises verified (for the record)

    Why no source changed

    AC #1 is the first, gating criterion and it resolves to its stop branch. Everything downstream depends on a single agreed template:

    Building any of these now would mislead about a migration that cannot proceed.

    Recommended next step (needs a human/PM decision)

    Reconcile the six fleet callers to one canonical shape first — explicitly deciding, per behavioural axis above:

    1. include the opened trigger or not;
    2. one concurrency strategy (per-head-SHA vs per-PR-number; cancel-in-progress true/false) applied fleet-wide;
    3. one Dependabot/no-PR skip guard;
    4. the canonical top-level/job permission surface;
    5. whether client_payload.pr_url forwarding is standard.

    Once the callers agree (or a canonical shape is chosen by a human), standards/workflows/pr-review.yml can be derived from that shared content (pinned @pr-review/v1-stable in lockstep with agent_ref: pr-review/v1-stable), and AC #2–#7 become implementable. Until then, per AC #1, I am not choosing a variant.

  2. don-petry commented on Oct 10, 2026

    @don-petry
    ContributorAuthor

    New instance for this issue: after the ADR-0007 collapse, markets pins pr-review.yml@pr-review/stable inside .github/workflows/agent-ingress.yml (job pr-review), no longer in a per-role stub file. Any fleet repin tool that looks for pr-review.yml stubs will not find it in a collapsed repo, so the repin has to understand the ingress form (per-job uses:), not just stub files. It carries a # DEFECTIVE PIN marker pointing at this class of problem.

  3. don-petry commented on Oct 10, 2026

    @don-petry
    ContributorAuthor

    Dev-Lead Implementation Plan

    Issue: #1125 — Fleet pr-review callers pin legacy bare pr-review/stable and are unmanaged — no pr-review release can reach the fleet

    Pre-flight tag check (petry-projects/.github-private, 2026-10-10)

    Tag Commit
    pr-review/stable f6f2e384d6bf2f360acaf44eef0324632368394f
    pr-review/v1-stable f6f2e384d6bf2f360acaf44eef0324632368394f
    pr-review/v1-ring1 f6f2e384d6bf2f360acaf44eef0324632368394f

    The two fleet tiers (ring1: TalkTerm, bmad-bgreat-suite; stable: broodly, google-app-scripts, ContentTwin) match pr-review/stable, so the re-pin changes no build. (pr-review/v1-ring0 has moved to 71920a24, but no fleet caller is on ring0.)

    Scope

    Add the canonical standards/workflows/pr-review.yml template and make the deploy sweep replace existing fleet callers (and only those) with the template body, re-pinned to each repo's tier channel.

    Implementation Checklist

    • standards/workflows/pr-review.yml: canonical content from the issue plus the standard SOURCE OF TRUTH / AGENTS — READ BEFORE EDITING header
    • scripts/lib/ring-pins.sh: ring_repin_uses matches the registry's reusable filename (ring_reusable_file), so a grandfathered pr-review.yml@ pin is rewritten in lockstep with agent_ref:
    • scripts/deploy-standard-workflows.sh:
      • add pr-review.yml to DEPLOYABLE_WORKFLOWS
      • new REPLACE_IF_PRESENT_WORKFLOWS mode with two explicit guards: never target .github-private (engine host), and never seed a repo without an existing caller
      • reusable_base_of / is_pin_compliant recognise the grandfathered reusable filename from the registry, not just *-reusable.yml
      • compliance for replace-if-present = the existing file equals the template body rendered at the repo's tier channel (so a leftover concurrency: block, or the bare pr-review/stable, is drift)
    • test/scripts/standards-templates/vform-pins.bats: ref_of helper handles the grandfathered pr-review.yml@ filename (helper only; expectations unchanged)

    Tests to Write

    • test/scripts/deploy-standard-workflows/pr-review-replace-if-present.bats: the engine host is never targeted, even with --force; a repo with no caller is never seeded; a bare pr-review/stable caller is replaced (ring1 → v1-ring1, stable → v1-stable); a right-pin/wrong-body caller is drift; the rendered body equals the template with both pin lines on the tier channel; a second run is a no-op
    • test/scripts/lib/ring-pins.bats: ring_repin_uses rewrites the grandfathered pr-review.yml@ and agent_ref together

    Test Command

    bats test/scripts/deploy-standard-workflows test/scripts/lib/ring-pins.bats test/scripts/standards-templates + bash .dev-lead/scripts/dev-lead-lint.sh

  4. don-petry commented on Oct 10, 2026

    @don-petry
    ContributorAuthor

    Dev-Lead: Implementation Complete — PR #1279

    The implementation for issue #1125 is durable: commits are pushed and a pull request is open. This record is backed by verifiable artifacts you can confirm in one click.

    Review happens on the PR. Acceptance criteria and test results are evidenced by the PR's diff and CI checks — not asserted here. If a later stage fails, this claim is retracted in place.

  5. added a commit that references this issue on Oct 10, 2026
  6. added a commit that references this issue on Oct 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugBug reportsdev-leadFor dev-lead agent pickup

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions