fix(memory): run the file-path tests in a temporary directory - #4937
Merged
Merged
Conversation
ensureMemoryFilePath takes the directory its default files live in, defaulting to the package directory. The file-path tests pass a per-test temporary directory instead of writing memory.json and memory.jsonl beside the source, where the main() that every importing test file starts migrated them away mid-test. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: cliffhall <cliff@futurescale.com>
🦋 Changeset detectedLatest commit: 478db6a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
3 of 12 tasks
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The published TypeScript change requires a patch changeset.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Isolates memory file-path tests from shared package files to prevent intermittent failures.
Changes:
- Adds an injectable base directory to
ensureMemoryFilePath. - Uses per-test temporary directories and verifies default behavior.
| File | Description |
|---|---|
src/memory/index.ts |
Adds configurable base-directory handling. |
src/memory/__tests__/file-path.test.ts |
Isolates migration tests in temporary directories. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: cliffhall <cliff@futurescale.com>
Member
Author
Member
Author
|
Copilot round 2: clean (no findings; round 1's changeset finding marked resolved). The review loop ends here. |
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
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.
Closes #4922
Description
The memory server's file-path tests no longer write the real default graph files, so they stop failing intermittently.
ensureMemoryFilePathtakes the directory its default files live in, defaulting to the package directory. The server still calls it with no argument.__tests__/file-path.test.tscreates a temporary directory per test and removes it afterwards, instead of writingmemory.jsonandmemory.jsonlbeside the source.MEMORY_FILE_PATHresolves against the package directory); it touches no file.Server Details
Motivation and Context
See #4922. The issue left the second writer unidentified; it is the server itself.
index.tscallsmain()at module load, andmain()runsensureMemoryFilePath()on the package directory. Every test file that imports../index.jstherefore runs the migration in its own Vitest worker, in parallel with this file's tests, and renamedmemory.jsonaway between the test's write and its assertion.Thanks to @kkkhs, whose #4926 took the same approach (an optional base directory on the helper). That PR targeted
mainand is closed under the contribution policy; this is the change made through the maintainers' flow againstv2/main.How Has This Been Tested?
No client-observable change, so the evidence is targeted probes (macOS, Node 26):
npm run local:gateexits 0 on this branch (head 478db6a).npm test -w src/memory20 times in a row: 0 failures (88 tests each). Before the change the issue records roughly one failure in ten runs.src/memoryholds nomemory.jsonormemory.jsonl, andgit statusis clean.Not tested: the failure was not reproduced on demand before the fix, so the before side rests on the two occurrences recorded on the issue.
Breaking Changes
None.
Types of changes
Checklist
npm run changeset) if this changes what a TypeScript server publishes (patch,.changeset/memory-file-path-base-dir.md)Additional context
Changeset: a patch for
@modelcontextprotocol/server-memory. The server's behavior does not change (the new parameter is never passed outside tests), but the compiledindex.jsdoes, so the entry says the change is internal.main()still runs at import in every test file and still reads the package directory. That is pre-existing and harmless to these tests now; #4854 rewrites the suite around an in-process harness.🤖 Generated with Claude Code