From 39c3850c2e8a4ff40d805136d10986b23f06c49e Mon Sep 17 00:00:00 2001 From: JATMN Date: Sun, 23 Aug 2026 19:20:19 -0700 Subject: [PATCH] docs: tighten PR review expectations in CONTRIBUTING and AGENTS guides (#2151) * docs: tighten PR review expectations in CONTRIBUTING and AGENTS guides - Add drift caveat to CodeRabbit findings: verify suggestions against PR intent before applying; decline out-of-scope with justification or ask a maintainer; never silently ignore findings - Add Keep Your Branch Current subsection (rebase duty, fix-churn warning) - Require full local CI-equivalent suite green before every push, with cross-platform exception - Add ignored/filler PR template submissions to close-without-review list - Expand follow-up guidance: multi-round review is normal, repeated fix requests signal root-cause investigation and better agent prompting - Mirror all of the above in AGENTS.md for coding agents * docs: address CodeRabbit findings on review-expectations guides - Make CONTRIBUTING.md Validation the single authoritative pre-push validation contract mirroring .github/workflows/pr-checks.yml exactly: --frozen-lockfile install, launcher compatibility checks, provider recommendation via npm as CI does, web job carve-out - Remove conflicting 'relevant subset' wording; cross-platform exception is the only carve-out from the full suite - AGENTS.md now defers to the CONTRIBUTING contract instead of defining a divergent core-checks list - Reword ambiguous 'submit the PR template ignored' bullet to 'submit a PR with the PR template ignored' * docs: align pre-push validation suite with CI semantics - Drop standalone 'bun run test:full'; bun run check already includes it - Document web workspace install (bun install --cwd web --frozen-lockfile) before web checks, matching the web CI job - Pass explicit --base/--head to security:pr-scan so local scans target the PR merge-base like CI does instead of script defaults * docs: use PR base commit ref for local security scan parity Replace git merge-base computation with origin/main and document the required invariant (fetch + keep branch rebased onto current origin/main) so the local scan matches CI's PR base.sha instead of diverging. * docs: use exact PR base for security scan * docs: make local validation contract portable * docs: scope local checks and baseline waivers * docs: harden contributor workflow guidance * docs: pin contributor safety contracts --- .github/pull_request_template.md | 8 +- AGENTS.md | 14 +++- CONTRIBUTING.md | 122 +++++++++++++++++++++++-------- README.md | 4 +- 4 files changed, 107 insertions(+), 41 deletions(-) diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index ca3531196..a71436b98 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -10,10 +10,10 @@ ## Testing -- [ ] `bun run build` -- [ ] `bun run smoke` -- [ ] `bun run check` -- [ ] focused tests: +- [ ] I ran the required [local preflight](https://github.com/Gitlawb/openclaude/blob/main/CONTRIBUTING.md#validation). +- exact commands and results: +- focused tests: +- documented skipped checks, platform limitations, or verified pre-existing failures: ## Notes diff --git a/AGENTS.md b/AGENTS.md index 21a0bbdb0..bcdd5aade 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -16,6 +16,8 @@ The installed CLI runs on Node.js `>=22.0.0`. Bun is used for source builds, scr - Add or update tests when behavior changes. - Update docs when setup, commands, provider behavior, or user-facing behavior changes. - For new features, larger refactors, dependencies, or runtime changes, follow the issue-first guidance in [CONTRIBUTING.md](CONTRIBUTING.md). +- Keep PR branches current with `main` using the synchronization and guarded-push workflow in [CONTRIBUTING.md § Keep Your Branch Current](CONTRIBUTING.md#keep-your-branch-current). Rebase whenever resuming work or pushing follow-up fixes, but never overwrite remote PR-head updates with an unguarded force-push. +- Run the authoritative local pre-push validation contract defined in [CONTRIBUTING.md § Validation](CONTRIBUTING.md#validation) before every push to a PR, not just the first one. CI adds clean-runner and supported-Node-matrix coverage that is not practical to reproduce in one local shell. ## Stack And Conventions @@ -46,7 +48,7 @@ Common libraries and patterns: ## Validation -Run the narrowest useful checks for your change, and list the exact commands in the PR. +The authoritative local pre-push validation contract lives in [CONTRIBUTING.md § Validation](CONTRIBUTING.md#validation) and must be run before every push to a PR, including follow-up fixes during review. It covers the same command families as `.github/workflows/pr-checks.yml`; CI remains authoritative for clean-runner, supported-Node-matrix, and platform-specific coverage. The lists below are for narrowing checks while you iterate; they do not replace the pre-push contract. Core checks: @@ -67,7 +69,7 @@ bun run test:provider bun run test:provider-recommendation ``` -Web checks, when touching `web/`: +Web checks, when changes can affect the site: ```bash bun run web:typecheck @@ -80,9 +82,10 @@ Diagnostics and PR hygiene: ```bash bun run doctor:runtime -bun run security:pr-scan ``` +For PR intent scanning, use the canonical upstream fetch and explicit-ref invocation in [CONTRIBUTING.md § Validation](CONTRIBUTING.md#validation); the scanner's default `origin/main` base is not portable to fork checkouts. + ## Provider Changes When modifying provider behavior: @@ -100,5 +103,8 @@ When modifying provider behavior: - Do not introduce dependencies without clear project benefit. - Do not skip tests for behavior changes. - Do not silently change provider tags; maintainers control them during review. -- Do not ignore CodeRabbit or maintainer feedback; address it before requesting more review. +- Do not ignore CodeRabbit or maintainer feedback; address it before requesting more review. Before applying an automated review suggestion, verify it does not pull the PR away from its stated scope or intent — decline out-of-scope suggestions with justification, or ask a maintainer when unsure. Never silently ignore findings. +- Do not push commits with failing, incomplete, or unrun local checks unless an exception in [CONTRIBUTING.md § Validation](CONTRIBUTING.md#validation) applies. Verify pre-existing failures against the current PR base and document the evidence in the PR; PR-owned failures must still be fixed. +- Do not submit a PR whose description still contains template placeholder text; fill in every section of the [PR template](.github/pull_request_template.md) for the actual change. +- Do not surface-patch recurring review findings; repeated fix requests usually indicate a core design issue — investigate and fix the root cause instead of the reported symptom. - Do not add a manually maintained release-notes data source to the static site; link to GitHub Releases instead. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index d64c3aa1b..7f6a99b50 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -10,6 +10,7 @@ OpenClaude is a rapidly evolving open-source coding-agent CLI with support for m - [Proposing New Features](#proposing-new-features) - [Pull Requests](#pull-requests) - [Automated Review (CodeRabbit)](#automated-review-coderabbit) + - [Keep Your Branch Current](#keep-your-branch-current) - [PR Follow-Up Requirements](#pr-follow-up-requirements) - [Duplicate PRs](#duplicate-prs) - [What Gets Closed Without Review](#what-gets-closed-without-review) @@ -52,7 +53,7 @@ Before opening a PR: - Read [`AGENTS.md`](AGENTS.md) for repo-specific coding-agent conventions, validation commands, provider guidance, and architecture rules. - Re-check open and recently closed PRs for duplicates. - Keep the branch focused on one issue or one clearly scoped improvement. -- Run the narrowest meaningful validation command for the touched area. +- Run narrow checks while iterating, then complete the required [Validation](#validation) contract before opening the PR. Every PR needs a reason. Your PR description must include: @@ -74,9 +75,32 @@ We use [CodeRabbit](https://coderabbit.ai) to assist with PR reviews. CodeRabbit **PR authors must address CodeRabbit findings** — do not ignore its comments and wait for a maintainer override. If you're waiting for a maintainer review and CodeRabbit has completed its review with findings, fix those findings first. Maintainer reviews will not proceed until automated review feedback has been addressed. +One important caveat: **verify each suggested change against your PR's stated scope and intent before applying it.** Automated reviewers can suggest fixes that accidentally force changes to surfaces you never intended to touch, or that quietly pull the PR away from its original framing. When a suggestion would cause that kind of drift: + +- you may decline it with a brief justification in the comment thread +- when unsure whether a suggestion is in scope, ask a maintainer before applying or declining it + +Declining an out-of-scope suggestion with a clear reason is acceptable. Silently ignoring findings is not — every finding should end up either fixed, declined with justification, or escalated to a maintainer. + +### Keep Your Branch Current + +The PR author is responsible for keeping their branch current with `main`. Rebase onto `main` whenever you resume work on a PR, and before pushing follow-up fixes. + +Stale branches cause real problems: maintainers end up requesting fixes for issues that are already resolved on `main`, which produces endless fix-churn requests for work nobody needs to do again. A quick rebase before each update keeps review feedback relevant and avoids wasted rounds on both sides. + +Before rebasing an already-pushed PR branch, fetch the current remote PR head, record its exact commit SHA, and incorporate any commits that were added to the remote branch. Then fetch upstream `main` and rebase. Publish with an explicit lease pinned to the PR-head SHA you recorded, never an unguarded `--force` or an unqualified lease: + +```bash +git push --force-with-lease=refs/heads/: HEAD:refs/heads/ +``` + +Pinning the expected SHA prevents a background fetch from silently moving a remote-tracking ref and weakening the lease. If the lease is rejected, stop: fetch and inspect the new remote PR head, incorporate its commits, redo the rebase, and retry with that newly recorded SHA. If the branch is shared or does not allow force-pushes, coordinate with a maintainer and use a repository-approved non-rewriting update path instead of discarding remote work. + +Note that PRs with merge conflicts will not be reviewed or approved regardless (see [Pull Requests](#pull-requests)) — staying current prevents that state entirely. + ### PR Follow-Up Requirements -Submitting a PR is a commitment to see it through. Please be prepared to: +Submitting a PR is a commitment to see it through to the end. Review here is in-depth and can take multiple rounds — do not be surprised if every review pass results in follow-up fix requests; that is the normal shape of this project's review process, not a signal that your PR is unwelcome. Please be prepared to: - **Respond to review feedback within 1 week** of a maintainer or CodeRabbit review request - **If you need more time**, leave a comment explaining your situation and expected timeline @@ -84,6 +108,8 @@ Submitting a PR is a commitment to see it through. Please be prepared to: This policy ensures the project stays maintainable and that contributor queue doesn't grow stale. We understand life happens — a quick note explaining a delay goes a long way. +If you find yourself getting fix requests on every round, treat that as a signal rather than noise: repeated findings on the same PR usually trace back to a core design issue, with each surface-level patch exposing another symptom. Investigate the root cause of what reviewers are flagging instead of patching the reported problem. And if you are driving the work with an AI agent, this is also the moment to re-evaluate how you are prompting it — vague or narrow prompts produce surface fixes; detailed instructions to investigate the requested change and its surrounding design produce fixes that hold up under review. + ### Duplicate PRs We are proactive about closing duplicate PRs. Before submitting, **it is your responsibility to check** whether a similar PR already exists: @@ -104,6 +130,7 @@ PRs may be closed without review if they: - drift from the approved scope of a linked issue - change the project's language, core runtime, or dependency stack without prior maintainer agreement - are drive-by contributions with no context, no tests, and no clear purpose +- submit a PR with the [PR template](.github/pull_request_template.md) ignored — generic filler text or leftover template placeholders (`what changed`, `provider/model path tested:`, etc.) show the description was not written for your PR. These will not be reviewed until corrected and risk being closed without review - are automated bounty-hunting or mass-submitted PRs that provide little meaningful value to the codebase - are advertisements, sales pitches, or promotional submissions for a product or service — open an issue first to discuss with maintainers if you believe your product or service is relevant to this project @@ -217,53 +244,86 @@ bun run dev:profile ## Validation -CI runs the following checks on every PR. Run the relevant ones locally before pushing. +CI runs a fixed set of checks on every PR (see `.github/workflows/pr-checks.yml`). This section is the **authoritative local pre-push validation contract** — `AGENTS.md` defers to it. **Run every locally applicable check before every push to an open PR, including follow-up pushes during review.** Do not wait for GitHub CI to discover failures you could have caught locally — wasted Actions minutes are a real cost on this repo. -Full check (smoke + unit tests): +The security scan requires the commit ancestry needed to find the PR's merge base. Check whether your clone is shallow: ```bash +git rev-parse --is-shallow-repository +``` + +If that prints `true`, fetch the rest of the current branch's history before running the preflight (replace the default remote if your PR branch is tracked elsewhere): + +```bash +git fetch --unshallow +``` + +Required local preflight: + +```bash +bun install --frozen-lockfile bun run check -``` - -Full test pass (single concurrency, matches CI): - -```bash -bun run test:full -``` - -Provider tests: - -```bash -bun run test:provider -``` - -Provider recommendation tests: - -```bash -bun run test:provider-recommendation -``` - -Typecheck (enforced by the dedicated `typecheck` CI job): - -```bash bun run typecheck bun run typecheck:type-tests +node bin/openclaude --version +bun run test:provider +npm run test:provider-recommendation +git fetch https://github.com/Gitlawb/openclaude.git main +bun run security:pr-scan -- --base FETCH_HEAD --head HEAD ``` -PR intent scan: +Also run the compile-cache-disabled launcher check using the syntax for your shell. + +Bash, zsh, and similar shells: ```bash -bun run security:pr-scan +NODE_DISABLE_COMPILE_CACHE=1 node bin/openclaude --version ``` -Web (if touching `web/`): +PowerShell: + +```powershell +& { + $previousValue = [Environment]::GetEnvironmentVariable('NODE_DISABLE_COMPILE_CACHE', 'Process') + try { + $env:NODE_DISABLE_COMPILE_CACHE = '1' + node bin/openclaude --version + $launcherExitCode = $LASTEXITCODE + if ($launcherExitCode -ne 0) { + throw "Launcher compatibility check failed with exit code $launcherExitCode" + } + } finally { + [Environment]::SetEnvironmentVariable('NODE_DISABLE_COMPILE_CACHE', $previousValue, 'Process') + } +} +``` + +If the PR can affect the website — including changes under `web/`, root or web dependency and lock files, shared site assets or content, or build/toolchain configuration used by the site — also run: ```bash +bun install --cwd web --frozen-lockfile bun run web:typecheck bun run web:build ``` -PRs that fail CI checks will not be merged. +Notes on the local preflight: + +- `bun run check` already builds the CLI and includes smoke, deadcode, and the full unit pass (`test:full`) — do not run those separately, or you execute work twice. +- Fetching upstream `main` by URL avoids assuming that a fork checkout's `origin` points at Gitlawb/openclaude. `FETCH_HEAD` is the fetched upstream tip. The scan deliberately uses local `HEAD` so it includes commits that have not been pushed yet; CI uses the pushed PR head SHA after the push. +- The web CI job remains unconditional as an integration backstop. Contributors do not need to run the web suite locally for changes that cannot affect the site. +- This preflight covers the same command families as CI, but it does not reproduce CI exactly in one shell. CI runs the main checks under Node 22 and 24.11.x, separately builds and launches under exact Node 22.0.0, and executes every job on a clean runner. + +If a local or GitHub CI check fails, first determine whether the PR owns the failure. Treat a failure as pre-existing only when you can reproduce it on the PR's current base (or link to the same failure on a current `main` CI run) and the PR neither changes the causal surface nor claims to fix that problem. + +For a verified pre-existing failure: + +- record the failing check, relevant output, and base commit or `main` CI run in the PR +- note the pre-existing failure in the PR summary or testing notes so maintainers can track it separately; link an existing issue when one is already available +- explain briefly why the failure is unrelated to the PR + +The contributor may then proceed and is not responsible for repairing that failure in the current PR. A maintainer may still need to rerun or waive a required GitHub check, or land the separate fix, before branch protection permits a merge. Failures caused by the PR, or in behavior the PR claims to fix, remain the author's responsibility and must be resolved. + +PRs with unresolved PR-owned CI failures will not be merged. Verified pre-existing failures follow the exception above. ### Recommended Local Checks diff --git a/README.md b/README.md index bf5fc976b..e8a224f60 100644 --- a/README.md +++ b/README.md @@ -442,7 +442,7 @@ Day-to-day commands: - `bun test path/to/file.test.ts` — focused runs for the areas you touch - `bun run test:coverage` — coverage to `coverage/lcov.info` plus a visual report at `coverage/index.html` (`bun run test:coverage:ui` rebuilds just the UI) - `bun run smoke` — smoke checks -- `bun run doctor:runtime`, `bun run verify:privacy`, `bun run security:pr-scan -- --base origin/main` +- `bun run doctor:runtime`, `bun run verify:privacy`; for PR intent scanning, use the fresh-upstream, explicit-ref workflow in the [local pre-push validation contract](CONTRIBUTING.md#validation) Focused suites: `bun run test:provider`, `bun run test:provider-recommendation`. @@ -470,7 +470,7 @@ OpenClaude leaves Node's standard compile-cache controls authoritative. Set `NODE_DISABLE_COMPILE_CACHE=1` to disable the optimization, including for V8 coverage runs that require uncached compilation. -Recommended validation before opening a PR: +Before opening or updating a PR, run the authoritative [local pre-push validation contract](CONTRIBUTING.md#validation). The commands below are useful for narrow iteration, but they do not replace that required preflight: - `bun run build` - `bun run smoke`