From 9c5317e7526a5c4414fe4e39ca7cda5c8e1ac263 Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Thu, 15 Oct 2026 17:47:52 +0200 Subject: [PATCH 1/3] docs(agents): add a pre-PR review skill and agent-neutral guidance Move the project guidance from CLAUDE.md into AGENTS.md so that every coding agent reads the same instructions; CLAUDE.md now only imports it. Add a pre-pr-review skill in the Agent Skills format under .agents/skills/. It chooses review lenses per diff (correctness always; security, design, tests and docs when the diff calls for them), runs a challenger pass that keeps only findings tied to a line and a concrete failure, and writes a Review record into the PR description so the approver can see what was reviewed, fixed, rejected and not checked. Add a Bugbot guide scoped to what CI cannot check, and a PR template with a Review record section. --- .agents/skills/pre-pr-review/SKILL.md | 82 +++++++++ .../pre-pr-review/references/challenger.md | 23 +++ .../pre-pr-review/references/correctness.md | 24 +++ .../skills/pre-pr-review/references/design.md | 18 ++ .../skills/pre-pr-review/references/docs.md | 16 ++ .../pre-pr-review/references/review-record.md | 48 ++++++ .../pre-pr-review/references/security.md | 23 +++ .../skills/pre-pr-review/references/tests.md | 16 ++ .cursor/BUGBOT.md | 27 +++ .github/pull_request_template.md | 15 ++ .gitignore | 3 +- AGENTS.md | 156 ++++++++++++++++++ CLAUDE.md | 128 +------------- 13 files changed, 451 insertions(+), 128 deletions(-) create mode 100644 .agents/skills/pre-pr-review/SKILL.md create mode 100644 .agents/skills/pre-pr-review/references/challenger.md create mode 100644 .agents/skills/pre-pr-review/references/correctness.md create mode 100644 .agents/skills/pre-pr-review/references/design.md create mode 100644 .agents/skills/pre-pr-review/references/docs.md create mode 100644 .agents/skills/pre-pr-review/references/review-record.md create mode 100644 .agents/skills/pre-pr-review/references/security.md create mode 100644 .agents/skills/pre-pr-review/references/tests.md create mode 100644 .cursor/BUGBOT.md create mode 100644 .github/pull_request_template.md create mode 100644 AGENTS.md diff --git a/.agents/skills/pre-pr-review/SKILL.md b/.agents/skills/pre-pr-review/SKILL.md new file mode 100644 index 000000000..e405c9159 --- /dev/null +++ b/.agents/skills/pre-pr-review/SKILL.md @@ -0,0 +1,82 @@ +--- +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; gh is optional and only used to read and update the PR. +--- + +# 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 | + +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 --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. + +## 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 --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:** + - `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. + +Commit fixes locally; the author pushes. Ask the author before you post replies on the PR, unless they've already told you to. diff --git a/.agents/skills/pre-pr-review/references/challenger.md b/.agents/skills/pre-pr-review/references/challenger.md new file mode 100644 index 000000000..e39e218df --- /dev/null +++ b/.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. diff --git a/.agents/skills/pre-pr-review/references/correctness.md b/.agents/skills/pre-pr-review/references/correctness.md new file mode 100644 index 000000000..f3f4c90de --- /dev/null +++ b/.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. diff --git a/.agents/skills/pre-pr-review/references/design.md b/.agents/skills/pre-pr-review/references/design.md new file mode 100644 index 000000000..fd1df1d57 --- /dev/null +++ b/.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. diff --git a/.agents/skills/pre-pr-review/references/docs.md b/.agents/skills/pre-pr-review/references/docs.md new file mode 100644 index 000000000..0935da054 --- /dev/null +++ b/.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. diff --git a/.agents/skills/pre-pr-review/references/review-record.md b/.agents/skills/pre-pr-review/references/review-record.md new file mode 100644 index 000000000..00047f565 --- /dev/null +++ b/.agents/skills/pre-pr-review/references/review-record.md @@ -0,0 +1,48 @@ +# 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: (); (). +Not checked: . +``` + +Leave out the "Review comments" list until there are comments to respond to. + +## 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/security.md b/.agents/skills/pre-pr-review/references/security.md new file mode 100644 index 000000000..385cf7af6 --- /dev/null +++ b/.agents/skills/pre-pr-review/references/security.md @@ -0,0 +1,23 @@ +# Security lens + +Find changes that let input reach somewhere it shouldn't, or let a caller do or see more than they're allowed to. Report only issues this diff introduces or makes reachable, not ones that already existed. + +## Look for + +- **Query-syntax injection.** A value that leaves its RediSearch clause and changes the query: tag alternation (`|`) widening a tag clause, a quote or backslash breaking out of a text phrase, wildcards or operators passing through unescaped. Treat this as injection, even though it isn't SQL. +- **Asymmetric escaping.** A value escaped at query time but not at index time, or the other way round, either fails to match or matches the wrong documents. +- **Authorisation that fails open.** This is mostly in `redisvl/mcp/`: tenant or claim scoping, locked filters, transport and Host/Origin checks. Look for `getattr(..., None)` chains, empty-list defaults and broad `except` blocks that end in "allow". +- **Destructive operations that hit the wrong target.** `delete`, `clear` and index drops that follow an alias, a prefix or a pattern further than the caller meant them to. +- **Secrets.** API keys, passwords or connection URLs written to logs, exceptions or `repr`. +- **Deserialisation and code execution.** `pickle`, unsafe YAML loading, `eval`, or importing a module named in user-supplied config. +- **CI and supply chain.** Changes under `.github/` that skip tests, widen token permissions, or run untrusted PR content. New or loosened dependency constraints. + +## Severity + +- **High:** an outsider can read or alter data beyond their scope, or delete data. +- **Medium:** the same, but it needs an unusual configuration or a privileged caller. +- Report nothing below medium from this lens. + +## Not in scope + +Denial of service through expensive but well-formed queries, rate limiting, and test-only code. Treat environment variables and CLI flags as trusted. diff --git a/.agents/skills/pre-pr-review/references/tests.md b/.agents/skills/pre-pr-review/references/tests.md new file mode 100644 index 000000000..077c7fe52 --- /dev/null +++ b/.agents/skills/pre-pr-review/references/tests.md @@ -0,0 +1,16 @@ +# Tests lens + +Check that the tests prove the change works, cover the paths that matter, and add nothing that doesn't earn its place. Deleting or merging tests is a valid finding. + +## Look for + +- **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. +- **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. + +## How to report + +For a missing test, name the scenario in one line ("async path with an empty result set"). For a test that should go, say what it duplicates or why it proves nothing. diff --git a/.cursor/BUGBOT.md b/.cursor/BUGBOT.md new file mode 100644 index 000000000..1a2f9bc34 --- /dev/null +++ b/.cursor/BUGBOT.md @@ -0,0 +1,27 @@ +# Bugbot review guide for RedisVL + +RedisVL is a Python client library for vector search on Redis 8 and later. CI already runs formatting, import sorting, mypy and the test matrix, so concentrate on what those can't catch. + +## Look for + +- **Sync and async drift.** Most core classes come in pairs (`SearchIndex` and `AsyncSearchIndex`, and the sync and async methods of extensions, vectorizers and rerankers). Flag a change made to one side and not the other. +- **Query values leaving their clause.** Values interpolated into RediSearch query strings or `SCAN` patterns must stay inside their clause. Flag unescaped tag values (`|` widens a tag clause), quotes or backslashes that break out of a text phrase, and escaping applied at query time but not at index time. +- **Authorisation in the MCP server** (`redisvl/mcp/`). Tenant or claim scoping, locked filters and transport checks must fail closed. Flag defaults or exception handling that end in "allow". +- **Destructive operations.** `delete`, `clear` and index drops must act only on the target the caller named, never on whatever an alias or prefix happens to resolve to. +- **Dependency floors.** A call into an optional dependency must exist in the oldest version `pyproject.toml` allows. Optional dependencies must be imported through `lazy_import` or inside a guard, not at module level. +- **Cluster behaviour.** Cluster tests never run in CI. Flag multi-key operations, pipelines, `SCAN` and partial-failure handling that would behave differently on a Redis cluster. +- **Silent truncation.** Reads meant to return everything must not rely on a default result limit. +- **Public API changes.** Changed signatures, defaults or behaviour on public classes with no deprecation path (`deprecated_argument` and `deprecated_function` in `redisvl.utils.utils`). +- **Docs that contradict the code.** Docstrings, comments, guides under `docs/` and notebooks under `docs/user_guide/` that describe behaviour this PR changes. + +## Ignore + +Formatting, import order, type annotations, naming preferences, and suggestions to add comments or tests for trivial code. + +## When unsure + +If a finding depends on code or behaviour you can't see, such as a Redis server version, a provider's API or a cluster topology, say what would need checking and leave the call to the human. Don't guess. + +## 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. diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md new file mode 100644 index 000000000..5e746f0e8 --- /dev/null +++ b/.github/pull_request_template.md @@ -0,0 +1,15 @@ +## Motivation + + + +## Changes + + + +## Release Notes + + + +## Review record + + diff --git a/.gitignore b/.gitignore index c7ebeec17..a3aa9cdeb 100644 --- a/.gitignore +++ b/.gitignore @@ -230,7 +230,8 @@ libs/redis/docs/.Trash* .python-version tests/data .git -.cursor +.cursor/* +!.cursor/BUGBOT.md .junie .undodir .claude/settings.local.json diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 000000000..eba09018d --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,156 @@ +# AGENTS.md + +Guidance for coding agents working on RedisVL, whichever agent or model you are. + +## Frequently Used Commands + +```bash +# Development workflow +make install # Install dependencies +make format # Format code (black + isort) +make check-types # Run mypy type checking +make lint # Run all linting (format + types) +make test # Run tests (no external APIs) +make test-all # Run all tests (includes API tests) +make check # Full check (lint + test) + +# Redis setup +make redis-start # Start Redis container +make redis-stop # Stop Redis container + +# Documentation +make docs-build # Build documentation +make docs-serve # Serve docs locally +``` + +Pre-commit hooks are also configured, which you should +run before you commit: +```bash +pre-commit run --all-files +``` + +## Important Architectural Patterns + +### Async/Sync Dual Interfaces +- Most core classes have both sync and async versions (e.g., `SearchIndex` / `AsyncSearchIndex`) +- Follow existing patterns when adding new functionality + +### Schema-Driven Design +```python +# Index schemas define structure +schema = IndexSchema.from_yaml("schema.yaml") +index = SearchIndex(schema, redis_url="redis://localhost:6379") +``` + +## Critical Rules + +### Do Not Modify +- **CRITICAL**: Do not change this line unless explicitly asked: + ```python + token.strip().strip(",").replace(""", "").replace(""", "").lower() + ``` + +### Protected Directories +**CRITICAL**: NEVER delete the `local_docs/` directory or any files within it. + +### Git Operations +**CRITICAL**: NEVER use `git push` or attempt to push to remote repositories. The user will handle all git push operations. + +### Code Quality +**IMPORTANT**: Always run `make format` before committing code to ensure proper formatting and linting compliance. + +### README.md Maintenance +**IMPORTANT**: DO NOT modify README.md unless explicitly requested. + +**If you need to document something, use these alternatives:** +- Development info → CONTRIBUTING.md +- API details → docs/ directory +- Examples → docs/examples/ +- Project memory (explicit preferences, directives, etc.) → AGENTS.md (`CLAUDE.md` only imports this file) + +## Testing Notes +RedisVL uses `pytest` with `testcontainers` for testing. + +- `make test` - all tests except those that call external APIs +- `make test-all` - run the full suite, including tests that call external APIs +- `pytest --run-api-tests` - explicitly run API-dependent tests (e.g., LangCache, + external vectorizer/reranker providers). These require the appropriate API + keys and environment variables to be set. + +## Project Structure + +``` +redisvl/ +├── cli/ # Command-line interface (rvl command) +├── extensions/ # AI extensions (cache, memory, routing) +│ ├── cache/ # Semantic caching for LLMs +│ ├── llmcache/ # LLM-specific caching +│ ├── message_history/ # Chat history management +│ ├── router/ # Semantic routing +│ └── session_manager/ # Session management +├── index/ # SearchIndex classes (sync/async) +├── mcp/ # MCP server (tools, auth, transport security) +├── migration/ # Index and data migration +├── query/ # Query builders (Vector, Range, Filter, Count) +├── redis/ # Redis client utilities +├── schema/ # Index schema definitions +└── utils/ # Utilities (vectorizers, rerankers, optimization) + ├── rerank/ # Result reranking + └── vectorize/ # Embedding providers integration +``` + +## Core Components + +### 1. Index Management +- `SearchIndex` / `AsyncSearchIndex` - Main interface for Redis vector indices +- `IndexSchema` - Define index structure with fields (text, tags, vectors, etc.) +- Support for JSON and Hash storage types + +### 2. Query System +- `VectorQuery` - Semantic similarity search +- `RangeQuery` - Vector search within distance range +- `FilterQuery` - Metadata filtering and full-text search +- `CountQuery` - Count matching records +- Etc. + +### 3. AI Extensions +- `SemanticCache` - LLM response caching with semantic similarity +- `EmbeddingsCache` - Cache for vector embeddings +- `MessageHistory` - Chat history with recency/relevancy retrieval +- `SemanticRouter` - Route queries to topics/intents + +### 4. Vectorizers (Optional Dependencies) +- OpenAI, Azure OpenAI, Cohere, HuggingFace, Mistral, VoyageAI +- Custom vectorizer support +- Batch processing capabilities + +## Documentation +- Main docs: https://docs.redisvl.com +- Built with Sphinx from `docs/` directory +- Includes API reference and user guides +- Example notebooks in documentation `docs/user_guide/...` + +## 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. + +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/`. + +In short, the review: + +1. Reads the full diff against `main`, plus the callers of anything public that changed. +2. Picks which of five lenses the diff needs (correctness always; security, design, tests and docs 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. +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. + +Re-run it after any substantive push. A record that names an older commit than the PR's head is stale. + +## 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. + +## Honesty rules + +- Never list a check under "Verified" 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/CLAUDE.md b/CLAUDE.md index 9f22e9b93..43c994c2d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1,127 +1 @@ -# CLAUDE.md - RedisVL Project Context - -## Frequently Used Commands - -```bash -# Development workflow -make install # Install dependencies -make format # Format code (black + isort) -make check-types # Run mypy type checking -make lint # Run all linting (format + types) -make test # Run tests (no external APIs) -make test-all # Run all tests (includes API tests) -make check # Full check (lint + test) - -# Redis setup -make redis-start # Start Redis container -make redis-stop # Stop Redis container - -# Documentation -make docs-build # Build documentation -make docs-serve # Serve docs locally -``` - -Pre-commit hooks are also configured, which you should -run before you commit: -```bash -pre-commit run --all-files -``` - -## Important Architectural Patterns - -### Async/Sync Dual Interfaces -- Most core classes have both sync and async versions (e.g., `SearchIndex` / `AsyncSearchIndex`) -- Follow existing patterns when adding new functionality - -### Schema-Driven Design -```python -# Index schemas define structure -schema = IndexSchema.from_yaml("schema.yaml") -index = SearchIndex(schema, redis_url="redis://localhost:6379") -``` - -## Critical Rules - -### Do Not Modify -- **CRITICAL**: Do not change this line unless explicitly asked: - ```python - token.strip().strip(",").replace(""", "").replace(""", "").lower() - ``` - -### Protected Directories -**CRITICAL**: NEVER delete the `local_docs/` directory or any files within it. - -### Git Operations -**CRITICAL**: NEVER use `git push` or attempt to push to remote repositories. The user will handle all git push operations. - -### Code Quality -**IMPORTANT**: Always run `make format` before committing code to ensure proper formatting and linting compliance. - -### README.md Maintenance -**IMPORTANT**: DO NOT modify README.md unless explicitly requested. - -**If you need to document something, use these alternatives:** -- Development info → CONTRIBUTING.md -- API details → docs/ directory -- Examples → docs/examples/ -- Project memory (explicit preferences, directives, etc.) → CLAUDE.md - -## Testing Notes -RedisVL uses `pytest` with `testcontainers` for testing. - -- `make test` - unit tests only (no external APIs) -- `make test-all` - run the full suite, including tests that call external APIs -- `pytest --run-api-tests` - explicitly run API-dependent tests (e.g., LangCache, - external vectorizer/reranker providers). These require the appropriate API - keys and environment variables to be set. - -## Project Structure - -``` -redisvl/ -├── cli/ # Command-line interface (rvl command) -├── extensions/ # AI extensions (cache, memory, routing) -│ ├── cache/ # Semantic caching for LLMs -│ ├── llmcache/ # LLM-specific caching -│ ├── message_history/ # Chat history management -│ ├── router/ # Semantic routing -│ └── session_manager/ # Session management -├── index/ # SearchIndex classes (sync/async) -├── query/ # Query builders (Vector, Range, Filter, Count) -├── redis/ # Redis client utilities -├── schema/ # Index schema definitions -└── utils/ # Utilities (vectorizers, rerankers, optimization) - ├── rerank/ # Result reranking - └── vectorize/ # Embedding providers integration -``` - -## Core Components - -### 1. Index Management -- `SearchIndex` / `AsyncSearchIndex` - Main interface for Redis vector indices -- `IndexSchema` - Define index structure with fields (text, tags, vectors, etc.) -- Support for JSON and Hash storage types - -### 2. Query System -- `VectorQuery` - Semantic similarity search -- `RangeQuery` - Vector search within distance range -- `FilterQuery` - Metadata filtering and full-text search -- `CountQuery` - Count matching records -- Etc. - -### 3. AI Extensions -- `SemanticCache` - LLM response caching with semantic similarity -- `EmbeddingsCache` - Cache for vector embeddings -- `MessageHistory` - Chat history with recency/relevancy retrieval -- `SemanticRouter` - Route queries to topics/intents - -### 4. Vectorizers (Optional Dependencies) -- OpenAI, Azure OpenAI, Cohere, HuggingFace, Mistral, VoyageAI -- Custom vectorizer support -- Batch processing capabilities - -## Documentation -- Main docs: https://docs.redisvl.com -- Built with Sphinx from `docs/` directory -- Includes API reference and user guides -- Example notebooks in documentation `docs/user_guide/...` +@AGENTS.md From 8ad1d450e9ac4d792a059c17fdbc55a397130878 Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Thu, 15 Oct 2026 17:50:40 +0200 Subject: [PATCH 2/3] docs(contributing): explain the Review record and how to read it Describe what the Review record in a PR description contains, how to produce it with the pre-PR review in AGENTS.md, and a short routine for human reviewers: check the reviewed commit matches the head, read the record alongside the Bugbot summary and open threads, and spot-check one fix and the "Not checked" items before approving. --- CONTRIBUTING.md | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 33e79f75f..992fd9ab6 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -284,6 +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 + +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. + +#### 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 or docs 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. + +A record states: + +- 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; +- what was **not** checked, for example cluster behaviour or provider APIs. + +#### 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: + +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. + +If the record, Bugbot and the spot-checks agree, approve. If they disagree, the disagreement shows you where to review in depth. + ### PR Labels This project uses an automated release process powered by [Auto](https://intuit.github.io/auto/). PRs must be labeled to indicate the type of change: @@ -311,6 +340,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 - [ ] Appropriate `auto:*` label is applied to indicate the change type ## Getting Help From 221aaf9395a609d3a7fdad80df4a965983130495 Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Thu, 15 Oct 2026 17:58:07 +0200 Subject: [PATCH 3/3] docs(agents): add a Redis behaviour lens backed by the Redis Docs MCP Add a sixth pre-PR review lens that checks the change's assumptions about Redis (command syntax, defaults, limits, reply shapes and version-specific behaviour) against the official documentation rather than model memory. It recommends the public, read-only Redis Docs MCP at https://redis.io/mcp, falls back to redis.io/docs, and requires each finding to cite the page it rests on. Recommend the same server in AGENTS.md for everyday Redis questions, and add a "Docs consulted" line to the Review record. --- .agents/skills/pre-pr-review/SKILL.md | 3 +- .../skills/pre-pr-review/references/redis.md | 34 +++++++++++++++++++ .../pre-pr-review/references/review-record.md | 3 +- AGENTS.md | 6 +++- CONTRIBUTING.md | 4 +-- 5 files changed, 45 insertions(+), 5 deletions(-) create mode 100644 .agents/skills/pre-pr-review/references/redis.md diff --git a/.agents/skills/pre-pr-review/SKILL.md b/.agents/skills/pre-pr-review/SKILL.md index e405c9159..e3babacc5 100644 --- a/.agents/skills/pre-pr-review/SKILL.md +++ b/.agents/skills/pre-pr-review/SKILL.md @@ -1,7 +1,7 @@ --- 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; gh is optional and only used to read and update the 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 @@ -28,6 +28,7 @@ Pick the lenses this diff needs, and write down the reason for each one you run | 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. diff --git a/.agents/skills/pre-pr-review/references/redis.md b/.agents/skills/pre-pr-review/references/redis.md new file mode 100644 index 000000000..6e49ce9df --- /dev/null +++ b/.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: `. 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. diff --git a/.agents/skills/pre-pr-review/references/review-record.md b/.agents/skills/pre-pr-review/references/review-record.md index 00047f565..5ea6de91d 100644 --- a/.agents/skills/pre-pr-review/references/review-record.md +++ b/.agents/skills/pre-pr-review/references/review-record.md @@ -17,10 +17,11 @@ Review comments: - : — . Verified: (); (). +Docs consulted: ; . Not checked: . ``` -Leave out the "Review comments" list until there are comments to respond to. +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 diff --git a/AGENTS.md b/AGENTS.md index eba09018d..e04e4b4d7 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -139,13 +139,17 @@ The procedure lives in `.agents/skills/pre-pr-review/SKILL.md`, in the Agent Ski In short, the review: 1. Reads the full diff against `main`, plus the callers of anything public that changed. -2. Picks which of five lenses the diff needs (correctness always; security, design, tests and docs when the diff calls for them) and records why. +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. 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. Re-run it after any substantive push. A record that names an older commit than the PR's head is stale. +## Redis documentation + +Check Redis behaviour against the official documentation, not memory. Redis hosts a public, read-only MCP server for its docs at `https://redis.io/mcp`, with no API key or sign-up; setup for each client is at `https://marketing.redis.io/docs-mcp/`. Its `search` and `fetch` tools are the quick way to confirm command syntax, defaults and version-specific behaviour while you work, and the pre-PR review's Redis behaviour lens uses them. Send it questions about Redis, not code or credentials. + ## 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. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 992fd9ab6..11bf5c4d2 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -290,7 +290,7 @@ Maintainer PRs, and any PR written with a coding agent, carry a **Review record* #### 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 or docs 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 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. A record states: @@ -298,7 +298,7 @@ A record states: - 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; +- 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. #### Reading one