diff --git a/.agents/skills/pre-pr-review/SKILL.md b/.agents/skills/pre-pr-review/SKILL.md index e3babacc..e49854c7 100644 --- a/.agents/skills/pre-pr-review/SKILL.md +++ b/.agents/skills/pre-pr-review/SKILL.md @@ -1,83 +1,102 @@ --- name: pre-pr-review -description: Review a RedisVL branch before its PR is marked ready, fix what the review finds, and write a Review record into the PR description for the human approver. Use before opening or marking a PR ready, after a substantive push, or when responding to Bugbot or reviewer comments on a PR. -compatibility: Works with any coding agent. Needs git, make and pytest. Optional - gh, to read and update the PR; the public Redis Docs MCP at https://redis.io/mcp, for the Redis behaviour lens. +description: Review a RedisVL branch before a human sees it, fix what holds up, and write a Reviewer Brief into the PR description so the approver knows where to look. Use it whenever you're about to open a PR, mark one ready for review, or push substantive changes to one, and when you're handling Bugbot or reviewer comments, even if nobody asked for a review by name. +compatibility: Works with any coding agent. Needs git, make, pytest and Python 3. Optional - gh, to read and update the PR; the public Redis Docs MCP at https://redis.io/mcp, for the Redis behaviour lens. --- # Pre-PR review -This review runs before a human sees the PR. Its output is a short Review record in the PR description, telling the approver what was reviewed and what wasn't, so they can check the evidence instead of re-reviewing the change. +A human approves every merge to `main`. This review runs before they see the PR, so the problems it catches never reach them. Its output, the Reviewer Brief, gives them a head start: where to begin reading, where the change could still fail, and what nobody has checked. -The review is only useful if the approver can trust it. Report only what you can tie to the code, fix only what survives challenge, and never claim a check you didn't run. +The approver can only rely on the brief if it's accurate, so report only what you can tie to the code, and never claim a check you didn't run. ## 1. Scope the change -- Diff the branch against its merge base with `main`: `git diff $(git merge-base origin/main HEAD)...HEAD`. -- Read each changed file in full, not just the hunks. -- For every changed public function, method or class, read its callers and its sync or async twin (`SearchIndex` and `AsyncSearchIndex`, and so on). -- Note the size (lines added outside `tests/` and `docs/`), the type (fix, feat, refactor, docs, test, chore or CI) and the paths touched. Step 2 uses all three. +- Diff the work against its merge base with `main`: `git diff $(git merge-base origin/main HEAD)` covers commits and uncommitted changes, and `git status` shows untracked files to read as well. +- Read each changed file in full, not just the hunks. Bugs often sit in the unchanged lines a hunk depends on. +- For every changed public function, method or class, read its callers and its sync or async twin (`SearchIndex` and `AsyncSearchIndex`, and so on). Drift between twins is one of this project's most common defects. +- In a stack of PRs you may review the whole stack at once, but keep track of which PR introduced each line. Each PR gets its own brief. ## 2. Choose lenses -Pick the lenses this diff needs, and write down the reason for each one you run and each one you skip. Most PRs need two or three. +Run all six lenses by default. Each catches problems the others miss, and running all of them is the most thorough option. -| Lens | File | Run it when | +A lens that finds nothing relevant in the diff has still done its job; say nothing about it beyond the footer. If you need to save tokens, the "Most useful when" column says where each lens pays off. Correctness always runs. The footer of the brief lists the lenses that ran, so the reviewer can see what was covered. + +| Lens | File | Most useful when | |---|---|---| -| Correctness | `references/correctness.md` | Always. A single quick pass for diffs under 100 lines and for chore, CI or refactor changes | +| Correctness | `references/correctness.md` | Always | | Security | `references/security.md` | The diff touches `redisvl/mcp/`, `redisvl/query/filter.py`, `redisvl/utils/token_escaper.py`, code that builds query strings or `SCAN` patterns, destructive methods (`delete`, `clear`, drop), `redisvl/redis/connection.py`, `.github/`, or dependencies | -| Design | `references/design.md` | Feature PRs over about 300 lines; new or changed public API or MCP surface; work on deprecated APIs; new vectorizer, reranker or provider modules | +| Design | `references/design.md` | Feature work, new or changed public API or MCP surface, work on deprecated APIs, new vectorizer, reranker or provider modules | | Tests | `references/tests.md` | Tests are added or changed, or the change sits in an area CI can't exercise (cluster, live provider APIs) | | Docs | `references/docs.md` | `docs/`, notebooks, docstrings or comments change, or public behaviour changes | -| Redis behaviour | `references/redis.md` | The diff changes how Redis commands or queries are built, relies on a server default, limit or reply shape, or states how Redis behaves. Checks claims against the official docs, ideally through the Redis Docs MCP | - -If you're unsure whether a lens applies, run it. A lens you skip is a gap the approver should know about, so the record lists it. +| Redis behaviour | `references/redis.md` | The diff changes how Redis commands or queries are built, relies on a server default, limit or reply shape, or states how Redis behaves | ## 3. Review, one lens at a time -Run one pass per chosen lens. Start each pass from the diff and that lens's file, not from an earlier pass's conclusions. +Run one pass per lens. Start each pass from the diff and that lens's file, not from an earlier pass's conclusions, so that one pass's blind spots don't become the next one's. - If your agent can start isolated sub-tasks (subagents, sub-sessions, parallel workers), give each lens its own, and run them in parallel. - If it can't, run the lenses one after another, and write each pass's findings to a scratch file before starting the next. -Every finding takes the same shape: +Write each finding in this shape, so the challenger can check it quickly: ``` [lens] path/to/file.py:LINE: claim, in plain words. Failure: the input or state that goes wrong, and what happens. Fix: the smallest change that resolves it. ``` -Drop formatting, import order and typing nits. CI and `make lint` already handle them. +Skip formatting, import order and typing nits, which CI and `make lint` already catch. ## 4. Challenge the findings -Run a separate challenger pass, as described in `references/challenger.md`. It starts from the code and the list of findings, not from the reasoning that produced them, and scores each finding from 1 to 10. Keep findings that score 7 or more and have a file, a line and a concrete failure or cost. Record the rest as rejected, each with its one-line reason. +Run a separate challenger pass, as described in `references/challenger.md`. Review passes produce false positives, and every one that survives costs someone time. Keep findings that score 7 or more out of 10 and have a file, a line and a concrete failure or cost. Run a second challenge round only if the first round's fixes changed the code materially. ## 5. Fix and re-check -- Fix only the kept findings, and only within the PR's scope. A real problem outside scope goes into the record as an open item, not into the diff. -- Fix the root cause. Add the narrowest regression test that fails without the fix, or say why none applies. -- Re-run whatever the fixes touched: `make format`, `make check-types`, and the affected tests (`make test ARGS="tests/unit/test_x.py"` or `pytest` on the specific file). +- Fix the kept findings that are within the PR's scope, at the root cause. A real problem outside scope goes to the author as a question, not into the diff. +- Add the narrowest regression test that fails without the fix, or note why none applies. A test you've seen fail without the fix is strong evidence for the brief's "Beyond CI" line. +- Re-run whatever the fixes touched: `make format`, `make check-types`, and the affected tests (`make test ARGS="tests/unit/test_x.py"`, or `pytest` on the specific file). +- Commit the fixes locally. The author pushes. + +## 6. Settle open questions with the author + +A PR should reach the reviewer with no open questions. Before the PR is opened or marked ready, and before each later rewrite of the brief, take everything the review couldn't settle to the author and get an answer: findings outside the PR's scope, rejections you're unsure about, and trade-offs. You work for the author, and the reviewer shouldn't have to answer questions the author never saw. + +Once the author has decided, record a deliberate trade-off that a reviewer might question in the PR's Notes section, as a decision with its reason. Notes say what was decided and why; they don't ask the reviewer anything. -## 6. Write the Review record +If you can't reach the author, for example because you're running unattended, don't open the PR or mark it ready. Leave it as a draft, or unopened, and put the questions in your report to the author, never in the PR. -Fill in the template in `references/review-record.md`. It replaces everything under the `## Review record` heading of the PR description, including the template's placeholder comment or any earlier record, and leaves the rest of the description untouched. +## 7. Write the Reviewer Brief + +Write the brief following `references/reviewer-brief.md`. The reviewer sees the change as it is now, so the brief points at the current code, not at the problems you fixed. + +To put it in the PR description, save the brief to a file and run: + +``` +python3 .agents/skills/pre-pr-review/scripts/update_brief.py +``` -With `gh`: fetch the current description (`gh pr view --json body -q .body`), replace only that section, write the whole description to a file and run `gh pr edit --body-file `. `--body-file` replaces the entire description, so never pass it the record alone. Without `gh`, print the record for the author to paste. +The script replaces only the `## Reviewer Brief` section and keeps the rest of the description, including Bugbot's summary. Add `--dry-run` to print the result without changing the PR. If the PR doesn't exist yet, put the brief under `## Reviewer Brief` in the description you open it with. Without `gh`, give the brief to the author to paste. ## Responding to review -When Bugbot or a person comments on the PR, handle each point like this: +When Bugbot or a person comments on the PR: -1. **Gather everything open.** That means review threads that are neither resolved nor outdated, review summaries and top-level PR comments. With `gh`, `gh pr view --comments` shows reviews and top-level comments, and `gh api --paginate repos/{owner}/{repo}/pulls//comments` lists inline comments. Resolution state is only exposed through the GraphQL `reviewThreads` field, so check it there or on GitHub. -2. **Verify each point against the current code.** The reviewer's wording is a claim to check, not a verdict. -3. **Give it exactly one disposition:** +1. **List what's still open:** + ``` + python3 .agents/skills/pre-pr-review/scripts/open_feedback.py + ``` + It prints unresolved, non-outdated review threads, review summaries and top-level comments, each with its author and location. +2. **Verify each point against the current code.** A reviewer's wording, human or bot, is a claim to check, not a verdict. +3. **Decide its outcome:** - `fixed in ` - `already fixed` - `invalid`, with the evidence - `duplicate of ` - - `needs a decision`, for the author or a maintainer to settle -4. **Before you reply "fixed", check the fix.** Re-read the exact file and line in the fixed tree, and search for other instances of the same pattern. -5. **Reply and record it.** Answer each thread with its disposition, then add one line per point to the Review record's "Review comments" list. + - `needs a decision`: ask the author, and reply once they've decided +4. **Check the fix before saying so.** Re-read the exact location in the fixed tree, and search for other instances of the same pattern, before you reply "fixed". +5. **Reply on each thread with its outcome, and bring the brief up to date.** Fixes that change behaviour need a fresh review of the new commits and a rewritten brief, because the reviewer will rely on it. Trivial fixes need only a "Since the last brief" line. Anything that needs a decision goes to the author. -Commit fixes locally; the author pushes. Ask the author before you post replies on the PR, unless they've already told you to. +Ask the author before posting replies on the PR, unless they've already said you can. diff --git a/.agents/skills/pre-pr-review/evals/evals.json b/.agents/skills/pre-pr-review/evals/evals.json new file mode 100644 index 00000000..87cbd696 --- /dev/null +++ b/.agents/skills/pre-pr-review/evals/evals.json @@ -0,0 +1,42 @@ +{ + "skill_name": "pre-pr-review", + "notes": "Fill in any with a current PR before running, since PR state changes. Run each prompt with and without the skill, from a checkout of the PR's head commit. Compare the Reviewer Briefs against the expectations. Never let an eval run push, edit a PR description or post comments: use update_brief.py --dry-run.", + "evals": [ + { + "id": 1, + "prompt": "I'm about to mark PR 777 (the OpenTelemetry spans one) ready for review. Run the pre-PR review on it and give me the Reviewer Brief, but don't touch the PR.", + "expected_output": "A brief that a reviewer who never saw the review can act on.", + "expectations": [ + "\"Start here\" names one file or function from the diff and says why it matters.", + "No section lists bugs that were fixed during the review, finding counts, open questions or lens names outside the footer.", + "\"Where it could fail\" has at most three items, each with a file and line, or a clearly named behaviour, and a concrete way it could go wrong.", + "\"Not verified\" names live provider APIs or cluster mode.", + "\"Beyond CI\" contains only checks CI does not run, or is omitted.", + "The footer names the reviewed commit, the agent and model, and the lenses that ran.", + "The visible brief is under about 200 words.", + "Any question the review could not settle is put to the user before the brief is written, and none appears in the brief or elsewhere in the PR." + ] + }, + { + "id": 2, + "prompt": "Quick one: I fixed a typo in a docstring in redisvl/query/filter.py and a broken link in docs/user_guide/how_to_guides/observability.md. Do the pre-PR review thing and write the brief.", + "expected_output": "A very short brief that scales to the change.", + "expectations": [ + "The brief is under about 60 words.", + "Nothing is invented to fill \"Where it could fail\".", + "The review does not raise findings about code the change did not touch." + ] + }, + { + "id": 3, + "prompt": "Bugbot left comments on PR . Go through them using the repo's review process and tell me what you'd reply to each. Don't post anything.", + "expected_output": "Each open point verified against the code and given exactly one outcome.", + "expectations": [ + "The agent lists the open feedback with scripts/open_feedback.py or an equivalent that respects resolved and outdated threads.", + "Each point gets exactly one of: fixed in , already fixed, invalid with evidence, duplicate, needs a decision.", + "Points that are already resolved on the PR are not re-litigated.", + "Nothing is posted to the PR." + ] + } + ] +} diff --git a/.agents/skills/pre-pr-review/references/challenger.md b/.agents/skills/pre-pr-review/references/challenger.md index e39e218d..f890b36b 100644 --- a/.agents/skills/pre-pr-review/references/challenger.md +++ b/.agents/skills/pre-pr-review/references/challenger.md @@ -18,6 +18,6 @@ For each finding: | 4–6 | Plausible but unproven: it depends on inputs or states you couldn't confirm can occur | | 1–3 | Wrong, already handled, out of scope, or a style preference | -Keep findings that score 7 or more and have a file, a line and a concrete failure or cost. Reject the rest. Each rejection gets a one-line reason, because the reasons go into the Review record and show the approver that the review discriminated. +Keep findings that score 7 or more and have a file, a line and a concrete failure or cost. Reject the rest, each with a one-line reason in your scratch notes. If you reject a finding a reasonable person might side with, raise it with the author rather than settling it silently. The challenger may also add a finding it noticed while checking. A finding it adds gets the same scoring, done in a fresh pass. diff --git a/.agents/skills/pre-pr-review/references/correctness.md b/.agents/skills/pre-pr-review/references/correctness.md index f3f4c90d..3a77f1ca 100644 --- a/.agents/skills/pre-pr-review/references/correctness.md +++ b/.agents/skills/pre-pr-review/references/correctness.md @@ -16,7 +16,7 @@ Find changes that make the code behave wrongly for some input, state or environm - **Dependency floors.** Check every call into an optional dependency against the oldest version `pyproject.toml` allows. A new attribute or keyword that only exists in a later release crashes users on the floor version. This is the most common high-severity finding in this repository. - **Redis replies.** The code has to handle RESP2 and RESP3 reply shapes, and both hash and JSON storage. - **Result limits.** Default result counts (`num_results`, `LIMIT`, `MAXSEARCHRESULTS`) silently truncate. Check any read that is meant to return everything. -- **Cluster mode.** Multi-key operations, `SCAN` and pipelines behave differently on a cluster, and partial failures are possible. CI never runs cluster tests, so reason about these cases from the code. If you can't settle one that way, put it under "Not checked" in the record. +- **Cluster mode.** Multi-key operations, `SCAN` and pipelines behave differently on a cluster, and partial failures are possible. CI never runs cluster tests, so reason about these cases from the code. If you can't settle one that way, put it under "Not verified" in the Reviewer Brief. - **Client ownership.** Who creates, owns and closes a Redis client, and what happens on reconnect, especially for async clients reused across event loops. ## Depth diff --git a/.agents/skills/pre-pr-review/references/redis.md b/.agents/skills/pre-pr-review/references/redis.md index 6e49ce9d..afc1dba2 100644 --- a/.agents/skills/pre-pr-review/references/redis.md +++ b/.agents/skills/pre-pr-review/references/redis.md @@ -12,7 +12,7 @@ Redis hosts a public, read-only MCP server for its documentation at `https://red Send it questions about Redis, not code from the diff, credentials or anything unreleased. The corpus is the redis.io documentation, which includes RedisVL's own docs. Check each page's version metadata against the Redis versions RedisVL supports before you rely on it. -If your agent can't use MCP, read the same pages on `https://redis.io/docs/latest/`. If you can reach neither, skip the lens and put it under "Not checked" in the record. +If your agent can't use MCP, read the same pages on `https://redis.io/docs/latest/`. If you can reach neither, skip the lens and say so under "Not verified" in the Reviewer Brief. ## Run it when @@ -31,4 +31,4 @@ If your agent can't use MCP, read the same pages on `https://redis.io/docs/lates ## How to report -Each finding cites the page it rests on: `... Per: `. A claim you couldn't confirm in the docs is not a finding. List it under "Not checked" instead, so the reviewer knows it was raised but not settled. Record each page you relied on in the record's "Docs consulted" line. +Each finding cites the page it rests on: `... Per: `. A claim you couldn't confirm in the docs is not a finding. If it matters, list it under "Not verified" in the Reviewer Brief, so the reviewer knows it was raised but not settled. When a point in the brief rests on a docs page, link the page in that line. diff --git a/.agents/skills/pre-pr-review/references/review-record.md b/.agents/skills/pre-pr-review/references/review-record.md deleted file mode 100644 index 5ea6de91..00000000 --- a/.agents/skills/pre-pr-review/references/review-record.md +++ /dev/null @@ -1,49 +0,0 @@ -# Review record - -The record is the content under the `## Review record` heading of the PR description, which the PR template provides. It replaces whatever is under that heading, and the heading itself stays as it is. It's written for a human approver who has a minute to decide whether to trust it, so keep it short and concrete. - -## Template - -```markdown -Reviewed at `` by () with `pre-pr-review`. -Lenses: (); (). Skipped: (). - -- findings raised, kept after challenge. -- Fixed: (`path.py:LINE`); (`path.py:LINE`). -- Rejected: (). -- Open for the reviewer: , or "none". - -Review comments: -- : — . - -Verified: (); (). -Docs consulted: ; . -Not checked: . -``` - -Leave out the "Review comments" list until there are comments to respond to, and the "Docs consulted" line when the Redis behaviour lens didn't run. - -## Rules - -- **Translate, never paste.** One line per finding, in plain words, with a file and line. Leave raw model output out of the record. -- **Rejections stay in.** The rejected findings and their reasons show that the review discriminated. Don't drop them to make the record look tidier. -- **"Verified" lists only commands you ran and whose output you saw,** with the result, for example "31 passed" or "no issues". -- **"Not checked" is mandatory.** If the change touches cluster behaviour, provider APIs, Redis versions below the CI matrix, or anything you reasoned about without running, say so. "Nothing" is a valid answer only for a change that CI fully exercises. -- **The sha names what was reviewed.** Re-run the review after any substantive push. For a push that changes nothing a reviewer would care about, such as a rebase or a typo fix, keep the reviewed sha and add a line: "Later commits: `` ()." Never move the sha to code the review didn't see. - -## Example - -An illustrative record for a small fix. The paths and counts are invented. - -```markdown -Reviewed at `7188c45` by Codex CLI (``) with `pre-pr-review`. -Lenses: correctness (always); tests (adds integration tests); docs (changes the `messages` docstrings). Skipped: security (no query-building, MCP or destructive paths); design (40-line fix, no API change). - -- 9 findings raised, 4 kept after challenge. -- Fixed: semantic history used `FilterQuery`'s default of 10 results (`semantic.py:212`); the new test wrote fewer than 10 messages, so it passed without the fix (`test_message_history.py:88`). -- Rejected: "unbounded read may be slow" (the property is documented as unbounded; `get_recent` is the bounded path). -- Open for the reviewer: none. - -Verified: `make check-types` (no issues); `pytest tests/integration/test_message_history.py` (31 passed, Redis 8.4). -Not checked: cluster mode. -``` diff --git a/.agents/skills/pre-pr-review/references/reviewer-brief.md b/.agents/skills/pre-pr-review/references/reviewer-brief.md new file mode 100644 index 00000000..cd065a2e --- /dev/null +++ b/.agents/skills/pre-pr-review/references/reviewer-brief.md @@ -0,0 +1,71 @@ +# Reviewer Brief + +The Reviewer Brief is the section under `## Reviewer Brief` in the PR description. It's written for one person: the human who will approve the PR. They see the change as it is now, not the history of how it got there. They haven't seen your review, and they want to know where to spend their attention. + +The brief isn't a log of your review. Leave out what you fixed, how many findings you raised, and suggestions you rejected. The reviewer never saw those problems, so reading about them costs time and changes nothing they do. The commits and the PR's review threads already hold that history for anyone who wants it. + +The brief also has no questions in it, because the PR shouldn't have any open ones. Anything the review couldn't settle, such as a finding outside the PR's scope, a rejected suggestion you're unsure about, or a trade-off, goes to the author before the PR is opened or marked ready. Once they've decided, a deliberate trade-off that a reviewer might question goes in the PR's Notes section, written as a decision with its reason. + +## Template + +```markdown +**Start here:** ``. + +**Where it could fail** +- ``: . + +**Not verified:** . + +**Beyond CI:** . + +**Since the last brief:** . + +Reviewed ``, fixes in `` · () · lenses: +``` + +"Start here", "Not verified" and the footer always appear. Leave out "Where it could fail" when nothing in the diff is a credible risk, "Beyond CI" when there's nothing beyond CI to report, and "Since the last brief" until the brief has been updated after a push. An empty heading tells the reviewer nothing. + +## What goes in each section + +- **Start here.** Name the one file or function the rest of the change hangs off, so the reviewer reads it first. Reviewers give more attention to whatever they read first, so this is the most valuable line in the brief. +- **Where it could fail.** At most three spots in the final diff where a defect would be most likely or most costly. Use where your review found problems as a hint, but describe the risk in the code as it stands. For example, "the cancellation path records a duration; check it also records the error type", not "we fixed a bug here". Mark anything that predates the PR as pre-existing. +- **Not verified.** Anything you reasoned about without running: cluster behaviour, live provider APIs, Redis versions outside the CI matrix, other platforms. This line tells the reviewer where the remaining risk sits. +- **Beyond CI.** Only evidence CI doesn't already show. Examples: a regression test confirmed to fail without the fix, a run on a Python version CI doesn't cover, or a measurement. Passing tests that CI also runs don't belong here. +- **Since the last brief.** One line per later push, so a returning reviewer knows what's new. +- **Footer.** The commit you reviewed, the commits that hold the review's own fixes (leave that part out if there are none), which agent and model ran the review, and the lenses that ran. If you reviewed uncommitted work, write "Reviewed the working tree on ``" and replace it with the commit once the work is committed. + +## Writing for the reviewer + +The reviewer should finish the brief with a working picture of the change: what to look at, why, and what could go wrong. Write so that someone who has never seen your review can act on every line. + +- **Consequence first, then mechanism.** "A cancelled call counts as a success, because no error type is recorded" tells the reviewer why it matters before telling them how. "Missing `error.type` on `CancelledError`" makes them work that out. +- **Use the reviewer's vocabulary.** Name the functions, files and behaviours they can see in the diff. Leave out lens names, finding numbers, scores and other words from your own process. +- **Define jargon or replace it.** If a term isn't in the codebase or in common use, describe the thing instead. +- **One sentence per item,** with numbers and units where they carry the point. +- **Cut what the diff or CI already says.** Before keeping a line, ask whether a reviewer who hasn't seen your review would do anything differently because of it. If not, delete it. +- **Scale to the change.** Aim for about 150 words on a large PR, and far fewer on a small one. A three-line fix might need only "Start here", "Not verified" and the footer. + +## Keeping it honest + +- **List only checks you saw run.** If "Beyond CI" claims a run you didn't watch, the reviewer will trust the brief for the wrong reasons. +- **The footer says what the brief covers.** It names the commit you reviewed and the commits that only apply that review's fixes. The reviewer compares those with the PR's head to judge whether the brief still applies, so don't list anything else there. +- **Re-review substantive changes.** After a push that changes behaviour, including fixes for review comments, re-run the review on the new commits and rewrite the brief. A "Since the last brief" line is enough only for pushes a reviewer wouldn't care about, such as a rebase, a typo fix or a comment tweak. +- **One brief per PR, even in a stack.** You may review the whole stack at once, but each PR's brief covers only that PR's diff, so it stands on its own. + +## Example + +The brief for an OpenTelemetry PR that adds spans and metrics to SemanticCache and the vectorizers: + +```markdown +**Start here:** `redisvl/utils/vectorize/_telemetry.py`, `embeddings()`. Every vectorizer's embed call in `base.py` now runs inside it, so it decides what each provider call records. + +**Where it could fail** +- `redisvl/utils/vectorize/_telemetry.py:49`: a cancelled or interrupted provider call must record an error type, or dashboards count it as a success. +- Prompt-text capture: only `SPAN_ONLY` and `SPAN_AND_EVENT` may put prompt text into spans. Check that no other value of the capture setting turns it on. + +**Not verified:** live provider APIs (the Google backend mapping is tested with a stand-in); cluster mode. + +**Beyond CI:** the new regression tests fail against the code before the fixes; an instrumented lookup with no SDK installed costs about 11 µs on an Apple M4 Pro. + +Reviewed `feb3fc9`, fixes in `5a55eec` · Claude Code (claude-opus-5-5) · lenses: correctness, security, design, tests, docs, Redis behaviour +``` diff --git a/.agents/skills/pre-pr-review/references/tests.md b/.agents/skills/pre-pr-review/references/tests.md index 077c7fe5..927be5c1 100644 --- a/.agents/skills/pre-pr-review/references/tests.md +++ b/.agents/skills/pre-pr-review/references/tests.md @@ -6,7 +6,7 @@ Check that the tests prove the change works, cover the paths that matter, and ad - **Vacuous tests.** A regression test that passes without the fix. Revert the fix mentally, or actually, and check that the test fails for the right reason, not because of an unrelated error earlier in setup. A test that builds an object which fails before reaching the code under test proves nothing. - **Missing critical paths.** Error paths, boundary values (empty, one, exactly the limit, one past it), both the sync and async variants, and both hash and JSON storage, where the change touches them. -- **Areas CI can't reach.** `requires_cluster` tests are skipped in CI, and `requires_api_keys` tests only run with keys. If the change is in one of those areas, ask for a hermetic unit test, using mocks or a fake client, that pins the behaviour. Otherwise, make sure the Review record lists it as not checked. +- **Areas CI can't reach.** `requires_cluster` tests are skipped in CI, and `requires_api_keys` tests only run with keys. If the change is in one of those areas, ask for a hermetic unit test, using mocks or a fake client, that pins the behaviour. Otherwise, make sure the Reviewer Brief lists it under "Not verified". - **Weakened assertions.** Loosened expectations, added skips or broader `pytest.raises` that make a failing test pass rather than fixing the code. - **Bloat.** Parametrisation that multiplies cases without covering new behaviour; several tests asserting the same thing; tests of trivial getters or of library code. Also tests that pin incidental details such as exact log text or wire format, which make refactoring expensive. - **Placement.** Pure logic belongs in `tests/unit/`, and anything needing Redis in `tests/integration/`. Integration tests share a container, so check that new ones clean up their keys and indexes. diff --git a/.agents/skills/pre-pr-review/scripts/open_feedback.py b/.agents/skills/pre-pr-review/scripts/open_feedback.py new file mode 100644 index 00000000..6358e630 --- /dev/null +++ b/.agents/skills/pre-pr-review/scripts/open_feedback.py @@ -0,0 +1,138 @@ +#!/usr/bin/env python3 +"""List the review feedback on a PR that still needs a response. + +Usage: open_feedback.py [--include-resolved] + +Prints, as Markdown: +- inline review threads that aren't resolved, with path, line and every + comment in the thread. Threads GitHub marks outdated are still listed, + tagged [outdated], because a later push can move a line without fixing it; +- review summaries that have a body; +- top-level PR comments, skipping the release bot. + +Resolution state is only available through GitHub's GraphQL API, which is why +this script exists. Requires the GitHub CLI (gh), authenticated for the +repository. +""" + +import argparse +import json +import subprocess +import sys + +QUERY = """ +query($owner: String!, $name: String!, $number: Int!, $cursor: String) { + repository(owner: $owner, name: $name) { + pullRequest(number: $number) { + reviewThreads(first: 50, after: $cursor) { + pageInfo { hasNextPage endCursor } + nodes { + isResolved + isOutdated + path + line + originalLine + comments(first: 50) { nodes { author { login } body url } } + } + } + reviews(first: 100) { nodes { author { login } state body url } } + comments(first: 100) { nodes { author { login } body url } } + } + } +} +""" + +SKIP_AUTHORS = {"applied-ai-release-bot"} + + +def gh_json(*args: str) -> dict: + result = subprocess.run(["gh", *args], capture_output=True, text=True) + if result.returncode != 0: + sys.exit(f"gh {' '.join(args[:2])} failed:\n{result.stderr.strip()}") + return json.loads(result.stdout) + + +def first_line(text: str, limit: int = 160) -> str: + line = next( + ( + part.strip() + for part in text.splitlines() + if part.strip() and not part.lstrip().startswith("", re.MULTILINE) +BUGBOT_BLOCK = re.compile(r"^(?:---\s*\n+)?", re.MULTILINE) + + +def gh(*args: str) -> str: + result = subprocess.run(["gh", *args], capture_output=True, text=True) + if result.returncode != 0: + sys.exit(f"gh {' '.join(args)} failed:\n{result.stderr.strip()}") + return result.stdout + + +def replace_brief(body: str, brief: str) -> str: + body = body.replace("\r\n", "\n") + section = f"{HEADING}\n\n{brief.strip()}\n" + match = SECTION.search(body) + if match: + end = NEXT_SECTION.search(body, match.end()) + tail = body[end.start() :] if end else "" + return body[: match.start()] + section + ("\n" + tail if tail else "") + bugbot = BUGBOT_BLOCK.search(body) + if bugbot: + head = body[: bugbot.start()].rstrip() + return f"{head}\n\n{section}\n{body[bugbot.start():]}" + return f"{body.rstrip()}\n\n{section}" + + +def main() -> None: + parser = argparse.ArgumentParser(description=__doc__.splitlines()[0]) + parser.add_argument("pr", help="PR number") + parser.add_argument( + "brief_file", help="file holding the brief, without its heading" + ) + parser.add_argument( + "--dry-run", + action="store_true", + help="print the new description instead of saving it", + ) + args = parser.parse_args() + + with open(args.brief_file, encoding="utf-8") as f: + brief = f.read() + brief = SECTION.sub("", brief.replace("\r\n", "\n"), count=1) + + body = gh("pr", "view", args.pr, "--json", "body", "--jq", ".body") + new_body = replace_brief(body, brief) + + if args.dry_run: + print(new_body) + return + with tempfile.NamedTemporaryFile( + "w", suffix=".md", delete=False, encoding="utf-8" + ) as f: + f.write(new_body) + gh("pr", "edit", args.pr, "--body-file", f.name) + print(f"Updated the Reviewer Brief on PR {args.pr}.") + + +if __name__ == "__main__": + main() diff --git a/.cursor/BUGBOT.md b/.cursor/BUGBOT.md index 1a2f9bc3..571d6019 100644 --- a/.cursor/BUGBOT.md +++ b/.cursor/BUGBOT.md @@ -24,4 +24,4 @@ If a finding depends on code or behaviour you can't see, such as a Redis server ## PR descriptions -Authors put a "Review record" section in the PR description. It lists what their own pre-PR review covered, fixed, rejected and didn't check. Treat it as a claim to verify, not as evidence. If you find a problem it says was fixed, or one in an area it says was checked, say so explicitly in the finding. +Authors put a "Reviewer Brief" section in the PR description. It names where to start reading, places the change could fail, and what wasn't verified. Treat it as a claim to verify, not as evidence. If you find a problem in an area the brief doesn't flag, or one that contradicts it, say so explicitly in the finding. diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 5e746f0e..6987c232 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -6,10 +6,14 @@ +## Notes + + + ## Release Notes -## Review record +## Reviewer Brief - + diff --git a/AGENTS.md b/AGENTS.md index e04e4b4d..0528bb8b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -132,19 +132,20 @@ redisvl/ ## Before you mark a PR ready -Run the pre-PR review and put its Review record in the PR description. A human approves every merge to `main`, and the record is what lets them do that quickly: it tells them which commit was reviewed, from which angles, what was fixed, what was rejected and why, and what was not checked. +Run the pre-PR review and put its Reviewer Brief in the PR description. A human approves every merge to `main`. The review catches problems before they reach that person, and the brief gives them a head start: where to begin reading, where the change could still fail, and what nobody has checked. Settle any open questions with the author before the PR is opened or marked ready, so the PR reaches the reviewer with none. -The procedure lives in `.agents/skills/pre-pr-review/SKILL.md`, in the Agent Skills format. If your agent doesn't discover skills there by itself, open `SKILL.md` and follow it as written: it's plain Markdown, and every file it refers to is under `.agents/skills/pre-pr-review/references/`. +The procedure lives in `.agents/skills/pre-pr-review/SKILL.md`, in the Agent Skills format. If your agent doesn't discover skills there by itself, open `SKILL.md` and follow it as written. It's plain Markdown, and every file it refers to is under `.agents/skills/pre-pr-review/`. In short, the review: 1. Reads the full diff against `main`, plus the callers of anything public that changed. -2. Picks which of six lenses the diff needs (correctness always; security, design, tests, docs and Redis behaviour when the diff calls for them) and records why. -3. Runs one pass per lens, then a separate challenger pass that drops any finding it can't tie to a file, a line and a concrete failure. +2. Runs six review lenses (correctness, security, design, tests, docs and Redis behaviour), each starting fresh from the diff. Authors who want to save tokens can run fewer; correctness always runs. +3. Runs a separate challenger pass that drops any finding it can't tie to a file, a line and a concrete failure. 4. Fixes the surviving findings, in scope, and re-runs the checks those fixes touched. -5. Writes the Review record into the PR description, replacing any earlier one. +5. Settles anything it couldn't resolve with the author, so the PR has no open questions. +6. Writes the Reviewer Brief into the PR description. `scripts/update_brief.py` replaces just that section. -Re-run it after any substantive push. A record that names an older commit than the PR's head is stale. +Write the brief for a reviewer who sees the change as it is now and has never seen your review. Point them at the current code, not at problems you've already fixed. Re-run the review after any substantive push, because the brief names the commit it reviewed. ## Redis documentation @@ -152,9 +153,9 @@ Check Redis behaviour against the official documentation, not memory. Redis host ## Responding to review -Cursor Bugbot reviews every PR, and humans may comment too. Handle each point as `SKILL.md` describes under "Responding to review": verify it against the code, give it one disposition, reply on the thread and add a line to the Review record. Treat a reviewer's wording as a claim to check, not a verdict. +Cursor Bugbot reviews every PR, and humans may comment too. Handle each point as `SKILL.md` describes under "Responding to review": verify it against the code, give it one outcome, reply on the thread, and update the Reviewer Brief if anything changed. Treat a reviewer's wording as a claim to check, not a verdict. ## Honesty rules -- Never list a check under "Verified" unless you ran it and saw its output. +- Never claim a check in the Reviewer Brief unless you ran it and saw its output. - Say what you didn't check. Cluster tests and tests that need API keys don't run in CI, so name them when the change touches those paths. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 11bf5c4d..bc6a8290 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -284,34 +284,35 @@ Before suggesting a new feature: - After feedback, we expect a response within two weeks - PRs may be closed if they show no activity after this period -### Review Record +### Reviewer Brief -Maintainer PRs, and any PR written with a coding agent, carry a **Review record**: a short section at the end of the PR description that says what review the change went through before a human looked at it. Outside contributors are welcome to add one but don't have to. +Maintainer PRs, and any PR written with a coding agent, carry a **Reviewer Brief**: a short section at the end of the PR description that helps the reviewer get started. Outside contributors are welcome to add one but don't have to. #### Writing one -Ask your coding agent to run the pre-PR review described in [AGENTS.md](AGENTS.md). The procedure is in `.agents/skills/pre-pr-review/`, and any agent can follow it. You can also follow it by hand. The review picks the angles the diff needs (correctness always, and security, design, tests, docs or Redis behaviour when the change calls for them), checks each finding against the code, fixes what holds up, and writes the record under the `## Review record` heading of the PR template. Run it again after any substantive push. +Ask your coding agent to run the pre-PR review described in [AGENTS.md](AGENTS.md). The procedure is in `.agents/skills/pre-pr-review/`, and any agent can follow it. You can also follow it by hand. The review looks at the change from six angles (correctness, security, design, tests, docs and Redis behaviour), checks each finding against the code, fixes what holds up, and then writes the brief under the `## Reviewer Brief` heading of the PR template. Run it again after any substantive push. -A record states: +A brief tells the reviewer: -- the commit it reviewed, and the agent and model that ran it; -- which review angles ran, which were skipped, and why; -- what was fixed, with file and line; -- what was rejected, and why; -- the commands that were run, with their results, and any Redis documentation pages relied on; -- what was **not** checked, for example cluster behaviour or provider APIs. +- where to start reading, and why; +- where the change could still fail, with file and line; +- what wasn't verified, for example cluster behaviour or provider APIs; +- any evidence CI doesn't show, such as a regression test confirmed to fail without the fix; +- which commit was reviewed, by which agent and model. + +It deliberately leaves out what the review fixed. You see the change as it is now, so problems that no longer exist would only cost you time. #### Reading one -The record is the author's claim about their own review. It isn't an approval and it isn't proof, so use it to decide where to look rather than as a reason not to look. A quick way to review a PR that has one: +The brief is the author's account of their own review. It isn't an approval and it isn't proof. Use it to decide where to look first, not as a reason to look less. A quick way to review a PR that has one: 1. Read the summary Bugbot adds to the PR description for what the change does and how risky it is. -2. Check that the record's commit matches the PR's head commit. If later commits aren't covered, ask the author to re-run the review. -3. Read the record. A record with no rejected findings and nothing under "Not checked" deserves more scrutiny, not less. -4. Look at any open Bugbot or reviewer threads. Each should have a reply giving its outcome (fixed, already fixed, invalid with evidence, duplicate, or needs a decision). -5. Spot-check one fixed finding in the diff, and decide whether anything under "Not checked" matters for this change. +2. Check that the commit in the brief's footer matches the PR's head commit, or that a "Since the last brief" line covers the later commits. If it doesn't, ask the author to re-run the review. +3. Start reading where the brief says, then look at each spot under "Where it could fail". +4. Decide whether anything under "Not verified" matters for this change, and read the Notes section for trade-offs the author made on purpose. +5. Check that every open Bugbot or reviewer thread has a reply giving its outcome. -If the record, Bugbot and the spot-checks agree, approve. If they disagree, the disagreement shows you where to review in depth. +If the brief, Bugbot and what you see in the diff agree, approve. Where they disagree is where to review in depth. ### PR Labels @@ -340,7 +341,7 @@ Before submitting your PR, ensure: - [ ] Documentation is updated if needed - [ ] Commit messages are clear and descriptive - [ ] PR description explains what changes were made and why -- [ ] For maintainer or agent-assisted PRs, the Review record names the PR's head commit +- [ ] For maintainer or agent-assisted PRs, the Reviewer Brief covers the PR's head commit - [ ] Appropriate `auto:*` label is applied to indicate the change type ## Getting Help diff --git a/tests/unit/test_pre_pr_review_scripts.py b/tests/unit/test_pre_pr_review_scripts.py new file mode 100644 index 00000000..a16b3d8c --- /dev/null +++ b/tests/unit/test_pre_pr_review_scripts.py @@ -0,0 +1,53 @@ +"""Tests for the pre-PR review skill's helper scripts, which agents run against real PRs.""" + +import importlib.util +from pathlib import Path + +SCRIPT = ( + Path(__file__).resolve().parents[2] + / ".agents/skills/pre-pr-review/scripts/update_brief.py" +) +_spec = importlib.util.spec_from_file_location("update_brief", SCRIPT) +update_brief = importlib.util.module_from_spec(_spec) +_spec.loader.exec_module(update_brief) + +BUGBOT = "\n---\n\n> [!NOTE]\n> bot summary\n\n" + + +def test_replaces_only_the_brief_and_keeps_bugbot_summary(): + body = f"## Motivation\n\nWhy.\n\n## Reviewer Brief\n\nOld brief.\n\n{BUGBOT}" + + result = update_brief.replace_brief(body, "New brief.") + + assert ( + result + == f"## Motivation\n\nWhy.\n\n## Reviewer Brief\n\nNew brief.\n\n{BUGBOT}" + ) + + +def test_windows_line_endings_do_not_duplicate_the_section(): + body = "## Motivation\r\n\r\nWhy.\r\n\r\n## Reviewer Brief\r\n\r\nOld brief.\r\n" + + result = update_brief.replace_brief(body, "New brief.") + + assert result.count("## Reviewer Brief") == 1 + assert "Old brief." not in result + + +def test_replaces_the_older_review_record_heading(): + body = "## Changes\n\nWhat.\n\n## Review record\n\nOld record.\n\n## Notes\n\nKeep me.\n" + + result = update_brief.replace_brief(body, "New brief.") + + assert "## Review record" not in result + assert "Old record." not in result + assert result.endswith("## Reviewer Brief\n\nNew brief.\n\n## Notes\n\nKeep me.\n") + + +def test_inserts_before_bugbot_summary_when_no_section_exists(): + body = f"## Motivation\n\nWhy.\n\n{BUGBOT}" + + result = update_brief.replace_brief(body, "New brief.") + + assert result.index("## Reviewer Brief") < result.index("") + assert result.endswith(BUGBOT)