Skip to content

Commit 1a5dd80

Browse files
author
Tehan
committed
fix(skill-memory): address PR review findings (cubic + greptile)
- Remove committed <<<<<<< HEAD conflict marker in CONFIGURATION.md (P1) - Move injectSkillIntentParam before the lastChatContext guard so the intent param is advertised even on tool.definition flights before first chat.message - Key intentByCallId by sessionID:callID + prefix-prune on session delete so a concurrent session's delete can't evict another session's in-flight intents - Log silent catch in promoteSkillObservations (observability for dropped writes) - Anchor frontmatter regex to start-of-file (drop m flag) so a later --- rule can't be misparsed; strip inline # comments from unquoted YAML scalars + block header - Scope distill report SQL to ('<identity>','*') instead of non-deterministic LIMIT 1 - Don't truncate skill name in TC: skill(<name>) marker (identity key); sanitize newlines/control chars - Normalize backslash->slash after fileURLToPath for Windows provenance checks - FTS self-heal rebuild in initializeDatabase when skill_memory_fts is empty but skill_memory has rows - Move ctx_skill_recall _test* DI fields to a separate test-only deps type - Hoist the shared registryKey dynamic import (one import, both blocks) Pushback: reembed pre-step errors are already logged (task-executor.ts) — the non-blocking try/catch is by design (failure leaves notes on the FTS rung).
1 parent dccab5e commit 1a5dd80

14 files changed

Lines changed: 199 additions & 58 deletions

File tree

‎packages/plugin/src/features/magic-context/dreamer/task-prompts.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -386,13 +386,14 @@ function buildDistillSkillMemoryPrompt(projectPath: string): string {
386386
- Merge (action="distill" + merge), prune, and promote are P3 / NOT YET IMPLEMENTED. Do NOT call ctx_skill_note with action="distill".
387387
388388
### Your task: produce a short read-only summary of skill-memory corpus health
389-
1. Query aggregate counts and flag obvious issues:
389+
1. Query aggregate counts and flag obvious issues (scoped to THIS project's
390+
own notes plus the cross-project global '*' partition):
390391
\`\`\`sql
391392
SELECT skill_id, tier, COUNT(*) as note_count,
392393
SUM(CASE WHEN pinned = 1 THEN 1 ELSE 0 END) as pinned_count,
393394
SUM(CASE WHEN intent_embedding IS NULL OR delta_embedding IS NULL THEN 1 ELSE 0 END) as missing_embedding_count
394395
FROM skill_memory
395-
WHERE project_identity = (SELECT project_identity FROM skill_memory LIMIT 1)
396+
WHERE project_identity IN ('${projectPath}', '*')
396397
GROUP BY skill_id, tier
397398
ORDER BY note_count DESC
398399
LIMIT 20;

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

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,4 +69,32 @@ body`;
6969
const cfg = parseFrontmatterConfig(md);
7070
expect(cfg?.ranking_relevance).toBeUndefined();
7171
});
72+
73+
test("does NOT misparse a later --- horizontal rule as frontmatter (start-anchored)", () => {
74+
// No real frontmatter; a horizontal-rule pair appears mid-document. With
75+
// an `m`-flagged regex `^` would match the rule's line start and capture
76+
// the block between the rules as config. Must return null.
77+
const md = `# Skill\n\nSome prose.\n\n---\nskill-memory:\n enabled: true\n---\n\nMore prose.`;
78+
expect(parseFrontmatterConfig(md)).toBeNull();
79+
});
80+
81+
test("honors enabled: true with a trailing inline comment", () => {
82+
const md = `---\nskill-memory:\n enabled: true # motor memory on\n max_tokens: 2000 # bump it\n---\nbody`;
83+
const cfg = parseFrontmatterConfig(md);
84+
expect(cfg).not.toBeNull();
85+
expect(cfg!.enabled).toBe(true);
86+
expect(cfg!.max_tokens).toBe(2000);
87+
});
88+
89+
test("honors an inline comment on the skill-memory block header", () => {
90+
const md = `---\nskill-memory: # procedural recall\n enabled: true\n---\nbody`;
91+
expect(parseFrontmatterConfig(md)?.enabled).toBe(true);
92+
});
93+
94+
test("does not strip a '#' inside a quoted scalar", () => {
95+
const md = `---\nskill-memory:\n enabled: "true"\n---\nbody`;
96+
// "true" (quoted) still enables; the quote-strip path runs after the
97+
// unquoted-only comment strip, so quoted values are untouched.
98+
expect(parseFrontmatterConfig(md)?.enabled).toBe(true);
99+
});
72100
});

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

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,11 @@ export interface SkillMemoryConfig {
1616
ranking_hit?: number;
1717
}
1818

