Skip to content

Commit 72e4b77

Browse files
claudemrsimpson
authored andcommitted
fix(core): link source contents directly into docset dir (no extra subfolder)
Fixes two related bugs in createSymlinks: 1. Path with no trailing slash (eg. /temp/demo): the directory name "demo" was appended as a subfolder, producing demo-docset/demo/ instead of exposing the contents at the docset root. 2. Path with trailing slash (eg. /temp/demo/): split("/").pop() returned an empty string, which fell back to the hardcoded name "unknown", producing demo-docset/unknown/. Root cause: the function symlinked the source directory itself, deriving the link name from the path string. Fix: iterate over the entries inside the source directory and create one symlink per entry directly inside targetDir. This matches the behaviour of git_repo and archive sources, which also populate the docset root without an extra named subfolder. The trailing-slash case is a non-issue because we now read directory contents rather than parsing the path string. https://claude.ai/code/session_014fPGYkZQTaWzSuLcDejZQD
1 parent b5ca44e commit 72e4b77

8 files changed

Lines changed: 155 additions & 250 deletions

File tree

packages/content-loader/src/docset-init.ts

Lines changed: 4 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/**
2-
* Shared docset initialization logic used by both the CLI and MCP server.
2+
* Docset initialization logic shared between CLI and MCP server.
33
*/
44

55
import { promises as fs } from "node:fs";
@@ -39,13 +39,8 @@ export interface InitDocsetOptions {
3939
}
4040

