feat: add search_notes tool for memory retrieval#112
Open
jjacke13 wants to merge 3 commits intomemovai:mainfrom
Open
feat: add search_notes tool for memory retrieval#112jjacke13 wants to merge 3 commits intomemovai:mainfrom
jjacke13 wants to merge 3 commits intomemovai:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds a new "search_notes" tool: implementation and header, registers it in the tool registry, includes the source in CMake, and updates the agent prompt. The tool parses JSON input, searches Changes
Sequence DiagramsequenceDiagram
participant Caller as Tool Caller
participant Exec as tool_search_notes_execute()
participant JSON as JSON Parser
participant FS as SPIFFS (memory/)
participant Search as Matcher
participant Formatter as Output Formatter
Note over Caller,Exec: New search_notes workflow
Caller->>Exec: input_json, output buffer
Exec->>JSON: parse input_json
JSON-->>Exec: query string
Exec->>Exec: split & dedupe query words
Exec->>FS: open memory/ directory
loop for each `.md` file
FS-->>Exec: file path
Exec->>FS: read file (bounded)
FS-->>Exec: file content
Exec->>Search: check each query word (case-insensitive)
Search-->>Exec: distinct-word hit count
Exec->>Exec: record/replace results (up to MAX_RESULTS)
end
Exec->>Exec: sort results by hit count desc
Exec->>Formatter: build human-readable output
Formatter-->>Exec: formatted string
Exec-->>Caller: write output buffer, return esp_err_t
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can scan for known vulnerabilities in your dependencies using OSV Scanner.OSV Scanner will automatically detect and report security vulnerabilities in your project's dependencies. No additional configuration is required. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
main/tools/tool_registry.c (1)
141-143: Align schema with runtime validation (querynon-empty).Runtime rejects empty query, but the schema currently allows
"". AddminLength: 1so invalid calls can fail earlier at schema level.Schema tweak
- "\"properties\":{\"query\":{\"type\":\"string\",\"description\":\"Space-separated keywords to search for in daily notes\"}}," + "\"properties\":{\"query\":{\"type\":\"string\",\"minLength\":1,\"description\":\"Space-separated keywords to search for in daily notes\"}},"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main/tools/tool_registry.c` around lines 141 - 143, The JSON schema for the tool's input currently allows an empty "query" string but runtime validation rejects empty queries; update the schema string that defines the "properties"->"query" (the JSON literal built in tool_registry.c) to include "minLength":1 for the "query" property so the schema enforces non-empty queries at validation time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@main/tools/tool_search_notes.c`:
- Around line 153-154: The logs in search_notes are printing the raw
user-provided query (ESP_LOGI(TAG, "search_notes: 0 matches for \"%s\"", query)
and the similar log at the later occurrence) which can expose sensitive data;
update the logging calls in the search_notes flow (references: function
search_notes, variable query, logger macro ESP_LOGI and TAG) to either omit the
query or replace it with a redacted placeholder (e.g., "[REDACTED]" or a
boolean/length indicator) before logging, ensuring you still convey useful
status (e.g., "0 matches for query" or "0 matches for query (redacted)") while
never writing the raw query to logs.
- Around line 66-70: The tokenization currently adds duplicate tokens from qbuf
into words (variables: tok, qbuf, words, word_count, MAX_QUERY_WORDS), which
inflates scoring later (see the scoring loop around lines 123-126). After
splitting tokens, filter out duplicates before incrementing word_count — e.g.,
for each tok check existing entries in words with strcmp and only add if not
already present — so words contains unique query terms; then ensure the scoring
code uses this deduplicated word_count (the distinct-word denominator) when
computing per-file scores.
- Around line 158-166: The current formatted appends to output using off and
output_size can underflow when off >= output_size; change both snprintf
invocations to first compute remaining = (off < output_size) ? output_size - off
: 0 and either break out of the loop (for the per-result loop) or skip the
snprintf when remaining == 0 so you never call snprintf with a huge size_t;
after each successful snprintf clamp off = (off + written > output_size) ?
output_size : off + written (or simply stop when off >= output_size - 1). Apply
this to the initial header snprintf and the per-item snprintf that references
results[i].path, result_count, matches, and word_count.
- Line 90: The loop currently stops scanning when result_count reaches
MAX_RESULTS; remove the result_count < MAX_RESULTS clause from the while
condition so the readdir(dir) loop scans all directory entries (i.e., use while
((ent = readdir(dir)) != NULL) ), and instead collect all candidate matches
(using the existing result structure tied to result_count or a temporary dynamic
array) while computing relevance scores for each match; after the directory scan
completes, sort the full list by relevance and then truncate or copy the top
MAX_RESULTS into the final results buffer (ensuring to update result_count),
preserving existing identifiers like ent, readdir, result_count and MAX_RESULTS.
---
Nitpick comments:
In `@main/tools/tool_registry.c`:
- Around line 141-143: The JSON schema for the tool's input currently allows an
empty "query" string but runtime validation rejects empty queries; update the
schema string that defines the "properties"->"query" (the JSON literal built in
tool_registry.c) to include "minLength":1 for the "query" property so the schema
enforces non-empty queries at validation time.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
main/CMakeLists.txtmain/tools/tool_registry.cmain/tools/tool_search_notes.cmain/tools/tool_search_notes.h
Comment on lines
+153
to
+154
| ESP_LOGI(TAG, "search_notes: 0 matches for \"%s\"", query); | ||
| cJSON_Delete(root); |
There was a problem hiding this comment.
Avoid logging raw query text.
Lines 153 and 169 log user-provided query strings directly. These may contain personal/sensitive data and should be redacted or omitted from logs.
Minimal log-safety change
- ESP_LOGI(TAG, "search_notes: 0 matches for \"%s\"", query);
+ ESP_LOGI(TAG, "search_notes: 0 matches");
...
- ESP_LOGI(TAG, "search_notes: %d matches for \"%s\"", result_count, query);
+ ESP_LOGI(TAG, "search_notes: %d matches", result_count);Also applies to: 169-169
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@main/tools/tool_search_notes.c` around lines 153 - 154, The logs in
search_notes are printing the raw user-provided query (ESP_LOGI(TAG,
"search_notes: 0 matches for \"%s\"", query) and the similar log at the later
occurrence) which can expose sensitive data; update the logging calls in the
search_notes flow (references: function search_notes, variable query, logger
macro ESP_LOGI and TAG) to either omit the query or replace it with a redacted
placeholder (e.g., "[REDACTED]" or a boolean/length indicator) before logging,
ensuring you still convey useful status (e.g., "0 matches for query" or "0
matches for query (redacted)") while never writing the raw query to logs.
Author
|
Thanks for the review! Addressed 3 of the 4 findings in fb0bd6e:
Re: logging raw query text — leaving as-is. This is a personal device with serial-only logs, no remote logging or multi-user scenario. The existing tools follow the same pattern (e.g. |
Member
|
Thanks for your contribution, how about add a memory search cli command like skill search? Command: skill_search <keyword>
What it does:
- Scans all skills/*.md files on SPIFFS
- Case-insensitive match against filename or any line in file content
- Reports each match with its full path and where it matched (filename or line number) |
Author
Is this needed? I cannot see the main use case for searching manually in the skills. |
Adds a new `search_notes` tool that searches /spiffs/memory/*.md files for space-separated keywords, ranks results by distinct word matches, and returns file paths. The agent can then use `read_file` to retrieve full content of matching notes. This gives the agent memory retrieval capability — it can already write daily notes via `write_file`, but had no way to search them. For best results, pair with a skill that teaches the agent when and how to search its notes (e.g. before answering questions that may relate to past conversations). Co-Authored-By: Claude <noreply@anthropic.com>
- Deduplicate query tokens (case-insensitive) so repeated words don't inflate scoring - Scan all directory entries instead of stopping at MAX_RESULTS; when the results array is full, replace the weakest entry if the new one scores higher - Guard snprintf append loop against size_t underflow when output buffer is full Co-Authored-By: Claude <noreply@anthropic.com>
Help the LLM to know when to use the tool proactively. Co-Authored-By: Claude <noreply@anthropic.com>
jjacke13
force-pushed
the
feat/search-notes-tool
branch
from
be83884 to
52e2f75
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
main/tools/tool_search_notes.c (1)
169-193:⚠️ Potential issue | 🟠 MajorStop logging the raw search query.
These searches are against personal memory, so the query string can contain exactly the sensitive data this tool is meant to retrieve. Keep the returned tool text if you want, but redact or omit the query from
ESP_LOGI.Minimal patch
- ESP_LOGI(TAG, "search_notes: 0 matches for \"%s\"", query); + ESP_LOGI(TAG, "search_notes: 0 matches"); ... - ESP_LOGI(TAG, "search_notes: %d matches for \"%s\"", result_count, query); + ESP_LOGI(TAG, "search_notes: %d matches", result_count);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main/tools/tool_search_notes.c` around lines 169 - 193, Remove the raw query from logs in the search routine: update the two ESP_LOGI calls that currently use the format "search_notes: ... \"%s\"" with the query variable so they no longer log query contents (e.g., change to ESP_LOGI(TAG, "search_notes: %d matches", result_count) or similar), ensuring you modify both occurrences that reference query; leave the returned output text and behavior (snprintf/output construction, cJSON_Delete, loop over results, variables like result_count, word_count, results, output/output_size) unchanged.
🧹 Nitpick comments (1)
main/tools/tool_search_notes.c (1)
99-105: Avoid baking the memory layout into string literals.This duplicates both
memory/andMEMORY.mdeven though the rest of the codebase already treats the memory location as configuration. If that layout changes,search_noteswill silently search the wrong files or skip the wrong one.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main/tools/tool_search_notes.c` around lines 99 - 105, The code in tool_search_notes.c hardcodes "memory/" and "MEMORY.md" when checking the archive entry name (variable name), which duplicates configuration and will break if the memory location/name changes; replace those string literals with the canonical configuration values used elsewhere (e.g., use the existing memory directory variable or getter like memory_dir or get_memory_dir() for the prefix check and use the configured memory index filename constant or accessor like MEMORY_INDEX_NAME or get_memory_index_name() for the skip check), compute the prefix length from that configured directory when doing strncmp/strlen/strcmp against name, and ensure path comparisons correctly handle trailing slashes or separators the configuration may include.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@main/tools/tool_search_notes.c`:
- Around line 59-75: The code silently truncates queries into qbuf and drops
tokens beyond MAX_QUERY_WORDS; update the parser in tool_search_notes.c (around
qbuf, strncpy, strtok, word_count) to detect these conditions and return
ESP_ERR_INVALID_ARG (or otherwise surface the applied limits) instead of
silently truncating: check if strlen(query) >= sizeof(qbuf) before copying and
return ESP_ERR_INVALID_ARG if true, and while tokenizing count total tokens
(separate from deduplicated word_count) and if total tokens exceed
MAX_QUERY_WORDS return ESP_ERR_INVALID_ARG; ensure the error is returned from
the function where qbuf/word_count are used so callers see the failure.
- Around line 112-123: The current code only reads up to READ_CAP into buf once
(using fread) so hits past the first 4 KiB are ignored; change the single fread
into a chunked read loop that reuses the allocated buffer and repeatedly
fread()s up to READ_CAP bytes from FILE *f (full_path) until EOF or until all
query terms are found, checking each chunk for the query terms as you go and
exiting early when every term has been found; ensure you fclose(f) and free(buf)
on all paths, keep buf null-terminated for string searches (buf[n]=0), and
preserve a small overlap between chunks (carry the last N bytes, where N is at
least the longest query term length minus 1) to avoid missing matches spanning
chunk boundaries.
---
Duplicate comments:
In `@main/tools/tool_search_notes.c`:
- Around line 169-193: Remove the raw query from logs in the search routine:
update the two ESP_LOGI calls that currently use the format "search_notes: ...
\"%s\"" with the query variable so they no longer log query contents (e.g.,
change to ESP_LOGI(TAG, "search_notes: %d matches", result_count) or similar),
ensuring you modify both occurrences that reference query; leave the returned
output text and behavior (snprintf/output construction, cJSON_Delete, loop over
results, variables like result_count, word_count, results, output/output_size)
unchanged.
---
Nitpick comments:
In `@main/tools/tool_search_notes.c`:
- Around line 99-105: The code in tool_search_notes.c hardcodes "memory/" and
"MEMORY.md" when checking the archive entry name (variable name), which
duplicates configuration and will break if the memory location/name changes;
replace those string literals with the canonical configuration values used
elsewhere (e.g., use the existing memory directory variable or getter like
memory_dir or get_memory_dir() for the prefix check and use the configured
memory index filename constant or accessor like MEMORY_INDEX_NAME or
get_memory_index_name() for the skip check), compute the prefix length from that
configured directory when doing strncmp/strlen/strcmp against name, and ensure
path comparisons correctly handle trailing slashes or separators the
configuration may include.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c5fdb4dc-84da-4eff-97bd-24c818b78259
📒 Files selected for processing (5)
main/CMakeLists.txtmain/agent/context_builder.cmain/tools/tool_registry.cmain/tools/tool_search_notes.cmain/tools/tool_search_notes.h
🚧 Files skipped from review as they are similar to previous changes (1)
- main/tools/tool_registry.c
Comment on lines
+59
to
+75
| /* Split query into words */ | ||
| char qbuf[256]; | ||
| strncpy(qbuf, query, sizeof(qbuf) - 1); | ||
| qbuf[sizeof(qbuf) - 1] = '\0'; | ||
|
|
||
| char *words[MAX_QUERY_WORDS]; | ||
| int word_count = 0; | ||
| char *tok = strtok(qbuf, " "); | ||
| while (tok && word_count < MAX_QUERY_WORDS) { | ||
| /* Deduplicate: skip if this word was already added */ | ||
| bool dup = false; | ||
| for (int i = 0; i < word_count; i++) { | ||
| if (strcasecmp(words[i], tok) == 0) { dup = true; break; } | ||
| } | ||
| if (!dup) words[word_count++] = tok; | ||
| tok = strtok(NULL, " "); | ||
| } |
There was a problem hiding this comment.
Reject queries that exceed the local limits.
The parser silently truncates at 255 bytes and silently ignores unique tokens after MAX_QUERY_WORDS. The tool then echoes the original query back in the response, so the reported ranking can disagree with what was actually searched. Return ESP_ERR_INVALID_ARG once either limit is exceeded, or surface the applied limits explicitly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@main/tools/tool_search_notes.c` around lines 59 - 75, The code silently
truncates queries into qbuf and drops tokens beyond MAX_QUERY_WORDS; update the
parser in tool_search_notes.c (around qbuf, strncpy, strtok, word_count) to
detect these conditions and return ESP_ERR_INVALID_ARG (or otherwise surface the
applied limits) instead of silently truncating: check if strlen(query) >=
sizeof(qbuf) before copying and return ESP_ERR_INVALID_ARG if true, and while
tokenizing count total tokens (separate from deduplicated word_count) and if
total tokens exceed MAX_QUERY_WORDS return ESP_ERR_INVALID_ARG; ensure the error
is returned from the function where qbuf/word_count are used so callers see the
failure.
Comment on lines
+112
to
+123
| /* Read file content (up to READ_CAP bytes) */ | ||
| FILE *f = fopen(full_path, "r"); | ||
| if (!f) continue; | ||
|
|
||
| char *buf = malloc(READ_CAP + 1); | ||
| if (!buf) { | ||
| fclose(f); | ||
| continue; | ||
| } | ||
|
|
||
| size_t n = fread(buf, 1, READ_CAP, f); | ||
| buf[n] = '\0'; |
There was a problem hiding this comment.
Only the first 4 KiB of each note is searchable.
fread(..., READ_CAP, ...) stops at 4096 bytes, so hits later in a long note are never counted. main/agent/context_builder.c already instructs the agent to keep appending to daily notes, so this will skew rankings as those files grow. A chunked scan with early exit once all query terms are found would keep memory bounded without dropping tail content.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@main/tools/tool_search_notes.c` around lines 112 - 123, The current code only
reads up to READ_CAP into buf once (using fread) so hits past the first 4 KiB
are ignored; change the single fread into a chunked read loop that reuses the
allocated buffer and repeatedly fread()s up to READ_CAP bytes from FILE *f
(full_path) until EOF or until all query terms are found, checking each chunk
for the query terms as you go and exiting early when every term has been found;
ensure you fclose(f) and free(buf) on all paths, keep buf null-terminated for
string searches (buf[n]=0), and preserve a small overlap between chunks (carry
the last N bytes, where N is at least the longest query term length minus 1) to
avoid missing matches spanning chunk boundaries.
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
search_notestool that searches/spiffs/memory/*.mdfor space-separated keywordsread_fileon results to retrieve full contentThis closes the loop on the agent's memory system: it can already write daily notes via
write_file, but had no way to search them. Now it can recall past conversations, decisions, and context.Changes
tools/tool_search_notes.htool_search_notes_executetools/tool_search_notes.ctools/tool_registry.csearch_notestoolCMakeLists.txttool_search_notes.cto buildUsage
The agent calls it like any other tool:
{"query": "birthday shopping list"}Returns:
Recommended: pair with a skill
For the agent to use
search_noteseffectively, we recommend adding a skill (markdown file in/spiffs/skills/) that instructs the agent when to search its notes — e.g. before answering questions that may reference past conversations, user preferences, or scheduled tasks. Without a guiding skill, the agent may not know to search proactively.Example skill (
/spiffs/skills/memory_recall.md):Summary by CodeRabbit
New Features
Documentation