|
| 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 | +## PR quality bar |
| 29 | + |
| 30 | +- Do not trust PR code by default. |
| 31 | +- Do not merge changes you cannot validate with a reproducible problem and a tested fix. |
| 32 | +- Keep types strict. Do not use `any` in implementation code. |
| 33 | +- Keep external-input boundaries typed and validated, including CLI input, environment variables, network payloads, and tool output. |
| 34 | +- Keep implementations properly scoped. Fix root causes, not local symptoms. |
| 35 | +- Identify and reuse canonical sources of truth so behavior does not drift across the codebase. |
| 36 | +- Harden changes. Always evaluate security impact and abuse paths. |
| 37 | +- Understand the system before changing it. Never make the codebase messier just to clear a PR queue. |
| 38 | + |
| 39 | +## Rebase and conflict resolution |
| 40 | + |
| 41 | +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. |
| 42 | + |
| 43 | +- During `prepare-pr`: rebase onto `main` is the first step, before fixing findings or running gates. |
| 44 | +- If conflicts are complex or touch areas you do not understand, stop and escalate. |
| 45 | +- Prefer **rebase** for linear history; **squash** when commit history is messy or unhelpful. |
| 46 | + |
| 47 | +## Commit and changelog rules |
| 48 | + |
| 49 | +- Create commits with `scripts/committer "<msg>" <file...>`; avoid manual `git add`/`git commit` so staging stays scoped. |
| 50 | +- Follow concise, action-oriented commit messages (e.g., `CLI: add verbose flag to send`). |
| 51 | +- Group related changes; avoid bundling unrelated refactors. |
| 52 | +- Changelog workflow: keep latest released version at top (no `Unreleased`); after publishing, bump version and start a new top section. |
| 53 | +- When working on a PR: add a changelog entry with the PR number and thank the contributor. |
| 54 | +- When working on an issue: reference the issue in the changelog entry. |
| 55 | +- Pure test additions/fixes generally do **not** need a changelog entry unless they alter user-facing behavior or the user asks for one. |
| 56 | + |
| 57 | +## Co-contributor and clawtributors |
| 58 | + |
| 59 | +- If we squash, add the PR author as a co-contributor in the commit. |
| 60 | +- 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. |
| 61 | +- When merging a PR: leave a PR comment that explains exactly what we did and include the SHA hashes. |
| 62 | +- 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. |
| 63 | + |
| 64 | +## Review mode vs landing mode |
| 65 | + |
| 66 | +- **Review mode (PR link only):** read `gh pr view`/`gh pr diff`; **do not** switch branches; **do not** change code. |
| 67 | +- **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! |
| 68 | + |
| 69 | +## Pre-review safety checks |
| 70 | + |
| 71 | +- 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. |
| 72 | +- PR review calls: prefer a single `gh pr view --json ...` to batch metadata/comments; run `gh pr diff` only when needed. |
| 73 | +- PRs should summarize scope, note testing performed, and mention any user-facing changes or new flags. |
| 74 | +- Read `docs/help/submitting-a-pr.md` ([Submitting a PR](https://docs.openclaw.ai/help/submitting-a-pr)) for what we expect from contributors. |
| 75 | + |
| 76 | +## Unified workflow |
| 77 | + |
| 78 | +Entry criteria: |
| 79 | + |
| 80 | +- PR URL/number is known. |
| 81 | +- Problem statement is clear enough to attempt reproduction. |
| 82 | +- A realistic verification path exists (tests, integration checks, or explicit manual validation). |
| 83 | + |
| 84 | +### 1) `review-pr` |
| 85 | + |
| 86 | +Purpose: |
| 87 | + |
| 88 | +- Review only: correctness, value, security risk, tests, docs, and changelog impact. |
| 89 | +- Produce structured findings and a recommendation. |
| 90 | + |
| 91 | +Expected output: |
| 92 | + |
| 93 | +- Recommendation: ready, needs work, needs discussion, or close. |
| 94 | +- `.local/review.md` with actionable findings. |
| 95 | + |
| 96 | +Maintainer checkpoint before `prepare-pr`: |
| 97 | + |
| 98 | +``` |
| 99 | +What problem are they trying to solve? |
| 100 | +What is the most optimal implementation? |
| 101 | +Is the code properly scoped? |
| 102 | +Can we fix up everything? |
| 103 | +Do we have any questions? |
| 104 | +``` |
| 105 | + |
| 106 | +Stop and escalate instead of continuing if: |
| 107 | + |
| 108 | +- The problem cannot be reproduced or confirmed. |
| 109 | +- The proposed PR scope does not match the stated problem. |
| 110 | +- The design introduces unresolved security or trust-boundary concerns. |
| 111 | + |
| 112 | +### 2) `prepare-pr` |
| 113 | + |
| 114 | +Purpose: |
| 115 | + |
| 116 | +- Make the PR merge-ready on its head branch. |
| 117 | +- Rebase onto current `main` first, then fix blocker/important findings, then run gates. |
| 118 | + |
| 119 | +Expected output: |
| 120 | + |
| 121 | +- Updated code and tests on the PR head branch. |
| 122 | +- `.local/prep.md` with changes, verification, and current HEAD SHA. |
| 123 | +- Final status: `PR is ready for /mergepr`. |
| 124 | + |
| 125 | +Maintainer checkpoint before `merge-pr`: |
| 126 | + |
| 127 | +``` |
| 128 | +Is this the most optimal implementation? |
| 129 | +Is the code properly scoped? |
| 130 | +Is the code properly typed? |
| 131 | +Is the code hardened? |
| 132 | +Do we have enough tests? |
| 133 | +Are tests using fake timers where relevant? (e.g., debounce/throttle, retry backoff, timeout branches, delayed callbacks, polling loops) |
| 134 | +Do not add performative tests, ensure tests are real and there are no regressions. |
| 135 | +Take your time, fix it properly, refactor if necessary. |
| 136 | +Do you see any follow-up refactors we should do? |
| 137 | +Did any changes introduce any potential security vulnerabilities? |
| 138 | +``` |
| 139 | + |
| 140 | +Stop and escalate instead of continuing if: |
| 141 | + |
| 142 | +- You cannot verify behavior changes with meaningful tests or validation. |
| 143 | +- Fixing findings requires broad architecture changes outside safe PR scope. |
| 144 | +- Security hardening requirements remain unresolved. |
| 145 | + |
| 146 | +### 3) `merge-pr` |
| 147 | + |
| 148 | +Purpose: |
| 149 | + |
| 150 | +- Merge only after review and prep artifacts are present and checks are green. |
| 151 | +- Use squash merge flow and verify the PR ends in `MERGED` state. |
| 152 | + |
| 153 | +Go or no-go checklist before merge: |
| 154 | + |
| 155 | +- All BLOCKER and IMPORTANT findings are resolved. |
| 156 | +- Verification is meaningful and regression risk is acceptably low. |
| 157 | +- Docs and changelog are updated when required. |
| 158 | +- Required CI checks are green and the branch is not behind `main`. |
| 159 | + |
| 160 | +Expected output: |
| 161 | + |
| 162 | +- Successful merge commit and recorded merge SHA. |
| 163 | +- Worktree cleanup after successful merge. |
| 164 | + |
| 165 | +Maintainer checkpoint after merge: |
| 166 | + |
| 167 | +- Were any refactors intentionally deferred and now need follow-up issue(s)? |
| 168 | +- Did this reveal broader architecture or test gaps we should address? |
| 169 | +- Run `bun scripts/update-clawtributors.ts` if the contributor is new. |
0 commit comments