mirror of
https://github.com/openclaw/openclaw.git
synced 2026-05-12 03:23:42 +00:00
chore: making PR review chores deterministic + less token hungry
This commit is contained in:
@@ -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 <PR>`
|
||||
- `scripts/pr review-checkout-main <PR>` or `scripts/pr review-checkout-pr <PR>` while reviewing
|
||||
- `scripts/pr review-guard <PR>` before writing review outputs
|
||||
- `scripts/pr review-validate-artifacts <PR>` after writing outputs
|
||||
- `scripts/pr-prepare init <PR>`
|
||||
- `scripts/pr-prepare validate-commit <PR>`
|
||||
- `scripts/pr-prepare gates <PR>`
|
||||
- `scripts/pr-prepare push <PR>`
|
||||
- Optional one-shot prepare: `scripts/pr-prepare run <PR>`
|
||||
- `scripts/pr-merge <PR>` (verify-only; short form remains backward compatible)
|
||||
- `scripts/pr-merge verify <PR>` (verify-only)
|
||||
- Optional one-shot merge: `scripts/pr-merge run <PR>`
|
||||
|
||||
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 <PR>` and `scripts/pr review-tests <PR> ...` 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 #<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 "<msg>" <file...>`; avoid manual `git add`/`git commit` so staging stays scoped.
|
||||
- In normal `prepare-pr` runs, commits are created via `scripts/committer "<msg>" <file...>`. 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: <summary> (openclaw#<PR>) thanks @<pr-author>`.
|
||||
- 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-<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:
|
||||
|
||||
|
||||
Reference in New Issue
Block a user