From 5c06f00e545a71c6ab5b1f1c2eccf92fd5519bd0 Mon Sep 17 00:00:00 2001 From: Scot Hastings <6472550+wfscot@users.noreply.github.com> Date: Fri, 6 Mar 2026 15:29:24 -0700 Subject: [PATCH 01/15] refactor(sources): extract shared helpers from fetch functions --- src/lib/sources.ts | 342 ++++++++++++++++++++++++--------------------- 1 file changed, 185 insertions(+), 157 deletions(-) diff --git a/src/lib/sources.ts b/src/lib/sources.ts index 861d80d7..9589306a 100644 --- a/src/lib/sources.ts +++ b/src/lib/sources.ts @@ -23,6 +23,7 @@ import { listDirectoryRecursive, withSemaphore } from "./github-utils.js"; import { parseSource } from "./source-parser.js"; import { type LockedSkill, + type LockedSource, type SourcesLock, computeSkillIntegrity, createEmptyLock, @@ -180,8 +181,148 @@ async function checkLockedSkillsExist(curatedDir: string, skillNames: string[]): return true; } +// --------------------------------------------------------------------------- +// Shared helpers for fetchSource and fetchSourceViaGit +// --------------------------------------------------------------------------- + +/** + * Remove previously curated skill directories for a source before re-fetching. + * Validates that each path resolves within the curated directory to prevent traversal. + */ +async function cleanPreviousCuratedSkills( + curatedDir: string, + lockedSkillNames: string[], +): Promise { + const resolvedCuratedDir = resolve(curatedDir); + for (const prevSkill of lockedSkillNames) { + const prevDir = join(curatedDir, prevSkill); + if (!resolve(prevDir).startsWith(resolvedCuratedDir + sep)) { + logger.warn( + `Skipping removal of "${prevSkill}": resolved path is outside the curated directory.`, + ); + continue; + } + if (await directoryExists(prevDir)) { + await removeDirectory(prevDir); + } + } +} + +/** + * Check whether a skill should be skipped during fetching. + * Returns true (with appropriate logging) if the skill should be skipped. + */ +function shouldSkipSkill( + skillName: string, + sourceKey: string, + localSkillNames: Set, + alreadyFetchedSkillNames: Set, +): boolean { + if (skillName.includes("..") || skillName.includes("/") || skillName.includes("\\")) { + logger.warn( + `Skipping skill with invalid name "${skillName}" from ${sourceKey}: contains path traversal characters.`, + ); + return true; + } + if (localSkillNames.has(skillName)) { + logger.debug( + `Skipping remote skill "${skillName}" from ${sourceKey}: local skill takes precedence.`, + ); + return true; + } + if (alreadyFetchedSkillNames.has(skillName)) { + logger.warn( + `Skipping duplicate skill "${skillName}" from ${sourceKey}: already fetched from another source.`, + ); + return true; + } + return false; +} + +/** + * Write skill files to disk, compute integrity, and check against the lockfile. + * Returns the computed LockedSkill entry. + */ +async function writeSkillAndComputeIntegrity(params: { + skillName: string; + files: Array<{ relativePath: string; content: string }>; + curatedDir: string; + locked: LockedSource | undefined; + resolvedSha: string; + sourceKey: string; +}): Promise { + const { skillName, files, curatedDir, locked, resolvedSha, sourceKey } = params; + const written: Array<{ path: string; content: string }> = []; + + for (const file of files) { + checkPathTraversal({ + relativePath: file.relativePath, + intendedRootDir: join(curatedDir, skillName), + }); + await writeFileContent(join(curatedDir, skillName, file.relativePath), file.content); + written.push({ path: file.relativePath, content: file.content }); + } + + const integrity = computeSkillIntegrity(written); + const lockedSkillEntry = locked?.skills[skillName]; + if ( + lockedSkillEntry?.integrity && + lockedSkillEntry.integrity !== integrity && + resolvedSha === locked?.resolvedRef + ) { + logger.warn( + `Integrity mismatch for skill "${skillName}" from ${sourceKey}: expected "${lockedSkillEntry.integrity}", got "${integrity}". Content may have been tampered with.`, + ); + } + + return { integrity }; +} + +/** + * Merge newly fetched skills with existing locked skills and update the lockfile. + */ +function buildLockUpdate(params: { + lock: SourcesLock; + sourceKey: string; + fetchedSkills: Record; + locked: LockedSource | undefined; + requestedRef: string | undefined; + resolvedSha: string; +}): { updatedLock: SourcesLock; fetchedNames: string[] } { + const { lock, sourceKey, fetchedSkills, locked, requestedRef, resolvedSha } = params; + const fetchedNames = Object.keys(fetchedSkills); + + // Merge newly fetched skills with existing locked skills that were skipped + // (due to local precedence, already-fetched, etc.) to prevent overwriting their entries + const mergedSkills: Record = { ...fetchedSkills }; + if (locked) { + for (const [skillName, skillEntry] of Object.entries(locked.skills)) { + if (!(skillName in mergedSkills)) { + mergedSkills[skillName] = skillEntry; + } + } + } + + const updatedLock = setLockedSource(lock, sourceKey, { + requestedRef, + resolvedRef: resolvedSha, + resolvedAt: new Date().toISOString(), + skills: mergedSkills, + }); + + logger.info( + `Fetched ${fetchedNames.length} skill(s) from ${sourceKey}: ${fetchedNames.join(", ") || "(none)"}`, + ); + + return { updatedLock, fetchedNames }; +} + +// --------------------------------------------------------------------------- +// Transport-specific fetch functions +// --------------------------------------------------------------------------- + /** - * Fetch skills from a single source entry. + * Fetch skills from a single source entry via the GitHub REST API. */ async function fetchSource(params: { sourceEntry: SourceEntry; @@ -276,50 +417,12 @@ async function fetchSource(params: { const semaphore = new Semaphore(FETCH_CONCURRENCY_LIMIT); const fetchedSkills: Record = {}; - // Clean previously curated skills for this source before re-fetching if (locked) { - const resolvedCuratedDir = resolve(curatedDir); - for (const prevSkill of lockedSkillNames) { - const prevDir = join(curatedDir, prevSkill); - // Verify the resolved path is within the curated directory to prevent traversal - if (!resolve(prevDir).startsWith(resolvedCuratedDir + sep)) { - logger.warn( - `Skipping removal of "${prevSkill}": resolved path is outside the curated directory.`, - ); - continue; - } - if (await directoryExists(prevDir)) { - await removeDirectory(prevDir); - } - } + await cleanPreviousCuratedSkills(curatedDir, lockedSkillNames); } for (const skillDir of filteredDirs) { - // Validate skill directory name to prevent path traversal - if ( - skillDir.name.includes("..") || - skillDir.name.includes("/") || - skillDir.name.includes("\\") - ) { - logger.warn( - `Skipping skill with invalid name "${skillDir.name}" from ${sourceKey}: contains path traversal characters.`, - ); - continue; - } - - // Skip skills that exist locally (local takes precedence) - if (localSkillNames.has(skillDir.name)) { - logger.debug( - `Skipping remote skill "${skillDir.name}" from ${sourceKey}: local skill takes precedence.`, - ); - continue; - } - - // Skip skills already fetched from an earlier source (first-declared wins) - if (alreadyFetchedSkillNames.has(skillDir.name)) { - logger.warn( - `Skipping duplicate skill "${skillDir.name}" from ${sourceKey}: already fetched from another source.`, - ); + if (shouldSkipSkill(skillDir.name, sourceKey, localSkillNames, alreadyFetchedSkillNames)) { continue; } @@ -344,75 +447,40 @@ async function fetchSource(params: { return true; }); - // Fetch all file contents and compute integrity hash - const skillFiles: Array<{ path: string; content: string }> = []; - + // Fetch all file contents + const skillFiles: Array<{ relativePath: string; content: string }> = []; for (const file of files) { - // Calculate relative path within the skill directory const relativeToSkill = file.path.substring(skillDir.path.length + 1); - const localFilePath = join(curatedDir, skillDir.name, relativeToSkill); - - // Validate path to prevent traversal attacks - checkPathTraversal({ - relativePath: relativeToSkill, - intendedRootDir: join(curatedDir, skillDir.name), - }); - const content = await withSemaphore(semaphore, () => client.getFileContent(parsed.owner, parsed.repo, file.path, ref), ); - await writeFileContent(localFilePath, content); - skillFiles.push({ path: relativeToSkill, content }); + skillFiles.push({ relativePath: relativeToSkill, content }); } - const integrity = computeSkillIntegrity(skillFiles); - - // Verify integrity against lockfile hash when available - const lockedSkillEntry = locked?.skills[skillDir.name]; - if ( - lockedSkillEntry && - lockedSkillEntry.integrity && - lockedSkillEntry.integrity !== integrity && - resolvedSha === locked?.resolvedRef - ) { - logger.warn( - `Integrity mismatch for skill "${skillDir.name}" from ${sourceKey}: expected "${lockedSkillEntry.integrity}", got "${integrity}". Content may have been tampered with.`, - ); - } - - fetchedSkills[skillDir.name] = { integrity }; + fetchedSkills[skillDir.name] = await writeSkillAndComputeIntegrity({ + skillName: skillDir.name, + files: skillFiles, + curatedDir, + locked, + resolvedSha, + sourceKey, + }); logger.debug(`Fetched skill "${skillDir.name}" from ${sourceKey}`); } - const fetchedNames = Object.keys(fetchedSkills); - - // Merge newly fetched skills with existing locked skills that were skipped - // (due to local precedence, already-fetched, etc.) to prevent overwriting their entries - const mergedSkills: Record = { ...fetchedSkills }; - if (locked) { - for (const [skillName, skillEntry] of Object.entries(locked.skills)) { - if (!(skillName in mergedSkills)) { - mergedSkills[skillName] = skillEntry; - } - } - } - - // Update lockfile entry - lock = setLockedSource(lock, sourceKey, { + const result = buildLockUpdate({ + lock, + sourceKey, + fetchedSkills, + locked, requestedRef, - resolvedRef: resolvedSha, - resolvedAt: new Date().toISOString(), - skills: mergedSkills, + resolvedSha, }); - logger.info( - `Fetched ${fetchedNames.length} skill(s) from ${sourceKey}: ${fetchedNames.join(", ") || "(none)"}`, - ); - return { - skillCount: fetchedNames.length, - fetchedSkillNames: fetchedNames, - updatedLock: lock, + skillCount: result.fetchedNames.length, + fetchedSkillNames: result.fetchedNames, + updatedLock: result.updatedLock, }; } @@ -496,76 +564,36 @@ async function fetchSourceViaGit(params: { const filteredNames = isWildcard ? allNames : allNames.filter((n) => skillFilter.includes(n)); if (locked) { - const base = resolve(curatedDir); - for (const prev of lockedSkillNames) { - const dir = join(curatedDir, prev); - if (resolve(dir).startsWith(base + sep) && (await directoryExists(dir))) { - await removeDirectory(dir); - } - } + await cleanPreviousCuratedSkills(curatedDir, lockedSkillNames); } const fetchedSkills: Record = {}; for (const skillName of filteredNames) { - if (skillName.includes("..") || skillName.includes("/") || skillName.includes("\\")) { - logger.warn( - `Skipping skill with invalid name "${skillName}" from ${url}: contains path traversal characters.`, - ); - continue; - } - if (localSkillNames.has(skillName)) { - logger.debug( - `Skipping remote skill "${skillName}" from ${url}: local skill takes precedence.`, - ); - continue; - } - if (alreadyFetchedSkillNames.has(skillName)) { - logger.warn( - `Skipping duplicate skill "${skillName}" from ${url}: already fetched from another source.`, - ); + if (shouldSkipSkill(skillName, url, localSkillNames, alreadyFetchedSkillNames)) { continue; } - const files = skillFileMap.get(skillName) ?? []; - const written: Array<{ path: string; content: string }> = []; - for (const file of files) { - checkPathTraversal({ - relativePath: file.relativePath, - intendedRootDir: join(curatedDir, skillName), - }); - await writeFileContent(join(curatedDir, skillName, file.relativePath), file.content); - written.push({ path: file.relativePath, content: file.content }); - } - - const integrity = computeSkillIntegrity(written); - const lockedSkillEntry = locked?.skills[skillName]; - if ( - lockedSkillEntry?.integrity && - lockedSkillEntry.integrity !== integrity && - resolvedSha === locked?.resolvedRef - ) { - logger.warn(`Integrity mismatch for skill "${skillName}" from ${url}.`); - } - fetchedSkills[skillName] = { integrity }; - } - - const fetchedNames = Object.keys(fetchedSkills); - const mergedSkills: Record = { ...fetchedSkills }; - if (locked) { - for (const [k, v] of Object.entries(locked.skills)) { - if (!(k in mergedSkills)) mergedSkills[k] = v; - } + fetchedSkills[skillName] = await writeSkillAndComputeIntegrity({ + skillName, + files: skillFileMap.get(skillName) ?? [], + curatedDir, + locked, + resolvedSha, + sourceKey: url, + }); } - lock = setLockedSource(lock, url, { + const result = buildLockUpdate({ + lock, + sourceKey: url, + fetchedSkills, + locked, requestedRef, - resolvedRef: resolvedSha, - resolvedAt: new Date().toISOString(), - skills: mergedSkills, + resolvedSha, }); - - logger.info( - `Fetched ${fetchedNames.length} skill(s) from ${url}: ${fetchedNames.join(", ") || "(none)"}`, - ); - return { skillCount: fetchedNames.length, fetchedSkillNames: fetchedNames, updatedLock: lock }; + return { + skillCount: result.fetchedNames.length, + fetchedSkillNames: result.fetchedNames, + updatedLock: result.updatedLock, + }; } From bbd0ed66292e03168d410b4222cf23d4832d5e45 Mon Sep 17 00:00:00 2001 From: Scot Hastings <6472550+wfscot@users.noreply.github.com> Date: Fri, 6 Mar 2026 15:29:24 -0700 Subject: [PATCH 02/15] refactor: deduplicate control character detection into shared validation utility --- config-schema.json | 31 +++++++++++++++++++++++++++---- src/config/config.ts | 9 +-------- src/lib/git-client.ts | 15 +-------------- src/utils/validation.ts | 25 +++++++++++++++++++++++++ 4 files changed, 54 insertions(+), 26 deletions(-) create mode 100644 src/utils/validation.ts diff --git a/config-schema.json b/config-schema.json index e3f30692..651741f0 100644 --- a/config-schema.json +++ b/config-schema.json @@ -50,7 +50,16 @@ "type": "array", "items": { "type": "string", - "enum": ["rules", "ignore", "mcp", "subagents", "commands", "skills", "hooks", "*"] + "enum": [ + "rules", + "ignore", + "mcp", + "subagents", + "commands", + "skills", + "hooks", + "*" + ] } }, { @@ -62,7 +71,16 @@ "type": "array", "items": { "type": "string", - "enum": ["rules", "ignore", "mcp", "subagents", "commands", "skills", "hooks", "*"] + "enum": [ + "rules", + "ignore", + "mcp", + "subagents", + "commands", + "skills", + "hooks", + "*" + ] } } } @@ -112,7 +130,10 @@ }, "transport": { "type": "string", - "enum": ["github", "git"] + "enum": [ + "github", + "git" + ] }, "ref": { "type": "string" @@ -121,7 +142,9 @@ "type": "string" } }, - "required": ["source"], + "required": [ + "source" + ], "additionalProperties": false } } diff --git a/src/config/config.ts b/src/config/config.ts index 81b661a3..694f0246 100644 --- a/src/config/config.ts +++ b/src/config/config.ts @@ -17,14 +17,7 @@ import { ToolTarget, ToolTargets, } from "../types/tool-targets.js"; - -function hasControlCharacters(value: string): boolean { - for (let i = 0; i < value.length; i++) { - const code = value.charCodeAt(i); - if ((code >= 0x00 && code <= 0x1f) || code === 0x7f) return true; - } - return false; -} +import { hasControlCharacters } from "../utils/validation.js"; /** * Schema for a single source entry in the sources array. diff --git a/src/lib/git-client.ts b/src/lib/git-client.ts index 9a004c67..87b801d8 100644 --- a/src/lib/git-client.ts +++ b/src/lib/git-client.ts @@ -13,6 +13,7 @@ import { removeTempDirectory, } from "../utils/file.js"; import { logger } from "../utils/logger.js"; +import { findControlCharacter } from "../utils/validation.js"; const execFileAsync = promisify(execFile); @@ -24,20 +25,6 @@ const ALLOWED_URL_SCHEMES = const INSECURE_URL_SCHEMES = /^(git:\/\/|http:\/\/)/; -/** - * Check for control characters and return the position and hex code of the first one found. - * Returns null if no control characters are found. - */ -function findControlCharacter(value: string): { position: number; hex: string } | null { - for (let i = 0; i < value.length; i++) { - const code = value.charCodeAt(i); - if ((code >= 0x00 && code <= 0x1f) || code === 0x7f) { - return { position: i, hex: `0x${code.toString(16).padStart(2, "0")}` }; - } - } - return null; -} - export class GitClientError extends Error { constructor(message: string, cause?: unknown) { super(message, { cause }); diff --git a/src/utils/validation.ts b/src/utils/validation.ts new file mode 100644 index 00000000..171766bb --- /dev/null +++ b/src/utils/validation.ts @@ -0,0 +1,25 @@ +/** + * Shared validation utilities for input sanitization. + */ + +/** + * Find the first control character in a string. + * Returns the position and hex code, or null if none found. + * Control characters: 0x00-0x1f and 0x7f (DEL). + */ +export function findControlCharacter(value: string): { position: number; hex: string } | null { + for (let i = 0; i < value.length; i++) { + const code = value.charCodeAt(i); + if ((code >= 0x00 && code <= 0x1f) || code === 0x7f) { + return { position: i, hex: `0x${code.toString(16).padStart(2, "0")}` }; + } + } + return null; +} + +/** + * Check whether a string contains any control characters. + */ +export function hasControlCharacters(value: string): boolean { + return findControlCharacter(value) !== null; +} From 76306a91802d95803d9f5e8a0475bd5fdb47b663 Mon Sep 17 00:00:00 2001 From: Scot Hastings <6472550+wfscot@users.noreply.github.com> Date: Fri, 6 Mar 2026 15:29:24 -0700 Subject: [PATCH 03/15] fix(git-client): validate ref returned by resolveDefaultRef --- src/lib/git-client.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/lib/git-client.ts b/src/lib/git-client.ts index 87b801d8..ca182f14 100644 --- a/src/lib/git-client.ts +++ b/src/lib/git-client.ts @@ -94,6 +94,7 @@ export async function resolveDefaultRef(url: string): Promise<{ ref: string; sha const ref = stdout.match(/^ref: refs\/heads\/(.+)\tHEAD$/m)?.[1]; const sha = stdout.match(/^([0-9a-f]{40})\tHEAD$/m)?.[1]; if (!ref || !sha) throw new GitClientError(`Could not parse default branch from: ${url}`); + validateRef(ref); return { ref, sha }; } catch (error) { if (error instanceof GitClientError) throw error; From 0cd5b8f5cf5d14ce349f45d3c2bce495db72941d Mon Sep 17 00:00:00 2001 From: Scot Hastings <6472550+wfscot@users.noreply.github.com> Date: Fri, 6 Mar 2026 15:29:25 -0700 Subject: [PATCH 04/15] fix(git-client): validate skillsPath in fetchSkillFiles --- src/lib/git-client.test.ts | 29 +++++++++++++++++++++++++++++ src/lib/git-client.ts | 13 ++++++++++++- 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/src/lib/git-client.test.ts b/src/lib/git-client.test.ts index bd4162ec..918d9e09 100644 --- a/src/lib/git-client.test.ts +++ b/src/lib/git-client.test.ts @@ -197,6 +197,35 @@ describe("git-client", () => { expect(removeTempDirectory).toHaveBeenCalledWith("/tmp/test"); }); + it("rejects skillsPath with path traversal", async () => { + await expect( + fetchSkillFiles({ url: "https://example.com/repo.git", ref: "main", skillsPath: "../etc" }), + ).rejects.toThrow(GitClientError); + await expect( + fetchSkillFiles({ url: "https://example.com/repo.git", ref: "main", skillsPath: "../etc" }), + ).rejects.toThrow("must be a relative path"); + }); + + it("rejects absolute skillsPath", async () => { + await expect( + fetchSkillFiles({ + url: "https://example.com/repo.git", + ref: "main", + skillsPath: "/etc/passwd", + }), + ).rejects.toThrow(GitClientError); + }); + + it("rejects skillsPath with control characters", async () => { + await expect( + fetchSkillFiles({ + url: "https://example.com/repo.git", + ref: "main", + skillsPath: "skills\x00", + }), + ).rejects.toThrow("control character"); + }); + it("returns empty when skills dir missing", async () => { mockExecFileAsync.mockResolvedValue({ stdout: "" }); vi.mocked(createTempDirectory).mockResolvedValue("/tmp/test"); diff --git a/src/lib/git-client.ts b/src/lib/git-client.ts index ca182f14..2aca2f80 100644 --- a/src/lib/git-client.ts +++ b/src/lib/git-client.ts @@ -1,5 +1,5 @@ import { execFile } from "node:child_process"; -import { join } from "node:path"; +import { isAbsolute, join } from "node:path"; import { promisify } from "node:util"; import { MAX_FILE_SIZE } from "../constants/rulesync-paths.js"; @@ -132,6 +132,17 @@ export async function fetchSkillFiles(params: { const { url, ref, skillsPath } = params; validateGitUrl(url); validateRef(ref); + if (skillsPath.includes("..") || isAbsolute(skillsPath)) { + throw new GitClientError( + `Invalid skillsPath "${skillsPath}": must be a relative path without ".."`, + ); + } + const ctrl = findControlCharacter(skillsPath); + if (ctrl) { + throw new GitClientError( + `skillsPath contains control character ${ctrl.hex} at position ${ctrl.position}`, + ); + } await checkGitAvailable(); const tmpDir = await createTempDirectory("rulesync-git-"); try { From 7cfb8b1ca5b77042c8d92796967ed553293e7112 Mon Sep 17 00:00:00 2001 From: Scot Hastings <6472550+wfscot@users.noreply.github.com> Date: Fri, 6 Mar 2026 15:29:25 -0700 Subject: [PATCH 05/15] fix(sources): add GitClientError hints in error handler --- src/lib/sources.ts | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/src/lib/sources.ts b/src/lib/sources.ts index 9589306a..9d55a9fe 100644 --- a/src/lib/sources.ts +++ b/src/lib/sources.ts @@ -17,7 +17,13 @@ import { writeFileContent, } from "../utils/file.js"; import { logger } from "../utils/logger.js"; -import { fetchSkillFiles, resolveDefaultRef, resolveRefToSha, validateRef } from "./git-client.js"; +import { + GitClientError, + fetchSkillFiles, + resolveDefaultRef, + resolveRefToSha, + validateRef, +} from "./git-client.js"; import { GitHubClient, GitHubClientError, logGitHubAuthHints } from "./github-client.js"; import { listDirectoryRecursive, withSemaphore } from "./github-utils.js"; import { parseSource } from "./source-parser.js"; @@ -142,6 +148,8 @@ export async function resolveAndFetchSources(params: { logger.error(`Failed to fetch source "${sourceEntry.source}": ${formatError(error)}`); if (error instanceof GitHubClientError) { logGitHubAuthHints(error); + } else if (error instanceof GitClientError) { + logGitClientHints(error); } } } @@ -168,6 +176,19 @@ export async function resolveAndFetchSources(params: { return { fetchedSkillCount: totalSkillCount, sourcesProcessed: sources.length }; } +/** + * Log contextual hints for GitClientError to help users troubleshoot. + */ +function logGitClientHints(error: GitClientError): void { + if (error.message.includes("not installed")) { + logger.info("Hint: Install git and ensure it is available on your PATH."); + } else { + logger.info( + "Hint: Check your git credentials (SSH keys, credential helper, or access token).", + ); + } +} + /** * Check if all locked skills exist on disk in the curated directory. */ From ab5087b2c0b6f90eb1003005bdee113c479d6379 Mon Sep 17 00:00:00 2001 From: Scot Hastings <6472550+wfscot@users.noreply.github.com> Date: Fri, 6 Mar 2026 15:29:25 -0700 Subject: [PATCH 06/15] refactor(git-client): use path.relative() in walkDirectory --- src/lib/git-client.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/lib/git-client.ts b/src/lib/git-client.ts index 2aca2f80..e1a80e97 100644 --- a/src/lib/git-client.ts +++ b/src/lib/git-client.ts @@ -1,5 +1,5 @@ import { execFile } from "node:child_process"; -import { isAbsolute, join } from "node:path"; +import { isAbsolute, join, relative } from "node:path"; import { promisify } from "node:util"; import { MAX_FILE_SIZE } from "../constants/rulesync-paths.js"; @@ -208,7 +208,7 @@ async function walkDirectory( continue; } const content = await readFileContent(fullPath); - results.push({ relativePath: fullPath.substring(baseDir.length + 1), content, size }); + results.push({ relativePath: relative(baseDir, fullPath), content, size }); } } return results; From fe97302423faa9295c47404a0bb8848c811ca8af Mon Sep 17 00:00:00 2001 From: Scot Hastings <6472550+wfscot@users.noreply.github.com> Date: Fri, 6 Mar 2026 15:29:26 -0700 Subject: [PATCH 07/15] fix(git-client): anchor SCP-style URL regex to prevent trailing junk --- src/lib/git-client.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib/git-client.ts b/src/lib/git-client.ts index e1a80e97..b85fa22f 100644 --- a/src/lib/git-client.ts +++ b/src/lib/git-client.ts @@ -21,7 +21,7 @@ const execFileAsync = promisify(execFile); const GIT_TIMEOUT_MS = 60_000; const ALLOWED_URL_SCHEMES = - /^(https?:\/\/|ssh:\/\/|git:\/\/|file:\/\/\/|[a-zA-Z0-9_.+-]+@[a-zA-Z0-9.-]+:[a-zA-Z0-9_.+/~-]+)/; + /^(https?:\/\/|ssh:\/\/|git:\/\/|file:\/\/\/).+$|^[a-zA-Z0-9_.+-]+@[a-zA-Z0-9.-]+:[a-zA-Z0-9_.+/~-]+$/; const INSECURE_URL_SCHEMES = /^(git:\/\/|http:\/\/)/; From d8446c9e337f4d4392752a8886d76991fc4c1d74 Mon Sep 17 00:00:00 2001 From: Scot Hastings <6472550+wfscot@users.noreply.github.com> Date: Fri, 6 Mar 2026 15:29:26 -0700 Subject: [PATCH 08/15] fix(git-client): add total file count and size limits to walkDirectory --- src/lib/git-client.ts | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/src/lib/git-client.ts b/src/lib/git-client.ts index b85fa22f..5d171ecb 100644 --- a/src/lib/git-client.ts +++ b/src/lib/git-client.ts @@ -178,11 +178,17 @@ export async function fetchSkillFiles(params: { } const MAX_WALK_DEPTH = 20; +const MAX_TOTAL_FILES = 10_000; +const MAX_TOTAL_SIZE = 100 * 1024 * 1024; // 100 MB + +/** Mutable context for tracking totals across recursive walkDirectory calls. */ +type WalkContext = { totalFiles: number; totalSize: number }; async function walkDirectory( dir: string, baseDir: string, depth: number = 0, + ctx: WalkContext = { totalFiles: 0, totalSize: 0 }, ): Promise> { if (depth > MAX_WALK_DEPTH) { throw new GitClientError( @@ -198,7 +204,7 @@ async function walkDirectory( continue; } if (await directoryExists(fullPath)) { - results.push(...(await walkDirectory(fullPath, baseDir, depth + 1))); + results.push(...(await walkDirectory(fullPath, baseDir, depth + 1, ctx))); } else { const size = await getFileSize(fullPath); if (size > MAX_FILE_SIZE) { @@ -207,6 +213,18 @@ async function walkDirectory( ); continue; } + ctx.totalFiles++; + ctx.totalSize += size; + if (ctx.totalFiles > MAX_TOTAL_FILES) { + throw new GitClientError( + `Repository exceeds max file count of ${MAX_TOTAL_FILES}. Aborting to prevent resource exhaustion.`, + ); + } + if (ctx.totalSize > MAX_TOTAL_SIZE) { + throw new GitClientError( + `Repository exceeds max total size of ${MAX_TOTAL_SIZE / 1024 / 1024}MB. Aborting to prevent resource exhaustion.`, + ); + } const content = await readFileContent(fullPath); results.push({ relativePath: relative(baseDir, fullPath), content, size }); } From d9bfa97dc1d80e5a08833a5e3d8264660a80cc3b Mon Sep 17 00:00:00 2001 From: Scot Hastings <6472550+wfscot@users.noreply.github.com> Date: Fri, 6 Mar 2026 15:29:26 -0700 Subject: [PATCH 09/15] fix(sources-lock): validate resolvedRef as 40-character hex SHA --- src/lib/sources-lock.test.ts | 6 ++++-- src/lib/sources-lock.ts | 6 ++++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/lib/sources-lock.test.ts b/src/lib/sources-lock.test.ts index 94847bd4..bd8fe29a 100644 --- a/src/lib/sources-lock.test.ts +++ b/src/lib/sources-lock.test.ts @@ -29,6 +29,8 @@ vi.mock("../utils/logger.js", () => ({ const { logger } = await vi.importMock("../utils/logger.js"); +const VALID_SHA = "a".repeat(40); + describe("sources-lock", () => { afterEach(() => { vi.clearAllMocks(); @@ -64,7 +66,7 @@ describe("sources-lock", () => { lockfileVersion: 1, sources: { "https://github.com/org/repo": { - resolvedRef: "abc123", + resolvedRef: VALID_SHA, skills: { "skill-a": { integrity: "sha256-abc" }, "skill-b": { integrity: "sha256-def" }, @@ -78,7 +80,7 @@ describe("sources-lock", () => { const lock = await readLockFile({ baseDir: testDir }); expect(lock.sources["https://github.com/org/repo"]).toEqual({ - resolvedRef: "abc123", + resolvedRef: VALID_SHA, skills: { "skill-a": { integrity: "sha256-abc" }, "skill-b": { integrity: "sha256-def" }, diff --git a/src/lib/sources-lock.ts b/src/lib/sources-lock.ts index bdd3d2a3..26566fbc 100644 --- a/src/lib/sources-lock.ts +++ b/src/lib/sources-lock.ts @@ -1,7 +1,7 @@ import { createHash } from "node:crypto"; import { join } from "node:path"; -import { optional, z } from "zod/mini"; +import { optional, refine, z } from "zod/mini"; import { RULESYNC_SOURCES_LOCK_RELATIVE_FILE_PATH } from "../constants/rulesync-paths.js"; import { fileExists, readFileContent, writeFileContent } from "../utils/file.js"; @@ -23,7 +23,9 @@ export type LockedSkill = z.infer; */ export const LockedSourceSchema = z.object({ requestedRef: optional(z.string()), - resolvedRef: z.string(), + resolvedRef: z.string().check( + refine((v) => /^[0-9a-f]{40}$/.test(v), "resolvedRef must be a 40-character hex SHA"), + ), resolvedAt: optional(z.string()), skills: z.record(z.string(), LockedSkillSchema), }); From 25c0d214d43347c2755a8ad89f78696cacb79425 Mon Sep 17 00:00:00 2001 From: Scot Hastings <6472550+wfscot@users.noreply.github.com> Date: Fri, 6 Mar 2026 15:29:26 -0700 Subject: [PATCH 10/15] test(sources): expand git transport test coverage --- src/lib/sources.test.ts | 193 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 193 insertions(+) diff --git a/src/lib/sources.test.ts b/src/lib/sources.test.ts index eecb56af..ccd5fb3b 100644 --- a/src/lib/sources.test.ts +++ b/src/lib/sources.test.ts @@ -770,4 +770,197 @@ describe("resolveAndFetchSources", () => { skillsPath: "exports/skills", }); }); + + it("should error in frozen mode when git source lockfile entry lacks requestedRef", async () => { + const { readLockFile } = await import("./sources-lock.js"); + + vi.mocked(readLockFile).mockResolvedValue({ + lockfileVersion: 1, + sources: { + "https://dev.azure.com/org/_git/repo": { + resolvedRef: "a".repeat(40), + skills: { "my-skill": { integrity: "sha256-x" } }, + }, + }, + }); + + // Skill dir missing so SHA-match skip fails + vi.mocked(directoryExists).mockResolvedValue(false); + + const result = await resolveAndFetchSources({ + sources: [ + { source: "https://dev.azure.com/org/_git/repo", transport: "git" }, + ], + baseDir: testDir, + options: { frozen: true }, + }); + + expect(result.fetchedSkillCount).toBe(0); + expect(logger.error).toHaveBeenCalledWith(expect.stringContaining("missing requestedRef")); + }); + + it("should skip re-fetch for git transport when locked SHA matches and skills exist", async () => { + const { readLockFile } = await import("./sources-lock.js"); + const { fetchSkillFiles } = await import("./git-client.js"); + const curatedDir = join(testDir, RULESYNC_CURATED_SKILLS_RELATIVE_DIR_PATH); + + vi.mocked(readLockFile).mockResolvedValue({ + lockfileVersion: 1, + sources: { + "https://dev.azure.com/org/_git/repo": { + resolvedRef: "b".repeat(40), + requestedRef: "main", + skills: { "cached-skill": { integrity: "sha256-cached" } }, + }, + }, + }); + + vi.mocked(directoryExists).mockImplementation(async (path: string) => { + if (path === join(curatedDir, "cached-skill")) return true; + return false; + }); + + const result = await resolveAndFetchSources({ + sources: [ + { source: "https://dev.azure.com/org/_git/repo", transport: "git" }, + ], + baseDir: testDir, + }); + + expect(result.fetchedSkillCount).toBe(0); + expect(vi.mocked(fetchSkillFiles)).not.toHaveBeenCalled(); + }); + + it("should apply skill filter for git transport", async () => { + const { resolveDefaultRef, fetchSkillFiles } = await import("./git-client.js"); + vi.mocked(resolveDefaultRef).mockResolvedValue({ ref: "main", sha: "c".repeat(40) }); + vi.mocked(fetchSkillFiles).mockResolvedValue([ + { relativePath: "skill-a/SKILL.md", content: "A", size: 10 }, + { relativePath: "skill-b/SKILL.md", content: "B", size: 10 }, + ]); + + const result = await resolveAndFetchSources({ + sources: [ + { + source: "https://dev.azure.com/org/_git/repo", + transport: "git", + skills: ["skill-a"], + }, + ], + baseDir: testDir, + }); + + expect(result.fetchedSkillCount).toBe(1); + const writeArgs = vi.mocked(writeFileContent).mock.calls.map((call) => call[0]); + expect(writeArgs.some((p) => p.includes("skill-a"))).toBe(true); + expect(writeArgs.some((p) => p.includes("skill-b"))).toBe(false); + }); + + it("should skip git transport skill when local skill takes precedence", async () => { + const { resolveDefaultRef, fetchSkillFiles } = await import("./git-client.js"); + vi.mocked(resolveDefaultRef).mockResolvedValue({ ref: "main", sha: "d".repeat(40) }); + vi.mocked(fetchSkillFiles).mockResolvedValue([ + { relativePath: "local-skill/SKILL.md", content: "remote", size: 10 }, + ]); + + // local-skill exists locally + vi.mocked(directoryExists).mockImplementation(async (path: string) => { + if (path.endsWith("skills")) return true; + return false; + }); + vi.mocked(findFilesByGlobs).mockResolvedValue([ + join(testDir, ".rulesync/skills/local-skill"), + ]); + + const result = await resolveAndFetchSources({ + sources: [ + { source: "https://dev.azure.com/org/_git/repo", transport: "git" }, + ], + baseDir: testDir, + }); + + expect(result.fetchedSkillCount).toBe(0); + expect(writeFileContent).not.toHaveBeenCalled(); + }); + + it("should skip duplicate git transport skill from later source", async () => { + const { resolveDefaultRef, fetchSkillFiles } = await import("./git-client.js"); + vi.mocked(resolveDefaultRef).mockResolvedValue({ ref: "main", sha: "e".repeat(40) }); + vi.mocked(fetchSkillFiles).mockResolvedValue([ + { relativePath: "shared-skill/SKILL.md", content: "content", size: 10 }, + ]); + + const result = await resolveAndFetchSources({ + sources: [ + { source: "https://dev.azure.com/org/_git/repo-a", transport: "git" }, + { source: "https://dev.azure.com/org/_git/repo-b", transport: "git" }, + ], + baseDir: testDir, + }); + + // First source fetches it, second source skips it + expect(result.fetchedSkillCount).toBe(1); + }); + + it("should warn on integrity mismatch for git transport skill", async () => { + const { readLockFile } = await import("./sources-lock.js"); + const { fetchSkillFiles } = await import("./git-client.js"); + const lockedSha = "f".repeat(40); + + vi.mocked(readLockFile).mockResolvedValue({ + lockfileVersion: 1, + sources: { + "https://dev.azure.com/org/_git/repo": { + resolvedRef: lockedSha, + requestedRef: "main", + skills: { "my-skill": { integrity: "sha256-original" } }, + }, + }, + }); + + // Skill dir missing so re-fetch is triggered + vi.mocked(directoryExists).mockResolvedValue(false); + vi.mocked(fetchSkillFiles).mockResolvedValue([ + { relativePath: "my-skill/SKILL.md", content: "tampered", size: 10 }, + ]); + + await resolveAndFetchSources({ + sources: [ + { source: "https://dev.azure.com/org/_git/repo", transport: "git" }, + ], + baseDir: testDir, + }); + + expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining("Integrity mismatch")); + }); + + it("should handle GitClientError gracefully and continue processing", async () => { + const { GitClientError } = await import("./git-client.js"); + const { resolveDefaultRef, fetchSkillFiles } = await import("./git-client.js"); + + let callCount = 0; + vi.mocked(resolveDefaultRef).mockImplementation(async () => { + callCount++; + if (callCount === 1) { + throw new GitClientError("git is not installed or not found in PATH"); + } + return { ref: "main", sha: "a".repeat(40) }; + }); + vi.mocked(fetchSkillFiles).mockResolvedValue([ + { relativePath: "good-skill/SKILL.md", content: "ok", size: 10 }, + ]); + + const result = await resolveAndFetchSources({ + sources: [ + { source: "https://dev.azure.com/org/_git/failing", transport: "git" }, + { source: "https://dev.azure.com/org/_git/good", transport: "git" }, + ], + baseDir: testDir, + }); + + expect(result.fetchedSkillCount).toBe(1); + expect(result.sourcesProcessed).toBe(2); + expect(logger.error).toHaveBeenCalledWith(expect.stringContaining("not installed")); + expect(logger.info).toHaveBeenCalledWith(expect.stringContaining("Hint")); + }); }); From 45e94ee632a606cf7f2d3183a7a137d54631eb2e Mon Sep 17 00:00:00 2001 From: Scot Hastings <6472550+wfscot@users.noreply.github.com> Date: Fri, 6 Mar 2026 15:29:27 -0700 Subject: [PATCH 11/15] fix(sources): use const for non-reassigned lock destructuring --- src/lib/sources.ts | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/lib/sources.ts b/src/lib/sources.ts index 9d55a9fe..112e4dde 100644 --- a/src/lib/sources.ts +++ b/src/lib/sources.ts @@ -183,9 +183,7 @@ function logGitClientHints(error: GitClientError): void { if (error.message.includes("not installed")) { logger.info("Hint: Install git and ensure it is available on your PATH."); } else { - logger.info( - "Hint: Check your git credentials (SSH keys, credential helper, or access token).", - ); + logger.info("Hint: Check your git credentials (SSH keys, credential helper, or access token)."); } } @@ -360,7 +358,7 @@ async function fetchSource(params: { }> { const { sourceEntry, client, baseDir, localSkillNames, alreadyFetchedSkillNames, updateSources } = params; - let { lock } = params; + const { lock } = params; const parsed = parseSource(sourceEntry.source); @@ -519,7 +517,7 @@ async function fetchSourceViaGit(params: { }): Promise<{ skillCount: number; fetchedSkillNames: string[]; updatedLock: SourcesLock }> { const { sourceEntry, baseDir, localSkillNames, alreadyFetchedSkillNames, updateSources, frozen } = params; - let { lock } = params; + const { lock } = params; const url = sourceEntry.source; const locked = getLockedSource(lock, url); const lockedSkillNames = locked ? getLockedSkillNames(locked) : []; From 8f44a2beeac43eb9186b78a3f84aebfaeb8e85b3 Mon Sep 17 00:00:00 2001 From: Scot Hastings <6472550+wfscot@users.noreply.github.com> Date: Fri, 6 Mar 2026 15:29:27 -0700 Subject: [PATCH 12/15] docs(schema): add descriptions to source entry fields in JSON schema --- config-schema.json | 44 ++++++++++----------------------- scripts/generate-json-schema.ts | 23 ++++++++++++++++- 2 files changed, 35 insertions(+), 32 deletions(-) diff --git a/config-schema.json b/config-schema.json index 651741f0..234c404f 100644 --- a/config-schema.json +++ b/config-schema.json @@ -50,16 +50,7 @@ "type": "array", "items": { "type": "string", - "enum": [ - "rules", - "ignore", - "mcp", - "subagents", - "commands", - "skills", - "hooks", - "*" - ] + "enum": ["rules", "ignore", "mcp", "subagents", "commands", "skills", "hooks", "*"] } }, { @@ -71,16 +62,7 @@ "type": "array", "items": { "type": "string", - "enum": [ - "rules", - "ignore", - "mcp", - "subagents", - "commands", - "skills", - "hooks", - "*" - ] + "enum": ["rules", "ignore", "mcp", "subagents", "commands", "skills", "hooks", "*"] } } } @@ -120,31 +102,31 @@ "properties": { "source": { "type": "string", - "minLength": 1 + "minLength": 1, + "description": "Repository identifier (e.g., 'org/repo' or a full URL)." }, "skills": { "type": "array", "items": { "type": "string" - } + }, + "description": "Skill names to fetch. Omit to fetch all skills from the source." }, "transport": { "type": "string", - "enum": [ - "github", - "git" - ] + "enum": ["github", "git"], + "description": "Transport protocol for fetching skills. 'github' uses the GitHub REST API (default). 'git' uses the git CLI and works with any git remote." }, "ref": { - "type": "string" + "type": "string", + "description": "Git ref (branch or tag) to fetch skills from. Defaults to the repository's default branch." }, "path": { - "type": "string" + "type": "string", + "description": "Path to the skills directory within the repository. Defaults to 'skills'." } }, - "required": [ - "source" - ], + "required": ["source"], "additionalProperties": false } } diff --git a/scripts/generate-json-schema.ts b/scripts/generate-json-schema.ts index 77c7d0a5..b174be95 100644 --- a/scripts/generate-json-schema.ts +++ b/scripts/generate-json-schema.ts @@ -12,9 +12,30 @@ const generatedSchema = z.toJSONSchema(ConfigFileSchema, { reused: "ref", }); +// Add descriptions to source entry fields (zod/mini lacks .describe()) +// Round-trip through JSON to get a plain object we can mutate without type assertions +const schemaObj = JSON.parse(JSON.stringify(generatedSchema)); +const sourceProps = schemaObj?.properties?.sources?.items?.properties; +if (sourceProps) { + if (sourceProps.source) + sourceProps.source.description = "Repository identifier (e.g., 'org/repo' or a full URL)."; + if (sourceProps.skills) + sourceProps.skills.description = + "Skill names to fetch. Omit to fetch all skills from the source."; + if (sourceProps.transport) + sourceProps.transport.description = + "Transport protocol for fetching skills. 'github' uses the GitHub REST API (default). 'git' uses the git CLI and works with any git remote."; + if (sourceProps.ref) + sourceProps.ref.description = + "Git ref (branch or tag) to fetch skills from. Defaults to the repository's default branch."; + if (sourceProps.path) + sourceProps.path.description = + "Path to the skills directory within the repository. Defaults to 'skills'."; +} + // Add JSON Schema meta properties (override Zod's default $schema with draft-07 for broader compatibility) const jsonSchema = { - ...generatedSchema, + ...schemaObj, $schema: "http://json-schema.org/draft-07/schema#", $id: "https://raw.githubusercontent.com/dyoshikawa/rulesync/refs/heads/main/config-schema.json", title: "Rulesync Configuration", From 0c56e4a8970544867d65cbb829313d72fc43ce17 Mon Sep 17 00:00:00 2001 From: Scot Hastings <6472550+wfscot@users.noreply.github.com> Date: Fri, 6 Mar 2026 15:29:27 -0700 Subject: [PATCH 13/15] fix(git-client): use segment-based path traversal check and fix off-by-one in walk limits --- src/lib/git-client.ts | 6 +++--- src/lib/sources-lock.ts | 6 +++--- src/lib/sources.test.ts | 20 +++++--------------- 3 files changed, 11 insertions(+), 21 deletions(-) diff --git a/src/lib/git-client.ts b/src/lib/git-client.ts index 5d171ecb..613888b2 100644 --- a/src/lib/git-client.ts +++ b/src/lib/git-client.ts @@ -132,7 +132,7 @@ export async function fetchSkillFiles(params: { const { url, ref, skillsPath } = params; validateGitUrl(url); validateRef(ref); - if (skillsPath.includes("..") || isAbsolute(skillsPath)) { + if (skillsPath.split(/[/\\]/).includes("..") || isAbsolute(skillsPath)) { throw new GitClientError( `Invalid skillsPath "${skillsPath}": must be a relative path without ".."`, ); @@ -215,12 +215,12 @@ async function walkDirectory( } ctx.totalFiles++; ctx.totalSize += size; - if (ctx.totalFiles > MAX_TOTAL_FILES) { + if (ctx.totalFiles >= MAX_TOTAL_FILES) { throw new GitClientError( `Repository exceeds max file count of ${MAX_TOTAL_FILES}. Aborting to prevent resource exhaustion.`, ); } - if (ctx.totalSize > MAX_TOTAL_SIZE) { + if (ctx.totalSize >= MAX_TOTAL_SIZE) { throw new GitClientError( `Repository exceeds max total size of ${MAX_TOTAL_SIZE / 1024 / 1024}MB. Aborting to prevent resource exhaustion.`, ); diff --git a/src/lib/sources-lock.ts b/src/lib/sources-lock.ts index 26566fbc..d8b6c3ee 100644 --- a/src/lib/sources-lock.ts +++ b/src/lib/sources-lock.ts @@ -23,9 +23,9 @@ export type LockedSkill = z.infer; */ export const LockedSourceSchema = z.object({ requestedRef: optional(z.string()), - resolvedRef: z.string().check( - refine((v) => /^[0-9a-f]{40}$/.test(v), "resolvedRef must be a 40-character hex SHA"), - ), + resolvedRef: z + .string() + .check(refine((v) => /^[0-9a-f]{40}$/.test(v), "resolvedRef must be a 40-character hex SHA")), resolvedAt: optional(z.string()), skills: z.record(z.string(), LockedSkillSchema), }); diff --git a/src/lib/sources.test.ts b/src/lib/sources.test.ts index ccd5fb3b..895f8ab2 100644 --- a/src/lib/sources.test.ts +++ b/src/lib/sources.test.ts @@ -788,9 +788,7 @@ describe("resolveAndFetchSources", () => { vi.mocked(directoryExists).mockResolvedValue(false); const result = await resolveAndFetchSources({ - sources: [ - { source: "https://dev.azure.com/org/_git/repo", transport: "git" }, - ], + sources: [{ source: "https://dev.azure.com/org/_git/repo", transport: "git" }], baseDir: testDir, options: { frozen: true }, }); @@ -821,9 +819,7 @@ describe("resolveAndFetchSources", () => { }); const result = await resolveAndFetchSources({ - sources: [ - { source: "https://dev.azure.com/org/_git/repo", transport: "git" }, - ], + sources: [{ source: "https://dev.azure.com/org/_git/repo", transport: "git" }], baseDir: testDir, }); @@ -868,14 +864,10 @@ describe("resolveAndFetchSources", () => { if (path.endsWith("skills")) return true; return false; }); - vi.mocked(findFilesByGlobs).mockResolvedValue([ - join(testDir, ".rulesync/skills/local-skill"), - ]); + vi.mocked(findFilesByGlobs).mockResolvedValue([join(testDir, ".rulesync/skills/local-skill")]); const result = await resolveAndFetchSources({ - sources: [ - { source: "https://dev.azure.com/org/_git/repo", transport: "git" }, - ], + sources: [{ source: "https://dev.azure.com/org/_git/repo", transport: "git" }], baseDir: testDir, }); @@ -925,9 +917,7 @@ describe("resolveAndFetchSources", () => { ]); await resolveAndFetchSources({ - sources: [ - { source: "https://dev.azure.com/org/_git/repo", transport: "git" }, - ], + sources: [{ source: "https://dev.azure.com/org/_git/repo", transport: "git" }], baseDir: testDir, }); From 656c5ac8d15cccfb3b01f74f71c85d539d790215 Mon Sep 17 00:00:00 2001 From: Scot Hastings <6472550+wfscot@users.noreply.github.com> Date: Fri, 6 Mar 2026 15:36:25 -0700 Subject: [PATCH 14/15] refactor: address code review feedback --- src/lib/sources.ts | 24 ++++++++++----- src/utils/validation.test.ts | 58 ++++++++++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+), 8 deletions(-) create mode 100644 src/utils/validation.test.ts diff --git a/src/lib/sources.ts b/src/lib/sources.ts index 112e4dde..c1dacc60 100644 --- a/src/lib/sources.ts +++ b/src/lib/sources.ts @@ -231,12 +231,13 @@ async function cleanPreviousCuratedSkills( * Check whether a skill should be skipped during fetching. * Returns true (with appropriate logging) if the skill should be skipped. */ -function shouldSkipSkill( - skillName: string, - sourceKey: string, - localSkillNames: Set, - alreadyFetchedSkillNames: Set, -): boolean { +function shouldSkipSkill(params: { + skillName: string; + sourceKey: string; + localSkillNames: Set; + alreadyFetchedSkillNames: Set; +}): boolean { + const { skillName, sourceKey, localSkillNames, alreadyFetchedSkillNames } = params; if (skillName.includes("..") || skillName.includes("/") || skillName.includes("\\")) { logger.warn( `Skipping skill with invalid name "${skillName}" from ${sourceKey}: contains path traversal characters.`, @@ -441,7 +442,14 @@ async function fetchSource(params: { } for (const skillDir of filteredDirs) { - if (shouldSkipSkill(skillDir.name, sourceKey, localSkillNames, alreadyFetchedSkillNames)) { + if ( + shouldSkipSkill({ + skillName: skillDir.name, + sourceKey, + localSkillNames, + alreadyFetchedSkillNames, + }) + ) { continue; } @@ -588,7 +596,7 @@ async function fetchSourceViaGit(params: { const fetchedSkills: Record = {}; for (const skillName of filteredNames) { - if (shouldSkipSkill(skillName, url, localSkillNames, alreadyFetchedSkillNames)) { + if (shouldSkipSkill({ skillName, sourceKey: url, localSkillNames, alreadyFetchedSkillNames })) { continue; } diff --git a/src/utils/validation.test.ts b/src/utils/validation.test.ts new file mode 100644 index 00000000..4aea4710 --- /dev/null +++ b/src/utils/validation.test.ts @@ -0,0 +1,58 @@ +import { describe, expect, it } from "vitest"; + +import { findControlCharacter, hasControlCharacters } from "./validation.js"; + +describe("findControlCharacter", () => { + it("returns null for clean strings", () => { + expect(findControlCharacter("hello world")).toBeNull(); + expect(findControlCharacter("path/to/file.ts")).toBeNull(); + expect(findControlCharacter("")).toBeNull(); + }); + + it("detects null byte (0x00)", () => { + const result = findControlCharacter("abc\x00def"); + expect(result).toEqual({ position: 3, hex: "0x00" }); + }); + + it("detects newline (0x0a)", () => { + const result = findControlCharacter("line\nbreak"); + expect(result).toEqual({ position: 4, hex: "0x0a" }); + }); + + it("detects tab (0x09)", () => { + const result = findControlCharacter("\tfoo"); + expect(result).toEqual({ position: 0, hex: "0x09" }); + }); + + it("detects DEL (0x7f)", () => { + const result = findControlCharacter("foo\x7fbar"); + expect(result).toEqual({ position: 3, hex: "0x7f" }); + }); + + it("detects boundary value 0x1f", () => { + const result = findControlCharacter("x\x1fy"); + expect(result).toEqual({ position: 1, hex: "0x1f" }); + }); + + it("returns the first control character when multiple exist", () => { + const result = findControlCharacter("a\x01b\x02c"); + expect(result).toEqual({ position: 1, hex: "0x01" }); + }); + + it("does not flag printable characters like space (0x20) or tilde (0x7e)", () => { + expect(findControlCharacter(" ")).toBeNull(); + expect(findControlCharacter("~")).toBeNull(); + }); +}); + +describe("hasControlCharacters", () => { + it("returns false for clean strings", () => { + expect(hasControlCharacters("hello")).toBe(false); + expect(hasControlCharacters("")).toBe(false); + }); + + it("returns true when control characters are present", () => { + expect(hasControlCharacters("abc\x00")).toBe(true); + expect(hasControlCharacters("\x7f")).toBe(true); + }); +}); From d6fd2e211fa3264bf4dc69af41929636ff662bd7 Mon Sep 17 00:00:00 2001 From: dyoshikawa Date: Sat, 7 Mar 2026 00:09:59 +0000 Subject: [PATCH 15/15] Commented on PR #1277 for clarification Co-authored-by: dyoshikawa-claw --- .serena/project.yml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.serena/project.yml b/.serena/project.yml index e046a9b5..4cdacb18 100644 --- a/.serena/project.yml +++ b/.serena/project.yml @@ -114,3 +114,7 @@ symbol_info_budget: # Note: the backend is fixed at startup. If a project with a different backend # is activated post-init, an error will be returned. language_backend: + +# list of regex patterns which, when matched, mark a memory entry as read‑only. +# Extends the list from the global configuration, merging the two lists. +read_only_memory_patterns: []