Skip to content

Commit 49a66ab

Browse files
author
Tehan
committed
fix(skill-memory): name-based path fallback when provenance line is truncated
Large skills (e.g. delegating at ~53KB) exceed opencode's MAX_BYTES=51200 tool-output truncation. The 'Base directory for this skill:' provenance line sits after the full SKILL.md content, so it lands in the dropped tail → parseSkillProvenance returns null → skillLoadRegistry never populates → ctx_skill_note hard-fails and the transparent <skill-memory> injection no-ops. Add resolveSkillPathByName (shared disk-walk in provenance.ts), wired into the after-hook as a fallback when the primary parse returns null. The skill name is always in the tool args (never truncated), so the fallback never depends on parsing truncatable output. ctx_skill_recall's cold-start walk refactored to reuse the same helper (removes duplication). Cross-family reviewed (M3): APPROVE must=0.
1 parent 46b04b9 commit 49a66ab

5 files changed

Lines changed: 343 additions & 51 deletions

File tree

‎packages/plugin/src/features/magic-context/skill-memory/provenance.test.ts‎

Lines changed: 148 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
1-
import { describe, expect, test } from "bun:test";
2-
import { parseSkillProvenance } from "./provenance";
1+
import { afterAll, beforeAll, describe, expect, test } from "bun:test";
2+
import { mkdirSync, rmSync, writeFileSync } from "node:fs";
3+
import { tmpdir } from "node:os";
4+
import { parseSkillProvenance, resolveSkillPathByName } from "./provenance";
35

