docs(agents): agent-neutral pre-PR review skill and Review record

.agents/skills/pre-pr-review/SKILL.md

@@ -0,0 +1,83 @@
+---
+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.
+---
+
+# 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.
+
+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.
+
+## 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.
+
+## 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.
+
+| Lens | File | Run it when |
+|---|---|---|
+| Correctness | `references/correctness.md` | Always. A single quick pass for diffs under 100 lines and for chore, CI or refactor changes |
+| 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 |
+| 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.
+
+## 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.
+
+- 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:
+
+```
+[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.
+
+## 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 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).
+
+## 6. Write the Review record
+
+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.
+
+With `gh`: fetch the current description (`gh pr view <number> --json body -q .body`), replace only that section, write the whole description to a file and run `gh pr edit <number> --body-file <file>`. `--body-file` replaces the entire description, so never pass it the record alone. Without `gh`, print the record for the author to paste.
+
+## Responding to review
+
+When Bugbot or a person comments on the PR, handle each point like this:
+
+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 <number> --comments` shows reviews and top-level comments, and `gh api --paginate repos/{owner}/{repo}/pulls/<number>/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:**
+   - `fixed in <sha>`
+   - `already fixed`
+   - `invalid`, with the evidence
+   - `duplicate of <item>`
+   - `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.
+
+Commit fixes locally; the author pushes. Ask the author before you post replies on the PR, unless they've already told you to.

.agents/skills/pre-pr-review/references/challenger.md

@@ -0,0 +1,23 @@
+# Challenger pass
+
+The challenger's job is to remove false positives before they cost anyone time. It starts from the code and the list of findings, not from the reasoning that produced them. If your agent supports isolated sub-tasks, run the challenger as one, giving it only the diff and the findings list. If it doesn't, put the lens passes out of mind: re-open only the diff and the scratch file of findings, and judge each finding from the code alone.
+
+For each finding:
+
+1. Open the file at the cited line and read enough context to judge the claim.
+2. Try to build the failure scenario the finding describes. If you can, write it down as an input and the result, or as a short test.
+3. Look for anything that already prevents it: a guard elsewhere, a caller that never passes that input, a test that covers it.
+4. Score it.
+
+## Scale
+
+| Score | Meaning |
+|---|---|
+| 9–10 | Demonstrated: the failure is reproduced or follows directly from the code |
+| 7–8 | Very likely: the failure follows from the code as written, and nothing prevents it |
+| 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.
+
+The challenger may also add a finding it noticed while checking. A finding it adds gets the same scoring, done in a fresh pass.

.agents/skills/pre-pr-review/references/correctness.md

@@ -0,0 +1,24 @@
+# Correctness lens
+
+Find changes that make the code behave wrongly for some input, state or environment the project supports. This lens runs on every PR.
+
+## Look for
+
+- **Line-level mistakes:** wrong or inverted conditions, off-by-one errors, a missing `await`, swallowed or over-broad exceptions, mutable default arguments, late-binding closures in loops.
+- **Removed behaviour:** for every deleted or rewritten line, name the invariant it enforced, such as a validation, an escape, a guard or a cleanup. Then find where the new code re-establishes it. If you can't find that place, report it.
+- **Broken contracts:** callers that rely on a changed return shape, a new exception, a new precondition or a changed default. Search for every caller, including tests, docs and notebooks.
+- **The sync or async twin:** a fix to `SearchIndex` that `AsyncSearchIndex` lacks, or the other way round. The same applies to every sync/async pair in `redisvl/extensions/` and to the async vectorizer and reranker methods.
+- **Wrappers and proxies:** a wrapper that drops arguments, ignores a return value, or forgets to close or release what it opened.
+- **Setup and teardown asymmetry:** something opened, locked or registered on one path and not released on another, especially on error paths.
+
+## RedisVL specifics
+
+- **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.
+- **Client ownership.** Who creates, owns and closes a Redis client, and what happens on reconnect, especially for async clients reused across event loops.
+
+## Depth
+
+For diffs under 100 lines, and for chore, CI or refactor changes, do a single pass over the diff and its direct callers. For anything larger, also trace each changed public function through its callers and its twin.

.agents/skills/pre-pr-review/references/design.md

@@ -0,0 +1,18 @@
+# Design lens
+
+Ask whether the change is a lasting solution at the right layer, or a patch that the next change will have to undo. In this repository, this lens more often produces a decision than a defect: for example, "remove the deprecated API instead of patching it".
+
+## Look for
+
+- **The wrong layer.** A special case bolted onto a caller when the problem lives in a shared builder, client or base class. Or a fix in one subclass that its siblings also need.
+- **Patching a footgun.** Effort spent making a deprecated or error-prone API safer, when finishing its deprecation or removing it would be simpler. Use `deprecated_argument` and `deprecated_function` from `redisvl.utils.utils` for the path out.
+- **Public API shape.** New or changed signatures, defaults and return types, and whether they stay consistent across sync and async classes and with sibling classes. One example is a new vectorizer that doesn't follow its siblings' constructor and method conventions.
+- **Compatibility.** Changes to public behaviour or defaults with no deprecation path. Compatibility branches left behind after a version floor was raised.
+- **New dependencies.** A new required dependency, or an optional one imported at module level instead of through `lazy_import`.
+- **Error semantics.** What the caller sees on partial failure, retry or reconnect, and whether that matches how the rest of the package behaves.
+
+## How to report
+
+Name the cost of leaving it as it is, such as "every new provider repeats this branch", and the smallest change that would remove that cost. If the better design is clearly out of scope, report it as an open item for the author to decide, not as something to fix in this PR.
+
+Don't propose new abstractions for a single use. Three similar lines are cheaper than a premature helper.

