diff --git a/docs/settings.md b/docs/settings.md index 4ae1dd652..b36dd5112 100644 --- a/docs/settings.md +++ b/docs/settings.md @@ -502,6 +502,9 @@ memory: | `compaction.remoteEnabled` | boolean | `true` | Allow remote compaction service. | | `compaction.autoContinue` | boolean | `true` | Continue automatically after compaction. | | `memory.backend` | enum | `off` | `off`, `local`, `hindsight`, `mnemopi`. Each backend has its own `hindsight.*` / `mnemopi.*` / `memories.*` tuning keys. | +| `autolearn.enabled` | boolean | `false` | Experimental: after the agent stops, nudge it to capture lessons to memory and create/enhance isolated managed skills under `~/.omp/agent/managed-skills`. Enables the `manage_skill` tool (and `learn` when a memory backend is active). | +| `autolearn.autoContinue` | boolean | `false` | When `autolearn.enabled`, auto-run one capture turn at stop (uses extra tokens). Off = a passive reminder rides your next turn. | +| `autolearn.minToolCalls` | number | `5` | Only nudge after a turn that used at least this many tools. | `compaction` has additional tuning keys (idle compaction, supersede/drop heuristics) visible in `omp config list`. See [Compaction](./compaction.md) for the full strategy reference. diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index d26e41891..c13538132 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -36,6 +36,7 @@ ### Removed - Removed the Python `openai-whisper` dependency and `pip` install path from speech-to-text — the bundled `transcribe.py` and all Python/whisper probes in `omp setup speech` are gone; the recorder (SoX/FFmpeg/arecord) remains the only external tool. +- Added an experimental, opt-in **auto-learn** loop (default off, `autolearn.enabled`). When enabled, after the agent stops it is nudged to capture reusable lessons: durable facts go to long-term memory and repeatable procedures become **managed skills** — `SKILL.md` files written to an isolated `~/.omp/agent/managed-skills` directory that is discovered and surfaced like authored skills but never overwrites user-authored skills (authored names always win). Two tools back this: `manage_skill` (create/update/delete managed skills) and `learn` (record a lesson, optionally minting a managed skill in the same call). `learn` works with the `hindsight`, `mnemopi`, or file-based `local` memory backend; under `local`, lessons append to a `learned.md` in the project's memory root (kept separate from the consolidation artifacts so they survive a consolidation pass) and are injected into future sessions. The nudge is passive by default (a hidden reminder rides the next turn); `autolearn.autoContinue` instead auto-runs one capture turn at stop, and `autolearn.minToolCalls` (default 5) gates trivial turns. Plan/goal-mode turns and subagents are never nudged. ### Fixed diff --git a/packages/coding-agent/src/autolearn/controller.ts b/packages/coding-agent/src/autolearn/controller.ts new file mode 100644 index 000000000..d204ee259 --- /dev/null +++ b/packages/coding-agent/src/autolearn/controller.ts @@ -0,0 +1,121 @@ +/** + * Auto-learn session controller (experimental). + * + * Subscribes to the session event stream and, after a substantive turn, + * nudges the agent to capture reusable lessons. Default posture is passive + * (a hidden reminder rides the next real turn); with `autolearn.autoContinue` + * it auto-runs exactly one synthetic capture turn at stop. + * + * Installed once per top-level session (taskDepth 0). The subscription lives + * for the session's lifetime — `newSession` resets the session in place + * without re-running startup — so the controller needs no disposal. + */ +import { logger } from "@oh-my-pi/pi-utils"; +import type { Settings } from "../config/settings"; +import autolearnGuidance from "../prompts/system/autolearn-guidance.md" with { type: "text" }; +import autolearnGuidanceLearn from "../prompts/system/autolearn-guidance-learn.md" with { type: "text" }; +import autolearnNudge from "../prompts/system/autolearn-nudge.md" with { type: "text" }; +import type { AgentSession, AgentSessionEvent } from "../session/agent-session"; + +const AUTOLEARN_NUDGE = autolearnNudge.trim(); +const DEFAULT_MIN_TOOL_CALLS = 5; + +/** + * Build the standing auto-learn guidance for the system prompt from the tools + * actually present in the active set, or null when `manage_skill` is absent. + * + * Driven by tool presence rather than live settings: the `learn`/`manage_skill` + * registry is built ONCE at session start (and only for top-level sessions), so + * keying the guidance on `autolearn.enabled` would let a mid-session enable — or + * a subagent that filtered the tools out — inject guidance pointing at tools the + * session never built. The `learn` addendum is included only when the `learn` + * tool is present (it requires a memory backend). + */ +export function buildAutoLearnInstructions(available: { manageSkill: boolean; learn: boolean }): string | null { + if (!available.manageSkill) return null; + const parts = [autolearnGuidance.trim()]; + if (available.learn) parts.push(autolearnGuidanceLearn.trim()); + return parts.join("\n\n"); +} + +export interface AutoLearnControllerOptions { + session: AgentSession; + settings: Settings; +} + +export class AutoLearnController { + readonly #session: AgentSession; + readonly #settings: Settings; + #toolCalls = 0; + /** Swallow the agent_end produced by an auto-run capture turn so it cannot re-trigger. */ + #suppressNext = false; + + constructor(options: AutoLearnControllerOptions) { + this.#session = options.session; + this.#settings = options.settings; + // The listener closure captures `this`, so the session's listener array + // keeps the controller alive — no stored unsubscribe needed. + this.#session.subscribe(event => this.#onEvent(event)); + } + + #onEvent(event: AgentSessionEvent): void { + if (event.type === "tool_execution_end") { + this.#toolCalls++; + return; + } + if (event.type === "agent_end") { + this.#onAgentEnd(); + } + } + + #onAgentEnd(): void { + // Snapshot and reset every turn: the counter describes only the + // just-finished turn, so below-threshold, disabled, and plan-mode stops + // must not let tool calls accumulate into a later turn. + const toolCalls = this.#toolCalls; + this.#toolCalls = 0; + + if (this.#suppressNext) { + this.#suppressNext = false; + return; + } + // Honor a live opt-out: the subscription outlives the setting, so re-check + // the current flag rather than trusting install-time state. + if (!this.#settings.get("autolearn.enabled")) return; + const minToolCalls = this.#settings.get("autolearn.minToolCalls") ?? DEFAULT_MIN_TOOL_CALLS; + if (toolCalls < minToolCalls) return; + // Never interrupt plan-mode review. + if (this.#session.getPlanModeState()?.enabled) return; + // Never divert an active goal loop: a passive nudge would ride the goal + // continuation, and auto-continue would compete with it. Skip entirely. + if (this.#session.getGoalModeState()?.enabled) return; + + // Auto-run a capture turn only when explicitly enabled; otherwise the + // hidden reminder rides the next real turn passively. + const autoContinue = this.#settings.get("autolearn.autoContinue") === true; + // Arm suppression synchronously: the synthetic capture turn's agent_end + // fires inside sendCustomMessage (before it resolves), so the flag must be + // set before then. Disarm when no turn actually started — a deferred/queued + // dispatch or a failed send produces no agent_end, and a latched flag would + // otherwise swallow the next real stop. + if (autoContinue) this.#suppressNext = true; + + this.#session + .sendCustomMessage( + { + customType: "autolearn-nudge", + content: AUTOLEARN_NUDGE, + display: false, + attribution: "user", + }, + { deliverAs: "nextTurn", triggerTurn: autoContinue }, + ) + .then(started => { + if (!started) this.#suppressNext = false; + }) + .catch(err => { + this.#suppressNext = false; + logger.warn("auto-learn nudge delivery failed", { err }); + }); + } +} diff --git a/packages/coding-agent/src/autolearn/managed-skills.ts b/packages/coding-agent/src/autolearn/managed-skills.ts new file mode 100644 index 000000000..52c994aff --- /dev/null +++ b/packages/coding-agent/src/autolearn/managed-skills.ts @@ -0,0 +1,220 @@ +/** + * Managed-skills primitives for the experimental auto-learn feature. + * + * Managed skills are auto-generated/enhanced `SKILL.md` files kept in an + * isolated directory (`~/.omp/agent/managed-skills`) separate from + * user-authored skills (`~/.omp/agent/skills`). They are discovered and + * surfaced like normal skills, but every write here is confined to + * `getManagedSkillsDir()` — auto-management can never touch authored skills. + */ +import * as fs from "node:fs/promises"; +import * as os from "node:os"; +import * as path from "node:path"; +import { isEnoent } from "@oh-my-pi/pi-utils"; +import { YAML } from "bun"; +import { SOURCE_PATHS } from "../discovery/helpers"; + +/** Provider id stamped on discovered managed skills (distinguishes them from authored). */ +export const MANAGED_SKILLS_PROVIDER_ID = "omp-managed"; + +/** Hard cap on a managed SKILL.md body to keep generated skills bounded. */ +export const MAX_MANAGED_SKILL_BYTES = 64_000; + +const SKILL_NAME_PATTERN = /^[a-z0-9][a-z0-9-]{0,63}$/; + +/** Resolve the isolated managed-skills directory (`~/.omp/agent/managed-skills`). */ +export function getManagedSkillsDir(home: string = os.homedir()): string { + return path.join(home, SOURCE_PATHS.native.userAgent, "managed-skills"); +} + +/** + * Validate + normalize a managed-skill name. Throws on anything outside the + * strict allowlist so a bad name can never escape `getManagedSkillsDir()` + * (blocks `..`, slashes, empty, and uppercase). + */ +export function sanitizeSkillName(raw: string): string { + const name = raw.trim().toLowerCase(); + if (!SKILL_NAME_PATTERN.test(name)) { + throw new Error( + `Invalid skill name "${raw}". Use lowercase letters, digits, and hyphens (1-64 chars, starting with a letter or digit).`, + ); + } + return name; +} + +/** + * Whether `name` is a safe managed-skill name (the exact post-sanitize shape). + * Used to validate names read from disk at discovery time — a managed + * `SKILL.md` whose `frontmatter.name` was not produced by `sanitizeSkillName` + * (e.g. hand-placed) must not render unescaped into the system prompt. + */ +export function isValidManagedSkillName(name: string): boolean { + return SKILL_NAME_PATTERN.test(name); +} + +/** + * Neutralize a machine-generated managed-skill description so it cannot break + * out of the system prompt's `` listing. Managed descriptions are + * generated from prior task content and persist across sessions, so this is a + * trust boundary: strip control/format chars, angle brackets (`` + * / ``), and Markdown fence delimiters (backticks, `~~~`), then collapse + * to a single line. Applied on BOTH write and read so existing files are safe too. + */ +export function sanitizeManagedDescription(raw: string): string { + return raw + .replace(/[\p{Cc}\p{Cf}]/gu, " ") + .replace(/[<>`]/g, "") + .replace(/~{2,}/g, "~") + .replace(/\s+/g, " ") + .trim(); +} + +/** + * Serialize the minimal `name`/`description` frontmatter block via the repo's + * YAML helper (round-trips through `parseFrontmatter`). + */ +export function toSkillFrontmatter(name: string, description: string): string { + const frontmatter = YAML.stringify( + { name, description: sanitizeManagedDescription(description) }, + null, + 2, + ).trimEnd(); + return `---\n${frontmatter}\n---\n`; +} + +export interface WriteManagedSkillInput { + action: "create" | "update"; + name: string; + description: string; + body: string; +} + +/** + * Serialize create/update/delete on the same skill name. Both tools are + * non-exclusive, so a parallel tool batch in one turn can run two mutations on + * the same skill at once (e.g. an update observing the file mid-delete). This + * per-name promise chain runs same-skill mutations in submission order while + * different names still proceed in parallel. In-process only; cross-process + * races are out of scope. + */ +const skillMutationChains = new Map>(); +function serializeSkillMutation(name: string, op: () => Promise): Promise { + const prev = skillMutationChains.get(name) ?? Promise.resolve(); + const run = prev.then(op, op); + const guarded = run.catch(() => {}); + skillMutationChains.set(name, guarded); + void guarded.finally(() => { + if (skillMutationChains.get(name) === guarded) skillMutationChains.delete(name); + }); + return run; +} + +/** + * Reject when the managed-skills root itself is a symlink. lstat on a child + * follows intermediate components, so a symlinked root would let an otherwise + * valid name write/delete outside the isolated directory (e.g. onto authored + * skills). Checked before composing any child path. + */ +async function assertManagedRootSafe(): Promise { + const rootStat = await fs.lstat(getManagedSkillsDir()).catch(err => { + if (isEnoent(err)) return null; + throw err; + }); + if (rootStat?.isSymbolicLink()) { + throw new Error("The managed-skills root is a symlink; refusing to operate outside the managed directory."); + } +} + +/** Create or update a managed `SKILL.md`. Returns the resolved file path. */ +export async function writeManagedSkill(input: WriteManagedSkillInput): Promise<{ path: string }> { + const name = sanitizeSkillName(input.name); + const description = sanitizeManagedDescription(input.description); + const body = input.body.trim(); + // Reject empty content: an all-whitespace/control description sanitizes to "" + // and the `requireDescription` discovery scan then silently drops the skill, + // so the tool would report success for a skill that never appears. + if (!description) { + throw new Error(`Managed skill "${name}" needs a non-empty description.`); + } + if (!body) { + throw new Error(`Managed skill "${name}" needs a non-empty body.`); + } + const content = `${toSkillFrontmatter(name, description)}\n${body}\n`; + // Cap the UTF-8 byte size of the FINAL file (body + description + frontmatter), + // not the UTF-16 code-unit length of the body alone. + const bytes = Buffer.byteLength(content, "utf8"); + if (bytes > MAX_MANAGED_SKILL_BYTES) { + throw new Error( + `Managed skill is ${bytes} bytes; the limit is ${MAX_MANAGED_SKILL_BYTES}. Trim the body or description.`, + ); + } + return serializeSkillMutation(name, async () => { + await assertManagedRootSafe(); + const dir = path.join(getManagedSkillsDir(), name); + const file = path.join(dir, "SKILL.md"); + // Reject a symlinked skill directory: an intermediate symlink would let the + // write escape the isolated managed root. lstat does not follow the final + // component, so a symlinked `dir` is caught here. + const dirStat = await fs.lstat(dir).catch(err => { + if (isEnoent(err)) return null; + throw err; + }); + if (dirStat?.isSymbolicLink()) { + throw new Error( + `Managed skill "${name}" resolves through a symlink; refusing to write outside the managed directory.`, + ); + } + if (input.action === "create") { + await fs.mkdir(dir, { recursive: true }); + // O_CREAT|O_EXCL ("wx"): atomic create that fails if the file already + // exists (closing the check-then-write race) and refuses a symlinked SKILL.md. + try { + await fs.writeFile(file, content, { flag: "wx" }); + } catch (err) { + if ((err as { code?: string }).code === "EEXIST") { + throw new Error(`Managed skill "${name}" already exists. Use action "update" to change it.`); + } + throw err; + } + return { path: file }; + } + // update: the file must already exist and must not be a symlink. + const fileStat = await fs.lstat(file).catch(err => { + if (isEnoent(err)) return null; + throw err; + }); + if (fileStat === null) { + throw new Error(`Managed skill "${name}" does not exist. Use action "create" to add it.`); + } + if (fileStat.isSymbolicLink()) { + throw new Error(`Managed skill "${name}" SKILL.md is a symlink; refusing to overwrite it.`); + } + await Bun.write(file, content); + return { path: file }; + }); +} + +/** Delete a managed skill directory. Throws when it does not exist. */ +export async function deleteManagedSkill(name: string): Promise { + const safe = sanitizeSkillName(name); + await serializeSkillMutation(safe, async () => { + await assertManagedRootSafe(); + const dir = path.join(getManagedSkillsDir(), safe); + // Refuse to follow a symlinked skill directory (rm would delete the target). + const dirStat = await fs.lstat(dir).catch(err => { + if (isEnoent(err)) return null; + throw err; + }); + if (dirStat?.isSymbolicLink()) { + throw new Error(`Managed skill "${safe}" is a symlink; refusing to delete outside the managed directory.`); + } + try { + await fs.rm(dir, { recursive: true }); + } catch (err) { + if (isEnoent(err)) { + throw new Error(`Managed skill "${safe}" does not exist.`); + } + throw err; + } + }); +} diff --git a/packages/coding-agent/src/config/settings-schema.ts b/packages/coding-agent/src/config/settings-schema.ts index a59b8c100..fddcb484f 100644 --- a/packages/coding-agent/src/config/settings-schema.ts +++ b/packages/coding-agent/src/config/settings-schema.ts @@ -118,7 +118,7 @@ export const TAB_GROUPS: Record = { "Power (macOS)", ], context: ["General", "Compaction", "Rules (TTSR)", "Experimental"], - memory: ["General", "Mnemopi", "Hindsight"], + memory: ["General", "Auto-Learn", "Mnemopi", "Hindsight"], files: ["Editing", "Reading", "Read Summaries", "LSP"], shell: ["Bash", "Eval & Python"], tools: [ @@ -1898,6 +1898,35 @@ export const SETTINGS_SCHEMA = { }, }, + // Auto-Learn (experimental): post-stop nudge to capture lessons to memory + // and mint/enhance isolated managed skills under ~/.omp/agent/managed-skills. + // Master flag is default-off → zero footprint; sub-flags gate behaviour. + "autolearn.enabled": { + type: "boolean", + default: false, + ui: { + tab: "memory", + group: "Auto-Learn", + label: "Auto-Learn (experimental)", + description: + "After the agent stops, nudge it to capture lessons to memory and create/enhance isolated managed skills", + }, + }, + "autolearn.autoContinue": { + type: "boolean", + default: false, + ui: { + tab: "memory", + group: "Auto-Learn", + label: "Auto-run capture at stop", + description: + "When on, auto-run one capture turn at stop (uses extra tokens). Off = passive reminder on your next turn.", + condition: "autolearnActive", + }, + }, + // Config-file-only knob (numbers without `options` are hidden from the UI). + "autolearn.minToolCalls": { type: "number", default: 5 }, + // Mnemopi local SQLite memory backend. "mnemopi.dbPath": { type: "string", diff --git a/packages/coding-agent/src/discovery/builtin.ts b/packages/coding-agent/src/discovery/builtin.ts index 38c3307dd..060c5de65 100644 --- a/packages/coding-agent/src/discovery/builtin.ts +++ b/packages/coding-agent/src/discovery/builtin.ts @@ -6,6 +6,7 @@ import * as path from "node:path"; import { logger, parseFrontmatter, tryParseJson } from "@oh-my-pi/pi-utils"; import { YAML } from "bun"; +import { getManagedSkillsDir, MANAGED_SKILLS_PROVIDER_ID } from "../autolearn/managed-skills"; import { registerProvider } from "../capability"; import { type ContextFile, contextFileCapability } from "../capability/context-file"; import { type Extension, type ExtensionManifest, extensionCapability } from "../capability/extension"; @@ -283,13 +284,26 @@ async function loadSkills(ctx: LoadContext): Promise> { }); const results = await Promise.all([...projectScans, userScan]); - return { items: results.flatMap(r => r.items), warnings: results.flatMap(r => r.warnings ?? []), }; } +// Managed skills (auto-learn) are a SEPARATE provider at the lowest skill +// priority, so an authored skill of the same name from ANY other provider wins +// the capability-level priority dedup. Discovery is unconditional (an empty +// managed dir is a no-op); only writing/nudging is gated by `autolearn.enabled`. +const MANAGED_SKILLS_PRIORITY = 5; +async function loadManagedSkills(ctx: LoadContext): Promise> { + return scanSkillsFromDir(ctx, { + dir: getManagedSkillsDir(ctx.home), + providerId: MANAGED_SKILLS_PROVIDER_ID, + level: "user", + requireDescription: true, + }); +} + registerProvider(skillCapability.id, { id: PROVIDER_ID, displayName: DISPLAY_NAME, @@ -298,6 +312,14 @@ registerProvider(skillCapability.id, { load: loadSkills, }); +registerProvider(skillCapability.id, { + id: MANAGED_SKILLS_PROVIDER_ID, + displayName: "Managed Skills (auto-learn)", + description: "Auto-generated managed skills from ~/.omp/agent/managed-skills", + priority: MANAGED_SKILLS_PRIORITY, + load: loadManagedSkills, +}); + // Slash Commands async function loadSlashCommands(ctx: LoadContext): Promise> { const items: SlashCommand[] = []; diff --git a/packages/coding-agent/src/extensibility/skills.ts b/packages/coding-agent/src/extensibility/skills.ts index 265ceaf92..53043b9c0 100644 --- a/packages/coding-agent/src/extensibility/skills.ts +++ b/packages/coding-agent/src/extensibility/skills.ts @@ -1,6 +1,11 @@ import * as fs from "node:fs/promises"; import * as os from "node:os"; import { getProjectDir } from "@oh-my-pi/pi-utils"; +import { + isValidManagedSkillName, + MANAGED_SKILLS_PROVIDER_ID, + sanitizeManagedDescription, +} from "../autolearn/managed-skills"; import { skillCapability } from "../capability/skill"; import type { SourceMeta } from "../capability/types"; import type { SkillsSettings } from "../config/settings"; @@ -137,6 +142,10 @@ export async function loadSkills(options: LoadSkillsOptions = {}): Promise + capSkill._source.provider === MANAGED_SKILLS_PROVIDER_ID && + isValidManagedSkillName(capSkill.name) && + !disabledSkillNames.has(capSkill.name) && + !matchesIgnorePatterns(capSkill.name) && + matchesIncludePatterns(capSkill.name), + ); + // Names claimed by any ENABLED authored skill (from the pre-dedup superset). + // Managed defers to these even when capability dedup hid an enabled authored + // skill behind a disabled higher-priority one, so managed never masks it. + const enabledAuthoredNames = new Set( + result.all + .filter( + capSkill => capSkill._source.provider !== MANAGED_SKILLS_PROVIDER_ID && isSourceEnabled(capSkill._source), + ) + .map(capSkill => capSkill.name), + ); + const managedRealPaths = await Promise.all( + managedCandidates.map(async capSkill => { + try { + return await fs.realpath(capSkill.path); + } catch { + return capSkill.path; + } + }), + ); + for (let i = 0; i < managedCandidates.length; i++) { + const capSkill = managedCandidates[i]; + const resolvedPath = managedRealPaths[i]; + if (realPathSet.has(resolvedPath)) continue; + if (enabledAuthoredNames.has(capSkill.name)) continue; // an enabled authored skill owns this name + // Already claimed — e.g. by a custom-directory skill. LOAD-BEARING: custom + // dirs never enter `result.all`, so they are absent from `enabledAuthoredNames` + // above; this map check is the ONLY veto that lets a custom-dir authored skill + // win over a same-named managed one. The custom-dir loop (which populates + // skillMap, ~30 lines up) MUST run before this block — do not reorder. + if (skillMap.has(capSkill.name)) continue; + const rawDescription = + typeof capSkill.frontmatter?.description === "string" ? capSkill.frontmatter.description : ""; + skillMap.set(capSkill.name, { + name: capSkill.name, + description: sanitizeManagedDescription(rawDescription), + filePath: capSkill.path, + baseDir: capSkill.path.replace(/[\\/]SKILL\.md$/, ""), + source: `${capSkill._source.provider}:${capSkill.level}`, + hide: capSkill.frontmatter?.hide === true || capSkill.frontmatter?.disableModelInvocation === true, + _source: capSkill._source, + }); + realPathSet.add(resolvedPath); + } + const skills = Array.from(skillMap.values()); // Deterministic ordering for prompt stability (case-insensitive, then exact name, then path). skills.sort((a, b) => compareSkillOrder(a.name, a.filePath, b.name, b.filePath)); diff --git a/packages/coding-agent/src/memories/index.ts b/packages/coding-agent/src/memories/index.ts index a5fb2dff3..9532b5ff7 100644 --- a/packages/coding-agent/src/memories/index.ts +++ b/packages/coding-agent/src/memories/index.ts @@ -5,11 +5,12 @@ import * as path from "node:path"; import type { AgentMessage } from "@oh-my-pi/pi-agent-core"; import { type ApiKey, completeSimple, Effort, type Model } from "@oh-my-pi/pi-ai"; import { clampThinkingLevelForModel } from "@oh-my-pi/pi-catalog/model-thinking"; -import { getAgentDbPath, getMemoriesDir, logger, parseJsonlLenient, prompt } from "@oh-my-pi/pi-utils"; +import { getAgentDbPath, getMemoriesDir, isEnoent, logger, parseJsonlLenient, prompt } from "@oh-my-pi/pi-utils"; import type { ModelRegistry } from "../config/model-registry"; import { getModelMatchPreferences, resolveModelRoleValue } from "../config/model-resolver"; import type { Settings } from "../config/settings"; +import type { MemoryBackendSaveInput, MemoryBackendSaveResult } from "../memory-backend/types"; import consolidationTemplate from "../prompts/memories/consolidation.md" with { type: "text" }; import consolidationSystemTemplate from "../prompts/memories/consolidation_system.md" with { type: "text" }; import readPathTemplate from "../prompts/memories/read-path.md" with { type: "text" }; @@ -156,22 +157,31 @@ export async function buildMemoryToolDeveloperInstructions( const cfg = loadMemoryConfig(settings); if (!cfg.enabled) return undefined; const memoryRoot = getMemoryRoot(agentDir, settings.getCwd()); - const summaryPath = path.join(memoryRoot, "memory_summary.md"); - let text: string; + let summary = ""; try { - text = await Bun.file(summaryPath).text(); + summary = (await Bun.file(path.join(memoryRoot, "memory_summary.md")).text()).trim(); } catch { - return undefined; + // Missing or unreadable summary — injection is best-effort; fall through + // so any captured lessons still surface on their own. } + const learned = await readLearnedLessons(memoryRoot); + if (!summary && !learned) return undefined; - const summary = text.trim(); - if (!summary) return undefined; - const truncated = truncateByApproxTokens(summary, cfg.summaryInjectionTokenLimit); - if (!truncated.trim()) return undefined; + const summaryOut = summary ? truncateByApproxTokens(summary, cfg.summaryInjectionTokenLimit).trim() : ""; + // Lessons share ONE injection budget with the summary so the combined block + // stays within `summaryInjectionTokenLimit` (~4 chars/token, matching + // truncateByApproxTokens). With no summary, lessons get the whole budget. + // Clamp to 0: truncateByApproxTokens appends a marker, so a truncated summary + // can exceed `limit * 4` chars and drive the remainder negative — when the + // summary already fills the budget, lessons are simply dropped. + const learnedBudget = Math.max(0, cfg.summaryInjectionTokenLimit - Math.ceil(summaryOut.length / 4)); + const learnedOut = learned && learnedBudget > 0 ? truncateByApproxTokens(learned, learnedBudget).trim() : ""; + if (!summaryOut && !learnedOut) return undefined; return prompt.render(readPathTemplate, { - memory_summary: truncated, + memory_summary: summaryOut, + learned: learnedOut, }); } @@ -982,6 +992,12 @@ function redactSecrets(input: string): string { /(?:sk|pk|rk|tok|key|secret|token|password)[-_A-Za-z0-9]{12,}/g, /[A-Za-z0-9_-]{16,}\.[A-Za-z0-9_-]{16,}\.[A-Za-z0-9_-]{16,}/g, /(?:AKIA|ASIA)[A-Z0-9]{16}/g, + // Common provider token prefixes (GitHub, npm, Slack, Google). + /(?:ghp|gho|ghu|ghs|ghr)_[A-Za-z0-9]{20,}/g, + /github_pat_[A-Za-z0-9_]{20,}/g, + /npm_[A-Za-z0-9]{30,}/g, + /xox[baprs]-[A-Za-z0-9-]{10,}/g, + /AIza[A-Za-z0-9_-]{30,}/g, ]; for (const pattern of patterns) { out = out.replace(pattern, "[REDACTED]"); @@ -1121,6 +1137,125 @@ export function getMemoryRoot(agentDir: string, cwd: string): string { return path.join(getMemoriesDir(agentDir), encodeProjectPath(cwd)); } +/** + * Filename of the captured-lessons file under a project's memory root. + * + * Written by the `learn` tool via {@link saveLearnedLesson} and read back by + * {@link buildMemoryToolDeveloperInstructions}. Deliberately distinct from the + * consolidation artifacts (`MEMORY.md`, `memory_summary.md`, `skills/`) so a + * consolidation pass never clobbers manually captured lessons. + */ +const LEARNED_LESSONS_FILE = "learned.md"; +/** Newest-first cap on retained lessons, bounding file growth by entry count. */ +const MAX_LEARNED_LESSONS = 100; +/** Per-field char caps so a single huge capture can't bloat learned.md. */ +const MAX_LEARNED_CONTENT_CHARS = 2000; +const MAX_LEARNED_CONTEXT_CHARS = 400; + +/** + * Strip prompt-injection vectors from a single line of lesson text: control/ + * format chars, angle brackets (``), backticks, and `~~~` fences, then + * collapse whitespace. Applied on BOTH write and read (the block renders + * unescaped into the system prompt), mirroring managed-skill descriptions. + */ +function neutralizeInjection(text: string): string { + return text + .replace(/[\p{Cc}\p{Cf}]/gu, " ") + .replace(/[<>`]/g, "") + .replace(/~{2,}/g, "~") + .replace(/\s+/g, " ") + .trim(); +} + +/** Slice to `maxChars`, dropping a trailing unpaired high surrogate. */ +function boundChars(text: string, maxChars: number): string { + if (text.length <= maxChars) return text; + const sliced = text.slice(0, maxChars); + return /[\uD800-\uDBFF]$/.test(sliced) ? sliced.slice(0, -1) : sliced; +} + +/** + * Normalize one lesson field for storage: neutralize injection delimiters + * FIRST, then redact secrets (so delimiter stripping can't reassemble a token + * the redactor would have caught), then bound the length. + */ +function normalizeLearnedText(text: string, maxChars: number): string { + return boundChars(redactSecrets(neutralizeInjection(text)).trim(), maxChars); +} + +/** Per-path write chains serializing `learned.md` read-modify-write. */ +const learnedWriteChains = new Map>(); + +/** + * Append one lesson to the project's `learned.md` (newest-first, deduped, + * capped, secret-redacted, injection-neutralized). The file backs the `learn` + * tool when `memory.backend` is `local`. + */ +export async function saveLearnedLesson( + agentDir: string, + cwd: string, + input: MemoryBackendSaveInput, +): Promise { + const content = normalizeLearnedText(input.content, MAX_LEARNED_CONTENT_CHARS); + if (!content) { + return { backend: "local", stored: 0, message: "Empty lesson; nothing stored." }; + } + const context = input.context ? normalizeLearnedText(input.context, MAX_LEARNED_CONTEXT_CHARS) : ""; + const line = context ? `- ${content} _(context: ${context})_` : `- ${content}`; + const filePath = path.join(getMemoryRoot(agentDir, cwd), LEARNED_LESSONS_FILE); + + // Serialize the read-modify-write per file: parallel `learn` calls (sibling + // subagents, or two shared tool calls in one turn) share the project memory + // root, so an unguarded RMW would let the last writer drop the other's lesson. + const run = (learnedWriteChains.get(filePath) ?? Promise.resolve()).then(() => appendLearnedLine(filePath, line)); + const guarded = run.catch(() => {}); + learnedWriteChains.set(filePath, guarded); + try { + await run; + } finally { + // Drop the entry once this write is the chain tail, so the map does not + // retain one promise per distinct memory root for the process lifetime. + if (learnedWriteChains.get(filePath) === guarded) learnedWriteChains.delete(filePath); + } + return { backend: "local", stored: 1, message: `Lesson saved to ${LEARNED_LESSONS_FILE}.` }; +} + +async function appendLearnedLine(filePath: string, line: string): Promise { + let existing = ""; + try { + existing = await Bun.file(filePath).text(); + } catch (err) { + if (!isEnoent(err)) throw err; + } + const prior = existing + .split("\n") + .map(l => l.trim()) + .filter(l => l.startsWith("- ") && l !== line); + const lessons = [line, ...prior].slice(0, MAX_LEARNED_LESSONS); + await Bun.write(filePath, `${lessons.join("\n")}\n`); +} + +/** + * Read `learned.md`, neutralizing each line on read too — a hand-edited or + * pre-existing file bypasses write-time normalization and the block renders + * unescaped into the system prompt. Returns "" when absent/unreadable. + */ +async function readLearnedLessons(memoryRoot: string): Promise { + let raw = ""; + try { + raw = (await Bun.file(path.join(memoryRoot, LEARNED_LESSONS_FILE)).text()).trim(); + } catch { + return ""; + } + if (!raw) return ""; + // Neutralize delimiters THEN redact per line — mirrors the write path so a + // hand-edited line cannot reassemble a token after delimiter stripping. + return raw + .split("\n") + .map(line => redactSecrets(neutralizeInjection(line))) + .join("\n"); +} + function encodeProjectPath(cwd: string): string { return `--${cwd.replace(/^[/\\]/, "").replace(/[/\\:]/g, "-")}--`; } diff --git a/packages/coding-agent/src/memory-backend/local-backend.ts b/packages/coding-agent/src/memory-backend/local-backend.ts index e36c7145d..0c7726ddb 100644 --- a/packages/coding-agent/src/memory-backend/local-backend.ts +++ b/packages/coding-agent/src/memory-backend/local-backend.ts @@ -2,6 +2,7 @@ import { buildMemoryToolDeveloperInstructions, clearMemoryData, enqueueMemoryConsolidation, + saveLearnedLesson, startMemoryStartupTask, } from "../memories"; import type { MemoryBackend } from "./types"; @@ -9,9 +10,10 @@ import type { MemoryBackend } from "./types"; /** * Wraps the existing `memories/` module as a `MemoryBackend`. * - * No behavioural change — every call delegates to the legacy entry points so - * the local memory pipeline (rollout summarisation → SQLite → memory_summary.md) - * keeps working exactly as before. + * The rollout-summarisation pipeline (rollouts → SQLite → memory_summary.md) is + * delegated unchanged. On top of it, `save()` persists `learn`-tool lessons to + * `learned.md` (so `status()` reports `writable: true`); structured search is + * still unavailable. */ export const localBackend: MemoryBackend = { id: "local", @@ -27,13 +29,17 @@ export const localBackend: MemoryBackend = { async enqueue(agentDir, cwd) { enqueueMemoryConsolidation(agentDir, cwd); }, + async save(context, input) { + return saveLearnedLesson(context.agentDir, context.cwd, input); + }, async status() { return { backend: "local" as const, active: true, - writable: false, + writable: true, searchable: false, - message: "Local rollout-summary memory is active; structured search/save is not available.", + message: + "Local rollout-summary memory is active; lessons from the `learn` tool are saved to learned.md. Structured search is not available.", }; }, }; diff --git a/packages/coding-agent/src/modes/components/settings-defs.ts b/packages/coding-agent/src/modes/components/settings-defs.ts index 1d04f3596..8c47c8c5e 100644 --- a/packages/coding-agent/src/modes/components/settings-defs.ts +++ b/packages/coding-agent/src/modes/components/settings-defs.ts @@ -90,6 +90,13 @@ const CONDITIONS: Record boolean> = { return false; } }, + autolearnActive: () => { + try { + return Settings.instance.get("autolearn.enabled") === true; + } catch { + return false; + } + }, autoThinkingActive: () => { try { return Settings.instance.get("defaultThinkingLevel") === "auto"; diff --git a/packages/coding-agent/src/prompts/memories/read-path.md b/packages/coding-agent/src/prompts/memories/read-path.md index fdc85934f..9ef90dcaa 100644 --- a/packages/coding-agent/src/prompts/memories/read-path.md +++ b/packages/coding-agent/src/prompts/memories/read-path.md @@ -7,5 +7,11 @@ Operational rules: 4) When memory changes your plan, cite the artifact path (e.g. `memory://root/skills//SKILL.md`) and pair it with current-repo evidence. 5) If memory disagrees with repo state or user instruction, treat memory as stale: proceed with corrected behavior, then update/regenerate memory artifacts. 6) Escalate confidence only after repository verification. Memory alone is NEVER sufficient proof. +{{#if memory_summary}} Memory summary: {{memory_summary}} +{{/if}} +{{#if learned}} +Learned lessons (captured via the `learn` tool; durable but may be stale — verify against the repo before relying on them): +{{learned}} +{{/if}} diff --git a/packages/coding-agent/src/prompts/system/autolearn-guidance-learn.md b/packages/coding-agent/src/prompts/system/autolearn-guidance-learn.md new file mode 100644 index 000000000..68211eb3a --- /dev/null +++ b/packages/coding-agent/src/prompts/system/autolearn-guidance-learn.md @@ -0,0 +1 @@ +When a lesson is a durable *fact* rather than a procedure — a project convention, a non-obvious fix, a user preference — record it with `learn`, which writes to long-term memory. `learn` can also mint or enhance a managed skill in the same call when the lesson is both a fact and a procedure. diff --git a/packages/coding-agent/src/prompts/system/autolearn-guidance.md b/packages/coding-agent/src/prompts/system/autolearn-guidance.md new file mode 100644 index 000000000..509ce1f66 --- /dev/null +++ b/packages/coding-agent/src/prompts/system/autolearn-guidance.md @@ -0,0 +1,7 @@ +## Auto-Learn (experimental) + +You can grow a library of reusable **managed skills** with the `manage_skill` tool. Managed skills are `SKILL.md` files kept in an isolated directory (`~/.omp/agent/managed-skills`); they are surfaced to you in future sessions like any other skill. + +- Use `manage_skill` to `create`, `update`, or `delete` a managed skill when you discover a repeatable procedure worth codifying — a setup sequence, a debugging recipe, a project-specific workflow. +- **Isolation rule:** managed skills are the ONLY skills you may write. NEVER edit user-authored skills under `~/.omp/agent/skills` or `.omp/skills`. +- Capture sparingly and specifically. A skill earns its place only if it will be reused; prefer enhancing an existing managed skill over creating a near-duplicate. diff --git a/packages/coding-agent/src/prompts/system/autolearn-nudge.md b/packages/coding-agent/src/prompts/system/autolearn-nudge.md new file mode 100644 index 000000000..2ee8322ba --- /dev/null +++ b/packages/coding-agent/src/prompts/system/autolearn-nudge.md @@ -0,0 +1,3 @@ +Before you finish: if this turn produced anything reusable, capture it now with your learning tools — a repeatable procedure becomes a managed skill (`manage_skill`), and a durable fact or convention is worth remembering (`learn`, when memory is enabled). + +Only capture what will genuinely help next time. If nothing this turn is worth keeping, do nothing. diff --git a/packages/coding-agent/src/prompts/tools/learn.md b/packages/coding-agent/src/prompts/tools/learn.md new file mode 100644 index 000000000..ed412ad5b --- /dev/null +++ b/packages/coding-agent/src/prompts/tools/learn.md @@ -0,0 +1,7 @@ +Capture a reusable lesson into long-term memory, and optionally mint or enhance a managed skill in the same call. + +Use after solving something whose insight will pay off again: a non-obvious fix, a project convention you had to discover, a workflow that worked. The `memory` field is the durable, self-contained lesson — include what, when, and why so a future session understands it without this conversation. + +Provide the optional `skill` object when the lesson is a repeatable *procedure* worth codifying as a `SKILL.md` (not just a fact). Managed skills are written to an isolated directory (`~/.omp/agent/managed-skills`) and are surfaced like normal skills next session. They NEVER touch user-authored skills. `body` is the SKILL.md content in markdown — do not include frontmatter; it is generated from `name` and `description`. Use `action: "update"` to enhance an existing managed skill. + +Capture sparingly and specifically. One strong, reusable lesson beats several vague ones. diff --git a/packages/coding-agent/src/prompts/tools/manage-skill.md b/packages/coding-agent/src/prompts/tools/manage-skill.md new file mode 100644 index 000000000..d957964f0 --- /dev/null +++ b/packages/coding-agent/src/prompts/tools/manage-skill.md @@ -0,0 +1,9 @@ +Create, update, or delete a managed skill — a `SKILL.md` written to an isolated directory (`~/.omp/agent/managed-skills`) and surfaced like a normal skill in future sessions. + +Managed skills are for repeatable procedures worth codifying: a setup sequence, a debugging recipe, a project-specific workflow. They are kept separate from user-authored skills and this tool NEVER edits those. + +- `action: "create"` — requires `name`, `description`, and `body`. Fails if the skill already exists. +- `action: "update"` — requires `name`, `description`, and `body`. Fails if the skill does not exist. Overwrites the body. +- `action: "delete"` — requires `name`. Fails if the skill does not exist. + +`name` is kebab-case (lowercase letters, digits, hyphens). `description` is a single line stating when to use the skill — it drives discovery, so make it specific. `body` is the SKILL.md content in markdown; do not include frontmatter (it is generated from `name` and `description`). diff --git a/packages/coding-agent/src/sdk.ts b/packages/coding-agent/src/sdk.ts index bc86d344b..ee98c806f 100644 --- a/packages/coding-agent/src/sdk.ts +++ b/packages/coding-agent/src/sdk.ts @@ -36,6 +36,7 @@ import { } from "@oh-my-pi/pi-utils"; import chalk from "chalk"; import { type AsyncJob, AsyncJobManager } from "./async"; +import { AutoLearnController, buildAutoLearnInstructions } from "./autolearn/controller"; import { loadCapability } from "./capability"; import { type Rule, ruleCapability, setActiveRules } from "./capability/rule"; import { bucketRules } from "./capability/rule-buckets"; @@ -2114,13 +2115,27 @@ export async function createAgentSession(options: CreateAgentSessionOptions = {} const memoryBackend = await resolveMemoryBackend(settings); const memoryInstructions = await memoryBackend.buildDeveloperInstructions(agentDir, settings, session); - // Build combined append prompt: memory instructions + MCP server instructions. - // For UI sessions MCP discovery is deferred, so `getServerInstructions()` is - // empty until the background connect completes; the rebuild that - // `refreshMCPTools` triggers post-discovery then picks up the now-connected - // servers' instructions, so they join the prompt for the rest of the session. + // Build combined append prompt: memory instructions + auto-learn guidance + // + MCP server instructions. For UI sessions MCP discovery is deferred, so + // `getServerInstructions()` is empty until the background connect completes; + // the rebuild that `refreshMCPTools` triggers post-discovery then picks up + // the now-connected servers' instructions, so they join the prompt for the + // rest of the session. const serverInstructions = mcpManager?.getServerInstructions(); - let appendPrompt: string | undefined = memoryInstructions ?? undefined; + // Drive guidance off the auto-learn BUILTINS that createTools actually built + // (provenance, not just an active name): `builtInToolNames` excludes a + // custom/extension tool that merely shares the name, and reflects the + // session-start build — so a subagent that filtered them out, a mid-session + // enable that never built them, or a same-named custom tool while auto-learn + // is off all get no guidance. + const autoLearnInstructions = buildAutoLearnInstructions({ + manageSkill: builtInToolNames.includes("manage_skill"), + learn: builtInToolNames.includes("learn"), + }); + const appendParts: string[] = []; + if (memoryInstructions) appendParts.push(memoryInstructions); + if (autoLearnInstructions) appendParts.push(autoLearnInstructions); + let appendPrompt: string | undefined = appendParts.length > 0 ? appendParts.join("\n\n") : undefined; if (serverInstructions && serverInstructions.size > 0) { const parts: string[] = []; if (appendPrompt) parts.push(appendPrompt); @@ -2186,6 +2201,20 @@ export async function createAgentSession(options: CreateAgentSessionOptions = {} ) { explicitlyRequestedToolNames.push("yield"); } + // Auto-learn builtins are force-included into the registry by `createTools` + // for enabled top-level sessions (tools/index.ts), but — like `yield` above — + // an explicit `toolNames` list would otherwise drop them from the ACTIVE set, + // leaving the nudge/guidance pointing at tools the model cannot call. Activate + // exactly the builtins createTools built (`builtInToolNames` — provenance, so a + // same-named custom/extension tool is never force-activated when auto-learn is + // off) to keep guidance, controller, and the active set consistent. + if (explicitlyRequestedToolNames) { + for (const name of ["manage_skill", "learn"]) { + if (builtInToolNames.includes(name) && !explicitlyRequestedToolNames.includes(name)) { + explicitlyRequestedToolNames.push(name); + } + } + } const requestedToolNames = explicitlyRequestedToolNames ?? toolNamesFromRegistry; const normalizedRequested = requestedToolNames.filter(name => toolRegistry.has(name)); const requestedToolNameSet = new Set(normalizedRequested); @@ -2665,6 +2694,23 @@ export async function createAgentSession(options: CreateAgentSessionOptions = {} } } + // Install the auto-learn post-stop nudge controller synchronously, before + // the possibly-slow memory backend starts, once per top-level session, when + // the feature is enabled at session start. A fast first turn must not finish + // before the listener subscribes, or its tool events are missed. + // + // Gated on `autolearn.enabled` to match the tools: `createTools` builds the + // `learn`/`manage_skill` registry ONCE at session start and no settings + // change rebuilds it, so installing the controller while disabled would let a + // mid-session enable fire a nudge pointing at tools the session never built. + // Activation is therefore a session-start decision for BOTH the controller + // and the tools; the fire-time re-check in `#onAgentEnd` still handles a + // mid-session DISABLE. The subscription lives for the session's lifetime; the + // reference is intentionally discarded (the listener retains it). + if (settings.get("autolearn.enabled") && taskDepth === 0) { + new AutoLearnController({ session, settings }); + } + logger.time("startMemoryStartupTask", async () => { const memoryBackend = await resolveMemoryBackend(settings); await memoryBackend.start({ diff --git a/packages/coding-agent/src/session/agent-session.ts b/packages/coding-agent/src/session/agent-session.ts index 1f33d2c23..957952f0c 100644 --- a/packages/coding-agent/src/session/agent-session.ts +++ b/packages/coding-agent/src/session/agent-session.ts @@ -5217,11 +5217,17 @@ export class AgentSession { * - Streaming: queue as steer/follow-up or store for next turn * - Not streaming + triggerTurn: appends to state/session, starts new turn unless the client cannot own it * - Not streaming + no trigger: appends to state/session, no turn + * + * @returns true iff this call synchronously started a new turn (awaited + * `agent.prompt`); false when the message was queued/appended without a turn + * — including when `triggerTurn` is downgraded because the client defers + * agent-initiated turns. Callers that must mirror the resulting `agent_end` + * use this to avoid acting on a turn that never ran. */ async sendCustomMessage( message: Pick, "customType" | "content" | "display" | "details" | "attribution">, options?: { triggerTurn?: boolean; deliverAs?: "steer" | "followUp" | "nextTurn"; queueChipText?: string }, - ): Promise { + ): Promise { const details = options?.queueChipText && options.deliverAs !== "nextTurn" ? ({ @@ -5245,7 +5251,7 @@ export class AgentSession { if (this.isStreaming) { if (options?.deliverAs === "nextTurn") { this.#queueHiddenNextTurnMessage(normalizedAppMessage, options?.triggerTurn ?? false); - return; + return false; } if (options?.deliverAs === "followUp") { @@ -5254,17 +5260,17 @@ export class AgentSession { this.agent.steer(normalizedAppMessage); } this.#scheduleIdleQueueDrain(); - return; + return false; } if (options?.deliverAs === "nextTurn") { if (options?.triggerTurn) { if (this.#clientBridge?.deferAgentInitiatedTurns && !this.#allowAcpAgentInitiatedTurns) { this.#queueHiddenNextTurnMessage(normalizedAppMessage, false); - return; + return false; } await this.agent.prompt(normalizedAppMessage); - return; + return true; } this.agent.appendMessage(normalizedAppMessage); this.sessionManager.appendCustomMessageEntry( @@ -5274,16 +5280,16 @@ export class AgentSession { message.details, message.attribution ?? "agent", ); - return; + return false; } if (options?.triggerTurn) { if (this.#clientBridge?.deferAgentInitiatedTurns && !this.#allowAcpAgentInitiatedTurns) { this.#queueHiddenNextTurnMessage(normalizedAppMessage, false); - return; + return false; } await this.agent.prompt(normalizedAppMessage); - return; + return true; } this.agent.appendMessage(normalizedAppMessage); @@ -5294,6 +5300,7 @@ export class AgentSession { message.details, message.attribution ?? "agent", ); + return false; } /** diff --git a/packages/coding-agent/src/task/executor.ts b/packages/coding-agent/src/task/executor.ts index 8da186151..5d7f4b93d 100644 --- a/packages/coding-agent/src/task/executor.ts +++ b/packages/coding-agent/src/task/executor.ts @@ -2005,7 +2005,7 @@ export async function runSubprocess(options: ExecutorOptions): Promise[] = []; + const pendingExtensionMessages: Promise[] = []; if (extensionRunner) { extensionRunner.initialize( { diff --git a/packages/coding-agent/src/tools/index.ts b/packages/coding-agent/src/tools/index.ts index 82099eeda..6b5256433 100644 --- a/packages/coding-agent/src/tools/index.ts +++ b/packages/coding-agent/src/tools/index.ts @@ -46,6 +46,8 @@ import { GithubTool } from "./gh"; import { InspectImageTool } from "./inspect-image"; import { IrcTool, isIrcEnabled } from "./irc"; import { JobTool } from "./job"; +import { LearnTool } from "./learn"; +import { ManageSkillTool } from "./manage-skill"; import { MemoryEditTool } from "./memory-edit"; import { MemoryRecallTool } from "./memory-recall"; import { MemoryReflectTool } from "./memory-reflect"; @@ -84,6 +86,8 @@ export * from "./image-gen"; export * from "./inspect-image"; export * from "./irc"; export * from "./job"; +export * from "./learn"; +export * from "./manage-skill"; export * from "./memory-edit"; export * from "./memory-recall"; export * from "./memory-reflect"; @@ -439,6 +443,8 @@ export const BUILTIN_TOOLS: Record = { retain: MemoryRetainTool.createIf, recall: MemoryRecallTool.createIf, reflect: MemoryReflectTool.createIf, + learn: LearnTool.createIf, + manage_skill: ManageSkillTool.createIf, }; export const HIDDEN_TOOLS: Record = { @@ -519,6 +525,21 @@ export async function createTools(session: ToolSession, toolNames?: string[]): P if (!requestedTools.includes(name)) requestedTools.push(name); } } + // Auto-learn tools are gated by `autolearn.enabled` but, like the memory + // tools above, must also be force-included into an explicit requestedTools + // list so a restricted top-level session whose controller/guidance is + // active still exposes the tools the nudge points at. Gated to top-level + // (taskDepth 0): the controller only runs there, so a subagent's explicit + // tool whitelist must never be silently widened with write-capable tools. + if (session.settings.get("autolearn.enabled") && (session.taskDepth ?? 0) === 0) { + if (!requestedTools.includes("manage_skill")) requestedTools.push("manage_skill"); + if ( + ["hindsight", "mnemopi", "local"].includes(session.settings.get("memory.backend") ?? "") && + !requestedTools.includes("learn") + ) { + requestedTools.push("learn"); + } + } } // Resolve effective tool discovery mode. // tools.discoveryMode controls the new modes; mcp.discoveryMode remains a back-compat alias for "mcp-only". @@ -552,6 +573,14 @@ export async function createTools(session: ToolSession, toolNames?: string[]): P if (name === "retain" || name === "recall" || name === "reflect") { return ["hindsight", "mnemopi"].includes(session.settings.get("memory.backend") ?? ""); } + if (name === "manage_skill") return session.settings.get("autolearn.enabled") && (session.taskDepth ?? 0) === 0; + if (name === "learn") { + return ( + session.settings.get("autolearn.enabled") && + (session.taskDepth ?? 0) === 0 && + ["hindsight", "mnemopi", "local"].includes(session.settings.get("memory.backend") ?? "") + ); + } if (name === "task") { return canSpawnAtDepth(session.settings.get("task.maxRecursionDepth") ?? 2, session.taskDepth ?? 0); } diff --git a/packages/coding-agent/src/tools/learn.ts b/packages/coding-agent/src/tools/learn.ts new file mode 100644 index 000000000..7faabe26f --- /dev/null +++ b/packages/coding-agent/src/tools/learn.ts @@ -0,0 +1,119 @@ +import type { AgentTool, AgentToolResult } from "@oh-my-pi/pi-agent-core"; +import { z } from "zod/v4"; +import { writeManagedSkill } from "../autolearn/managed-skills"; +import { localBackend } from "../memory-backend/local-backend"; +import learnDescription from "../prompts/tools/learn.md" with { type: "text" }; +import type { ToolSession } from "."; + +const learnSchema = z.object({ + memory: z.string().describe("the durable, self-contained lesson to remember (what, when, why)"), + context: z.string().describe("optional source context for the lesson").optional(), + skill: z + .object({ + action: z.enum(["create", "update"]), + name: z.string().describe("kebab-case skill name"), + description: z.string().describe("one-line description of when to use the skill"), + body: z.string().describe("the SKILL.md body in markdown (no frontmatter)"), + }) + .describe("also create or enhance a managed skill in the same call") + .optional(), +}); + +export type LearnParams = z.infer; + +/** + * Orchestrating "learn" tool: persists a lesson to long-term memory and, + * given a `skill` payload, mints/enhances a managed skill via the shared + * `writeManagedSkill` primitive. Gated behind `autolearn.enabled` plus a live + * memory backend — `hindsight`/`mnemopi` (remote/SQLite) or `local` (the + * file-based rollout backend, where lessons append to `learned.md`). + */ +export class LearnTool implements AgentTool { + readonly name = "learn"; + readonly approval = (args: unknown) => + (args as Partial).skill || this.session.settings.get("memory.backend") === "local" + ? "write" + : "read"; + readonly label = "Learn"; + readonly description = learnDescription; + readonly parameters = learnSchema; + readonly strict = true; + readonly loadMode = "essential" as const; + readonly summary = "Capture a reusable lesson to memory (and optionally a managed skill)"; + + constructor(private readonly session: ToolSession) {} + + static createIf(session: ToolSession): LearnTool | null { + if (!session.settings.get("autolearn.enabled")) return null; + const backend = session.settings.get("memory.backend"); + if (backend !== "hindsight" && backend !== "mnemopi" && backend !== "local") return null; + return new LearnTool(session); + } + + async execute(_id: string, params: LearnParams): Promise { + // 1) Persist the lesson to long-term memory (mirrors MemoryRetainTool). + const backend = this.session.settings.get("memory.backend"); + if (backend === "mnemopi") { + const state = this.session.getMnemopiSessionState?.(); + if (!state) { + throw new Error("Mnemopi backend is not initialised for this session."); + } + const id = state.rememberScoped(params.memory, { + source: "coding-agent-learn", + importance: 0.8, + metadata: { + session_id: state.sessionId, + cwd: state.session.sessionManager.getCwd(), + context: params.context ?? null, + tool: "learn", + }, + scope: "bank", + extract: true, + extractEntities: true, + veracity: "tool", + memoryType: "fact", + }); + // rememberScoped returns undefined when the retain failed (closed DB / + // disk error); mirror mnemopiBackend.save and fail loudly rather than + // reporting (and minting a skill for) a lesson that was silently dropped. + if (!id) { + throw new Error("Mnemopi did not store the lesson (no memory id returned)."); + } + } else if (backend === "local") { + const result = await localBackend.save?.( + { agentDir: this.session.settings.getAgentDir(), cwd: this.session.settings.getCwd() }, + { content: params.memory, context: params.context, source: "coding-agent-learn", importance: 0.8 }, + ); + if (!result || result.stored === 0) { + throw new Error("Lesson was empty after sanitization; nothing stored."); + } + } else { + const state = this.session.getHindsightSessionState?.(); + if (!state) { + throw new Error("Hindsight backend is not initialised for this session."); + } + state.enqueueRetain(params.memory, params.context); + } + + // 2) Optionally mint/enhance a managed skill. A failure here is surfaced + // as a partial outcome — the lesson is already persisted. + if (params.skill) { + try { + await writeManagedSkill(params.skill); + } catch (err) { + const reason = err instanceof Error ? err.message : String(err); + throw new Error(`Lesson stored, but the managed skill could not be written: ${reason}`); + } + const verb = params.skill.action === "create" ? "Created" : "Updated"; + return { + content: [{ type: "text", text: `Lesson stored. ${verb} managed skill "${params.skill.name}".` }], + details: { skill: params.skill.name }, + }; + } + + return { + content: [{ type: "text", text: "Lesson stored." }], + details: { skill: null }, + }; + } +} diff --git a/packages/coding-agent/src/tools/manage-skill.ts b/packages/coding-agent/src/tools/manage-skill.ts new file mode 100644 index 000000000..12f7c8e12 --- /dev/null +++ b/packages/coding-agent/src/tools/manage-skill.ts @@ -0,0 +1,81 @@ +import * as path from "node:path"; +import type { AgentTool, AgentToolResult } from "@oh-my-pi/pi-agent-core"; +import { z } from "zod/v4"; +import { deleteManagedSkill, getManagedSkillsDir, writeManagedSkill } from "../autolearn/managed-skills"; +import manageSkillDescription from "../prompts/tools/manage-skill.md" with { type: "text" }; +import type { ToolSession } from "."; + +const manageSkillSchema = z + .object({ + action: z.enum(["create", "update", "delete"]), + name: z.string().describe("kebab-case skill name"), + description: z + .string() + .describe("one-line description of when to use the skill (required for create/update)") + .optional(), + body: z + .string() + .describe("the SKILL.md body in markdown, no frontmatter (required for create/update)") + .optional(), + }) + // Enforce the action/field contract at validation time rather than only in + // execute. Kept as a cross-field refine (not a discriminated union) so the + // wire schema stays a single root object — strict structured-output mode and + // the Anthropic tool-schema builder both require that. + .refine(p => p.action === "delete" || (p.description !== undefined && p.body !== undefined), { + message: '"create" and "update" require both "description" and "body".', + path: ["description"], + }); + +export type ManageSkillParams = z.infer; + +/** + * Direct create/update/delete of isolated managed skills. Gated behind + * `autolearn.enabled`; backend-independent (the skill side is standalone). + */ +export class ManageSkillTool implements AgentTool { + readonly name = "manage_skill"; + readonly approval = "write" as const; + readonly label = "Manage Skill"; + readonly description = manageSkillDescription; + readonly parameters = manageSkillSchema; + readonly strict = true; + readonly loadMode = "essential" as const; + readonly summary = "Create, update, or delete an isolated managed skill"; + + // No session state needed: createIf reads settings; writes target the + // home-based managed-skills dir directly. + static createIf(session: ToolSession): ManageSkillTool | null { + if (!session.settings.get("autolearn.enabled")) return null; + return new ManageSkillTool(); + } + + async execute(_id: string, params: ManageSkillParams): Promise { + if (params.action === "delete") { + await deleteManagedSkill(params.name); + return { + content: [{ type: "text", text: `Deleted managed skill "${params.name}".` }], + details: { action: "delete", name: params.name }, + }; + } + + // Defensive narrowing: the schema refine already rejects create/update + // without both fields, so this is unreachable for valid input — it only + // proves the strings are present to `writeManagedSkill`'s typed contract. + if (!params.description || !params.body) { + throw new Error(`"${params.action}" requires both "description" and "body".`); + } + const { path: skillPath } = await writeManagedSkill({ + action: params.action, + name: params.name, + description: params.description, + body: params.body, + }); + const relativePath = path.relative(getManagedSkillsDir(), skillPath); + const verb = params.action === "create" ? "Created" : "Updated"; + return { + content: [{ type: "text", text: `${verb} managed skill "${params.name}" (managed-skills/${relativePath}).` }], + details: { action: params.action, name: params.name }, + }; + } +} diff --git a/packages/coding-agent/test/autolearn-controller.test.ts b/packages/coding-agent/test/autolearn-controller.test.ts new file mode 100644 index 000000000..ffb55eb93 --- /dev/null +++ b/packages/coding-agent/test/autolearn-controller.test.ts @@ -0,0 +1,229 @@ +import { describe, expect, it } from "bun:test"; +import { AutoLearnController, buildAutoLearnInstructions } from "@oh-my-pi/pi-coding-agent/autolearn/controller"; +import { Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; +import type { AgentSession, AgentSessionEvent } from "@oh-my-pi/pi-coding-agent/session/agent-session"; + +interface CapturedNudge { + message: { customType: string; content: string; display?: boolean; attribution?: string }; + options?: { deliverAs?: string; triggerTurn?: boolean }; +} + +class FakeSession { + readonly listeners: Array<(event: AgentSessionEvent) => void> = []; + readonly sent: CapturedNudge[] = []; + planEnabled = false; + goalEnabled = false; + /** Whether a triggerTurn dispatch actually starts a synthetic turn. */ + turnStarts = true; + /** Force the dispatch to reject (models a failed send). */ + failSend = false; + + subscribe(listener: (event: AgentSessionEvent) => void): () => void { + this.listeners.push(listener); + return () => {}; + } + + async sendCustomMessage(message: CapturedNudge["message"], options?: CapturedNudge["options"]): Promise { + if (this.failSend) throw new Error("send failed"); + this.sent.push({ message, options }); + // Mirror AgentSession: a turn starts only when triggerTurn is honored. + return options?.triggerTurn === true && this.turnStarts; + } + + getPlanModeState(): { enabled: boolean } | undefined { + return this.planEnabled ? { enabled: true } : undefined; + } + + getGoalModeState(): { enabled: boolean } | undefined { + return this.goalEnabled ? { enabled: true } : undefined; + } + + emit(event: AgentSessionEvent): void { + for (const listener of [...this.listeners]) listener(event); + } + + toolCalls(n: number): void { + for (let i = 0; i < n; i++) { + this.emit({ type: "tool_execution_end", toolCallId: `t${i}`, toolName: "read", result: null }); + } + } + + agentEnd(): void { + this.emit({ type: "agent_end", messages: [] }); + } +} + +function install(session: FakeSession, overrides: Record = {}): Settings { + const settings = Settings.isolated({ "autolearn.enabled": true, ...overrides }); + new AutoLearnController({ session: session as unknown as AgentSession, settings }); + return settings; +} + +describe("AutoLearnController", () => { + it("fires one passive nudge once the tool-call threshold is met", () => { + const session = new FakeSession(); + install(session); + session.toolCalls(5); + session.agentEnd(); + + expect(session.sent).toHaveLength(1); + expect(session.sent[0]?.message.customType).toBe("autolearn-nudge"); + expect(session.sent[0]?.message.display).toBe(false); + expect(session.sent[0]?.options?.deliverAs).toBe("nextTurn"); + expect(session.sent[0]?.options?.triggerTurn).toBe(false); + }); + + it("does not nudge below the threshold", () => { + const session = new FakeSession(); + install(session); + session.toolCalls(4); + session.agentEnd(); + expect(session.sent).toHaveLength(0); + }); + + it("does not nudge during plan mode", () => { + const session = new FakeSession(); + session.planEnabled = true; + install(session); + session.toolCalls(5); + session.agentEnd(); + expect(session.sent).toHaveLength(0); + }); + it("does not combine tool calls across separate sub-threshold turns", () => { + const session = new FakeSession(); + install(session); + session.toolCalls(3); + session.agentEnd(); + session.toolCalls(3); + session.agentEnd(); + // Neither turn reached the threshold; the counter must not accumulate. + expect(session.sent).toHaveLength(0); + }); + + it("discards plan-mode tool calls instead of leaking them into the next turn", () => { + const session = new FakeSession(); + session.planEnabled = true; + install(session); + session.toolCalls(5); + session.agentEnd(); // plan mode: no fire, counter reset + session.planEnabled = false; + session.toolCalls(1); + session.agentEnd(); // 1 < threshold -> no fire (no plan-mode leak) + expect(session.sent).toHaveLength(0); + }); + + it("stops nudging when autolearn is disabled mid-session", () => { + const session = new FakeSession(); + // Enable via the global layer (not an isolated override) so the live flag + // can be flipped and the controller's fire-time re-check is exercised. + const settings = Settings.isolated({}); + settings.set("autolearn.enabled", true); + new AutoLearnController({ session: session as unknown as AgentSession, settings }); + session.toolCalls(5); + session.agentEnd(); + expect(session.sent).toHaveLength(1); // fires while enabled + settings.set("autolearn.enabled", false); + session.toolCalls(5); + session.agentEnd(); + expect(session.sent).toHaveLength(1); // no new nudge after disable + // The disabled stop must NOT leave its tool calls queued: re-enabling and + // doing a sub-threshold turn must not fire from leaked counts. + settings.set("autolearn.enabled", true); + session.toolCalls(1); + session.agentEnd(); + expect(session.sent).toHaveLength(1); + }); + + it("does not nudge during goal mode and leaks no suppression latch", () => { + const session = new FakeSession(); + session.goalEnabled = true; + install(session, { "autolearn.autoContinue": true }); + session.toolCalls(5); + session.agentEnd(); + // Goal mode owns the continuation; auto-learn stays out of the loop. + expect(session.sent).toHaveLength(0); + // The skipped stop must not arm suppression for the next non-goal stop. + session.goalEnabled = false; + session.toolCalls(5); + session.agentEnd(); + expect(session.sent).toHaveLength(1); + }); + + it("auto-runs a capture turn and suppresses exactly one follow-up agent_end", () => { + const session = new FakeSession(); + install(session, { "autolearn.autoContinue": true }); + + session.toolCalls(5); + session.agentEnd(); + expect(session.sent).toHaveLength(1); + expect(session.sent[0]?.options?.triggerTurn).toBe(true); + + // The synthetic capture turn's agent_end is swallowed. + session.toolCalls(5); + session.agentEnd(); + expect(session.sent).toHaveLength(1); + + // Suppression is one-shot: the next qualifying stop fires again. + session.toolCalls(5); + session.agentEnd(); + expect(session.sent).toHaveLength(2); + }); + + it("disarms suppression when the capture turn is deferred (not started)", async () => { + const session = new FakeSession(); + // triggerTurn honored but downgraded to a queue: no synthetic agent_end. + session.turnStarts = false; + install(session, { "autolearn.autoContinue": true }); + session.toolCalls(5); + session.agentEnd(); + expect(session.sent).toHaveLength(1); + expect(session.sent[0]?.options?.triggerTurn).toBe(true); + await Bun.sleep(1); // flush the async disarm + // No turn ran, so the next real stop must still nudge. + session.toolCalls(5); + session.agentEnd(); + expect(session.sent).toHaveLength(2); + }); + + it("disarms suppression when the capture-turn dispatch fails", async () => { + const session = new FakeSession(); + session.failSend = true; + install(session, { "autolearn.autoContinue": true }); + session.toolCalls(5); + session.agentEnd(); // dispatch rejects: armed, then disarmed in .catch + expect(session.sent).toHaveLength(0); + await Bun.sleep(1); // flush the async disarm + session.failSend = false; + session.toolCalls(5); + session.agentEnd(); + expect(session.sent).toHaveLength(1); + }); + + it("respects a custom minToolCalls threshold", () => { + const session = new FakeSession(); + install(session, { "autolearn.minToolCalls": 2 }); + session.toolCalls(2); + session.agentEnd(); + expect(session.sent).toHaveLength(1); + }); +}); + +describe("buildAutoLearnInstructions", () => { + it("returns null when manage_skill is not in the active tool set", () => { + expect(buildAutoLearnInstructions({ manageSkill: false, learn: false })).toBeNull(); + // learn without manage_skill still yields no guidance (manage_skill gates it). + expect(buildAutoLearnInstructions({ manageSkill: false, learn: true })).toBeNull(); + }); + + it("includes the learn addendum when the learn tool is present", () => { + const text = buildAutoLearnInstructions({ manageSkill: true, learn: true }); + expect(text).toContain("manage_skill"); + expect(text).toContain("long-term memory"); + }); + + it("omits the learn addendum when only manage_skill is present", () => { + const text = buildAutoLearnInstructions({ manageSkill: true, learn: false }); + expect(text).toContain("manage_skill"); + expect(text).not.toContain("long-term memory"); + }); +}); diff --git a/packages/coding-agent/test/autolearn-discovery.test.ts b/packages/coding-agent/test/autolearn-discovery.test.ts new file mode 100644 index 000000000..caf6ce952 --- /dev/null +++ b/packages/coding-agent/test/autolearn-discovery.test.ts @@ -0,0 +1,129 @@ +import { afterEach, beforeEach, describe, expect, it, spyOn } from "bun:test"; +import * as fs from "node:fs/promises"; +import * as os from "node:os"; +import * as path from "node:path"; +import { getManagedSkillsDir } from "@oh-my-pi/pi-coding-agent/autolearn/managed-skills"; +import "@oh-my-pi/pi-coding-agent/discovery"; +import { loadSkills } from "@oh-my-pi/pi-coding-agent/extensibility/skills"; + +async function writeSkill(dir: string, name: string, description: string): Promise { + const file = path.join(dir, name, "SKILL.md"); + await fs.mkdir(path.dirname(file), { recursive: true }); + await fs.writeFile(file, ["---", `description: ${description}`, "---", "", `# ${name}`].join("\n")); +} + +describe("managed-skills discovery", () => { + let tempHome: string; + let tempCwd: string; + let managedDir: string; + let authoredDir: string; + + beforeEach(async () => { + tempHome = await fs.mkdtemp(path.join(os.tmpdir(), "omp-managed-disco-home-")); + // cwd MUST live under the fake home so loadSkills' ancestor walk is bounded + // and cannot pick up ambient /tmp/.omp or /.omp fixtures (full-suite-safe). + tempCwd = path.join(tempHome, "work"); + await fs.mkdir(tempCwd, { recursive: true }); + spyOn(os, "homedir").mockReturnValue(tempHome); + managedDir = getManagedSkillsDir(); + // Authored user skills live in the sibling `skills/` dir under .../agent. + authoredDir = path.join(path.dirname(managedDir), "skills"); + }); + + afterEach(async () => { + spyOn(os, "homedir").mockRestore(); + await fs.rm(tempHome, { recursive: true, force: true }); + }); + + it("surfaces a managed skill tagged with the omp-managed provider", async () => { + await writeSkill(managedDir, "foo", "A managed skill."); + const { skills } = await loadSkills({ cwd: tempCwd }); + const foo = skills.find(s => s.name === "foo"); + expect(foo).toBeDefined(); + expect(foo?.source).toBe("omp-managed:user"); + }); + + it("lets an authored skill win a name collision and drops the managed one", async () => { + await writeSkill(authoredDir, "bar", "Authored bar."); + await writeSkill(managedDir, "bar", "Managed bar."); + const { skills } = await loadSkills({ cwd: tempCwd }); + const bars = skills.filter(s => s.name === "bar"); + expect(bars).toHaveLength(1); + expect(bars[0]?.source).toBe("native:user"); + expect(skills.some(s => s.name === "bar" && s.source === "omp-managed:user")).toBe(false); + }); + + it("lets an authored skill from a NON-native provider win over a managed skill", async () => { + // `.agents/skills` is the `agents` provider — a different provider than the + // one that discovers managed skills. Authored must still win globally. + await writeSkill(path.join(tempHome, ".agents", "skills"), "baz", "Authored baz (.agents)."); + await writeSkill(managedDir, "baz", "Managed baz."); + const { skills } = await loadSkills({ cwd: tempCwd }); + const bazzes = skills.filter(s => s.name === "baz"); + expect(bazzes).toHaveLength(1); + expect(bazzes[0]?.source).toBe("agents:user"); + expect(skills.some(s => s.name === "baz" && s.source === "omp-managed:user")).toBe(false); + }); + + it("lets a custom-directory authored skill win over a managed skill", async () => { + // Custom directories are merged AFTER loadCapability, so this exercises the + // skills.ts dead-last backstop rather than capability-level priority dedup. + const customDir = path.join(tempHome, "custom-skills"); + await writeSkill(customDir, "qux", "Authored qux (custom)."); + await writeSkill(managedDir, "qux", "Managed qux."); + const { skills } = await loadSkills({ cwd: tempCwd, customDirectories: [customDir] }); + const quxes = skills.filter(s => s.name === "qux"); + expect(quxes).toHaveLength(1); + expect(quxes[0]?.source).toBe("custom:user"); + }); + + it("keeps a managed skill visible even when a disabled provider has the same name", async () => { + // loadCapability dedupes before source filtering, so a fully-DISABLED higher- + // priority authored skill must not consume the managed fallback. claude is + // discovered at user AND (because cwd is under home) project level, so both + // toggles must be off to truly disable it. + await writeSkill(path.join(tempHome, ".claude", "skills"), "dis", "Disabled claude dis."); + await writeSkill(managedDir, "dis", "Managed dis."); + const { skills } = await loadSkills({ + cwd: tempCwd, + enableClaudeUser: false, + enableClaudeProject: false, + }); + const dises = skills.filter(s => s.name === "dis"); + expect(dises).toHaveLength(1); + expect(dises[0]?.source).toBe("omp-managed:user"); + }); + + it("defers a managed skill to an ENABLED authored skill hidden behind a disabled higher-priority one", async () => { + // claude (priority 80, fully disabled) shadows agents (70, enabled) at + // capability dedup; agents survives only in result.all. Managed must NOT mask + // the enabled authored name, so no omp-managed skill is surfaced here. + await writeSkill(path.join(tempHome, ".claude", "skills"), "shadowed", "Disabled claude."); + await writeSkill(path.join(tempHome, ".agents", "skills"), "shadowed", "Enabled agents."); + await writeSkill(managedDir, "shadowed", "Managed shadowed."); + const { skills } = await loadSkills({ + cwd: tempCwd, + enableClaudeUser: false, + enableClaudeProject: false, + }); + expect(skills.some(s => s.name === "shadowed" && s.source === "omp-managed:user")).toBe(false); + }); + + it("skips a managed skill whose on-disk frontmatter name is unsafe", async () => { + const dir = path.join(managedDir, "evil-holder"); + await fs.mkdir(dir, { recursive: true }); + await fs.writeFile( + path.join(dir, "SKILL.md"), + ["---", 'name: "evil"', "description: Evil.", "---", "", "# evil"].join("\n"), + ); + const { skills } = await loadSkills({ cwd: tempCwd }); + expect(skills.some(s => s.name.includes("<"))).toBe(false); + expect(skills.some(s => s.source === "omp-managed:user")).toBe(false); + }); + + it("is a no-op when the managed dir is absent", async () => { + const { skills, warnings } = await loadSkills({ cwd: tempCwd }); + expect(skills.some(s => s.source === "omp-managed:user")).toBe(false); + expect(warnings.some(w => w.message.includes("managed-skills"))).toBe(false); + }); +}); diff --git a/packages/coding-agent/test/autolearn-learn-local.test.ts b/packages/coding-agent/test/autolearn-learn-local.test.ts new file mode 100644 index 000000000..7501e970e --- /dev/null +++ b/packages/coding-agent/test/autolearn-learn-local.test.ts @@ -0,0 +1,265 @@ +import { afterEach, beforeEach, describe, expect, it, spyOn } from "bun:test"; +import * as fs from "node:fs/promises"; +import * as os from "node:os"; +import * as path from "node:path"; +import { Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; +import { + buildMemoryToolDeveloperInstructions, + getMemoryRoot, + saveLearnedLesson, +} from "@oh-my-pi/pi-coding-agent/memories"; +import { localBackend } from "@oh-my-pi/pi-coding-agent/memory-backend/local-backend"; +import type { ToolSession } from "@oh-my-pi/pi-coding-agent/tools"; +import { LearnTool } from "@oh-my-pi/pi-coding-agent/tools/learn"; + +Bun.env.PI_PYTHON_SKIP_CHECK = "1"; + +describe("learned-lesson storage (local backend)", () => { + let tmp: string; + let agentDir: string; + let projCwd: string; + let learnedFile: string; + + beforeEach(async () => { + tmp = await fs.mkdtemp(path.join(os.tmpdir(), "omp-learned-")); + agentDir = path.join(tmp, "agent"); + projCwd = path.join(tmp, "proj"); + learnedFile = path.join(getMemoryRoot(agentDir, projCwd), "learned.md"); + }); + afterEach(async () => { + await fs.rm(tmp, { recursive: true, force: true }); + }); + + it("appends a bullet, normalizes whitespace, and inlines context", async () => { + const result = await saveLearnedLesson(agentDir, projCwd, { + content: "Prefer Bun.file\nover\n\nreadFileSync.", + context: "from the build", + }); + expect(result.stored).toBe(1); + expect(await Bun.file(learnedFile).text()).toBe( + "- Prefer Bun.file over readFileSync. _(context: from the build)_\n", + ); + }); + + it("redacts secrets, including provider token prefixes, before persisting", async () => { + const ghToken = `ghp_${"A".repeat(36)}`; + await saveLearnedLesson(agentDir, projCwd, { + content: `API token-abcdefghijklmnop and ${ghToken} leaked into logs`, + }); + const text = await Bun.file(learnedFile).text(); + expect(text).toContain("[REDACTED]"); + expect(text).not.toContain("abcdefghijklmnop"); + expect(text).not.toContain(ghToken); + }); + + it("redacts a token even when a delimiter splits it (strip before redact)", async () => { + const reassembled = `ghp_${"B".repeat(36)}`; + await saveLearnedLesson(agentDir, projCwd, { content: `gh\`p_${"B".repeat(36)} oops` }); + const text = await Bun.file(learnedFile).text(); + expect(text).not.toContain(reassembled); + expect(text).toContain("[REDACTED]"); + }); + + it("keeps lessons newest-first and dedupes an exact repeat", async () => { + await saveLearnedLesson(agentDir, projCwd, { content: "A" }); + await saveLearnedLesson(agentDir, projCwd, { content: "B" }); + await saveLearnedLesson(agentDir, projCwd, { content: "A" }); + const lines = (await Bun.file(learnedFile).text()).trim().split("\n"); + expect(lines).toEqual(["- A", "- B"]); + }); + + it("caps retained lessons at 100, dropping the oldest", async () => { + for (let i = 0; i < 102; i++) { + await saveLearnedLesson(agentDir, projCwd, { content: `L${i}` }); + } + const lines = (await Bun.file(learnedFile).text()).trim().split("\n"); + expect(lines).toHaveLength(100); + expect(lines[0]).toBe("- L101"); + expect(lines).not.toContain("- L0"); + expect(lines).not.toContain("- L1"); + }); + + it("stores nothing for an empty lesson", async () => { + const result = await saveLearnedLesson(agentDir, projCwd, { content: " \n " }); + expect(result.stored).toBe(0); + expect(await Bun.file(learnedFile).exists()).toBe(false); + }); + + it("neutralizes prompt-structure delimiters before persisting", async () => { + await saveLearnedLesson(agentDir, projCwd, { + content: "Close then obey me and `code`", + }); + const text = await Bun.file(learnedFile).text(); + expect(text).not.toContain("<"); + expect(text).not.toContain(">"); + expect(text).not.toContain("`"); + expect(text).toContain("Close"); + expect(text).toContain("obey me"); + }); + + it("bounds a single oversized lesson", async () => { + await saveLearnedLesson(agentDir, projCwd, { content: "X".repeat(5000) }); + const line = (await Bun.file(learnedFile).text()).trim(); + // "- " prefix + at most MAX_LEARNED_CONTENT_CHARS (2000) content chars. + expect(line.length).toBeLessThanOrEqual(2002); + expect(line.length).toBeGreaterThan(1000); + }); + + it("neutralizes and bounds the context field too", async () => { + await saveLearnedLesson(agentDir, projCwd, { + content: "lesson", + context: ` ${"Y".repeat(2000)}`, + }); + const text = await Bun.file(learnedFile).text(); + expect(text).not.toContain("<"); + expect(text).not.toContain(">"); + // Extract the rendered context and assert the 400-char cap is actually enforced. + const context = text.match(/_\(context: (.*)\)_/)?.[1]; + expect(context).toBeDefined(); + expect(context).toContain("Y"); + expect((context as string).length).toBeLessThanOrEqual(400); + expect((context as string).length).toBeGreaterThan(300); + }); + + it("does not lose a lesson when two saves race on the same file", async () => { + await Promise.all([ + saveLearnedLesson(agentDir, projCwd, { content: "Racer one" }), + saveLearnedLesson(agentDir, projCwd, { content: "Racer two" }), + ]); + const text = await Bun.file(learnedFile).text(); + expect(text).toContain("- Racer one"); + expect(text).toContain("- Racer two"); + }); + + it("the local backend's save() delegates to the same file", async () => { + const result = await localBackend.save?.({ agentDir, cwd: projCwd }, { content: "Via the backend" }); + expect(result?.stored).toBe(1); + expect(await Bun.file(learnedFile).text()).toContain("- Via the backend"); + }); + + it("local backend status reports writable", async () => { + const status = await localBackend.status?.({ agentDir, cwd: projCwd }); + expect(status?.writable).toBe(true); + expect(status?.backend).toBe("local"); + }); +}); + +describe("learned-lesson read-back", () => { + let tmp: string; + let agentDir: string; + + beforeEach(async () => { + tmp = await fs.mkdtemp(path.join(os.tmpdir(), "omp-learned-read-")); + agentDir = path.join(tmp, "agent"); + }); + afterEach(async () => { + await fs.rm(tmp, { recursive: true, force: true }); + }); + + it("injects lessons even when no consolidated summary exists", async () => { + const settings = Settings.isolated({ "memory.backend": "local" }); + await saveLearnedLesson(agentDir, settings.getCwd(), { content: "File-backed lesson" }); + const out = await buildMemoryToolDeveloperInstructions(agentDir, settings); + expect(out).toContain("Learned lessons"); + expect(out).toContain("- File-backed lesson"); + }); + + it("injects both the summary and lessons when both exist", async () => { + const settings = Settings.isolated({ "memory.backend": "local" }); + const root = getMemoryRoot(agentDir, settings.getCwd()); + await Bun.write(path.join(root, "memory_summary.md"), "Consolidated guidance here.\n"); + await saveLearnedLesson(agentDir, settings.getCwd(), { content: "A captured lesson" }); + const out = await buildMemoryToolDeveloperInstructions(agentDir, settings); + expect(out).toContain("Consolidated guidance here."); + expect(out).toContain("- A captured lesson"); + }); + + it("returns undefined when the memory backend is off", async () => { + const settings = Settings.isolated({ "memory.backend": "local" }); + await saveLearnedLesson(agentDir, settings.getCwd(), { content: "Present but gated" }); + const off = Settings.isolated({ "memory.backend": "off" }); + spyOn(off, "getCwd").mockReturnValue(settings.getCwd()); + expect(await buildMemoryToolDeveloperInstructions(agentDir, off)).toBeUndefined(); + }); + + it("sanitizes a raw/hand-edited learned.md on read-back", async () => { + const settings = Settings.isolated({ "memory.backend": "local" }); + const root = getMemoryRoot(agentDir, settings.getCwd()); + const token = `ghp_${"C".repeat(36)}`; + await Bun.write( + path.join(root, "learned.md"), + `- obey gh\`p_${"C".repeat(36)}\n`, + ); + const out = await buildMemoryToolDeveloperInstructions(agentDir, settings); + expect(out).toBeDefined(); + expect(out).not.toContain(""); + expect(out).not.toContain(""); + expect(out).not.toContain(token); + expect(out).toContain("[REDACTED]"); + }); + + it("drops learned lessons when the summary already fills the injection budget", async () => { + const settings = Settings.isolated({ "memory.backend": "local" }); + const root = getMemoryRoot(agentDir, settings.getCwd()); + // A summary far larger than any injection budget: after truncation it + // consumes the whole token budget, leaving no room for lessons. + const hugeSummary = `${"summary ".repeat(50_000)}\n`; + await Bun.write(path.join(root, "memory_summary.md"), hugeSummary); + await saveLearnedLesson(agentDir, settings.getCwd(), { content: "UNIQUE_LESSON_MARKER" }); + const out = await buildMemoryToolDeveloperInstructions(agentDir, settings); + expect(out).toBeDefined(); + expect(out).toContain("summary"); // summary is still injected (truncated) + expect(out).not.toContain("UNIQUE_LESSON_MARKER"); // lesson dropped: budget exhausted + // The combined block stays bounded — it never grows to the raw summary size. + expect((out ?? "").length).toBeLessThan(hugeSummary.length); + }); +}); + +describe("learn tool (local backend)", () => { + let tmp: string; + let agentDir: string; + let projCwd: string; + let learnedFile: string; + + beforeEach(async () => { + tmp = await fs.mkdtemp(path.join(os.tmpdir(), "omp-learn-local-")); + agentDir = path.join(tmp, "agent"); + projCwd = path.join(tmp, "proj"); + learnedFile = path.join(getMemoryRoot(agentDir, projCwd), "learned.md"); + }); + afterEach(async () => { + await fs.rm(tmp, { recursive: true, force: true }); + }); + + function localSession(): ToolSession { + const settings = Settings.isolated({ "autolearn.enabled": true, "memory.backend": "local" }); + spyOn(settings, "getAgentDir").mockReturnValue(agentDir); + spyOn(settings, "getCwd").mockReturnValue(projCwd); + return { + cwd: projCwd, + hasUI: false, + skipPythonPreflight: true, + getSessionFile: () => null, + getSessionSpawns: () => "*", + settings, + }; + } + + it("createIf returns a tool for the local backend", () => { + expect(LearnTool.createIf(localSession())).toBeInstanceOf(LearnTool); + }); + + it("tiers the local save as a write approval even without a skill payload", () => { + expect(new LearnTool(localSession()).approval({ memory: "x" })).toBe("write"); + }); + + it("execute writes the lesson to learned.md", async () => { + await new LearnTool(localSession()).execute("1", { memory: "A local tool lesson" }); + expect(await Bun.file(learnedFile).text()).toContain("- A local tool lesson"); + }); + + it("execute throws when the lesson is empty after sanitization", async () => { + await expect(new LearnTool(localSession()).execute("2", { memory: " " })).rejects.toThrow(/empty/i); + expect(await Bun.file(learnedFile).exists()).toBe(false); + }); +}); diff --git a/packages/coding-agent/test/autolearn-managed-skills.test.ts b/packages/coding-agent/test/autolearn-managed-skills.test.ts new file mode 100644 index 000000000..f7c44e200 --- /dev/null +++ b/packages/coding-agent/test/autolearn-managed-skills.test.ts @@ -0,0 +1,237 @@ +import { afterEach, beforeEach, describe, expect, it, spyOn } from "bun:test"; +import * as fs from "node:fs/promises"; +import * as os from "node:os"; +import * as path from "node:path"; +import { + deleteManagedSkill, + getManagedSkillsDir, + MAX_MANAGED_SKILL_BYTES, + sanitizeSkillName, + toSkillFrontmatter, + writeManagedSkill, +} from "@oh-my-pi/pi-coding-agent/autolearn/managed-skills"; +import { parseFrontmatter } from "@oh-my-pi/pi-utils"; + +describe("managed-skills primitives", () => { + let tempHome: string; + + beforeEach(async () => { + tempHome = await fs.mkdtemp(path.join(os.tmpdir(), "omp-managed-skills-")); + spyOn(os, "homedir").mockReturnValue(tempHome); + }); + + afterEach(async () => { + spyOn(os, "homedir").mockRestore(); + await fs.rm(tempHome, { recursive: true, force: true }); + }); + + const skillFile = (name: string) => path.join(getManagedSkillsDir(), name, "SKILL.md"); + + describe("sanitizeSkillName", () => { + it("rejects traversal, slashes, and empty names", () => { + expect(() => sanitizeSkillName("../escape")).toThrow(); + expect(() => sanitizeSkillName("a/b")).toThrow(); + expect(() => sanitizeSkillName("")).toThrow(); + expect(() => sanitizeSkillName("has space")).toThrow(); + }); + + it("normalizes and accepts a valid kebab name", () => { + expect(sanitizeSkillName(" Demo-Skill ")).toBe("demo-skill"); + }); + }); + + describe("toSkillFrontmatter", () => { + it("round-trips name and a description with a quote + newline through parseFrontmatter", () => { + const content = `${toSkillFrontmatter("demo", 'has a "quote"\nand newline')}\nbody`; + const { frontmatter } = parseFrontmatter(content, { source: "test" }); + expect(frontmatter.name).toBe("demo"); + expect(frontmatter.description).toBe('has a "quote" and newline'); + }); + }); + + describe("writeManagedSkill", () => { + it("creates a parseable SKILL.md and rejects a duplicate create", async () => { + await writeManagedSkill({ action: "create", name: "foo", description: "When to foo.", body: "# Foo\nbody" }); + const content = await Bun.file(skillFile("foo")).text(); + const { frontmatter, body } = parseFrontmatter(content, { source: "test" }); + expect(frontmatter.name).toBe("foo"); + expect(frontmatter.description).toBe("When to foo."); + expect(body).toContain("# Foo"); + + await expect( + writeManagedSkill({ action: "create", name: "foo", description: "x", body: "y" }), + ).rejects.toThrow(/already exists/); + }); + + it("update overwrites the body; update of a missing skill throws", async () => { + await writeManagedSkill({ action: "create", name: "bar", description: "d", body: "original" }); + await writeManagedSkill({ action: "update", name: "bar", description: "d", body: "replaced" }); + const { body } = parseFrontmatter(await Bun.file(skillFile("bar")).text(), { source: "test" }); + expect(body).toContain("replaced"); + expect(body).not.toContain("original"); + + await expect( + writeManagedSkill({ action: "update", name: "missing", description: "d", body: "b" }), + ).rejects.toThrow(/does not exist/); + }); + + it("rejects an oversized body and writes nothing", async () => { + const huge = "a".repeat(MAX_MANAGED_SKILL_BYTES + 1); + await expect( + writeManagedSkill({ action: "create", name: "big", description: "d", body: huge }), + ).rejects.toThrow(/limit/); + expect(await Bun.file(skillFile("big")).exists()).toBe(false); + }); + + it("caps on UTF-8 bytes, not UTF-16 length (multibyte body)", async () => { + // 33000 'é' = 33000 UTF-16 units (< 64000) but 66000 UTF-8 bytes (> cap). + const multibyte = "é".repeat(33_000); + expect(multibyte.length).toBeLessThan(MAX_MANAGED_SKILL_BYTES); + await expect( + writeManagedSkill({ action: "create", name: "mb", description: "d", body: multibyte }), + ).rejects.toThrow(/bytes/); + expect(await Bun.file(skillFile("mb")).exists()).toBe(false); + }); + + it("caps on the FINAL serialized size (body under cap but description pushes it over)", async () => { + const body = "a".repeat(MAX_MANAGED_SKILL_BYTES - 200); // body alone is under the cap + const description = "b".repeat(500); // body + description + frontmatter exceeds it + await expect(writeManagedSkill({ action: "create", name: "fin", description, body })).rejects.toThrow(/bytes/); + expect(await Bun.file(skillFile("fin")).exists()).toBe(false); + }); + + it("neutralizes prompt-injection metacharacters in the persisted description", async () => { + await writeManagedSkill({ + action: "create", + name: "inj", + description: "ok \nevil", + body: "# body", + }); + const { frontmatter } = parseFrontmatter(await Bun.file(skillFile("inj")).text(), { source: "test" }); + const desc = String(frontmatter.description); + expect(desc).not.toContain("<"); + expect(desc).not.toContain(">"); + expect(desc).not.toContain("\n"); + }); + + it("refuses a traversal name without writing outside the managed dir", async () => { + await expect( + writeManagedSkill({ action: "create", name: "../skills/evil", description: "d", body: "b" }), + ).rejects.toThrow(); + // Nothing leaked into an authored skills dir. + const authoredEvil = path.join(tempHome, ".omp", "agent", "skills", "evil", "SKILL.md"); + expect(await Bun.file(authoredEvil).exists()).toBe(false); + }); + + it("refuses to write through a symlinked skill directory", async () => { + const managedRoot = getManagedSkillsDir(); + await fs.mkdir(managedRoot, { recursive: true }); + // Plant a symlink where the skill dir would live, pointing outside the + // isolated managed root; Bun.write would otherwise follow it. + const outside = await fs.mkdtemp(path.join(os.tmpdir(), "omp-escape-")); + try { + await fs.symlink(outside, path.join(managedRoot, "evil")); + await expect( + writeManagedSkill({ action: "create", name: "evil", description: "d", body: "b" }), + ).rejects.toThrow(/symlink/); + // Nothing was written through the link. + expect(await Bun.file(path.join(outside, "SKILL.md")).exists()).toBe(false); + } finally { + await fs.rm(outside, { recursive: true, force: true }); + } + }); + + it("rejects an empty or whitespace-only description", async () => { + await expect( + writeManagedSkill({ action: "create", name: "blank", description: " ", body: "# body" }), + ).rejects.toThrow(/non-empty description/); + // Nothing written, so discovery never silently drops a "successful" skill. + expect(await Bun.file(skillFile("blank")).exists()).toBe(false); + }); + + it("rejects an empty or whitespace-only body", async () => { + await expect( + writeManagedSkill({ action: "create", name: "nobody", description: "d", body: " \n " }), + ).rejects.toThrow(/non-empty body/); + }); + + it("refuses to write when the managed-skills root itself is a symlink", async () => { + const realRoot = await fs.mkdtemp(path.join(os.tmpdir(), "omp-realroot-")); + try { + await fs.mkdir(path.dirname(getManagedSkillsDir()), { recursive: true }); + await fs.symlink(realRoot, getManagedSkillsDir()); + await expect( + writeManagedSkill({ action: "create", name: "demo", description: "d", body: "b" }), + ).rejects.toThrow(/managed-skills root is a symlink/); + expect(await Bun.file(path.join(realRoot, "demo", "SKILL.md")).exists()).toBe(false); + } finally { + await fs.rm(realRoot, { recursive: true, force: true }); + } + }); + + it("serializes a concurrent create+update of the same name in submission order", async () => { + const [createRes, updateRes] = await Promise.allSettled([ + writeManagedSkill({ action: "create", name: "seq", description: "d", body: "v1" }), + writeManagedSkill({ action: "update", name: "seq", description: "d", body: "v2" }), + ]); + // Without serialization the update could observe the file missing and throw. + expect(createRes.status).toBe("fulfilled"); + expect(updateRes.status).toBe("fulfilled"); + const { body } = parseFrontmatter(await Bun.file(skillFile("seq")).text(), { source: "test" }); + expect(body).toContain("v2"); + }); + + it("lets exactly one of two concurrent creates win", async () => { + const results = await Promise.allSettled([ + writeManagedSkill({ action: "create", name: "race", description: "d", body: "first" }), + writeManagedSkill({ action: "create", name: "race", description: "d", body: "second" }), + ]); + expect(results.filter(r => r.status === "fulfilled")).toHaveLength(1); + const rejected = results.filter(r => r.status === "rejected") as PromiseRejectedResult[]; + expect(rejected).toHaveLength(1); + expect(String(rejected[0]?.reason)).toMatch(/already exists/); + }); + + it("refuses to update a SKILL.md that is a symlink", async () => { + await writeManagedSkill({ action: "create", name: "linky", description: "d", body: "real" }); + const outside = await fs.mkdtemp(path.join(os.tmpdir(), "omp-link-")); + const target = path.join(outside, "target.md"); + await Bun.write(target, "outside content"); + try { + await fs.rm(skillFile("linky")); + await fs.symlink(target, skillFile("linky")); + await expect( + writeManagedSkill({ action: "update", name: "linky", description: "d", body: "hacked" }), + ).rejects.toThrow(/symlink/); + expect(await Bun.file(target).text()).toBe("outside content"); + } finally { + await fs.rm(outside, { recursive: true, force: true }); + } + }); + }); + + describe("deleteManagedSkill", () => { + it("removes an existing skill and throws for a missing one", async () => { + await writeManagedSkill({ action: "create", name: "gone", description: "d", body: "b" }); + await deleteManagedSkill("gone"); + expect(await Bun.file(skillFile("gone")).exists()).toBe(false); + + await expect(deleteManagedSkill("gone")).rejects.toThrow(/does not exist/); + }); + + it("refuses to delete through a symlinked skill directory", async () => { + const managedRoot = getManagedSkillsDir(); + await fs.mkdir(managedRoot, { recursive: true }); + const outside = await fs.mkdtemp(path.join(os.tmpdir(), "omp-deltarget-")); + await Bun.write(path.join(outside, "keep.txt"), "keep"); + try { + await fs.symlink(outside, path.join(managedRoot, "linked")); + await expect(deleteManagedSkill("linked")).rejects.toThrow(/symlink/); + // The symlink target's contents are untouched. + expect(await Bun.file(path.join(outside, "keep.txt")).exists()).toBe(true); + } finally { + await fs.rm(outside, { recursive: true, force: true }); + } + }); + }); +}); diff --git a/packages/coding-agent/test/autolearn-tools-gating.test.ts b/packages/coding-agent/test/autolearn-tools-gating.test.ts new file mode 100644 index 000000000..90a5641f6 --- /dev/null +++ b/packages/coding-agent/test/autolearn-tools-gating.test.ts @@ -0,0 +1,219 @@ +import { afterEach, beforeEach, describe, expect, it, spyOn } from "bun:test"; +import * as fs from "node:fs/promises"; +import * as os from "node:os"; +import * as path from "node:path"; +import { getManagedSkillsDir } from "@oh-my-pi/pi-coding-agent/autolearn/managed-skills"; +import { type SettingPath, Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; +import type { MnemopiSessionState } from "@oh-my-pi/pi-coding-agent/mnemopi/state"; +import { createTools, type ToolSession } from "@oh-my-pi/pi-coding-agent/tools"; +import { LearnTool } from "@oh-my-pi/pi-coding-agent/tools/learn"; +import { ManageSkillTool } from "@oh-my-pi/pi-coding-agent/tools/manage-skill"; + +function makeSession( + settingsOverrides: Partial> = {}, + extra: Partial = {}, +): ToolSession { + return { + cwd: "/tmp/test", + hasUI: false, + skipPythonPreflight: true, + getSessionFile: () => null, + getSessionSpawns: () => "*", + settings: Settings.isolated(settingsOverrides), + ...extra, + }; +} + +describe("autolearn tool gating", () => { + it("offers neither tool by default (autolearn disabled)", async () => { + const names = (await createTools(makeSession())).map(t => t.name); + expect(names).not.toContain("learn"); + expect(names).not.toContain("manage_skill"); + }); + + it("offers manage_skill but not learn when enabled with no memory backend", async () => { + const names = (await createTools(makeSession({ "autolearn.enabled": true, "memory.backend": "off" }))).map( + t => t.name, + ); + expect(names).toContain("manage_skill"); + expect(names).not.toContain("learn"); + }); + + it("offers both tools, marked essential, when enabled with a live backend", async () => { + const tools = await createTools(makeSession({ "autolearn.enabled": true, "memory.backend": "mnemopi" })); + const learn = tools.find(t => t.name === "learn"); + const manage = tools.find(t => t.name === "manage_skill"); + expect(learn).toBeDefined(); + expect(manage).toBeDefined(); + // loadMode "essential" is what keeps them active under tools.discoveryMode "all". + expect(learn?.loadMode).toBe("essential"); + expect(manage?.loadMode).toBe("essential"); + }); + + it("force-includes the tools into an explicit restricted toolNames list", async () => { + // A session created with autolearn on but a narrow tool list still gets the + // controller/guidance, so the tools the nudge points at must be present. + const withBackend = ( + await createTools(makeSession({ "autolearn.enabled": true, "memory.backend": "mnemopi" }), ["read"]) + ).map(t => t.name); + expect(withBackend).toContain("manage_skill"); + expect(withBackend).toContain("learn"); + + const noBackend = ( + await createTools(makeSession({ "autolearn.enabled": true, "memory.backend": "off" }), ["read"]) + ).map(t => t.name); + expect(noBackend).toContain("manage_skill"); + expect(noBackend).not.toContain("learn"); + }); + + it("excludes the tools from a subagent even with an explicit list", async () => { + // taskDepth > 0: the controller never runs here, so a subagent's explicit + // whitelist must not be silently widened with write-capable tools. + const sub = ( + await createTools(makeSession({ "autolearn.enabled": true, "memory.backend": "mnemopi" }, { taskDepth: 1 }), [ + "read", + ]) + ).map(t => t.name); + expect(sub).not.toContain("manage_skill"); + expect(sub).not.toContain("learn"); + + // Nor via discovery (no explicit list) at depth. + const subDiscovered = ( + await createTools(makeSession({ "autolearn.enabled": true, "memory.backend": "mnemopi" }, { taskDepth: 1 })) + ).map(t => t.name); + expect(subDiscovered).not.toContain("manage_skill"); + expect(subDiscovered).not.toContain("learn"); + }); + + it("offers learn with the file-based local backend", async () => { + const names = (await createTools(makeSession({ "autolearn.enabled": true, "memory.backend": "local" }))).map( + t => t.name, + ); + expect(names).toContain("learn"); + expect(names).toContain("manage_skill"); + + // Force-included into an explicit restricted toolNames list too. + const restricted = ( + await createTools(makeSession({ "autolearn.enabled": true, "memory.backend": "local" }), ["read"]) + ).map(t => t.name); + expect(restricted).toContain("learn"); + }); +}); + +describe("manage_skill execute", () => { + let tempHome: string; + + beforeEach(async () => { + tempHome = await fs.mkdtemp(path.join(os.tmpdir(), "omp-manage-skill-")); + spyOn(os, "homedir").mockReturnValue(tempHome); + }); + + afterEach(async () => { + spyOn(os, "homedir").mockRestore(); + await fs.rm(tempHome, { recursive: true, force: true }); + }); + + const tool = () => ManageSkillTool.createIf(makeSession({ "autolearn.enabled": true }))!; + + it("create writes the managed SKILL.md; delete removes it", async () => { + const file = path.join(getManagedSkillsDir(), "demo", "SKILL.md"); + await tool().execute("1", { action: "create", name: "demo", description: "When to demo.", body: "# Demo" }); + expect(await Bun.file(file).exists()).toBe(true); + + await tool().execute("2", { action: "delete", name: "demo" }); + expect(await Bun.file(file).exists()).toBe(false); + }); + + it("rejects create without a body and delete of a missing skill", async () => { + await expect(tool().execute("3", { action: "create", name: "nobody", description: "d" })).rejects.toThrow( + /requires/, + ); + await expect(tool().execute("4", { action: "delete", name: "absent" })).rejects.toThrow(/does not exist/); + }); + + it("schema rejects create/update without description+body but allows delete", () => { + const schema = tool().parameters; + expect(schema.safeParse({ action: "create", name: "x" }).success).toBe(false); + expect(schema.safeParse({ action: "update", name: "x", description: "d" }).success).toBe(false); + expect(schema.safeParse({ action: "create", name: "x", description: "d", body: "b" }).success).toBe(true); + expect(schema.safeParse({ action: "delete", name: "x" }).success).toBe(true); + }); +}); + +describe("learn execute", () => { + let tempHome: string; + let remembered: string[]; + + function learnSession(): ToolSession { + const fakeState = { + sessionId: "sess-1", + session: { sessionManager: { getCwd: () => "/tmp/work" } }, + rememberScoped: (memory: string) => { + remembered.push(memory); + return "mem-id"; + }, + }; + return makeSession( + { "autolearn.enabled": true, "memory.backend": "mnemopi" }, + { getMnemopiSessionState: () => fakeState as unknown as MnemopiSessionState }, + ); + } + + beforeEach(async () => { + tempHome = await fs.mkdtemp(path.join(os.tmpdir(), "omp-learn-")); + spyOn(os, "homedir").mockReturnValue(tempHome); + remembered = []; + }); + + afterEach(async () => { + spyOn(os, "homedir").mockRestore(); + await fs.rm(tempHome, { recursive: true, force: true }); + }); + + it("stores a lesson to memory without writing a skill when no skill payload", async () => { + await new LearnTool(learnSession()).execute("1", { memory: "Prefer Bun.file over readFileSync." }); + expect(remembered).toEqual(["Prefer Bun.file over readFileSync."]); + // No managed skills written. + expect(await fs.readdir(getManagedSkillsDir()).catch(() => [])).toHaveLength(0); + }); + + it("stores a lesson AND mints a managed skill when a skill payload is given", async () => { + await new LearnTool(learnSession()).execute("2", { + memory: "Use the worker host entry pattern.", + skill: { action: "create", name: "worker-host", description: "Spawn workers.", body: "# Worker host" }, + }); + expect(remembered).toHaveLength(1); + expect(await Bun.file(path.join(getManagedSkillsDir(), "worker-host", "SKILL.md")).exists()).toBe(true); + }); + + it("surfaces a partial-outcome error when the skill name is invalid", async () => { + await expect( + new LearnTool(learnSession()).execute("3", { + memory: "lesson", + skill: { action: "create", name: "../evil", description: "d", body: "b" }, + }), + ).rejects.toThrow(/Lesson stored, but the managed skill could not be written/); + // The memory half still ran. + expect(remembered).toHaveLength(1); + }); + + it("fails the lesson and skips the skill when mnemopi returns no id", async () => { + const failingState = { + sessionId: "sess-2", + session: { sessionManager: { getCwd: () => "/tmp/work" } }, + rememberScoped: () => undefined, + }; + const session = makeSession( + { "autolearn.enabled": true, "memory.backend": "mnemopi" }, + { getMnemopiSessionState: () => failingState as unknown as MnemopiSessionState }, + ); + await expect( + new LearnTool(session).execute("5", { + memory: "lesson", + skill: { action: "create", name: "should-not-exist", description: "d", body: "b" }, + }), + ).rejects.toThrow(/did not store/i); + // A failed lesson must not leave a minted skill behind. + expect(await Bun.file(path.join(getManagedSkillsDir(), "should-not-exist", "SKILL.md")).exists()).toBe(false); + }); +}); diff --git a/packages/coding-agent/test/memory-backend-resolve.test.ts b/packages/coding-agent/test/memory-backend-resolve.test.ts index a46fe005c..576fd288d 100644 --- a/packages/coding-agent/test/memory-backend-resolve.test.ts +++ b/packages/coding-agent/test/memory-backend-resolve.test.ts @@ -29,7 +29,7 @@ describe("resolveMemoryBackend", () => { }); }); - it("reports local backend runtime status without structured search/save support", async () => { + it("reports local backend runtime status as writable (lessons) without structured search", async () => { const settings = Settings.isolated({ "memory.backend": "local" }); const memory = createMemoryRuntimeContext({ agentDir: "/tmp/agent", @@ -40,7 +40,7 @@ describe("resolveMemoryBackend", () => { await expect(memory.status()).resolves.toMatchObject({ backend: "local", active: true, - writable: false, + writable: true, searchable: false, }); await expect(memory.search("project preference")).resolves.toMatchObject({ diff --git a/packages/coding-agent/test/sdk-autolearn-active-tools.test.ts b/packages/coding-agent/test/sdk-autolearn-active-tools.test.ts new file mode 100644 index 000000000..298b2a505 --- /dev/null +++ b/packages/coding-agent/test/sdk-autolearn-active-tools.test.ts @@ -0,0 +1,68 @@ +import { afterAll, beforeAll, describe, expect, it } from "bun:test"; +import * as fs from "node:fs"; +import * as os from "node:os"; +import * as path from "node:path"; +import { AuthStorage } from "@oh-my-pi/pi-ai"; +import { getBundledModel } from "@oh-my-pi/pi-catalog/models"; +import { ModelRegistry } from "@oh-my-pi/pi-coding-agent/config/model-registry"; +import { Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; +import { createAgentSession } from "@oh-my-pi/pi-coding-agent/sdk"; +import type { AgentSession } from "@oh-my-pi/pi-coding-agent/session/agent-session"; +import { SessionManager } from "@oh-my-pi/pi-coding-agent/session/session-manager"; +import { Snowflake } from "@oh-my-pi/pi-utils"; + +// Guards the auto-learn tool ACTIVATION wiring in createAgentSession: createTools +// force-includes manage_skill into the built registry for an enabled top-level +// session, but an explicit `toolNames` whitelist would otherwise drop it from the +// active set — so the SDK must re-activate it (mirroring the `yield` invariant), +// or the nudge/guidance would point at a tool the model cannot call. No memory +// backend is configured (manage_skill needs only `autolearn.enabled`), so the +// session starts without a heavy backend. +describe("createAgentSession auto-learn tool activation", () => { + let registryDir: string; + let authStorage: AuthStorage; + let modelRegistry: ModelRegistry; + const sessions: AgentSession[] = []; + + beforeAll(async () => { + registryDir = path.join(os.tmpdir(), `pi-autolearn-active-${Snowflake.next()}`); + fs.mkdirSync(registryDir, { recursive: true }); + authStorage = await AuthStorage.create(path.join(registryDir, "auth.db")); + modelRegistry = new ModelRegistry(authStorage); + }); + + afterAll(async () => { + for (const session of sessions) await session.dispose().catch(() => {}); + authStorage.close(); + if (fs.existsSync(registryDir)) fs.rmSync(registryDir, { recursive: true, force: true }); + }); + + async function activeToolNames(settings: Settings): Promise { + const { session } = await createAgentSession({ + cwd: registryDir, + agentDir: registryDir, + modelRegistry, + sessionManager: SessionManager.inMemory(), + settings, + model: getBundledModel("openai", "gpt-4o-mini"), + disableExtensionDiscovery: true, + toolNames: ["read"], + }); + sessions.push(session); + return session.getActiveToolNames(); + } + + it("activates force-included manage_skill in a restricted top-level session", async () => { + const names = await activeToolNames(Settings.isolated({ "autolearn.enabled": true })); + expect(names).toContain("read"); + // Built by createTools' force-include AND activated by the SDK's explicit-list + // re-inclusion, so guidance/controller point at a callable tool. + expect(names).toContain("manage_skill"); + }); + + it("omits manage_skill from a restricted session when auto-learn is off", async () => { + const names = await activeToolNames(Settings.isolated({})); + expect(names).toContain("read"); + expect(names).not.toContain("manage_skill"); + }); +}); diff --git a/packages/coding-agent/test/tool-discovery/initial-tools.test.ts b/packages/coding-agent/test/tool-discovery/initial-tools.test.ts index 635dcec20..d174338d1 100644 --- a/packages/coding-agent/test/tool-discovery/initial-tools.test.ts +++ b/packages/coding-agent/test/tool-discovery/initial-tools.test.ts @@ -28,6 +28,7 @@ const allToolsSettings = Settings.isolated({ "checkpoint.enabled": true, "todo.enabled": true, "memory.backend": "mnemopi", + "autolearn.enabled": true, "tools.discoveryMode": "all", });