46
// Build fixture paths from process.env.HOME to avoid hardcoded /home/icetea paths
57
// that break on Mac, CI, or root environments.
@@ -127,3 +129,147 @@ describe("parseSkillProvenance", () => {
127129
expect(result!.resolvedPath).toBe(`${HOME}/.config/opencode/skills/right/SKILL.md`);
128130
});
129131
});
132+
133+
describe("resolveSkillPathByName", () => {
134+
let projectDir: string;
135+
let skillDir: string;
136+
137+
beforeAll(() => {
138+
projectDir = `${tmpdir()}/skill-resolve-test-${Date.now()}`;
139+
skillDir = `${projectDir}/.opencode/skills/test-skill`;
140+
mkdirSync(skillDir, { recursive: true });
141+
writeFileSync(
142+
`${skillDir}/SKILL.md`,
143+
"---\nskill-memory:\n enabled: true\n---\n\n# Test Skill\n",
144+
);
145+
});
146+
147+
afterAll(() => {
148+
rmSync(projectDir, { recursive: true, force: true });
149+
});
150+
151+
test("finds a project-local skill on disk", () => {
152+
const result = resolveSkillPathByName("test-skill", projectDir);
153+
expect(result).not.toBeNull();
154+
expect(result!.resolvedPath).toBe(`${skillDir}/SKILL.md`);
155+
expect(result!.tier).toBe("project");
156+
expect(result!.skillSource).toBe("opencode-project");
157+
});
158+
159+
test("returns null when no SKILL.md exists for the name", () => {
160+
const result = resolveSkillPathByName("nonexistent-skill", projectDir);
161+
expect(result).toBeNull();
162+
});
163+
164+
test("returns null for empty project dir (no skill dirs)", () => {
165+
const emptyDir = `${tmpdir()}/skill-resolve-empty-${Date.now()}`;
166+
mkdirSync(emptyDir, { recursive: true });
167+
try {
168+
const result = resolveSkillPathByName("any-skill", emptyDir);
169+
expect(result).toBeNull();
170+
} finally {
171+
rmSync(emptyDir, { recursive: true, force: true });
172+
}
173+
});
174+
175+
test("project shadows global when both contain same-named skill", () => {
176+
// Override HOME to a tmpdir that simulates global skill dirs.
177+
const savedHome = process.env.HOME;
178+
const globalHome = `${tmpdir()}/skill-resolve-global-home-${Date.now()}`;
179+
const projectDir = `${tmpdir()}/skill-resolve-proj-${Date.now()}`;
180+
try {
181+
// Global: ~/.config/opencode/skills/shadow-skill/SKILL.md
182+
const globalSkillDir = `${globalHome}/.config/opencode/skills/shadow-skill`;
183+
mkdirSync(globalSkillDir, { recursive: true });
184+
writeFileSync(`${globalSkillDir}/SKILL.md`, "---\n---\n\n# Global shadow\n");
185+
// Project: .opencode/skills/shadow-skill/SKILL.md
186+
const projectSkillDir = `${projectDir}/.opencode/skills/shadow-skill`;
187+
mkdirSync(projectSkillDir, { recursive: true });
188+
writeFileSync(`${projectSkillDir}/SKILL.md`, "---\n---\n\n# Project shadow\n");
189+
190+
process.env.HOME = globalHome;
191+
const result = resolveSkillPathByName("shadow-skill", projectDir);
192+
expect(result).not.toBeNull();
193+
// Project-first ordering: project path wins, not global
194+
expect(result!.resolvedPath).toBe(`${projectSkillDir}/SKILL.md`);
195+
expect(result!.tier).toBe("project");
196+
} finally {
197+
process.env.HOME = savedHome;
198+
rmSync(globalHome, { recursive: true, force: true });
199+
rmSync(projectDir, { recursive: true, force: true });
200+
}
201+
});
202+
203+
test("handles singular .opencode/skill/ path (project tier)", () => {
204+
const dir = `${projectDir}/.opencode/skill/singular-skill`;
205+
mkdirSync(dir, { recursive: true });
206+
writeFileSync(`${dir}/SKILL.md`, "---\n---\n\n# Singular\n");
207+
try {
208+
const result = resolveSkillPathByName("singular-skill", projectDir);
209+
expect(result).not.toBeNull();
210+
expect(result!.resolvedPath).toBe(`${dir}/SKILL.md`);
211+
expect(result!.tier).toBe("project");
212+
expect(result!.skillSource).toBe("opencode-project");
213+
} finally {
214+
rmSync(dir, { recursive: true, force: true });
215+
}
216+
});
217+
218+
test("handles project .claude/skills/ path (project tier)", () => {
219+
// deriveSkillSource uses startsWith(HOME/.claude/skills/) — only global
220+
// claude paths get skillSource="claude-skills". Project-local .claude/skills/
221+
// fall through to the default "opencode-project".
222+
const dir = `${projectDir}/.claude/skills/claude-proj-skill`;
223+
mkdirSync(dir, { recursive: true });
224+
writeFileSync(`${dir}/SKILL.md`, "---\n---\n\n# Claude project\n");
225+
try {
226+
const result = resolveSkillPathByName("claude-proj-skill", projectDir);
227+
expect(result).not.toBeNull();
228+
expect(result!.resolvedPath).toBe(`${dir}/SKILL.md`);
229+
expect(result!.tier).toBe("project");
230+
expect(result!.skillSource).toBe("opencode-project");
231+
} finally {
232+
rmSync(dir, { recursive: true, force: true });
233+
}
234+
});
235+
236+
test("handles global ~/.claude/skills/ path (global tier, claude-skills source)", () => {
237+
const savedHome = process.env.HOME;
238+
const globalHome = `${tmpdir()}/skill-resolve-claude-global-${Date.now()}`;
239+
try {
240+
const dir = `${globalHome}/.claude/skills/claude-global-skill`;
241+
mkdirSync(dir, { recursive: true });
242+
writeFileSync(`${dir}/SKILL.md`, "---\n---\n\n# Claude global\n");
243+
244+
process.env.HOME = globalHome;
245+
const result = resolveSkillPathByName("claude-global-skill", "/tmp/nonexistent");
246+
expect(result).not.toBeNull();
247+
expect(result!.resolvedPath).toBe(`${dir}/SKILL.md`);
248+
expect(result!.tier).toBe("global");
249+
expect(result!.skillSource).toBe("claude-skills");
250+
} finally {
251+
process.env.HOME = savedHome;
252+
rmSync(globalHome, { recursive: true, force: true });
253+
}
254+
});
255+
256+
test("handles global ~/.config/opencode/skill/ (singular) path", () => {
257+
const savedHome = process.env.HOME;
258+
const globalHome = `${tmpdir()}/skill-resolve-singular-global-${Date.now()}`;
259+
try {
260+
const dir = `${globalHome}/.config/opencode/skill/singular-global-skill`;
261+
mkdirSync(dir, { recursive: true });
262+
writeFileSync(`${dir}/SKILL.md`, "---\n---\n\n# Singular global\n");
263+
264+
process.env.HOME = globalHome;
265+
const result = resolveSkillPathByName("singular-global-skill", "/tmp/nonexistent");
266+
expect(result).not.toBeNull();
267+
expect(result!.resolvedPath).toBe(`${dir}/SKILL.md`);
268+
expect(result!.tier).toBe("global");
269+
expect(result!.skillSource).toBe("opencode-global");
270+
} finally {
271+
process.env.HOME = savedHome;
272+
rmSync(globalHome, { recursive: true, force: true });
273+
}
274+
});
275+
});

