feat: agent-memory layer — learnings, saved queries, recall (DEV-1357)#100
Merged
whimo merged 5 commits intoMay 6, 2026
Merged
Conversation
Add a memory layer alongside the semantic layer so agents can record free-form notes and example queries against canonical SLayer entities, then look them up before drafting a new query. Four new MCP tools: save_learning, save_query, delete_learning_or_query, recall. Every entity reference is canonicalised to one of <ds>, <ds>.<model>, or <ds>.<model>.<leaf> (≤ 3 dotted segments) — aggregation suffixes are stripped, *:count collapses to the source model, multi-hop join paths keep only the leaf. inspect_model gains an auto-pruned Learnings section listing every note that overlaps the model's entity set; the query tool docstring now directs agents to call recall first. Storage: ID format (L<int>/Q<int>), monotonic non-reuse, and the entity-intersection filter live on the StorageBackend ABC so they can never diverge between backends (per the standing backend-agnostic rule). YAMLStorage and SQLiteStorage only implement the row-shaped CRUD primitives + seq counter. New tables/files: learnings, learning_entities, saved_queries, saved_query_entities, id_counters in SQLite; learnings.yaml, saved_queries.yaml, counters.yaml alongside priority.yaml in YAML. Tests: 47 storage tests parameterised across both backends, 41 entity-resolution tests, 24 MCP tool tests, 7 inspect_model integration tests. Full unit suite green (1955 passed); ruff clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughAdds an agent-memory system (DEV-1357): canonical entity resolution, Memory Pydantic models and responses, MemoryService, storage support (YAML/SQLite) with sequence IDs, MCP/REST/CLI/Python surfaces for save_memory/forget_memory/recall_memories, inspect_model Learnings integration, extensive tests, and docs/specs. ChangesAgent Memory System (single cohort — cohesive DAG)
Sequence Diagram(s)sequenceDiagram
participant Agent
participant MCP as MCP Server
participant Service as MemoryService
participant Resolver as Entity Resolver
participant Storage as StorageBackend
participant DB as YAML/SQLite
Agent->>MCP: save_memory(learning, linked_entities)
MCP->>Service: save_memory(...)
Service->>Resolver: resolve_entity()/extract_entities_from_query()
Resolver->>Storage: read models/datasources
Storage->>DB: read files/tables
DB-->>Storage: metadata
Storage-->>Resolver: metadata
Resolver-->>Service: canonical_forms + warnings
Service->>Storage: save_memory(learning, entities, query)
Storage->>DB: INSERT memory row / update counters
DB-->>Storage: memory_id
Storage-->>Service: saved Memory
Service-->>MCP: SaveMemoryResponse
MCP-->>Agent: response
Agent->>MCP: recall_memories(about, max_learnings, max_queries)
MCP->>Service: recall_memories(...)
Service->>Resolver: extract_entities_from_query(about)
Resolver->>Storage: metadata lookups
Service->>Storage: list_memories(entities)
Storage->>DB: SELECT matching rows
DB-->>Storage: matching rows
Storage-->>Service: Memories
Service-->>MCP: RecallResponse {learnings, queries, resolved_input_entities}
MCP-->>Agent: recalled memories
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
tests/test_learnings_mcp.py (1)
40-83: ⚡ Quick winMove the shared MCP/YAML reset fixtures into
tests/conftest.py.This is the third copy of the session-scoped server +
_reset_storagepattern. Every new persisted artifact now requires editing multiple files; this PR already had to teach each copy aboutlearnings.yaml,saved_queries.yaml, andcounters.yaml. Centralizing it would prevent drift and cross-test leakage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_learnings_mcp.py` around lines 40 - 83, Extract the session-scoped fixtures _shared_storage, _shared_mcp_server and the helper _reset_storage (and the thin wrappers storage and mcp_server) out of tests/test_learnings_mcp.py and place them in tests/conftest.py so they are shared across tests; ensure you preserve the fixtures' signatures (YAMLStorage type, scope="session") and behavior (temporary directory creation, YAMLStorage(base_dir=tmpdir), create_mcp_server(storage=_shared_storage), and the same file cleanup list including "priority.yaml", "learnings.yaml", "saved_queries.yaml", "counters.yaml"); then remove the duplicated definitions from test_learnings_mcp.py and update any local imports/usages (they should work via pytest fixture discovery) so tests reference the shared fixtures (_shared_storage, _shared_mcp_server, _reset_storage, storage, mcp_server) in conftest.py.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@slayer/learnings/resolver.py`:
- Around line 235-264: The code currently treats any aggregation (agg) as valid
for the special wildcard branches, causing invalid forms like '*:sum' or
'orders.*:avg' to collapse to the model; change the logic so these branches only
accept the aggregation literal "count" (i.e., check agg == "count" rather than
agg is None). Update the two places that handle the wildcard shapes — the prefix
== "*" branch (where it currently returns an EntityResolution with source_model)
and the prefix.endswith(".*") branch (which calls resolve_entity) — to
explicitly validate agg == "count", raise an EntityResolutionError for any other
agg (using raw in the message for context), and keep the existing
source_model/storage/resolve_entity behavior only when agg == "count".
In `@slayer/learnings/service.py`:
- Around line 200-207: The recall method currently slices results using
max_learnings and max_queries which, when negative, invoke Python's
negative-index slicing; validate that max_learnings and max_queries are either
None or non-negative integers at the start of recall (and similarly for the
other function around the latter block) and raise a ValueError with a clear
message if a negative value is provided, then perform slicing normally; update
parameter handling in recall to check max_learnings and max_queries before any
list slicing to fail fast.
In `@slayer/mcp/server.py`:
- Around line 1456-1466: The current construction of wanted includes all
model.columns, including hidden ones; update the list comprehensions so only
visible columns are included by filtering model.columns with a visibility check
(e.g., for columns: for c in model.columns if not getattr(c, "hidden", False) or
if getattr(c, "visible", True)). Apply the same visibility guard when adding
measures and aggregations: only include m/ a if m.name is not None and the
measure/aggregation itself is not hidden (getattr(m, "hidden", False) is False)
or, if they reference a column, only include them when the referenced column is
visible (use getattr on the referenced column or column name safely). Ensure you
update the three extensions that reference model.columns, model.measures, and
model.aggregations (the variable wanted) accordingly.
In `@slayer/storage/yaml_storage.py`:
- Around line 177-202: When _read_counters() returns {} (missing/malformed
counters.yaml) the async methods _next_learning_seq() and
_next_saved_query_seq() can restart sequences at 1 and overwrite existing
records; fix by, inside _next_learning_seq() and _next_saved_query_seq(),
detecting empty or invalid counters and recovering the highest existing sequence
from the corresponding data file (read learnings.yaml for learning_seq and
saved_queries.yaml for saved_query_seq), parse existing record IDs (e.g. "L123",
"Q45") to find the max numeric suffix, initialize counters["learning_seq"] /
counters["saved_query_seq"] to that max (or 0 if none), then increment, persist
via _write_counters(counters) and return the new seq; keep using
_read_counters()/_write_counters() to persist the recovered value.
In `@tests/test_entity_resolution.py`:
- Around line 601-610: Update test_star_count_uses_source_model to assert the
exact canonical form list instead of just membership: after calling
extract_entities_from_query(q, storage=storage), assert that
result.canonical_forms is exactly the single expected entry for the source model
(e.g. equals {"mydb.orders"} or a list with length 1 and that single value) so
that an extra canonical form for "*:count" would fail; reference the test
function name test_star_count_uses_source_model and objects SlayerQuery,
ModelMeasure, and extract_entities_from_query when making the change.
In `@tests/test_learnings_inspect.py`:
- Around line 145-175: Update the two tests
(test_section_pruned_when_no_relevant_learnings and
test_section_pruned_when_no_learnings_at_all) to assert that the "## Learnings"
section header is absent in the rendered result as well as the learning body;
specifically, after calling _call(..., name="inspect_model", ...) add assertions
like asserting "## Learnings" not in result and also assert "## learnings" not
in result.lower() so an empty or names-only placeholder is detected and fails
the test. Ensure the added assertions appear in both the existing test and the
analogous test around the other mentioned block.
In `@tests/test_learnings_storage.py`:
- Around line 161-186: Extend the existing
test_id_counter_persists_across_backend_reopen test to also verify the
saved_query sequence by creating two saved queries, closing and re-instantiating
the backend, then creating a third saved query and asserting its id increments
(expects "Q3"); do this for both YAMLStorage and SQLiteStorage. Concretely,
after the learning assertions add for YAMLStorage: call the saved-query creation
method (e.g., save_saved_query or the appropriate API that returns an object
with .id) twice, delete ys, re-create ys2, create a third saved query and assert
third.id == "Q3"; repeat the same sequence for SQLiteStorage (ss, ss2) to ensure
saved_query_seq is persisted separately from learning_seq. Ensure you use the
exact saved-query creation function name present in the codebase and the "Q" id
prefix in assertions.
In `@tests/test_mcp_server.py`:
- Around line 1193-1196: Update the test_resolve_inspect_sections_empty unit to
assert the exact ordered contents of the resolved list instead of only its
length: call _resolve_inspect_sections([]) (function _resolve_inspect_sections)
and assert that resolved equals the explicit 7-item ordered list of expected
section names (matching the same pattern as the None case) and also assert
unknown is empty, so the test fails if any item is missing, duplicated, or out
of order.
---
Nitpick comments:
In `@tests/test_learnings_mcp.py`:
- Around line 40-83: Extract the session-scoped fixtures _shared_storage,
_shared_mcp_server and the helper _reset_storage (and the thin wrappers storage
and mcp_server) out of tests/test_learnings_mcp.py and place them in
tests/conftest.py so they are shared across tests; ensure you preserve the
fixtures' signatures (YAMLStorage type, scope="session") and behavior (temporary
directory creation, YAMLStorage(base_dir=tmpdir),
create_mcp_server(storage=_shared_storage), and the same file cleanup list
including "priority.yaml", "learnings.yaml", "saved_queries.yaml",
"counters.yaml"); then remove the duplicated definitions from
test_learnings_mcp.py and update any local imports/usages (they should work via
pytest fixture discovery) so tests reference the shared fixtures
(_shared_storage, _shared_mcp_server, _reset_storage, storage, mcp_server) in
conftest.py.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ac689c09-140a-4d34-bc3b-cf4b8f31902a
📒 Files selected for processing (20)
.claude/skills/slayer-overview.mdCLAUDE.mddocs/concepts/learnings.mddocs/configuration/storage.mdslayer/core/errors.pyslayer/learnings/__init__.pyslayer/learnings/models.pyslayer/learnings/resolver.pyslayer/learnings/service.pyslayer/mcp/server.pyslayer/storage/base.pyslayer/storage/join_sync.pyslayer/storage/migrations.pyslayer/storage/sqlite_storage.pyslayer/storage/yaml_storage.pytests/test_entity_resolution.pytests/test_learnings_inspect.pytests/test_learnings_mcp.pytests/test_learnings_storage.pytests/test_mcp_server.py
…ace (DEV-1357)
Replace the four-tool surface (`save_learning` / `save_query` /
`delete_learning_or_query` / `recall`) with three tools that operate on
a single `Memory` entity:
- `save_memory(learning, linked_entities)` — `linked_entities` accepts
either a list of entity strings (resolved strictly) or a `SlayerQuery`
/ dict (entities auto-extracted, query persisted on the memory).
- `forget_memory(id)` — accepts int or its decimal string form.
- `recall_memories(about, max_learnings, max_queries)` — single union
arg; empty input falls back to all memories ranked by recency with a
warning. Splits results by whether the memory carries an attached
query.
Memory ids are monotonic positive ints and never reused. The
`inspect_model` Learnings section now surfaces only memories where
`query is None`; query-bearing memories appear via `recall_memories`.
Also extends the surface from MCP-only to all four: MCP, REST
(`POST /memories`, `DELETE /memories/{id}`, `POST /memories/recall`),
CLI (`slayer memory {save,forget,recall}`), and `SlayerClient`.
Storage drops the previous `Learning` / `SavedQuery` types and their
two SQLite tables / two YAML files / two seq counters in favour of a
single `Memory` row, one `memories` table + `memory_entities` index
(SQLite) or `memories.yaml` (YAML), and one `memory_seq` counter. The
branch was never deployed, so there is no migration path.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
slayer/mcp/server.py (1)
1457-1469:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFilter hidden columns out of the Learnings lookup set.
wantedis built from every column on the model, so a memory linked only to a hidden field can still appear here and expose that hidden column name viamatched_entities.Suggested fix
if "learnings" in included_set: ds = model.data_source wanted = [f"{ds}.{model.name}"] - wanted.extend(f"{ds}.{model.name}.{c.name}" for c in model.columns) + wanted.extend( + f"{ds}.{model.name}.{c.name}" for c in visible_columns + ) wanted.extend( f"{ds}.{model.name}.{m.name}" for m in model.measures if m.name is not None )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@slayer/mcp/server.py` around lines 1457 - 1469, The lookup builds wanted from every model.column and model.aggregations which can include hidden fields; change the construction of wanted (used by storage.list_memories) to skip hidden columns/aggregations by filtering model.columns and model.aggregations before extending wanted (e.g., only include c if not c.hidden and/or not getattr(c, "is_hidden", False)), so memories tied to hidden fields are not added to wanted and therefore not returned by storage.list_memories.slayer/storage/yaml_storage.py (1)
189-194:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRecover
memory_seqfrom existing rows when counters are unavailable.If
counters.yamlis lost or parses as{}whilememories.yamlstill contains rows, this restarts at1. The next save then reuses an existing id and_save_memory_row()overwrites that memory, which breaks the monotonic/non-reused id contract.Suggested fix
+ def _max_existing_memory_id(self) -> int: + max_id = 0 + for row in self._read_yaml_list(self._memories_path): + row_id = row.get("id") + if isinstance(row_id, int): + max_id = max(max_id, row_id) + return max_id + async def _next_memory_seq(self) -> int: counters = self._read_counters() - seq = counters.get("memory_seq", 0) + 1 + base = counters.get("memory_seq") + if base is None: + base = self._max_existing_memory_id() + seq = base + 1 counters["memory_seq"] = seq self._write_counters(counters) return seq🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@slayer/storage/yaml_storage.py` around lines 189 - 194, The _next_memory_seq implementation must recover the maximum existing memory id when counters.yaml is missing or empty to avoid reusing IDs; update _next_memory_seq to call the existing reader for stored memory rows (e.g., use the method or routine that reads memories.yaml / the _read_memories() helper) and compute seq = max(existing_ids, 0) + 1 when counters.get("memory_seq") is missing or zero, then persist the new counters via _write_counters(counters) before returning seq; ensure it still increments the counters.get("memory_seq") when present and handles the empty-memories case by returning 1.slayer/memories/resolver.py (1)
235-264:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOnly
countshould be accepted for wildcard entities.These branches currently treat any aggregation suffix as valid, so invalid inputs like
*:sumororders.*:avgcollapse to the model and get indexed under the wrong canonical entity.Suggested fix
if prefix == "*": - if agg is None: + if agg != "count": raise EntityResolutionError( - "'*' is not a valid entity reference; use '*:count' " + f"'{raw}' is not a valid entity reference; use '*:count' " "to refer to a model's row count." ) if source_model is None: raise EntityResolutionError( "'*:count' requires a model context: write " @@ if prefix.endswith(".*"): - if agg is None: + if agg != "count": raise EntityResolutionError( f"'{raw}' is not a valid entity reference; use the " f"'<model>.*:count' form." )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@slayer/memories/resolver.py` around lines 235 - 264, The code allows any aggregation suffix on wildcard entities, causing inputs like "*:sum" or "orders.*:avg" to be treated as valid; update the logic in the prefix == "*" branch and the prefix.endswith(".*") branch to accept only agg == "count" and raise EntityResolutionError otherwise (use the same error wording pattern), while continuing to require source_model for "*:count" and to call resolve_entity(model_part, storage=storage, source_model=source_model) for "<model>.*:count"; reference the variables prefix, agg, source_model, raw and the functions/types EntityResolution and EntityResolutionError and resolve_entity when locating the change.
🧹 Nitpick comments (5)
slayer/api/server.py (1)
80-102: 💤 Low valueConsider using Union type instead of
Anyfor stronger validation.
linked_entitiesandaboutare typed asAny, but the service layer expectsUnion[List[str], SlayerQuery, dict](perslayer/memories/service.py:39-41). While the service re-validates, usingUnion[List[Any], Dict[str, Any]]at the API boundary would provide earlier rejection of invalid shapes (e.g., a bare string or number).♻️ Proposed type refinement
class SaveMemoryRequest(BaseModel): learning: str - linked_entities: Any + linked_entities: Union[List[Any], Dict[str, Any]] class RecallMemoriesRequest(BaseModel): - about: Any + about: Union[List[Any], Dict[str, Any]] max_learnings: Optional[int] = None max_queries: Optional[int] = 2🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@slayer/api/server.py` around lines 80 - 102, Update the Pydantic request models to use a Union type instead of Any for stricter validation: change SaveMemoryRequest.linked_entities and RecallMemoriesRequest.about to accept Union[List[str], SlayerQuery, Dict[str, Any]] (or Union[List[Any], Dict[str, Any]] if importing SlayerQuery is problematic) so the API layer rejects bare strings/numbers early; update imports to include typing.Union, List, Dict and the SlayerQuery model, and run existing tests to ensure compatibility with the service functions that expect the same union shape.slayer/cli.py (1)
1067-1174: ⚖️ Poor tradeoffConsider refactoring to reduce cognitive complexity.
SonarCloud flags this function at cognitive complexity 49 (allowed: 15). The three subcommand branches (
save,forget,recall) each contain nested exception handling and conditional logic. Extracting each subcommand into a dedicated helper (e.g.,_run_memory_save,_run_memory_forget,_run_memory_recall) would improve readability and testability.♻️ Suggested structure
def _run_memory(args): """Dispatcher for ``slayer memory <save|forget|recall>``.""" sub = args.memory_command if sub == "save": _run_memory_save(args) elif sub == "forget": _run_memory_forget(args) elif sub == "recall": _run_memory_recall(args) else: print("Usage: slayer memory {save,forget,recall}") sys.exit(1) def _run_memory_save(args): # ... save logic ... def _run_memory_forget(args): # ... forget logic ... def _run_memory_recall(args): # ... recall logic ...🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@slayer/cli.py` around lines 1067 - 1174, The _run_memory function is too complex; extract each subcommand branch into its own helper to reduce cognitive complexity: create _run_memory_save(args, service), _run_memory_forget(args, service), and _run_memory_recall(args, service) that encapsulate the existing logic for the "save", "forget", and "recall" branches (including the same validation, exception handling for EntityResolutionError/AmbiguousModelError/MemoryNotFoundError/ValueError, calls to MemoryService methods, and printing of responses/warnings), keep the shared storage resolution and MemoryService instantiation in _run_memory and dispatch to these helpers based on args.memory_command, and ensure behavior, printed messages and exit codes remain identical.tests/test_memories_rest.py (1)
160-173: 💤 Low valueRemove unnecessary
asynckeyword.This test only uses synchronous
TestClientcalls and doesn'tawaitanything. SonarCloud flags this as wasteful. The same applies totest_empty_entity_list_returns_400(line 175),test_delete_missing_returns_404(line 197), andtest_recall_resolution_error_returns_400(line 260).♻️ Remove async from pure-sync tests
- async def test_resolution_error_returns_400( + def test_resolution_error_returns_400( self, client: TestClient, seeded: YAMLStorage ) -> None:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_memories_rest.py` around lines 160 - 173, The marked tests are defined as async but perform only synchronous TestClient calls; change the four test definitions from "async def" to plain "def" for test_resolution_error_returns_400, test_empty_entity_list_returns_400, test_delete_missing_returns_404, and test_recall_resolution_error_returns_400 (and remove any now-unused asyncio/async fixtures if present); ensure none of these functions contain await expressions before making this change.tests/test_entity_resolution.py (1)
223-223: ⚡ Quick winUse keyword arguments consistently for these multi-parameter helper calls.
The repo style here is
resolve_entity(raw="other", storage=storage)/extract_entities_from_query(query=q, storage=storage). Applying that consistently across this module will make the tests match the project convention.Example cleanup
- result = await resolve_entity("other", storage=storage) + result = await resolve_entity(raw="other", storage=storage) - result = await extract_entities_from_query(q, storage=storage) + result = await extract_entities_from_query(query=q, storage=storage)As per coding guidelines,
**/*.py: Use keyword arguments for functions with more than 1 parameter.Also applies to: 506-506
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_entity_resolution.py` at line 223, Tests call multi-parameter helpers positionally (e.g., resolve_entity("other", storage=storage)); update these to use keyword arguments consistently per project style—replace positional calls to resolve_entity and extract_entities_from_query with resolve_entity(raw="...", storage=storage) and extract_entities_from_query(query=..., storage=storage) (and any similar multi-arg helper calls around lines referenced) so all calls use explicit keywords for parameters.tests/test_memories_storage.py (1)
240-265: 🏗️ Heavy lift
deldoesn’t make this a strict reopen-persistence test.Re-instantiating the backend in the same interpreter can still pass if the sequence counter is cached in class/module state, so this doesn’t fully pin the “persisted across reopen” contract. Consider asserting the persisted counter directly in the YAML/SQLite backing store, or moving the second save into a fresh-process helper.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_memories_storage.py` around lines 240 - 265, The test currently uses del to "reopen" YAMLStorage and SQLiteStorage but may pass if sequence counters are cached in memory; update the test to verify persistence by reading the backend's stored counter/state directly or by doing the second save from a fresh process. Concretely, after creating ys and calling save_memory twice, either open the YAML backing file (via YAMLStorage.load or directly inspect the YAML files) to assert the next id is 3, or spawn a subprocess that instantiates YAMLStorage and calls save_memory to ensure the returned Memory.id == 3; do the analogous change for SQLiteStorage by querying the SQLite sequence/table directly (or using a fresh-process helper that constructs SQLiteStorage and calls save_memory) so the test proves on-disk persistence rather than in-process caching.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@slayer/mcp/server.py`:
- Around line 1599-1610: The JSON output for learnings is using the wrong
attribute (lr.body) causing a failure when consumers expect the Memory.learning
field; update the comprehension that builds payload["learnings"] (the block
iterating relevant_learnings) to use lr.learning instead of lr.body while
keeping the existing id and matched_entities logic (i.e., keep lr.id and the set
intersection with lr.entities) so inspect_model(format="json",
sections=["learnings"]) returns the correct field.
In `@slayer/memories/service.py`:
- Around line 179-185: The recall_memories method currently allows negative caps
which get passed into slice operations; add validation at the start of
recall_memories to reject negative max_learnings and max_queries (e.g., if
max_learnings is not None and max_learnings < 0: raise ValueError(...); same for
max_queries) so invalid inputs fail fast; make the same change to the other
method in this module that accepts max_learnings/max_queries (the nearby
parallel retrieval/query function) to ensure consistent validation across both
code paths.
In `@slayer/storage/sqlite_storage.py`:
- Around line 225-240: The _next_seq_sync helper for counter "memory_seq"
assumes the id_counters row exists; change it so that after the INSERT OR IGNORE
and before incrementing you detect if the counter row was newly created (or if
UPDATE returned no row) and, in that case, seed last_value to MAX(id) from the
memories table (or 0 if no rows) before performing the increment; update the
logic in _next_seq_sync (and ensure _next_memory_seq still calls it) so the
returned value is based on last_value seeded from SELECT COALESCE(MAX(id), 0)
FROM memories when the id_counters.memory_seq row was missing to avoid reusing
existing memory IDs in _save_memory_sync.
In `@specs/DEV-1357-memories.md`:
- Around line 225-229: The fenced code block containing "<storage_root>/" and
the two lines "memories.yaml # list of Memory dicts" and "counters.yaml
# { memory_seq: 42 }" needs a language tag to satisfy markdown linting (MD040);
update that fence to include a language identifier such as "text" or "yaml"
(e.g., change ``` to ```text) so the block is explicitly typed—locate the fence
around the "<storage_root>/" snippet in specs/DEV-1357-memories.md and add the
chosen language tag.
---
Duplicate comments:
In `@slayer/mcp/server.py`:
- Around line 1457-1469: The lookup builds wanted from every model.column and
model.aggregations which can include hidden fields; change the construction of
wanted (used by storage.list_memories) to skip hidden columns/aggregations by
filtering model.columns and model.aggregations before extending wanted (e.g.,
only include c if not c.hidden and/or not getattr(c, "is_hidden", False)), so
memories tied to hidden fields are not added to wanted and therefore not
returned by storage.list_memories.
In `@slayer/memories/resolver.py`:
- Around line 235-264: The code allows any aggregation suffix on wildcard
entities, causing inputs like "*:sum" or "orders.*:avg" to be treated as valid;
update the logic in the prefix == "*" branch and the prefix.endswith(".*")
branch to accept only agg == "count" and raise EntityResolutionError otherwise
(use the same error wording pattern), while continuing to require source_model
for "*:count" and to call resolve_entity(model_part, storage=storage,
source_model=source_model) for "<model>.*:count"; reference the variables
prefix, agg, source_model, raw and the functions/types EntityResolution and
EntityResolutionError and resolve_entity when locating the change.
In `@slayer/storage/yaml_storage.py`:
- Around line 189-194: The _next_memory_seq implementation must recover the
maximum existing memory id when counters.yaml is missing or empty to avoid
reusing IDs; update _next_memory_seq to call the existing reader for stored
memory rows (e.g., use the method or routine that reads memories.yaml / the
_read_memories() helper) and compute seq = max(existing_ids, 0) + 1 when
counters.get("memory_seq") is missing or zero, then persist the new counters via
_write_counters(counters) before returning seq; ensure it still increments the
counters.get("memory_seq") when present and handles the empty-memories case by
returning 1.
---
Nitpick comments:
In `@slayer/api/server.py`:
- Around line 80-102: Update the Pydantic request models to use a Union type
instead of Any for stricter validation: change SaveMemoryRequest.linked_entities
and RecallMemoriesRequest.about to accept Union[List[str], SlayerQuery,
Dict[str, Any]] (or Union[List[Any], Dict[str, Any]] if importing SlayerQuery is
problematic) so the API layer rejects bare strings/numbers early; update imports
to include typing.Union, List, Dict and the SlayerQuery model, and run existing
tests to ensure compatibility with the service functions that expect the same
union shape.
In `@slayer/cli.py`:
- Around line 1067-1174: The _run_memory function is too complex; extract each
subcommand branch into its own helper to reduce cognitive complexity: create
_run_memory_save(args, service), _run_memory_forget(args, service), and
_run_memory_recall(args, service) that encapsulate the existing logic for the
"save", "forget", and "recall" branches (including the same validation,
exception handling for
EntityResolutionError/AmbiguousModelError/MemoryNotFoundError/ValueError, calls
to MemoryService methods, and printing of responses/warnings), keep the shared
storage resolution and MemoryService instantiation in _run_memory and dispatch
to these helpers based on args.memory_command, and ensure behavior, printed
messages and exit codes remain identical.
In `@tests/test_entity_resolution.py`:
- Line 223: Tests call multi-parameter helpers positionally (e.g.,
resolve_entity("other", storage=storage)); update these to use keyword arguments
consistently per project style—replace positional calls to resolve_entity and
extract_entities_from_query with resolve_entity(raw="...", storage=storage) and
extract_entities_from_query(query=..., storage=storage) (and any similar
multi-arg helper calls around lines referenced) so all calls use explicit
keywords for parameters.
In `@tests/test_memories_rest.py`:
- Around line 160-173: The marked tests are defined as async but perform only
synchronous TestClient calls; change the four test definitions from "async def"
to plain "def" for test_resolution_error_returns_400,
test_empty_entity_list_returns_400, test_delete_missing_returns_404, and
test_recall_resolution_error_returns_400 (and remove any now-unused
asyncio/async fixtures if present); ensure none of these functions contain await
expressions before making this change.
In `@tests/test_memories_storage.py`:
- Around line 240-265: The test currently uses del to "reopen" YAMLStorage and
SQLiteStorage but may pass if sequence counters are cached in memory; update the
test to verify persistence by reading the backend's stored counter/state
directly or by doing the second save from a fresh process. Concretely, after
creating ys and calling save_memory twice, either open the YAML backing file
(via YAMLStorage.load or directly inspect the YAML files) to assert the next id
is 3, or spawn a subprocess that instantiates YAMLStorage and calls save_memory
to ensure the returned Memory.id == 3; do the analogous change for SQLiteStorage
by querying the SQLite sequence/table directly (or using a fresh-process helper
that constructs SQLiteStorage and calls save_memory) so the test proves on-disk
persistence rather than in-process caching.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8f95afa1-baa3-4875-8a34-22afe52b28ce
📒 Files selected for processing (26)
.claude/skills/slayer-overview.mdCLAUDE.mddocs/concepts/memories.mddocs/configuration/storage.mdslayer/api/server.pyslayer/cli.pyslayer/client/slayer_client.pyslayer/core/errors.pyslayer/mcp/server.pyslayer/memories/__init__.pyslayer/memories/models.pyslayer/memories/resolver.pyslayer/memories/service.pyslayer/storage/base.pyslayer/storage/join_sync.pyslayer/storage/migrations.pyslayer/storage/sqlite_storage.pyslayer/storage/yaml_storage.pyspecs/DEV-1357-memories.mdtests/test_entity_resolution.pytests/test_memories_cli.pytests/test_memories_client.pytests/test_memories_inspect.pytests/test_memories_mcp.pytests/test_memories_rest.pytests/test_memories_storage.py
- Update tests/integration/test_mcp_inspect.py — the section-gating test was asserting the pre-DEV-1357 form of the omitted-sections footer (`reachable_fields, samples`). After learnings joined the omittable list the actual footer is `reachable_fields, samples, learnings`. Stale assertion from the v1 PR. - NOSONAR(S3776) on slayer/memories/resolver.py for resolve_entity and extract_entities_from_query — pre-existing complexity copied verbatim from the deleted slayer/learnings/resolver.py during the Memory rewrite. Refactor is out of scope for this PR. - NOSONAR(S7503) on the four test_memories_rest.py tests that don't await internally — keeps the class's async signature uniform so the seeded async fixture binds the same way for every test. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Persistence correctness (CodeRabbit threads 1, 3, 10): - resolver.py: reject ``*:sum`` / ``<model>.*:avg`` etc. — only ``count`` is a valid wildcard aggregation. Previously any aggregation silently collapsed to the model and stored under the wrong canonical entity. - yaml_storage.py: when ``counters.yaml`` is wiped but ``memories.yaml`` still has rows, seed ``memory_seq`` from ``MAX(id)`` instead of restarting at 1 and overwriting memory 1. - sqlite_storage.py: same fix on the SQLite side — when the ``id_counters.memory_seq`` row is missing but ``memories`` has rows, seed ``last_value`` from ``MAX(id)`` before the increment. Memory surface bugs (CodeRabbit threads 8, 9): - mcp/server.py: ``inspect_model(format="json", sections=["learnings"])`` was reading ``lr.body`` — the unified ``Memory`` carries ``learning``, so the read AttributeError'd as soon as a memory matched. Use ``memory.learning``. - service.py: reject negative ``max_learnings`` / ``max_queries`` in ``recall_memories`` (previously slipped into Python's negative-index slicing). Test tightening (CodeRabbit threads 4, 5, 7): - test_star_count_uses_source_model: pin exact list match (also closes Sonar S4144 duplicate-test). - test_resolve_inspect_sections_empty: pin the full ordered list. - test_section_pruned_when_no_relevant_memories + test_section_excluded_when_not_in_sections: assert the ``## Learnings`` header is absent. CLI cognitive complexity (Sonar S3776 cli.py): - Refactor ``_run_memory`` into a dispatch table over ``_run_memory_save`` / ``_run_memory_forget`` / ``_run_memory_recall`` with shared ``_print_recall_section`` and ``_exit_with_error`` helpers. Behaviour unchanged. Cosmetic (CodeRabbit thread 11): - specs/DEV-1357-memories.md: add ``text`` language tag to the YAMLStorage-layout fence (markdownlint MD040). Regression tests added for every behaviour change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
slayer/mcp/server.py (1)
1456-1468:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHidden columns still included in learnings entity list.
The
wantedlist at line 1460 iterates over allmodel.columnswithout filtering hidden columns. This means learnings linked to hidden fields can surface ininspect_model, andmatched_entitiesmay expose hidden column names.A previous review flagged this issue. The fix should filter to visible columns only.
Suggested fix
wanted = [f"{ds}.{model.name}"] - wanted.extend(f"{ds}.{model.name}.{c.name}" for c in model.columns) + wanted.extend( + f"{ds}.{model.name}.{c.name}" + for c in model.columns + if not c.hidden + ) wanted.extend( f"{ds}.{model.name}.{m.name}" for m in model.measures🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@slayer/mcp/server.py` around lines 1456 - 1468, The wanted list currently includes every item in model.columns; update the construction in inspect_model (where wanted is built) to skip hidden columns by filtering model.columns to only visible ones—for example replace iterations over model.columns with a filtered list like [c for c in model.columns if not getattr(c, "hidden", False)] (or use c.is_hidden if that is the attribute) so hidden columns are not added to wanted or exposed in matched_entities.
🧹 Nitpick comments (1)
tests/test_memories_mcp.py (1)
591-612: 💤 Low valueTest name and comments don't match the actual test behavior.
The test is named
test_query_with_no_entities_returns_all_with_warningand the docstring mentions "A query that doesn't reference anything resolvable" and "bare query with no source_model", but line 608 passes"about": []— an empty list, not a query.This makes the test functionally identical to
test_empty_about_returns_all_with_warning(lines 571-589). Consider either:
- Renaming this test to reflect what it actually tests, or
- Replacing the
[]with an actual query that extracts zero entities (e.g.,{"source_model": "nonexistent_model", "measures": [...]}) to test the distinct fallback path for queries that resolve to no entities.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_memories_mcp.py` around lines 591 - 612, The test named test_query_with_no_entities_returns_all_with_warning currently passes an empty "about": [] (duplicating test_empty_about_returns_all_with_warning); update this test to match its intent by either renaming it to test_empty_about_returns_all_with_warning_duplicate to avoid duplication, or change the arguments in the _call within test_query_with_no_entities_returns_all_with_warning to submit a payload that represents a query which extracts zero entities (for example include a non-existent source_model and measures so extraction returns no entities) so the distinct "query resolves to no entities" recency-fallback path is exercised; locate the test function by name and modify the _call arguments accordingly or rename the test to reflect the current behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@slayer/mcp/server.py`:
- Around line 1456-1468: The wanted list currently includes every item in
model.columns; update the construction in inspect_model (where wanted is built)
to skip hidden columns by filtering model.columns to only visible ones—for
example replace iterations over model.columns with a filtered list like [c for c
in model.columns if not getattr(c, "hidden", False)] (or use c.is_hidden if that
is the attribute) so hidden columns are not added to wanted or exposed in
matched_entities.
---
Nitpick comments:
In `@tests/test_memories_mcp.py`:
- Around line 591-612: The test named
test_query_with_no_entities_returns_all_with_warning currently passes an empty
"about": [] (duplicating test_empty_about_returns_all_with_warning); update this
test to match its intent by either renaming it to
test_empty_about_returns_all_with_warning_duplicate to avoid duplication, or
change the arguments in the _call within
test_query_with_no_entities_returns_all_with_warning to submit a payload that
represents a query which extracts zero entities (for example include a
non-existent source_model and measures so extraction returns no entities) so the
distinct "query resolves to no entities" recency-fallback path is exercised;
locate the test function by name and modify the _call arguments accordingly or
rename the test to reflect the current behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 50389f03-ede5-486e-8a00-97dd273f4620
📒 Files selected for processing (12)
slayer/cli.pyslayer/mcp/server.pyslayer/memories/resolver.pyslayer/memories/service.pyslayer/storage/sqlite_storage.pyslayer/storage/yaml_storage.pyspecs/DEV-1357-memories.mdtests/test_entity_resolution.pytests/test_mcp_server.pytests/test_memories_inspect.pytests/test_memories_mcp.pytests/test_memories_storage.py
`test_query_with_no_entities_returns_all_with_warning` claimed to exercise "a query that doesn't reference anything resolvable" but actually passed `about=[]` — identical to `test_empty_about_returns_all_with_warning` directly above it. The empty-list path is fully covered by the surviving test, and a query that genuinely extracts zero entities is awkward to construct (the resolver raises on unknown source_models rather than silently yielding nothing), so dropping the misleading duplicate is the cleanest fix. Addresses CodeRabbit nitpick on PR #100. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_memories_mcp.py (1)
40-79: ⚡ Quick winPrefer function-scoped storage/server fixtures for stronger isolation
Using session-scoped
_shared_storage/_shared_mcp_serverplus file-level cleanup can become order-dependent if storage/server internals keep in-memory state (e.g., counters/cache). A freshYAMLStorage+create_mcp_server(...)per test is more robust and simplifies setup.♻️ Suggested fixture simplification
-@pytest.fixture(scope="session") -def _shared_storage() -> Generator[YAMLStorage, None, None]: - with tempfile.TemporaryDirectory() as tmpdir: - yield YAMLStorage(base_dir=tmpdir) - - -@pytest.fixture(scope="session") -def _shared_mcp_server(_shared_storage: YAMLStorage): - return create_mcp_server(storage=_shared_storage) - - -def _reset_storage(storage: YAMLStorage) -> None: - ... - - -@pytest.fixture -def storage(_shared_storage: YAMLStorage) -> YAMLStorage: - _reset_storage(_shared_storage) - return _shared_storage - - -@pytest.fixture -def mcp_server(_shared_mcp_server, storage: YAMLStorage): - return _shared_mcp_server +@pytest.fixture +def storage() -> Generator[YAMLStorage, None, None]: + with tempfile.TemporaryDirectory() as tmpdir: + yield YAMLStorage(base_dir=tmpdir) + + +@pytest.fixture +def mcp_server(storage: YAMLStorage): + return create_mcp_server(storage=storage)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_memories_mcp.py` around lines 40 - 79, The current session-scoped fixtures _shared_storage and _shared_mcp_server introduce shared state risk; change them to function-scoped by removing scope="session" so each test gets a fresh YAMLStorage and MCP server, and eliminate the manual _reset_storage dance: make a storage fixture that constructs and yields YAMLStorage(base_dir=tempdir) per test and a mcp_server fixture that calls create_mcp_server(storage=storage) per test; update/remove _shared_storage, _shared_mcp_server, and _reset_storage references and ensure fixture names storage and mcp_server return new instances for each test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/test_memories_mcp.py`:
- Around line 40-79: The current session-scoped fixtures _shared_storage and
_shared_mcp_server introduce shared state risk; change them to function-scoped
by removing scope="session" so each test gets a fresh YAMLStorage and MCP
server, and eliminate the manual _reset_storage dance: make a storage fixture
that constructs and yields YAMLStorage(base_dir=tempdir) per test and a
mcp_server fixture that calls create_mcp_server(storage=storage) per test;
update/remove _shared_storage, _shared_mcp_server, and _reset_storage references
and ensure fixture names storage and mcp_server return new instances for each
test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 933778b4-b84a-477d-934e-1be164f75c3e
📒 Files selected for processing (1)
tests/test_memories_mcp.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_memories_mcp.py`:
- Around line 61-68: The test reset helper currently deletes "memories.yaml"
under storage.base_dir but misses the split memory-layer files used by this PR,
causing test state leakage; update the same cleanup loop in the
_reset_storage/test helper (the block iterating over filenames that references
storage.base_dir and removes files) to also check for and remove
"learnings.yaml" and "saved_queries.yaml" if they exist so each test starts with
a clean memory state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9d206f97-1f5c-4c03-845b-5746d91572d5
📒 Files selected for processing (1)
tests/test_memories_mcp.py
Comment on lines
+61
to
+68
| for f in ( | ||
| "priority.yaml", | ||
| "memories.yaml", | ||
| "counters.yaml", | ||
| ): | ||
| p = os.path.join(storage.base_dir, f) | ||
| if os.path.exists(p): | ||
| os.remove(p) |
Contributor
There was a problem hiding this comment.
Reset helper misses memory-layer YAML files, which can leak state across tests.
_reset_storage removes memories.yaml, but this PR’s storage layout uses split memory files (learnings.yaml / saved_queries.yaml). If those files are present, test isolation breaks (e.g., ID and list assertions become order-dependent).
Suggested fix
for f in (
"priority.yaml",
- "memories.yaml",
+ # Keep both legacy + current names to make reset robust.
+ "memories.yaml",
+ "learnings.yaml",
+ "saved_queries.yaml",
"counters.yaml",
):
p = os.path.join(storage.base_dir, f)
if os.path.exists(p):
os.remove(p)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_memories_mcp.py` around lines 61 - 68, The test reset helper
currently deletes "memories.yaml" under storage.base_dir but misses the split
memory-layer files used by this PR, causing test state leakage; update the same
cleanup loop in the _reset_storage/test helper (the block iterating over
filenames that references storage.base_dir and removes files) to also check for
and remove "learnings.yaml" and "saved_queries.yaml" if they exist so each test
starts with a clean memory state.
This was referenced May 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
save_learning) and example queries (save_query) against canonical SLayer entities, then look them up viarecallbefore drafting a new query.<ds>/<ds>.<model>/<ds>.<model>.<leaf>) — aggregation suffixes stripped,*:countcollapses to the model, multi-hop join paths keep only the leaf.inspect_modelauto-renders a Learnings section (auto-pruned when empty); thequerytool docstring directs agents to callrecallfirst.Implementation notes
L<int>/Q<int>ID format, monotonic non-reuse, entity-intersection filter) lives on theStorageBackendABC — backends only implement the row-shaped CRUD + seq counter, per the standing backend-agnostic rule.learnings,learning_entities(with index),saved_queries,saved_query_entities(with index),id_counters. New YAML files:learnings.yaml,saved_queries.yaml,counters.yaml.slayer/learnings/resolver.py; service inslayer/learnings/service.py. Four MCP tools registered inslayer/mcp/server.py.Test plan
*:countwith/without source-model context;extract_entities_from_querywalks dimensions / time_dimensions / measures / filters / source_model)max_queries=2,max_learnings=Nonereturns all)inspect_model+querydocstring integration testspoetry run pytest -m "not integration"→ 1955 passedpoetry run ruff check slayer/ tests/Docs
docs/concepts/learnings.mdcovering the canonical entity form, the four tools, and the recommended agent workflow.CLAUDE.mdKey Conventions entry;.claude/skills/slayer-overview.mdworkflow note;docs/configuration/storage.mdupdated for new YAML files + SQLite tables.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Improvements
Tests