19-
const FRONTMATTER_REGEX = /^---\r?\n([\s\S]*?)\r?\n---/m;
19+
// Anchored to the very start of the file (NO `m` flag): frontmatter is only
20+
// valid as the first bytes of the document. With `m`, `^` matches any line
21+
// start, so a later `--- ... ---` block (e.g. a markdown horizontal rule) could
22+
// be misparsed as config.
23+
const FRONTMATTER_REGEX = /^---\r?\n([\s\S]*?)\r?\n---/;
2024

2125
export function parseFrontmatterConfig(content: string): SkillMemoryConfig | null {
2226
try {
@@ -76,7 +80,9 @@ function extractSkillMemoryBlock(fmText: string): Record<string, unknown> | null
7680

7781
for (const line of lines) {
7882
if (!inSkillMemory) {
79-
if (/^skill-memory:\s*$/.test(line)) {
83+
// Tolerate a trailing inline comment after the block header
84+
// (`skill-memory: # motor memory`), which is valid YAML.
85+
if (/^skill-memory:\s*(#.*)?$/.test(line)) {
8086
inSkillMemory = true;
8187
found = true;
8288
}
@@ -97,6 +103,17 @@ function extractSkillMemoryBlock(fmText: string): Record<string, unknown> | null
97103
}
98104

99105
function parseYamlScalar(raw: string): unknown {
106+
// Strip an inline `# comment` for UNQUOTED scalars (YAML requires whitespace
107+
// before the `#`). Quoted values keep their content verbatim so a literal
108+
// "#" inside quotes survives. Without this, `enabled: true # on` would parse
109+
// as the string "true # on" and silently fail the strict true/false check.
110+
const isQuoted =
111+
(raw.startsWith('"') && raw.endsWith('"')) || (raw.startsWith("'") && raw.endsWith("'"));
112+
if (!isQuoted) {
113+
const commentIdx = raw.search(/\s#/);
114+
if (commentIdx >= 0) raw = raw.slice(0, commentIdx).trim();
115+
}
116+
100117
if (raw === "true") return true;
101118
if (raw === "false") return false;
102119
if (raw === "null" || raw === "~") return null;

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

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import { log } from "../../../shared/logger";
12
import type { Database } from "../../../shared/sqlite";
23
import { computeNormalizedHash } from "../memory/normalize-hash";
34
import { bumpHitCount, findExistingNote, insertSkillMemoryNote, partitionKey } from "./storage";
@@ -51,8 +52,13 @@ export function promoteSkillObservations(
5152
createdAt: Date.now(),
5253
});
5354
if (id !== null) written++;
54-
} catch {
55-
// Best-effort: one bad observation must not block the publish.
55+
} catch (err) {
56+
// Best-effort: one bad observation must not block the publish, but
57+
// log it so silent persistence failures (schema drift, DB lock,
58+
// constraint violation) remain observable.
59+
log(
60+
`[skill-memory] promoteSkillObservations: skipped observation for skill "${obs.skillId}" — ${err instanceof Error ? err.message : String(err)}`,
61+
);
5662
}
5763
}
5864

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

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,11 @@ export function parseSkillProvenance(output: string, skillId: string): SkillProv
1919
const fileUrl = match[1].trim();
2020
let absDir: string;
2121
try {
22-
absDir = fileURLToPath(new URL(fileUrl));
22+
// Normalize OS-native separators to forward slashes: on Windows
23+
// fileURLToPath yields backslash paths, which would fail the
24+
// forward-slash startsWith/includes tier/source checks below and
25+
// misclassify global skills as project-local.
26+
absDir = fileURLToPath(new URL(fileUrl)).replace(/\\/g, "/");
2327
} catch {
2428
return null;
2529
}
@@ -36,7 +40,7 @@ export function deriveSkillTier(absDir: string): "project" | "global" {
3640
// ~/.config/opencode/skills/ — via config.directories() + {skill,skills}/**/SKILL.md
3741
// ~/.agents/skills/ — via AGENTS_EXTERNAL_DIR + skills/**/SKILL.md
3842
// ~/.claude/skills/ — via CLAUDE_EXTERNAL_DIR + skills/**/SKILL.md
39-
const home = process.env.HOME ?? process.env.USERPROFILE ?? "";
43+
const home = (process.env.HOME ?? process.env.USERPROFILE ?? "").replace(/\\/g, "/");
4044
if (
4145
absDir.startsWith(`${home}/.config/opencode/skills/`) ||
4246
absDir.startsWith(`${home}/.agents/skills/`) ||
@@ -50,7 +54,7 @@ export function deriveSkillTier(absDir: string): "project" | "global" {
5054
export function deriveSkillSource(
5155
absDir: string,
5256
): "opencode-project" | "opencode-global" | "claude-skills" | "agents-skills" {
53-
const home = process.env.HOME ?? process.env.USERPROFILE ?? "";
57+
const home = (process.env.HOME ?? process.env.USERPROFILE ?? "").replace(/\\/g, "/");
5458
if (absDir.startsWith(`${home}/.config/opencode/skills/`)) return "opencode-global";
5559
if (absDir.startsWith(`${home}/.claude/skills/`)) return "claude-skills";
5660
if (absDir.includes("/.agents/skills/")) return "agents-skills";

‎packages/plugin/src/features/magic-context/storage-db.ts‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1088,6 +1088,26 @@ CREATE INDEX IF NOT EXISTS idx_dream_queue_pending ON dream_queue(started_at, en
10881088
CREATE INDEX IF NOT EXISTS idx_message_history_index_updated_at ON message_history_index(updated_at);
10891089
`);
10901090

1091+
// Self-heal: backfill skill_memory_fts if it's empty while skill_memory has
1092+
// rows. The CREATE TABLE/TRIGGER block above only indexes FUTURE writes; rows
1093+
// that predate the FTS table (e.g. a DB where v50 ran but v51 hadn't, or a
1094+
// lost migration row) would be invisible to FTS rung-3 recall until re-saved.
1095+
// Guarded so this fires once (on the gap), not on every boot. Mirrors v51's
1096+
// INSERT INTO skill_memory_fts(skill_memory_fts) VALUES('rebuild').
1097+
try {
1098+
const ftsCount = (
1099+
db.prepare("SELECT COUNT(*) AS n FROM skill_memory_fts").get() as { n: number }
1100+
).n;
1101+
const rowCount = (
1102+
db.prepare("SELECT COUNT(*) AS n FROM skill_memory").get() as { n: number }
1103+
).n;
1104+
if (ftsCount === 0 && rowCount > 0) {
1105+
db.exec("INSERT INTO skill_memory_fts(skill_memory_fts) VALUES('rebuild');");
1106+
}
1107+
} catch {
1108+
// Non-fatal: FTS rung-3 degrades gracefully (embedding + flat recall unaffected).
1109+
}
1110+
10911111
ensureColumn(db, "primer_candidates", "harness", "TEXT NOT NULL DEFAULT 'opencode'");
10921112
ensureColumn(db, "primer_candidates", "source_start_message_id", "TEXT NOT NULL DEFAULT ''");
10931113
ensureColumn(db, "primer_candidates", "source_end_message_id", "TEXT NOT NULL DEFAULT ''");

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

Lines changed: 37 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -485,6 +485,23 @@ export function createIntentByCallIdMap(): IntentByCallIdMap {
485485
return new Map();
486486
}
487487

488+
/**
489+
* Composite key for the intent stash: `${sessionId}:${callId}`. Keying by
490+
* session (not bare callID) lets onSessionDeleted prune one session's entries
491+
* by prefix without evicting concurrent sessions' in-flight intents.
492+
*/
493+
export function intentKey(sessionId: string, callId: string): string {
494+
return `${sessionId}:${callId}`;
495+
}
496+
497+
/** Delete all stash entries belonging to one session (prefix prune on delete). */
498+
export function pruneIntentsForSession(map: IntentByCallIdMap, sessionId: string): void {
499+
const prefix = `${sessionId}:`;
500+
for (const key of map.keys()) {
501+
if (key.startsWith(prefix)) map.delete(key);
502+
}
503+
}
504+
488505
const INTENT_TTL_MS = 60_000;
489506
const INTENT_MAP_CAP = 256;
490507

@@ -556,13 +573,19 @@ export async function maybeInjectSkillMemory(
556573

557574
export function createToolExecuteBeforeHook(args: { intentByCallId: IntentByCallIdMap }) {
558575
return async (input: unknown, output?: unknown) => {
559-
const typedInput = input as { tool?: string; callID?: string };
576+
const typedInput = input as { tool?: string; callID?: string; sessionID?: string };
560577
const typedOutput = output as { args?: Record<string, unknown> } | undefined;
561578
if (typedInput.tool !== "skill") return;
562-
if (!typedInput.callID) return;
579+
if (!typedInput.callID || !typedInput.sessionID) return;
563580
const intent = typedOutput?.args?.intent;
564581
if (typeof intent !== "string") return;
565-
stashIntent(args.intentByCallId, typedInput.callID, intent);
582+
// Key by sessionID:callID so a concurrent session's delete (which prunes
583+
// by prefix) can't evict this session's in-flight intents.
584+
stashIntent(
585+
args.intentByCallId,
586+
intentKey(typedInput.sessionID, typedInput.callID),
587+
intent,
588+
);
566589
};
567590
}
568591

@@ -601,10 +624,13 @@ export function createToolExecuteAfterHook(args: {
601624
const skillArgs = typedInput.args as { name?: unknown } | undefined;
602625
const skillId = typeof skillArgs?.name === "string" ? skillArgs.name : null;
603626
if (skillId) {
627+
// One dynamic import of the provenance module shared by both
628+
// the registry-populate and the injection blocks below
629+
// (lazy-loaded only when the skill tool actually fires).
630+
const { parseSkillProvenance, registryKey } = await import(
631+
"../../features/magic-context/skill-memory/provenance"
632+
);
604633
try {
605-
const { parseSkillProvenance, registryKey } = await import(
606-
"../../features/magic-context/skill-memory/provenance"
607-
);
608634
const { parseFrontmatterConfig } = await import(
609635
"../../features/magic-context/skill-memory/frontmatter"
610636
);
@@ -639,11 +665,8 @@ export function createToolExecuteAfterHook(args: {
639665
// appends the <skill-memory> block to output.output.
640666
// Non-fatal: recall failure must never block the tool result.
641667
try {
642-
const { registryKey: rKey } = await import(
643-
"../../features/magic-context/skill-memory/provenance"
644-
);
645668
const registryEntry = args.skillLoadRegistry.get(
646-
rKey(typedInput.sessionID, skillId),
669+
registryKey(typedInput.sessionID, skillId),
647670
);
648671
if (registryEntry) {
649672
// First-turn fallback: if the map has no entry yet
@@ -657,8 +680,10 @@ export function createToolExecuteAfterHook(args: {
657680
args.defaultDirectory;
658681
const projectIdentity = resolveProjectIdentity(sessionDir);
659682
const stashed = typedInput.callID
660-
? (getAndDeleteIntent(args.intentByCallId, typedInput.callID) ??
661-
undefined)
683+
? (getAndDeleteIntent(
684+
args.intentByCallId,
685+
intentKey(typedInput.sessionID, typedInput.callID),
686+
) ?? undefined)
662687
: undefined;
663688
await maybeInjectSkillMemory(
664689
args.db,

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

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,7 @@ import {
8181
createToolExecuteAfterHook,
8282
createToolExecuteBeforeHook,
8383
getLiveNotificationParams,
84+
pruneIntentsForSession,
8485
} from "./hook-handlers";
8586
import type { LiveSessionState } from "./live-session-state";
8687
import { sendIgnoredMessage } from "./send-session-notification";
@@ -677,14 +678,12 @@ export function createMagicContextHook(deps: MagicContextDeps) {
677678
internalChildSessions.delete(sessionId);
678679
channel1StateBySession.delete(sessionId);
679680
clearEmbedSessionState(sessionId);
680-
// NOTE: intentByCallId is keyed by callID (not sessionID:callID), so .clear() removes
681-
// entries from ALL concurrent sessions, not just the deleted one. This is an accepted
682-
// design trade-off: the 60s TTL + 256-entry hard cap are the real leak guards; the
683-
// .clear() here is a belt-and-braces backstop for long-lived sessions. Cross-session
684-
// clearing degrades quality (lost intents for concurrent sessions) but is not fatal.
685-
// If concurrent multi-session use becomes common, key entries as `${sessionID}:${callID}`
686-
// and filter on delete. For P1, document-as-intentional is the chosen fix.
687-
intentByCallId.clear(); // clear all entries on session delete (bounded map; cross-session clear is intentional — see note above)
681+
// intentByCallId is keyed `${sessionID}:${callID}` — prune only THIS
682+
// session's entries by prefix so a concurrent session's delete can't
683+
// evict another session's in-flight intents (which would silently
684+
// degrade its skill-memory recall to the flat rung). The 60s TTL +
685+
// 256-entry cap remain the leak backstops.
686+
pruneIntentsForSession(intentByCallId, sessionId);
688687
// skillLoadRegistry is keyed as `${sessionId}:${skillId}` so we can prune
689688
// per-session entries without cross-session bleed. Without this, deleted
690689
// sessions' skill loads would persist in the registry for the plugin's

‎packages/plugin/src/hooks/magic-context/read-session-formatting.ts‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -72,9 +72,14 @@ export function extractToolCallSummaries(parts: unknown[]): string[] {
7272

7373
// Skill tool: surface the skill name (input.name) before the description
7474
// fallback, which would otherwise mask it if metadata.description exists.
75+
// The name is an IDENTITY key (the historian extracts skill-id from this
76+
// marker), so do NOT truncate it. Sanitize newlines/control chars and a
77+
// stray ")" so the single-line `TC: skill(<name>)` marker can't be
78+
// corrupted — skill names are normally slugs, this is defensive only.
7579
if (p.tool === "skill") {
76-
const name = input && typeof input.name === "string" ? input.name : "";
77-
summaries.push(name ? `TC: skill(${truncateArg(name)})` : "TC: skill");
80+
const rawName = input && typeof input.name === "string" ? input.name : "";
81+
const name = rawName.replace(/[\r\n\t)]/g, " ").trim();
82+
summaries.push(name ? `TC: skill(${name})` : "TC: skill");
7883
continue;
7984
}
8085

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

Lines changed: 22 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,11 @@
11
import { describe, expect, test } from "bun:test";
2-
import { createIntentByCallIdMap, getAndDeleteIntent, stashIntent } from "./hook-handlers";
2+
import {
3+
createIntentByCallIdMap,
4+
getAndDeleteIntent,
5+
intentKey,
6+
pruneIntentsForSession,
7+
stashIntent,
8+
} from "./hook-handlers";
39

410
describe("intentByCallId stash map", () => {
511
test("stashIntent stores intent keyed by callId", () => {
@@ -36,11 +42,21 @@ describe("intentByCallId stash map", () => {
3642
expect(map.has("call-overflow")).toBe(true);
3743
});
3844

39-
test("clearIntentMap removes all entries (onSessionDeleted)", () => {
45+
test("pruneIntentsForSession removes ONLY the deleted session's entries", () => {
46+
// Regression (P1): a bare-callID key + .clear() on session delete wiped
47+
// EVERY concurrent session's in-flight intents. Keying by
48+
// `${sessionId}:${callId}` + prefix-prune isolates the delete.
4049
const map = createIntentByCallIdMap();
41-
stashIntent(map, "call-a", "intent a");
42-
stashIntent(map, "call-b", "intent b");
43-
map.clear();
44-
expect(map.size).toBe(0);
50+
stashIntent(map, intentKey("ses-A", "call-1"), "A intent");
51+
stashIntent(map, intentKey("ses-B", "call-1"), "B intent");
52+
stashIntent(map, intentKey("ses-B", "call-2"), "B intent 2");
53+
54+
pruneIntentsForSession(map, "ses-A");
55+
56+
// ses-A's entry is gone; ses-B's two survive (callID "call-1" collides
57+
// across sessions but the composite key keeps them distinct).
58+
expect(getAndDeleteIntent(map, intentKey("ses-A", "call-1"))).toBeNull();
59+
expect(getAndDeleteIntent(map, intentKey("ses-B", "call-1"))).toBe("B intent");
60+
expect(getAndDeleteIntent(map, intentKey("ses-B", "call-2"))).toBe("B intent 2");
4561
});
4662
});

0 commit comments

Comments
 (0)