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..1cabf2bb --- /dev/null +++ b/packages/agents/content/skills/review-bb-pr/SKILL.md @@ -0,0 +1,168 @@ +--- +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` — 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. + +### 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-branch/SKILL.md b/packages/agents/content/skills/review-branch/SKILL.md new file mode 100644 index 00000000..50592840 --- /dev/null +++ b/packages/agents/content/skills/review-branch/SKILL.md @@ -0,0 +1,172 @@ +--- +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, <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). + +## Arguments + +| Flag | Effect | Default | +| ------------------- | ------------------------------------------------------------------------------------------------------------------------------------------ | ------------------------- | +| `--diff-base=<ref>` | Reference to diff against. Reviews `merge-base(HEAD, <ref>)..HEAD`. | Project's default branch | +| `--ticket=<source>` | Ticket or requirements to check the implementation against. Resolved per [ticket source resolution](../_data/ticket-source-resolution.md). | Auto-resolved (see below) | + +## 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): + - **Explicit `--ticket=<source>`**: 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 <merge-base-sha>..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 `artifact_base_dir`, `project_slug`, and `ticket_id` from step 1. + +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}/`. 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 +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 [<ref>]`: Review a specific commit (HEAD if ref omitted) -- `ticket <source>` _(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 <ref>^..<ref>` (or `git show <ref>` 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 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..11c17594 --- /dev/null +++ b/packages/agents/content/skills/review-gh-pr/SKILL.md @@ -0,0 +1,150 @@ +--- +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`. + +## 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}) +``` + +### 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..3fa97bae --- /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. 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`, `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 + +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` | 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: + +| 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. + +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 + +`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 +# Check out the PR branch first, then review it. +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). 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).