fix(skill-memory): harden name-based fallback per review findings

- Fallback only resolves project-tier candidates when the session directory
  is authoritative (sessionDirectoryBySession hit); a launch-dir guess must
  not register a wrong same-named project skill (cubic P1). Global tier
  resolves from HOME regardless.
- Ancestor walk for project-tier candidates (nearest-first, bounded at 20
  levels, stops at $HOME/root) — sessions rooted in a worktree subdir now
  find repo-root project skills, matching opencode's discoverSkills walk-up.
- DB cleanup (try/finally closeQuietly) in the truncation tests.
- Drop redundant test-provider reset in reembed.test.ts.

Author: Tehan

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

@@ -272,4 +272,81 @@ describe("resolveSkillPathByName", () => {
             rmSync(globalHome, { recursive: true, force: true });
         }
     });
+
+    test("null projectDirectory skips project candidates, resolves global only", () => {
+        const savedHome = process.env.HOME;
+        const projectRoot = `${tmpdir()}/skill-null-proj-${Date.now()}`;
+        const globalHome = `${tmpdir()}/skill-null-global-${Date.now()}`;
+        try {
+            // Create a project-local skill — should be IGNORED when null is passed
+            const projectSkillDir = `${projectRoot}/.opencode/skills/null-guard-skill`;
+            mkdirSync(projectSkillDir, { recursive: true });
+            writeFileSync(`${projectSkillDir}/SKILL.md`, "---\n---\n\n# Project\n");
+
+            // Create a global skill with a DIFFERENT name — should resolve
+            const globalSkillDir = `${globalHome}/.config/opencode/skills/null-global-skill`;
+            mkdirSync(globalSkillDir, { recursive: true });
+            writeFileSync(`${globalSkillDir}/SKILL.md`, "---\n---\n\n# Global\n");
+
+            process.env.HOME = globalHome;
+            // Pass null → project candidates are skipped
+            const resultNull = resolveSkillPathByName("null-guard-skill", null);
+            // The project skill exists but should NOT be found (null skips project)
+            expect(resultNull).toBeNull();
+
+            // The global skill with different name SHOULD resolve
+            const resultGlobal = resolveSkillPathByName("null-global-skill", null);
+            expect(resultGlobal).not.toBeNull();
+            expect(resultGlobal!.resolvedPath).toBe(`${globalSkillDir}/SKILL.md`);
+            expect(resultGlobal!.tier).toBe("global");
+        } finally {
+            process.env.HOME = savedHome;
+            rmSync(projectRoot, { recursive: true, force: true });
+            rmSync(globalHome, { recursive: true, force: true });
+        }
+    });
+
+    test("finds skill from ancestor walk (subdir misses, root has it)", () => {
+        const root = `${tmpdir()}/skill-ancestor-root-${Date.now()}`;
+        const subdir = `${root}/sub/deep/here`;
+        try {
+            // Skill at root level: .opencode/skills/ancestor-skill/SKILL.md
+            const skillDir = `${root}/.opencode/skills/ancestor-skill`;
+            mkdirSync(subdir, { recursive: true });
+            mkdirSync(skillDir, { recursive: true });
+            writeFileSync(`${skillDir}/SKILL.md`, "---\n---\n\n# Ancestor\n");
+
+            // Call from deep subdir — ancestor walk should find it at root
+            const result = resolveSkillPathByName("ancestor-skill", subdir);
+            expect(result).not.toBeNull();
+            expect(result!.resolvedPath).toBe(`${skillDir}/SKILL.md`);
+            expect(result!.tier).toBe("project");
+        } finally {
+            rmSync(root, { recursive: true, force: true });
+        }
+    });
+
+    test("ancestor walk: nearest ancestor wins when two levels both have the skill", () => {
+        const root = `${tmpdir()}/skill-nearest-wins-${Date.now()}`;
+        const child = `${root}/child`;
+        try {
+            // Root-level skill (farther ancestor)
+            const rootSkillDir = `${root}/.opencode/skills/nearest-skill`;
+            mkdirSync(rootSkillDir, { recursive: true });
+            writeFileSync(`${rootSkillDir}/SKILL.md`, "---\n---\n\n# Root\n");
+
+            // Child-level skill (nearer ancestor) — should win
+            const childSkillDir = `${child}/.opencode/skills/nearest-skill`;
+            mkdirSync(childSkillDir, { recursive: true });
+            writeFileSync(`${childSkillDir}/SKILL.md`, "---\n---\n\n# Child\n");
+
+            // Call from child — nearest ancestor (child dir) should win
+            const result = resolveSkillPathByName("nearest-skill", child);
+            expect(result).not.toBeNull();
+            expect(result!.resolvedPath).toBe(`${childSkillDir}/SKILL.md`);
+            expect(result!.tier).toBe("project");
+        } finally {
+            rmSync(root, { recursive: true, force: true });
+        }
+    });
 });

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

