From f668c0c0360133efad89aedf9b5513df9d8753e2 Mon Sep 17 00:00:00 2001 From: William Thorsen Date: Sun, 10 May 2026 08:52:18 -0700 Subject: [PATCH 1/7] agents|refactor: Rename review-change to review-branch and simplify Convert the existing `review-change` skill into the focused `review-branch` skill. The skill now reviews the branch diff `merge-base(HEAD, )..HEAD` against an explicit `--diff-base=` flag (defaulting to the project's default branch). The previous `commit ` positional and the entire commit-scope code path are removed: to review a specific commit or range, check out the desired commit and invoke `/review-branch --diff-base=~1`. Compilation, dependency installation, and test execution all require the working tree to match the commit being reviewed, so requiring a checkout is honest about the prerequisites and removes a redundant entry point. The previous `ticket ` positional becomes `--ticket=` for consistency with sibling skills. Ticket auto-resolution from the active run directory continues to work when `--ticket` is omitted. The "Ticket compliance" section becomes "Specification compliance," structured as a list of `{ source_type, label, content, criteria? }` records. With zero sources the section is omitted (matching the prior "no ticket" behavior); with one source the rendered output is structurally identical to the previous single-ticket layout; with multiple sources the subsection repeats. This shape is the canonical input for `review-pr`, which appends the PR description as a `pr_description` source on top of the ticket source. The skill body documents that `review-branch` is the canonical home of the shared review process and that `review-pr` invokes it after resolving platform-specific inputs, so future maintainers do not re-add the commit-scope path. --- .../content/skills/review-branch/SKILL.md | 170 ++++++++++++ .../content/skills/review-change/SKILL.md | 242 ------------------ 2 files changed, 170 insertions(+), 242 deletions(-) create mode 100644 packages/agents/content/skills/review-branch/SKILL.md delete mode 100644 packages/agents/content/skills/review-change/SKILL.md diff --git a/packages/agents/content/skills/review-branch/SKILL.md b/packages/agents/content/skills/review-branch/SKILL.md new file mode 100644 index 00000000..c6390ea8 --- /dev/null +++ b/packages/agents/content/skills/review-branch/SKILL.md @@ -0,0 +1,170 @@ +--- +name: review-branch +description: Perform code review of branch changes against a diff base +user-invocable: true +--- + +# Review branch + +Act as a conscientious code reviewer for the changes on the current branch relative to a diff base. Review the diff `merge-base(HEAD, )..HEAD`. + +This skill is the canonical home of the shared review process. `review-pr` invokes the same review process after resolving platform-specific inputs (PR metadata, HEAD verification, ticket from PR linked issues, PR description as a second specification source). Do not re-add a commit-scope path: to review a specific commit or range, check out the desired commit and invoke `/review-branch --diff-base=~1`. Compilation, dependency installation, and test execution all require the working tree to match the commit being reviewed, so requiring a checkout is honest about the prerequisites and removes a redundant entry point. + +## Arguments + +| Flag | Effect | Default | +| ------------------- | ------------------------------------------------------------------------------------------------------------------------------------------ | ------------------------- | +| `--diff-base=` | Reference to diff against. Reviews `merge-base(HEAD, )..HEAD`. | Project's default branch | +| `--ticket=` | Ticket or requirements to check the implementation against. Resolved per [ticket source resolution](../_data/ticket-source-resolution.md). | Auto-resolved (see below) | + +## Process + +1. **Get context** using `get-session-context` to obtain `default_branch`, `ticket_id`, `ticket_ref`, `project_slug`, and `artifact_base_dir`. +2. **Resolve diff base** — if `--diff-base=` was provided, use ``; otherwise use `default_branch`. Compute the merge-base SHA once: `git merge-base HEAD `. Use this SHA for the diff command in step 5. +3. **Resolve specification sources** — produce a list of spec sources (each a `{ source_type, label, content, criteria? }` record): + - **Explicit `--ticket=`**: resolve per [ticket source resolution](../_data/ticket-source-resolution.md) and append as a `ticket` source. + - **Auto-resolve**: if `--ticket` was omitted and `ticket_id` is non-null, scan `{artifact_base_dir}/projects/{project_slug}/tickets/{ticket_id}/` for the most recent `*_ticket.md` file and append it as a `ticket` source. + - **No source available**: leave the list empty. The "Specification compliance" section is omitted from the output. + + `review-pr` may pass additional sources (notably the PR description as `pr_description`). The list is the canonical input for the "Specification compliance" section regardless of who populated it. + +4. **Read prior artifacts** — if a run directory exists for this ticket, read all artifacts chronologically for context (including any prior dispositions). +5. **Analyze changes**: `git diff ..HEAD`. +6. **Review thoroughly** following the guidelines below. +7. **Assign a score** out of 10. +8. **Save the review** per the [Saving](#saving) section. +9. **Present next steps** — after saving, present a next-steps prompt following [next-steps-after-review](../_data/next-steps-after-review.md). Supply recommendation context: finding counts and categories from the review, and whether specification compliance gaps or unplanned work were identified. The next-steps prompt is interactive output only and is not saved in the review artifact. + +## Review guidelines + +Comprehensive review — trace logic, verify edge cases, assess architectural impact. + +### Criteria + +Read and apply the `review-criteria` skill (`../review-criteria/SKILL.md`). Additionally for this skill: + +- Reference associated Jira ticket if integrations.jira.enabled is true +- Look at commit messages for context +- Suggest refactoring if new code adds tech debt + +## Issue numbering + +Uniquely number all issues for easy reference. See [finding scheme](../_data/artifact-conventions.md#finding-scheme-fwtrs--legacy-suffix) for full category criteria and criticality mapping. + +- FIXMEs: `F{n}` — critical, must fix before merge +- Warnings: `W{n}` — questionable decisions, may block merge +- TODOs: `T{n}` — should fix, can wait for next PR +- Recommendations: `R{n}` — advisable but discretionary +- Suggestions: `S{n}` — optional improvement +- Legacy: `{F,W,T,R,S}{n}-L` — observation in pre-existing code, not authored in this change. Uses the same severity letter as the equivalent author finding plus a `-L` suffix (e.g., `F3-L`, `W2-L`) + +## Output format + +Section-header icons (🚨, ⚠️, 📋, 🧠, ☝️, 🔍) come from the canonical [finding scheme](../_data/artifact-conventions.md#finding-scheme-fwtrs--legacy-suffix); render them as shown. + +When `ticket_ref` is null (no ticket on the branch), omit the `{ticket_ref}: ` portion so the heading reads naturally without it — e.g., `# Code review: {description}`. + +```markdown +# Code review: {ticket_ref}: {description in imperative mood} + +Commit: {short hash of HEAD} +Timestamp: {YYYY-MM-DD HH:MM UTC} +Author: {author(s)} +Generated by: {Agent name} (model: {model}) + +## Summary of changes + +## Strengths + +### Code quality + +### Documentation + +### Test quality + +## Action required + +### FIXMEs 🚨 + +{Critical issues - regressions, broken functionality, unsafe code, type safety violations} + +### Warnings ⚠️ + +{Questionable decisions that may block merge - require justification} + +### TODOs 📋 + +{Should-fix changes that can wait for next PR} + +## Areas for improvement + +### Recommendations 🧠 + +{Advisable but discretionary - don't count against score} + +### Suggestions ☝️ + +{Optional improvements - don't count against score} + +### Legacy observations 🔍 + +{Observations in pre-existing code, using severity-tagged IDs with `-L` suffix (e.g., `F3-L`, `T2-L`). Frame as future opportunities, don't count against score} + +## Technical assessment + +## Conclusion + +Score: X/10 + +## Specification compliance + +{Omit this entire section when the spec-source list is empty. Otherwise render one subsection per source. With one source the section is structurally identical to the prior single-ticket "Ticket compliance" output; with two or more, repeat the subsection per source.} + +### {source.label} + +{The label is `{source_type}: {short identifier}` — for example, `ticket: #553` or `pr_description: PR #1024`. Use the source's natural identifier so a reader can tell at a glance which specification a row evaluates against.} + +#### Acceptance criteria + +| # | Criterion | Status | Notes | +| --- | ---------------- | -------- | ------- | +| 1 | {criterion text} | {status} | {notes} | + +Status values: ✅ Met, ⚠️ Partial, ❌ Not addressed + +Extract criteria from whatever structure the source uses (numbered lists, checkboxes, prose). If the source does not have clearly delimited acceptance criteria, derive them from its problem statement and solution description. PR descriptions typically expose criteria as the bullet items under `## What`, `## Summary`, or an explicit acceptance-criteria heading; fall back to the description body when no list is present. + +#### Unplanned work + +{Bullet list of changes not traceable to any criterion in this source. If none, state "None."} + +#### Assessment + +{1-2 sentence summary of alignment between the implementation and this source.} +``` + +## Scoring + +- Score only the quality of changes in the reviewed scope +- Do not deduct for failure to address pre-existing issues +- Legacy observations don't affect score + +## Saving + +### Path resolution + +Use `get-session-context` to obtain `artifact_base_dir`, `project_slug`, and `ticket_id`. + +Follow [artifact conventions](../_data/artifact-conventions.md). + +### Run artifact + +The review is saved as a run artifact: `{timestamp}_reviewer_review.md` + +1. Resolve ticket directory: `{artifact_base_dir}/projects/{project_slug}/tickets/{ticket_id}/` +2. Find or create a run directory: + - **If an active run exists** (the most recent run directory whose `run-index.json` has `context.branch` matching the current branch AND `completedAt` is absent): save into it + - **If no active run exists**: create a new run directory named `{timestamp}-interactive` where timestamp matches this review's timestamp +3. Save: `{run-dir}/{timestamp}_reviewer_review.md` + +Each review is a separate artifact in the run directory. Do not append to existing files — the chronological sequence of files is the history. diff --git a/packages/agents/content/skills/review-change/SKILL.md b/packages/agents/content/skills/review-change/SKILL.md deleted file mode 100644 index 68c63bec..00000000 --- a/packages/agents/content/skills/review-change/SKILL.md +++ /dev/null @@ -1,242 +0,0 @@ ---- -name: review-change -description: Perform code review of branch or commit changes -user-invocable: true ---- - -# Review change - -Act as a conscientious code reviewer for changes in the current scope. - -## Arguments - -- _(no arguments)_: Review branch changes (default scope) -- `commit []`: Review a specific commit (HEAD if ref omitted) -- `ticket ` _(optional, branch scope only)_: Ticket or requirements to check code against. Resolve using the [ticket source resolution](../_data/ticket-source-resolution.md) table. - -## Process - -1. **Get context** using `get-session-context` to obtain `default_branch`, `ticket_id`, `ticket_ref`, `project_slug`, and `artifact_base_dir` -2. **Resolve ticket** _(branch scope only)_ — resolve the ticket source using this priority order: - 1. **Explicit argument** — if a `ticket` argument was provided, resolve it per the [Arguments](#arguments) table - 2. **Auto-resolve** — if no argument, scan `{artifact_base_dir}/projects/{project_slug}/tickets/{ticket_id}/` for the most recent `*_ticket.md` file and read it - 3. **No ticket found** — skip ticket compliance; review proceeds without it -3. **Read prior artifacts** — if a run directory exists for this ticket, read all artifacts chronologically for context (including any prior dispositions). _(Branch scope only.)_ -4. **Analyze changes**: - - Branch scope: `git diff $DEFAULT_BRANCH...HEAD` - - Commit scope: `git diff ^..` (or `git show ` for context) -5. **Review thoroughly** following the guidelines below -6. **Assign a score** out of 10 -7. **Save the review** per the [Saving](#saving) section -8. **Present next steps** _(branch scope only)_ — after saving, present a next-steps prompt following [next-steps-after-review](../_data/next-steps-after-review.md). Supply recommendation context: finding counts and categories from the review, and whether ticket compliance gaps or unplanned work were identified. The next-steps prompt is interactive output only and is not saved in the review artifact. - -## Review guidelines - -### Depth calibration - -- **Branch scope**: Comprehensive review — trace logic, verify edge cases, assess architectural impact. -- **Commit scope**: Confine analysis to changes in the commit. - -### Criteria - -Read and apply the `review-criteria` skill (`../review-criteria/SKILL.md`). Additionally for this skill: - -- Reference associated Jira ticket if integrations.jira.enabled is true -- Look at commit messages for context -- Suggest refactoring if new code adds tech debt -- Examine preceding commits with same issue ID if relevant _(commit scope)_ -- Alert to changes that appear to create regressions _(commit scope)_ - -## Issue numbering - -Uniquely number all issues for easy reference. See [finding scheme](../_data/artifact-conventions.md#finding-scheme-fwtrs--legacy-suffix) for full category criteria and criticality mapping. - -- FIXMEs: `F{n}` — critical, must fix before merge -- Warnings: `W{n}` — questionable decisions, may block merge -- TODOs: `T{n}` — should fix, can wait for next PR -- Recommendations: `R{n}` — advisable but discretionary -- Suggestions: `S{n}` — optional improvement -- Legacy: `{F,W,T,R,S}{n}-L` — observation in pre-existing code, not authored in this change. Uses the same severity letter as the equivalent author finding plus a `-L` suffix (e.g., `F3-L`, `W2-L`) - -## Output format - -Section-header icons (🚨, ⚠️, 📋, 🧠, ☝️, 🔍) come from the canonical [finding scheme](../_data/artifact-conventions.md#finding-scheme-fwtrs--legacy-suffix); render them as shown. - -When `ticket_ref` is null (no ticket on the branch), omit the `{ticket_ref}: ` portion (or `{ticket_ref} ` for commit scope) so the heading reads naturally without it — e.g., `# Code review: {description}` or `# Commit review: [{WORK_TYPE}] - {description}`. - -### Branch scope - -```markdown -# Code review: {ticket_ref}: {description in imperative mood} - -Commit: {short hash of HEAD} -Timestamp: {YYYY-MM-DD HH:MM UTC} -Author: {author(s)} -Generated by: {Agent name} (model: {model}) - -## Summary of changes - -## Strengths - -### Code quality - -### Documentation - -### Test quality - -## Action required - -### FIXMEs 🚨 - -{Critical issues - regressions, broken functionality, unsafe code, type safety violations} - -### Warnings ⚠️ - -{Questionable decisions that may block merge - require justification} - -### TODOs 📋 - -{Should-fix changes that can wait for next PR} - -## Areas for improvement - -### Recommendations 🧠 - -{Advisable but discretionary - don't count against score} - -### Suggestions ☝️ - -{Optional improvements - don't count against score} - -### Legacy observations 🔍 - -{Observations in pre-existing code, using severity-tagged IDs with `-L` suffix (e.g., `F3-L`, `T2-L`). Frame as future opportunities, don't count against score} - -## Technical assessment - -## Conclusion - -Score: X/10 - -## Ticket compliance - -{Omit this entire section when no ticket is available. Include it when a ticket was resolved, even if all criteria are met and no unplanned work exists.} - -### Acceptance criteria - -| # | Criterion | Status | Notes | -| --- | ---------------- | -------- | ------- | -| 1 | {criterion text} | {status} | {notes} | - -Status values: ✅ Met, ⚠️ Partial, ❌ Not addressed - -Extract criteria from whatever structure the ticket uses (numbered lists, checkboxes, prose). If the ticket does not have clearly delimited acceptance criteria, derive them from the ticket's problem statement and solution description. - -### Unplanned work - -{Bullet list of changes not traceable to any acceptance criterion. If none, state "None."} - -### Assessment - -{1-2 sentence summary of alignment between the implementation and the ticket requirements.} -``` - -### Commit scope - -```markdown -# Commit review: {ticket_ref} [{WORK_TYPE}] - {description in imperative mood} - -Commit: {short hash} -Timestamp: {YYYY-MM-DD HH:MM UTC} -Author: {author(s)} -Generated by: {Agent name} (model: {model}) - -## Summary of changes - -## Strengths - -### Code quality - -### Documentation - -### Test quality - -## Action required - -### FIXMEs 🚨 - -{Critical issues - regression risks} - -### Warnings ⚠️ - -{Questionable decisions that may block merge - require justification} - -### TODOs 📋 - -{Should-fix changes that can wait for next PR} - -## Areas for improvement - -### Recommendations 🧠 - -{Advisable but discretionary - don't count against score} - -### Suggestions ☝️ - -{Optional improvements - focus only on author's changes, don't count against score} - -### Legacy observations 🔍 - -{Observations in pre-existing code, using severity-tagged IDs with `-L` suffix (e.g., `F3-L`, `T2-L`). Frame as future opportunities, don't count against score} - -## Technical assessment - -## Conclusion - -Score: X/10 -``` - -## Scoring - -- Score only the quality of changes in the reviewed scope -- Do not deduct for failure to address pre-existing issues -- Legacy observations don't affect score -- For commit scope: remember this commit may be one of many in a branch - -## Saving - -### Path resolution - -Use `get-session-context` to obtain `artifact_base_dir`, `project_slug`, and `ticket_id`. - -Follow [artifact conventions](../_data/artifact-conventions.md). - -### Branch scope - -The review is saved as a run artifact: `{timestamp}_reviewer_review.md` - -1. Resolve ticket directory: `{artifact_base_dir}/projects/{project_slug}/tickets/{ticket_id}/` -2. Find or create a run directory: - - **If an active run exists** (the most recent run directory whose `run-index.json` has `context.branch` matching the current branch AND `completedAt` is absent): save into it - - **If no active run exists**: create a new run directory named `{timestamp}-interactive` where timestamp matches this review's timestamp -3. Save: `{run-dir}/{timestamp}_reviewer_review.md` - -Each review is a separate artifact in the run directory. Do not append to existing files — the chronological sequence of files is the history. - -### Commit scope - -Ticket directory: `{artifact_base_dir}/projects/{project_slug}/tickets/{ticket_id}/` - -Artifact type: `review`. Filename format: - -``` -{timestamp}_{slug}_review.md -``` - -## Revisions _(commit scope only)_ - -If author makes subsequent revisions and requests re-review: - -- Mention only changes relative to previous review -- Do not comment on author's responsiveness to suggestions -- Append new review to existing file if it exists From 96d77d22f3faa744d485b73063c90c8fc0d077db Mon Sep 17 00:00:00 2001 From: William Thorsen Date: Sun, 10 May 2026 08:55:37 -0700 Subject: [PATCH 2/7] agents|feat: Add review-pr surface with platform delegates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Introduce `/review-pr` as a user-invocable entry skill that reviews a pull request, alongside the internal `review-gh-pr` and `review-bb-pr` delegates that handle the platform-specific work. The new surface mirrors the established `create-pr` / `merge-pr` delegation pattern: the entry skill detects the platform, dispatches to the appropriate delegate to fetch PR metadata, verify HEAD, and resolve specification sources, then invokes the shared review process from `review-branch` with the prepared inputs. `/review-pr` accepts a positional PR identifier (number or full URL) and the same `--diff-base=` and `--ticket=` flags as `/review-branch`. Platform detection uses the cascade in `_data/ticket-source-resolution.md#platform-resolution-cascade`, with URL hosts overriding the cascade when a full URL is supplied. The delegate returns a resolved-input record (`merge_base_sha`, `diff_base`, `spec_sources`, `pr_metadata`) that the entry skill passes to `review-branch`'s review process — the entry skill itself runs no review logic. `review-gh-pr` issues a single `gh pr view --json` call to fetch every needed field, then verifies HEAD against `headRefOid`. On mismatch, it exits with the literal `gh pr checkout {n}` suggestion so the user has a one-line fix. Ticket resolution follows the cascade `--ticket override → first closingIssuesReferences entry → PR body parse for closes/fixes/resolves/bare-hash references → none`. The PR description is always included as a `pr_description` source; the ticket is prepended when one is resolved. `review-bb-pr` follows the same shape using the Bitbucket REST API at `api.bitbucket.org/2.0` and the same auth resolution as `bb-pr-inline-comment` and `create-bitbucket-pr` (no new client). Bitbucket Cloud's PR API does not expose a structured linked-issues field equivalent to GitHub's `closingIssuesReferences`, so the ticket-resolution cascade for the Bitbucket delegate is `--ticket override → PR body parse → none`. This divergence is documented in the skill body so future readers do not assume parity, and the cascade can grow a linked-issues step later without breaking the delegate interface. The HEAD-mismatch error message includes a Bitbucket-equivalent fetch-and-checkout pair. The two specification sources (ticket + PR description) are listed `[ticket?, pr_description]` because the ticket is the higher-authority source — it states what was asked of the implementation, while the PR description presents what was delivered. Calling out divergence between the two is deferred to a follow-up ticket because it raises distinct design questions. --- .../content/skills/review-bb-pr/SKILL.md | 170 ++++++++++++++++++ .../content/skills/review-gh-pr/SKILL.md | 152 ++++++++++++++++ .../agents/content/skills/review-pr/SKILL.md | 117 ++++++++++++ 3 files changed, 439 insertions(+) create mode 100644 packages/agents/content/skills/review-bb-pr/SKILL.md create mode 100644 packages/agents/content/skills/review-gh-pr/SKILL.md create mode 100644 packages/agents/content/skills/review-pr/SKILL.md diff --git a/packages/agents/content/skills/review-bb-pr/SKILL.md b/packages/agents/content/skills/review-bb-pr/SKILL.md new file mode 100644 index 00000000..3f4748a6 --- /dev/null +++ b/packages/agents/content/skills/review-bb-pr/SKILL.md @@ -0,0 +1,170 @@ +--- +name: review-bb-pr +description: Fetch Bitbucket PR metadata, verify HEAD, and resolve specification sources for review-pr +user-invocable: false +--- + +# Review Bitbucket pull request + +Internal delegate that handles the Bitbucket-specific work for `review-pr`: fetch PR metadata via the Bitbucket REST API, verify the local HEAD matches the PR's head commit, resolve the ticket (with body-parse fallback), and prepare the spec-source list. Returns a resolved-input record that `review-pr` passes to `review-branch`'s shared review process. + +This skill does not run a review. The review logic lives in `review-branch`. + +## Delegate interface + +| Input | Type | Description | +| -------------------- | -------------- | ------------------------------------------------- | +| `pr_id` | string | PR number (e.g., `"42"`) or full Bitbucket PR URL | +| `diff_base_override` | string \| null | `--diff-base` value from `review-pr` if provided | +| `ticket_override` | string \| null | `--ticket` value from `review-pr` if provided | +| `project_slug` | string | From session context | +| `ticket_id` | string \| null | From session context | +| `artifact_base_dir` | string | From session context | + +## Resolved-output contract + +On success, return a record with the same shape as `review-gh-pr`'s output. `review-pr` consumes both delegates' outputs interchangeably: + +| Field | Type | Description | +| ---------------- | ----------------------------------------------------- | ----------------------------------------------------------------- | +| `merge_base_sha` | string | Result of `git merge-base HEAD ` | +| `diff_base` | string | Resolved ref (override if provided, else PR's destination branch) | +| `spec_sources` | array of `{ source_type, label, content, criteria? }` | One entry per available specification source | +| `pr_metadata` | object | `{ number, url, head_oid, base_ref, title }` | + +On HEAD mismatch, do not return — exit non-zero with the mismatch error. `review-pr` surfaces the message and stops. + +## Bitbucket access + +Use the same access mechanism as `bb-pr-inline-comment` and `create-bitbucket-pr` — the Bitbucket Cloud REST API at `https://api.bitbucket.org/2.0/`. **Do not introduce a new client.** Authentication resolves in priority order: + +1. **Bot credentials (Basic auth):** `BITBUCKET_BOT_USERNAME` + `BITBUCKET_BOT_TOKEN` env vars. +2. **API token (Bearer auth):** `BITBUCKET_API_TOKEN` env var. +3. **macOS keychain (Bearer auth):** `security find-generic-password -a "$USER" -s "bitbucket-api-token" -w`. + +If no credentials are available, exit non-zero with the same auth-setup hint that `bb-pr-inline-comment` prints. + +## Process + +### 1. Normalize `pr_id` and detect workspace/repo + +If `pr_id` is a URL of the form `https://bitbucket.org/{workspace}/{repo}/pull-requests/{number}`, extract `{workspace}`, `{repo}`, and `{number}`. + +Otherwise treat `pr_id` as the number directly. Auto-detect workspace and repo from `git remote get-url origin` using the same parser as `bb-pr-inline-comment` (supports both `https://bitbucket.org/ws/repo` and `git@bitbucket.org:ws/repo.git`). + +### 2. Fetch PR metadata + +Issue a single Bitbucket REST call: + +``` +GET https://api.bitbucket.org/2.0/repositories/{workspace}/{repo}/pullrequests/{pr_number} +``` + +Parse the JSON response with `jq` (or python3). Capture from the response: + +- `id` (PR number), `title`, `description` (PR body), `links.html.href` (URL) +- `source.branch.name` (head branch name), `source.commit.hash` (full head commit SHA) +- `destination.branch.name` (base branch name) + +If the API call fails (non-2xx), surface the response status and body and stop. + +### 3. Verify HEAD + +Compare the local HEAD against the PR's head commit: + +```bash +local_head=$(git rev-parse HEAD) +``` + +If `local_head` does not equal `source.commit.hash`, exit non-zero with: + +``` +PR #{number}'s head commit is {short(source_commit_hash)} but HEAD is at {short(local_head)}. Check out the PR branch first (e.g., "git fetch origin pull-requests/{number}/from:pr-{number} && git checkout pr-{number}") or pull the latest commits on {source_branch_name}. +``` + +Use the first 7 characters for short SHAs. **Fail closed** — never proceed with mismatched state. + +The Bitbucket equivalent of `gh pr checkout` is not a single CLI subcommand, so the message lists the canonical fetch-and-checkout pair. Bitbucket does not expose a numeric `pull-requests/{n}/from` ref by default in every workflow — if a project uses a different convention, the user can substitute `git checkout {source_branch_name}` and pull the latest. + +### 4. Resolve the diff base + +Apply this cascade: + +1. If `diff_base_override` is non-null, use it. +2. Otherwise, use `destination.branch.name` from PR metadata. + +Compute the merge-base once: + +```bash +merge_base_sha=$(git merge-base HEAD {diff_base}) +``` + +### 5. Resolve the ticket + +**Bitbucket linked-issues divergence from GitHub.** Bitbucket Cloud's PR API does not expose a structured `closingIssuesReferences` field equivalent to GitHub's. (Bitbucket has a separate Issues product with linked-issue support, but its surface differs and is not always enabled per workspace.) Rather than introducing a partial linked-issues mechanism here, this delegate uses a simpler cascade: + +1. **`ticket_override`** — if non-null, resolve per [ticket source resolution](../_data/ticket-source-resolution.md) and use it. +2. **Parse PR body for issue references** — scan `description` for the first match of any of these patterns (case-insensitive): + - `closes #{n}`, `closes: #{n}` + - `fixes #{n}`, `fixes: #{n}` + - `resolves #{n}`, `resolves: #{n}` + - bare `#{n}` + - Jira-style keys (e.g., `MAC-42`) when the project's `ticket_ref_prefix` is configured to a Jira prefix. + + Fetch the matched issue per [ticket source resolution](../_data/ticket-source-resolution.md). + +3. **No ticket** — proceed with the PR description as the only spec source. + +The divergence from `review-gh-pr` is intentional and documented here so future readers do not assume parity. If Bitbucket linked-issue parity is added later (via the Jira integration or a future Bitbucket API field), the cascade can grow a step 2 between override and body parse without breaking the delegate interface. + +### 6. Build the spec-source list + +Always include the PR description as a source: + +``` +{ + source_type: "pr_description", + label: "pr_description: PR #{number}", + content: , + criteria: +} +``` + +If a ticket was resolved in step 5, prepend it: + +``` +{ + source_type: "ticket", + label: "ticket: {ticket_ref or short identifier}", + content: , + criteria: +} +``` + +The list order is `[ticket?, pr_description]` — same as `review-gh-pr`. + +### 7. Return the resolved-output record + +``` +{ + merge_base_sha: , + diff_base: , + spec_sources: , + pr_metadata: { + number: , + url: , + head_oid: , + base_ref: , + title: + } +} +``` + +`review-pr` passes this to `review-branch` and the review proceeds. + +## Important + +- **Single REST call for metadata.** All fields are fetched at once. Do not split into multiple calls. +- **HEAD mismatch is a hard stop.** The error message must include a Bitbucket-equivalent checkout suggestion so the user has a one-line fix path even though Bitbucket lacks `gh pr checkout`'s exact equivalent. +- **Linked-issues divergence is intentional.** The Bitbucket cascade lacks the GitHub `closingIssuesReferences` step. This is documented above so the parity gap is visible to future readers; do not silently re-add a partial implementation. +- **No review logic here.** This delegate prepares inputs only. The review process runs inside `review-branch` after `review-pr` invokes it with the resolved inputs. diff --git a/packages/agents/content/skills/review-gh-pr/SKILL.md b/packages/agents/content/skills/review-gh-pr/SKILL.md new file mode 100644 index 00000000..4372a4ad --- /dev/null +++ b/packages/agents/content/skills/review-gh-pr/SKILL.md @@ -0,0 +1,152 @@ +--- +name: review-gh-pr +description: Fetch GitHub PR metadata, verify HEAD, and resolve specification sources for review-pr +user-invocable: false +--- + +# Review GitHub pull request + +Internal delegate that handles the GitHub-specific work for `review-pr`: fetch PR metadata in a single `gh pr view` call, verify the local HEAD matches the PR's head commit, resolve the ticket from PR linked issues (with body-parse fallback), and prepare the spec-source list. Returns a resolved-input record that `review-pr` passes to `review-branch`'s shared review process. + +This skill does not run a review. The review logic lives in `review-branch`. This skill prepares inputs and verifies preconditions; it never produces findings, never saves a review artifact, and never calls anything beyond `gh` and `git`. + +## Delegate interface + +| Input | Type | Description | +| -------------------- | -------------- | ------------------------------------------------------------------------------------ | +| `pr_id` | string | PR number (e.g., `"1024"`) or full URL (`"https://github.com/owner/repo/pull/1024"`) | +| `diff_base_override` | string \| null | `--diff-base` value from `review-pr` if provided | +| `ticket_override` | string \| null | `--ticket` value from `review-pr` if provided | +| `project_slug` | string | From session context | +| `ticket_id` | string \| null | From session context | +| `artifact_base_dir` | string | From session context | + +## Resolved-output contract + +On success, return a record with the following fields. `review-pr` passes this directly to `review-branch`: + +| Field | Type | Description | +| ---------------- | ----------------------------------------------------- | ------------------------------------------------------------------------------------------------ | +| `merge_base_sha` | string | Result of `git merge-base HEAD <diff_base>` | +| `diff_base` | string | Resolved ref (override if provided, else `baseRefName`) | +| `spec_sources` | array of `{ source_type, label, content, criteria? }` | One entry per available specification source (PR description always present; ticket if resolved) | +| `pr_metadata` | object | `{ number, url, head_oid, base_ref, title }` | + +On HEAD mismatch, do not return — exit non-zero with the mismatch error. `review-pr` surfaces the message and stops. + +## Process + +### 1. Normalize `pr_id` + +If `pr_id` is a URL of the form `https://github.com/{owner}/{repo}/pull/{number}`, extract `{number}`. Otherwise treat `pr_id` as the number directly. + +### 2. Fetch PR metadata + +Issue a single `gh pr view` call with all fields needed downstream: + +```bash +gh pr view {pr_number} --json number,title,body,url,headRefName,headRefOid,baseRefName,closingIssuesReferences +``` + +Parse the JSON with a real parser (`python3 -c "import sys,json; ..."` or `jq`). Capture: + +- `number`, `title`, `body`, `url` +- `headRefName`, `headRefOid`, `baseRefName` +- `closingIssuesReferences` (array of `{ number, title, url }` or empty) + +If `gh pr view` exits non-zero, surface its stderr and stop. + +### 3. Verify HEAD + +Compare the local HEAD against the PR's head commit: + +```bash +local_head=$(git rev-parse HEAD) +``` + +If `local_head` does not equal `headRefOid`, exit non-zero with: + +``` +PR #{number}'s head commit is {short(headRefOid)} but HEAD is at {short(local_head)}. Run "gh pr checkout {number}" first. +``` + +Use the first 7 characters of each SHA for the short form. **Fail closed** — never proceed with mismatched state. Compilation, dependency installation, and test execution all require the working tree to match the commit being reviewed. + +### 4. Resolve the diff base + +Apply this cascade: + +1. If `diff_base_override` is non-null, use it. +2. Otherwise, use `baseRefName` from PR metadata. + +Compute the merge-base once: + +```bash +merge_base_sha=$(git merge-base HEAD {diff_base}) +``` + +Cache `merge_base_sha` for the resolved-output record. + +### 5. Resolve the ticket + +Apply this cascade in order; the first match wins: + +1. **`ticket_override`** — if non-null, resolve per [ticket source resolution](../_data/ticket-source-resolution.md) and use it. +2. **First entry in `closingIssuesReferences`** — if the array is non-empty, fetch the first entry's content via `gh issue view --json number,title,body,labels {number}` and use it. +3. **Parse PR body for issue references** — scan `body` for the first match of any of these patterns (case-insensitive for keywords): + - `closes #{n}`, `closes: #{n}` + - `fixes #{n}`, `fixes: #{n}` + - `resolves #{n}`, `resolves: #{n}` + - bare `#{n}` + + Take the first match's number and fetch the issue via `gh issue view --json number,title,body,labels {number}`. + +4. **No ticket** — proceed with the PR description as the only spec source. + +### 6. Build the spec-source list + +Always include the PR description as a source: + +``` +{ + source_type: "pr_description", + label: "pr_description: PR #{number}", + content: <body>, + criteria: <optional — extracted bullets from `## What`, `## Summary`, or an explicit acceptance-criteria heading; null when no list is present> +} +``` + +If a ticket was resolved in step 5, prepend it: + +``` +{ + source_type: "ticket", + label: "ticket: {ticket_ref or short identifier}", + content: <ticket body>, + criteria: <optional — extracted from the ticket structure> +} +``` + +The list order is `[ticket?, pr_description]` — when both are present, the ticket is listed first because the ticket is the higher-authority source (PR description is a presentation of what the implementation delivers; the ticket states what was asked). + +### 7. Return the resolved-output record + +Return: + +``` +{ + merge_base_sha: <from step 4>, + diff_base: <from step 4>, + spec_sources: <from step 6>, + pr_metadata: { number, url, head_oid: headRefOid, base_ref: baseRefName, title } +} +``` + +`review-pr` passes this to `review-branch` and the review proceeds. + +## Important + +- **Single `gh pr view` call.** All fields are fetched at once. Do not split into multiple calls — repeated `gh` invocations are slow and add failure modes. +- **HEAD mismatch is a hard stop.** The error message must include the literal `gh pr checkout {number}` suggestion so the user has a one-line copy-pasteable fix. +- **Ticket-resolution cascade order is fixed.** `ticket_override` → `closingIssuesReferences[0]` → body parse → none. Document this order in any future change so future readers do not silently rearrange it. +- **No review logic here.** This delegate prepares inputs only. The review process — diff analysis, finding generation, "Specification compliance" rendering — runs inside `review-branch` after `review-pr` invokes it with the resolved inputs. diff --git a/packages/agents/content/skills/review-pr/SKILL.md b/packages/agents/content/skills/review-pr/SKILL.md new file mode 100644 index 00000000..b81bdfc1 --- /dev/null +++ b/packages/agents/content/skills/review-pr/SKILL.md @@ -0,0 +1,117 @@ +--- +name: review-pr +description: Review a pull request by detecting the platform, fetching PR metadata via a delegate, and running the shared branch-review process +user-invocable: true +--- + +# Review pull request + +Review a pull request on the appropriate platform. Detects the platform, dispatches to a delegate (`review-gh-pr` or `review-bb-pr`) that fetches PR metadata, verifies HEAD matches the PR's head commit, and resolves specification sources, then invokes the shared review process from `review-branch` with the resolved inputs. + +This is a thin entry skill: the shared review logic — diff analysis, finding generation, "Specification compliance" rendering, artifact saving — lives in `review-branch`. Delegates own only the platform-specific work (PR-metadata fetch, HEAD verification, ticket resolution from PR linked issues, PR-description preparation). After the delegate returns its resolved inputs, this skill invokes `review-branch`'s review process with the prepared spec-source list and resolved diff base. + +## Arguments + +| Argument | Description | Default | +| ------------------- | ------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------ | +| `<pr_id>` | Positional. PR number or full PR URL. URLs are parsed for `{owner}/{repo}#{number}` (GitHub) or the Bitbucket equivalent. | _(required)_ | +| `--diff-base=<ref>` | Override the diff base. Reviews `merge-base(HEAD, <ref>)..HEAD`. | The PR's `baseRefName` (or Bitbucket equivalent) | +| `--ticket=<source>` | Override the auto-resolved ticket. Resolved per [ticket source resolution](../_data/ticket-source-resolution.md). | PR's first linked issue, then PR-body parse, then none | + +## Process + +### 1. Get session context + +Use `get-session-context` to obtain `project_slug`, `ticket_id`, `artifact_base_dir`, and `platform`. + +### 2. Detect platform + +Apply the [platform resolution cascade](../_data/ticket-source-resolution.md#platform-resolution-cascade): + +1. Check `.agents/preferences.yaml` → `integrations` (if exactly one is enabled, use it; if multiple, ask). +2. Check `git remote get-url origin` (`github.com` → GitHub; `bitbucket.org` → Bitbucket). +3. Ask the user. + +If `<pr_id>` is a full URL, the URL host overrides the cascade — a `https://github.com/...` URL is GitHub regardless of preferences. Numeric `<pr_id>` inputs use the cascade-resolved platform. + +### 3. Select delegate + +| Platform | Delegate | +| ------------------------- | -------------- | +| `github` | `review-gh-pr` | +| `bitbucket` | `review-bb-pr` | +| Unknown after the cascade | Ask the user | + +### 4. Call delegate + +Pass the following inputs to the selected delegate per its delegate interface: + +| Input | Value | +| -------------------- | ----------------------------------------------------------------------------------------- | +| `pr_id` | Normalized to platform-native id (number for GitHub; workspace/repo+number for Bitbucket) | +| `diff_base_override` | Value of `--diff-base` if provided; otherwise `null` | +| `ticket_override` | Value of `--ticket` if provided; otherwise `null` | +| `project_slug` | From session context | +| `ticket_id` | From session context | +| `artifact_base_dir` | From session context | + +The delegate returns a resolved-input record: + +| Field | Type | Description | +| ---------------- | ----------------------------------------------------- | -------------------------------------------------------------------------- | +| `merge_base_sha` | string | Result of `git merge-base HEAD <diff-base>` after delegate-side resolution | +| `diff_base` | string | The ref the delegate resolved against | +| `spec_sources` | array of `{ source_type, label, content, criteria? }` | One entry per available specification source | +| `pr_metadata` | object (PR number, URL, head SHA, base ref, title) | Used by the review heading | + +If the delegate exits with a HEAD-mismatch error (the PR's head commit is not the local HEAD), surface its error message and stop. Do not proceed with mismatched state. + +### 5. Invoke the shared review process + +Invoke `review-branch`'s review process with the resolved inputs: + +- The diff base is `merge_base_sha` (already computed by the delegate). +- The spec-source list is `spec_sources` from the delegate. The "Specification compliance" section in the review output renders one subsection per entry. For a typical PR, this list contains both the ticket (when one was resolved) and the PR description as a `pr_description` source. +- The review heading uses `pr_metadata` to surface the PR number and URL alongside the ticket reference. + +The skill **does not duplicate** the review logic; it delegates to `review-branch`'s [Process](../review-branch/SKILL.md#process) starting at step 4 (read prior artifacts), passing the already-resolved spec sources and merge-base SHA so the inner steps 1–3 are short-circuited by inputs. + +### 6. Save and present next steps + +`review-branch`'s saving and next-steps logic apply unchanged. The review artifact lands in the active run directory for the ticket (or a new `{timestamp}-interactive` run directory if none). + +## Examples + +### Review the PR for the current branch + +```bash +# Implicit PR id when the current branch has exactly one open PR. +gh pr checkout 1024 +/review-pr 1024 +``` + +### Review a PR by URL + +```bash +gh pr checkout https://github.com/williamthorsen/codeassembly/pull/1024 +/review-pr https://github.com/williamthorsen/codeassembly/pull/1024 +``` + +### Review a stacked PR with a non-default diff base + +```bash +gh pr checkout 1024 +/review-pr 1024 --diff-base=feature/parent-branch +``` + +### Override the auto-resolved ticket + +```bash +/review-pr 1024 --ticket=#553 +``` + +## Important + +- **Always check out the PR first.** The delegate compares `git rev-parse HEAD` to the PR's head commit and fails closed if they differ. The error message includes the platform-specific checkout command (e.g., `gh pr checkout <n>`). +- **The orchestrator owns no review logic.** All findings, scoring, and the "Specification compliance" rendering happen inside `review-branch`. This skill is platform detection + delegate dispatch + invocation of the shared review process. +- **Two specification sources by default.** Unlike `/review-branch` (one source: the ticket), `/review-pr` adds the PR description as a second source so the review evaluates the implementation against both. A separate concern — calling out divergence between the ticket and the PR description as a specification-vs-specification check — is deferred (tracked separately). From c29e7781195b10dfb120a31528a3eced827e80ba Mon Sep 17 00:00:00 2001 From: William Thorsen <william@thorsen.dev> Date: Sun, 10 May 2026 08:56:21 -0700 Subject: [PATCH 3/7] agents|docs: Update review-change references to review-branch and review-pr MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace `/review-change` references with the appropriate successor in the two skill files that mention it. In `wrap-up/SKILL.md`, the Review session-type signal now matches output from either `/review-branch` or `/review-pr`. In `save-artifact/SKILL.md`, the list of skills that produce run artifacts now references `review-branch`, since that skill is the canonical home of the shared review process and the only run-artifact-producing entry skill among the new surface (the `review-pr` entry skill itself does not save artifacts; it delegates artifact saving to `review-branch`). `_data/artifact-conventions.md` was audited and contained no live references to update. Historical entries in `CHANGELOG.md` and `.meta/changelog.json` are intentionally left untouched — those record what happened at the time of those releases. --- packages/agents/content/skills/save-artifact/SKILL.md | 2 +- packages/agents/content/skills/wrap-up/SKILL.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/agents/content/skills/save-artifact/SKILL.md b/packages/agents/content/skills/save-artifact/SKILL.md index 81766f8c..bf371b2d 100644 --- a/packages/agents/content/skills/save-artifact/SKILL.md +++ b/packages/agents/content/skills/save-artifact/SKILL.md @@ -43,7 +43,7 @@ Save AI-generated files with standardized naming conventions. - **role**: Kebab-case identifier; hyphens are free within the name, underscores are reserved as structural separators. Each role has a `roleType` (one of: `orchestrator`, `analyst`, `planner`, `author`, `reviewer`). See [artifact-conventions.md](../_data/artifact-conventions.md#run-artifacts-review-workflow) for the current role list and [roleType taxonomy](../_data/artifact-conventions.md#roletype-taxonomy). - **artifact**: Kebab-case identifier following the same naming conventions. See [artifact-conventions.md](../_data/artifact-conventions.md#artifact-types) for the complete artifact type list. -Run artifacts are saved by the skills that produce them (`review-change`, `respond-to-review`). They handle run directory discovery and creation. +Run artifacts are saved by the skills that produce them (`review-branch`, `respond-to-review`). They handle run directory discovery and creation. > **Note:** In orchestrated runs, the orchestrator is responsible for maintaining `run-index.json` — individual skills do not write to it directly. diff --git a/packages/agents/content/skills/wrap-up/SKILL.md b/packages/agents/content/skills/wrap-up/SKILL.md index 7b360529..892c1c31 100644 --- a/packages/agents/content/skills/wrap-up/SKILL.md +++ b/packages/agents/content/skills/wrap-up/SKILL.md @@ -45,7 +45,7 @@ Check these signals in order to classify the session: | -------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------ | | Orchestrated run artifacts | Look for run subdirectories under the current ticket directory (resolve via `get-session-context` → ticket ID, then list subdirectories of `{artifact_base_dir}/projects/{project_slug}/tickets/{ticket_id}/` that contain `run-index.json`) | **Orchestrated** | | Code changes on branch | `git diff --name-only {default_branch}...HEAD` produces output | **Interactive dev** | -| Review artifacts in conversation | Conversation contains review findings or `/review-change` output | **Review** | +| Review artifacts in conversation | Conversation contains review findings or `/review-branch` / `/review-pr` output | **Review** | | None of the above | No code changes, no run artifacts, no review artifacts | **Research/exploration** | Check from top to bottom. Use the first match. If an orchestrated run also has interactive changes after the run, treat it as orchestrated (the run-summary already captured the orchestrated portion). From 874977e38becab84b3f79cb79d5826ebc5002809 Mon Sep 17 00:00:00 2001 From: William Thorsen <william@thorsen.dev> Date: Sun, 10 May 2026 09:08:53 -0700 Subject: [PATCH 4/7] agents|docs: Clarify review-pr/review-branch invocation contracts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Make the inter-skill contract between `review-pr` and `review-branch` self-describing for both invocation paths. In `review-pr`, expand step 1 to also obtain `ticket_ref` and `default_branch` from session context, and replace the terse "starting at step 4" instruction with explicit text stating that `review-branch`'s steps 1–3 are already complete: session context was gathered above and remains in scope, and `merge_base_sha` and `spec_sources` come from the delegate. In `review-branch`, add a note after step 3 documenting that when invoked by `review-pr`, the caller provides `merge_base_sha` and `spec_sources` so steps 2–3 are skipped. Correct the `<pr_id>` argument description in `review-pr` from a misleading `{owner}/{repo}#{number}` shorthand notation (which `review-gh-pr` does not parse) to "Full GitHub or Bitbucket PR URL, or a bare PR number," matching what the delegates actually accept. Replace the contradictory comment in the first usage example ("# Implicit PR id when the current branch has exactly one open PR.") — which conflicts with the explicit `1024` argument on the next line — with an accurate "# Check out the PR branch first, then review it." Specify behavior in `review-branch`'s saving step when `ticket_id` is null: auto-generate a ticket id in the documented `{YYYYMMDD}-{4 random hex}` format from artifact-conventions.md, matching the project-wide convention used by `plan` and `plan-orchestrable-steps`. This prevents a literal `tickets/null/` path on branches with no recognizable ticket prefix. --- .../agents/content/skills/review-branch/SKILL.md | 4 +++- .../agents/content/skills/review-pr/SKILL.md | 16 ++++++++-------- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/packages/agents/content/skills/review-branch/SKILL.md b/packages/agents/content/skills/review-branch/SKILL.md index c6390ea8..190f0570 100644 --- a/packages/agents/content/skills/review-branch/SKILL.md +++ b/packages/agents/content/skills/review-branch/SKILL.md @@ -28,6 +28,8 @@ This skill is the canonical home of the shared review process. `review-pr` invok `review-pr` may pass additional sources (notably the PR description as `pr_description`). The list is the canonical input for the "Specification compliance" section regardless of who populated it. +> **When invoked by `review-pr`:** `merge_base_sha` and `spec_sources` are provided by the caller. Skip steps 2 and 3 and use the provided values; the session-context values from step 1 (gathered by `review-pr`) remain in scope. + 4. **Read prior artifacts** — if a run directory exists for this ticket, read all artifacts chronologically for context (including any prior dispositions). 5. **Analyze changes**: `git diff <merge-base-sha>..HEAD`. 6. **Review thoroughly** following the guidelines below. @@ -161,7 +163,7 @@ Follow [artifact conventions](../_data/artifact-conventions.md). The review is saved as a run artifact: `{timestamp}_reviewer_review.md` -1. Resolve ticket directory: `{artifact_base_dir}/projects/{project_slug}/tickets/{ticket_id}/` +1. Resolve ticket directory: `{artifact_base_dir}/projects/{project_slug}/tickets/{ticket_id}/`. If `ticket_id` is null (e.g., a branch with no recognizable ticket prefix), auto-generate one in the format `{YYYYMMDD}-{4 random hex}` per [artifact conventions](../_data/artifact-conventions.md#ticket-id) and use it for this review's directory. Never produce a path containing `tickets/null/`. 2. Find or create a run directory: - **If an active run exists** (the most recent run directory whose `run-index.json` has `context.branch` matching the current branch AND `completedAt` is absent): save into it - **If no active run exists**: create a new run directory named `{timestamp}-interactive` where timestamp matches this review's timestamp diff --git a/packages/agents/content/skills/review-pr/SKILL.md b/packages/agents/content/skills/review-pr/SKILL.md index b81bdfc1..b360fd5c 100644 --- a/packages/agents/content/skills/review-pr/SKILL.md +++ b/packages/agents/content/skills/review-pr/SKILL.md @@ -12,17 +12,17 @@ This is a thin entry skill: the shared review logic — diff analysis, finding g ## Arguments -| Argument | Description | Default | -| ------------------- | ------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------ | -| `<pr_id>` | Positional. PR number or full PR URL. URLs are parsed for `{owner}/{repo}#{number}` (GitHub) or the Bitbucket equivalent. | _(required)_ | -| `--diff-base=<ref>` | Override the diff base. Reviews `merge-base(HEAD, <ref>)..HEAD`. | The PR's `baseRefName` (or Bitbucket equivalent) | -| `--ticket=<source>` | Override the auto-resolved ticket. Resolved per [ticket source resolution](../_data/ticket-source-resolution.md). | PR's first linked issue, then PR-body parse, then none | +| Argument | Description | Default | +| ------------------- | ----------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------ | +| `<pr_id>` | Positional. Full GitHub or Bitbucket PR URL, or a bare PR number. | _(required)_ | +| `--diff-base=<ref>` | Override the diff base. Reviews `merge-base(HEAD, <ref>)..HEAD`. | The PR's `baseRefName` (or Bitbucket equivalent) | +| `--ticket=<source>` | Override the auto-resolved ticket. Resolved per [ticket source resolution](../_data/ticket-source-resolution.md). | PR's first linked issue, then PR-body parse, then none | ## Process ### 1. Get session context -Use `get-session-context` to obtain `project_slug`, `ticket_id`, `artifact_base_dir`, and `platform`. +Use `get-session-context` to obtain `project_slug`, `ticket_id`, `ticket_ref`, `default_branch`, `artifact_base_dir`, and `platform`. These values are carried forward into `review-branch`'s steps 4–9 (review header, scoring, saving) so that `review-branch`'s own step 1 does not need to re-run. ### 2. Detect platform @@ -74,7 +74,7 @@ Invoke `review-branch`'s review process with the resolved inputs: - The spec-source list is `spec_sources` from the delegate. The "Specification compliance" section in the review output renders one subsection per entry. For a typical PR, this list contains both the ticket (when one was resolved) and the PR description as a `pr_description` source. - The review heading uses `pr_metadata` to surface the PR number and URL alongside the ticket reference. -The skill **does not duplicate** the review logic; it delegates to `review-branch`'s [Process](../review-branch/SKILL.md#process) starting at step 4 (read prior artifacts), passing the already-resolved spec sources and merge-base SHA so the inner steps 1–3 are short-circuited by inputs. +The skill **does not duplicate** the review logic; it delegates to `review-branch`'s [Process](../review-branch/SKILL.md#process) starting at step 4 (read prior artifacts). Steps 1–3 of `review-branch` are already complete: session context (`ticket_ref`, `project_slug`, `artifact_base_dir`, `default_branch`) was gathered in step 1 above and remains in scope for the review header, scoring, and saving; `merge_base_sha` and `spec_sources` were resolved by the delegate and are passed in directly. ### 6. Save and present next steps @@ -85,7 +85,7 @@ The skill **does not duplicate** the review logic; it delegates to `review-branc ### Review the PR for the current branch ```bash -# Implicit PR id when the current branch has exactly one open PR. +# Check out the PR branch first, then review it. gh pr checkout 1024 /review-pr 1024 ``` From f90f2695f9e1398a24223f88d27b953c4d127cfc Mon Sep 17 00:00:00 2001 From: William Thorsen <william@thorsen.dev> Date: Sun, 10 May 2026 09:16:15 -0700 Subject: [PATCH 5/7] agents|refactor: Tighten prose in review-pr skill family Drop redundant boundary restatement, embedded maintainer rationale, and explanatory paragraphs that duplicated information already present in adjacent sections. In `review-gh-pr`, drop the "never produces findings, never saves..." sentence that restated the boundary already established by the first intro sentence and the `## Important` "No review logic here" bullet, and drop the trailing "Cache `merge_base_sha` for the resolved-output record." note since the variable's role is conveyed by the step 7 return-record spec. In `review-branch`, drop the two-sentence maintainer note about not re-adding a commit-scope path (the `--diff-base` flag in the Arguments table already communicates the commit-review escape hatch), and replace the redundant `get-session-context` call in the saving section with a back-reference to step 1. In `review-pr` step 5, compress the explanatory paragraph to a single sentence that retains the operational instruction and brief reason. In `review-bb-pr`, drop the two-sentence rationale paragraph after the fail-closed instruction in step 3, since the error message template already conveys the user-facing checkout path. --- packages/agents/content/skills/review-bb-pr/SKILL.md | 2 -- packages/agents/content/skills/review-branch/SKILL.md | 4 ++-- packages/agents/content/skills/review-gh-pr/SKILL.md | 4 +--- packages/agents/content/skills/review-pr/SKILL.md | 2 +- 4 files changed, 4 insertions(+), 8 deletions(-) diff --git a/packages/agents/content/skills/review-bb-pr/SKILL.md b/packages/agents/content/skills/review-bb-pr/SKILL.md index 3f4748a6..ff1729e2 100644 --- a/packages/agents/content/skills/review-bb-pr/SKILL.md +++ b/packages/agents/content/skills/review-bb-pr/SKILL.md @@ -84,8 +84,6 @@ PR #{number}'s head commit is {short(source_commit_hash)} but HEAD is at {short( Use the first 7 characters for short SHAs. **Fail closed** — never proceed with mismatched state. -The Bitbucket equivalent of `gh pr checkout` is not a single CLI subcommand, so the message lists the canonical fetch-and-checkout pair. Bitbucket does not expose a numeric `pull-requests/{n}/from` ref by default in every workflow — if a project uses a different convention, the user can substitute `git checkout {source_branch_name}` and pull the latest. - ### 4. Resolve the diff base Apply this cascade: diff --git a/packages/agents/content/skills/review-branch/SKILL.md b/packages/agents/content/skills/review-branch/SKILL.md index 190f0570..09e97b6b 100644 --- a/packages/agents/content/skills/review-branch/SKILL.md +++ b/packages/agents/content/skills/review-branch/SKILL.md @@ -8,7 +8,7 @@ user-invocable: true Act as a conscientious code reviewer for the changes on the current branch relative to a diff base. Review the diff `merge-base(HEAD, <diff-base>)..HEAD`. -This skill is the canonical home of the shared review process. `review-pr` invokes the same review process after resolving platform-specific inputs (PR metadata, HEAD verification, ticket from PR linked issues, PR description as a second specification source). Do not re-add a commit-scope path: to review a specific commit or range, check out the desired commit and invoke `/review-branch --diff-base=<ref>~1`. Compilation, dependency installation, and test execution all require the working tree to match the commit being reviewed, so requiring a checkout is honest about the prerequisites and removes a redundant entry point. +This skill is the canonical home of the shared review process. `review-pr` invokes the same review process after resolving platform-specific inputs (PR metadata, HEAD verification, ticket from PR linked issues, PR description as a second specification source). ## Arguments @@ -155,7 +155,7 @@ Extract criteria from whatever structure the source uses (numbered lists, checkb ### Path resolution -Use `get-session-context` to obtain `artifact_base_dir`, `project_slug`, and `ticket_id`. +Use `artifact_base_dir`, `project_slug`, and `ticket_id` from step 1. Follow [artifact conventions](../_data/artifact-conventions.md). diff --git a/packages/agents/content/skills/review-gh-pr/SKILL.md b/packages/agents/content/skills/review-gh-pr/SKILL.md index 4372a4ad..11c17594 100644 --- a/packages/agents/content/skills/review-gh-pr/SKILL.md +++ b/packages/agents/content/skills/review-gh-pr/SKILL.md @@ -8,7 +8,7 @@ user-invocable: false Internal delegate that handles the GitHub-specific work for `review-pr`: fetch PR metadata in a single `gh pr view` call, verify the local HEAD matches the PR's head commit, resolve the ticket from PR linked issues (with body-parse fallback), and prepare the spec-source list. Returns a resolved-input record that `review-pr` passes to `review-branch`'s shared review process. -This skill does not run a review. The review logic lives in `review-branch`. This skill prepares inputs and verifies preconditions; it never produces findings, never saves a review artifact, and never calls anything beyond `gh` and `git`. +This skill does not run a review. The review logic lives in `review-branch`. ## Delegate interface @@ -85,8 +85,6 @@ Compute the merge-base once: merge_base_sha=$(git merge-base HEAD {diff_base}) ``` -Cache `merge_base_sha` for the resolved-output record. - ### 5. Resolve the ticket Apply this cascade in order; the first match wins: diff --git a/packages/agents/content/skills/review-pr/SKILL.md b/packages/agents/content/skills/review-pr/SKILL.md index b360fd5c..2505cf0e 100644 --- a/packages/agents/content/skills/review-pr/SKILL.md +++ b/packages/agents/content/skills/review-pr/SKILL.md @@ -74,7 +74,7 @@ Invoke `review-branch`'s review process with the resolved inputs: - The spec-source list is `spec_sources` from the delegate. The "Specification compliance" section in the review output renders one subsection per entry. For a typical PR, this list contains both the ticket (when one was resolved) and the PR description as a `pr_description` source. - The review heading uses `pr_metadata` to surface the PR number and URL alongside the ticket reference. -The skill **does not duplicate** the review logic; it delegates to `review-branch`'s [Process](../review-branch/SKILL.md#process) starting at step 4 (read prior artifacts). Steps 1–3 of `review-branch` are already complete: session context (`ticket_ref`, `project_slug`, `artifact_base_dir`, `default_branch`) was gathered in step 1 above and remains in scope for the review header, scoring, and saving; `merge_base_sha` and `spec_sources` were resolved by the delegate and are passed in directly. +Invoke `review-branch`'s [Process](../review-branch/SKILL.md#process) starting at step 4 (read prior artifacts). Steps 1–3 of `review-branch` are already complete: session context was gathered in step 1 above; `merge_base_sha` and `spec_sources` were resolved by the delegate. ### 6. Save and present next steps From 8ef1fdce4d9c64c453e3f2c7bfb29a2d03a9658f Mon Sep 17 00:00:00 2001 From: William Thorsen <william@thorsen.dev> Date: Sun, 10 May 2026 09:44:19 -0700 Subject: [PATCH 6/7] agents|docs: Tighten review-branch step ordering and prose Move the "when invoked by review-pr" override blockquote to the top of the Process section so a linear executor sees it before running steps 2-3. Tighten the null-ticket fallback prose in the saving section. --- packages/agents/content/skills/review-branch/SKILL.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/agents/content/skills/review-branch/SKILL.md b/packages/agents/content/skills/review-branch/SKILL.md index 09e97b6b..50592840 100644 --- a/packages/agents/content/skills/review-branch/SKILL.md +++ b/packages/agents/content/skills/review-branch/SKILL.md @@ -19,6 +19,8 @@ This skill is the canonical home of the shared review process. `review-pr` invok ## Process +> **When invoked by `review-pr`:** Steps 1–3 are already complete — `review-pr` performed `get-session-context` and the platform delegate resolved `merge_base_sha` and `spec_sources`. Begin at step 4 with these values in scope. + 1. **Get context** using `get-session-context` to obtain `default_branch`, `ticket_id`, `ticket_ref`, `project_slug`, and `artifact_base_dir`. 2. **Resolve diff base** — if `--diff-base=<ref>` was provided, use `<ref>`; otherwise use `default_branch`. Compute the merge-base SHA once: `git merge-base HEAD <diff-base>`. Use this SHA for the diff command in step 5. 3. **Resolve specification sources** — produce a list of spec sources (each a `{ source_type, label, content, criteria? }` record): @@ -28,8 +30,6 @@ This skill is the canonical home of the shared review process. `review-pr` invok `review-pr` may pass additional sources (notably the PR description as `pr_description`). The list is the canonical input for the "Specification compliance" section regardless of who populated it. -> **When invoked by `review-pr`:** `merge_base_sha` and `spec_sources` are provided by the caller. Skip steps 2 and 3 and use the provided values; the session-context values from step 1 (gathered by `review-pr`) remain in scope. - 4. **Read prior artifacts** — if a run directory exists for this ticket, read all artifacts chronologically for context (including any prior dispositions). 5. **Analyze changes**: `git diff <merge-base-sha>..HEAD`. 6. **Review thoroughly** following the guidelines below. @@ -163,7 +163,7 @@ Follow [artifact conventions](../_data/artifact-conventions.md). The review is saved as a run artifact: `{timestamp}_reviewer_review.md` -1. Resolve ticket directory: `{artifact_base_dir}/projects/{project_slug}/tickets/{ticket_id}/`. If `ticket_id` is null (e.g., a branch with no recognizable ticket prefix), auto-generate one in the format `{YYYYMMDD}-{4 random hex}` per [artifact conventions](../_data/artifact-conventions.md#ticket-id) and use it for this review's directory. Never produce a path containing `tickets/null/`. +1. Resolve ticket directory: `{artifact_base_dir}/projects/{project_slug}/tickets/{ticket_id}/`. When `ticket_id` is null, auto-generate one in the format `{YYYYMMDD}-{4 random hex}` per [artifact conventions](../_data/artifact-conventions.md#ticket-id) — never construct a path with a literal `null` segment. 2. Find or create a run directory: - **If an active run exists** (the most recent run directory whose `run-index.json` has `context.branch` matching the current branch AND `completedAt` is absent): save into it - **If no active run exists**: create a new run directory named `{timestamp}-interactive` where timestamp matches this review's timestamp From 9fa85a6bb864570947daabff46ac830806c2179a Mon Sep 17 00:00:00 2001 From: William Thorsen <william@thorsen.dev> Date: Sun, 10 May 2026 10:20:36 -0700 Subject: [PATCH 7/7] agents|docs: Correct delegate-orchestrator boundary descriptions Update the pr_id row in review-pr's delegate-input table to describe the actual division of labor: the orchestrator passes pr_id through verbatim, and the delegates parse URLs (with the Bitbucket delegate also auto-detecting workspace/repo from git remote). Drop create-bitbucket-pr from review-bb-pr's access-mechanism precedent reference. That skill is intentionally agnostic about access mechanism, so citing it as a REST-API precedent misrepresents its stance. The bb-pr-inline-comment reference is the accurate precedent. --- .../agents/content/skills/review-bb-pr/SKILL.md | 2 +- .../agents/content/skills/review-pr/SKILL.md | 16 ++++++++-------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/packages/agents/content/skills/review-bb-pr/SKILL.md b/packages/agents/content/skills/review-bb-pr/SKILL.md index ff1729e2..1cabf2bb 100644 --- a/packages/agents/content/skills/review-bb-pr/SKILL.md +++ b/packages/agents/content/skills/review-bb-pr/SKILL.md @@ -36,7 +36,7 @@ On HEAD mismatch, do not return — exit non-zero with the mismatch error. `revi ## Bitbucket access -Use the same access mechanism as `bb-pr-inline-comment` and `create-bitbucket-pr` — the Bitbucket Cloud REST API at `https://api.bitbucket.org/2.0/`. **Do not introduce a new client.** Authentication resolves in priority order: +Use the same access mechanism as `bb-pr-inline-comment` — the Bitbucket Cloud REST API at `https://api.bitbucket.org/2.0/`. **Do not introduce a new client.** Authentication resolves in priority order: 1. **Bot credentials (Basic auth):** `BITBUCKET_BOT_USERNAME` + `BITBUCKET_BOT_TOKEN` env vars. 2. **API token (Bearer auth):** `BITBUCKET_API_TOKEN` env var. diff --git a/packages/agents/content/skills/review-pr/SKILL.md b/packages/agents/content/skills/review-pr/SKILL.md index 2505cf0e..3fa97bae 100644 --- a/packages/agents/content/skills/review-pr/SKILL.md +++ b/packages/agents/content/skills/review-pr/SKILL.md @@ -46,14 +46,14 @@ If `<pr_id>` is a full URL, the URL host overrides the cascade — a `https://gi Pass the following inputs to the selected delegate per its delegate interface: -| Input | Value | -| -------------------- | ----------------------------------------------------------------------------------------- | -| `pr_id` | Normalized to platform-native id (number for GitHub; workspace/repo+number for Bitbucket) | -| `diff_base_override` | Value of `--diff-base` if provided; otherwise `null` | -| `ticket_override` | Value of `--ticket` if provided; otherwise `null` | -| `project_slug` | From session context | -| `ticket_id` | From session context | -| `artifact_base_dir` | From session context | +| Input | Value | +| -------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `pr_id` | Passed through verbatim. The delegate parses and normalizes it (extracts the PR number from a URL when applicable; the Bitbucket delegate also auto-detects workspace/repo from `git remote get-url origin`). | +| `diff_base_override` | Value of `--diff-base` if provided; otherwise `null` | +| `ticket_override` | Value of `--ticket` if provided; otherwise `null` | +| `project_slug` | From session context | +| `ticket_id` | From session context | +| `artifact_base_dir` | From session context | The delegate returns a resolved-input record: