fix(skill-memory): address council findings (disabled-path crash + 2 routing/parser gaps)

Tehan · Tehan · commit 942cd32e4c63 · 2026-07-02T17:20:06.000+02:00
Council review (deepseek/sonnet/gpt-5.5) of PR #181: - Must (consensus rev-2+rev-3): the ctx_skill_note fail-loud guard threw during plugin init when the plugin is disabled (enabled:false OR conflict-disabled) — createSessionHooks returns {magicContext:null} by design, so the unconditional guard crashed the entry module on the disabled path. Gate it on pluginConfig.enabled; pass a throwaway Map to createToolRegistry (which early-returns {} when disabled and never reads it). - Must (rev-3): singular ~/.config/opencode/skill/ global path was misclassified as project tier (opencode's pattern is {skill,skills}/**/SKILL.md) — fixed in deriveSkillTier/deriveSkillSource + the ctx_skill_recall cold-start search list. - Must (rev-3): the frontmatter parser rejected the inline flow-mapping form 'skill-memory: { enabled: true }' — the EXACT form the ctx_skill_recall remediation message and ARCHITECTURE/CONFIGURATION/README advertise. Added inline-mapping parsing so guidance and parser agree. - Should (consensus rev-1+rev-3): recallSkillMemoryBlock swallowed all errors silently — added a log() so FTS/blob corruption is diagnosable (still no-throw). Regression tests: inline frontmatter form (3 cases), singular skill/ global path (2 cases).
diff --git a/packages/plugin/src/features/magic-context/skill-memory/frontmatter.test.ts b/packages/plugin/src/features/magic-context/skill-memory/frontmatter.test.ts
@@ -91,6 +91,30 @@ body`;
         expect(parseFrontmatterConfig(md)?.enabled).toBe(true);
     });
 
+    test("parses the inline flow-mapping form the docs/remediation advertise", () => {
+        // ctx_skill_recall remediation + ARCHITECTURE/CONFIGURATION/README all tell
+        // users to add `skill-memory: { enabled: true }`. The parser MUST accept it
+        // or the guidance is dead-on-arrival.
+        const cfg = parseFrontmatterConfig("---\nskill-memory: { enabled: true }\n---\n# Skill");
+        expect(cfg).not.toBeNull();
+        expect(cfg!.enabled).toBe(true);
+    });
+
+    test("parses inline flow-mapping with multiple keys", () => {
+        const cfg = parseFrontmatterConfig(
+            "---\nskill-memory: { enabled: true, max_tokens: 2000, dedup_threshold: 0.8 }\n---\nbody",
+        );
+        expect(cfg!.enabled).toBe(true);
+        expect(cfg!.max_tokens).toBe(2000);
+        expect(cfg!.dedup_threshold).toBe(0.8);
+    });
+
+    test("inline form with enabled: false stays inert", () => {
+        expect(
+            parseFrontmatterConfig("---\nskill-memory: { enabled: false }\n---\nbody"),
+        ).toBeNull();
+    });
+
     test("a plain quoted scalar still enables", () => {
         const md = `---\nskill-memory:\n  enabled: "true"\n---\nbody`;
         expect(parseFrontmatterConfig(md)?.enabled).toBe(true);
diff --git a/packages/plugin/src/features/magic-context/skill-memory/frontmatter.ts b/packages/plugin/src/features/magic-context/skill-memory/frontmatter.ts
@@ -83,6 +83,25 @@ function extractSkillMemoryBlock(fmText: string): Record<string, unknown> | null
 
     for (const line of lines) {
         if (!inSkillMemory) {
+            // Inline flow-mapping form on the header line:
+            //   `skill-memory: { enabled: true, max_tokens: 2000 }`
+            // This is the form the ctx_skill_recall remediation message and the
+            // root docs advertise, so it MUST parse — otherwise a user following
+            // the guidance silently gets an inert config. Parse the {...} body
+            // into the same flat map the block form produces, then stop (a flow
+            // mapping is self-contained on one line).
+            const inlineMatch = line.match(/^skill-memory:\s*\{(.*)\}\s*(#.*)?$/);
+            if (inlineMatch) {
+                found = true;
+                for (const pair of splitFlowEntries(inlineMatch[1])) {
+                    const sep = pair.indexOf(":");
+                    if (sep < 0) continue;
+                    const key = pair.slice(0, sep).trim();
+                    if (!/^\w+$/.test(key)) continue;
+                    result[key] = parseYamlScalar(pair.slice(sep + 1).trim());
+                }
+                break;
+            }
             // Tolerate a trailing inline comment after the block header
             // (`skill-memory:   # motor memory`), which is valid YAML.
             if (/^skill-memory:\s*(#.*)?$/.test(line)) {
@@ -105,6 +124,37 @@ function extractSkillMemoryBlock(fmText: string): Record<string, unknown> | null
     return found ? result : null;
 }
 
+/**
+ * Split a YAML flow-mapping body (the text inside `{...}`) on top-level commas,
+ * leaving quoted segments intact. Minimal — the skill-memory config is a flat
+ * map of scalar values, so we don't need nested {}/[] handling.
+ */
+function splitFlowEntries(body: string): string[] {
+    const entries: string[] = [];
+    let current = "";
+    let quote: '"' | "'" | null = null;
+    for (const ch of body) {
+        if (quote) {
+            current += ch;
+            if (ch === quote) quote = null;
+            continue;
+        }
+        if (ch === '"' || ch === "'") {
+            quote = ch;
+            current += ch;
+            continue;
+        }
+        if (ch === ",") {
+            entries.push(current);
+            current = "";
+            continue;
+        }
+        current += ch;
+    }
+    if (current.trim()) entries.push(current);
+    return entries;
+}
+
 function parseYamlScalar(raw: string): unknown {
     // Strip an inline `# comment` for UNQUOTED scalars (YAML requires whitespace
     // before the `#`). Quoted values keep their content verbatim so a literal
diff --git a/packages/plugin/src/features/magic-context/skill-memory/provenance.test.ts b/packages/plugin/src/features/magic-context/skill-memory/provenance.test.ts
@@ -51,4 +51,22 @@ describe("parseSkillProvenance", () => {
         expect(result!.tier).toBe("project");
         expect(result!.skillSource).toBe("opencode-project");
     });
+
+    test("handles the SINGULAR ~/.config/opencode/skill/ global path (OPENCODE_SKILL_PATTERN covers both)", () => {
+        // Regression: opencode's pattern is {skill,skills}/**/SKILL.md, so the
+        // global config dir resolves under singular `skill/` too. Before the fix,
+        // deriveSkillTier classified it as project → notes written to the wrong
+        // partition + cold recall couldn't find SKILL.md.
+        const output = `Base directory for this skill: file://${HOME}/.config/opencode/skill/my-skill`;
+        const result = parseSkillProvenance(output, "my-skill");
+        expect(result!.tier).toBe("global");
+        expect(result!.skillSource).toBe("opencode-global");
+    });
+
+    test("handles the plural ~/.config/opencode/skills/ global path", () => {
+        const output = `Base directory for this skill: file://${HOME}/.config/opencode/skills/my-skill`;
+        const result = parseSkillProvenance(output, "my-skill");
+        expect(result!.tier).toBe("global");
+        expect(result!.skillSource).toBe("opencode-global");
+    });
 });
