diff --git a/.agents/skills/PR_WORKFLOW.md b/.agents/skills/PR_WORKFLOW.md index 1062f1fd4db..40306507355 100644 --- a/.agents/skills/PR_WORKFLOW.md +++ b/.agents/skills/PR_WORKFLOW.md @@ -1,18 +1,22 @@ -# PR Review Instructions +# PR Workflow for Maintainers Please read this in full and do not skip sections. +This is the single source of truth for the maintainer PR workflow. + +## Triage order + +Process PRs **oldest to newest**. Older PRs are more likely to have merge conflicts and stale dependencies; resolving them first keeps the queue healthy and avoids snowballing rebase pain. ## Working rule -Skills execute workflow, maintainers provide judgment. +Skills execute workflow. Maintainers provide judgment. Always pause between skills to evaluate technical direction, not just command success. -Default mode is local-first, do not write to GitHub until maintainer explicitly says go. These three skills must be used in order: -1. `review-pr` -2. `prepare-pr` -3. `merge-pr` +1. `review-pr` — review only, produce findings +2. `prepare-pr` — rebase, fix, gate, push to PR head branch +3. `merge-pr` — squash-merge, verify MERGED state, clean up They are necessary, but not sufficient. Maintainers must steer between steps and understand the code before moving forward. @@ -21,26 +25,64 @@ If submitted code is low quality, ignore it and implement the best solution for Do not continue if you cannot verify the problem is real or test the fix. -## Remote write policy +## Script-first contract -Until the maintainer explicitly approves remote actions, stay local-only. +Skill runs should invoke these wrappers automatically. You only need to run them manually when debugging or doing an explicit script-only run: -Remote actions include: +- `scripts/pr-review ` +- `scripts/pr review-checkout-main ` or `scripts/pr review-checkout-pr ` while reviewing +- `scripts/pr review-guard ` before writing review outputs +- `scripts/pr review-validate-artifacts ` after writing outputs +- `scripts/pr-prepare init ` +- `scripts/pr-prepare validate-commit ` +- `scripts/pr-prepare gates ` +- `scripts/pr-prepare push ` +- Optional one-shot prepare: `scripts/pr-prepare run ` +- `scripts/pr-merge ` (verify-only; short form remains backward compatible) +- `scripts/pr-merge verify ` (verify-only) +- Optional one-shot merge: `scripts/pr-merge run ` -- Pushing branches. -- Posting PR comments. -- Editing PR metadata (labels, assignees, state). -- Merging PRs. -- Editing advisory state or publishing advisories. +These wrappers run shared preflight checks and generate deterministic artifacts. They are designed to work from repo root or PR worktree cwd. -Allowed before approval: +## Required artifacts -- Local code changes. -- Local tests and validation. -- Drafting copy for PR/advisory comments. -- Read-only `gh` commands. +- `.local/pr-meta.json` and `.local/pr-meta.env` from review init. +- `.local/review.md` and `.local/review.json` from review output. +- `.local/prep-context.env` and `.local/prep.md` from prepare. +- `.local/prep.env` from prepare completion. -When approved, perform only the approved remote action, then pause for next instruction. +## Structured review handoff + +`review-pr` must write `.local/review.json`. +In normal skill runs this is handled automatically. Use `scripts/pr review-artifacts-init ` and `scripts/pr review-tests ...` manually only for debugging or explicit script-only runs. + +Minimum schema: + +```json +{ + "recommendation": "READY FOR /prepare-pr", + "findings": [ + { + "id": "F1", + "severity": "IMPORTANT", + "title": "Missing changelog entry", + "area": "CHANGELOG.md", + "fix": "Add a Fixes entry for PR #" + } + ], + "tests": { + "ran": ["pnpm test -- ..."], + "gaps": ["..."], + "result": "pass" + } +} +``` + +`prepare-pr` resolves all `BLOCKER` and `IMPORTANT` findings from this file. + +## Coding Agent + +Use ChatGPT 5.3 Codex High. Fall back to 5.2 Codex High or 5.3 Codex Medium if necessary. ## PR quality bar @@ -53,6 +95,60 @@ When approved, perform only the approved remote action, then pause for next inst - Harden changes. Always evaluate security impact and abuse paths. - Understand the system before changing it. Never make the codebase messier just to clear a PR queue. +## Rebase and conflict resolution + +Before any substantive review or prep work, **always rebase the PR branch onto current `main` and resolve merge conflicts first**. A PR that cannot cleanly rebase is not ready for review — fix conflicts before evaluating correctness. + +- During `prepare-pr`: rebase onto `main` as the first step, before fixing findings or running gates. +- If conflicts are complex or touch areas you do not understand, stop and escalate. +- Prefer **rebase** for linear history; **squash** when commit history is messy or unhelpful. + +## Commit and changelog rules + +- In normal `prepare-pr` runs, commits are created via `scripts/committer "" `. Use it manually only when operating outside the skill flow; avoid manual `git add`/`git commit` so staging stays scoped. +- Follow concise, action-oriented commit messages (e.g., `CLI: add verbose flag to send`). +- During `prepare-pr`, use concise, action-oriented subjects **without** PR numbers or thanks; reserve `(#) thanks @` for the final merge/squash commit. +- Group related changes; avoid bundling unrelated refactors. +- Changelog workflow: keep the latest released version at the top (no `Unreleased`); after publishing, bump the version and start a new top section. +- When working on a PR: add a changelog entry with the PR number and thank the contributor (mandatory in this workflow). +- When working on an issue: reference the issue in the changelog entry. +- In this workflow, changelog is always required even for internal/test-only changes. + +## Gate policy + +In fresh worktrees, dependency bootstrap is handled by wrappers before local gates. Manual equivalent: + +```sh +pnpm install --frozen-lockfile +``` + +Gate set: + +- Always: `pnpm build`, `pnpm check` +- `pnpm test` required unless high-confidence docs-only criteria pass. + +## Co-contributor and clawtributors + +- If we squash, add the PR author as a co-contributor in the commit body using a `Co-authored-by:` trailer. +- When maintainer prepares and merges the PR, add the maintainer as an additional `Co-authored-by:` trailer too. +- Avoid `--auto` merges for maintainer landings. Merge only after checks are green so the maintainer account is the actor and attribution is deterministic. +- For squash merges, set `--author-email` to a reviewer-owned email with fallback candidates; if merge fails due to author-email validation, retry once with the next candidate. +- If you review a PR and later do work on it, land via merge/squash (no direct-main commits) and always add the PR author as a co-contributor. +- When merging a PR: leave a PR comment that explains exactly what we did, include the SHA hashes, and record the comment URL in the final report. +- Manual post-merge step for new contributors: run `bun scripts/update-clawtributors.ts` to add their avatar to the README "Thanks to all clawtributors" list, then commit the regenerated README. + +## Review mode vs landing mode + +- **Review mode (PR link only):** read `gh pr view`/`gh pr diff`; **do not** switch branches; **do not** change code. +- **Landing mode (exception path):** use only when normal `review-pr -> prepare-pr -> merge-pr` flow cannot safely preserve attribution or cannot satisfy branch protection. Create an integration branch from `main`, bring in PR commits (**prefer rebase** for linear history; **merge allowed** when complexity/conflicts make it safer), apply fixes, add changelog (+ thanks + PR #), run full gate **locally before committing** (`pnpm build && pnpm check && pnpm test`), commit, merge back to `main`, then `git switch main` (never stay on a topic branch after landing). Important: the contributor needs to be in the git graph after this! + +## Pre-review safety checks + +- Before starting a review when a GH Issue/PR is pasted: `review-pr`/`scripts/pr-review` should create and use an isolated `.worktrees/pr-` checkout from `origin/main` automatically. Do not require a clean main checkout, and do not run `git pull` in a dirty main checkout. +- PR review calls: prefer a single `gh pr view --json ...` to batch metadata/comments; run `gh pr diff` only when needed. +- PRs should summarize scope, note testing performed, and mention any user-facing changes or new flags. +- Read `docs/help/submitting-a-pr.md` ([Submitting a PR](https://docs.openclaw.ai/help/submitting-a-pr)) for what we expect from contributors. + ## Unified workflow Entry criteria: @@ -78,7 +174,6 @@ Maintainer checkpoint before `prepare-pr`: ``` What problem are they trying to solve? What is the most optimal implementation? -Is the code properly scoped? Can we fix up everything? Do we have any questions? ``` @@ -94,27 +189,30 @@ Stop and escalate instead of continuing if: Purpose: - Make the PR merge-ready on its head branch. -- Rebase onto current `main`, fix blocker/important findings, and run gates. +- Rebase onto current `main` first, then fix blocker/important findings, then run gates. +- In fresh worktrees, bootstrap dependencies before local gates (`pnpm install --frozen-lockfile`). Expected output: - Updated code and tests on the PR head branch. - `.local/prep.md` with changes, verification, and current HEAD SHA. -- Final status: `PR is ready for /mergepr`. +- Final status: `PR is ready for /merge-pr`. Maintainer checkpoint before `merge-pr`: ``` Is this the most optimal implementation? Is the code properly scoped? +Is the code properly reusing existing logic in the codebase? Is the code properly typed? Is the code hardened? Do we have enough tests? -Are tests using fake timers where relevant? (e.g., debounce/throttle, retry backoff, timeout branches, delayed callbacks, polling loops) +Do we need regression tests? +Are tests using fake timers where appropriate? (e.g., debounce/throttle, retry backoff, timeout branches, delayed callbacks, polling loops) Do not add performative tests, ensure tests are real and there are no regressions. -Take your time, fix it properly, refactor if necessary. Do you see any follow-up refactors we should do? Did any changes introduce any potential security vulnerabilities? +Take your time, fix it properly, refactor if necessary. ``` Stop and escalate instead of continuing if: @@ -123,59 +221,29 @@ Stop and escalate instead of continuing if: - Fixing findings requires broad architecture changes outside safe PR scope. - Security hardening requirements remain unresolved. -### Security advisory companion flow - -Use this for GHSA-linked fixes and private reports. - -1. Implement and test the fix locally first, do not edit advisory content yet. -2. Land the code fix PR through normal flow, including attribution and changelog where needed. -3. Prepare public-safe advisory text: - - No internal workflow chatter. - - No unnecessary exploit detail. - - Clear impact, affected range, fixed range, remediation, credits. -4. In GitHub advisory UI, set package ranges in the structured fields: - - `Affected versions`: `< fixed_version` - - `Patched versions`: `>= fixed_version` - Do not rely on description text alone. -5. If collaborator can edit text but cannot change advisory state, hand off to a Publisher to move triage -> accepted draft -> publish. -6. Advisory comments are posted manually in UI when required by policy. Do not rely on `gh api` automation for advisory comments. - -Maintainer checkpoint for security advisories: - -- Is the rewrite public-safe and free of internal/process notes? -- Are affected and patched ranges correctly set in the advisory form fields? -- Are credits present and accurate? -- Do we have Publisher action if state controls are unavailable? - ### 3) `merge-pr` Purpose: - Merge only after review and prep artifacts are present and checks are green. -- Use squash merge flow and verify the PR ends in `MERGED` state. +- Use deterministic squash merge flow (`--match-head-commit` + explicit subject/body with co-author trailer), then verify the PR ends in `MERGED` state. +- If no required checks are configured on the PR, treat that as acceptable and continue after branch-up-to-date validation. Go or no-go checklist before merge: - All BLOCKER and IMPORTANT findings are resolved. - Verification is meaningful and regression risk is acceptably low. -- Docs and changelog are updated when required. +- Changelog is updated (mandatory) and docs are updated when required. - Required CI checks are green and the branch is not behind `main`. Expected output: - Successful merge commit and recorded merge SHA. - Worktree cleanup after successful merge. +- Comment on PR indicating merge was successful. Maintainer checkpoint after merge: - Were any refactors intentionally deferred and now need follow-up issue(s)? - Did this reveal broader architecture or test gaps we should address? - -## Chasing main mitigation - -To reduce repeated "branch behind main" loops: - -1. Keep prep and merge windows short. -2. Rebase/update once, as late as possible, right before final checks. -3. Avoid non-essential commits on the PR branch after checks start. -4. Prefer merge queue or auto-merge when available. +- Run `bun scripts/update-clawtributors.ts` if the contributor is new. diff --git a/.agents/skills/merge-pr/SKILL.md b/.agents/skills/merge-pr/SKILL.md index c007ff89cb2..041e79a6768 100644 --- a/.agents/skills/merge-pr/SKILL.md +++ b/.agents/skills/merge-pr/SKILL.md @@ -1,182 +1,99 @@ --- name: merge-pr -description: Merge a GitHub PR via squash after /preparepr. Use when asked to merge a ready PR. Do not push to main or modify code. Ensure the PR ends in MERGED state and clean up worktrees after success. +description: Script-first deterministic squash merge with strict required-check gating, head-SHA pinning, and reliable attribution/commenting. --- # Merge PR ## Overview -Merge a prepared PR via `gh pr merge --squash` and clean up the worktree after success. +Merge a prepared PR only after deterministic validation. ## Inputs - Ask for PR number or URL. -- If missing, auto-detect from conversation. -- If ambiguous, ask. +- If missing, use `.local/prep.env` from the PR worktree. ## Safety -- Use `gh pr merge --squash` as the only path to `main`. -- Do not run `git push` at all during merge. -- Do not run gateway stop commands. Do not kill processes. Do not touch port 18792. -- Do not execute merge or PR-comment GitHub write actions until maintainer explicitly approves. +- Never use `gh pr merge --auto` in this flow. +- Never run `git push` directly. +- Require `--match-head-commit` during merge. +- Wrapper commands are cwd-agnostic; you can run them from repo root or inside the PR worktree. -## Execution Rule +## Execution Contract -- Execute the workflow. Do not stop after printing the TODO checklist. -- If delegating, require the delegate to run commands and capture outputs. - -## Known Footguns - -- If you see "fatal: not a git repository", you are in the wrong directory. Use `~/dev/openclaw` if available; otherwise ask user. -- Read `.local/review.md` and `.local/prep.md` in the worktree. Do not skip. -- Clean up the real worktree directory `.worktrees/pr-` only after a successful merge. -- Expect cleanup to remove `.local/` artifacts. - -## Completion Criteria - -- Ensure `gh pr merge` succeeds. -- Ensure PR state is `MERGED`, never `CLOSED`. -- Record the merge SHA. -- Run cleanup only after merge success. - -## First: Create a TODO Checklist - -Create a checklist of all merge steps, print it, then continue and execute the commands. - -## Setup: Use a Worktree - -Use an isolated worktree for all merge work. +1. Validate merge readiness: ```sh -cd ~/dev/openclaw -# Sanity: confirm you are in the repo -git rev-parse --show-toplevel - -WORKTREE_DIR=".worktrees/pr-" +scripts/pr-merge verify ``` -Run all commands inside the worktree directory. - -## Load Local Artifacts (Mandatory) - -Expect these files from earlier steps: - -- `.local/review.md` from `/reviewpr` -- `.local/prep.md` from `/preparepr` +Backward-compatible verify form also works: ```sh -ls -la .local || true - -if [ -f .local/review.md ]; then - echo "Found .local/review.md" - sed -n '1,120p' .local/review.md -else - echo "Missing .local/review.md. Stop and run /reviewpr, then /preparepr." - exit 1 -fi - -if [ -f .local/prep.md ]; then - echo "Found .local/prep.md" - sed -n '1,120p' .local/prep.md -else - echo "Missing .local/prep.md. Stop and run /preparepr first." - exit 1 -fi +scripts/pr-merge ``` +2. Run one-shot deterministic merge: + +```sh +scripts/pr-merge run +``` + +3. Ensure output reports: + +- `merge_sha=` +- `merge_author_email=` +- `comment_url=` + ## Steps -1. Identify PR meta +1. Validate artifacts ```sh -gh pr view --json number,title,state,isDraft,author,headRefName,baseRefName,headRepository,body --jq '{number,title,state,isDraft,author:.author.login,head:.headRefName,base:.baseRefName,headRepo:.headRepository.nameWithOwner,body}' -contrib=$(gh pr view --json author --jq .author.login) -head=$(gh pr view --json headRefName --jq .headRefName) -head_repo_url=$(gh pr view --json headRepository --jq .headRepository.url) +require=(.local/review.md .local/review.json .local/prep.md .local/prep.env) +for f in "${require[@]}"; do + [ -s "$f" ] || { echo "Missing artifact: $f"; exit 1; } +done ``` -2. Run sanity checks - -Stop if any are true: - -- PR is a draft. -- Required checks are failing. -- Branch is behind main. +2. Validate checks and branch status ```sh -# Checks -gh pr checks - -# Check behind main -git fetch origin main -git fetch origin pull//head:pr- -git merge-base --is-ancestor origin/main pr- || echo "PR branch is behind main, run /preparepr" +scripts/pr-merge verify +source .local/prep.env ``` -If anything is failing or behind, stop and say to run `/preparepr`. +`scripts/pr-merge` treats “no required checks configured” as acceptable (`[]`), but fails on any required `fail` or `pending`. -3. Merge PR and delete branch - -If checks are still running, use `--auto` to queue the merge. +3. Merge deterministically (wrapper-managed) ```sh -# Check status first -check_status=$(gh pr checks 2>&1) -if echo "$check_status" | grep -q "pending\|queued"; then - echo "Checks still running, using --auto to queue merge" - gh pr merge --squash --delete-branch --auto - echo "Merge queued. Monitor with: gh pr checks --watch" -else - gh pr merge --squash --delete-branch -fi +scripts/pr-merge run ``` -Before running merge command, pause and ask for explicit maintainer go-ahead. +`scripts/pr-merge run` performs: -If merge fails, report the error and stop. Do not retry in a loop. -If the PR needs changes beyond what `/preparepr` already did, stop and say to run `/preparepr` again. +- deterministic squash merge pinned to `PREP_HEAD_SHA` +- reviewer merge author email selection with fallback candidates +- one retry only when merge fails due to author-email validation +- co-author trailers for PR author and reviewer +- post-merge verification of both co-author trailers on commit message +- PR comment retry (3 attempts), then comment URL extraction +- cleanup after confirmed `MERGED` -4. Get merge SHA +4. Manual fallback (only if wrapper is unavailable) ```sh -merge_sha=$(gh pr view --json mergeCommit --jq '.mergeCommit.oid') -echo "merge_sha=$merge_sha" +scripts/pr merge-run ``` -5. Optional comment +5. Cleanup -Use a literal multiline string or heredoc for newlines. - -```sh -gh pr comment --body "$(printf 'Merged via squash.\n\n- Merge commit: %s\n\nThanks @%s!\n' \"$merge_sha\" \"$contrib\")" -``` - -6. Verify PR state is MERGED - -```sh -gh pr view --json state --jq .state -``` - -7. Clean up worktree only on success - -Run cleanup only if step 6 returned `MERGED`. - -```sh -cd ~/dev/openclaw - -git worktree remove ".worktrees/pr-" --force - -git branch -D temp/pr- 2>/dev/null || true -git branch -D pr- 2>/dev/null || true -``` +Cleanup is handled by `run` after merge success. ## Guardrails -- Worktree only. -- Do not close PRs. -- End in MERGED state. -- Clean up only after merge success. -- Never push to main. Use `gh pr merge --squash` only. -- Do not run `git push` at all in this command. +- End in `MERGED`, never `CLOSED`. +- Cleanup only after confirmed merge. diff --git a/.agents/skills/prepare-pr/SKILL.md b/.agents/skills/prepare-pr/SKILL.md index dc1813f3da3..462e5bc2bd4 100644 --- a/.agents/skills/prepare-pr/SKILL.md +++ b/.agents/skills/prepare-pr/SKILL.md @@ -1,251 +1,122 @@ --- name: prepare-pr -description: Prepare a GitHub PR for merge by rebasing onto main, fixing review findings, running gates, committing fixes, and pushing to the PR head branch. Use after /reviewpr. Never merge or push to main. +description: Script-first PR preparation with structured findings resolution, deterministic push safety, and explicit gate execution. --- # Prepare PR ## Overview -Prepare a PR branch for merge with review fixes, green gates, and an updated head branch. +Prepare the PR head branch for merge after `/review-pr`. ## Inputs - Ask for PR number or URL. -- If missing, auto-detect from conversation. -- If ambiguous, ask. +- If missing, use `.local/pr-meta.env` if present in the PR worktree. ## Safety -- Never push to `main` or `origin/main`. Push only to the PR head branch. -- Never run `git push` without specifying remote and branch explicitly. Do not run bare `git push`. -- Do not run gateway stop commands. Do not kill processes. Do not touch port 18792. +- Never push to `main`. +- Only push to PR head with explicit `--force-with-lease` against known head SHA. - Do not run `git clean -fdx`. -- Do not run `git add -A` or `git add .`. Stage only specific files changed. -- Do not push to GitHub until the maintainer explicitly approves the push step. +- Wrappers are cwd-agnostic; run from repo root or PR worktree. -## Execution Rule +## Execution Contract -- Execute the workflow. Do not stop after printing the TODO checklist. -- If delegating, require the delegate to run commands and capture outputs. - -## Known Footguns - -- If you see "fatal: not a git repository", you are in the wrong directory. Use `~/dev/openclaw` if available; otherwise ask user. -- Do not run `git clean -fdx`. -- Do not run `git add -A` or `git add .`. - -## Completion Criteria - -- Rebase PR commits onto `origin/main`. -- Fix all BLOCKER and IMPORTANT items from `.local/review.md`. -- Run gates and pass. -- Commit prep changes. -- Push the updated HEAD back to the PR head branch. -- Write `.local/prep.md` with a prep summary. -- Output exactly: `PR is ready for /mergepr`. - -## First: Create a TODO Checklist - -Create a checklist of all prep steps, print it, then continue and execute the commands. - -## Setup: Use a Worktree - -Use an isolated worktree for all prep work. +1. Run setup: ```sh -cd ~/openclaw -# Sanity: confirm you are in the repo -git rev-parse --show-toplevel - -WORKTREE_DIR=".worktrees/pr-" +scripts/pr-prepare init ``` -Run all commands inside the worktree directory. +2. Resolve findings from structured review: -## Load Review Findings (Mandatory) +- `.local/review.json` is mandatory. +- Resolve all `BLOCKER` and `IMPORTANT` items. + +3. Commit scoped changes with concise subjects (no PR number/thanks; those belong on the final merge/squash commit). + +4. Run gates via wrapper. + +5. Push via wrapper (includes pre-push remote verification, one automatic lease-retry path, and post-push API propagation retry). + +Optional one-shot path: ```sh -if [ -f .local/review.md ]; then - echo "Found review findings from /reviewpr" -else - echo "Missing .local/review.md. Run /reviewpr first and save findings." - exit 1 -fi - -# Read it -sed -n '1,200p' .local/review.md +scripts/pr-prepare run ``` ## Steps -1. Identify PR meta (author, head branch, head repo URL) +1. Setup and artifacts ```sh -gh pr view --json number,title,author,headRefName,baseRefName,headRepository,body --jq '{number,title,author:.author.login,head:.headRefName,base:.baseRefName,headRepo:.headRepository.nameWithOwner,body}' -contrib=$(gh pr view --json author --jq .author.login) -head=$(gh pr view --json headRefName --jq .headRefName) -head_repo_url=$(gh pr view --json headRepository --jq .headRepository.url) +scripts/pr-prepare init + +ls -la .local/review.md .local/review.json .local/pr-meta.env .local/prep-context.env +jq . .local/review.json >/dev/null ``` -2. Fetch the PR branch tip into a local ref +2. Resolve required findings + +List required items: ```sh -git fetch origin pull//head:pr- +jq -r '.findings[] | select(.severity=="BLOCKER" or .severity=="IMPORTANT") | "- [\(.severity)] \(.id): \(.title) => \(.fix)"' .local/review.json ``` -3. Rebase PR commits onto latest main +Fix all required findings. Keep scope tight. + +3. Update changelog/docs (changelog is mandatory in this workflow) ```sh -# Move worktree to the PR tip first -git reset --hard pr- - -# Rebase onto current main -git fetch origin main -git rebase origin/main +jq -r '.changelog' .local/review.json +jq -r '.docs' .local/review.json ``` -If conflicts happen: +4. Commit scoped changes -- Resolve each conflicted file. -- Run `git add ` for each file. -- Run `git rebase --continue`. +Use concise, action-oriented subject lines without PR numbers/thanks. The final merge/squash commit is the only place we include PR numbers and contributor thanks. -If the rebase gets confusing or you resolve conflicts 3 or more times, stop and report. - -4. Fix issues from `.local/review.md` - -- Fix all BLOCKER and IMPORTANT items. -- NITs are optional. -- Keep scope tight. - -Keep a running log in `.local/prep.md`: - -- List which review items you fixed. -- List which files you touched. -- Note behavior changes. - -5. Update `CHANGELOG.md` if flagged in review - -Check `.local/review.md` section H for guidance. -If flagged and user-facing: - -- Check if `CHANGELOG.md` exists. +Use explicit file list: ```sh -ls CHANGELOG.md 2>/dev/null +scripts/committer "fix: " ... ``` -- Follow existing format. -- Add a concise entry with PR number and contributor. - -6. Update docs if flagged in review - -Check `.local/review.md` section G for guidance. -If flagged, update only docs related to the PR changes. - -7. Commit prep fixes - -Stage only specific files: +5. Run gates ```sh -git add ... +scripts/pr-prepare gates ``` -Preferred commit tool: +6. Push safely to PR head ```sh -committer "fix: (#) (thanks @$contrib)" +scripts/pr-prepare push ``` -If `committer` is not found: +This push step includes: + +- robust fork remote resolution from owner/name, +- pre-push remote SHA verification, +- one automatic rebase + gate rerun + retry if lease push fails, +- post-push PR-head propagation retry, +- idempotent behavior when local prep HEAD is already on the PR head, +- post-push SHA verification and `.local/prep.env` generation. + +7. Verify handoff artifacts ```sh -git commit -m "fix: (#) (thanks @$contrib)" +ls -la .local/prep.md .local/prep.env ``` -8. Run full gates before pushing +8. Output -```sh -pnpm install -pnpm build -pnpm ui:build -pnpm check -pnpm test -``` - -Require all to pass. If something fails, fix, commit, and rerun. Allow at most 3 fix and rerun cycles. If gates still fail after 3 attempts, stop and report the failures. Do not loop indefinitely. - -9. Push updates back to the PR head branch - -```sh -# Ensure remote for PR head exists -git remote add prhead "$head_repo_url.git" 2>/dev/null || git remote set-url prhead "$head_repo_url.git" - -# Use force with lease after rebase -# Double check: $head must NOT be "main" or "master" -echo "Pushing to branch: $head" -if [ "$head" = "main" ] || [ "$head" = "master" ]; then - echo "ERROR: head branch is main/master. This is wrong. Stopping." - exit 1 -fi -git push --force-with-lease prhead HEAD:$head -``` - -Before running the command above, pause and ask for explicit maintainer go-ahead to perform the push. - -10. Verify PR is not behind main (Mandatory) - -```sh -git fetch origin main -git fetch origin pull//head:pr--verify --force -git merge-base --is-ancestor origin/main pr--verify && echo "PR is up to date with main" || echo "ERROR: PR is still behind main, rebase again" -git branch -D pr--verify 2>/dev/null || true -``` - -If still behind main, repeat steps 2 through 9. - -11. Write prep summary artifacts (Mandatory) - -Update `.local/prep.md` with: - -- Current HEAD sha from `git rev-parse HEAD`. -- Short bullet list of changes. -- Gate results. -- Push confirmation. -- Rebase verification result. - -Create or overwrite `.local/prep.md` and verify it exists and is non-empty: - -```sh -git rev-parse HEAD -ls -la .local/prep.md -wc -l .local/prep.md -``` - -12. Output - -Include a diff stat summary: - -```sh -git diff --stat origin/main..HEAD -git diff --shortstat origin/main..HEAD -``` - -Report totals: X files changed, Y insertions(+), Z deletions(-). - -If gates passed and push succeeded, print exactly: - -``` -PR is ready for /mergepr -``` - -Otherwise, list remaining failures and stop. +- Summarize resolved findings and gate results. +- Print exactly: `PR is ready for /merge-pr`. ## Guardrails -- Worktree only. -- Do not delete the worktree on success. `/mergepr` may reuse it. -- Do not run `gh pr merge`. -- Never push to main. Only push to the PR head branch. -- Run and pass all gates before pushing. +- Do not run `gh pr merge` in this skill. +- Do not delete worktree. diff --git a/.agents/skills/review-pr/SKILL.md b/.agents/skills/review-pr/SKILL.md index e516c97aaa0..f5694ca2c41 100644 --- a/.agents/skills/review-pr/SKILL.md +++ b/.agents/skills/review-pr/SKILL.md @@ -1,229 +1,142 @@ --- name: review-pr -description: Review-only GitHub pull request analysis with the gh CLI. Use when asked to review a PR, provide structured feedback, or assess readiness to land. Do not merge, push, or make code changes you intend to keep. +description: Script-first review-only GitHub pull request analysis. Use for deterministic PR review with structured findings handoff to /prepare-pr. --- # Review PR ## Overview -Perform a thorough review-only PR assessment and return a structured recommendation on readiness for /preparepr. +Perform a read-only review and produce both human and machine-readable outputs. ## Inputs - Ask for PR number or URL. -- If missing, always ask. Never auto-detect from conversation. -- If ambiguous, ask. +- If missing, always ask. ## Safety -- Never push to `main` or `origin/main`, not during review, not ever. -- Do not run `git push` at all during review. Treat review as read only. -- Do not stop or kill the gateway. Do not run gateway stop commands. Do not kill processes on port 18792. -- Do not perform any GitHub write action (comments, assignees, labels, state changes) unless maintainer explicitly approves it. +- Never push, merge, or modify code intended to keep. +- Work only in `.worktrees/pr-`. +- Wrapper commands are cwd-agnostic; you can run them from repo root or inside the PR worktree. -## Execution Rule +## Execution Contract -- Execute the workflow. Do not stop after printing the TODO checklist. -- If delegating, require the delegate to run commands and capture outputs, not a plan. - -## Known Failure Modes - -- If you see "fatal: not a git repository", you are in the wrong directory. Use `~/dev/openclaw` if available; otherwise ask user. -- Do not stop after printing the checklist. That is not completion. - -## Writing Style for Output - -- Write casual and direct. -- Avoid em dashes and en dashes. Use commas or separate sentences. - -## Completion Criteria - -- Run the commands in the worktree and inspect the PR directly. -- Produce the structured review sections A through J. -- Save the full review to `.local/review.md` inside the worktree. - -## First: Create a TODO Checklist - -Create a checklist of all review steps, print it, then continue and execute the commands. - -## Setup: Use a Worktree - -Use an isolated worktree for all review work. +1. Run wrapper setup: ```sh -cd ~/dev/openclaw -# Sanity: confirm you are in the repo -git rev-parse --show-toplevel - -WORKTREE_DIR=".worktrees/pr-" -git fetch origin main - -# Reuse existing worktree if it exists, otherwise create new -if [ -d "$WORKTREE_DIR" ]; then - cd "$WORKTREE_DIR" - git checkout temp/pr- 2>/dev/null || git checkout -b temp/pr- - git fetch origin main - git reset --hard origin/main -else - git worktree add "$WORKTREE_DIR" -b temp/pr- origin/main - cd "$WORKTREE_DIR" -fi - -# Create local scratch space that persists across /reviewpr to /preparepr to /mergepr -mkdir -p .local +scripts/pr-review ``` -Run all commands inside the worktree directory. -Start on `origin/main` so you can check for existing implementations before looking at PR code. +2. Use explicit branch mode switches: + +- Main baseline mode: `scripts/pr review-checkout-main ` +- PR-head mode: `scripts/pr review-checkout-pr ` + +3. Before writing review outputs, run branch guard: + +```sh +scripts/pr review-guard +``` + +4. Write both outputs: + +- `.local/review.md` with sections A through J. +- `.local/review.json` with structured findings. + +5. Validate artifacts semantically: + +```sh +scripts/pr review-validate-artifacts +``` ## Steps -1. Identify PR meta and context +1. Setup and metadata ```sh -gh pr view --json number,title,state,isDraft,author,baseRefName,headRefName,headRepository,url,body,labels,assignees,reviewRequests,files,additions,deletions --jq '{number,title,url,state,isDraft,author:.author.login,base:.baseRefName,head:.headRefName,headRepo:.headRepository.nameWithOwner,additions,deletions,files:.files|length,body}' +scripts/pr-review +ls -la .local/pr-meta.json .local/pr-meta.env .local/review-context.env .local/review-mode.env ``` -2. Check if this already exists in main before looking at the PR branch - -- Identify the core feature or fix from the PR title and description. -- Search for existing implementations using keywords from the PR title, changed file paths, and function or component names from the diff. +2. Existing implementation check on main ```sh -# Use keywords from the PR title and changed files -rg -n "" -S src packages apps ui || true -rg -n "" -S src packages apps ui || true - -git log --oneline --all --grep="" | head -20 +scripts/pr review-checkout-main +rg -n "" -S src extensions apps || true +git log --oneline --all --grep "" | head -20 ``` -If it already exists, call it out as a BLOCKER or at least IMPORTANT. - -3. Optional claim step, only with explicit approval - -If the maintainer asks to claim the PR, assign yourself. Otherwise skip this. +3. Claim PR ```sh gh_user=$(gh api user --jq .login) -gh pr edit --add-assignee "$gh_user" +gh pr edit --add-assignee "$gh_user" || echo "Could not assign reviewer, continuing" ``` -4. Read the PR description carefully - -Use the body from step 1. Summarize goal, scope, and missing context. - -5. Read the diff thoroughly - -Minimum: +4. Read PR description and diff ```sh +scripts/pr review-checkout-pr gh pr diff + +source .local/review-context.env +git diff --stat "$MERGE_BASE"..pr- +git diff "$MERGE_BASE"..pr- ``` -If you need full code context locally, fetch the PR head to a local ref and diff it. Do not create a merge commit. +5. Optional local tests + +Use the wrapper for target validation and executed-test verification: ```sh -git fetch origin pull//head:pr- -# Show changes without modifying the working tree - -git diff --stat origin/main..pr- -git diff origin/main..pr- +scripts/pr review-tests [ ...] ``` -If you want to browse the PR version of files directly, temporarily check out `pr-` in the worktree. Do not commit or push. Return to `temp/pr-` and reset to `origin/main` afterward. +6. Initialize review artifact templates ```sh -# Use only if needed -# git checkout pr- -# ...inspect files... - -git checkout temp/pr- -git reset --hard origin/main +scripts/pr review-artifacts-init ``` -6. Validate the change is needed and valuable +7. Produce review outputs -Be honest. Call out low value AI slop. +- Fill `.local/review.md` sections A through J. +- Fill `.local/review.json`. -7. Evaluate implementation quality +Minimum JSON shape: -Review correctness, design, performance, and ergonomics. +```json +{ + "recommendation": "READY FOR /prepare-pr", + "findings": [ + { + "id": "F1", + "severity": "IMPORTANT", + "title": "...", + "area": "path/or/component", + "fix": "Actionable fix" + } + ], + "tests": { + "ran": [], + "gaps": [], + "result": "pass" + }, + "docs": "up_to_date|missing|not_applicable", + "changelog": "required" +} +``` -8. Perform a security review - -Assume OpenClaw subagents run with full disk access, including git, gh, and shell. Check auth, input validation, secrets, dependencies, tool safety, and privacy. - -9. Review tests and verification - -Identify what exists, what is missing, and what would be a minimal regression test. - -10. Check docs - -Check if the PR touches code with related documentation such as README, docs, inline API docs, or config examples. - -- If docs exist for the changed area and the PR does not update them, flag as IMPORTANT. -- If the PR adds a new feature or config option with no docs, flag as IMPORTANT. -- If the change is purely internal with no user-facing impact, skip this. - -11. Check changelog - -Check if `CHANGELOG.md` exists and whether the PR warrants an entry. - -- If the project has a changelog and the PR is user-facing, flag missing entry as IMPORTANT. -- Leave the change for /preparepr, only flag it here. - -12. Answer the key question - -Decide if /preparepr can fix issues or the contributor must update the PR. - -13. Save findings to the worktree - -Write the full structured review sections A through J to `.local/review.md`. -Create or overwrite the file and verify it exists and is non-empty. +8. Guard + validate before final output ```sh -ls -la .local/review.md -wc -l .local/review.md +scripts/pr review-guard +scripts/pr review-validate-artifacts ``` -14. Output the structured review - -Produce a review that matches what you saved to `.local/review.md`. - -A) TL;DR recommendation - -- One of: READY FOR /preparepr | NEEDS WORK | NEEDS DISCUSSION | NOT USEFUL (CLOSE) -- 1 to 3 sentences. - -B) What changed - -C) What is good - -D) Security findings - -E) Concerns or questions (actionable) - -- Numbered list. -- Mark each item as BLOCKER, IMPORTANT, or NIT. -- For each, point to file or area and propose a concrete fix. - -F) Tests - -G) Docs status - -- State if related docs are up to date, missing, or not applicable. - -H) Changelog - -- State if `CHANGELOG.md` needs an entry and which category. - -I) Follow ups (optional) - -J) Suggested PR comment (optional) - ## Guardrails -- Worktree only. -- Do not delete the worktree after review. -- Review only, do not merge, do not push. +- Keep review read-only. +- Do not delete worktree. +- Use merge-base scoped diff for local context to avoid stale branch drift.