From a0734bd162bd9e69c1cd567899934110315862c5 Mon Sep 17 00:00:00 2001 From: Mario Zechner Date: Fri, 27 Mar 2026 03:48:28 +0100 Subject: [PATCH] fix(coding-agent): tighten skill discovery and edit diffs closes #2603 --- packages/coding-agent/CHANGELOG.md | 2 + packages/coding-agent/docs/skills.md | 5 +- .../coding-agent/src/core/package-manager.ts | 58 +++++++++----- .../coding-agent/src/core/tools/edit-diff.ts | 79 ++++++++++++------- .../coding-agent/test/package-manager.test.ts | 49 ++++++++++++ packages/coding-agent/test/tools.test.ts | 23 ++++++ 6 files changed, 168 insertions(+), 48 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 0e9e59af..9263d2b8 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -6,6 +6,8 @@ - Fixed repeated compactions dropping messages that were kept by an earlier compaction by re-summarizing from the previous kept boundary and recalculating `tokensBefore` from the rebuilt session context ([#2608](https://github.com/badlogic/pi-mono/issues/2608)) - Fixed interactive compaction UI updates so `ctx.compact()` rebuilds the chat through unified compaction events, manual compaction no longer duplicates the summary block, and the `trigger-compact` example only fires when context usage crosses its threshold ([#2617](https://github.com/badlogic/pi-mono/issues/2617)) +- Fixed skill discovery to stop recursing once a directory contains `SKILL.md`, and to ignore root `*.md` files in `.agents/skills` while keeping root markdown skill files supported in `~/.pi/agent/skills`, `.pi/skills`, and package `skills/` directories ([#2603](https://github.com/badlogic/pi-mono/issues/2603)) +- Fixed edit tool diff rendering for multi-edit operations with large unchanged gaps so distant edits collapse intermediate context instead of dumping the full unchanged middle block - Fixed auto-compaction overflow recovery for Ollama models when the backend returns explicit `prompt too long; exceeded max context length ...` errors instead of silently truncating input ([#2626](https://github.com/badlogic/pi-mono/issues/2626)) - Fixed built-in tool overrides that reuse built-in parameter schemas to still honor custom `renderCall` and `renderResult` renderers in the interactive TUI, restoring the `minimal-mode` example ([#2595](https://github.com/badlogic/pi-mono/issues/2595)) diff --git a/packages/coding-agent/docs/skills.md b/packages/coding-agent/docs/skills.md index 4bcfc031..1753f3cc 100644 --- a/packages/coding-agent/docs/skills.md +++ b/packages/coding-agent/docs/skills.md @@ -34,8 +34,9 @@ Pi loads skills from: - CLI: `--skill ` (repeatable, additive even with `--no-skills`) Discovery rules: -- Direct `.md` files in the skills directory root -- Recursive `SKILL.md` files under subdirectories +- In `~/.pi/agent/skills/` and `.pi/skills/`, direct root `.md` files are discovered as individual skills +- In all skill locations, directories containing `SKILL.md` are discovered recursively +- In `~/.agents/skills/` and project `.agents/skills/`, root `.md` files are ignored Disable discovery with `--no-skills` (explicit `--skill` paths still load). diff --git a/packages/coding-agent/src/core/package-manager.ts b/packages/coding-agent/src/core/package-manager.ts index 7e449e22..b8c599cf 100644 --- a/packages/coding-agent/src/core/package-manager.ts +++ b/packages/coding-agent/src/core/package-manager.ts @@ -249,9 +249,11 @@ function collectFiles( return files; } +type SkillDiscoveryMode = "pi" | "agents"; + function collectSkillEntries( dir: string, - includeRootFiles = true, + mode: SkillDiscoveryMode, ignoreMatcher?: IgnoreMatcher, rootDir?: string, ): string[] { @@ -264,6 +266,29 @@ function collectSkillEntries( try { const dirEntries = readdirSync(dir, { withFileTypes: true }); + + for (const entry of dirEntries) { + if (entry.name !== "SKILL.md") { + continue; + } + + const fullPath = join(dir, entry.name); + let isFile = entry.isFile(); + if (entry.isSymbolicLink()) { + try { + isFile = statSync(fullPath).isFile(); + } catch { + continue; + } + } + + const relPath = toPosixPath(relative(root, fullPath)); + if (isFile && !ig.ignores(relPath)) { + entries.push(fullPath); + return entries; + } + } + for (const entry of dirEntries) { if (entry.name.startsWith(".")) continue; if (entry.name === "node_modules") continue; @@ -283,18 +308,15 @@ function collectSkillEntries( } const relPath = toPosixPath(relative(root, fullPath)); - const ignorePath = isDir ? `${relPath}/` : relPath; - if (ig.ignores(ignorePath)) continue; - - if (isDir) { - entries.push(...collectSkillEntries(fullPath, false, ig, root)); - } else if (isFile) { - const isRootMd = includeRootFiles && entry.name.endsWith(".md"); - const isSkillMd = !includeRootFiles && entry.name === "SKILL.md"; - if (isRootMd || isSkillMd) { - entries.push(fullPath); - } + if (mode === "pi" && dir === root && isFile && entry.name.endsWith(".md") && !ig.ignores(relPath)) { + entries.push(fullPath); + continue; } + + if (!isDir) continue; + if (ig.ignores(`${relPath}/`)) continue; + + entries.push(...collectSkillEntries(fullPath, mode, ig, root)); } } catch { // Ignore errors @@ -303,8 +325,8 @@ function collectSkillEntries( return entries; } -function collectAutoSkillEntries(dir: string, includeRootFiles = true): string[] { - return collectSkillEntries(dir, includeRootFiles); +function collectAutoSkillEntries(dir: string, mode: SkillDiscoveryMode): string[] { + return collectSkillEntries(dir, mode); } function findGitRepoRoot(startDir: string): string | null { @@ -516,7 +538,7 @@ function collectAutoExtensionEntries(dir: string): string[] { */ function collectResourceFiles(dir: string, resourceType: ResourceType): string[] { if (resourceType === "skills") { - return collectSkillEntries(dir); + return collectSkillEntries(dir, "pi"); } if (resourceType === "extensions") { return collectAutoExtensionEntries(dir); @@ -1964,8 +1986,8 @@ export class DefaultPackageManager implements PackageManager { addResources( "skills", [ - ...collectAutoSkillEntries(projectDirs.skills), - ...projectAgentsSkillDirs.flatMap((dir) => collectAutoSkillEntries(dir)), + ...collectAutoSkillEntries(projectDirs.skills, "pi"), + ...projectAgentsSkillDirs.flatMap((dir) => collectAutoSkillEntries(dir, "agents")), ], projectMetadata, projectOverrides.skills, @@ -1995,7 +2017,7 @@ export class DefaultPackageManager implements PackageManager { ); addResources( "skills", - [...collectAutoSkillEntries(userDirs.skills), ...collectAutoSkillEntries(userAgentsSkillsDir)], + [...collectAutoSkillEntries(userDirs.skills, "pi"), ...collectAutoSkillEntries(userAgentsSkillsDir, "agents")], userMetadata, userOverrides.skills, globalBaseDir, diff --git a/packages/coding-agent/src/core/tools/edit-diff.ts b/packages/coding-agent/src/core/tools/edit-diff.ts index 24b0828f..161d3909 100644 --- a/packages/coding-agent/src/core/tools/edit-diff.ts +++ b/packages/coding-agent/src/core/tools/edit-diff.ts @@ -311,46 +311,69 @@ export function generateDiffString( } else { // Context lines - only show a few before/after changes const nextPartIsChange = i < parts.length - 1 && (parts[i + 1].added || parts[i + 1].removed); + const hasLeadingChange = lastWasChange; + const hasTrailingChange = nextPartIsChange; - if (lastWasChange || nextPartIsChange) { - // Show context - let linesToShow = raw; - let skipStart = 0; - let skipEnd = 0; + if (hasLeadingChange && hasTrailingChange) { + if (raw.length <= contextLines * 2) { + for (const line of raw) { + const lineNum = String(oldLineNum).padStart(lineNumWidth, " "); + output.push(` ${lineNum} ${line}`); + oldLineNum++; + newLineNum++; + } + } else { + const leadingLines = raw.slice(0, contextLines); + const trailingLines = raw.slice(raw.length - contextLines); + const skippedLines = raw.length - leadingLines.length - trailingLines.length; - if (!lastWasChange) { - // Show only last N lines as leading context - skipStart = Math.max(0, raw.length - contextLines); - linesToShow = raw.slice(skipStart); - } + for (const line of leadingLines) { + const lineNum = String(oldLineNum).padStart(lineNumWidth, " "); + output.push(` ${lineNum} ${line}`); + oldLineNum++; + newLineNum++; + } - if (!nextPartIsChange && linesToShow.length > contextLines) { - // Show only first N lines as trailing context - skipEnd = linesToShow.length - contextLines; - linesToShow = linesToShow.slice(0, contextLines); - } - - // Add ellipsis if we skipped lines at start - if (skipStart > 0) { output.push(` ${"".padStart(lineNumWidth, " ")} ...`); - // Update line numbers for the skipped leading context - oldLineNum += skipStart; - newLineNum += skipStart; - } + oldLineNum += skippedLines; + newLineNum += skippedLines; - for (const line of linesToShow) { + for (const line of trailingLines) { + const lineNum = String(oldLineNum).padStart(lineNumWidth, " "); + output.push(` ${lineNum} ${line}`); + oldLineNum++; + newLineNum++; + } + } + } else if (hasLeadingChange) { + const shownLines = raw.slice(0, contextLines); + const skippedLines = raw.length - shownLines.length; + + for (const line of shownLines) { const lineNum = String(oldLineNum).padStart(lineNumWidth, " "); output.push(` ${lineNum} ${line}`); oldLineNum++; newLineNum++; } - // Add ellipsis if we skipped lines at end - if (skipEnd > 0) { + if (skippedLines > 0) { output.push(` ${"".padStart(lineNumWidth, " ")} ...`); - // Update line numbers for the skipped trailing context - oldLineNum += skipEnd; - newLineNum += skipEnd; + oldLineNum += skippedLines; + newLineNum += skippedLines; + } + } else if (hasTrailingChange) { + const skippedLines = Math.max(0, raw.length - contextLines); + if (skippedLines > 0) { + output.push(` ${"".padStart(lineNumWidth, " ")} ...`); + oldLineNum += skippedLines; + newLineNum += skippedLines; + } + + for (const line of raw.slice(skippedLines)) { + const lineNum = String(oldLineNum).padStart(lineNumWidth, " "); + output.push(` ${lineNum} ${line}`); + oldLineNum++; + newLineNum++; } } else { // Skip these context lines entirely diff --git a/packages/coding-agent/test/package-manager.test.ts b/packages/coding-agent/test/package-manager.test.ts index 77f91f2e..a7fa50cd 100644 --- a/packages/coding-agent/test/package-manager.test.ts +++ b/packages/coding-agent/test/package-manager.test.ts @@ -106,6 +106,22 @@ Content`, expect(result.skills.some((r) => r.path === skillFile && r.enabled)).toBe(true); }); + it("should auto-discover root markdown skills from .pi skill dirs", async () => { + const skillFile = join(agentDir, "skills", "single-file.md"); + mkdirSync(join(agentDir, "skills"), { recursive: true }); + writeFileSync( + skillFile, + `--- +name: single-file +description: A root markdown skill +--- +Content`, + ); + + const result = await packageManager.resolve(); + expect(result.skills.some((r) => r.path === skillFile && r.enabled)).toBe(true); + }); + it("should resolve project paths relative to .pi", async () => { const extDir = join(tempDir, ".pi", "extensions"); mkdirSync(extDir, { recursive: true }); @@ -232,6 +248,26 @@ Content`, expect(result.skills.some((r) => r.path === middleSkill && r.enabled)).toBe(true); }); + it("should ignore root markdown files in .agents/skills", async () => { + const agentsSkillsDir = join(tempDir, ".agents", "skills"); + mkdirSync(join(agentsSkillsDir, "nested-skill"), { recursive: true }); + const rootSkill = join(agentsSkillsDir, "root-file.md"); + const nestedSkill = join(agentsSkillsDir, "nested-skill", "SKILL.md"); + writeFileSync(rootSkill, "---\nname: root-file\ndescription: Root markdown file\n---\n"); + writeFileSync(nestedSkill, "---\nname: nested-skill\ndescription: Nested skill\n---\n"); + + const pm = new DefaultPackageManager({ + cwd: join(tempDir, "work"), + agentDir, + settingsManager, + }); + mkdirSync(join(tempDir, "work"), { recursive: true }); + + const result = await pm.resolve(); + expect(result.skills.some((r) => r.path === rootSkill)).toBe(false); + expect(result.skills.some((r) => r.path === nestedSkill && r.enabled)).toBe(true); + }); + it("should keep ~/.agents/skills user-scoped when cwd is under home in a non-git directory", async () => { const previousHome = process.env.HOME; process.env.HOME = tempDir; @@ -352,6 +388,19 @@ Content`, expect(result.extensions.some((r) => pathEndsWith(r.path, "main.ts") && r.enabled)).toBe(true); expect(result.themes.some((r) => pathEndsWith(r.path, "dark.json") && r.enabled)).toBe(true); }); + + it("should stop recursing when a package skill directory contains SKILL.md", async () => { + const pkgDir = join(tempDir, "skill-root-pkg"); + mkdirSync(join(pkgDir, "skills", "root-skill", "nested-skill"), { recursive: true }); + const rootSkill = join(pkgDir, "skills", "root-skill", "SKILL.md"); + const nestedSkill = join(pkgDir, "skills", "root-skill", "nested-skill", "SKILL.md"); + writeFileSync(rootSkill, "---\nname: root-skill\ndescription: Root skill\n---\n"); + writeFileSync(nestedSkill, "---\nname: nested-skill\ndescription: Nested skill\n---\n"); + + const result = await packageManager.resolveExtensionSources([pkgDir]); + expect(result.skills.some((r) => r.path === rootSkill && r.enabled)).toBe(true); + expect(result.skills.some((r) => r.path === nestedSkill)).toBe(false); + }); }); describe("progress callback", () => { diff --git a/packages/coding-agent/test/tools.test.ts b/packages/coding-agent/test/tools.test.ts index 103d93dd..020fe094 100644 --- a/packages/coding-agent/test/tools.test.ts +++ b/packages/coding-agent/test/tools.test.ts @@ -277,6 +277,29 @@ describe("Coding Agent Tools", () => { expect(result.details?.diff).toContain("GAMMA"); }); + it("should collapse large unchanged gaps in multi-edit diffs", async () => { + const testFile = join(testDir, "edit-multi-large-gap.txt"); + const lines = Array.from({ length: 600 }, (_, i) => `line ${String(i + 1).padStart(3, "0")}`); + writeFileSync(testFile, `${lines.join("\n")}\n`); + + const result = await editTool.execute("test-call-8b", { + path: testFile, + edits: [ + { oldText: "line 100\n", newText: "LINE 100\n" }, + { oldText: "line 300\n", newText: "LINE 300\n" }, + { oldText: "line 500\n", newText: "LINE 500\n" }, + ], + }); + + const diff = result.details?.diff ?? ""; + expect(diff).toContain("LINE 100"); + expect(diff).toContain("LINE 300"); + expect(diff).toContain("LINE 500"); + expect(diff).toContain("..."); + expect(diff).not.toContain("line 250"); + expect(diff.split("\n").length).toBeLessThan(50); + }); + it("should match edits against the original file, not incrementally", async () => { const testFile = join(testDir, "edit-multi-original.txt"); writeFileSync(testFile, "foo\nbar\nbaz\n");