From 30fb9eeaa0ab751c5726cd49f14e2794ea67d960 Mon Sep 17 00:00:00 2001 From: vchamarthi <123653452+vchamarthi@users.noreply.github.com> Date: Fri, 23 Oct 2026 12:53:24 -0500 Subject: [PATCH 1/2] docs: add a review guide and a pre-PR self-review checklist REVIEW.md says what a defect looks like in dokimi, how to raise a finding, and how to keep review rounds short. AGENTS.md gains a self-review section to run before opening a PR and a soft size limit. The PR template asks how to revert the change and whether it was self-reviewed. Signed-off-by: vchamarthi <123653452+vchamarthi@users.noreply.github.com> --- .github/pull_request_template.md | 5 ++++ AGENTS.md | 18 +++++++++++ REVIEW.md | 51 ++++++++++++++++++++++++++++++++ 3 files changed, 74 insertions(+) create mode 100644 REVIEW.md diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 61087837..245c00e8 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -15,10 +15,15 @@ +## Revert + + + ## Checklist - [ ] Tests added/updated and `pytest` passes - [ ] `ruff`, `mypy`, and schema checks pass - [ ] Docs updated (or N/A) — see the docs-drift checklist in CONTRIBUTING.md +- [ ] Self-reviewed per "Before opening a PR" in `AGENTS.md` - [ ] No ad-hoc run artifacts committed - [ ] New files carry the SPDX license header (or are exempt in `.github/.licenserc.yaml`) diff --git a/AGENTS.md b/AGENTS.md index ca6b0e2c..c061b0be 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -126,3 +126,21 @@ Registries, bundled in the package (`dokimi/dokimi/`) so they ship in the wheel: branches, `dependabot/*` and `renovate/*`, are the exception). - When a change depends on an open PR, wait for it to merge or put both in one PR; PRs are not stacked on each other's branches. +- Keep a PR to one change. Past about 600 changed lines, split it or say in + the description why it stays whole: the largest PRs take the most review + rounds and hide the most misses. + +## Before opening a PR + +Review it yourself the way `REVIEW.md` asks a reviewer to, so the first round +finds little: + +- For each new test, break the code it covers (flip a condition, drop a + branch) and confirm the test fails. +- For a bug fix, find every other site with the same bug class and fix or + list them. +- Run the changed command on a bad input: missing file, empty file, wrong + shape, `None` where a value is expected, a Windows path. +- Check that the PR title, description, docstrings and changelog fragment say + what the code does (exit codes especially). +- Re-review your own fix commits during review with the same care. diff --git a/REVIEW.md b/REVIEW.md new file mode 100644 index 00000000..a77c821a --- /dev/null +++ b/REVIEW.md @@ -0,0 +1,51 @@ + + +# Review guide + +For anyone reviewing a dokimi PR, person or agent. CI already enforces lint, +types, coverage, growth, import layers and test quality (`AGENTS.md`); do not +repeat what it checks. + +## What a defect looks like here + +- A command gives a wrong number, verdict or exit code, or crashes where it + should print a message. +- A test passes when the behaviour it names is broken. +- A doc, docstring, PR title or changelog fragment says something the code + does not do. +- A stored record is written or read with keys or a shape other than the + schema's. +- Behaviour differs on Windows or with a `None`, empty or missing input. + +## How to raise a finding + +- Name the file and line, and show the input that breaks it: a command, a + failing test or a mutation that the tests still pass. A finding found by + running code lands far more often than one found by reading. +- Name the bug class and every other site that has it, so one round fixes all + of them. +- Where a fix and a test are short, push them or put them in a suggestion + instead of describing them. +- Mark each finding must-fix or nit. At most three nits per review; skip + wording in `dokimi/changelog.d/`. +- Leave code the PR did not change alone, unless the change makes it wrong. +- Do not repeat a finding already raised on the PR. +- Start with the count of findings, and say "nothing to fix" when there is + nothing. + +## Rounds + +- After the first round, review only what changed since. +- Re-run the original failing input on every fix, including fixes the + reviewer pushed. +- If a PR is still collecting must-fix findings after three rounds, step back + and agree on the approach in the thread before more commits. +- Close each thread as fixed or with a one-line reason. + +## When the same comment keeps coming back + +Move it upstream: a test, a check in `make ci-local`, or a line in `AGENTS.md`, +so the next PR cannot reach review with it. From a54f76cec8f5f13211a5a8c761c03f12ec4ce3c8 Mon Sep 17 00:00:00 2001 From: vchamarthi <123653452+vchamarthi@users.noreply.github.com> Date: Fri, 23 Oct 2026 13:07:13 -0500 Subject: [PATCH 2/2] docs: tighten review guide wording and point Copilot at it Changelog fragments are reviewed for accuracy, findings on unchanged code are noted as pre-existing, a finding is raised again when its fix did not hold, re-review covers callers of the changed code, and threads close with a commit or a reason. The self-review checklist reverts mutation checks, keeps sibling fixes within the PR's scope, and asks to say what could not be run. The soft PR size limit is removed. .github/copilot-instructions.md points Copilot review at REVIEW.md. Signed-off-by: vchamarthi <123653452+vchamarthi@users.noreply.github.com> --- .github/copilot-instructions.md | 7 +++++++ AGENTS.md | 16 +++++++--------- REVIEW.md | 16 ++++++++++------ 3 files changed, 24 insertions(+), 15 deletions(-) create mode 100644 .github/copilot-instructions.md diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md new file mode 100644 index 00000000..0f5cd1c9 --- /dev/null +++ b/.github/copilot-instructions.md @@ -0,0 +1,7 @@ + + +When reviewing a pull request, follow `REVIEW.md` at the repository root. +When writing code, follow `AGENTS.md`. diff --git a/AGENTS.md b/AGENTS.md index c061b0be..0e154b0f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -126,21 +126,19 @@ Registries, bundled in the package (`dokimi/dokimi/`) so they ship in the wheel: branches, `dependabot/*` and `renovate/*`, are the exception). - When a change depends on an open PR, wait for it to merge or put both in one PR; PRs are not stacked on each other's branches. -- Keep a PR to one change. Past about 600 changed lines, split it or say in - the description why it stays whole: the largest PRs take the most review - rounds and hide the most misses. ## Before opening a PR Review it yourself the way `REVIEW.md` asks a reviewer to, so the first round finds little: -- For each new test, break the code it covers (flip a condition, drop a - branch) and confirm the test fails. -- For a bug fix, find every other site with the same bug class and fix or - list them. -- Run the changed command on a bad input: missing file, empty file, wrong - shape, `None` where a value is expected, a Windows path. +- For each test of changed behaviour, break the code it covers (flip a + condition, drop a branch), confirm the test fails, then revert the break. +- For a bug fix, find every other site with the same bug class. Fix the ones + within this PR's scope; list the rest in the description or an issue. +- Where it applies, run the changed command on a bad input: missing file, + empty file, wrong shape, `None` where a value is expected, a Windows path. + Say in the description what you could not run. - Check that the PR title, description, docstrings and changelog fragment say what the code does (exit codes especially). - Re-review your own fix commits during review with the same care. diff --git a/REVIEW.md b/REVIEW.md index a77c821a..387d5be5 100644 --- a/REVIEW.md +++ b/REVIEW.md @@ -29,21 +29,25 @@ repeat what it checks. of them. - Where a fix and a test are short, push them or put them in a suggestion instead of describing them. -- Mark each finding must-fix or nit. At most three nits per review; skip - wording in `dokimi/changelog.d/`. -- Leave code the PR did not change alone, unless the change makes it wrong. -- Do not repeat a finding already raised on the PR. +- Mark each finding must-fix or nit. At most three nits per review; skip the + style of `dokimi/changelog.d/` fragments, not their accuracy. +- A bug in code the PR did not change: mention it once as pre-existing. It + does not block the PR unless the change makes it worse. +- Do not repeat a finding already raised on the PR, unless its fix did not + hold. - Start with the count of findings, and say "nothing to fix" when there is nothing. ## Rounds -- After the first round, review only what changed since. +- After the first round, review what changed since and the code that calls + it. - Re-run the original failing input on every fix, including fixes the reviewer pushed. - If a PR is still collecting must-fix findings after three rounds, step back and agree on the approach in the thread before more commits. -- Close each thread as fixed or with a one-line reason. +- Close each thread with the commit that fixed it, or one line on why it is + not a defect. The reviewer closes their own thread where they can. ## When the same comment keeps coming back