-
Notifications
You must be signed in to change notification settings - Fork 3.8k
feat: ✨ Implement Claude memory tool #8269
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat: ✨ Implement Claude memory tool #8269
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4 issues found across 18 files
Prompt for AI agents (all 4 issues)
Understand the root cause of the following 4 issues and fix them.
<file name="extensions/cli/src/tools/memory.ts">
<violation number="1" location="extensions/cli/src/tools/memory.ts:130">
Creating a memory file in a new directory always fails: after creating the missing parent directory we throw `Path not found`, so fs.writeFile never runs. Drop the throw so the command can proceed.</violation>
</file>
<file name="core/tools/implementations/memory.ts">
<violation number="1" location="core/tools/implementations/memory.ts:121">
Core memory command handling logic duplicates extensions/cli/src/tools/memory.ts:executeMemoryCommand() and its handlers</violation>
<violation number="2" location="core/tools/implementations/memory.ts:131">
After creating a missing directory, this branch still throws `Path not found`, so the create command always fails for new paths.</violation>
</file>
<file name="extensions/cli/src/tools/index.tsx">
<violation number="1" location="extensions/cli/src/tools/index.tsx:141">
This enables the memory tool for any model containing “claude”, but the feature must be restricted to the specific allowlisted models from the PR description.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
|
@chezsmithy this is neat! |
Fixed TypeScript compilation errors in memory.vitest.ts by adding non-null assertions to all parameter property accesses. The Tool interface defines parameters as optional, but memoryTool always has parameters defined, making the assertions safe. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
The test was incorrectly expecting an error when creating a file in a non-existent directory. The actual implementation (handleCreate in memoryShared.ts) automatically creates parent directories recursively before writing the file, which is the correct behavior. Updated the test to expect success and verify that mkdir is called with the recursive option, matching the actual implementation. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Apply Prettier formatting to memory tool test files. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Add CI_GITHUB_TOKEN environment variable to setup and install steps in the CLI PR checks workflow. This prevents GitHub API rate limiting issues when npm downloads packages with binaries from GitHub releases. Without authentication, GitHub API allows only 60 requests/hour. With authentication, the limit increases to 5,000 requests/hour. This matches the pattern used in other component workflows (VSCode, GUI) and references the same issue: microsoft/vscode-ripgrep#9 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Add CI_GITHUB_TOKEN to the "Setup packages" step which was missing from the previous commit. This step runs build-packages.js which installs dependencies for 7 packages in parallel. Without the token, these parallel npm installs hit GitHub API rate limits (60 req/hour) when downloading binaries, causing processes to hang and eventually be killed with SIGTERM (exit code 143). The token increases the rate limit to 5,000 req/hour, preventing timeouts during the parallel package builds. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
5 issues found across 18 files
Prompt for AI agents (all 5 issues)
Understand the root cause of the following 5 issues and fix them.
<file name="core/tools/index.test.ts">
<violation number="1" location="core/tools/index.test.ts:208">
This test claims it expects an empty array but only checks that the result is an array. Assert that no tools are returned to ensure the behavior is actually tested.</violation>
</file>
<file name="core/llm/llms/Bedrock.ts">
<violation number="1" location="core/llm/llms/Bedrock.ts:45">
Model memory tool support logic duplicates packages/openai-adapters/src/apis/Bedrock.ts:supportsMemoryTool() function. This duplication introduces a maintenance risk, as updates to the list of supported models might not be synchronized across both files, leading to inconsistent behavior.</violation>
</file>
<file name="packages/openai-adapters/src/apis/Bedrock.ts">
<violation number="1" location="packages/openai-adapters/src/apis/Bedrock.ts:608">
`tool_calls[].index` should be the zero-based ordinal of the tool call, but this change now copies Bedrock’s `contentBlockIndex`, which also counts text blocks. Any assistant text before a tool call will shift the index and cause downstream OpenAI consumers to mis-order or drop the tool call.</violation>
</file>
<file name="core/tools/implementations/memoryShared.ts">
<violation number="1" location="core/tools/implementations/memoryShared.ts:102">
Path validation allows escaping the memories root because it only checks `startsWith` on the absolute path prefix; include a path-separator-aware check (or compare `path.relative`) to keep operations inside the sandbox.</violation>
<violation number="2" location="core/tools/implementations/memoryShared.ts:286">
Root delete protection only matches the literal string `/memories`, so variants like `/memories/` delete the entire store; check the resolved path instead of the raw argument.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| const tools = getConfigDependentToolDefinitions( | ||
| createParams({ modelName: "" }), | ||
| ); | ||
| expect(Array.isArray(tools)).toBe(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test claims it expects an empty array but only checks that the result is an array. Assert that no tools are returned to ensure the behavior is actually tested.
Prompt for AI agents
Address the following comment on core/tools/index.test.ts at line 208:
<comment>This test claims it expects an empty array but only checks that the result is an array. Assert that no tools are returned to ensure the behavior is actually tested.</comment>
<file context>
@@ -0,0 +1,263 @@
+ const tools = getConfigDependentToolDefinitions(
+ createParams({ modelName: "" }),
+ );
+ expect(Array.isArray(tools)).toBe(true);
+ });
+
</file context>
| /** | ||
| * Checks if a model supports the memory tool (Claude 4+ models only) | ||
| */ | ||
| function supportsMemoryTool(modelName: string | undefined): boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Model memory tool support logic duplicates packages/openai-adapters/src/apis/Bedrock.ts:supportsMemoryTool() function. This duplication introduces a maintenance risk, as updates to the list of supported models might not be synchronized across both files, leading to inconsistent behavior.
Prompt for AI agents
Address the following comment on core/llm/llms/Bedrock.ts at line 45:
<comment>Model memory tool support logic duplicates packages/openai-adapters/src/apis/Bedrock.ts:supportsMemoryTool() function. This duplication introduces a maintenance risk, as updates to the list of supported models might not be synchronized across both files, leading to inconsistent behavior.</comment>
<file context>
@@ -39,6 +39,27 @@ interface PromptCachingMetrics {
+/**
+ * Checks if a model supports the memory tool (Claude 4+ models only)
+ */
+function supportsMemoryTool(modelName: string | undefined): boolean {
+ if (!modelName) {
+ return false;
</file context>
| tool_calls: [ | ||
| { | ||
| index: 0, | ||
| index: blockIndex, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tool_calls[].index should be the zero-based ordinal of the tool call, but this change now copies Bedrock’s contentBlockIndex, which also counts text blocks. Any assistant text before a tool call will shift the index and cause downstream OpenAI consumers to mis-order or drop the tool call.
Prompt for AI agents
Address the following comment on packages/openai-adapters/src/apis/Bedrock.ts at line 608:
<comment>`tool_calls[].index` should be the zero-based ordinal of the tool call, but this change now copies Bedrock’s `contentBlockIndex`, which also counts text blocks. Any assistant text before a tool call will shift the index and cause downstream OpenAI consumers to mis-order or drop the tool call.</comment>
<file context>
@@ -518,7 +605,7 @@ export class BedrockApi implements BaseLlmApi {
tool_calls: [
{
- index: 0,
+ index: blockIndex,
id: delta.toolUse.toolUseId,
type: "function",
</file context>
| ): Promise<MemoryCommandResult> { | ||
| const fullPath = validateMemoryPath(args.path, memoryRoot); | ||
|
|
||
| if (args.path === `/${MEMORIES_ROOT_NAME}`) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Root delete protection only matches the literal string /memories, so variants like /memories/ delete the entire store; check the resolved path instead of the raw argument.
Prompt for AI agents
Address the following comment on core/tools/implementations/memoryShared.ts at line 286:
<comment>Root delete protection only matches the literal string `/memories`, so variants like `/memories/` delete the entire store; check the resolved path instead of the raw argument.</comment>
<file context>
@@ -0,0 +1,364 @@
+): Promise<MemoryCommandResult> {
+ const fullPath = validateMemoryPath(args.path, memoryRoot);
+
+ if (args.path === `/${MEMORIES_ROOT_NAME}`) {
+ throw new Error(
+ `Cannot delete the /${MEMORIES_ROOT_NAME} directory itself`,
</file context>
| if (args.path === `/${MEMORIES_ROOT_NAME}`) { | |
| if (path.resolve(fullPath) === path.resolve(memoryRoot)) { |
| const resolved = path.resolve(memoryRoot, relative.length ? relative : "."); | ||
| const normalizedRoot = path.resolve(memoryRoot); | ||
|
|
||
| if (!resolved.startsWith(normalizedRoot)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Path validation allows escaping the memories root because it only checks startsWith on the absolute path prefix; include a path-separator-aware check (or compare path.relative) to keep operations inside the sandbox.
Prompt for AI agents
Address the following comment on core/tools/implementations/memoryShared.ts at line 102:
<comment>Path validation allows escaping the memories root because it only checks `startsWith` on the absolute path prefix; include a path-separator-aware check (or compare `path.relative`) to keep operations inside the sandbox.</comment>
<file context>
@@ -0,0 +1,364 @@
+ const resolved = path.resolve(memoryRoot, relative.length ? relative : ".");
+ const normalizedRoot = path.resolve(memoryRoot);
+
+ if (!resolved.startsWith(normalizedRoot)) {
+ throw new Error(
+ `Path ${memoryPath} would escape /${MEMORIES_ROOT_NAME} directory`,
</file context>
|
@chezsmithy bumping that some of the cubic nitpicks seems valid! |
Description
The tool is only enabled for the following models:
AI Code Review
@continue-general-reviewor@continue-detailed-reviewChecklist
Screen recording or screenshot
[ When applicable, please include a short screen recording or screenshot - this makes it much easier for us as contributors to review and understand your changes. See this PR as a good example. ]
Tests
Added tests as needed.