Repository navigation
docs(agents): add create-implementation-plan-for-redis-api-change skill (stacked on #574) - #575
Conversation
NRedisStack had no agent instructions for adding commands. Add .agents/skills/extend-commands-api/SKILL.md, with .claude/skills symlinked to it as in Lettuce, modelled on the Jedis and Lettuce skills of the same name and grounded in this repo's conventions: - the module layout: interfaces with <seealso> docs, sync classes inheriting async, <Module>CommandBuilder returning SerializedCommand with a CommandCategory, and the Literals classes - PublicAPI.Unshipped.txt, and Experiments for preview features - tests: EndpointsFixture with AllEnvironments / StandaloneOnly, CreateKeyName for multi-key commands on cluster, SkipIfRedisTheory gating, and REDIS_VERSION - dotnet format, plus FT.ALIASLIST (redis#518) as the worked example Supervised (default) follows plan mode and approval. Unattended mode, used only on `Mode: unattended` or CLIENT_SKILL_MODE=unattended, takes the HLD and the endpoints from the automation. It runs the new theories against standalone and cluster via REDIS_ENDPOINTS_CONFIG_PATH and reports counts per topology.
The automation's standalone and cluster now require passwords. endpoints.json carries them per entry, EndpointConfig.CreateConnection applies them, and the cluster probe authenticates.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 663ccfca4c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| |---|---|---| | ||
| | Command literal | `src/NRedisStack/<Module>/Literals/Commands.cs`, `internal class FT` / `JSON` / `TS` / `BF` / `CF` / `CMS` / `TOPK` / `TDIGEST` | `public const string X = "FT.X";` | | ||
| | Argument tokens | `src/NRedisStack/<Module>/Literals/CommandArgs.cs` (`SearchArgs`, ...), `Literals/Enums/`, `Literals/FieldOptions.cs` | one const per new token, in wire order where the builder emits them | | ||
| | Builder | `src/NRedisStack/<Module>/<Module>CommandBuilder.cs` (`TimeSeriesCommandsBuilder.cs` for TS) | `public static SerializedCommand X(...)` returning `new(CommandCategories.<Rung>, FT.X, args...)`; a computed category (cf. `AggregateCategory`) when it depends on arguments | |
There was a problem hiding this comment.
When <Module> is CuckooFilter or CountMinSketch as defined above, this template expands to files that do not exist (CuckooFilterCommandBuilder.cs and CountMinSketchCommandBuilder.cs); the repository uses CuckooCommandBuilder.cs and CmsCommandBuilder.cs, with the same abbreviated stems for interfaces, implementations, and tests. A CF/CMS plan generated from this map will therefore either fail the skill's existence check or create parallel classes instead of extending the real API, so add an explicit folder-to-class/test stem mapping.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3b0158a: the repository map now has an explicit folder-to-stem table - CuckooFilter -> Cuckoo* (CuckooCommandBuilder.cs, ICuckooCommands(Async).cs, CuckooCommands(Async).cs, CuckooFilter/CuckooTests.cs), CountMinSketch -> Cms*, TimeSeries' TimeSeriesCommandsBuilder.cs - and says a CF/CMS plan extends those classes rather than creating parallel ones.
| `--filter "FullyQualifiedName~A|FullyQualifiedName~B"`, and `~TestAliasList` also matches the | ||
| `Async` twin. Name the sync theory per class; the coding task runs them against both |
There was a problem hiding this comment.
For TimeSeries changes the sync and async theories are in different classes—for example NRedisStack.Tests.TimeSeries.TestAPI.TestRead.TestReadBatch versus ...TestReadAsync.TestReadBatchAsync (tests/NRedisStack.Tests/TimeSeries/TestAPI/TestRead.cs:7,20 and TestReadAsync.cs:8,15). A FullyQualifiedName~ target containing the sync class and method cannot also match the async name, unlike the same-class SearchTests.TestAliasList example, so naming only the sync theory leaves the parity pipeline's async tests unexecuted; require a separate async target for split classes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3b0158a: rule 11 now states that ~<Class>.<Theory> matches the Async twin only when both are in the same class, and that TimeSeries' split classes (TestRead.TestReadBatch vs TestReadAsync.TestReadBatchAsync) need both the sync and the async target; the frontmatter example carries the same note.
|
|
||
| A plan that cites line numbers, proposes a builder method without a command category, passes a | ||
| key as `string`, proposes a signature with no sibling or `R.x` behind it, lists a file that does | ||
| not exist on `master`, or names only sync theories or only `StandaloneOnly` without a reason has |
There was a problem hiding this comment.
For changes that require a new result model or enum, a new TimeSeries test file, docs/exp/NRSxxx.md, or an entire class-E module, the required path | add row necessarily names a file absent from master. This unconditional failure criterion contradicts the output contract's add/edit table and will make valid plans omit required layers or be rejected; restrict the existence rule to cited existing files and permit paths explicitly marked add.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3b0158a: the existence rule applies to files cited as existing; rows marked add in the Files-to-change table may name files not yet on master (Purpose and the Testing-the-skill failure criterion both updated).
- folder-to-class stem mapping for CuckooFilter (Cuckoo*) and CountMinSketch (Cms*) - TimeSeries split test classes need both the sync and the async target - add rows may name files not yet on master; decision_class: none for no-change plans; supervised runs default to ./PLAN.md; agent guidance is procedure, not data
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b0158aded
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| |---|---|---| | ||
| | FT.CREATE `COMPRESSION SQ8` / `TRAINING_THRESHOLD`, RediSearch `v8.10.1..v8.11.80` (module PR #11330) | HLD at `./HLD.md`, `Mode: unattended` | `decision_class: A`; changes confined to `Schema.VectorField` (typed attribute or the `Attributes` dictionary, as the HLD decides) and `Search/Literals/CommandArgs.cs`; FT.INFO additions, if any, in `Search/DataTypes/InfoResult.cs`; no interface or impl change; `FT.CREATE` keeps `WriteAccumulating`; theories in `Search/SearchTests.cs` gated at the first Redis release bundling the module and an args unit test in `IndexCreationTests.cs`; `integration_targets` fully qualified | | ||
| | FT.ALIASLIST (#518) re-planned from an HLD | supervised | `decision_class: C`; the file list equals the `37f7662` trace; `CommandCategories.ReadOnly` with reasoning; five PublicAPI lines named; `TestAliasList` + `TestAliasListAsync` over `AllEnvironments` gated `8.10.0`; `integration_targets: [NRedisStack.Tests.Search.SearchTests.TestAliasList]` | | ||
| | A core command that `IDatabase` already exposes (HLD surface `core`) | supervised or unattended | `decision_class: D`, `estimated_size: none`, no steps; section 1 names the `IDatabase` member found | |
There was a problem hiding this comment.
decision_class: none
When the core command is already exposed, this acceptance row requires D with estimated_size: none, but procedure step 2 and the frontmatter contract both say decision_class must be none for a no-change plan. An unattended run for exactly this scenario therefore cannot satisfy both the output contract and the skill test; set this row to none or document the exception consistently.
Useful? React with 👍 / 👎.
| 4. **Files to change** - `path | add/edit | what`, including tests, `PublicAPI.Unshipped.txt`, | ||
| `CommandCategoryTests.cs` and, if experimental, `Experiments.cs` + `docs/exp/` + |
There was a problem hiding this comment.
For changes that reuse an existing parameter surface (such as the documented VectorField.Attributes alternative) there is no public member to track, and for fixed-category builders such as FT.ALIASLIST no CommandCategoryTests row is needed; nevertheless this add/edit-only table requires both files. This conflicts with procedure step 4's per-layer needed/not-needed decision and with the 37f7662 acceptance file list, so generated plans can prescribe unnecessary edits; require each file only when its corresponding public API or argument-dependent category actually changes.
Useful? React with 👍 / 👎.
1 participant
What
Adds the
create-implementation-plan-for-redis-api-changeagent skill (.agents/skills/create-implementation-plan-for-redis-api-change/SKILL.md): given the shared client HLD for a Redis API change, it produces this repository's implementation plan (exact public signatures per API flavor, files to change, ordered steps, test plan and the integration test classes to run) without writing code.extend-commands-apiskill, referenced by section heading (decision tree, API/arguments conventions, test matrix); this skill adds the planning structure and the repository map the planner must consult.metadata.modes: supervised, unattended): supervised in Claude Code / Codex / Cursor; unattended in the Redis clients parity pipeline (RedisClientsBot), which runs it after an HLD is approved and opens the resulting plan for human review before any code is written.Stacked on #574 (which adds the
extend-commands-apiskill this one references): the diff shows both until #574 merges, after which only the planning skill, its.claude/skillssymlink and the CONTRIBUTING "Agent skills" section remain.Running it locally
Open this repo in Claude Code and ask, for example: "Use the create-implementation-plan-for-redis-api-change skill to plan BLESS (redis/redis#15649) from ./HLD.md". The skill writes
./PLAN.md.Drafted with AI assistance (Claude Code) and reviewed by a human before opening; verify before relying on it.
🤖 Generated with Claude Code
Note
Low Risk
Documentation and agent workflow only; no runtime library, API, or test code changes.
Overview
Adds a new read-only agent skill,
create-implementation-plan-for-redis-api-change, that turns a shared client HLD into a single reviewablePLAN.md(public API, file list, steps, tests, risks) before any NRedisStack code is written. It delegates coding conventions toextend-commands-apiand documents supervised vs unattended (RedisClientsBot) behavior, evidence rules, a repository layer map, and a pydantic-validated plan frontmatter contract.A
.claude/skills/symlink points at the same skill folder for Claude Code. CONTRIBUTING.md gains an Agent skills section listing both skills and where they live under.agents/skills/.Reviewed by Cursor Bugbot for commit 3b0158a. Bugbot is set up for automated code reviews on this repo. Configure here.