#553 Replace /review-change with /review-branch and /review-pr

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>`                       |
+| `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: <description>,
+  criteria: <optional — extracted from `## What`, `## Summary`, or an explicit acceptance-criteria heading>
+}
+```
+
+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]` — same as `review-gh-pr`.
+
+### 7. Return the resolved-output record
+
+```
+{
+  merge_base_sha: <from step 4>,
+  diff_base: <from step 4>,
+  spec_sources: <from step 6>,
+  pr_metadata: {
+    number: <id>,
+    url: <links.html.href>,
+    head_oid: <source.commit.hash>,
+    base_ref: <destination.branch.name>,
+    title: <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.

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.