diff --git a/packages/plugin/src/features/magic-context/skill-memory/provenance.ts b/packages/plugin/src/features/magic-context/skill-memory/provenance.ts
@@ -42,7 +42,10 @@ export function deriveSkillTier(absDir: string): "project" | "global" {
     //   ~/.claude/skills/          — via CLAUDE_EXTERNAL_DIR + skills/**/SKILL.md
     const home = (process.env.HOME ?? process.env.USERPROFILE ?? "").replace(/\\/g, "/");
     if (
+        // opencode's OPENCODE_SKILL_PATTERN is `{skill,skills}/**/SKILL.md`, so
+        // the global config dir resolves under BOTH singular and plural.
         absDir.startsWith(`${home}/.config/opencode/skills/`) ||
+        absDir.startsWith(`${home}/.config/opencode/skill/`) ||
         absDir.startsWith(`${home}/.agents/skills/`) ||
         absDir.startsWith(`${home}/.claude/skills/`)
     ) {
@@ -55,7 +58,11 @@ export function deriveSkillSource(
     absDir: string,
 ): "opencode-project" | "opencode-global" | "claude-skills" | "agents-skills" {
     const home = (process.env.HOME ?? process.env.USERPROFILE ?? "").replace(/\\/g, "/");
-    if (absDir.startsWith(`${home}/.config/opencode/skills/`)) return "opencode-global";
+    if (
+        absDir.startsWith(`${home}/.config/opencode/skills/`) ||
+        absDir.startsWith(`${home}/.config/opencode/skill/`)
+    )
+        return "opencode-global";
     if (absDir.startsWith(`${home}/.claude/skills/`)) return "claude-skills";
     if (absDir.includes("/.agents/skills/")) return "agents-skills";
     // Both singular .opencode/skill/ and plural .opencode/skills/ are valid —
diff --git a/packages/plugin/src/features/magic-context/skill-memory/recall.ts b/packages/plugin/src/features/magic-context/skill-memory/recall.ts
@@ -1,3 +1,4 @@
+import { log } from "../../../shared/logger";
 import type { Database } from "../../../shared/sqlite";
 import { cosineSimilarity } from "../memory/cosine-similarity";
 import { embedTextForProject } from "../memory/embedding";
@@ -277,7 +278,14 @@ export async function recallSkillMemoryBlock(
         );
         const selected = budgetFill(ordered, maxTokens, maxPinned);
         return finalize("fts5-fallback", selected);
-    } catch {
+    } catch (err) {
+        // Cache-safe + non-choking: never throw from recall (a thrown error here
+        // would surface in the skill tool result). But log it — a broken FTS
+        // table or bad vector blob would otherwise look identical to "no notes",
+        // making durable skill-memory data loss undiagnosable.
+        log(
+            `[skill-memory] recallSkillMemoryBlock failed for skill "${opts.skill}": ${err instanceof Error ? err.message : String(err)}`,
+        );
         return "";
     }
 }
diff --git a/packages/plugin/src/index.ts b/packages/plugin/src/index.ts
@@ -157,7 +157,14 @@ const server: Plugin = async (ctx) => {
     // every note would return "No recent skill load found" — the exact
     // opposite of "fail loud". Catch a wiring regression at startup, not
     // at the first ctx_skill_note call from an agent.
-    if (!hooks.magicContext?.skillLoadRegistry) {
+    //
+    // ONLY when the plugin is enabled: when disabled by config (`enabled: false`)
+    // or by a detected conflict (sets enabled=false above), createSessionHooks
+    // returns `{ magicContext: null }` by design and no tools are exposed, so a
+    // missing registry is expected — throwing here would crash plugin init on
+    // the disabled path (an entry-module throw — the exact load-crash class this
+    // plugin must avoid).
+    if (pluginConfig.enabled && !hooks.magicContext?.skillLoadRegistry) {
         throw new Error(
             "[magic-context] ctx_skill_note registration failed: " +
                 "hooks.magicContext.skillLoadRegistry is missing. " +
@@ -167,7 +174,11 @@ const server: Plugin = async (ctx) => {
     const tools = createToolRegistry({
         ctx,
         pluginConfig,
-        skillLoadRegistry: hooks.magicContext.skillLoadRegistry,
+        // Disabled path: magicContext is null and createToolRegistry early-returns
+        // {} without using the registry. Pass a throwaway Map so the argument
+        // expression never dereferences null (createToolRegistry never reads it
+        // when disabled).
+        skillLoadRegistry: hooks.magicContext?.skillLoadRegistry ?? new Map(),
     });
 
     // v22 deferred legacy-memory identity backfill. createSessionHooks() opens
diff --git a/packages/plugin/src/tools/ctx-skill-recall/tools.ts b/packages/plugin/src/tools/ctx-skill-recall/tools.ts
@@ -128,6 +128,7 @@ export function createCtxSkillRecallTool(deps: CtxSkillRecallToolDeps): ToolDefi
                     `${projectDirectory}/.claude/skills/${args.skill}`,
                     // Global dirs second
                     `${home}/.config/opencode/skills/${args.skill}`, // via Global.Path.config + {skill,skills}/**/SKILL.md
+                    `${home}/.config/opencode/skill/${args.skill}`, // singular — OPENCODE_SKILL_PATTERN covers both
                     `${home}/.agents/skills/${args.skill}`, // via AGENTS_EXTERNAL_DIR
                     `${home}/.claude/skills/${args.skill}`, // via CLAUDE_EXTERNAL_DIR
                 ];
