|
| 1 | +# PR Workflow for Maintainers |
| 2 | + |
| 3 | +Please read this in full and do not skip sections. |
| 4 | +This is the single source of truth for the maintainer PR workflow. |
| 5 | + |
| 6 | +## Triage order |
| 7 | + |
| 8 | +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. |
| 9 | + |
| 10 | +## Working rule |
| 11 | + |
| 12 | +Skills execute workflow. Maintainers provide judgment. |
| 13 | +Always pause between skills to evaluate technical direction, not just command success. |
| 14 | + |
| 15 | +These three skills must be used in order: |
| 16 | + |
| 17 | +1. `review-pr` — review only, produce findings |
| 18 | +2. `prepare-pr` — rebase, fix, gate, push to PR head branch |
| 19 | +3. `merge-pr` — squash-merge, verify MERGED state, clean up |
| 20 | + |
| 21 | +They are necessary, but not sufficient. Maintainers must steer between steps and understand the code before moving forward. |
| 22 | + |
| 23 | +Treat PRs as reports first, code second. |
| 24 | +If submitted code is low quality, ignore it and implement the best solution for the problem. |
| 25 | + |
| 26 | +Do not continue if you cannot verify the problem is real or test the fix. |
| 27 | + |
| 28 | +## Coding Agent |
| 29 | + |
| 30 | +Use ChatGPT 5.3 Codex High. Fall back to 5.2 Codex High or 5.3 Codex Medium if necessary. |
| 31 | + |
| 32 | +## PR quality bar |
| 33 | + |
| 34 | +- Do not trust PR code by default. |
| 35 | +- Do not merge changes you cannot validate with a reproducible problem and a tested fix. |
| 36 | +- Keep types strict. Do not use `any` in implementation code. |
| 37 | +- Keep external-input boundaries typed and validated, including CLI input, environment variables, network payloads, and tool output. |
| 38 | +- Keep implementations properly scoped. Fix root causes, not local symptoms. |
| 39 | +- Identify and reuse canonical sources of truth so behavior does not drift across the codebase. |
| 40 | +- Harden changes. Always evaluate security impact and abuse paths. |
| 41 | +- Understand the system before changing it. Never make the codebase messier just to clear a PR queue. |
| 42 | + |
| 43 | +## Rebase and conflict resolution |
| 44 | + |
| 45 | +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. |
| 46 | + |
| 47 | +- During `prepare-pr`: rebase onto `main` as the first step, before fixing findings or running gates. |
| 48 | +- If conflicts are complex or touch areas you do not understand, stop and escalate. |
| 49 | +- Prefer **rebase** for linear history; **squash** when commit history is messy or unhelpful. |
| 50 | + |
| 51 | +## Commit and changelog rules |
| 52 | + |
| 53 | +- Create commits with `scripts/committer "<msg>" <file...>`; avoid manual `git add`/`git commit` so staging stays scoped. |
| 54 | +- Follow concise, action-oriented commit messages (e.g., `CLI: add verbose flag to send`). |
| 55 | +- During `prepare-pr`, use this commit subject format: `fix: <summary> (openclaw#<PR>) thanks @<pr-author>`. |
| 56 | +- Group related changes; avoid bundling unrelated refactors. |
| 57 | +- Changelog workflow: keep the latest released version at the top (no `Unreleased`); after publishing, bump the version and start a new top section. |
| 58 | +- When working on a PR: add a changelog entry with the PR number and thank the contributor. |
| 59 | +- When working on an issue: reference the issue in the changelog entry. |
| 60 | +- Pure test additions/fixes generally do **not** need a changelog entry unless they alter user-facing behavior or the user asks for one. |
| 61 | + |
| 62 | +## Co-contributor and clawtributors |
| 63 | + |
| 64 | +- If we squash, add the PR author as a co-contributor in the commit body using a `Co-authored-by:` trailer. |
| 65 | +- When maintainer prepares and merges the PR, add the maintainer as an additional `Co-authored-by:` trailer too. |
| 66 | +- Avoid `--auto` merges for maintainer landings. Merge only after checks are green so the maintainer account is the actor and attribution is deterministic. |
| 67 | +- 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. |
| 68 | +- 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. |
| 69 | +- 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. |
| 70 | +- 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. |
| 71 | + |
| 72 | +## Review mode vs landing mode |
| 73 | + |
| 74 | +- **Review mode (PR link only):** read `gh pr view`/`gh pr diff`; **do not** switch branches; **do not** change code. |
| 75 | +- **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! |
| 76 | + |
| 77 | +## Pre-review safety checks |
| 78 | + |
| 79 | +- Before starting a review when a GH Issue/PR is pasted: use an isolated `.worktrees/pr-<PR>` checkout from `origin/main`. Do not require a clean main checkout, and do not run `git pull` in a dirty main checkout. |
| 80 | +- PR review calls: prefer a single `gh pr view --json ...` to batch metadata/comments; run `gh pr diff` only when needed. |
| 81 | +- PRs should summarize scope, note testing performed, and mention any user-facing changes or new flags. |
| 82 | +- Read `docs/help/submitting-a-pr.md` ([Submitting a PR](https://docs.openclaw.ai/help/submitting-a-pr)) for what we expect from contributors. |
| 83 | + |
| 84 | +## Unified workflow |
| 85 | + |
| 86 | +Entry criteria: |
| 87 | + |
| 88 | +- PR URL/number is known. |
| 89 | +- Problem statement is clear enough to attempt reproduction. |
| 90 | +- A realistic verification path exists (tests, integration checks, or explicit manual validation). |
| 91 | + |
| 92 | +### 1) `review-pr` |
| 93 | + |
| 94 | +Purpose: |
| 95 | + |
| 96 | +- Review only: correctness, value, security risk, tests, docs, and changelog impact. |
| 97 | +- Produce structured findings and a recommendation. |
| 98 | + |
| 99 | +Expected output: |
| 100 | + |
| 101 | +- Recommendation: ready, needs work, needs discussion, or close. |
| 102 | +- `.local/review.md` with actionable findings. |
| 103 | + |
| 104 | +Maintainer checkpoint before `prepare-pr`: |
| 105 | + |
| 106 | +``` |
| 107 | +What problem are they trying to solve? |
| 108 | +What is the most optimal implementation? |
| 109 | +Can we fix up everything? |
| 110 | +Do we have any questions? |
| 111 | +``` |
| 112 | + |
| 113 | +Stop and escalate instead of continuing if: |
| 114 | + |
| 115 | +- The problem cannot be reproduced or confirmed. |
| 116 | +- The proposed PR scope does not match the stated problem. |
| 117 | +- The design introduces unresolved security or trust-boundary concerns. |
| 118 | + |
| 119 | +### 2) `prepare-pr` |
| 120 | + |
| 121 | +Purpose: |
| 122 | + |
| 123 | +- Make the PR merge-ready on its head branch. |
| 124 | +- Rebase onto current `main` first, then fix blocker/important findings, then run gates. |
| 125 | +- In fresh worktrees, bootstrap dependencies before local gates (`pnpm install --frozen-lockfile`). |
| 126 | + |
| 127 | +Expected output: |
| 128 | + |
| 129 | +- Updated code and tests on the PR head branch. |
| 130 | +- `.local/prep.md` with changes, verification, and current HEAD SHA. |
| 131 | +- Final status: `PR is ready for /mergepr`. |
| 132 | + |
| 133 | +Maintainer checkpoint before `merge-pr`: |
| 134 | + |
| 135 | +``` |
| 136 | +Is this the most optimal implementation? |
| 137 | +Is the code properly scoped? |
| 138 | +Is the code properly reusing existing logic in the codebase? |
| 139 | +Is the code properly typed? |
| 140 | +Is the code hardened? |
| 141 | +Do we have enough tests? |
| 142 | +Do we need regression tests? |
| 143 | +Are tests using fake timers where appropriate? (e.g., debounce/throttle, retry backoff, timeout branches, delayed callbacks, polling loops) |
| 144 | +Do not add performative tests, ensure tests are real and there are no regressions. |
| 145 | +Do you see any follow-up refactors we should do? |
| 146 | +Take your time, fix it properly, refactor if necessary. |
| 147 | +Did any changes introduce any potential security vulnerabilities? |
| 148 | +``` |
| 149 | + |
| 150 | +Stop and escalate instead of continuing if: |
| 151 | + |
| 152 | +- You cannot verify behavior changes with meaningful tests or validation. |
| 153 | +- Fixing findings requires broad architecture changes outside safe PR scope. |
| 154 | +- Security hardening requirements remain unresolved. |
| 155 | + |
| 156 | +### 3) `merge-pr` |
| 157 | + |
| 158 | +Purpose: |
| 159 | + |
| 160 | +- Merge only after review and prep artifacts are present and checks are green. |
| 161 | +- Use deterministic squash merge flow (`--match-head-commit` + explicit subject/body with co-author trailer), then verify the PR ends in `MERGED` state. |
| 162 | +- If no required checks are configured on the PR, treat that as acceptable and continue after branch-up-to-date validation. |
| 163 | + |
| 164 | +Go or no-go checklist before merge: |
| 165 | + |
| 166 | +- All BLOCKER and IMPORTANT findings are resolved. |
| 167 | +- Verification is meaningful and regression risk is acceptably low. |
| 168 | +- Docs and changelog are updated when required. |
| 169 | +- Required CI checks are green and the branch is not behind `main`. |
| 170 | + |
| 171 | +Expected output: |
| 172 | + |
| 173 | +- Successful merge commit and recorded merge SHA. |
| 174 | +- Worktree cleanup after successful merge. |
| 175 | +- Comment on PR indicating merge was successful. |
| 176 | + |
| 177 | +Maintainer checkpoint after merge: |
| 178 | + |
| 179 | +- Were any refactors intentionally deferred and now need follow-up issue(s)? |
| 180 | +- Did this reveal broader architecture or test gaps we should address? |
| 181 | +- Run `bun scripts/update-clawtributors.ts` if the contributor is new. |
0 commit comments