@@ -92,35 +92,73 @@ export function deriveSkillSource(
  * truncated from the skill tool output (MAX_BYTES=51200 cutoff).
  *
  * Search order matches opencode's `discoverSkills()`:
- *   - Project dirs first (project shadows global — finding U3)
- *   - Global dirs second
+ *   - Project-dir ancestors first (project shadows global — finding U3).
+ *     When `projectDirectory` is non-null, walks UP the ancestor chain from
+ *     that directory (each level checking the 4 project patterns in order —
+ *     nearest dir first), stopping at $HOME or the filesystem root, with a
+ *     hard bound of 20 levels.
+ *   - Global dirs second (always checked, regardless of projectDirectory).
+ *
+ * When `projectDirectory` is null, project-tier candidates are SKIPPED
+ * entirely — only global dirs are searched. Callers should pass null when
+ * the directory is a guess (e.g. Desktop-launched session without an
+ * authoritative sessionDirectoryBySession entry); a wrong dir could resolve
+ * a same-named project skill and poison the registry.
  *
  * Does NOT read SKILL.md content — callers are responsible for their own
  * frontmatter parsing.
  */
 export function resolveSkillPathByName(
     skillName: string,
-    projectDirectory: string,
+    projectDirectory: string | null,
 ): {
     resolvedPath: string;
     tier: "project" | "global";
     skillSource: SkillProvenance["skillSource"];
 } | null {
     const home = (process.env.HOME ?? process.env.USERPROFILE ?? "").replace(/\\/g, "/");
-    const candidateDirs = [
-        // Project-local dirs first (project shadows global — finding U3)
-        `${projectDirectory}/.opencode/skill/${skillName}`,
-        `${projectDirectory}/.opencode/skills/${skillName}`,
-        `${projectDirectory}/.agents/skills/${skillName}`,
-        `${projectDirectory}/.claude/skills/${skillName}`,
-        // Global dirs second
+    const projectPatterns = [
+        ".opencode/skill",
+        ".opencode/skills",
+        ".agents/skills",
+        ".claude/skills",
+    ] as const;
+    const globalDirs = [
         `${home}/.config/opencode/skills/${skillName}`, // Global.Path.config + {skill,skills}/**/SKILL.md
         `${home}/.config/opencode/skill/${skillName}`, // singular — OPENCODE_SKILL_PATTERN covers both
         `${home}/.agents/skills/${skillName}`, // AGENTS_EXTERNAL_DIR
         `${home}/.claude/skills/${skillName}`, // CLAUDE_EXTERNAL_DIR
     ];
 
-    for (const dir of candidateDirs) {
+    // ── Project-tier ancestor walk ──────────────────────────────────────
+    if (projectDirectory !== null) {
+        const normalizedHome = home.endsWith("/") ? home.slice(0, -1) : home;
+        let current = projectDirectory;
+        // eslint-disable-next-line @typescript-eslint/no-unused-vars
+        for (let depth = 0; depth < 20 && current.length > 0; depth++) {
+            // Stop at $HOME (exclusive) or filesystem root
+            if (current === normalizedHome || current === "/") break;
+
+            for (const pat of projectPatterns) {
+                const dir = `${current}/${pat}/${skillName}`;
+                const candidate = `${dir}/SKILL.md`;
+                if (existsSync(candidate)) {
+                    return {
+                        resolvedPath: candidate,
+                        tier: deriveSkillTier(dir),
+                        skillSource: deriveSkillSource(dir),
+                    };
+                }
+            }
+
+            // Walk up: strip last path segment (or empty string if none left)
+            const sep = current.lastIndexOf("/");
+            current = sep > 0 ? current.slice(0, sep) : "";
+        }
+    }
+
+    // ── Global dirs (always checked) ────────────────────────────────────
+    for (const dir of globalDirs) {
         const candidate = `${dir}/SKILL.md`;
         if (existsSync(candidate)) {
             return {

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

@@ -50,7 +50,8 @@ function registerReembedTestProject(
 describe("reembed", () => {
     test("reembedStaleSkillNotes fills NULL embeddings (bounded, idempotent)", async () => {
         _resetProjectEmbeddingRegistryForTests();
-        _setTestProviderFactoryForProject(null);
+        // installReembedTestProvider() unconditionally sets the factory, so
+        // a null-reset before it is redundant.
         installReembedTestProvider();
 
         const db = new Database(":memory:");

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

@@ -635,14 +635,16 @@ export function createToolExecuteAfterHook(args: {
                     // (lazy-loaded only when the skill tool actually fires).
                     const { parseSkillProvenance, resolveSkillPathByName, registryKey } =
                         await import("../../features/magic-context/skill-memory/provenance");
-                    // Compute sessionDir once so it's shared between the
-                    // provenance-resolve and injection blocks below.
-                    // First-turn fallback: skill tool fires before
-                    // sessionDirectoryBySession is populated → fall back
-                    // to args.defaultDirectory.
-                    const sessionDir =
-                        args.sessionDirectoryBySession.get(typedInput.sessionID) ??
-                        args.defaultDirectory;
+                    // Split sessionDir into two signals:
+                    //   - mappedDir: authoritative (from sessionDirectoryBySession)
+                    //     → fed to the fallback's project-tier resolution.
+                    //   - sessionDir: the injection block's fallback (mappedDir ?? defaultDirectory)
+                    //     → same pre-existing behaviour, not changed here.
+                    // When mappedDir is undefined (map miss), the fallback receives null
+                    // and SKIPS project-tier candidates — a wrong launch-dir guess
+                    // must not resolve a same-named project skill and poison the registry.
+                    const mappedDir = args.sessionDirectoryBySession.get(typedInput.sessionID);
+                    const sessionDir = mappedDir ?? args.defaultDirectory;
                     try {
                         const { parseFrontmatterConfig } = await import(
                             "../../features/magic-context/skill-memory/frontmatter"
@@ -654,7 +656,11 @@ export function createToolExecuteAfterHook(args: {
                         // For large skills (e.g. delegating at 52KB), the line
                         // sits past the cutoff and is dropped from output.
                         if (!provenance) {
-                            const resolved = resolveSkillPathByName(skillId, sessionDir);
+                            // mappedDir is authoritative (from sessionDirectoryBySession);
+                            // null means "don't resolve project-tier candidates" — the
+                            // session directory is a guess and could resolve the wrong
+                            // same-named project skill.
+                            const resolved = resolveSkillPathByName(skillId, mappedDir ?? null);
                             if (resolved) {
                                 provenance = {
                                     resolvedPath: resolved.resolvedPath,

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

@@ -242,44 +242,48 @@ describe("createToolExecuteAfterHook skill registry (truncation fallback)", () =
         // "No recent skill load found... provenance parse failure".
         const db = makeDb();
         const registry = createSkillLoadRegistry();
+        // Seed the map so project-tier resolution is authoritative (P1 fix).
         const hook = createToolExecuteAfterHook({
             db,
             channel1StateBySession: new Map(),
             skillLoadRegistry: registry,
-            sessionDirectoryBySession: new Map(),
-            defaultDirectory: projectDir,
+            sessionDirectoryBySession: new Map([["ses_trunc", projectDir]]),
+            defaultDirectory: "/tmp/irrelevant",
             intentByCallId: createIntentByCallIdMap(),
         });
+        try {
+            // Simulated truncated skill output: NO "Base directory for this skill:" line
+            const output = {
+                output: [
+                    '<skill_content name="truncated-skill">',
+                    "# Truncated Skill",
+                    "",
+                    "Some skill content that would normally be long enough",
+                    "to push the provenance line past the 51200-byte cutoff.",
+                    "",
+                    "More content...",
+                    "</skill_content>",
+                ].join("\n"),
+            };
 
-        // Simulated truncated skill output: NO "Base directory for this skill:" line
-        const output = {
-            output: [
-                '<skill_content name="truncated-skill">',
-                "# Truncated Skill",
-                "",
-                "Some skill content that would normally be long enough",
-                "to push the provenance line past the 51200-byte cutoff.",
-                "",
-                "More content...",
-                "</skill_content>",
-            ].join("\n"),
-        };
-
-        await hook(
-            { tool: "skill", sessionID: "ses_trunc", args: { name: "truncated-skill" } },
-            output,
-        );
+            await hook(
+                { tool: "skill", sessionID: "ses_trunc", args: { name: "truncated-skill" } },
+                output,
+            );
 
-        const entry = registry.get(registryKey("ses_trunc", "truncated-skill"));
-        // Without the name-based fallback this would be undefined (RED test).
-        expect(entry).not.toBeUndefined();
-        expect(entry!.resolvedPath).toBe(`${skillDir}/SKILL.md`);
-        expect(entry!.tier).toBe("project");
-        expect(entry!.skillSource).toBe("opencode-project");
-        expect(entry!.skillId).toBe("truncated-skill");
-        // Frontmatter should be parsed from the on-disk SKILL.md
-        expect(entry!.frontmatterConfig).not.toBeNull();
-        expect(entry!.frontmatterConfig!.enabled).toBe(true);
+            const entry = registry.get(registryKey("ses_trunc", "truncated-skill"));
+            // Without the name-based fallback this would be undefined (RED test).
+            expect(entry).not.toBeUndefined();
+            expect(entry!.resolvedPath).toBe(`${skillDir}/SKILL.md`);
+            expect(entry!.tier).toBe("project");
+            expect(entry!.skillSource).toBe("opencode-project");
+            expect(entry!.skillId).toBe("truncated-skill");
+            // Frontmatter should be parsed from the on-disk SKILL.md
+            expect(entry!.frontmatterConfig).not.toBeNull();
+            expect(entry!.frontmatterConfig!.enabled).toBe(true);
+        } finally {
+            closeQuietly(db);
+        }
     });
 
     test("registry stays empty when fallback also cannot find SKILL.md", async () => {
@@ -289,22 +293,65 @@ describe("createToolExecuteAfterHook skill registry (truncation fallback)", () =
             db,
             channel1StateBySession: new Map(),
             skillLoadRegistry: registry,
-            sessionDirectoryBySession: new Map(),
-            defaultDirectory: projectDir,
+            sessionDirectoryBySession: new Map([["ses_miss", projectDir]]),
+            defaultDirectory: "/tmp/irrelevant",
             intentByCallId: createIntentByCallIdMap(),
         });
+        try {
+            // Skill that doesn't exist on disk at all
+            const output = {
+                output: "# Nonexistent Skill\n\nNo base dir here either.",
+            };
 
-        // Skill that doesn't exist on disk at all
-        const output = {
-            output: "# Nonexistent Skill\n\nNo base dir here either.",
-        };
+            await hook(
+                { tool: "skill", sessionID: "ses_miss", args: { name: "nonexistent-skill" } },
+                output,
+            );
 
-        await hook(
-            { tool: "skill", sessionID: "ses_miss", args: { name: "nonexistent-skill" } },
-            output,
-        );
+            const entry = registry.get(registryKey("ses_miss", "nonexistent-skill"));
+            expect(entry).toBeUndefined();
+        } finally {
+            closeQuietly(db);
+        }
+    });
 
-        const entry = registry.get(registryKey("ses_miss", "nonexistent-skill"));
-        expect(entry).toBeUndefined();
+    test("map-miss does NOT resolve a project-local skill (P1 guard)", async () => {
+        // When sessionDirectoryBySession has no entry for the session,
+        // the fallback receives null as projectDirectory and MUST NOT
+        // resolve project-tier candidates — the defaultDirectory is a guess.
+        const db = makeDb();
+        const registry = createSkillLoadRegistry();
+        // Empty map (no entry for ses_guess) → mappedDir is undefined → null → skip project
+        const hook = createToolExecuteAfterHook({
+            db,
+            channel1StateBySession: new Map(),
+            skillLoadRegistry: registry,
+            sessionDirectoryBySession: new Map(),
+            defaultDirectory: projectDir, // projectDir HAS the skill, but it's a guess
+            intentByCallId: createIntentByCallIdMap(),
+        });
+        try {
+            const output = {
+                output: [
+                    '<skill_content name="truncated-skill">',
+                    "# Truncated Skill",
+                    "",
+                    "Some content...",
+                    "</skill_content>",
+                ].join("\n"),
+            };
+
+            await hook(
+                { tool: "skill", sessionID: "ses_guess", args: { name: "truncated-skill" } },
+                output,
+            );
+
+            // Registry must NOT contain the project-local skill — the fallback
+            // skips project candidates when mappedDir is null.
+            const entry = registry.get(registryKey("ses_guess", "truncated-skill"));
+            expect(entry).toBeUndefined();
+        } finally {
+            closeQuietly(db);
+        }
     });
 });