Add pr-review skill for reviewing remote GitHub PRs and GitLab MRs - #99
Add pr-review skill for reviewing remote GitHub PRs and GitLab MRs#99asafbennatan wants to merge 3 commits into
Conversation
WalkthroughChangesPR Review Workflow
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pr-review/guidelines.md`:
- Around line 81-82: Update the authentication guidance in the /clean workflow
so gh auth status or glab auth status is required only for phases that call host
APIs, not before local cleanup. Allow /clean to proceed when the local context
is known, even if host credentials are missing or expired.
In `@pr-review/README.md`:
- Around line 1-9: Add a repository-level README entry for pr-review so the
structure-check recognizes this component, using the repository’s existing
README entry format and preserving the workflow-specific README content.
- Around line 102-106: Update the README section describing suggested-change
blocks to document both provider-specific syntaxes: retain the bare fenced
suggestion form for providers that support it, and specify GitLab’s required
offset form consistent with comment-style.md. Ensure the contract no longer
implies that GitLab accepts the bare syntax.
In `@pr-review/SKILL.md`:
- Around line 18-28: Update SKILL.md’s Quick Start entry point to include
compact relative references for /start, /revise, /publish, /continue, and
/clean, alongside the controller workflow reference. Remove redundant
quick-start prose so the file remains a thin entry point under 30 lines while
preserving the existing failure-stop guidance where necessary.
In `@pr-review/skills/clean.md`:
- Around line 90-95: Update the cleanup flow for the context-derived branch
around base_repo_is_scratch so it records whether the workflow created
pr-review/{context} and its expected worktree or commit identity. Before
invoking git branch -D, verify the existing branch matches that recorded
ownership; delete only on a match, otherwise preserve the branch and report that
it was not owned by the workflow.
- Around line 100-114: Update the cleanup instructions around Step 5 and Step 7
so artifact deletion occurs only after git worktree list --porcelain confirms
the worktree is no longer registered. If removal fails or registration remains,
retain the artifact directory, report the exact failure, and stop cleanup;
document this escalation behavior explicitly.
- Around line 43-50: Update Step 3 in the clean workflow to define recovery of
base_repo and base_repo_is_scratch from Git metadata when review-metadata.json
is missing, and validate that both values are available before proceeding to
destructive cleanup. If either value cannot be reliably recovered, stop the
cleanup and ask the user for guidance rather than continuing to Step 5.
In `@pr-review/skills/controller.md`:
- Around line 117-123: Define the restart transition in the /start controller
flow before reusing existing state: require /clean confirmation and complete
cleanup, then begin a fresh /start, or explicitly create a new context while
preserving the existing artifacts. Ensure the resume paths (/revise or /publish)
remain unchanged.
In `@pr-review/skills/publish.md`:
- Around line 41-48: Update Step 1 in the publishing workflow to read the
matching decisions-{NNN}.json alongside the draft, and filter the comments to
post using only entries marked “keep.” Preserve the existing provider metadata
and draft-loading behavior, while excluding dropped or disagreed findings and
transparency notes from publication.
- Around line 50-52: Update the publish workflow’s Step 1 and Step 5 to define
and enforce explicit publishable states, including the already-published state,
while rejecting all others. Persist each successfully created discussion’s post
ID in review-metadata.json and reuse it on retry so completed comments and the
summary note are skipped rather than duplicated; only advance to published after
all required posts succeed.
- Around line 54-66: Update the “Re-Verify Line Anchors” procedure to validate
each retained comment against diff hunks containing line ranges, rather than
relying on git diff --name-status. Use a zero-context diff or equivalent parser,
such as the suggested --unified=0 command, and preserve the existing requirement
to report affected comments and ask whether to drop, relocate, or abort.
In `@pr-review/skills/revise.md`:
- Around line 39-49: Document prerequisite validation and tool-failure handling
in pr-review/skills/revise.md lines 39-49: validate review metadata, the latest
draft, decisions file, and worktree before edits, and specify clear reporting
plus retry or escalation guidance for missing or malformed inputs. Also update
pr-review/skills/continue.md lines 37-59 to validate review metadata and
published-head state and define handling for gh/glab failures, missing
prerequisites, and zero results before refreshing.
In `@pr-review/skills/start.md`:
- Line 348: Update the Markdown template around the “Comment 2” heading and the
third Step 9 list item: remove the heading’s trailing ellipsis/punctuation, and
change that ordered-list marker from “3.” to the configured “1.” prefix.
Preserve the surrounding template content.
- Around line 140-151: Update the Git command examples around the worktree setup
and merge-base calculation to safely handle remote-derived values such as
{baseRefName}. Quote every interpolated branch, ref, and path, and use an
argument-safe command runner where commands are executed; preserve the existing
fetch, worktree, reset, and merge-base behavior.
- Around line 26-28: Update the read-only Git operations policy in the
reviewed-repository phase to permit the later-required commands git rev-parse,
git remote, git reset --hard, and git worktree remove where appropriate. Clearly
distinguish read-only operations against reviewed history from local worktree
setup, refresh, restart, and artifact operations, while preserving the
prohibition on modifying or pushing to the reviewed repository.
- Around line 389-400: Use valid JSON or explicitly label pseudocode in the
decisions example at pr-review/skills/start.md lines 389-400 and metadata
example at lines 413-418, quoting or structurally representing all placeholders.
In pr-review/skills/publish.md lines 77-85 and 109-120, serialize GitHub and
GitLab payload values rather than interpolating comment text or other
placeholders directly.
- Around line 203-205: Encode the complete GitLab project path, including all
namespace separators, before passing it as the project identifier to every REST
API call. Apply this consistently to the commands in pr-review/skills/start.md
(203-205), pr-review/skills/continue.md (95-97), and pr-review/skills/publish.md
(96-124); preserve the existing API operations and pagination behavior.
- Around line 47-79: The PR/MR input handling must extract and retain every URL
component needed by later commands. Update the parsing instructions around
provider detection to define owner/repo extraction for GitHub URLs and
host/namespace/project extraction for GitLab URLs, including nested namespaces;
persist the GitLab host in review-metadata.json alongside the existing provider
and repository fields, and require all GitLab operations used by /continue and
/publish to pass that host to glab rather than assuming gitlab.com.
- Around line 143-148: Update the refresh procedure in
pr-review/skills/start.md:143-148 to fetch the PR head into FETCH_HEAD or a
remote-tracking ref instead of refs/heads/pr-review/{context}, then hard-reset
the checked-out worktree to that fetched commit. Update Step 2 in
pr-review/skills/continue.md:63-66 to reference the same safe refresh procedure;
both sites must avoid fetching directly into the checked-out branch.
In `@pr-review/templates/comment-style.md`:
- Around line 3-6: Correct the path references in comment-style.md so
skills/start.md, skills/revise.md, skills/publish.md, and the Step 10 reference
resolve from the template’s location using ../skills/..., or explicitly define
resolution against the pr-review workflow root. Keep the documented workflow
behavior consistent across all referenced files.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4175ef2b-eb50-4701-8c8b-d017fea9ae03
📒 Files selected for processing (15)
pr-review/README.mdpr-review/SKILL.mdpr-review/commands/clean.mdpr-review/commands/continue.mdpr-review/commands/publish.mdpr-review/commands/revise.mdpr-review/commands/start.mdpr-review/guidelines.mdpr-review/skills/clean.mdpr-review/skills/continue.mdpr-review/skills/controller.mdpr-review/skills/publish.mdpr-review/skills/revise.mdpr-review/skills/start.mdpr-review/templates/comment-style.md
📜 Review details
⚠️ CI failures not shown inline (2)
GitHub Actions: Lint / 3_Validate Structure.txt: Add pr-review skill for reviewing remote GitHub PRs and GitLab MRs
Conclusion: failure
##[group]Run python3 skill-reviewer/scripts/pre-review-checks.py --all --repo-root .
�[36;1mpython3 skill-reviewer/scripts/pre-review-checks.py --all --repo-root .�[0m
shell: /usr/bin/bash -e {0}
env:
pythonLocation: /opt/hostedtoolcache/Python/3.12.13/x64
PKG_CONFIG_PATH: /opt/hostedtoolcache/Python/3.12.13/x64/lib/pkgconfig
Python_ROOT_DIR: /opt/hostedtoolcache/Python/3.12.13/x64
Python2_ROOT_DIR: /opt/hostedtoolcache/Python/3.12.13/x64
Python3_ROOT_DIR: /opt/hostedtoolcache/Python/3.12.13/x64
LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.12.13/x64/lib
##[endgroup]
Discovered workflows: ai-ready, bugfix, code-review, cve-fix, design, docs-writer, e2e, implement, kcs, pr-review, prd, rebase-stack, sizing, skill-reviewer, triage
=== Pre-Review Automated Checks: ai-ready ===
--- Structure ---
PASS: SKILL.md exists
PASS: skills/ directory with 2 skill file(s)
PASS: guidelines.md present
PASS: README.md present
PASS: commands/ directory with 1 command file(s)
--- Frontmatter ---
PASS: SKILL.md has valid frontmatter (name, version, description)
PASS: commands/update.md has valid frontmatter
--- Summary ---
Checks: 7 | PASS: 7 | WARN: 0 | FAIL: 0
=== Pre-Review Automated Checks: bugfix ===
--- Structure ---
PASS: SKILL.md exists
PASS: skills/ directory with 12 skill file(s)
PASS: guidelines.md present
PASS: README.md present
PASS: commands/ directory with 11 command file(s)
--- Frontmatter ---
PASS: SKILL.md has valid frontmatter (name, version, description)
PASS: commands/assess.md has valid frontmatter
PASS: commands/diagnose.md has valid frontmatter
PASS: commands/document.md has valid frontmatter
PASS: commands/feedback.md has valid frontmatter
PASS: commands/fix.md has valid frontmatter
PASS: commands/pr.md has valid frontmatter
PASS: commands/reproduce.md has valid frontmatter
PASS: commands/review.md has valid frontmatter
PASS: commands/start.md has valid frontmatter
PASS: commands/test.md has valid frontma...
GitHub Actions: Lint / Validate Structure: Add pr-review skill for reviewing remote GitHub PRs and GitLab MRs
Conclusion: failure
##[group]Run python3 skill-reviewer/scripts/pre-review-checks.py --all --repo-root .
�[36;1mpython3 skill-reviewer/scripts/pre-review-checks.py --all --repo-root .�[0m
shell: /usr/bin/bash -e {0}
env:
pythonLocation: /opt/hostedtoolcache/Python/3.12.13/x64
PKG_CONFIG_PATH: /opt/hostedtoolcache/Python/3.12.13/x64/lib/pkgconfig
Python_ROOT_DIR: /opt/hostedtoolcache/Python/3.12.13/x64
Python2_ROOT_DIR: /opt/hostedtoolcache/Python/3.12.13/x64
Python3_ROOT_DIR: /opt/hostedtoolcache/Python/3.12.13/x64
LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.12.13/x64/lib
##[endgroup]
Discovered workflows: ai-ready, bugfix, code-review, cve-fix, design, docs-writer, e2e, implement, kcs, pr-review, prd, rebase-stack, sizing, skill-reviewer, triage
=== Pre-Review Automated Checks: ai-ready ===
--- Structure ---
PASS: SKILL.md exists
PASS: skills/ directory with 2 skill file(s)
PASS: guidelines.md present
PASS: README.md present
PASS: commands/ directory with 1 command file(s)
--- Frontmatter ---
PASS: SKILL.md has valid frontmatter (name, version, description)
PASS: commands/update.md has valid frontmatter
--- Summary ---
Checks: 7 | PASS: 7 | WARN: 0 | FAIL: 0
=== Pre-Review Automated Checks: bugfix ===
--- Structure ---
PASS: SKILL.md exists
PASS: skills/ directory with 12 skill file(s)
PASS: guidelines.md present
PASS: README.md present
PASS: commands/ directory with 11 command file(s)
--- Frontmatter ---
PASS: SKILL.md has valid frontmatter (name, version, description)
PASS: commands/assess.md has valid frontmatter
PASS: commands/diagnose.md has valid frontmatter
PASS: commands/document.md has valid frontmatter
PASS: commands/feedback.md has valid frontmatter
PASS: commands/fix.md has valid frontmatter
PASS: commands/pr.md has valid frontmatter
PASS: commands/reproduce.md has valid frontmatter
PASS: commands/review.md has valid frontmatter
PASS: commands/start.md has valid frontmatter
PASS: commands/test.md has valid frontma...
🧰 Additional context used
📓 Path-based instructions (15)
**/commands/*.{md,yaml,yml}
📄 CodeRabbit inference engine (Custom checks)
For any file in a commands/ directory, verify the YAML frontmatter name field uses colon notation matching the pattern {workflow-name}:{phase-name} (e.g., bugfix:assess, design:ingest). The workflow-name must match the parent workflow directory name. Flag any command whose name field is missing, does not contain a colon, or has a prefix that doesn't match its workflow directory.
Files:
pr-review/commands/revise.mdpr-review/commands/publish.mdpr-review/commands/clean.mdpr-review/commands/continue.mdpr-review/commands/start.md
**/{SKILL.md,guidelines.md,skills/*.md,commands/*.md}
📄 CodeRabbit inference engine (Custom checks)
Flag any absolute filesystem path in markdown files within workflow directories (*/SKILL.md, /skills/.md, /commands/.md, */guidelines.md). Paths like /home/, /Users/, /tmp/, /var/, /opt/ are prohibited because workflows are installed via symlink and must use relative paths only. Paths inside fenced code blocks that are clearly examples (containing "example", "e.g.", or placeholder usernames like /home/user/) are exempt.
Files:
pr-review/commands/revise.mdpr-review/commands/publish.mdpr-review/commands/clean.mdpr-review/commands/continue.mdpr-review/skills/controller.mdpr-review/commands/start.mdpr-review/skills/publish.mdpr-review/skills/revise.mdpr-review/skills/start.mdpr-review/skills/clean.mdpr-review/guidelines.mdpr-review/skills/continue.mdpr-review/SKILL.md
**/*.md
📄 CodeRabbit inference engine (Custom checks)
For any changed markdown file in a workflow directory, verify that file path references (backtick-quoted paths like
../skills/controller.mdorguidelines.md) point to files that exist. Flag references to files that don't exist (dangling references). Also flag skill or command files that exist but are never referenced from SKILL.md, controller.md, or any command file (orphaned files).
Files:
pr-review/commands/revise.mdpr-review/commands/publish.mdpr-review/commands/clean.mdpr-review/templates/comment-style.mdpr-review/commands/continue.mdpr-review/skills/controller.mdpr-review/commands/start.mdpr-review/skills/publish.mdpr-review/skills/revise.mdpr-review/README.mdpr-review/skills/start.mdpr-review/skills/clean.mdpr-review/guidelines.mdpr-review/skills/continue.mdpr-review/SKILL.md
⚙️ CodeRabbit configuration file
**/*.md: Cross-workflow consistency (ai-workflows conventions):
- All file references must be relative paths (never absolute) —
this is critical for symlink compatibility- No IDE-specific syntax (Cursor-specific, VS Code-specific, etc.)
- Consistent terminology within a workflow: pick one term, stick
with it- Schema field names and types must match between producer and
consumer files (e.g., if a field is defined in one phase skill
and consumed in another, names and types must agree)- No verbatim duplication of multi-line instruction blocks
across SKILL.md, guidelines.md, and controller.md — each has
a distinct role (shared phase names and brief references are
expected cross-referencing, not duplication)
Files:
pr-review/commands/revise.mdpr-review/commands/publish.mdpr-review/commands/clean.mdpr-review/templates/comment-style.mdpr-review/commands/continue.mdpr-review/skills/controller.mdpr-review/commands/start.mdpr-review/skills/publish.mdpr-review/skills/revise.mdpr-review/README.mdpr-review/skills/start.mdpr-review/skills/clean.mdpr-review/guidelines.mdpr-review/skills/continue.mdpr-review/SKILL.md
**/commands/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/commands/*.md: Command files must reference../skills/controller.md,../SKILL.md, or../skills/phase-name.mdusing relative paths.
Onlycve-fix /close,design /sync, andsizing /applymay write to Jira, and each requires explicit approval.
Files:
pr-review/commands/revise.mdpr-review/commands/publish.mdpr-review/commands/clean.mdpr-review/commands/continue.mdpr-review/commands/start.md
⚙️ CodeRabbit configuration file
**/commands/*.md: Command file review (ai-workflows conventions):
- YAML frontmatter required with name and description fields
- name field must use colon notation: {workflow-name}:{phase-name}
(e.g., bugfix:assess, design:ingest)- Commands must be thin wrappers — they dispatch to a skill,
not implement logic themselves. Flag commands that contain
step-by-step instructions or decision logic- Must include $ARGUMENTS placeholder to pass user context
- Path references must be relative to the command file's location:
use ../skills/controller.md or ../SKILL.md, not absolute paths
and not skills/controller.md (missing ../ prefix)- Every command must have a corresponding skill file it routes to
- No IDE-specific syntax
Files:
pr-review/commands/revise.mdpr-review/commands/publish.mdpr-review/commands/clean.mdpr-review/commands/continue.mdpr-review/commands/start.md
**/*.{md,py,sh}
📄 CodeRabbit inference engine (AGENTS.md)
Workflow content must use plain markdown and contain no IDE-specific syntax.
Files:
pr-review/commands/revise.mdpr-review/commands/publish.mdpr-review/commands/clean.mdpr-review/templates/comment-style.mdpr-review/commands/continue.mdpr-review/skills/controller.mdpr-review/commands/start.mdpr-review/skills/publish.mdpr-review/skills/revise.mdpr-review/README.mdpr-review/skills/start.mdpr-review/skills/clean.mdpr-review/guidelines.mdpr-review/skills/continue.mdpr-review/SKILL.md
**/{SKILL,guidelines,README,skills,commands,templates,prompts}/*
📄 CodeRabbit inference engine (AGENTS.md)
Use relative paths for all file references to preserve symlink compatibility across installation scopes.
Files:
pr-review/commands/revise.mdpr-review/commands/publish.mdpr-review/commands/clean.mdpr-review/templates/comment-style.mdpr-review/commands/continue.mdpr-review/skills/controller.mdpr-review/commands/start.mdpr-review/skills/publish.mdpr-review/skills/revise.mdpr-review/skills/start.mdpr-review/skills/clean.mdpr-review/skills/continue.md
**/*
📄 CodeRabbit inference engine (AGENTS.md)
Save all significant workflow outputs under
.artifacts/{workflow-name}/{context}/.
Files:
pr-review/commands/revise.mdpr-review/commands/publish.mdpr-review/commands/clean.mdpr-review/templates/comment-style.mdpr-review/commands/continue.mdpr-review/skills/controller.mdpr-review/commands/start.mdpr-review/skills/publish.mdpr-review/skills/revise.mdpr-review/README.mdpr-review/skills/start.mdpr-review/skills/clean.mdpr-review/guidelines.mdpr-review/skills/continue.mdpr-review/SKILL.md
**/{SKILL,guidelines,skills,commands,templates,prompts}/*
📄 CodeRabbit inference engine (AGENTS.md)
Behavioral changes must include the version bump in the same commit; do not create a separate version-bump commit.
Files:
pr-review/commands/revise.mdpr-review/commands/publish.mdpr-review/commands/clean.mdpr-review/templates/comment-style.mdpr-review/commands/continue.mdpr-review/skills/controller.mdpr-review/commands/start.mdpr-review/skills/publish.mdpr-review/skills/revise.mdpr-review/skills/start.mdpr-review/skills/clean.mdpr-review/skills/continue.md
**/{templates,prompts}/*.md
⚙️ CodeRabbit configuration file
**/{templates,prompts}/*.md: Template/prompt review (ai-workflows conventions):
- Templates use {placeholder} syntax for caller-filled values —
verify all placeholders are documented- Prompt templates must be self-contained: the sub-agent receives
only the prompt, not the caller's context- Prompts must instruct output to .artifacts/, not inline return
- Templates must work when the workflow is installed via symlink
- No absolute paths in templates
Files:
pr-review/templates/comment-style.md
**/{SKILL.md,guidelines.md,controller.md}
📄 CodeRabbit inference engine (Custom checks)
When any of SKILL.md, guidelines.md, or controller.md in a workflow is changed, compare it against whichever of the other two files are present and check for verbatim duplication of multi-line instruction blocks or paragraphs. Each has a distinct role: SKILL.md is the thin entry point, guidelines.md holds principles/limits/safety/quality/escalation, controller.md manages phase dispatch. Phase names and brief one-line descriptions appearing in multiple files is EXPECTED (cross-referencing, not duplication) — only flag substantial blocks of identical prose or step-by-step instructions that are copied between files.
Files:
pr-review/skills/controller.mdpr-review/guidelines.mdpr-review/SKILL.md
**/skills/*.md
📄 CodeRabbit inference engine (Custom checks)
For any changed skills/*.md file, verify that main steps are numbered sequentially (Step 1, Step 2, Step 3... or ## Step 1, ## Step 2...). Flag: gaps in numbering (1, 2, 4), duplicate numbers (two Step 3s), and any skill with more than 10 main steps (cognitive load risk for AI agents). Sub-steps (Step 1a, Step 3b) are acceptable ONLY when they represent conditional branches off the parent step (e.g., "Step 1a: If , do X"). Flag sub-steps that are actually new main steps inserted to avoid renumbering — those should be promoted to full steps with the sequence renumbered.
When workflows invoke commands that can affect shared systems, follow the documented approval and safety gates: verify
git statusbefore destructive Git operations, confirm branch and base before PR/MR pushes, require explicit approval for Jira writes, and run Vale before applying documentation changes.
Files:
pr-review/skills/controller.mdpr-review/skills/publish.mdpr-review/skills/revise.mdpr-review/skills/start.mdpr-review/skills/clean.mdpr-review/skills/continue.md
⚙️ CodeRabbit configuration file
**/skills/*.md: Phase skill review (ai-workflows conventions):
- Maximum 10 steps per skill invocation — flag if exceeded
(cognitive load / context window risk for AI agents)- Main steps must be numbered sequentially: no gaps, no
duplicates. Sub-steps (e.g., Step 1a) are allowed ONLY for
conditional branches off a parent step — never as a way to
insert a new main step without renumbering- Internal cross-references (e.g., "see Step 4") must point to
correct step numbers- No step should depend on output from a later step
- Synthesis tasks (summarization, assessment, verdict) must NOT
be buried after heavy per-item processing — they degrade in
long contexts- controller.md must reference sibling skills as phase-name.md
(not skills/phase-name.md) — relative to its own directory- Skills referencing _shared/ resources must use the correct
relative path depth (e.g., ../../_shared/recipes/self-review-gate.md
from skills/)- Failure modes must be documented: what to do when prerequisites
are missing, when zero results are returned, when tools are
unavailable- Escalation criteria must be clear: when to stop and ask the user
- Instructions must be unambiguous — an AI agent reading
top-to-bottom should produce correct output on the first try- If the file has YAML frontmatter, name and description are required
Files:
pr-review/skills/controller.mdpr-review/skills/publish.mdpr-review/skills/revise.mdpr-review/skills/start.mdpr-review/skills/clean.mdpr-review/skills/continue.md
**/skills/controller.md
📄 CodeRabbit inference engine (AGENTS.md)
Controllers must reference sibling skills as
phase-name.md, notskills/phase-name.md.
Files:
pr-review/skills/controller.md
*/README.md
⚙️ CodeRabbit configuration file
*/README.md: Workflow README review (ai-workflows conventions):
- Must document .artifacts/ output path for the workflow
- Phase descriptions must match what SKILL.md and skills/
actually implement — flag any documentation drift- Features mentioned in README must exist in the skill files;
features implemented in skills must be documented in README- Prerequisites (required tools, environment, integrations)
must be listed- Usage examples should show actual command invocations
(e.g., /workflow:phase)
Files:
pr-review/README.md
**/guidelines.md
⚙️ CodeRabbit configuration file
**/guidelines.md: Guidelines review (ai-workflows conventions):
- Must contain: Principles, Hard Limits, Safety, Quality, and
Escalation sections (or equivalent coverage)- Content must NOT duplicate SKILL.md or controller.md — each
file has a distinct role- Escalation criteria must be specific and actionable (not vague
"when things go wrong")- Hard limits must be concrete prohibitions, not suggestions
- All phase references should use consistent naming matching
the workflow's actual phase names
Files:
pr-review/guidelines.md
**/SKILL.md
📄 CodeRabbit inference engine (Custom checks)
For any SKILL.md file changed in this PR, verify it is under 30 lines total (including frontmatter). SKILL.md must be thin entry points using progressive disclosure. If a SKILL.md exceeds 30 lines, flag it with the count and suggest moving content to guidelines.md or skills/ files.
**/SKILL.md: Every workflow must have aSKILL.mdentry point with YAML frontmatter containingname,version, anddescription.
KeepSKILL.mdunder 30 lines and place detailed behavior inguidelines.mdandskills/.
SKILL.mdmay referenceguidelines.mdand optionallyskills/controller.mdusing same-directory relative paths.
Workflows must not auto-advance in attended mode unless an explicit unattended mode is documented.
When behavioral workflow files change, update the corresponding workflow version inSKILL.mdaccording to semver: patch for wording or formatting, minor for behavioral or phase changes, and major for removed, renamed, or restructured phases.
Do not bump versions for non-behavioral files such asREADME.mdandGUIDE.md.
Files:
pr-review/SKILL.md
⚙️ CodeRabbit configuration file
**/SKILL.md: SKILL.md review (ai-workflows conventions):
- YAML frontmatter required: opening/closing --- delimiters
- Required fields: name (lowercase, hyphens only, max 64 chars),
description (third person, includes trigger terms and
activated-by commands)- Total file length must be under 30 lines (progressive
disclosure rule — details belong in guidelines.md or skills/)- Must reference guidelines.md for principles/limits/safety/quality
- Must NOT duplicate content from guidelines.md or controller.md
- Should list all phases with references to skills/ or commands/
- No IDE-specific syntax — plain markdown only
- Verify every file path reference resolves to an existing file
Files:
pr-review/SKILL.md
🧠 Learnings (6)
📚 Learning: 2026-05-25T17:11:32.207Z
Learnt from: galel12
Repo: flightctl/ai-workflows PR: 47
File: README.md:140-142
Timestamp: 2026-05-25T17:11:32.207Z
Learning: In markdown files under the repo’s skill/command areas (e.g., `skills/**` and `commands/**`), any references to other files on disk (like links/includes pointing to other skill/command markdown such as `../skills/controller.md` or `commands/*.md`) must use relative paths—never absolute paths (no leading `/` or fully-qualified filesystem paths). This ensures the references remain symlink-safe and resolve correctly at runtime. Do not apply this rule to human-facing prose docs like `README.md`/`CONTRIBUTING.md`; when those documents intentionally distinguish user-level vs project-level install locations, keep the absolute user-level paths (e.g., `~/.cursor/commands/`) as written so the distinction is clear.
Applied to files:
pr-review/commands/revise.mdpr-review/commands/publish.mdpr-review/commands/clean.mdpr-review/commands/continue.mdpr-review/skills/controller.mdpr-review/commands/start.mdpr-review/skills/publish.mdpr-review/skills/revise.mdpr-review/skills/start.mdpr-review/skills/clean.mdpr-review/skills/continue.md
📚 Learning: 2026-07-23T14:18:59.204Z
Learnt from: adalton
Repo: flightctl/ai-workflows PR: 84
File: bugfix/SKILL.md:3-3
Timestamp: 2026-07-23T14:18:59.204Z
Learning: In flightctl/ai-workflows documentation, treat backtick-quoted workflow path templates that include placeholders (e.g., `commands/{command}.md`, `skills/{phase}.md`) as runtime-dispatch/template instructions for AI agents, not literal Markdown links. When these appear, do not flag them as dangling/invalid references solely because the braces indicate substitution of an invoked command or phase name at runtime.
Applied to files:
pr-review/commands/revise.mdpr-review/commands/publish.mdpr-review/commands/clean.mdpr-review/templates/comment-style.mdpr-review/commands/continue.mdpr-review/skills/controller.mdpr-review/commands/start.mdpr-review/skills/publish.mdpr-review/skills/revise.mdpr-review/README.mdpr-review/skills/start.mdpr-review/skills/clean.mdpr-review/guidelines.mdpr-review/skills/continue.mdpr-review/SKILL.md
📚 Learning: 2026-04-12T00:25:51.234Z
Learnt from: adalton
Repo: flightctl/ai-workflows PR: 20
File: design/skills/respond.md:29-31
Timestamp: 2026-04-12T00:25:51.234Z
Learning: In flightctl/ai-workflows skill markdown files, treat path references as two categories:
1) For cross-document markdown links (e.g., links to other .md files like ../skills/controller.md or ../../templates/design.md), use paths relative to the current markdown file’s location so links work under symlinks.
2) For runtime artifact paths used as prose instructions to the AI agent (e.g., .artifacts/design/{issue-number}/publish-metadata.json or .artifacts/prd/config.json), keep them repo-root-relative (start with .artifacts/). Do not convert these artifact paths to be relative to the skill file directory (e.g., don’t rewrite to ../../.artifacts/...), because the AI resolves them from the repo root.
Applied to files:
pr-review/skills/controller.mdpr-review/skills/publish.mdpr-review/skills/revise.mdpr-review/skills/start.mdpr-review/skills/clean.mdpr-review/skills/continue.md
📚 Learning: 2026-04-15T10:19:54.839Z
Learnt from: galel12
Repo: flightctl/ai-workflows PR: 22
File: kcs/skills/gather.md:25-26
Timestamp: 2026-04-15T10:19:54.839Z
Learning: In flightctl/ai-workflows, for Jira URL examples inside skill Markdown files, follow the repo-wide convention and use a real example Jira link of the form `https://issues.redhat.com/browse/PROJ-123` (not a generic placeholder like `https://example.com/...`). Since this is a documented convention, do not flag it as a portability/documentation hardcoding issue when reviewing similar skill markdown files.
Applied to files:
pr-review/skills/controller.mdpr-review/skills/publish.mdpr-review/skills/revise.mdpr-review/skills/start.mdpr-review/skills/clean.mdpr-review/skills/continue.md
📚 Learning: 2026-04-16T10:39:50.418Z
Learnt from: galel12
Repo: flightctl/ai-workflows PR: 22
File: kcs/skills/gather.md:34-37
Timestamp: 2026-04-16T10:39:50.418Z
Learning: In flightctl/ai-workflows workflow skill files (e.g., kcs/bugfix/prd/design skills), do not require sanitization/normalization of free-form user-supplied identifier placeholders (such as {issue-key} or {issue-number}) when they’re used to construct artifact paths like `.artifacts/{workflow}/{identifier}/`. This is intentional because these workflows run in human-supervised IDE sessions where the user provides the values interactively and confirms the output. Therefore, do not flag missing sanitization/normalization of these identifiers as a security or correctness issue during review for these skill files.
Applied to files:
pr-review/skills/controller.mdpr-review/skills/publish.mdpr-review/skills/revise.mdpr-review/skills/start.mdpr-review/skills/clean.mdpr-review/skills/continue.md
📚 Learning: 2026-06-15T15:50:50.503Z
Learnt from: adalton
Repo: flightctl/ai-workflows PR: 64
File: skill-reviewer/SKILL.md:3-3
Timestamp: 2026-06-15T15:50:50.503Z
Learning: In flightctl/ai-workflows, treat `SKILL.md` as a size-constrained document: keep it at or under 30 lines. If a `SKILL.md` already exceeds 30 lines but was not changed by the current PR (a known pre-existing issue), don’t require fixing it as part of the PR. If the PR does modify a too-long `SKILL.md`, refactor it into a thin entry point (e.g., move bulk content to smaller companion docs and leave only a brief overview/links) so the `SKILL.md` itself stays within the 30-line limit.
Applied to files:
pr-review/SKILL.md
🪛 GitHub Actions: Lint / 1_Markdown Lint.txt
pr-review/skills/start.md
[error] 348-348: markdownlint MD026/no-trailing-punctuation: Heading contains trailing punctuation ('...'). Remove the punctuation from the heading.
🪛 GitHub Actions: Lint / 3_Validate Structure.txt
pr-review/README.md
[error] 1-1: Pre-review checks failed: README.md is missing an entry for workflow 'pr-review'.
🪛 GitHub Actions: Lint / Markdown Lint
pr-review/skills/start.md
[error] 348-348: markdownlint MD026/no-trailing-punctuation: Heading contains trailing punctuation (' ...'). Remove the trailing punctuation.
🪛 GitHub Actions: Lint / Validate Structure
pr-review/README.md
[error] 1-1: Missing README entry for workflow 'pr-review'. Command: python3 skill-reviewer/scripts/pre-review-checks.py --all --repo-root .
🪛 GitHub Check: Markdown Lint
pr-review/skills/start.md
[failure] 367-367: Ordered list item prefix
pr-review/skills/start.md:367:1 MD029/ol-prefix Ordered list item prefix [Expected: 1; Actual: 3; Style: 1/1/1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md029.md
[failure] 348-348: Trailing punctuation in heading
pr-review/skills/start.md:348:17 MD026/no-trailing-punctuation Trailing punctuation in heading [Punctuation: ' ...'] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md026.md
🪛 LanguageTool
pr-review/templates/comment-style.md
[style] ~14-~14: Since ownership is already implied, this phrasing may be redundant.
Context: ...pository's own preference, checked into its own repo (analogous to how the prd wor...
(PRP_OWN)
[style] ~29-~29: The expression “make sense” can be too colloquial for certain contexts. For a more formal tone, try using an alternative.
Context: ...What if this used a map instead?", "Would it make sense to extract this into a helper?" - Avo...
(WOULD_IT_MAKE_SENSE)
[style] ~46-~46: Since ownership is already implied, this phrasing may be redundant.
Context: ...** Each posted comment is anchored to its own line by the host's own UI; it doesn't n...
(PRP_OWN)
pr-review/skills/publish.md
[style] ~26-~26: Consider an alternative for the overused word “exactly”.
Context: ...raft.** The posted comment bodies are exactly the "Comment (as it will be posted)" te...
(EXACTLY_PRECISELY)
pr-review/skills/revise.md
[style] ~61-~61: Since ownership is already implied, this phrasing may be redundant.
Context: ... don't leave a comment standing that your own answer just undermined. ### Step 3: Ap...
(PRP_OWN)
[style] ~85-~85: Since ownership is already implied, this phrasing may be redundant.
Context: ...gory and rationale the user gave, or your own assessment if they didn't specify one)....
(PRP_OWN)
[style] ~119-~119: Consider an alternative to strengthen your wording.
Context: ...ll them to run /publish. If they have further changes, they can run /revise again -- there ...
(CHANGES_ADJUSTMENTS)
pr-review/README.md
[style] ~110-~110: Consider using the typographical ellipsis character here instead.
Context: ...efault to suggestive framing ("Should we...", never "Do X") with no severity/catego...
(ELLIPSIS)
[style] ~134-~134: Consider using the typographical ellipsis character here instead.
Context: .../{n}/head| | List existing comments |gh api .../pulls/{n}/comments+.../reviews|...
(ELLIPSIS)
[style] ~134-~134: Consider using the typographical ellipsis character here instead.
Context: ...nts | gh api .../pulls/{n}/comments + .../reviews | glab api .../discussions |...
(ELLIPSIS)
[style] ~134-~134: Consider using the typographical ellipsis character here instead.
Context: ..../pulls/{n}/comments+.../reviews|glab api .../discussions` | | Post the review | One ...
(ELLIPSIS)
[style] ~135-~135: Consider using the typographical ellipsis character here instead.
Context: ...ost the review | One batched review via gh api .../pulls/{n}/reviews | One discussion per...
(ELLIPSIS)
[style] ~135-~135: Consider using the typographical ellipsis character here instead.
Context: ...views| One discussion per comment viaglab api .../discussions`, plus a separate summary n...
(ELLIPSIS)
pr-review/skills/start.md
[style] ~37-~37: Since ownership is already implied, this phrasing may be redundant.
Context: ...tly.** After obtaining the review, form your own opinion on each finding's value befor...
(PRP_OWN)
[uncategorized] ~54-~54: The official name of this software platform is spelled with a capital “H”.
Context: ...e / before the #) -> provider = github, CLI = gh. Split on # for `{num...
(GITHUB)
[uncategorized] ~56-~56: The official name of this software platform is spelled with a capital “H”.
Context: ...th contains /pull/{n} -> provider = github, CLI = gh. - Path contains `/-/merg...
(GITHUB)
[grammar] ~122-~122: Ensure spelling is correct
Context: ...e SSH and the other HTTPS), use that toplevel path as {base-repo}. No clone need...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~156-~156: Consider using a more formal/concise alternative here.
Context: ...git worktree add fails for any reason other than "already checked out in the main wor...
(OTHER_THAN)
[uncategorized] ~219-~219: The official name of this software platform is spelled with a capital “H”.
Context: ...configuration files 4. CI/CD workflows (.github/workflows/, .gitlab-ci.yml) 5. Test ...
(GITHUB)
[typographical] ~348-~348: Consider using an em dash (—) instead of ‘--’.
Context: ...ally agree}: {rationale} ### Comment 2 -- ... ``` ### Step 9: Present for Local ...
(TWO_HYPHENS)
[style] ~348-~348: Consider using the typographical ellipsis character here instead.
Context: ...y agree}: {rationale} ### Comment 2 -- ... ``` ### Step 9: Present for Local Appr...
(ELLIPSIS)
pr-review/guidelines.md
[style] ~40-~40: Consider using the typographical ellipsis character here instead.
Context: ...comment tone is suggestive ("Should we...", "What if...") by default, and pluggab...
(ELLIPSIS)
[style] ~40-~40: Consider using the typographical ellipsis character here instead.
Context: ...s suggestive ("Should we...", "What if...") by default, and pluggable per `temp...
(ELLIPSIS)
[style] ~47-~47: This word has been used in one of the immediately preceding sentences. Using a synonym could make your text more interesting to read, unless the repetition is intentional.
Context: .... - The human decides. The reviewer proposes findings; the local user decides whic...
(EN_REPEATEDWORDS_PROPOSE)
[style] ~115-~115: Consider using a more formal/concise alternative here.
Context: ...- git worktree add fails for a reason other than "already checked out" (see `skills/st...
(OTHER_THAN)
pr-review/skills/continue.md
[style] ~68-~68: This phrase is redundant. Consider using “outside”.
Context: ...). If the worktree was somehow removed outside of /clean (e.g., manually deleted), fall...
(OUTSIDE_OF)
[style] ~177-~177: This word has been used in one of the immediately preceding sentences. Using a synonym could make your text more interesting to read, unless the repetition is intentional.
Context: ...e incremental draft (or "nothing new to propose") and the previously-posted-comment sta...
(EN_REPEATEDWORDS_PROPOSE)
🔇 Additional comments (6)
pr-review/commands/clean.md (1)
1-12: LGTM!pr-review/commands/continue.md (1)
1-12: LGTM!pr-review/commands/publish.md (1)
1-12: LGTM!pr-review/commands/revise.md (1)
1-12: LGTM!pr-review/skills/clean.md (1)
82-88: 🗄️ Data Integrity & IntegrationNo change needed. With
git -C {base-repo},.artifacts/pr-review/{context}/worktreeis relative to the repository where the worktree is configured, so this is not a cross-directory delete risk.pr-review/commands/start.md (1)
1-11: LGTM!
|
Thanks for putting this together, @asafbennatan — the thoroughness is clear, especially the provider-specific posting mechanics (GitHub vs. GitLab suggestion syntax, the After reviewing the full PR, I want to share some thinking about how we evaluate whether something belongs in this repo as a workflow versus being better served by direct prompting — not because this work isn't solid, but because this framing will help calibrate future contributions too. The "what does this add over prompting?" testThe core question we ask of every workflow in this repo: does it produce meaningfully better outcomes than a well-crafted natural-language prompt? For example, most of what
or even:
The AI already knows how to fetch PR metadata, read the diff, build context, validate findings against real lines, and format review comments. The review judgment — the hard part — isn't something the workflow improves over what a prompt like "be constructive and suggestive in tone" achieves. Workflows in this repo earn their place when they:
PR review of a remote PR/MR is something AI coding tools handle well natively, and the structured ceremony here doesn't measurably improve the outcome. On the relationship to code-reviewOne thing worth flagging: Next stepsI'm not going to merge this as-is. I'm happy to set up a call to talk it through — I may be wrong, and either way it'd be useful to align on how we evaluate workflow proposals. Feel free to DM me to find a time. |
Reuses the code-review workflow's review protocol and human-in-the-loop approval loop, but operates on a remote PR/MR URL: checks it out into a disposable git worktree, drafts inline comments with permalinks, snippets, and provider-correct suggestion blocks, and posts them only after explicit local approval. Co-authored-by: Cursor <cursoragent@cursor.com>
- Register pr-review in the repo-level AGENTS.md and README.md so the structure-check and markdown-lint CI jobs pass. - Fix a nested-fence bug in start.md that silently broke Step 9's numbered list, plus a stray heading with trailing punctuation. - Skip generated/vendored/binary files and de-duplicate against existing PR/MR discussion during the initial /start pass (parity with the prior code-review PR #46 attempt). - Replace the refs/heads/pr-review/{context} branch with a plain refs/pr-review/{context} ref: fetching into a checked-out branch is refused by Git, which broke every worktree refresh in /start and /continue. - publish.md now filters posts through decisions-{NNN}.json instead of posting every drafted candidate, re-verifies line anchors against real diff hunks instead of --name-status, and persists GitLab discussion IDs incrementally so a retry after a partial failure can't double-post. - Persist and use the GitLab host and a fully URL-encoded project path everywhere, so self-hosted instances and nested subgroups work. - clean.md now derives base_repo/base_repo_is_scratch when metadata is missing, verifies the ref before deleting it, and stops instead of rm -rf'ing artifacts if worktree removal genuinely fails. - Assorted doc-only fixes: safety/auth scoping for /clean, restart semantics in controller.md, prerequisite-failure handling in revise.md/continue.md, and relative-path/step-reference fixes in comment-style.md. Bumps pr-review to 0.2.0 (behavioral changes across most skill files). Co-authored-by: Cursor <cursoragent@cursor.com>
…terpolation CodeRabbit correctly narrowed the earlier "illustrative JSON" pushback to a real gap: publish.md's payload-building steps didn't tell the agent how to safely embed comment text (which routinely contains quotes, backslashes, and newlines) into the JSON files posted to gh/glab api. Switch to jq --rawfile for body text so escaping is handled correctly instead of relying on naive string interpolation. Bump pr-review to 0.2.1 (behavioral fix to skills/publish.md). Co-authored-by: Cursor <cursoragent@cursor.com>
e4803cc to
4c8214a
Compare
There was a problem hiding this comment.
Actionable comments posted: 22
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pr-review/guidelines.md`:
- Around line 57-58: Update the “No auto-advancing between phases” rule in the
guidelines to explicitly cover every phase transition, including /start,
/continue, /revise, /publish, and /clean; state that the controller must always
wait for user input before moving between any of these phases.
In `@pr-review/README.md`:
- Around line 197-201: Update the README examples around the pr-review workflow
start command to use the installed user-facing command names: show
`/pr-review:start` for Claude Code and `/pr-review-start` for Cursor, or
explicitly identify the runtime that supports `/start`; ensure the examples
contain actual invocations rather than unscoped commands.
- Around line 75-77: Update the `/clean` documentation to explicitly state that
it operates locally only, removing local workflow references, the worktree,
scratch clone, and artifacts, while never modifying the reviewed repository’s
remote.
In `@pr-review/SKILL.md`:
- Around line 4-14: Rewrite the frontmatter description in SKILL.md as a
third-person sentence, replacing the imperative “Use when asked” phrasing while
preserving the existing PR/MR, GitHub, GitLab, review, and activated command
trigger terms.
In `@pr-review/skills/clean.md`:
- Around line 120-138: The cleanup logic for refs/pr-review/{context} must never
treat mere existence as ownership when review metadata is missing. Update the
custom-ref deletion flow to delete only when it matches a recorded head_sha or
another verifiable workflow-owned identity; otherwise preserve the ref and
report that ownership could not be verified.
- Around line 43-66: Add validation to Step 3 after reading
review-metadata.json: reject invalid JSON, missing required fields, or a
base_repo that is not a valid Git repository. On any validation failure, stop
before destructive worktree/ref cleanup, retain the artifact directory, and ask
the user for guidance; document these failure cases alongside the existing
missing-metadata handling.
In `@pr-review/skills/continue.md`:
- Around line 162-185: Update Step 8 metadata handling to persist the analyzed
commit as last_reviewed_head_sha, including when no net-new comments are
proposed. Update Step 2’s continuation baseline to reuse this recorded head
before falling back to publish-metadata.json’s head_sha_reviewed, so subsequent
/continue rounds do not reprocess the same commits.
- Around line 53-66: Update the metadata-fetching step before comparing with
head_sha_reviewed to derive a scalar current head SHA: use GitHub headRefOid or
the final commits[].oid, and GitLab sha or diff_refs.head_sha. If no supported
SHA field is present, stop and report the missing field without performing the
comparison.
In `@pr-review/skills/controller.md`:
- Around line 49-56: Align /continue artifact ownership in
pr-review/skills/controller.md: add /continue to the producer list for
decisions-{NNN}.json. In pr-review/README.md at line 42, add
review-metadata.json to the documented Continue artifacts.
In `@pr-review/skills/publish.md`:
- Around line 153-159: Update the publish flow to persist the successful `glab
mr note` result immediately, using the returned note ID or a summary_posted
marker in the iteration-specific publish metadata. During retry handling, check
this marker before posting “See comments below” and skip the summary note when
it is already recorded, while preserving existing posted_comments behavior for
individual discussions.
- Around line 75-83: The anchor-validation command around the git diff
invocation must safely handle remote-derived paths and refs. Update the command
runner and all uses of {path} and {merge-base-sha} in this validation flow to
pass them as separate, argument-safe parameters after --, preserving paths
containing spaces or shell metacharacters without changing the hunk-range
validation behavior.
- Around line 100-119: Isolate generated review payload files for each publish
attempt instead of globbing shared tmp-comment files. Update the publishing flow
around the per-comment jq generation and final jq -s assembly to use an
iteration/attempt-keyed temporary directory or an explicit list containing only
files created in the current run, while preserving the existing payload
structure.
- Around line 161-172: Update the GitLab discussion payload construction for the
remaining kept comments to include position.line_range.start and
position.line_range.end, using each endpoint’s correct line_code and type while
preserving single-line anchors. Validate that multi-line ranges can be
constructed; reject invalid findings and request relocation instead of posting
an incomplete anchor.
In `@pr-review/skills/revise.md`:
- Around line 41-51: Update the revision preflight in the workflow described by
review-metadata.json to validate review state and publication metadata before
creating a new draft. Reject published reviews and direct the user to /continue;
for partial GitLab posts, resume the existing iteration or preserve its posted
comment IDs when advancing, preventing duplicate discussions. Keep the existing
missing-file and missing-worktree reporting behavior unchanged.
In `@pr-review/skills/start.md`:
- Around line 147-153: Update the repository setup instructions around Step 3 to
explicitly derive a canonical {clone-url} for each supported provider before
running git clone. Ensure provider authentication failures prevent cloning and
terminate with the exact clone error, rather than leaving URL or credential
resolution to inference; preserve the existing scratch-repository path and
metadata behavior.
- Around line 184-187: Update the git worktree add failure handling in step 6 to
stop for every failure, including “already checked out in the main working
tree.” Report the registered worktree path when available, and instruct the user
to inspect it with git worktree list and clean stale entries with git worktree
remove --force before retrying.
- Around line 283-291: Update the exception guidance to remove the blanket
treatment of package-lock.json, go.sum, and Cargo.lock as skipped files. In the
file-skipping instructions, require checking project conventions to determine
whether each lockfile is generated; otherwise summarize its dependency and
security-impacting changes or include it in dependency-security review, and
mention any lockfile skipped with its specific justification in Step 9.
- Around line 108-110: Update the context resolution described in start.md to
include a normalized GitLab host, or a hash derived from the full canonical
PR/MR URL, along with the owner/namespace, repository/project, and number. Apply
this resolver consistently to all .artifacts/pr-review/{context}/ paths and
refs/pr-review/{context}, and use the identical context resolver in clean.md.
- Around line 459-479: Update the review-metadata.json schema in the /start
workflow to include the deterministic worktree path required by downstream
phases. Ensure the new worktree field name and type exactly match the
expectations in continue.md Step 1 and publish.md Step 1, and populate it when
metadata is written.
- Around line 170-180: Update provider normalization to define both {head-ref}
and {base-ref}, then revise the existing-worktree refresh instructions to fetch
both refs on every refresh instead of using unresolved {ref}. Keep the reset
target as refs/pr-review/{context}, and update the merge-base instructions in
continue.md to reference the exact normalized base-ref/head-ref fetch commands
so an advancing target branch is reflected.
- Around line 97-103: Normalize the JSON returned by the GitLab `glab mr view`
command before Steps 2–10 consume it: map `target_branch`, `source_branch`, and
`web_url` to `{baseRefName}`, `{headRefName}`, and `{url}`; normalize
`description`, `author`, and `state`; explicitly fetch and normalize the commit
list; and derive and validate `{head-sha}` from the fetched ref or a documented
GitLab SHA field.
In `@pr-review/templates/comment-style.md`:
- Around line 20-22: Update the candidate override resolution rule in the
comment-style template so a missing project override silently uses the built-in
default. Keep warnings for existing overrides that are unreadable or empty, and
retain the announcement when a valid project override is used.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 16bedb6c-44fb-4aab-9628-72f84bcbe247
📒 Files selected for processing (12)
AGENTS.mdREADME.mdpr-review/README.mdpr-review/SKILL.mdpr-review/guidelines.mdpr-review/skills/clean.mdpr-review/skills/continue.mdpr-review/skills/controller.mdpr-review/skills/publish.mdpr-review/skills/revise.mdpr-review/skills/start.mdpr-review/templates/comment-style.md
| - **No auto-advancing between phases.** Always wait for the user between | ||
| `/start`, `/revise`, and `/publish`. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
State the no-auto-advance boundary for every phase.
The rule names only /start, /revise, and /publish. It does not explicitly cover /continue or /clean. An agent could advance from /continue into /revise, or run cleanup without a user checkpoint.
Replace the command list with “every phase transition”, or include all five phases.
As per path instructions, the controller must list all phases and always wait for the user between phases.
Proposed wording
- **No auto-advancing between phases.** Always wait for the user between
- `/start`, `/revise`, and `/publish`.
+ every phase transition, including `/start`, `/revise`, `/publish`,
+ `/continue`, and `/clean`.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - **No auto-advancing between phases.** Always wait for the user between | |
| `/start`, `/revise`, and `/publish`. | |
| - **No auto-advancing between phases.** Always wait for the user between | |
| `/start`, `/revise`, and `/publish`. | |
| **No auto-advancing between phases.** Always wait for the user between | |
| every phase transition, including `/start`, `/revise`, `/publish`, | |
| `/continue`, and `/clean`. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pr-review/guidelines.md` around lines 57 - 58, Update the “No auto-advancing
between phases” rule in the guidelines to explicitly cover every phase
transition, including /start, /continue, /revise, /publish, and /clean; state
that the controller must always wait for user input before moving between any of
these phases.
Source: Path instructions
| /clean (once you're fully done with this PR/MR) | ||
| -> the only phase that removes the worktree, branch/scratch-clone, and | ||
| artifacts |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
test -f pr-review/skills/clean.md
rg -n -C 5 \
'\b(branch|worktree|scratch|push|remote|delete|remove|prune)\b' \
pr-review/skills/clean.mdRepository: flightctl/ai-workflows
Length of output: 5549
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- README lines 68-82 ---'
sed -n '68,82p' pr-review/README.md
printf '%s\n' '--- clean skill command lines ---'
rg -n '^\s*(git|gh|rm|find|mv|cp|mkdir|rmdir|push|fetch|remote)\b|remote|push|delete|update-ref|worktree remove' \
pr-review/skills/clean.mdRepository: flightctl/ai-workflows
Length of output: 1525
Make /clean explicitly local-only.
State that /clean removes only local workflow refs, the worktree, scratch clone, and artifacts. It must not modify the reviewed repository’s remote.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pr-review/README.md` around lines 75 - 77, Update the `/clean` documentation
to explicitly state that it operates locally only, removing local workflow
references, the worktree, scratch clone, and artifacts, while never modifying
the reviewed repository’s remote.
| Then run the `pr-review` workflow's `start` command with a PR or MR URL: | ||
|
|
||
| ```text | ||
| /start https://github.com/{owner}/{repo}/pull/{number} | ||
| /start https://gitlab.com/{namespace}/{project}/-/merge_requests/{number} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the installed command names in the examples.
These examples use unscoped /start commands. The repository’s user-facing conventions use /workflow:phase for Claude Code and /workflow-phase for Cursor. Show /pr-review:start and /pr-review-start, or state which runtime accepts /start.
Without this change, a reader can follow the README and invoke a command that the installer did not create.
As per path instructions, README usage examples must show actual command invocations, such as /workflow:phase.
Proposed command examples
-/start https://github.com/{owner}/{repo}/pull/{number}
-/start https://gitlab.com/{namespace}/{project}/-/merge_requests/{number}
+/pr-review:start https://github.com/{owner}/{repo}/pull/{number}
+/pr-review:start https://gitlab.com/{namespace}/{project}/-/merge_requests/{number}
+
+# Cursor uses:
+/pr-review-start https://github.com/{owner}/{repo}/pull/{number}
+/pr-review-start https://gitlab.com/{namespace}/{project}/-/merge_requests/{number}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Then run the `pr-review` workflow's `start` command with a PR or MR URL: | |
| ```text | |
| /start https://github.com/{owner}/{repo}/pull/{number} | |
| /start https://gitlab.com/{namespace}/{project}/-/merge_requests/{number} | |
| Then run the `pr-review` workflow's `start` command with a PR or MR URL: | |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pr-review/README.md` around lines 197 - 201, Update the README examples
around the pr-review workflow start command to use the installed user-facing
command names: show `/pr-review:start` for Claude Code and `/pr-review-start`
for Cursor, or explicitly identify the runtime that supports `/start`; ensure
the examples contain actual invocations rather than unscoped commands.
Source: Path instructions
| description: >- | ||
| AI-driven review of a remote pull request or merge request, given its URL | ||
| (GitHub or GitLab, auto-detected). Checks out the PR/MR into a git worktree, | ||
| explains PR context and key decisions, evaluates changes using the shared | ||
| code-review protocol, and drafts inline review comments (with code links, | ||
| snippets, and suggested-change blocks) in a pluggable, suggestive tone. | ||
| Always presents the draft for local approval before posting a review, never | ||
| changes the reviewed code, supports revision based on questions or new | ||
| findings, and can resume after the PR/MR receives new commits. Use when | ||
| asked to review, comment on, or give feedback on a GitHub PR or GitLab MR | ||
| URL. Activated by commands: /start, /revise, /publish, /continue, /clean. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Make the frontmatter description third person.
The description uses a noun phrase followed by the imperative “Use when asked”. Rewrite it as a third-person sentence while keeping the PR/MR, GitHub, GitLab, review, and command trigger terms.
As per path instructions, SKILL.md descriptions must be third person and include trigger terms and activated-by commands.
Proposed metadata wording
-description: >-
- AI-driven review of a remote pull request or merge request, given its URL
+description: >-
+ This workflow reviews a remote pull request or merge request from its URL
(GitHub or GitLab, auto-detected). Checks out the PR/MR into a git worktree,
explains PR context and key decisions, evaluates changes using the shared
code-review protocol, and drafts inline review comments (with code links,
snippets, and suggested-change blocks) in a pluggable, suggestive tone.
- Always presents the draft for local approval before posting, never
+ It presents the draft for local approval before posting, never
changes the reviewed code, supports revision based on questions or new
findings, and can resume after the PR/MR receives new commits. Use when
asked to review, comment on, or give feedback on a GitHub PR or GitLab MR
URL. Activated by commands: /start, /revise, /publish, /continue, /clean.
+ It activates on requests to review, comment on, or give feedback on a
+ GitHub PR or GitLab MR URL. Activated by commands: /start, /revise,
+ /publish, /continue, /clean.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pr-review/SKILL.md` around lines 4 - 14, Rewrite the frontmatter description
in SKILL.md as a third-person sentence, replacing the imperative “Use when
asked” phrasing while preserving the existing PR/MR, GitHub, GitLab, review, and
activated command trigger terms.
Source: Path instructions
| ### Step 3: Read Worktree Metadata | ||
|
|
||
| Read `.artifacts/pr-review/{context}/review-metadata.json` (if present) for | ||
| `base_repo`, `base_repo_is_scratch`, and `context` -- needed to know exactly | ||
| what to tear down. | ||
|
|
||
| If metadata is missing (e.g., an interrupted `/start`), derive both values | ||
| instead of guessing: | ||
|
|
||
| - `base_repo_is_scratch`: `true` if | ||
| `.artifacts/pr-review/{context}/_scratch-repo/` exists, else `false`. | ||
| - `base_repo`: if scratch, it's `.artifacts/pr-review/{context}/_scratch-repo`. | ||
| Otherwise, derive it from the worktree itself (if | ||
| `.artifacts/pr-review/{context}/worktree/` exists): | ||
| ```bash | ||
| git -C .artifacts/pr-review/{context}/worktree rev-parse --path-format=absolute --git-common-dir | ||
| ``` | ||
| `{base-repo}` is that path's parent directory (strip the trailing `/.git`). | ||
|
|
||
| If neither the worktree nor a scratch clone exists, there is nothing | ||
| requiring `{base-repo}` -- skip straight to Step 7 (artifact removal). | ||
| If `{base-repo}` still can't be determined from the above, stop and ask the | ||
| user for it rather than guessing; do not proceed to Step 5's worktree/ref | ||
| removal without it. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Stop before cleanup when metadata is malformed.
Step 3 handles missing metadata, but it does not define validation for invalid JSON, missing fields, or a base_repo that is not a valid Git repository. Later destructive commands can then run with guessed or empty state.
Validate required fields and path identity. If validation fails, retain the artifact directory and ask the user for guidance.
As per path instructions: “Failure modes must be documented: what to do when prerequisites are missing, when zero results are returned, when tools are unavailable.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pr-review/skills/clean.md` around lines 43 - 66, Add validation to Step 3
after reading review-metadata.json: reject invalid JSON, missing required
fields, or a base_repo that is not a valid Git repository. On any validation
failure, stop before destructive worktree/ref cleanup, retain the artifact
directory, and ask the user for guidance; document these failure cases alongside
the existing missing-metadata handling.
Source: Path instructions
| **To refresh an existing worktree instead** (step 1's branch): re-run | ||
| the same `fetch origin "{ref}:refs/pr-review/{context}"` command | ||
| (updates the local ref to the new head -- safe even though the worktree | ||
| is checked out, since this isn't a branch), then: | ||
| ```bash | ||
| git -C .artifacts/pr-review/{context}/worktree reset --hard refs/pr-review/{context} | ||
| ``` | ||
| 5. Compute the merge base and record `{head-sha}`: | ||
| ```bash | ||
| git -C {base-repo} merge-base "origin/{baseRefName}" refs/pr-review/{context} | ||
| git -C {base-repo} rev-parse refs/pr-review/{context} |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Define and refresh both provider refs.
The refresh form uses an unresolved {ref} at Line 171. It also refreshes only the head ref, while the merge-base later reads origin/{baseRefName}. If the target branch advances, /continue can compute a stale merge base.
Define {head-ref} and {base-ref} during provider normalization. Fetch both on every refresh, then reset the worktree to refs/pr-review/{context}. Make continue.md reference these exact commands.
Proposed refresh contract
- re-run the same `fetch origin "{ref}:refs/pr-review/{context}"` command
+ fetch the normalized provider head ref into `refs/pr-review/{context}`
+ fetch the normalized base branch into `refs/remotes/origin/{baseRefName}`
+ reset the worktree to `refs/pr-review/{context}`🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pr-review/skills/start.md` around lines 170 - 180, Update provider
normalization to define both {head-ref} and {base-ref}, then revise the
existing-worktree refresh instructions to fetch both refs on every refresh
instead of using unresolved {ref}. Keep the reset target as
refs/pr-review/{context}, and update the merge-base instructions in continue.md
to reference the exact normalized base-ref/head-ref fetch commands so an
advancing target branch is reflected.
| 6. If `git worktree add` fails for any reason other than "already checked | ||
| out in the main working tree": stop, report the error, and ask the user | ||
| to run `git worktree list` to inspect and `git worktree remove --force` | ||
| to clean up any stale entries before retrying. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Stop on every git worktree add failure.
The instructions exclude the “already checked out in the main working tree” error from the stop condition but define no action for it. A stale worktree can own refs/pr-review/{context}, so continuing can produce no worktree or use the wrong worktree.
Report the registered worktree path and ask the user to inspect it before retrying.
As per path instructions: “Failure modes must be documented: what to do when prerequisites are missing, when zero results are returned, when tools are unavailable.”
🧰 Tools
🪛 LanguageTool
[style] ~184-~184: Consider using a more formal/concise alternative here.
Context: ...git worktree add fails for any reason other than "already checked out in the main wor...
(OTHER_THAN)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pr-review/skills/start.md` around lines 184 - 187, Update the git worktree
add failure handling in step 6 to stop for every failure, including “already
checked out in the main working tree.” Report the registered worktree path when
available, and instruct the user to inspect it with git worktree list and clean
stale entries with git worktree remove --force before retrying.
Source: Path instructions
| **Exception: skip generated, vendored, and binary files.** Exclude files | ||
| matching common generated/vendored patterns (e.g. `*.gen.go`, `*.pb.go`, | ||
| `*_pb2.py`, `vendor/`, `node_modules/`, `dist/`, `build/`, lock files like | ||
| `package-lock.json`/`go.sum`/`Cargo.lock`) and any file the diff or `git | ||
| -C {worktree} diff --stat` shows as binary. Confirm against the target | ||
| project's own conventions (`.gitattributes`, `AGENTS.md`) where available -- | ||
| a project may generate files this default list doesn't recognize. Never | ||
| draft findings against a skipped file; mention which files were skipped | ||
| and why when presenting the draft in Step 9. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Do not skip lockfiles without dependency-specific handling.
The default skip list excludes package-lock.json, go.sum, and Cargo.lock. These files can change dependency versions, integrity hashes, and transitive security exposure.
Skip a lockfile only when the target project's conventions identify it as generated. Otherwise, summarize dependency changes or apply a dependency-security review.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pr-review/skills/start.md` around lines 283 - 291, Update the exception
guidance to remove the blanket treatment of package-lock.json, go.sum, and
Cargo.lock as skipped files. In the file-skipping instructions, require checking
project conventions to determine whether each lockfile is generated; otherwise
summarize its dependency and security-impacting changes or include it in
dependency-security review, and mention any lockfile skipped with its specific
justification in Step 9.
| Write `.artifacts/pr-review/{context}/review-metadata.json`: | ||
|
|
||
| ```json | ||
| { | ||
| "provider": "{provider}", | ||
| "host": "{host, if provider is gitlab; null if github}", | ||
| "owner_or_namespace": "{owner-or-namespace}", | ||
| "repo_or_project": "{repo-or-project}", | ||
| "number": {number}, | ||
| "context": "{context}", | ||
| "base_repo": "{base-repo path}", | ||
| "base_repo_is_scratch": false, | ||
| "base_ref_name": "{baseRefName}", | ||
| "head_sha": "{head-sha}", | ||
| "merge_base_sha": "{merge-base-sha}", | ||
| "iteration": 1, | ||
| "state": "awaiting_decision", | ||
| "started": "{ISO 8601 timestamp}", | ||
| "last_updated": "{ISO 8601 timestamp}", | ||
| "reviewer_agent_id": "{agent ID if a subagent was spawned, null otherwise}" | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Persist the worktree path required by downstream phases.
The metadata example does not contain a worktree field. However, continue.md Step 1 and publish.md Step 1 require a worktree location. A normal /start run can therefore produce metadata that downstream phases reject as incomplete.
Add the deterministic worktree path, or change all consumers to derive and validate it.
As per path instructions: “Schema field names and types must match between producer and consumer files.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pr-review/skills/start.md` around lines 459 - 479, Update the
review-metadata.json schema in the /start workflow to include the deterministic
worktree path required by downstream phases. Ensure the new worktree field name
and type exactly match the expectations in continue.md Step 1 and publish.md
Step 1, and populate it when metadata is written.
Source: Path instructions
| If a candidate override is missing, unreadable, or empty, warn the user and | ||
| fall back to this built-in default. If using a project override, announce | ||
| it: *"Using project override for comment style."* |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Treat a missing project override as normal.
The built-in file is the fallback. The current rule warns when the project override is missing, so every repository without .pr-review/templates/comment-style.md produces a false warning.
Use the built-in default silently when the optional override is absent. Warn only when an existing override is unreadable or empty.
The resolution rules in this template make the project override optional.
Proposed fallback rule
-If a candidate override is missing, unreadable, or empty, warn the user and
-fall back to this built-in default. If using a project override, announce
-it: *"Using project override for comment style."*
+If the project override does not exist, use this built-in default silently.
+If it exists but is unreadable or empty, warn the user and fall back to this
+built-in default. If using a project override, announce it: *"Using project
+override for comment style."*📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| If a candidate override is missing, unreadable, or empty, warn the user and | |
| fall back to this built-in default. If using a project override, announce | |
| it: *"Using project override for comment style."* | |
| If the project override does not exist, use this built-in default silently. | |
| If it exists but is unreadable or empty, warn the user and fall back to this | |
| built-in default. If using a project override, announce it: *"Using project | |
| override for comment style."* |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pr-review/templates/comment-style.md` around lines 20 - 22, Update the
candidate override resolution rule in the comment-style template so a missing
project override silently uses the built-in default. Keep warnings for existing
overrides that are unreadable or empty, and retain the announcement when a valid
project override is used.
2 participants
Summary
pr-reviewworkflow that reviews a remote GitHub PR or GitLab MR given its URL (provider auto-detected), reusingcode-review's review protocol and human-in-the-loop approval flow./clean), builds a reviewer profile from the target repo's own conventions, and drafts findings extended with permalinks, code snippets, and provider-correct suggestion blocks (GitHub```suggestion, GitLab```suggestion:-N+M)./publishposts them as inline review comments, in a pluggable suggestive tone (templates/comment-style.md, overridable per-project)./revise(answer questions, edit/drop/add comments) and/continue(incremental re-review after the PR/MR receives new commits)./start,/revise,/publish,/continue,/clean.Test plan
/pr-review:startagainst a real GitHub PR URL and confirm worktree setup, context summary, and draft review with correctly anchored comments/suggestion blocks/pr-review:startagainst a real GitLab MR URL and confirm the same, including GitLab's offset-annotated suggestion syntax/pr-review:revise(ask a question, drop a comment, add a user-authored finding) and confirm the redrafted review reflects it/pr-review:publish, confirm comments post correctly on the host andpublish-metadata.jsonis written/pr-review:continue, confirm only net-new findings are drafted/pr-review:cleanand confirm the worktree, scratch clone, and artifacts are fully removedMade with Cursor
Summary
pr-reviewworkflow for remote GitHub pull requests and GitLab merge requests./start,/revise,/publish,/continue, and/cleancommands.Workflow and convention impact
pr-reviewworkflow.code-reviewprotocol and approval flow.README.mdandAGENTS.mdto document the workflow and its file organization._shared/resources or cross-workflow conventions.