feat(api): add GET /agentmemory/memories/:id (#196)#208
Merged
Conversation
The route was simply not registered. POST /remember writes to
\`KV.memories\` keyed by the generated id; GET /memories lists from
the same scope. There was no by-id retrieval handler at all, so any
\`save then fetch-by-ID\` round-trip (including the MCP tool chain's
memory verification step) returned 404 even though the data was in
storage.
Adds \`api::memory-by-id\` with path-param \`:id\`. Returns 200
\`{ memory }\` on hit, 404 \`{ error }\` on miss, 400 if id is empty.
Auth gate matches the rest of the protected endpoints.
Thanks @Axialon for the precise repro and root-cause guess (which
was correct: it was a missing handler, not a scope mismatch).
Closes #196
📝 WalkthroughWalkthroughA new authenticated GET endpoint is added to retrieve individual memories by ID ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/triggers/api.ts (1)
1346-1349: Optional: tighten and trim theidvalidation.
req.path_params?.["id"]typed asstringalready implies string, but a whitespace-only segment would currently pass. Rejecting blank/whitespace ids early (and trimming) avoids a needless KV lookup and gives a clearer 400.♻️ Suggested refinement
- const id = req.path_params?.["id"]; - if (!id || typeof id !== "string") { - return { status_code: 400, body: { error: "id path parameter is required" } }; - } - const memory = await kv.get<import("../types.js").Memory>(KV.memories, id); + const rawId = req.path_params?.["id"]; + const id = typeof rawId === "string" ? rawId.trim() : ""; + if (!id) { + return { status_code: 400, body: { error: "id path parameter is required" } }; + } + const memory = await kv.get<import("../types.js").Memory>(KV.memories, id);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/triggers/api.ts` around lines 1346 - 1349, The id extracted from req.path_params (const id = req.path_params?.["id"]) can be a whitespace-only string; update the validation to trim and reject empty values to avoid unnecessary KV lookups: coerce/ensure the value is a string, do const idRaw = req.path_params?.["id"]; const id = typeof idRaw === "string" ? idRaw.trim() : ""; then if (!id) return 400 with the same error; use the trimmed id for subsequent operations (e.g., KV lookup) so whitespace-only ids are rejected early.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/triggers/api.ts`:
- Around line 1342-1361: Update the stale endpoint count from 107 to 104: change
the README text that currently says "107 endpoints" to "104 endpoints" and
update the runtime log string that prints "Endpoints: 107 REST" to "Endpoints:
104 REST" (look for the exact literal "Endpoints: 107 REST" in the index logging
code, e.g., where the app prints endpoint counts via console.log or a logger).
---
Nitpick comments:
In `@src/triggers/api.ts`:
- Around line 1346-1349: The id extracted from req.path_params (const id =
req.path_params?.["id"]) can be a whitespace-only string; update the validation
to trim and reject empty values to avoid unnecessary KV lookups: coerce/ensure
the value is a string, do const idRaw = req.path_params?.["id"]; const id =
typeof idRaw === "string" ? idRaw.trim() : ""; then if (!id) return 400 with the
same error; use the trimmed id for subsequent operations (e.g., KV lookup) so
whitespace-only ids are rejected early.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+1342
to
+1361
| sdk.registerFunction("api::memory-by-id", | ||
| async (req: ApiRequest): Promise<Response> => { | ||
| const authErr = checkAuth(req, secret); | ||
| if (authErr) return authErr; | ||
| const id = req.path_params?.["id"]; | ||
| if (!id || typeof id !== "string") { | ||
| return { status_code: 400, body: { error: "id path parameter is required" } }; | ||
| } | ||
| const memory = await kv.get<import("../types.js").Memory>(KV.memories, id); | ||
| if (!memory) { | ||
| return { status_code: 404, body: { error: `memory not found: ${id}` } }; | ||
| } | ||
| return { status_code: 200, body: { memory } }; | ||
| }, | ||
| ); | ||
| sdk.registerTrigger({ | ||
| type: "http", | ||
| function_id: "api::memory-by-id", | ||
| config: { api_path: "/agentmemory/memories/:id", http_method: "GET" }, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify README and index.ts mention the new endpoint and any endpoint counts are updated.
fd -t f 'README.md' --max-depth 2
rg -nP '/agentmemory/memories(/:id|/\{id\})?' -C2
rg -nP 'memory-by-id|memories/:id' -C2
rg -nPi 'endpoints?\s*[:\-]?\s*\d+|\d+\s+endpoints?' README.md 2>/dev/nullRepository: rohitg00/agentmemory
Length of output: 1562
🏁 Script executed:
#!/bin/bash
# Check index.ts for endpoint count references
find . -name 'index.ts' -type f | head -10
# Count actual endpoints registered in src/triggers/api.ts
echo "=== Total registerTrigger calls in api.ts ==="
rg -c 'registerTrigger' src/triggers/api.ts
echo "=== List all endpoints in api.ts ==="
rg -oP 'api_path:\s*"([^"]+)"' src/triggers/api.ts | sort | uniq -c | sort -rn
# Check if index.ts exists at repo root or src root
echo "=== Checking for index.ts ==="
cat src/index.ts 2>/dev/null | grep -i 'endpoint' -C 2 || echo "No endpoint references in src/index.ts"Repository: rohitg00/agentmemory
Length of output: 5848
Update endpoint counts in README.md and src/index.ts — they show 107 but should be 104.
The handler implementation itself is correct — it's registered in api.ts (lines 1342–1361) and uses proper auth/validation patterns consistent with other endpoints. However, the endpoint counts in your documentation are stale:
README.mdtext states "107 endpoints" (should be 104 — matches the badge already)src/index.tslogs "Endpoints: 107 REST" (should be 104)
Both need to be corrected to 104 to match the actual unique endpoint count in the codebase.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/triggers/api.ts` around lines 1342 - 1361, Update the stale endpoint
count from 107 to 104: change the README text that currently says "107
endpoints" to "104 endpoints" and update the runtime log string that prints
"Endpoints: 107 REST" to "Endpoints: 104 REST" (look for the exact literal
"Endpoints: 107 REST" in the index logging code, e.g., where the app prints
endpoint counts via console.log or a logger).
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
1 participant
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
The route was simply not registered. `POST /remember` writes to `KV.memories` keyed by the generated id; `GET /memories` lists from the same scope. There was no by-id retrieval handler at all, so any "save then fetch-by-ID" round-trip returned 404 even though the data was in storage and visible in list responses.
Adds `api::memory-by-id` with path-param `:id`:
Thanks @Axialon for the precise repro and root-cause guess (correct: it was a missing handler, not a scope mismatch).
Closes #196
Test plan
Summary by CodeRabbit