Add add-pr-reviewer-to-repo agent skill#8
Conversation
Adds a new agent skill at .agents/skills/onboard-pr-review/SKILL.md that guides agents through setting up or upgrading repos to use the PR reviewer reusable workflow from docker/docker-agent-action. Covers: - How to choose between the 1-workflow (same-repo) and 2-workflow (fork) patterns - Complete workflow YAML for both patterns, sourced from review-pr/README.md - Trigger mode selection (Mode A vs Mode B) - VERSION placeholder guidance (current latest: v2.0.0) - Upgrade checklist for repos already using the workflow - Common mistakes and troubleshooting (OIDC, bot loops, artifact download, workflow_run matching)
Add an 'Agent skills' section documenting the .agents/skills/ directory and the new onboard-pr-review skill. Also add .agents/skills/ to the repo layout tree so agents know the directory exists and what it contains. Includes a usage hint: when asked to onboard a repo to the PR reviewer, load the skill before starting.
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The PR adds .agents/skills/onboard-pr-review/SKILL.md and updates AGENTS.md. The workflow YAML snippets in the skill file use correct syntax, proper permissions blocks, valid event triggers, and accurate action/workflow references consistent with review-pr/README.md. The @VERSION placeholder guidance is clear and actionable. The 1-workflow vs 2-workflow fork patterns, trigger mode selection (A/B), upgrade checklist, and troubleshooting section are internally consistent and align with the repo's conventions. No bugs introduced by this PR.
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
Diff reviewed: .agents/skills/onboard-pr-review/SKILL.md (new file, 270 lines) and AGENTS.md (updated docs).
Summary: The new skill guide is accurate and well-structured. Workflow YAML examples match the source of truth in review-pr/README.md character-for-character, including the actions/upload-artifact SHA pin, bot-loop filter conditions, workflow_run trigger name ("PR Review - Trigger"), required permissions blocks, and the trigger-run-id expression. The @VERSION substitution instruction is unambiguous. The stated latest release (v2.0.0) matches the actual latest release. AGENTS.md additions are consistent with the new skill.
No bugs, incorrect instructions, or misleading guidance found in the introduced changes.
Add a prominent 'How the reviewer is triggered' section near the top of the skill explaining that the primary UX is adding docker-agent as a reviewer in the PR sidebar (fires review_requested), re-requesting to re-trigger, and that /review is deprecated. Includes a note to communicate this to contributors when onboarding a repo.
Add a 'What you DON'T need to add' section explaining that the reusable workflow handles bot filtering, org membership checks, PR/issue disambiguation, draft PR skipping, and concurrent review locking internally. Callers only need to choose the 1-workflow vs 2-workflow pattern. Also notes that caller-side author_association/bot filters are a valid Actions-minutes optimization but not required for correctness or safety.
Expand the existing 'Built-in defense-in-depth' callout into a proper 'What you don't need to add' section with a reference table covering: bot comment filtering, org membership/auth, PR vs issue disambiguation, draft PR skipping, and concurrent review locking. Clarifies that the canonical YAML examples are the complete recommended setup, and that caller-side author_association/bot filters are an optional Actions-minutes optimization, not a correctness requirement.
Rename .agents/skills/onboard-pr-review/ to .agents/skills/add-pr-reviewer-to-repo/ and update all references: - SKILL.md frontmatter name field - Repo layout tree in AGENTS.md - Agent skills table and usage hint in AGENTS.md
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The onboard-pr-review skill document is accurate, well-structured, and internally consistent. Reviewed the full diff (312 lines, SKILL.md only — AGENTS.md and README.md changes were pre-filtered as low-risk).
What was checked:
- Workflow YAML snippets (1-workflow and 2-workflow patterns): triggers, permissions, and job structure are correct
@VERSIONplaceholder guidance: the replacement instruction (replace@VERSION→@v2.0.0) is unambiguous and produces validuses:referencesworkflow_runname matching: correctly documented — the string"PR Review - Trigger"in the handler'sworkflows:array matches thename:field in the trigger workflow exactly- Bot loop prevention: all five filter conditions (
docker-agent,docker-agent[bot],Bottype, both HTML markers) are documented correctly - OIDC permissions:
id-token: writeis present on the job-levelpermissionsblock (not just top-level) in all examples - Fork pattern artifact handling:
actions/upload-artifactSHA pin (043fb46d...@ v7.0.1) matchesreview-pr/README.md;actions: readpermission is present in the handler workflow - The note about the "What you DON'T need to add" section correctly reflects the reusable workflow's built-in protections per AGENTS.md
No bugs or correctness issues found.
Add a concrete procedure for validating all docker-org repos that consume the PR reviewer reusable workflow: - Step 1: discovery via gh search code across the org - Step 2: per-repo validation checklist (version, fork pattern, permissions, trigger types, unnecessary if: guards, fork-specific wiring) - Step 3: summary report format grouped by ✅ /⚠️ / ❌ status - Step 4: remediation guidance (one PR per repo, reference skill sections, don't fix cosmetic-only issues in isolation)
The save-context job in pr-review-trigger.yml examples had an if: condition filtering docker-agent comments and bot users. This is inconsistent with the 'you don't need caller-side guards' guidance since the reusable workflow handles all filtering internally. - review-pr/README.md: remove if: block from save-context job - SKILL.md section 4b: same removal from the trigger YAML example - SKILL.md section 7 audit checklist: remove the carve-out note that called these filters 'required' in the trigger workflow; they are now listed as unnecessary everywhere
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
4 documentation contradictions found in the new that could cause agents following it to produce incorrect or conflicting workflow configurations. All findings are in new lines introduced by this PR. See inline comments for details.
| jobs: | ||
| review: | ||
| if: | | ||
| (github.event_name == 'issue_comment' && |
There was a problem hiding this comment.
[MEDIUM] Contradiction: canonical Section 4b YAML includes bot-login filters that the "built-in protections" section says not to add
The "What you DON'T need to add" section (§2) explicitly tells readers: "Do not add caller-side guards for these — they create maintenance burden without improving correctness or safety." Bot comment filtering is listed as one of those built-in protections.
Yet the canonical pr-review.yml example in §4b includes a full if: condition with bot-login and marker filters for the issue_comment path — without labelling them as optional or explaining the discrepancy.
An agent (or human) following the canonical YAML will copy the bot filters; an agent following the prose will omit them. Both behaviours are consistent with the document, which sends conflicting authoritative signals.
Suggested fix: Either (a) remove the bot-filter if: from the §4b canonical YAML entirely (since the reusable workflow handles them internally), or (b) add a comment inside the YAML block explicitly marking the if: condition as an optional Actions-minutes optimization, so readers understand it is not required for correctness.
| - [ ] **Version/tag is current** — compare the `@VERSION` in `uses:` against the latest release from `gh release list --repo docker/docker-agent-action --limit 1`. Update if behind. | ||
| - [ ] **All required permissions are present** — `contents: read`, `pull-requests: write`, `issues: write`, `id-token: write`. Missing any of these causes silent failures or OIDC auth errors. | ||
| - [ ] **`checks: write` is present** (optional but recommended) — without it the review won't appear as a check run on the PR. | ||
| - [ ] **Bot-filter `if` condition is correct** — the condition must filter out `docker-agent`, `docker-agent[bot]`, any `Bot` user type, and comments containing `<!-- docker-agent-review -->` or `<!-- docker-agent-review-reply -->`. A missing or incomplete filter causes infinite review loops. |
There was a problem hiding this comment.
[HIGH] Upgrade Checklist bot-filter item will cause agents to add unnecessary caller-side filters to correctly-configured setups
This checklist item states: "A missing or incomplete filter causes infinite review loops."
However, the §4a canonical YAML (1-workflow pattern) has no if: bot-filter guard at all — because the reusable workflow handles infinite-loop prevention internally. An agent processing this upgrade checklist would conclude that a §4a-style setup without a caller-side bot filter is broken and in need of fixing. It would then add bot-filter if: conditions that are not only unnecessary but could interfere with legitimate review triggers (e.g., an overly broad user.type != 'Bot' guard might block future bot reviewers).
The checklist item also contradicts the §2 table which explicitly says bot comment filtering is "handled internally" and callers should not add guards for it.
Suggested fix: Rewrite this checklist item to clarify it applies only to repos that have already added an optional caller-side if: guard (to verify its completeness), not as a requirement for all setups. Or remove the item entirely and rely on the internal protections documented in §2. Example rewrite: "(Optional) If your workflow has a caller-side if: guard for bot filtering, verify it filters all of: docker-agent, docker-agent[bot], Bot user type, <!-- docker-agent-review -->, <!-- docker-agent-review-reply -->."
|
|
||
| ### Infinite review loop | ||
|
|
||
| **Cause:** The `if` condition on the `save-context` job (trigger workflow) or the `review` job is not filtering bot comments. The agent posts a comment → that fires an `issue_comment` or `pull_request_review_comment` event → the workflow triggers again → repeat. |
There was a problem hiding this comment.
[MEDIUM] Troubleshooting guide incorrectly attributes infinite loop risk to the save-context job in the trigger workflow
The troubleshooting cause says: "The if condition on the save-context job (trigger workflow) or the review job is not filtering bot comments."
This is inaccurate. The trigger workflow (pr-review-trigger.yml) handles only pull_request and pull_request_review_comment events — not issue_comment. The save-context job in §4b's canonical trigger workflow has no if: guard (correctly so, since pull_request events are not triggered by bot comments in the review flow). Bot-posted issue_comment events go directly to pr-review.yml (the handler), where the review job's if: condition provides the guard.
An agent reading this troubleshooting entry could add an if: guard to the save-context job in the trigger workflow — the wrong place — and then wonder why reviews on fork PRs stop working (the trigger would fail to save context for legitimate PRs if the guard is badly formed).
Suggested fix: Remove "the save-context job (trigger workflow) or" from the Cause sentence. The infinite loop risk is only on the review job in the pr-review.yml handler, specifically for issue_comment events. The corrected cause: "The if condition on the review job is not filtering bot comments for issue_comment events."
| The reusable workflow handles all safety checks internally. Flag any of the following as unnecessary (safe to remove, not a correctness issue): | ||
|
|
||
| - [ ] `author_association` checks on the calling job | ||
| - [ ] Bot-login filters (`github.event.comment.user.login != 'docker-agent'`, etc.) on any job in the calling workflow |
There was a problem hiding this comment.
[MEDIUM] Section 7 audit checklist flags bot-login filters as "unnecessary" but Section 4b's canonical YAML includes them — self-defeating audit rule
The §7 audit checklist instructs auditors to flag bot-login filters (e.g., github.event.comment.user.login != 'docker-agent') as "unnecessary (safe to remove, not a correctness issue)" on any job in the calling workflow.
But §4b's canonical recommended pr-review.yml YAML for fork setups includes exactly those filters on the review job. An agent running a §7 audit against a repo that correctly followed §4b would flag its own recommended setup as needing remediation and open a PR to remove the filters.
The closing note ("Don't fix what isn't broken") partially mitigates this, but an agent reading the checklist sequentially would still log these as "
Suggested fix: Scope the "Unnecessary caller-side if: guards" checklist to the 1-workflow pattern only, or add an exception note: "Exception: the bot-login if: condition in the §4b fork-pattern pr-review.yml is expected and should remain — only flag additional or duplicate guards beyond what §4b recommends."
Adds a new agent skill and updates docs to clarify the built-in protections in the PR reviewer reusable workflow.
Changes
.agents/skills/add-pr-reviewer-to-repo/SKILL.md(new)A skill for agents setting up or upgrading repos to use
docker/docker-agent-action/.github/workflows/review-pr.yml. Contents:docker-agentas a reviewer in the PR sidebar (review_requestedevent); re-request to re-trigger;/reviewis deprecatedif:guards for bots, org membership, PR/issue disambiguation, draft skipping, concurrent locking); only the fork vs same-repo choice is the caller's responsibilityreview-pr/README.md:synchronize) vs Mode B (recommended default)@mainfor bleeding edge onlyworkflow_runname matchingworkflow_runnever firing, missing check runAGENTS.md.agents/skills/add-pr-reviewer-to-repo/to the repo layout treereview-pr/README.mdauthor_association/bot filters are an optional Actions-minutes optimization, not a correctness or safety requirement