diff --git a/.agents/archive/PR_WORKFLOW_V1.md b/.agents/archive/PR_WORKFLOW_V1.md new file mode 100644 index 0000000000..1cb6ab653b --- /dev/null +++ b/.agents/archive/PR_WORKFLOW_V1.md @@ -0,0 +1,181 @@ +# 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. +Always pause between skills to evaluate technical direction, not just command success. + +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 + +They are necessary, but not sufficient. Maintainers must steer between steps and understand the code before moving forward. + +Treat PRs as reports first, code second. +If submitted code is low quality, ignore it and implement the best solution for the problem. + +Do not continue if you cannot verify the problem is real or test the fix. + +## 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 + +- Do not trust PR code by default. +- Do not merge changes you cannot validate with a reproducible problem and a tested fix. +- Keep types strict. Do not use `any` in implementation code. +- Keep external-input boundaries typed and validated, including CLI input, environment variables, network payloads, and tool output. +- Keep implementations properly scoped. Fix root causes, not local symptoms. +- Identify and reuse canonical sources of truth so behavior does not drift across the codebase. +- 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 + +- Create commits with `scripts/committer "" `; 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 this commit subject format: `fix: (openclaw#) thanks @`. +- 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. +- When working on an issue: reference the issue in the changelog entry. +- Pure test additions/fixes generally do **not** need a changelog entry unless they alter user-facing behavior or the user asks for one. + +## 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. +- When merging a PR from a new contributor: 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: use an isolated `.worktrees/pr-` checkout from `origin/main`. 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: + +- PR URL/number is known. +- Problem statement is clear enough to attempt reproduction. +- A realistic verification path exists (tests, integration checks, or explicit manual validation). + +### 1) `review-pr` + +Purpose: + +- Review only: correctness, value, security risk, tests, docs, and changelog impact. +- Produce structured findings and a recommendation. + +Expected output: + +- Recommendation: ready, needs work, needs discussion, or close. +- `.local/review.md` with actionable findings. + +Maintainer checkpoint before `prepare-pr`: + +``` +What problem are they trying to solve? +What is the most optimal implementation? +Can we fix up everything? +Do we have any questions? +``` + +Stop and escalate instead of continuing if: + +- The problem cannot be reproduced or confirmed. +- The proposed PR scope does not match the stated problem. +- The design introduces unresolved security or trust-boundary concerns. + +### 2) `prepare-pr` + +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`). + +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`. + +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) +Do not add performative tests, ensure tests are real and there are no regressions. +Do you see any follow-up refactors we should do? +Take your time, fix it properly, refactor if necessary. +Did any changes introduce any potential security vulnerabilities? +``` + +Stop and escalate instead of continuing if: + +- You cannot verify behavior changes with meaningful tests or validation. +- Fixing findings requires broad architecture changes outside safe PR scope. +- Security hardening requirements remain unresolved. + +### 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. + +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. +- 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. diff --git a/.agents/archive/merge-pr-v1/SKILL.md b/.agents/archive/merge-pr-v1/SKILL.md new file mode 100644 index 0000000000..0956699eb5 --- /dev/null +++ b/.agents/archive/merge-pr-v1/SKILL.md @@ -0,0 +1,304 @@ +--- +name: merge-pr +description: Merge a GitHub PR via squash after /prepare-pr. 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 via deterministic squash merge (`--match-head-commit` + explicit co-author trailer), then clean up the worktree after success. + +## Inputs + +- Ask for PR number or URL. +- If missing, use `.local/prep.env` from the worktree if present. +- If ambiguous, ask. + +## Safety + +- Use `gh pr merge --squash` as the only path to `main`. +- Do not run `git push` at all during merge. +- Do not use `gh pr merge --auto` for maintainer landings. +- Do not run gateway stop commands. Do not kill processes. Do not touch port 18792. + +## Execution Rule + +- 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. Move to the repo root and retry. +- Read `.local/review.md`, `.local/prep.md`, and `.local/prep.env` in the worktree. Do not skip. +- Always merge with `--match-head-commit "$PREP_HEAD_SHA"` to prevent racing stale or changed heads. +- Clean up `.worktrees/pr-` only after confirmed `MERGED`. + +## Completion Criteria + +- Ensure `gh pr merge` succeeds. +- Ensure PR state is `MERGED`, never `CLOSED`. +- Record the merge SHA. +- Leave a PR comment with merge SHA and prepared head SHA, and capture the comment URL. +- 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 +repo_root=$(git rev-parse --show-toplevel) +cd "$repo_root" +gh auth status + +WORKTREE_DIR=".worktrees/pr-" +cd "$WORKTREE_DIR" +``` + +Run all commands inside the worktree directory. + +## Load Local Artifacts (Mandatory) + +Expect these files from earlier steps: + +- `.local/review.md` from `/review-pr` +- `.local/prep.md` from `/prepare-pr` +- `.local/prep.env` from `/prepare-pr` + +```sh +ls -la .local || true + +for required in .local/review.md .local/prep.md .local/prep.env; do + if [ ! -f "$required" ]; then + echo "Missing $required. Stop and run /review-pr then /prepare-pr." + exit 1 + fi +done + +sed -n '1,120p' .local/review.md +sed -n '1,120p' .local/prep.md +source .local/prep.env +``` + +## Steps + +1. Identify PR meta and verify prepared SHA still matches + +```sh +pr_meta_json=$(gh pr view --json number,title,state,isDraft,author,headRefName,headRefOid,baseRefName,headRepository,body) +printf '%s\n' "$pr_meta_json" | jq '{number,title,state,isDraft,author:.author.login,head:.headRefName,headSha:.headRefOid,base:.baseRefName,headRepo:.headRepository.nameWithOwner,body}' +pr_title=$(printf '%s\n' "$pr_meta_json" | jq -r .title) +pr_number=$(printf '%s\n' "$pr_meta_json" | jq -r .number) +pr_head_sha=$(printf '%s\n' "$pr_meta_json" | jq -r .headRefOid) +contrib=$(printf '%s\n' "$pr_meta_json" | jq -r .author.login) +is_draft=$(printf '%s\n' "$pr_meta_json" | jq -r .isDraft) + +if [ "$is_draft" = "true" ]; then + echo "ERROR: PR is draft. Stop and run /prepare-pr after draft is cleared." + exit 1 +fi + +if [ "$pr_head_sha" != "$PREP_HEAD_SHA" ]; then + echo "ERROR: PR head changed after /prepare-pr (expected $PREP_HEAD_SHA, got $pr_head_sha). Re-run /prepare-pr." + exit 1 +fi +``` + +2. Run sanity checks + +Stop if any are true: + +- PR is a draft. +- Required checks are failing. +- Branch is behind main. + +If checks are pending, wait for completion before merging. Do not use `--auto`. +If no required checks are configured, continue. + +```sh +gh pr checks --required --watch --fail-fast || true +checks_json=$(gh pr checks --required --json name,bucket,state 2>/tmp/gh-checks.err || true) +if [ -z "$checks_json" ]; then + checks_json='[]' +fi +required_count=$(printf '%s\n' "$checks_json" | jq 'length') +if [ "$required_count" -eq 0 ]; then + echo "No required checks configured for this PR." +fi +printf '%s\n' "$checks_json" | jq -r '.[] | "\(.bucket)\t\(.name)\t\(.state)"' + +failed_required=$(printf '%s\n' "$checks_json" | jq '[.[] | select(.bucket=="fail")] | length') +pending_required=$(printf '%s\n' "$checks_json" | jq '[.[] | select(.bucket=="pending")] | length') +if [ "$failed_required" -gt 0 ]; then + echo "Required checks are failing, run /prepare-pr." + exit 1 +fi +if [ "$pending_required" -gt 0 ]; then + echo "Required checks are still pending, retry /merge-pr when green." + exit 1 +fi + +git fetch origin main +git fetch origin pull//head:pr- --force +git merge-base --is-ancestor origin/main pr- || (echo "PR branch is behind main, run /prepare-pr" && exit 1) +``` + +If anything is failing or behind, stop and say to run `/prepare-pr`. + +3. Merge PR with explicit attribution metadata + +```sh +reviewer=$(gh api user --jq .login) +reviewer_id=$(gh api user --jq .id) +coauthor_email=${COAUTHOR_EMAIL:-"$contrib@users.noreply.github.com"} +if [ -z "$coauthor_email" ] || [ "$coauthor_email" = "null" ]; then + contrib_id=$(gh api users/$contrib --jq .id) + coauthor_email="${contrib_id}+${contrib}@users.noreply.github.com" +fi + +gh_email=$(gh api user --jq '.email // ""' || true) +git_email=$(git config user.email || true) +mapfile -t reviewer_email_candidates < <( + printf '%s\n' \ + "$gh_email" \ + "$git_email" \ + "${reviewer_id}+${reviewer}@users.noreply.github.com" \ + "${reviewer}@users.noreply.github.com" | awk 'NF && !seen[$0]++' +) +[ "${#reviewer_email_candidates[@]}" -gt 0 ] || { echo "ERROR: could not resolve reviewer author email"; exit 1; } +reviewer_email="${reviewer_email_candidates[0]}" + +cat > .local/merge-body.txt < /prepare-pr -> /merge-pr. + +Prepared head SHA: $PREP_HEAD_SHA +Co-authored-by: $contrib <$coauthor_email> +Co-authored-by: $reviewer <$reviewer_email> +Reviewed-by: @$reviewer +EOF + +run_merge() { + local email="$1" + local stderr_file + stderr_file=$(mktemp) + if gh pr merge \ + --squash \ + --delete-branch \ + --match-head-commit "$PREP_HEAD_SHA" \ + --author-email "$email" \ + --subject "$pr_title (#$pr_number)" \ + --body-file .local/merge-body.txt \ + 2> >(tee "$stderr_file" >&2) + then + rm -f "$stderr_file" + return 0 + fi + merge_err=$(cat "$stderr_file") + rm -f "$stderr_file" + return 1 +} + +merge_err="" +selected_merge_author_email="$reviewer_email" +if ! run_merge "$selected_merge_author_email"; then + if printf '%s\n' "$merge_err" | rg -qi 'author.?email|email.*associated|associated.*email|invalid.*email' && [ "${#reviewer_email_candidates[@]}" -ge 2 ]; then + selected_merge_author_email="${reviewer_email_candidates[1]}" + echo "Retrying once with fallback author email: $selected_merge_author_email" + run_merge "$selected_merge_author_email" || { echo "ERROR: merge failed after fallback retry"; exit 1; } + else + echo "ERROR: merge failed" + exit 1 + fi +fi +``` + +Retry is allowed exactly once when the error is clearly author-email validation. + +4. Verify PR state and capture merge SHA + +```sh +state=$(gh pr view --json state --jq .state) +if [ "$state" != "MERGED" ]; then + echo "Merge not finalized yet (state=$state), waiting up to 15 minutes..." + for _ in $(seq 1 90); do + sleep 10 + state=$(gh pr view --json state --jq .state) + if [ "$state" = "MERGED" ]; then + break + fi + done +fi + +if [ "$state" != "MERGED" ]; then + echo "ERROR: PR state is $state after waiting. Leave worktree and retry /merge-pr later." + exit 1 +fi + +merge_sha=$(gh pr view --json mergeCommit --jq '.mergeCommit.oid') +if [ -z "$merge_sha" ] || [ "$merge_sha" = "null" ]; then + echo "ERROR: merge commit SHA missing." + exit 1 +fi + +commit_body=$(gh api repos/:owner/:repo/commits/$merge_sha --jq .commit.message) +contrib=${contrib:-$(gh pr view --json author --jq .author.login)} +reviewer=${reviewer:-$(gh api user --jq .login)} +printf '%s\n' "$commit_body" | rg -q "^Co-authored-by: $contrib <" || { echo "ERROR: missing PR author co-author trailer"; exit 1; } +printf '%s\n' "$commit_body" | rg -q "^Co-authored-by: $reviewer <" || { echo "ERROR: missing reviewer co-author trailer"; exit 1; } + +echo "merge_sha=$merge_sha" +``` + +5. PR comment + +Use a multiline heredoc with interpolation enabled. + +```sh +ok=0 +comment_output="" +for _ in 1 2 3; do + if comment_output=$(gh pr comment -F - <" --force +git branch -D temp/pr- 2>/dev/null || true +git branch -D pr- 2>/dev/null || true +git branch -D pr--prep 2>/dev/null || true +``` + +## 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. diff --git a/.agents/archive/merge-pr-v1/agents/openai.yaml b/.agents/archive/merge-pr-v1/agents/openai.yaml new file mode 100644 index 0000000000..9c10ae4d27 --- /dev/null +++ b/.agents/archive/merge-pr-v1/agents/openai.yaml @@ -0,0 +1,4 @@ +interface: + display_name: "Merge PR" + short_description: "Merge GitHub PRs via squash" + default_prompt: "Use $merge-pr to merge a GitHub PR via squash after preparation." diff --git a/.agents/archive/prepare-pr-v1/SKILL.md b/.agents/archive/prepare-pr-v1/SKILL.md new file mode 100644 index 0000000000..91c4508a07 --- /dev/null +++ b/.agents/archive/prepare-pr-v1/SKILL.md @@ -0,0 +1,336 @@ +--- +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 /review-pr. Never merge or push to main. +--- + +# Prepare PR + +## Overview + +Prepare a PR head branch for merge with review fixes, green gates, and deterministic merge handoff artifacts. + +## Inputs + +- Ask for PR number or URL. +- If missing, use `.local/pr-meta.env` from the PR worktree if present. +- If ambiguous, ask. + +## Safety + +- Never push to `main` or `origin/main`. Push only to the PR head branch. +- Never run `git push` without explicit remote and branch. 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`. +- Do not run `git add -A` or `git add .`. + +## Execution Rule + +- Execute the workflow. Do not stop after printing the TODO checklist. +- If delegating, require the delegate to run commands and capture outputs. + +## Completion Criteria + +- Rebase PR commits onto `origin/main`. +- Fix all BLOCKER and IMPORTANT items from `.local/review.md`. +- Commit prep changes with required subject format. +- Run required gates and pass (`pnpm test` may be skipped only for high-confidence docs-only changes). +- Push the updated HEAD back to the PR head branch. +- Write `.local/prep.md` and `.local/prep.env`. +- 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 +repo_root=$(git rev-parse --show-toplevel) +cd "$repo_root" +gh auth status + +WORKTREE_DIR=".worktrees/pr-" +if [ ! -d "$WORKTREE_DIR" ]; then + git fetch origin main + git worktree add "$WORKTREE_DIR" -b temp/pr- origin/main +fi +cd "$WORKTREE_DIR" +mkdir -p .local +``` + +Run all commands inside the worktree directory. + +## Load Review Artifacts (Mandatory) + +```sh +if [ ! -f .local/review.md ]; then + echo "Missing .local/review.md. Run /review-pr first and save findings." + exit 1 +fi + +if [ ! -f .local/pr-meta.env ]; then + echo "Missing .local/pr-meta.env. Run /review-pr first and save metadata." + exit 1 +fi + +sed -n '1,220p' .local/review.md +source .local/pr-meta.env +``` + +## Steps + +1. Identify PR meta with one API call + +```sh +pr_meta_json=$(gh pr view --json number,title,author,headRefName,headRefOid,baseRefName,headRepository,headRepositoryOwner,body) +printf '%s\n' "$pr_meta_json" | jq '{number,title,author:.author.login,head:.headRefName,headSha:.headRefOid,base:.baseRefName,headRepo:.headRepository.nameWithOwner,headRepoOwner:.headRepositoryOwner.login,headRepoName:.headRepository.name,body}' + +pr_number=$(printf '%s\n' "$pr_meta_json" | jq -r .number) +contrib=$(printf '%s\n' "$pr_meta_json" | jq -r .author.login) +head=$(printf '%s\n' "$pr_meta_json" | jq -r .headRefName) +pr_head_sha_before=$(printf '%s\n' "$pr_meta_json" | jq -r .headRefOid) +head_owner=$(printf '%s\n' "$pr_meta_json" | jq -r '.headRepositoryOwner.login // empty') +head_repo_name=$(printf '%s\n' "$pr_meta_json" | jq -r '.headRepository.name // empty') +head_repo_url=$(printf '%s\n' "$pr_meta_json" | jq -r '.headRepository.url // empty') + +if [ -n "${PR_HEAD:-}" ] && [ "$head" != "$PR_HEAD" ]; then + echo "ERROR: PR head branch changed from $PR_HEAD to $head. Re-run /review-pr." + exit 1 +fi +``` + +2. Fetch PR head and rebase on latest `origin/main` + +```sh +git fetch origin pull//head:pr- --force +git checkout -B pr--prep pr- +git fetch origin main +git rebase origin/main +``` + +If conflicts happen: + +- Resolve each conflicted file. +- Run `git add ` for each file. +- Run `git rebase --continue`. + +If the rebase gets confusing or you resolve conflicts 3 or more times, stop and report. + +3. 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. + +4. Optional quick feedback tests before full gates + +Targeted tests are optional quick feedback, not a substitute for full gates. + +If running targeted tests in a fresh worktree: + +```sh +if [ ! -x node_modules/.bin/vitest ]; then + pnpm install --frozen-lockfile +fi +``` + +5. Commit prep fixes with required subject format + +Use `scripts/committer` with explicit file paths. + +Required subject format: + +- `fix: (openclaw#) thanks @` + +```sh +commit_msg="fix: (openclaw#$pr_number) thanks @$contrib" +scripts/committer "$commit_msg" ... +``` + +If there are no local changes, do not create a no-op commit. + +Post-commit validation (mandatory): + +```sh +subject=$(git log -1 --pretty=%s) +echo "$subject" | rg -q "openclaw#$pr_number" || { echo "ERROR: commit subject missing openclaw#$pr_number"; exit 1; } +echo "$subject" | rg -q "thanks @$contrib" || { echo "ERROR: commit subject missing thanks @$contrib"; exit 1; } +``` + +6. Decide verification mode and run required gates before pushing + +If you are highly confident the change is docs-only, you may skip `pnpm test`. + +High-confidence docs-only criteria (all must be true): + +- Every changed file is documentation-only (`docs/**`, `README*.md`, `CHANGELOG.md`, `*.md`, `*.mdx`, `mintlify.json`, `docs.json`). +- No code, runtime, test, dependency, or build config files changed (`src/**`, `extensions/**`, `apps/**`, `package.json`, lockfiles, TS/JS config, test files, scripts). +- `.local/review.md` does not call for non-doc behavior fixes. + +Suggested check: + +```sh +changed_files=$(git diff --name-only origin/main...HEAD) +non_docs=$(printf "%s\n" "$changed_files" | grep -Ev '^(docs/|README.*\.md$|CHANGELOG\.md$|.*\.md$|.*\.mdx$|mintlify\.json$|docs\.json$)' || true) + +docs_only=false +if [ -n "$changed_files" ] && [ -z "$non_docs" ]; then + docs_only=true +fi + +echo "docs_only=$docs_only" +``` + +Bootstrap dependencies in a fresh worktree before gates: + +```sh +if [ ! -d node_modules ]; then + pnpm install --frozen-lockfile +fi +``` + +Run required gates: + +```sh +pnpm build +pnpm check + +if [ "$docs_only" = "true" ]; then + echo "Docs-only change detected with high confidence; skipping pnpm test." | tee -a .local/prep.md +else + pnpm test +fi +``` + +Require all required gates to pass. If something fails, fix, commit, and rerun. Allow at most 3 fix-and-rerun cycles. + +7. Push safely to the PR head branch + +Build `prhead` from owner/name first, then validate remote branch SHA before push. + +```sh +if [ -n "$head_owner" ] && [ -n "$head_repo_name" ]; then + head_repo_push_url="https://github.com/$head_owner/$head_repo_name.git" +elif [ -n "$head_repo_url" ] && [ "$head_repo_url" != "null" ]; then + case "$head_repo_url" in + *.git) head_repo_push_url="$head_repo_url" ;; + *) head_repo_push_url="$head_repo_url.git" ;; + esac +else + echo "ERROR: unable to determine PR head repo push URL" + exit 1 +fi + +git remote add prhead "$head_repo_push_url" 2>/dev/null || git remote set-url prhead "$head_repo_push_url" + +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 + +remote_sha=$(git ls-remote prhead "refs/heads/$head" | awk '{print $1}') +if [ -z "$remote_sha" ]; then + echo "ERROR: remote branch refs/heads/$head not found on prhead" + exit 1 +fi +if [ "$remote_sha" != "$pr_head_sha_before" ]; then + echo "ERROR: expected remote SHA $pr_head_sha_before, got $remote_sha. Re-fetch metadata and rebase first." + exit 1 +fi + +git push --force-with-lease=refs/heads/$head:$pr_head_sha_before prhead HEAD:$head || push_failed=1 +``` + +If lease push fails because head moved, perform one automatic retry: + +```sh +if [ "${push_failed:-0}" = "1" ]; then + echo "Lease push failed, retrying once with fresh PR head..." + + pr_head_sha_before=$(gh pr view --json headRefOid --jq .headRefOid) + git fetch origin pull//head:pr--latest --force + git rebase pr--latest + + pnpm build + pnpm check + if [ "$docs_only" != "true" ]; then + pnpm test + fi + + git push --force-with-lease=refs/heads/$head:$pr_head_sha_before prhead HEAD:$head +fi +``` + +8. Verify PR head and base relation (Mandatory) + +```sh +prep_head_sha=$(git rev-parse HEAD) +pr_head_sha_after=$(gh pr view --json headRefOid --jq .headRefOid) + +if [ "$prep_head_sha" != "$pr_head_sha_after" ]; then + echo "ERROR: pushed head SHA does not match PR head SHA." + exit 1 +fi + +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" && exit 1) +git branch -D pr--verify 2>/dev/null || true +``` + +9. Write prep summary artifacts (Mandatory) + +Write `.local/prep.md` and `.local/prep.env` for merge handoff. + +```sh +contrib_id=$(gh api users/$contrib --jq .id) +coauthor_email="${contrib_id}+${contrib}@users.noreply.github.com" + +cat > .local/prep.env < origin/main +else + git worktree add "$WORKTREE_DIR" -b temp/pr- origin/main + cd "$WORKTREE_DIR" +fi + +# Create local scratch space that persists across /review-pr to /prepare-pr to /merge-pr +mkdir -p .local +``` + +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. Identify PR meta and context + +```sh +pr_meta_json=$(gh pr view --json number,title,state,isDraft,author,baseRefName,headRefName,headRefOid,headRepository,url,body,labels,assignees,reviewRequests,files,additions,deletions,statusCheckRollup) +printf '%s\n' "$pr_meta_json" | jq '{number,title,url,state,isDraft,author:.author.login,base:.baseRefName,head:.headRefName,headSha:.headRefOid,headRepo:.headRepository.nameWithOwner,additions,deletions,files:(.files|length),body}' + +cat > .local/pr-meta.env <" -S src packages apps ui || true +rg -n "" -S src packages apps ui || true + +git log --oneline --all --grep="" | head -20 +``` + +If it already exists, call it out as a BLOCKER or at least IMPORTANT. + +3. Claim the PR + +Assign yourself so others know someone is reviewing. Skip if the PR looks like spam or is a draft you plan to recommend closing. + +```sh +gh_user=$(gh api user --jq .login) +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: + +```sh +gh pr diff +``` + +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 +git fetch origin pull//head:pr- --force +mb=$(git merge-base origin/main pr-) + +# Show only this PR patch relative to merge-base, not total branch drift +git diff --stat "$mb"..pr- +git diff "$mb"..pr- +``` + +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 +# Use only if needed +# git checkout pr- +# git branch --show-current +# ...inspect files... + +git checkout temp/pr- +git checkout -B temp/pr- origin/main +git branch --show-current +``` + +6. Validate the change is needed and valuable + +Be honest. Call out low value AI slop. + +7. Evaluate implementation quality + +Review correctness, design, performance, and ergonomics. + +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. + +If you run local tests in the worktree, bootstrap dependencies first: + +```sh +if [ ! -x node_modules/.bin/vitest ]; then + pnpm install --frozen-lockfile +fi +``` + +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 /prepare-pr, only flag it here. + +12. Answer the key question + +Decide if /prepare-pr 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 +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 /prepare-pr | 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. diff --git a/.agents/archive/review-pr-v1/agents/openai.yaml b/.agents/archive/review-pr-v1/agents/openai.yaml new file mode 100644 index 0000000000..f659349950 --- /dev/null +++ b/.agents/archive/review-pr-v1/agents/openai.yaml @@ -0,0 +1,4 @@ +interface: + display_name: "Review PR" + short_description: "Review GitHub PRs without merging" + default_prompt: "Use $review-pr to perform a thorough, review-only GitHub PR review." diff --git a/.agents/skills/PR_WORKFLOW.md b/.agents/skills/PR_WORKFLOW.md index f091bfe12b..b4de1c49ec 100644 --- a/.agents/skills/PR_WORKFLOW.md +++ b/.agents/skills/PR_WORKFLOW.md @@ -9,7 +9,7 @@ Process PRs **oldest to newest**. Older PRs are more likely to have merge confli ## 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. These three skills must be used in order: @@ -25,6 +25,65 @@ 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 + +Skill runs should invoke these wrappers automatically. You only need to run them manually when debugging or doing an explicit script-only run: + +- `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 ` + +These wrappers run shared preflight checks and generate deterministic artifacts. They are designed to work from repo root or PR worktree cwd. + +## Required artifacts + +- `.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. + +## 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 - Do not trust PR code by default. @@ -40,35 +99,52 @@ Do not continue if you cannot verify the problem is real or test the fix. 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` is the first step, before fixing findings or running gates. +- 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 -- Create commits with `scripts/committer "" `; avoid manual `git add`/`git commit` so staging stays scoped. +- 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 this commit subject format: `fix: (openclaw#) thanks @`. - Group related changes; avoid bundling unrelated refactors. -- Changelog workflow: keep latest released version at top (no `Unreleased`); after publishing, bump version and start a new top section. +- 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. - When working on an issue: reference the issue in the changelog entry. - Pure test additions/fixes generally do **not** need a changelog entry unless they alter user-facing behavior or the user asks for one. +## 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. +- 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 and include the SHA hashes. -- When merging a PR from a new contributor: run `bun scripts/update-clawtributors.ts` to add their avatar to the README "Thanks to all clawtributors" list, then commit the regenerated README. +- 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:** 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: contributor needs to be in git graph after this! +- **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: run `git pull`; if there are local changes or unpushed commits, stop and alert the user before reviewing. +- 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. @@ -98,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? ``` @@ -115,26 +190,29 @@ 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`). 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: @@ -148,7 +226,8 @@ Stop and escalate instead of continuing if: 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: @@ -161,6 +240,7 @@ 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: diff --git a/.agents/skills/merge-pr/SKILL.md b/.agents/skills/merge-pr/SKILL.md index 4bf02231d7..ae89b1a274 100644 --- a/.agents/skills/merge-pr/SKILL.md +++ b/.agents/skills/merge-pr/SKILL.md @@ -1,187 +1,98 @@ --- name: merge-pr -description: Merge a GitHub PR via squash after /prepare-pr. 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. +- Never use `gh pr merge --auto` in this flow. +- Never run `git push` directly. +- Require `--match-head-commit` during merge. -## 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 `/prepare-pr` +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 /prepare-pr." - 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 /prepare-pr 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. - -If `.local/prep.md` contains `Docs-only change detected with high confidence; skipping pnpm test.`, that local test skip is allowed. CI checks still must be green. +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 /prepare-pr" +scripts/pr-merge verify +source .local/prep.env ``` -If anything is failing or behind, stop and say to run `/prepare-pr`. +`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 ``` -If merge fails, report the error and stop. Do not retry in a loop. -If the PR needs changes beyond what `/prepare-pr` already did, stop and say to run `/prepare-pr` again. +`scripts/pr-merge run` performs: -4. Get merge SHA +- 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. 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. PR comment +5. Cleanup -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 -``` +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 a68fd5c7b5..e219141eb7 100644 --- a/.agents/skills/prepare-pr/SKILL.md +++ b/.agents/skills/prepare-pr/SKILL.md @@ -1,277 +1,131 @@ --- 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 /review-pr. 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. +- 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 required gates and pass (docs-only PRs may skip `pnpm test` when high-confidence docs-only criteria are met and documented). -- 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 with required subject format and validate it. + +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 /review-pr" -else - echo "Missing .local/review.md. Run /review-pr 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 when required ```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`. +Required commit subject format: -If the rebase gets confusing or you resolve conflicts 3 or more times, stop and report. +- `fix: (openclaw#) thanks @` -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 +source .local/pr-meta.env +scripts/committer "fix: (openclaw#$PR_NUMBER) thanks @$PR_AUTHOR" ... ``` -- 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: +Validate commit subject: ```sh -git add ... +scripts/pr-prepare validate-commit ``` -Preferred commit tool: +5. Run gates ```sh -committer "fix: (#) (thanks @$contrib)" +scripts/pr-prepare gates ``` -If `committer` is not found: +6. Push safely to PR head ```sh -git commit -m "fix: (#) (thanks @$contrib)" +scripts/pr-prepare push ``` -8. Decide verification mode and run required gates before pushing +This push step includes: -If you are highly confident the change is docs-only, you may skip `pnpm test`. +- 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. -High-confidence docs-only criteria (all must be true): - -- Every changed file is documentation-only (`docs/**`, `README*.md`, `CHANGELOG.md`, `*.md`, `*.mdx`, `mintlify.json`, `docs.json`). -- No code, runtime, test, dependency, or build config files changed (`src/**`, `extensions/**`, `apps/**`, `package.json`, lockfiles, TS/JS config, test files, scripts). -- `.local/review.md` does not call for non-doc behavior fixes. - -Suggested check: +7. Verify handoff artifacts ```sh -changed_files=$(git diff --name-only origin/main...HEAD) -non_docs=$(printf "%s\n" "$changed_files" | grep -Ev '^(docs/|README.*\.md$|CHANGELOG\.md$|.*\.md$|.*\.mdx$|mintlify\.json$|docs\.json$)' || true) - -docs_only=false -if [ -n "$changed_files" ] && [ -z "$non_docs" ]; then - docs_only=true -fi - -echo "docs_only=$docs_only" +ls -la .local/prep.md .local/prep.env ``` -Run required gates: +8. Output -```sh -pnpm install -pnpm build -pnpm ui:build -pnpm check - -if [ "$docs_only" = "true" ]; then - echo "Docs-only change detected with high confidence; skipping pnpm test." | tee -a .local/prep.md -else - pnpm test -fi -``` - -Require all required gates 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 -``` - -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 required gates before pushing. `pnpm test` may be skipped only for high-confidence docs-only changes, and the skip must be explicitly recorded in `.local/prep.md`. +- 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 04e4aa6c69..7327b34333 100644 --- a/.agents/skills/review-pr/SKILL.md +++ b/.agents/skills/review-pr/SKILL.md @@ -1,228 +1,141 @@ --- 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 /prepare-pr. +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. +- Never push, merge, or modify code intended to keep. +- Work only in `.worktrees/pr-`. -## 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 /review-pr to /prepare-pr to /merge-pr -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. Claim the PR - -Assign yourself so others know someone is reviewing. Skip if the PR looks like spam or is a draft you plan to recommend closing. +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|not_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 /prepare-pr, only flag it here. - -12. Answer the key question - -Decide if /prepare-pr 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 /prepare-pr | 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. diff --git a/scripts/pr b/scripts/pr new file mode 100755 index 0000000000..d32fbdb009 --- /dev/null +++ b/scripts/pr @@ -0,0 +1,1056 @@ +#!/usr/bin/env bash + +set -euo pipefail + +usage() { + cat < + scripts/pr review-checkout-main + scripts/pr review-checkout-pr + scripts/pr review-guard + scripts/pr review-artifacts-init + scripts/pr review-validate-artifacts + scripts/pr review-tests [ ...] + scripts/pr prepare-init + scripts/pr prepare-validate-commit + scripts/pr prepare-gates + scripts/pr prepare-push + scripts/pr prepare-run + scripts/pr merge-verify + scripts/pr merge-run +USAGE +} + +require_cmds() { + local missing=() + local cmd + for cmd in git gh jq rg pnpm node; do + if ! command -v "$cmd" >/dev/null 2>&1; then + missing+=("$cmd") + fi + done + + if [ "${#missing[@]}" -gt 0 ]; then + echo "Missing required command(s): ${missing[*]}" + exit 1 + fi +} + +repo_root() { + # Resolve canonical root from script location so wrappers work from root or worktree cwd. + local script_dir + script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" + (cd "$script_dir/.." && pwd) +} + +enter_worktree() { + local pr="$1" + local reset_to_main="${2:-false}" + local invoke_cwd + invoke_cwd="$PWD" + local root + root=$(repo_root) + + if [ "$invoke_cwd" != "$root" ]; then + echo "Detected non-root invocation cwd=$invoke_cwd, using canonical root $root" + fi + + cd "$root" + gh auth status >/dev/null + git fetch origin main + + local dir=".worktrees/pr-$pr" + if [ -d "$dir" ]; then + cd "$dir" + git fetch origin main + if [ "$reset_to_main" = "true" ]; then + git checkout -B "temp/pr-$pr" origin/main + fi + else + git worktree add "$dir" -b "temp/pr-$pr" origin/main + cd "$dir" + fi + + mkdir -p .local +} + +pr_meta_json() { + local pr="$1" + gh pr view "$pr" --json number,title,state,isDraft,author,baseRefName,headRefName,headRefOid,headRepository,headRepositoryOwner,url,body,labels,assignees,reviewRequests,files,additions,deletions,statusCheckRollup +} + +write_pr_meta_files() { + local json="$1" + + printf '%s\n' "$json" > .local/pr-meta.json + + cat > .local/pr-meta.env </dev/null || true) + local git_email + git_email=$(git config user.email 2>/dev/null || true) + + printf '%s\n' \ + "$gh_email" \ + "$git_email" \ + "${reviewer_id}+${reviewer}@users.noreply.github.com" \ + "${reviewer}@users.noreply.github.com" | awk 'NF && !seen[$0]++' +} + +checkout_prep_branch() { + local pr="$1" + require_artifact .local/prep-context.env + # shellcheck disable=SC1091 + source .local/prep-context.env + + local prep_branch="${PREP_BRANCH:-pr-$pr-prep}" + if ! git show-ref --verify --quiet "refs/heads/$prep_branch"; then + echo "Expected prep branch $prep_branch not found. Run prepare-init first." + exit 1 + fi + + git checkout "$prep_branch" +} + +resolve_head_push_url() { + # shellcheck disable=SC1091 + source .local/pr-meta.env + + if [ -n "${PR_HEAD_OWNER:-}" ] && [ -n "${PR_HEAD_REPO_NAME:-}" ]; then + printf 'https://github.com/%s/%s.git\n' "$PR_HEAD_OWNER" "$PR_HEAD_REPO_NAME" + return 0 + fi + + if [ -n "${PR_HEAD_REPO_URL:-}" ] && [ "$PR_HEAD_REPO_URL" != "null" ]; then + case "$PR_HEAD_REPO_URL" in + *.git) printf '%s\n' "$PR_HEAD_REPO_URL" ;; + *) printf '%s.git\n' "$PR_HEAD_REPO_URL" ;; + esac + return 0 + fi + + return 1 +} + +set_review_mode() { + local mode="$1" + cat > .local/review-mode.env < .local/review.md <<'EOF_MD' +A) TL;DR recommendation + +B) What changed + +C) What is good + +D) Security findings + +E) Concerns or questions (actionable) + +F) Tests + +G) Docs status + +H) Changelog + +I) Follow ups (optional) + +J) Suggested PR comment (optional) +EOF_MD + fi + + if [ ! -f .local/review.json ]; then + cat > .local/review.json <<'EOF_JSON' +{ + "recommendation": "READY FOR /prepare-pr", + "findings": [], + "tests": { + "ran": [], + "gaps": [], + "result": "pass" + }, + "docs": "not_applicable", + "changelog": "not_required" +} +EOF_JSON + fi + + echo "review artifact templates are ready" + echo "files=.local/review.md .local/review.json" +} + +review_validate_artifacts() { + local pr="$1" + enter_worktree "$pr" false + require_artifact .local/review.md + require_artifact .local/review.json + require_artifact .local/pr-meta.env + + review_guard "$pr" + + jq . .local/review.json >/dev/null + + local section + for section in "A)" "B)" "C)" "D)" "E)" "F)" "G)" "H)" "I)" "J)"; do + awk -v s="$section" 'index($0, s) == 1 { found=1; exit } END { exit(found ? 0 : 1) }' .local/review.md || { + echo "Missing section header in .local/review.md: $section" + exit 1 + } + done + + local recommendation + recommendation=$(jq -r '.recommendation // ""' .local/review.json) + case "$recommendation" in + "READY FOR /prepare-pr"|"NEEDS WORK"|"NEEDS DISCUSSION"|"NOT USEFUL (CLOSE)") + ;; + *) + echo "Invalid recommendation in .local/review.json: $recommendation" + exit 1 + ;; + esac + + local invalid_severity_count + invalid_severity_count=$(jq '[.findings[]? | select((.severity // "") != "BLOCKER" and (.severity // "") != "IMPORTANT" and (.severity // "") != "NIT")] | length' .local/review.json) + if [ "$invalid_severity_count" -gt 0 ]; then + echo "Invalid finding severity in .local/review.json" + exit 1 + fi + + local invalid_findings_count + invalid_findings_count=$(jq '[.findings[]? | select((.id|type)!="string" or (.title|type)!="string" or (.area|type)!="string" or (.fix|type)!="string")] | length' .local/review.json) + if [ "$invalid_findings_count" -gt 0 ]; then + echo "Invalid finding shape in .local/review.json (id/title/area/fix must be strings)" + exit 1 + fi + + local docs_status + docs_status=$(jq -r '.docs // ""' .local/review.json) + case "$docs_status" in + "up_to_date"|"missing"|"not_applicable") + ;; + *) + echo "Invalid docs status in .local/review.json: $docs_status" + exit 1 + ;; + esac + + local changelog_status + changelog_status=$(jq -r '.changelog // ""' .local/review.json) + case "$changelog_status" in + "required"|"not_required") + ;; + *) + echo "Invalid changelog status in .local/review.json: $changelog_status" + exit 1 + ;; + esac + + if [ "$changelog_status" = "required" ]; then + local changelog_finding_count + changelog_finding_count=$(jq '[.findings[]? | select(((.area // "" | ascii_downcase | contains("changelog")) or (.title // "" | ascii_downcase | contains("changelog")) or (.fix // "" | ascii_downcase | contains("changelog"))))] | length' .local/review.json) + if [ "$changelog_finding_count" -eq 0 ]; then + echo "changelog is required but no changelog-related finding exists in .local/review.json" + exit 1 + fi + fi + + echo "review artifacts validated" +} + +review_tests() { + local pr="$1" + shift + if [ "$#" -lt 1 ]; then + echo "Usage: scripts/pr review-tests [ ...]" + exit 2 + fi + + enter_worktree "$pr" false + review_guard "$pr" + + local target + for target in "$@"; do + if [ ! -f "$target" ]; then + echo "Missing test target file: $target" + exit 1 + fi + done + + bootstrap_deps_if_needed + + local list_log=".local/review-tests-list.log" + pnpm vitest list "$@" 2>&1 | tee "$list_log" + + local missing_list=() + for target in "$@"; do + local base + base=$(basename "$target") + if ! rg -F -q "$target" "$list_log" && ! rg -F -q "$base" "$list_log"; then + missing_list+=("$target") + fi + done + + if [ "${#missing_list[@]}" -gt 0 ]; then + echo "These requested targets were not selected by vitest list:" + printf ' - %s\n' "${missing_list[@]}" + exit 1 + fi + + local run_log=".local/review-tests-run.log" + pnpm vitest run "$@" 2>&1 | tee "$run_log" + + local missing_run=() + for target in "$@"; do + local base + base=$(basename "$target") + if ! rg -F -q "$target" "$run_log" && ! rg -F -q "$base" "$run_log"; then + missing_run+=("$target") + fi + done + + if [ "${#missing_run[@]}" -gt 0 ]; then + echo "These requested targets were not observed in vitest run output:" + printf ' - %s\n' "${missing_run[@]}" + exit 1 + fi + + { + echo "REVIEW_TESTS_AT=$(date -u +%Y-%m-%dT%H:%M:%SZ)" + echo "REVIEW_TEST_TARGET_COUNT=$#" + } > .local/review-tests.env + + echo "review tests passed and were observed in output" +} + +review_init() { + local pr="$1" + enter_worktree "$pr" true + + local json + json=$(pr_meta_json "$pr") + write_pr_meta_files "$json" + + git fetch origin "pull/$pr/head:pr-$pr" --force + local mb + mb=$(git merge-base origin/main "pr-$pr") + + cat > .local/review-context.env < .local/prep-context.env < .local/prep.md < .local/gates.env </dev/null || git remote set-url prhead "$push_url" + + local remote_sha + remote_sha=$(git ls-remote prhead "refs/heads/$PR_HEAD" | awk '{print $1}') + if [ -z "$remote_sha" ]; then + echo "Remote branch refs/heads/$PR_HEAD not found on prhead" + exit 1 + fi + + local pushed_from_sha="$remote_sha" + if [ "$remote_sha" = "$prep_head_sha" ]; then + echo "Remote branch already at local prep HEAD; skipping push." + else + if [ "$remote_sha" != "$lease_sha" ]; then + echo "Remote SHA $remote_sha differs from PR head SHA $lease_sha. Refreshing lease SHA from remote." + lease_sha="$remote_sha" + fi + pushed_from_sha="$lease_sha" + if ! git push --force-with-lease=refs/heads/$PR_HEAD:$lease_sha prhead HEAD:$PR_HEAD; then + echo "Lease push failed, retrying once with fresh PR head..." + + lease_sha=$(gh pr view "$pr" --json headRefOid --jq .headRefOid) + pushed_from_sha="$lease_sha" + + git fetch origin "pull/$pr/head:pr-$pr-latest" --force + git rebase "pr-$pr-latest" + prep_head_sha=$(git rev-parse HEAD) + + bootstrap_deps_if_needed + pnpm build + pnpm check + if [ "${DOCS_ONLY:-false}" != "true" ]; then + pnpm test + fi + + git push --force-with-lease=refs/heads/$PR_HEAD:$lease_sha prhead HEAD:$PR_HEAD + fi + fi + + if ! wait_for_pr_head_sha "$pr" "$prep_head_sha" 8 3; then + local observed_sha + observed_sha=$(gh pr view "$pr" --json headRefOid --jq .headRefOid) + echo "Pushed head SHA propagation timed out. expected=$prep_head_sha observed=$observed_sha" + exit 1 + fi + + local pr_head_sha_after + pr_head_sha_after=$(gh pr view "$pr" --json headRefOid --jq .headRefOid) + + git fetch origin main + git fetch origin "pull/$pr/head:pr-$pr-verify" --force + git merge-base --is-ancestor origin/main "pr-$pr-verify" || { + echo "PR branch is behind main after push." + exit 1 + } + git branch -D "pr-$pr-verify" 2>/dev/null || true + + local contrib="${PR_AUTHOR:-}" + if [ -z "$contrib" ]; then + contrib=$(gh pr view "$pr" --json author --jq .author.login) + fi + local contrib_id + contrib_id=$(gh api "users/$contrib" --jq .id) + local coauthor_email="${contrib_id}+${contrib}@users.noreply.github.com" + + cat >> .local/prep.md < .local/prep.env </dev/null + + echo "prepare-push complete" + echo "prep_branch=$(git branch --show-current)" + echo "prep_head_sha=$prep_head_sha" + echo "pr_head_sha=$pr_head_sha_after" + echo "artifacts=.local/prep.md .local/prep.env" +} + +prepare_run() { + local pr="$1" + prepare_init "$pr" + prepare_validate_commit "$pr" + prepare_gates "$pr" + prepare_push "$pr" + echo "prepare-run complete for PR #$pr" +} + +merge_verify() { + local pr="$1" + enter_worktree "$pr" false + + require_artifact .local/prep.env + # shellcheck disable=SC1091 + source .local/prep.env + + local json + json=$(pr_meta_json "$pr") + local is_draft + is_draft=$(printf '%s\n' "$json" | jq -r .isDraft) + if [ "$is_draft" = "true" ]; then + echo "PR is draft." + exit 1 + fi + local pr_head_sha + pr_head_sha=$(printf '%s\n' "$json" | jq -r .headRefOid) + + if [ "$pr_head_sha" != "$PREP_HEAD_SHA" ]; then + echo "PR head changed after prepare (expected $PREP_HEAD_SHA, got $pr_head_sha)." + echo "Re-run prepare to refresh prep artifacts and gates: scripts/pr-prepare run $pr" + + # Best-effort delta summary to show exactly what changed since PREP_HEAD_SHA. + git fetch origin "pull/$pr/head" >/dev/null 2>&1 || true + if git cat-file -e "${PREP_HEAD_SHA}^{commit}" 2>/dev/null && git cat-file -e "${pr_head_sha}^{commit}" 2>/dev/null; then + echo "HEAD delta (expected...current):" + git log --oneline --left-right "${PREP_HEAD_SHA}...${pr_head_sha}" | sed 's/^/ /' || true + else + echo "HEAD delta unavailable locally (could not resolve one of the SHAs)." + fi + exit 1 + fi + + gh pr checks "$pr" --required --watch --fail-fast || true + local checks_json + local checks_err_file + checks_err_file=$(mktemp) + checks_json=$(gh pr checks "$pr" --required --json name,bucket,state 2>"$checks_err_file" || true) + rm -f "$checks_err_file" + if [ -z "$checks_json" ]; then + checks_json='[]' + fi + local required_count + required_count=$(printf '%s\n' "$checks_json" | jq 'length') + if [ "$required_count" -eq 0 ]; then + echo "No required checks configured for this PR." + fi + printf '%s\n' "$checks_json" | jq -r '.[] | "\(.bucket)\t\(.name)\t\(.state)"' + + local failed_required + failed_required=$(printf '%s\n' "$checks_json" | jq '[.[] | select(.bucket=="fail")] | length') + local pending_required + pending_required=$(printf '%s\n' "$checks_json" | jq '[.[] | select(.bucket=="pending")] | length') + + if [ "$failed_required" -gt 0 ]; then + echo "Required checks are failing." + exit 1 + fi + + if [ "$pending_required" -gt 0 ]; then + echo "Required checks are still pending." + exit 1 + fi + + git fetch origin main + git fetch origin "pull/$pr/head:pr-$pr" --force + git merge-base --is-ancestor origin/main "pr-$pr" || { + echo "PR branch is behind main." + exit 1 + } + + echo "merge-verify passed for PR #$pr" +} + +merge_run() { + local pr="$1" + enter_worktree "$pr" false + + local required + for required in .local/review.md .local/review.json .local/prep.md .local/prep.env; do + require_artifact "$required" + done + + merge_verify "$pr" + # shellcheck disable=SC1091 + source .local/prep.env + + local pr_meta_json + pr_meta_json=$(gh pr view "$pr" --json number,title,state,isDraft,author) + local pr_title + pr_title=$(printf '%s\n' "$pr_meta_json" | jq -r .title) + local pr_number + pr_number=$(printf '%s\n' "$pr_meta_json" | jq -r .number) + local contrib + contrib=$(printf '%s\n' "$pr_meta_json" | jq -r .author.login) + local is_draft + is_draft=$(printf '%s\n' "$pr_meta_json" | jq -r .isDraft) + if [ "$is_draft" = "true" ]; then + echo "PR is draft; stop." + exit 1 + fi + + local reviewer + reviewer=$(gh api user --jq .login) + local reviewer_id + reviewer_id=$(gh api user --jq .id) + + local contrib_coauthor_email="${COAUTHOR_EMAIL:-}" + if [ -z "$contrib_coauthor_email" ] || [ "$contrib_coauthor_email" = "null" ]; then + local contrib_id + contrib_id=$(gh api "users/$contrib" --jq .id) + contrib_coauthor_email="${contrib_id}+${contrib}@users.noreply.github.com" + fi + + local reviewer_email_candidates=() + local reviewer_email_candidate + while IFS= read -r reviewer_email_candidate; do + [ -n "$reviewer_email_candidate" ] || continue + reviewer_email_candidates+=("$reviewer_email_candidate") + done < <(merge_author_email_candidates "$reviewer" "$reviewer_id") + if [ "${#reviewer_email_candidates[@]}" -eq 0 ]; then + echo "Unable to resolve a candidate merge author email for reviewer $reviewer" + exit 1 + fi + + local reviewer_email="${reviewer_email_candidates[0]}" + local reviewer_coauthor_email="${reviewer_id}+${reviewer}@users.noreply.github.com" + + cat > .local/merge-body.txt < /prepare-pr -> /merge-pr. + +Prepared head SHA: $PREP_HEAD_SHA +Co-authored-by: $contrib <$contrib_coauthor_email> +Co-authored-by: $reviewer <$reviewer_coauthor_email> +Reviewed-by: @$reviewer +EOF_BODY + + run_merge_with_email() { + local email="$1" + local merge_output_file + merge_output_file=$(mktemp) + if gh pr merge "$pr" \ + --squash \ + --delete-branch \ + --match-head-commit "$PREP_HEAD_SHA" \ + --author-email "$email" \ + --subject "$pr_title (#$pr_number)" \ + --body-file .local/merge-body.txt \ + >"$merge_output_file" 2>&1 + then + cat "$merge_output_file" + rm -f "$merge_output_file" + return 0 + fi + + MERGE_ERR_MSG=$(cat "$merge_output_file") + [ -n "$MERGE_ERR_MSG" ] && printf '%s\n' "$MERGE_ERR_MSG" >&2 + rm -f "$merge_output_file" + return 1 + } + + local MERGE_ERR_MSG="" + local selected_merge_author_email="$reviewer_email" + if ! run_merge_with_email "$selected_merge_author_email"; then + if is_author_email_merge_error "$MERGE_ERR_MSG" && [ "${#reviewer_email_candidates[@]}" -ge 2 ]; then + selected_merge_author_email="${reviewer_email_candidates[1]}" + echo "Retrying merge once with fallback author email: $selected_merge_author_email" + run_merge_with_email "$selected_merge_author_email" || { + echo "Merge failed after fallback retry." + exit 1 + } + else + echo "Merge failed." + exit 1 + fi + fi + + local state + state=$(gh pr view "$pr" --json state --jq .state) + if [ "$state" != "MERGED" ]; then + echo "Merge not finalized yet (state=$state), waiting up to 15 minutes..." + local i + for i in $(seq 1 90); do + sleep 10 + state=$(gh pr view "$pr" --json state --jq .state) + if [ "$state" = "MERGED" ]; then + break + fi + done + fi + + if [ "$state" != "MERGED" ]; then + echo "PR state is $state after waiting." + exit 1 + fi + + local merge_sha + merge_sha=$(gh pr view "$pr" --json mergeCommit --jq '.mergeCommit.oid') + if [ -z "$merge_sha" ] || [ "$merge_sha" = "null" ]; then + echo "Merge commit SHA missing." + exit 1 + fi + + local commit_body + commit_body=$(gh api repos/:owner/:repo/commits/"$merge_sha" --jq .commit.message) + printf '%s\n' "$commit_body" | rg -q "^Co-authored-by: $contrib <" || { echo "Missing PR author co-author trailer"; exit 1; } + printf '%s\n' "$commit_body" | rg -q "^Co-authored-by: $reviewer <" || { echo "Missing reviewer co-author trailer"; exit 1; } + + local ok=0 + local comment_output="" + local attempt + for attempt in 1 2 3; do + if comment_output=$(gh pr comment "$pr" -F - 2>&1 </dev/null || true + git branch -D "pr-$pr" 2>/dev/null || true + git branch -D "pr-$pr-prep" 2>/dev/null || true + + echo "merge-run complete for PR #$pr" + echo "merge_sha=$merge_sha" + echo "merge_author_email=$selected_merge_author_email" + echo "comment_url=$comment_url" +} + +main() { + if [ "$#" -lt 2 ]; then + usage + exit 2 + fi + + require_cmds + + local cmd="${1-}" + shift || true + local pr="${1-}" + shift || true + + if [ -z "$cmd" ] || [ -z "$pr" ]; then + usage + exit 2 + fi + + case "$cmd" in + review-init) + review_init "$pr" + ;; + review-checkout-main) + review_checkout_main "$pr" + ;; + review-checkout-pr) + review_checkout_pr "$pr" + ;; + review-guard) + review_guard "$pr" + ;; + review-artifacts-init) + review_artifacts_init "$pr" + ;; + review-validate-artifacts) + review_validate_artifacts "$pr" + ;; + review-tests) + review_tests "$pr" "$@" + ;; + prepare-init) + prepare_init "$pr" + ;; + prepare-validate-commit) + prepare_validate_commit "$pr" + ;; + prepare-gates) + prepare_gates "$pr" + ;; + prepare-push) + prepare_push "$pr" + ;; + prepare-run) + prepare_run "$pr" + ;; + merge-verify) + merge_verify "$pr" + ;; + merge-run) + merge_run "$pr" + ;; + *) + usage + exit 2 + ;; + esac +} + +main "$@" diff --git a/scripts/pr-merge b/scripts/pr-merge new file mode 100755 index 0000000000..745d74d885 --- /dev/null +++ b/scripts/pr-merge @@ -0,0 +1,37 @@ +#!/usr/bin/env bash +set -euo pipefail + +script_dir="$(cd "$(dirname "$0")" && pwd)" + +usage() { + cat < # verify only (backward compatible) + scripts/pr-merge verify # verify only + scripts/pr-merge run # verify + merge + post-merge checks + cleanup +USAGE +} + +if [ "$#" -eq 1 ]; then + exec "$script_dir/pr" merge-verify "$1" +fi + +if [ "$#" -eq 2 ]; then + mode="$1" + pr="$2" + case "$mode" in + verify) + exec "$script_dir/pr" merge-verify "$pr" + ;; + run) + exec "$script_dir/pr" merge-run "$pr" + ;; + *) + usage + exit 2 + ;; + esac +fi + +usage +exit 2 diff --git a/scripts/pr-prepare b/scripts/pr-prepare new file mode 100755 index 0000000000..c308aabf67 --- /dev/null +++ b/scripts/pr-prepare @@ -0,0 +1,33 @@ +#!/usr/bin/env bash +set -euo pipefail + +if [ "$#" -ne 2 ]; then + echo "Usage: scripts/pr-prepare " + exit 2 +fi + +mode="$1" +pr="$2" +base="$(cd "$(dirname "$0")" && pwd)/pr" + +case "$mode" in + init) + exec "$base" prepare-init "$pr" + ;; + validate-commit) + exec "$base" prepare-validate-commit "$pr" + ;; + gates) + exec "$base" prepare-gates "$pr" + ;; + push) + exec "$base" prepare-push "$pr" + ;; + run) + exec "$base" prepare-run "$pr" + ;; + *) + echo "Usage: scripts/pr-prepare " + exit 2 + ;; +esac diff --git a/scripts/pr-review b/scripts/pr-review new file mode 100755 index 0000000000..1376080e15 --- /dev/null +++ b/scripts/pr-review @@ -0,0 +1,3 @@ +#!/usr/bin/env bash +set -euo pipefail +exec "$(cd "$(dirname "$0")" && pwd)/pr" review-init "$@"