.agents/skills/pre-pr-review/references/docs.md

@@ -0,0 +1,16 @@
+# Docs lens
+
+Check that docstrings, comments, guides and notebooks say what the code actually does, and nothing more. Removing lines is a valid finding.
+
+## Look for
+
+- **Claims that contradict the code:** a docstring describing an old default, a comment explaining a branch that no longer exists, a guide calling a role "least privilege" when it grants more than it needs.
+- **Stale guides and notebooks:** pages under `docs/` and notebooks under `docs/user_guide/` that call a changed API, rely on a changed default, or show output that would now differ. Check every usage the diff makes stale, not just the files it touches.
+- **Bloat:** comments that restate the code, docstrings longer than the function they describe, and the same explanation repeated in several places when one should link to the other. A comment should explain why, or warn about something non-obvious.
+- **Missing docs for new public API:** a new public class, function or parameter with no docstring, or with a docstring that doesn't state its defaults and what it raises.
+- **Sphinx breakage:** wrong `currentmodule` or cross-reference targets, and new modules missing from `docs/api/`.
+- **The PR description:** check that it names the change's most consequential behaviour change. If public behaviour changes, check that the description has a Release Notes line a user could act on.
+
+## How to report
+
+Quote the claim that is wrong, or the lines that should go, in a few words, and give the file and line. Don't rewrite prose for style. Report only text that misleads, is out of date, or wastes the reader's time.

.agents/skills/pre-pr-review/references/redis.md

@@ -0,0 +1,34 @@
+# Redis behaviour lens
+
+Check every assumption the change makes about how Redis behaves against the official documentation, not against memory. Version-specific behaviour is where models are most confidently wrong about Redis, and Redis 8 changed several defaults.
+
+## The Redis Docs MCP
+
+Redis hosts a public, read-only MCP server for its documentation at `https://redis.io/mcp`. It needs no API key or sign-up. Add it to your agent as a streamable-HTTP MCP server named `redis-docs`. Setup for each client is at `https://marketing.redis.io/docs-mcp/`. It has three tools:
+
+- `search`: free-text search across the redis.io docs. It returns matching pages with snippets and stable ids. Start here.
+- `fetch`: the full text of one page, given an id from `search`. Use it when a snippet isn't enough to settle the claim.
+- `ask`: a grounded answer plus the source pages behind it. Keep it for questions that span several pages. It runs a model over the results, so it's slower than `search` and `fetch`.
+
+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.
+
+## Run it when
+
+- The diff changes how commands are built or called: `FT.CREATE`, `FT.SEARCH`, `FT.AGGREGATE`, `FT.CURSOR`, `FT.INFO` arguments, query syntax, schema field attributes, vector index parameters, or `SCAN`, pipeline, expiry or eviction behaviour.
+- The diff relies on a server default or limit, for example result limits, dialects, TTL semantics or timeout behaviour.
+- The diff handles replies whose shape depends on RESP2 or RESP3, on storage type (hash or JSON), or on cluster mode.
+- A docstring, comment, guide or notebook in the diff states how Redis behaves.
+
+## Look for
+
+- **Syntax the server doesn't accept,** or accepts with a different meaning. Examples are query operators, escaping rules, field-type options, and argument order in `FT.*` commands.
+- **Defaults assumed wrongly:** a default dialect, result limit or distance metric, or an option the code assumes is on by default.
+- **Version gaps:** behaviour that exists only in some of the Redis versions RedisVL supports, with no guard or note.
+- **Reply-shape assumptions** that hold for one protocol, storage type or topology and break for another.
+- **Docs claims that contradict the documentation.**
+
+## How to report
+
+Each finding cites the page it rests on: `... Per: <redis.io URL>`. 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.

.agents/skills/pre-pr-review/references/review-record.md

@@ -0,0 +1,49 @@
+# 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 `<short sha>` by <agent> (<model>) with `pre-pr-review`.
+Lenses: <lens> (<reason>); <lens> (<reason>). Skipped: <lens> (<reason>).
+
+- <n> findings raised, <k> kept after challenge.
+- Fixed: <claim> (`path.py:LINE`); <claim> (`path.py:LINE`).
+- Rejected: <claim> (<reason>).
+- Open for the reviewer: <item needing a decision>, or "none".
+
+Review comments:
+- <source>: <point> — <disposition>.
+
+Verified: <command> (<result>); <command> (<result>).
+Docs consulted: <redis.io URL>; <redis.io URL>.
+Not checked: <what wasn't exercised, and why>.
+```
+
+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: `<sha>` (<what changed>)." 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 (`<model>`) 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.
+```