4141
/**
42-
* Initialize the sources for a docset: download / symlink content, write
43-
* metadata files. Pure logic — no console output, no config loading.
44-
*
45-
* @param docsetId Docset identifier (used in metadata)
46-
* @param docset Already-resolved DocsetConfig
47-
* @param configPath Absolute path to the `.knowledge/config.yaml` file
48-
* @param options Optional flags and progress callback
42+
* Download / symlink all sources for a docset and write metadata files.
43+
* Does not load config or produce console output — callers handle both.
4944
*/
5045
export async function initDocset(
5146
docsetId: string,
@@ -61,13 +56,12 @@ export async function initDocset(
6156

6257
const localPath = calculateLocalPath(docset, configPath);
6358

64-
// Check if already initialized
6559
let existsAlready = false;
6660
try {
6761
const stat = await fs.stat(localPath);
6862
if (stat.isDirectory()) existsAlready = true;
6963
} catch {
70-
// Directory doesn't exist yet — that's fine
64+
// not yet created
7165
}
7266

7367
if (existsAlready && !force) {

packages/core/src/__tests__/cleanup.test.ts

Lines changed: 16 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,6 @@ describe("Safe Directory Cleanup", () => {
3333

3434
describe("safelyClearDirectory", () => {
3535
it("should clear directory with regular files", async () => {
36-
// Create some files
3736
await fs.writeFile(path.join(targetDir, "file1.txt"), "content1");
3837
await fs.writeFile(path.join(targetDir, "file2.txt"), "content2");
3938
await fs.mkdir(path.join(targetDir, "subdir"), { recursive: true });
@@ -42,10 +41,8 @@ describe("Safe Directory Cleanup", () => {
4241
"content3",
4342
);
4443

45-
// Clear directory
4644
await safelyClearDirectory(targetDir);
4745

48-
// Directory should not exist
4946
const exists = await fs
5047
.access(targetDir)
5148
.then(() => true)
@@ -54,78 +51,63 @@ describe("Safe Directory Cleanup", () => {
5451
});
5552

5653
it("should handle non-existent directory gracefully", async () => {
57-
const nonExistent = path.join(testDir, "does-not-exist");
58-
59-
// Should not throw
60-
await expect(safelyClearDirectory(nonExistent)).resolves.not.toThrow();
54+
await expect(
55+
safelyClearDirectory(path.join(testDir, "does-not-exist")),
56+
).resolves.not.toThrow();
6157
});
6258

6359
it("should clear directory with symlinks without deleting source files", async () => {
64-
// Create source file
6560
const srcFolder = path.join(sourceDir, "src");
6661
await fs.mkdir(srcFolder, { recursive: true });
6762
const sourceFile = path.join(srcFolder, "important.js");
6863
await fs.writeFile(sourceFile, "IMPORTANT DATA");
6964

70-
// Create symlink
7165
await createSymlinks(["src"], targetDir, sourceDir);
7266

73-
// Verify symlink exists
74-
const symlinkPath = path.join(targetDir, "src");
75-
const stat = await fs.lstat(symlinkPath);
76-
expect(stat.isSymbolicLink()).toBe(true);
67+
const symlinkPath = path.join(targetDir, "important.js");
68+
expect((await fs.lstat(symlinkPath)).isSymbolicLink()).toBe(true);
7769

78-
// Clear target directory
7970
await safelyClearDirectory(targetDir);
8071

81-
// Source file must still exist!
82-
const sourceContent = await fs.readFile(sourceFile, "utf-8");
83-
expect(sourceContent).toBe("IMPORTANT DATA");
84-
85-
// Target directory should be gone
86-
const targetExists = await fs
87-
.access(targetDir)
88-
.then(() => true)
89-
.catch(() => false);
90-
expect(targetExists).toBe(false);
72+
// Source must survive removal of the docset directory
73+
expect(await fs.readFile(sourceFile, "utf-8")).toBe("IMPORTANT DATA");
74+
expect(
75+
await fs
76+
.access(targetDir)
77+
.then(() => true)
78+
.catch(() => false),
79+
).toBe(false);
9180
});
9281
});
9382

9483
describe("containsSymlinks", () => {
9584
it("should detect symlinks", async () => {
96-
// Create a source folder
9785
const srcFolder = path.join(sourceDir, "src");
9886
await fs.mkdir(srcFolder, { recursive: true });
9987
await fs.writeFile(path.join(srcFolder, "file.js"), "content");
10088

101-
// Create symlink
10289
await createSymlinks(["src"], targetDir, sourceDir);
10390

104-
const hasSymlinks = await containsSymlinks(targetDir);
105-
expect(hasSymlinks).toBe(true);
91+
expect(await containsSymlinks(targetDir)).toBe(true);
10692
});
10793

10894
it("should return false for directory with no symlinks", async () => {
10995
await fs.writeFile(path.join(targetDir, "regular.txt"), "content");
11096

111-
const hasSymlinks = await containsSymlinks(targetDir);
112-
expect(hasSymlinks).toBe(false);
97+
expect(await containsSymlinks(targetDir)).toBe(false);
11398
});
11499

115100
it("should return false for non-existent directory", async () => {
116-
const hasSymlinks = await containsSymlinks(path.join(testDir, "nope"));
117-
expect(hasSymlinks).toBe(false);
101+
expect(await containsSymlinks(path.join(testDir, "nope"))).toBe(false);
118102
});
119103
});
120104

121105
describe("getDirectoryInfo", () => {
122106
it("should count different entry types", async () => {
123-
// Create mixed content
124107
await fs.writeFile(path.join(targetDir, "file1.txt"), "content");
125108
await fs.writeFile(path.join(targetDir, "file2.txt"), "content");
126109
await fs.mkdir(path.join(targetDir, "subdir"), { recursive: true });
127110

128-
// Create symlink
129111
const srcFolder = path.join(sourceDir, "src");
130112
await fs.mkdir(srcFolder, { recursive: true });
131113
await fs.writeFile(path.join(srcFolder, "file.js"), "content");

packages/core/src/__tests__/local-folder-safety.test.ts

Lines changed: 48 additions & 77 deletions
Original file line numberDiff line numberDiff line change
@@ -15,15 +15,13 @@ describe("Local Folder Cleanup Safety", () => {
1515
let sourceFile: string;
1616

1717
beforeEach(async () => {
18-
// Create test directories
1918
testDir = path.join(tmpdir(), `agentic-safety-test-${Date.now()}`);
2019
sourceDir = path.join(testDir, "source");
2120
targetDir = path.join(testDir, "target");
2221

2322
await fs.mkdir(sourceDir, { recursive: true });
2423
await fs.mkdir(targetDir, { recursive: true });
2524

26-
// Create a source directory with actual files
2725
const actualSourceFolder = path.join(sourceDir, "src");
2826
await fs.mkdir(actualSourceFolder, { recursive: true });
2927
sourceFile = path.join(actualSourceFolder, "important-file.js");
@@ -35,34 +33,26 @@ describe("Local Folder Cleanup Safety", () => {
3533
});
3634

3735
it("CRITICAL: should NOT delete source files when removing symlinks", async () => {
38-
// Create symlink to source directory
3936
await createSymlinks(["src"], targetDir, sourceDir);
4037

41-
// Verify symlink was created
42-
const symlinkPath = path.join(targetDir, "src");
43-
const linkStat = await fs.lstat(symlinkPath);
44-
expect(linkStat.isSymbolicLink()).toBe(true);
45-
46-
// Verify we can access the source file through the symlink
47-
const fileViaSymlink = path.join(symlinkPath, "important-file.js");
48-
const content = await fs.readFile(fileViaSymlink, "utf-8");
49-
expect(content).toBe("CRITICAL DATA - DO NOT DELETE");
38+
const symlinkPath = path.join(targetDir, "important-file.js");
39+
expect((await fs.lstat(symlinkPath)).isSymbolicLink()).toBe(true);
40+
expect(await fs.readFile(symlinkPath, "utf-8")).toBe(
41+
"CRITICAL DATA - DO NOT DELETE",
42+
);
5043

51-
// Remove symlinks
5244
await removeSymlinks(targetDir);
5345

54-
// CRITICAL: Source file must still exist!
46+
// CRITICAL: removing the symlink must never touch the source
5547
const stillExists = await fs
5648
.access(sourceFile)
5749
.then(() => true)
5850
.catch(() => false);
5951
expect(stillExists).toBe(true);
52+
expect(await fs.readFile(sourceFile, "utf-8")).toBe(
53+
"CRITICAL DATA - DO NOT DELETE",
54+
);
6055

61-
// Verify content is unchanged
62-
const originalContent = await fs.readFile(sourceFile, "utf-8");
63-
expect(originalContent).toBe("CRITICAL DATA - DO NOT DELETE");
64-
65-
// Symlink should be gone
6656
const symlinkGone = await fs
6757
.lstat(symlinkPath)
6858
.then(() => false)
@@ -71,107 +61,88 @@ describe("Local Folder Cleanup Safety", () => {
7161
});
7262

7363
it("CRITICAL: should NOT delete source files when clearing target directory", async () => {
74-
// Create symlink
7564
await createSymlinks(["src"], targetDir, sourceDir);
7665

77-
// Verify source file exists
78-
expect(await fs.readFile(sourceFile, "utf-8")).toBe(
79-
"CRITICAL DATA - DO NOT DELETE",
80-
);
81-
82-
// Simulate clearing target directory (what --force does)
83-
// This is the DANGEROUS operation we need to test
66+
// Simulate --force: delete the whole docset directory
8467
await fs.rm(targetDir, { recursive: true, force: true });
8568

86-
// CRITICAL: Source file must STILL exist after removing target!
69+
// CRITICAL: fs.rm must not follow symlinks into the source
8770
const stillExists = await fs
8871
.access(sourceFile)
8972
.then(() => true)
9073
.catch(() => false);
91-
9274
expect(stillExists).toBe(
9375
true,
9476
"CRITICAL FAILURE: Source file was deleted!",
9577
);
9678

9779
if (stillExists) {
98-
const content = await fs.readFile(sourceFile, "utf-8");
99-
expect(content).toBe("CRITICAL DATA - DO NOT DELETE");
80+
expect(await fs.readFile(sourceFile, "utf-8")).toBe(
81+
"CRITICAL DATA - DO NOT DELETE",
82+
);
10083
}
10184
});
10285

10386
it("CRITICAL: should handle nested symlinks safely", async () => {
104-
// Create nested structure in source
10587
const nestedDir = path.join(sourceDir, "src", "nested");
10688
await fs.mkdir(nestedDir, { recursive: true });
10789
const nestedFile = path.join(nestedDir, "nested-file.js");
10890
await fs.writeFile(nestedFile, "NESTED CRITICAL DATA");
10991

110-
// Create symlink
11192
await createSymlinks(["src"], targetDir, sourceDir);
112-
113-
// Clear target directory
11493
await fs.rm(targetDir, { recursive: true, force: true });
11594

116-
// CRITICAL: All source files must still exist
117-
const sourceExists = await fs
118-
.access(sourceFile)
119-
.then(() => true)
120-
.catch(() => false);
121-
const nestedExists = await fs
122-
.access(nestedFile)
123-
.then(() => true)
124-
.catch(() => false);
125-
126-
expect(sourceExists).toBe(true, "Source file was deleted!");
127-
expect(nestedExists).toBe(true, "Nested source file was deleted!");
95+
// CRITICAL: All source files must survive target removal
96+
expect(
97+
await fs
98+
.access(sourceFile)
99+
.then(() => true)
100+
.catch(() => false),
101+
).toBe(true, "Source file was deleted!");
102+
expect(
103+
await fs
104+
.access(nestedFile)
105+
.then(() => true)
106+
.catch(() => false),
107+
).toBe(true, "Nested source file was deleted!");
128108
});
129109

130110
it("should safely handle mixed content (symlinks and regular files)", async () => {
131-
// Create symlink
132111
await createSymlinks(["src"], targetDir, sourceDir);
133112

134-
// Add a regular file to target directory
135113
const regularFile = path.join(targetDir, "regular-file.txt");
136114
await fs.writeFile(regularFile, "This can be deleted");
137115

138-
// Clear target directory
139116
await fs.rm(targetDir, { recursive: true, force: true });
140117

141-
// Source file must still exist
142-
const sourceExists = await fs
143-
.access(sourceFile)
144-
.then(() => true)
145-
.catch(() => false);
146-
expect(sourceExists).toBe(true);
147-
148-
// Target directory should be gone
149-
const targetExists = await fs
150-
.access(targetDir)
151-
.then(() => true)
152-
.catch(() => false);
153-
expect(targetExists).toBe(false);
118+
expect(
119+
await fs
120+
.access(sourceFile)
121+
.then(() => true)
122+
.catch(() => false),
123+
).toBe(true);
124+
expect(
125+
await fs
126+
.access(targetDir)
127+
.then(() => true)
128+
.catch(() => false),
129+
).toBe(false);
154130
});
155131

156132
it("should document Node.js symlink behavior", async () => {
157-
// This test documents how Node.js handles symlinks with fs.rm
158-
// According to Node.js docs, fs.rm should NOT follow symlinks
159-
133+
// fs.rm with recursive:true must NOT follow symlinks — this test pins that contract.
160134
await createSymlinks(["src"], targetDir, sourceDir);
161-
const symlinkPath = path.join(targetDir, "src");
135+
const symlinkPath = path.join(targetDir, "important-file.js");
162136

163-
// Verify it's a symlink
164-
const stats = await fs.lstat(symlinkPath);
165-
expect(stats.isSymbolicLink()).toBe(true);
137+
expect((await fs.lstat(symlinkPath)).isSymbolicLink()).toBe(true);
166138

167-
// Remove just the symlink using fs.unlink
168139
await fs.unlink(symlinkPath);
169140

170-
// Source should still exist
171-
const sourceExists = await fs
172-
.access(sourceFile)
173-
.then(() => true)
174-
.catch(() => false);
175-
expect(sourceExists).toBe(true);
141+
expect(
142+
await fs
143+
.access(sourceFile)
144+
.then(() => true)
145+
.catch(() => false),
146+
).toBe(true);
176147
});
177148
});

0 commit comments

Comments
 (0)