‎packages/plugin/src/features/magic-context/skill-memory/provenance.ts‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import { existsSync } from "node:fs";
12
import { fileURLToPath } from "node:url";
23

34
export interface SkillProvenance {
@@ -84,6 +85,55 @@ export function deriveSkillSource(
8485
return "opencode-project"; // default for unknown project-local paths
8586
}
8687

88+
/**
89+
* Name-based skill-path resolution — walks known skill directories in opencode
90+
* discovery order, returning the first match. This is the cold-start fallback
91+
* when the "Base directory for this skill:" provenance line is absent or
92+
* truncated from the skill tool output (MAX_BYTES=51200 cutoff).
93+
*
94+
* Search order matches opencode's `discoverSkills()`:
95+
* - Project dirs first (project shadows global — finding U3)
96+
* - Global dirs second
97+
*
98+
* Does NOT read SKILL.md content — callers are responsible for their own
99+
* frontmatter parsing.
100+
*/
101+
export function resolveSkillPathByName(
102+
skillName: string,
103+
projectDirectory: string,
104+
): {
105+
resolvedPath: string;
106+
tier: "project" | "global";
107+
skillSource: SkillProvenance["skillSource"];
108+
} | null {
109+
const home = (process.env.HOME ?? process.env.USERPROFILE ?? "").replace(/\\/g, "/");
110+
const candidateDirs = [
111+
// Project-local dirs first (project shadows global — finding U3)
112+
`${projectDirectory}/.opencode/skill/${skillName}`,
113+
`${projectDirectory}/.opencode/skills/${skillName}`,
114+
`${projectDirectory}/.agents/skills/${skillName}`,
115+
`${projectDirectory}/.claude/skills/${skillName}`,
116+
// Global dirs second
117+
`${home}/.config/opencode/skills/${skillName}`, // Global.Path.config + {skill,skills}/**/SKILL.md
118+
`${home}/.config/opencode/skill/${skillName}`, // singular — OPENCODE_SKILL_PATTERN covers both
119+
`${home}/.agents/skills/${skillName}`, // AGENTS_EXTERNAL_DIR
120+
`${home}/.claude/skills/${skillName}`, // CLAUDE_EXTERNAL_DIR
121+
];
122+
123+
for (const dir of candidateDirs) {
124+
const candidate = `${dir}/SKILL.md`;
125+
if (existsSync(candidate)) {
126+
return {
127+
resolvedPath: candidate,
128+
tier: deriveSkillTier(dir),
129+
skillSource: deriveSkillSource(dir),
130+
};
131+
}
132+
}
133+
134+
return null;
135+
}
136+
87137
// ── Session-scoped skill-load registry ─────────────────────────────────────
88138
// Key: `${sessionId}:${skillId}` — populated in tool.execute.after when
89139
// input.tool === "skill". Cleaned up in onSessionDeleted.

‎packages/plugin/src/hooks/magic-context/hook-handlers.ts‎

Lines changed: 30 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -633,14 +633,38 @@ export function createToolExecuteAfterHook(args: {
633633
// One dynamic import of the provenance module shared by both
634634
// the registry-populate and the injection blocks below
635635
// (lazy-loaded only when the skill tool actually fires).
636-
const { parseSkillProvenance, registryKey } = await import(
637-
"../../features/magic-context/skill-memory/provenance"
638-
);
636+
const { parseSkillProvenance, resolveSkillPathByName, registryKey } =
637+
await import("../../features/magic-context/skill-memory/provenance");
638+
// Compute sessionDir once so it's shared between the
639+
// provenance-resolve and injection blocks below.
640+
// First-turn fallback: skill tool fires before
641+
// sessionDirectoryBySession is populated → fall back
642+
// to args.defaultDirectory.
643+
const sessionDir =
644+
args.sessionDirectoryBySession.get(typedInput.sessionID) ??
645+
args.defaultDirectory;
639646
try {
640647
const { parseFrontmatterConfig } = await import(
641648
"../../features/magic-context/skill-memory/frontmatter"
642649
);
643-
const provenance = parseSkillProvenance(typedOutput.output, skillId);
650+
// PRIMARY path: parse the "Base directory for this skill:" line
651+
let provenance = parseSkillProvenance(typedOutput.output, skillId);
652+
// FALLBACK: name-based disk resolution when the provenance
653+
// line was truncated (MAX_BYTES=51200 cutoff) or absent.
654+
// For large skills (e.g. delegating at 52KB), the line
655+
// sits past the cutoff and is dropped from output.
656+
if (!provenance) {
657+
const resolved = resolveSkillPathByName(skillId, sessionDir);
658+
if (resolved) {
659+
provenance = {
660+
resolvedPath: resolved.resolvedPath,
661+
tier: resolved.tier,
662+
skillSource: resolved.skillSource,
663+
skillId,
664+
loadedAt: Date.now(),
665+
};
666+
}
667+
}
644668
if (provenance) {
645669
let frontmatterConfig:
646670
| import("../../features/magic-context/skill-memory/frontmatter").SkillMemoryConfig
@@ -675,15 +699,8 @@ export function createToolExecuteAfterHook(args: {
675699
registryKey(typedInput.sessionID, skillId),
676700
);
677701
if (registryEntry) {
678-
// First-turn fallback: if the map has no entry yet
679-
// (skill tool fires before sessionDirectoryBySession
680-
// is populated), fall back to args.defaultDirectory.
681-
// Intentional: multi-project / Desktop-launched sessions
682-
// may misattribute on the very first skill call;
683-
// subsequent calls resolve correctly.
684-
const sessionDir =
685-
args.sessionDirectoryBySession.get(typedInput.sessionID) ??
686-
args.defaultDirectory;
702+
// sessionDir was already computed above (shared with the
703+
// provenance-resolve block) — reuse it here.
687704
const projectIdentity = resolveProjectIdentity(sessionDir);
688705
const stashed = typedInput.callID
689706
? (getAndDeleteIntent(

‎packages/plugin/src/hooks/magic-context/skill-memory-injection.test.ts‎

Lines changed: 100 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,21 @@
1-
import { describe, expect, test } from "bun:test";
1+
import { afterAll, describe, expect, test } from "bun:test";
2+
import { mkdirSync, rmSync, writeFileSync } from "node:fs";
3+
import { tmpdir } from "node:os";
24
import { runMigrations } from "../../features/magic-context/migrations";
5+
import {
6+
createSkillLoadRegistry,
7+
registryKey,
8+
} from "../../features/magic-context/skill-memory/provenance";
39
import { recallSkillMemoryBlock } from "../../features/magic-context/skill-memory/recall";
410
import { insertSkillMemoryNote } from "../../features/magic-context/skill-memory/storage";
511
import { initializeDatabase } from "../../features/magic-context/storage-db";
612
import { Database } from "../../shared/sqlite";
713
import { closeQuietly } from "../../shared/sqlite-helpers";
8-
import { maybeInjectSkillMemory } from "../magic-context/hook-handlers";
14+
import {
15+
createIntentByCallIdMap,
16+
createToolExecuteAfterHook,
17+
maybeInjectSkillMemory,
18+
} from "../magic-context/hook-handlers";
919

1020
function makeDb(): Database {
1121
const db = new Database(":memory:");
@@ -210,3 +220,91 @@ describe("maybeInjectSkillMemory", () => {
210220
}
211221
});
212222
});
223+
224+
describe("createToolExecuteAfterHook skill registry (truncation fallback)", () => {
225+
const projectDir = `${tmpdir()}/skill-truncation-test-${Date.now()}`;
226+
const skillDir = `${projectDir}/.opencode/skills/truncated-skill`;
227+
mkdirSync(skillDir, { recursive: true });
228+
writeFileSync(
229+
`${skillDir}/SKILL.md`,
230+
"---\nskill-memory:\n enabled: true\n max_tokens: 1500\n---\n\n# Truncated Skill\n",
231+
);
232+
233+
afterAll(() => {
234+
rmSync(projectDir, { recursive: true, force: true });
235+
});
236+
237+
test("populates registry via name-based fallback when Base-dir line is absent (truncation simulation)", async () => {
238+
// Simulates the bug: MAX_BYTES=51200 truncation drops the
239+
// "Base directory for this skill:" line, making parseSkillProvenance
240+
// return null. The fallback resolveSkillPathByName must populate
241+
// the registry so ctx_skill_note doesn't hard-fail with
242+
// "No recent skill load found... provenance parse failure".
243+
const db = makeDb();
244+
const registry = createSkillLoadRegistry();
245+
const hook = createToolExecuteAfterHook({
246+
db,
247+
channel1StateBySession: new Map(),
248+
skillLoadRegistry: registry,
249+
sessionDirectoryBySession: new Map(),
250+
defaultDirectory: projectDir,
251+
intentByCallId: createIntentByCallIdMap(),
252+
});
253+
254+
// Simulated truncated skill output: NO "Base directory for this skill:" line
255+
const output = {
256+
output: [
257+
'<skill_content name="truncated-skill">',
258+
"# Truncated Skill",
259+
"",
260+
"Some skill content that would normally be long enough",
261+
"to push the provenance line past the 51200-byte cutoff.",
262+
"",
263+
"More content...",
264+
"</skill_content>",
265+
].join("\n"),
266+
};
267+
268+
await hook(
269+
{ tool: "skill", sessionID: "ses_trunc", args: { name: "truncated-skill" } },
270+
output,
271+
);
272+
273+
const entry = registry.get(registryKey("ses_trunc", "truncated-skill"));
274+
// Without the name-based fallback this would be undefined (RED test).
275+
expect(entry).not.toBeUndefined();
276+
expect(entry!.resolvedPath).toBe(`${skillDir}/SKILL.md`);
277+
expect(entry!.tier).toBe("project");
278+
expect(entry!.skillSource).toBe("opencode-project");
279+
expect(entry!.skillId).toBe("truncated-skill");
280+
// Frontmatter should be parsed from the on-disk SKILL.md
281+
expect(entry!.frontmatterConfig).not.toBeNull();
282+
expect(entry!.frontmatterConfig!.enabled).toBe(true);
283+
});
284+
285+
test("registry stays empty when fallback also cannot find SKILL.md", async () => {
286+
const db = makeDb();
287+
const registry = createSkillLoadRegistry();
288+
const hook = createToolExecuteAfterHook({
289+
db,
290+
channel1StateBySession: new Map(),
291+
skillLoadRegistry: registry,
292+
sessionDirectoryBySession: new Map(),
293+
defaultDirectory: projectDir,
294+
intentByCallId: createIntentByCallIdMap(),
295+
});
296+
297+
// Skill that doesn't exist on disk at all
298+
const output = {
299+
output: "# Nonexistent Skill\n\nNo base dir here either.",
300+
};
301+
302+
await hook(
303+
{ tool: "skill", sessionID: "ses_miss", args: { name: "nonexistent-skill" } },
304+
output,
305+
);
306+
307+
const entry = registry.get(registryKey("ses_miss", "nonexistent-skill"));
308+
expect(entry).toBeUndefined();
309+
});
310+
});

0 commit comments

Comments
 (0)