diff --git a/.agents/skills/PR_WORKFLOW.md b/.agents/skills/PR_WORKFLOW.md index 4030650735..f85943045e 100644 --- a/.agents/skills/PR_WORKFLOW.md +++ b/.agents/skills/PR_WORKFLOW.md @@ -1,22 +1,18 @@ -# PR Workflow for Maintainers +# PR Review Instructions 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` — 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 +1. `review-pr` +2. `prepare-pr` +3. `merge-pr` They are necessary, but not sufficient. Maintainers must steer between steps and understand the code before moving forward. @@ -25,64 +21,26 @@ 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. -## Script-first contract +## Remote write policy -Skill runs should invoke these wrappers automatically. You only need to run them manually when debugging or doing an explicit script-only run: +Until the maintainer explicitly approves remote actions, stay local-only. -- `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 ` +Remote actions include: -These wrappers run shared preflight checks and generate deterministic artifacts. They are designed to work from repo root or PR worktree cwd. +- Pushing branches. +- Posting PR comments. +- Editing PR metadata (labels, assignees, state). +- Merging PRs. +- Editing advisory state or publishing advisories. -## Required artifacts +Allowed before approval: -- `.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. +- Local code changes. +- Local tests and validation. +- Drafting copy for PR/advisory comments. +- Read-only `gh` commands. -## 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. +When approved, perform only the approved remote action, then pause for next instruction. ## PR quality bar @@ -95,60 +53,6 @@ Use ChatGPT 5.3 Codex High. Fall back to 5.2 Codex High or 5.3 Codex Medium if n - 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: @@ -174,6 +78,7 @@ 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? ``` @@ -189,30 +94,27 @@ Stop and escalate instead of continuing if: Purpose: - Make the PR merge-ready on its head branch. -- 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`). +- Rebase onto current `main`, fix blocker/important findings, and run gates. 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 /merge-pr`. +- Final status: `PR is ready for /mergepr`. 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? -Do we need regression tests? -Are tests using fake timers where appropriate? (e.g., debounce/throttle, retry backoff, timeout branches, delayed callbacks, polling loops) +Are tests using fake timers where relevant? (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: @@ -221,29 +123,59 @@ 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 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. +- Use squash merge flow and verify the PR ends in `MERGED` state. Go or no-go checklist before merge: - All BLOCKER and IMPORTANT findings are resolved. - Verification is meaningful and regression risk is acceptably low. -- Changelog is updated (mandatory) and docs are updated when required. +- Docs and changelog 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? -- Run `bun scripts/update-clawtributors.ts` if the contributor is new. + +## 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. diff --git a/.agents/skills/merge-pr/SKILL.md b/.agents/skills/merge-pr/SKILL.md index 041e79a676..c4f5e4b7f7 100644 --- a/.agents/skills/merge-pr/SKILL.md +++ b/.agents/skills/merge-pr/SKILL.md @@ -1,99 +1,188 @@ --- name: merge-pr -description: Script-first deterministic squash merge with strict required-check gating, head-SHA pinning, and reliable attribution/commenting. +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. --- # Merge PR ## Overview -Merge a prepared PR only after deterministic validation. +Merge a prepared PR via `gh pr merge --squash` and clean up the worktree after success. ## Inputs - Ask for PR number or URL. -- If missing, use `.local/prep.env` from the PR worktree. +- If missing, auto-detect from conversation. +- If ambiguous, ask. ## Safety -- 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. +- 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. -## Execution Contract +## Execution Rule -1. Validate merge readiness: +- 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. ```sh -scripts/pr-merge verify +cd ~/dev/openclaw +# Sanity: confirm you are in the repo +git rev-parse --show-toplevel + +WORKTREE_DIR=".worktrees/pr-" ``` -Backward-compatible verify form also works: +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` ```sh -scripts/pr-merge +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 ``` -2. Run one-shot deterministic merge: - -```sh -scripts/pr-merge run -``` - -3. Ensure output reports: - -- `merge_sha=` -- `merge_author_email=` -- `comment_url=` - ## Steps -1. Validate artifacts +1. Identify PR meta ```sh -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 +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) ``` -2. Validate checks and branch status +2. Run sanity checks + +Stop if any are true: + +- PR is a draft. +- Required checks are failing. +- Branch is behind main. ```sh -scripts/pr-merge verify -source .local/prep.env +# 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` treats “no required checks configured” as acceptable (`[]`), but fails on any required `fail` or `pending`. +If anything is failing or behind, stop and say to run `/preparepr`. -3. Merge deterministically (wrapper-managed) +3. Merge PR and delete branch + +If checks are still running, use `--auto` to queue the merge. ```sh -scripts/pr-merge run +# 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` performs: +Before running merge command, pause and ask for explicit maintainer go-ahead. -- 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` +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. -4. Manual fallback (only if wrapper is unavailable) +4. Get merge SHA ```sh -scripts/pr merge-run +merge_sha=$(gh pr view --json mergeCommit --jq '.mergeCommit.oid') +echo "merge_sha=$merge_sha" ``` -5. Cleanup +5. Optional comment -Cleanup is handled by `run` after merge success. +Use a literal multiline string or heredoc for newlines. + +```sh +gh pr comment -F - <<'EOF' +Merged via squash. + +- Merge commit: $merge_sha + +Thanks @$contrib! +EOF +``` + +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 +``` ## Guardrails -- End in `MERGED`, never `CLOSED`. -- Cleanup only after confirmed merge. +- 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. diff --git a/.agents/skills/prepare-pr/SKILL.md b/.agents/skills/prepare-pr/SKILL.md index 462e5bc2bd..dc1813f3da 100644 --- a/.agents/skills/prepare-pr/SKILL.md +++ b/.agents/skills/prepare-pr/SKILL.md @@ -1,122 +1,251 @@ --- name: prepare-pr -description: Script-first PR preparation with structured findings resolution, deterministic push safety, and explicit gate execution. +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. --- # Prepare PR ## Overview -Prepare the PR head branch for merge after `/review-pr`. +Prepare a PR branch for merge with review fixes, green gates, and an updated head branch. ## Inputs - Ask for PR number or URL. -- If missing, use `.local/pr-meta.env` if present in the PR worktree. +- If missing, auto-detect from conversation. +- If ambiguous, ask. ## Safety -- Never push to `main`. -- Only push to PR head with explicit `--force-with-lease` against known head SHA. +- 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. - Do not run `git clean -fdx`. -- Wrappers are cwd-agnostic; run from repo root or PR worktree. +- 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. -## Execution Contract +## Execution Rule -1. Run setup: +- 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. ```sh -scripts/pr-prepare init +cd ~/openclaw +# Sanity: confirm you are in the repo +git rev-parse --show-toplevel + +WORKTREE_DIR=".worktrees/pr-" ``` -2. Resolve findings from structured review: +Run all commands inside the worktree directory. -- `.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: +## Load Review Findings (Mandatory) ```sh -scripts/pr-prepare run +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 ``` ## Steps -1. Setup and artifacts +1. Identify PR meta (author, head branch, head repo URL) ```sh -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 +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) ``` -2. Resolve required findings - -List required items: +2. Fetch the PR branch tip into a local ref ```sh -jq -r '.findings[] | select(.severity=="BLOCKER" or .severity=="IMPORTANT") | "- [\(.severity)] \(.id): \(.title) => \(.fix)"' .local/review.json +git fetch origin pull//head:pr- ``` -Fix all required findings. Keep scope tight. - -3. Update changelog/docs (changelog is mandatory in this workflow) +3. Rebase PR commits onto latest main ```sh -jq -r '.changelog' .local/review.json -jq -r '.docs' .local/review.json +# Move worktree to the PR tip first +git reset --hard pr- + +# Rebase onto current main +git fetch origin main +git rebase origin/main ``` -4. Commit scoped changes +If conflicts happen: -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. +- Resolve each conflicted file. +- Run `git add ` for each file. +- Run `git rebase --continue`. -Use explicit file list: +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. ```sh -scripts/committer "fix: " ... +ls CHANGELOG.md 2>/dev/null ``` -5. Run gates +- 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: ```sh -scripts/pr-prepare gates +git add ... ``` -6. Push safely to PR head +Preferred commit tool: ```sh -scripts/pr-prepare push +committer "fix: (#) (thanks @$contrib)" ``` -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 +If `committer` is not found: ```sh -ls -la .local/prep.md .local/prep.env +git commit -m "fix: (#) (thanks @$contrib)" ``` -8. Output +8. Run full gates before pushing -- Summarize resolved findings and gate results. -- Print exactly: `PR is ready for /merge-pr`. +```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. ## Guardrails -- Do not run `gh pr merge` in this skill. -- Do not delete worktree. +- 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. diff --git a/.agents/skills/review-pr/SKILL.md b/.agents/skills/review-pr/SKILL.md index f5694ca2c4..e516c97aaa 100644 --- a/.agents/skills/review-pr/SKILL.md +++ b/.agents/skills/review-pr/SKILL.md @@ -1,142 +1,229 @@ --- name: review-pr -description: Script-first review-only GitHub pull request analysis. Use for deterministic PR review with structured findings handoff to /prepare-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. --- # Review PR ## Overview -Perform a read-only review and produce both human and machine-readable outputs. +Perform a thorough review-only PR assessment and return a structured recommendation on readiness for /preparepr. ## Inputs - Ask for PR number or URL. -- If missing, always ask. +- If missing, always ask. Never auto-detect from conversation. +- If ambiguous, ask. ## Safety -- 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. +- 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. -## Execution Contract +## Execution Rule -1. Run wrapper setup: +- 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. ```sh -scripts/pr-review +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 ``` -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 -``` +Run all commands inside the worktree directory. +Start on `origin/main` so you can check for existing implementations before looking at PR code. ## Steps -1. Setup and metadata +1. Identify PR meta and context ```sh -scripts/pr-review -ls -la .local/pr-meta.json .local/pr-meta.env .local/review-context.env .local/review-mode.env +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}' ``` -2. Existing implementation check on main +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. ```sh -scripts/pr review-checkout-main -rg -n "" -S src extensions apps || true -git log --oneline --all --grep "" | head -20 +# 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 ``` -3. Claim PR +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. ```sh gh_user=$(gh api user --jq .login) -gh pr edit --add-assignee "$gh_user" || echo "Could not assign reviewer, continuing" +gh pr edit --add-assignee "$gh_user" ``` -4. Read PR description and diff +4. Read the PR description carefully + +Use the body from step 1. Summarize goal, scope, and missing context. + +5. Read the diff thoroughly + +Minimum: ```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- ``` -5. Optional local tests - -Use the wrapper for target validation and executed-test verification: +If you need full code context locally, fetch the PR head to a local ref and diff it. Do not create a merge commit. ```sh -scripts/pr review-tests [ ...] +git fetch origin pull//head:pr- +# Show changes without modifying the working tree + +git diff --stat origin/main..pr- +git diff origin/main..pr- ``` -6. Initialize review artifact templates +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. ```sh -scripts/pr review-artifacts-init +# Use only if needed +# git checkout pr- +# ...inspect files... + +git checkout temp/pr- +git reset --hard origin/main ``` -7. Produce review outputs +6. Validate the change is needed and valuable -- Fill `.local/review.md` sections A through J. -- Fill `.local/review.json`. +Be honest. Call out low value AI slop. -Minimum JSON shape: +7. Evaluate implementation quality -```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" -} -``` +Review correctness, design, performance, and ergonomics. -8. Guard + validate before final output +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. ```sh -scripts/pr review-guard -scripts/pr review-validate-artifacts +ls -la .local/review.md +wc -l .local/review.md ``` +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 -- Keep review read-only. -- Do not delete worktree. -- Use merge-base scoped diff for local context to avoid stale branch drift. +- Worktree only. +- Do not delete the worktree after review. +- Review only, do not merge, do not push.