From a5f1ba6bd3e83e6a7eb10cae940f63076b4e7a8e Mon Sep 17 00:00:00 2001 From: Nivesh353 Date: Wed, 5 Aug 2026 18:12:35 +0530 Subject: [PATCH 1/4] Scaffold gitagent agent --- src/index.ts | 10 ++- src/learning/llm-call.ts | 91 ++++++++++++++++++++++++++ src/learning/reflection.ts | 72 +++++++++++++++++++++ src/learning/skill-repair.ts | 76 ++++++++++++++++++++++ src/sdk.ts | 6 ++ src/tools/index.ts | 10 ++- src/tools/shared.ts | 2 +- src/tools/skill-learner.ts | 120 +++++++++++++++++++++++++++++++++-- src/tools/task-tracker.ts | 39 ++++++++++-- 9 files changed, 413 insertions(+), 13 deletions(-) create mode 100644 src/learning/llm-call.ts create mode 100644 src/learning/reflection.ts create mode 100644 src/learning/skill-repair.ts diff --git a/src/index.ts b/src/index.ts index ba4eeb2..6ba2efa 100644 --- a/src/index.ts +++ b/src/index.ts @@ -530,6 +530,11 @@ async function main(): Promise { // Collect plugin memory layers const pluginMemoryLayers = loaded.plugins.flatMap((p) => p.memoryLayers); + // Hoisted above tool creation (was declared later, alongside the agent's own + // message_end handler) so task_tracker's reflection LLM call can add to the + // same running total via onUsage below. + let _totalCostUsd = 0; + // Build tools — built-in + declarative let tools: AgentTool[] = createBuiltinTools({ dir, @@ -537,6 +542,10 @@ async function main(): Promise { sandbox: sandboxCtx, gitagentDir, pluginMemoryLayers: pluginMemoryLayers.length > 0 ? pluginMemoryLayers : undefined, + model: loaded.model, + onUsage: (msg) => { + if (msg.usage) _totalCostUsd += msg.usage.costUsd ?? 0; + }, }); // Load declarative tools from tools/*.yaml (Phase 2.2) @@ -589,7 +598,6 @@ async function main(): Promise { "gitagent.entry": "cli", }); let _llmCallStart = 0; - let _totalCostUsd = 0; const agent = new Agent({ initialState: { diff --git a/src/learning/llm-call.ts b/src/learning/llm-call.ts new file mode 100644 index 0000000..70d8007 --- /dev/null +++ b/src/learning/llm-call.ts @@ -0,0 +1,91 @@ +import { Agent } from "@mariozechner/pi-agent-core"; +import type { Model } from "@mariozechner/pi-ai"; +import { recordGenAiCall } from "../telemetry.js"; +import type { GCAssistantMessage } from "../sdk-types.js"; + +export interface OneOffCompletionOpts { + temperature?: number; + maxTokens?: number; + timeoutMs?: number; +} + +/** + * Runs a single bare, tool-less completion outside the main agent loop + * (used for Reflexion-style reflection and Voyager-style skill repair). + * Forwards usage via `onUsage`, since these calls happen outside the main + * loop's own message_end handler and would otherwise be invisible to the + * caller's cost tracking. + * + * Throws on error, empty output, or timeout — callers decide whether to + * fail-soft or propagate. + */ +export async function runOneOffCompletion( + model: Model, + systemPrompt: string, + userPrompt: string, + opts: OneOffCompletionOpts = {}, + onUsage?: (msg: GCAssistantMessage) => void, +): Promise { + const { temperature = 0, maxTokens = 300, timeoutMs = 15_000 } = opts; + + const agent = new Agent({ + initialState: { + systemPrompt, + model, + tools: [], + temperature, + maxTokens, + } as any, + }); + + let collected = ""; + let failed: string | undefined; + const startedAt = Date.now(); + agent.subscribe((event: any) => { + if (event.type === "message_end" && event.message?.role === "assistant") { + const msg = event.message; + for (const block of msg.content) { + if (block.type === "text") collected += block.text; + } + if (msg.stopReason === "error") failed = msg.errorMessage || "LLM call failed"; + recordGenAiCall(msg, { durationMs: Date.now() - startedAt }); + + if (onUsage && msg.usage) { + onUsage({ + type: "assistant", + content: collected, + model: msg.model ?? "unknown", + provider: msg.provider ?? "unknown", + stopReason: msg.stopReason ?? "stop", + errorMessage: msg.errorMessage, + usage: { + inputTokens: msg.usage.input ?? 0, + outputTokens: msg.usage.output ?? 0, + cacheReadTokens: msg.usage.cacheRead ?? 0, + cacheWriteTokens: msg.usage.cacheWrite ?? 0, + totalTokens: msg.usage.totalTokens ?? 0, + costUsd: msg.usage.cost?.total ?? 0, + }, + }); + } + } + }); + + const timer = setTimeout(() => { + try { + agent.abort(); + } catch { + /* ignore */ + } + }, timeoutMs); + try { + await agent.prompt(userPrompt); + } finally { + clearTimeout(timer); + } + + if (failed) throw new Error(failed); + const text = collected.trim(); + if (!text) throw new Error("LLM call produced empty output"); + return text; +} diff --git a/src/learning/reflection.ts b/src/learning/reflection.ts new file mode 100644 index 0000000..5e501eb --- /dev/null +++ b/src/learning/reflection.ts @@ -0,0 +1,72 @@ +import type { Model } from "@mariozechner/pi-ai"; +import type { GCAssistantMessage } from "../sdk-types.js"; +import { runOneOffCompletion } from "./llm-call.js"; + +// ── Types ─────────────────────────────────────────────────────────────── + +export interface ReflectionInput { + objective: string; + /** Ordered step descriptions actually recorded for the failed attempt. */ + steps: string[]; + /** The model's own raw one-line failure report, if it gave one. */ + failureReason?: string; +} + +const REFLECTION_TIMEOUT_MS = 15_000; +// Hard cap regardless of model verbosity — protects the 10-slot +// negative_examples array from unbounded growth. +const MAX_REFLECTION_CHARS = 500; + +const SYSTEM_PROMPT = `You are a terse failure-analysis assistant for an autonomous coding agent. +You will be given a task objective, the ordered steps the agent actually +took, and how the agent itself described the failure. + +Write EXACTLY ONE plain-text paragraph, 2-4 sentences, under 400 characters, +with no markdown, no headers, no bullet points, no line breaks, and no +preamble ("Root cause:", "Here is my analysis", etc.). The paragraph must: +1. State the most likely root cause, grounded in the specific steps shown + (not generic advice like "check for errors"). +2. State one concrete, different strategy to try on the next attempt. + +Do not restate the objective. Output only the paragraph.`; + +function buildUserPrompt(input: ReflectionInput): string { + const stepsText = input.steps.length + ? input.steps.map((s, i) => `${i + 1}. ${s}`).join("\n") + : "(no steps were recorded)"; + return ( + `Objective: ${input.objective}\n\n` + + `Steps taken (in order):\n${stepsText}\n\n` + + `Reported outcome: failure — "${input.failureReason || "not specified"}"\n\n` + + `Write the reflection now.` + ); +} + +/** + * Reflexion-style verbal reflection on a failed skill-using attempt. Grounds + * the reflection in the attempt's actual recorded steps, not just the + * model's own one-line failure report, so retries get an analyzed lesson + * instead of a rephrased complaint. + * + * Fails soft: any error (timeout, empty output, model unavailable) throws, + * and the caller is expected to fall back to the raw failure reason. This + * must never be the reason a task_tracker "end" call fails. + */ +export async function reflectOnFailure( + model: Model, + input: ReflectionInput, + onUsage?: (msg: GCAssistantMessage) => void, +): Promise { + const text = await runOneOffCompletion( + model, + SYSTEM_PROMPT, + buildUserPrompt(input), + { temperature: 0, maxTokens: 300, timeoutMs: REFLECTION_TIMEOUT_MS }, + onUsage, + ); + + if (text.length > MAX_REFLECTION_CHARS) { + return text.slice(0, MAX_REFLECTION_CHARS - 1).trimEnd() + "…"; + } + return text; +} diff --git a/src/learning/skill-repair.ts b/src/learning/skill-repair.ts new file mode 100644 index 0000000..a8b738f --- /dev/null +++ b/src/learning/skill-repair.ts @@ -0,0 +1,76 @@ +import type { Model } from "@mariozechner/pi-ai"; +import type { GCAssistantMessage } from "../sdk-types.js"; +import { runOneOffCompletion } from "./llm-call.js"; + +// ── Types ─────────────────────────────────────────────────────────────── + +export interface SkillRepairInput { + skillDescription: string; + /** Current numbered (or otherwise formatted) steps section of the skill. */ + currentSteps: string; + /** Accumulated failure lessons that got this skill flagged. */ + negativeExamples: string[]; +} + +const REPAIR_TIMEOUT_MS = 20_000; +// Hard cap regardless of model verbosity — a repaired skill's steps section +// shouldn't be allowed to balloon indefinitely. +const MAX_REPAIR_CHARS = 3000; + +const SYSTEM_PROMPT = `You are a skill-repair assistant for an autonomous coding agent's skill library. +You will be given a skill's description, its current steps, and the concrete +lessons learned from real failures while following those steps. + +Rewrite the steps so they avoid every named failure mode while still +achieving the skill's description. Keep the steps generalizable — do not +hard-code project-specific paths, names, or values that only applied to one +past failure. + +Output ONLY a bare numbered list of steps. No headers, no preamble, no +commentary, no markdown code fences.`; + +function buildUserPrompt(input: SkillRepairInput): string { + const lessonsText = input.negativeExamples.length + ? input.negativeExamples.map((n, i) => `${i + 1}. ${n}`).join("\n") + : "(no specific lessons recorded)"; + return ( + `Skill description: ${input.skillDescription}\n\n` + + `Current steps:\n${input.currentSteps}\n\n` + + `Lessons learned from real failures:\n${lessonsText}\n\n` + + `Write the repaired steps now.` + ); +} + +function stripCodeFence(text: string): string { + const fenced = text.match(/^```[a-z]*\r?\n([\s\S]*?)\r?\n```$/); + return fenced ? fenced[1].trim() : text; +} + +/** + * Voyager-style skill repair: rewrites a flagged skill's steps using its own + * accumulated Reflexion-style lessons, so a broken skill can be repaired by + * the agent itself instead of requiring a human to rewrite it by hand. + * + * Unlike reflection (an invisible side-effect of task_tracker "end"), repair + * is an explicit action the model chooses to call — errors here propagate + * as normal tool errors rather than failing soft. + */ +export async function repairSkillSteps( + model: Model, + input: SkillRepairInput, + onUsage?: (msg: GCAssistantMessage) => void, +): Promise { + const text = await runOneOffCompletion( + model, + SYSTEM_PROMPT, + buildUserPrompt(input), + { temperature: 0, maxTokens: 600, timeoutMs: REPAIR_TIMEOUT_MS }, + onUsage, + ); + + const cleaned = stripCodeFence(text.trim()); + if (cleaned.length > MAX_REPAIR_CHARS) { + return cleaned.slice(0, MAX_REPAIR_CHARS - 1).trimEnd() + "…"; + } + return cleaned; +} diff --git a/src/sdk.ts b/src/sdk.ts index 55b941c..10a4b18 100644 --- a/src/sdk.ts +++ b/src/sdk.ts @@ -185,6 +185,12 @@ export function query(options: QueryOptions): Query { sandbox: sandboxCtx, gitagentDir: loaded.gitagentDir, pluginMemoryLayers: pluginMemoryLayers.length > 0 ? pluginMemoryLayers : undefined, + model: loaded.model, + onUsage: (msg) => { + if (!msg.usage) return; + costTracker.add(`${msg.provider}:${msg.model}`, msg.usage); + _totalCostUsd += msg.usage.costUsd ?? 0; + }, }); } diff --git a/src/tools/index.ts b/src/tools/index.ts index 6e4f1a4..c2f0b43 100644 --- a/src/tools/index.ts +++ b/src/tools/index.ts @@ -1,6 +1,8 @@ import type { AgentTool } from "@mariozechner/pi-agent-core"; +import type { Model } from "@mariozechner/pi-ai"; import type { SandboxContext } from "../sandbox.js"; import type { MemoryLayerDef } from "../plugin-types.js"; +import type { GCAssistantMessage } from "../sdk-types.js"; import { createCliTool } from "./cli.js"; import { createReadTool } from "./read.js"; import { createWriteTool } from "./write.js"; @@ -21,6 +23,10 @@ export interface BuiltinToolsConfig { sandbox?: SandboxContext; gitagentDir?: string; pluginMemoryLayers?: MemoryLayerDef[]; + /** Resolved model, used by task_tracker for Reflexion-style failure reflection. */ + model?: Model; + /** Called with the reflection LLM call's usage, so callers can feed it into their own cost tracking. */ + onUsage?: (msg: GCAssistantMessage) => void; } /** @@ -50,8 +56,8 @@ export function createBuiltinTools(config: BuiltinToolsConfig): AgentTool[] // Add learning tools if gitagentDir is available if (config.gitagentDir) { - tools.push(createTaskTrackerTool(config.dir, config.gitagentDir)); - tools.push(createSkillLearnerTool(config.dir, config.gitagentDir)); + tools.push(createTaskTrackerTool(config.dir, config.gitagentDir, config.model, config.onUsage)); + tools.push(createSkillLearnerTool(config.dir, config.gitagentDir, config.model, config.onUsage)); } return tools; diff --git a/src/tools/shared.ts b/src/tools/shared.ts index abf3240..c2e1788 100644 --- a/src/tools/shared.ts +++ b/src/tools/shared.ts @@ -58,7 +58,7 @@ export const capturePhotoSchema = Type.Object({ }); export const skillLearnerSchema = Type.Object({ - action: Type.Union([Type.Literal("evaluate"), Type.Literal("crystallize"), Type.Literal("status"), Type.Literal("review"), Type.Literal("update"), Type.Literal("delete")], { description: "Action to perform" }), + action: Type.Union([Type.Literal("evaluate"), Type.Literal("crystallize"), Type.Literal("status"), Type.Literal("review"), Type.Literal("repair"), Type.Literal("update"), Type.Literal("delete")], { description: "Action to perform" }), task_id: Type.Optional(Type.String({ description: "Task ID (for evaluate/crystallize)" })), skill_name: Type.Optional(Type.String({ description: "Skill name (for crystallize/update/delete)" })), skill_description: Type.Optional(Type.String({ description: "Skill description (for crystallize)" })), diff --git a/src/tools/skill-learner.ts b/src/tools/skill-learner.ts index 91194e3..57f84e5 100644 --- a/src/tools/skill-learner.ts +++ b/src/tools/skill-learner.ts @@ -3,11 +3,21 @@ import { join } from "path"; import { execSync } from "child_process"; import { type Static } from "@sinclair/typebox"; import type { AgentTool } from "@mariozechner/pi-agent-core"; +import type { Model } from "@mariozechner/pi-ai"; +import type { GCAssistantMessage } from "../sdk-types.js"; import { skillLearnerSchema } from "./shared.js"; import { loadSkillStats, isSkillFlagged } from "../learning/reinforcement.js"; +import { repairSkillSteps } from "../learning/skill-repair.js"; import type { TaskRecord } from "./task-tracker.js"; import yaml from "js-yaml"; +// Caps how many times a single skill can be auto-repaired before it must +// go to a human (via "update" or "delete") instead. +const MAX_REPAIRS = 3; +// Clears the <0.4 flag immediately but stays below what a genuinely-proven +// skill earns — a repaired skill has to re-earn trust, not start clean. +const REPAIR_RESET_CONFIDENCE = 0.6; + // ── Helpers ───────────────────────────────────────────────────────────── interface TasksStore { @@ -83,6 +93,18 @@ async function getExistingSkillDescriptions(agentDir: string): Promise { + try { + const content = await readFile(join(skillDir, "SKILL.md"), "utf-8"); + const fmMatch = content.match(/^---\r?\n([\s\S]*?)\r?\n---/); + if (!fmMatch) return 0; + const fm = yaml.load(fmMatch[1]) as Record; + return typeof fm.repair_count === "number" ? fm.repair_count : 0; + } catch { + return 0; + } +} + function gitCommit(agentDir: string, files: string[], message: string): void { try { for (const f of files) { @@ -99,12 +121,17 @@ function gitCommit(agentDir: string, files: string[], message: string): void { // ── Tool factory ──────────────────────────────────────────────────────── -export function createSkillLearnerTool(agentDir: string, gitagentDir: string): AgentTool { +export function createSkillLearnerTool( + agentDir: string, + gitagentDir: string, + model?: Model, + onUsage?: (msg: GCAssistantMessage) => void, +): AgentTool { return { name: "skill_learner", label: "skill_learner", description: - "Learn from successful tasks. Use 'evaluate' to check if a completed task is worth saving as a skill, 'crystallize' to save it, 'status' to list all skills with confidence scores, 'review' to see flagged low-confidence skills, 'update' to modify a skill, 'delete' to remove one.", + "Learn from successful tasks. Use 'evaluate' to check if a completed task is worth saving as a skill, 'crystallize' to save it, 'status' to list all skills with confidence scores, 'review' to see flagged low-confidence skills, 'repair' to have the agent rewrite a flagged skill's steps using its own accumulated failure lessons, 'update' to modify a skill, 'delete' to remove one.", parameters: skillLearnerSchema, execute: async ( _toolCallId: string, @@ -268,18 +295,20 @@ export function createSkillLearnerTool(agentDir: string, gitagentDir: string): A }; } - const skills: Array<{ name: string; confidence: number; usage: number; ratio: string }> = []; + const skills: Array<{ name: string; confidence: number; usage: number; ratio: string; repairs: number }> = []; for (const entry of entries) { if (!entry.isDirectory()) continue; const dir = join(skillsDir, entry.name); const stats = await loadSkillStats(dir); + const repairs = await loadRepairCount(dir); // Only include learned skills (those with stats fields) skills.push({ name: entry.name, confidence: stats.confidence, usage: stats.usage_count, ratio: `${stats.success_count}/${stats.success_count + stats.failure_count}`, + repairs, }); } @@ -291,7 +320,8 @@ export function createSkillLearnerTool(agentDir: string, gitagentDir: string): A } const lines = skills.map((s) => - ` ${s.name}: confidence=${s.confidence}, usage=${s.usage}, success_ratio=${s.ratio}`, + ` ${s.name}: confidence=${s.confidence}, usage=${s.usage}, success_ratio=${s.ratio}` + + (s.repairs > 0 ? `, repairs=${s.repairs}/${MAX_REPAIRS}` : ""), ); return { content: [{ type: "text", text: `Skills:\n${lines.join("\n")}` }], @@ -342,11 +372,91 @@ export function createSkillLearnerTool(agentDir: string, gitagentDir: string): A }); return { - content: [{ type: "text", text: `Flagged skills (confidence < 0.4):\n${lines.join("\n")}\n\nConsider updating or deleting these skills.` }], + content: [{ + type: "text", + text: `Flagged skills (confidence < 0.4):\n${lines.join("\n")}\n\nCall skill_learner action "repair" with a skill_name to let the agent rewrite its steps using these lessons (up to ${MAX_REPAIRS} times per skill). If a skill has already been repaired ${MAX_REPAIRS} times, use "update" or "delete" instead.`, + }], details: { flagged }, }; } + case "repair": { + if (!params.skill_name) throw new Error("skill_name is required for repair action"); + if (!model) throw new Error("Repair requires a model to be configured for this agent."); + + const skillFile = join(agentDir, "skills", params.skill_name, "SKILL.md"); + let content: string; + try { + content = await readFile(skillFile, "utf-8"); + } catch { + throw new Error(`Skill not found: ${params.skill_name}`); + } + + const fmMatch = content.match(/^---\r?\n([\s\S]*?)\r?\n---\r?\n?([\s\S]*)$/); + if (!fmMatch) throw new Error("Invalid SKILL.md format"); + const frontmatter = yaml.load(fmMatch[1]) as Record; + const body = fmMatch[2]; + + const skillDir = join(agentDir, "skills", params.skill_name); + const stats = await loadSkillStats(skillDir); + if (!isSkillFlagged(stats)) { + throw new Error( + `Skill "${params.skill_name}" is not flagged (confidence ${stats.confidence} >= 0.4). Repair is only for flagged skills.`, + ); + } + + const repairCount = typeof frontmatter.repair_count === "number" ? frontmatter.repair_count : 0; + if (repairCount >= MAX_REPAIRS) { + throw new Error( + `Skill "${params.skill_name}" has already been repaired ${repairCount}/${MAX_REPAIRS} times. Use "update" or "delete" instead.`, + ); + } + + const stepsMatch = body.match(/## Steps\r?\n([\s\S]*?)(?=\r?\n## |\s*$)/); + const currentSteps = stepsMatch ? stepsMatch[1].trim() : body.trim(); + + const repairedSteps = await repairSkillSteps( + model, + { + skillDescription: frontmatter.description || "", + currentSteps, + negativeExamples: stats.negative_examples, + }, + onUsage, + ); + + const historyMatch = body.match(/## Repair History\r?\n([\s\S]*?)(?=\r?\n## |\s*$)/); + const existingHistory = historyMatch ? historyMatch[1].trim() : ""; + const attempt = repairCount + 1; + const lessonLines = stats.negative_examples.length + ? stats.negative_examples.map((n) => ` - ${n}`).join("\n") + : " - (no specific lessons recorded)"; + const newEntry = `- Repair #${attempt} on ${new Date().toISOString()}:\n${lessonLines}`; + const historyBody = existingHistory ? `${existingHistory}\n${newEntry}` : newEntry; + + frontmatter.confidence = REPAIR_RESET_CONFIDENCE; + frontmatter.usage_count = 0; + frontmatter.success_count = 0; + frontmatter.failure_count = 0; + frontmatter.negative_examples = []; + frontmatter.repair_count = attempt; + + const newBody = `\n## Steps\n${repairedSteps}\n\n## Repair History\n${historyBody}\n`; + const yamlStr = yaml.dump(frontmatter, { lineWidth: -1, noRefs: true }).trimEnd(); + const updated = `---\n${yamlStr}\n---\n${newBody}`; + + await writeFile(skillFile, updated, "utf-8"); + gitCommit(agentDir, [`skills/${params.skill_name}/SKILL.md`], `Repair skill: ${params.skill_name}`); + + return { + content: [{ + type: "text", + text: `Skill "${params.skill_name}" repaired (attempt ${attempt}/${MAX_REPAIRS}) and committed.\nConfidence reset to ${REPAIR_RESET_CONFIDENCE} (was ${stats.confidence}).\nSteps rewritten based on ${stats.negative_examples.length} recorded failure(s).`, + }], + details: { skill_name: params.skill_name, repair_count: attempt, confidence: REPAIR_RESET_CONFIDENCE }, + }; + } + case "update": { if (!params.skill_name) throw new Error("skill_name is required for update action"); if (!params.instructions) throw new Error("instructions is required for update action"); diff --git a/src/tools/task-tracker.ts b/src/tools/task-tracker.ts index 71778e1..beb638b 100644 --- a/src/tools/task-tracker.ts +++ b/src/tools/task-tracker.ts @@ -3,8 +3,11 @@ import { join } from "path"; import { randomUUID } from "crypto"; import { type Static } from "@sinclair/typebox"; import type { AgentTool } from "@mariozechner/pi-agent-core"; +import type { Model } from "@mariozechner/pi-ai"; +import type { GCAssistantMessage } from "../sdk-types.js"; import { taskTrackerSchema } from "./shared.js"; import { adjustConfidence, loadSkillStats, saveSkillStats } from "../learning/reinforcement.js"; +import { reflectOnFailure } from "../learning/reflection.js"; import yaml from "js-yaml"; // ── Types ─────────────────────────────────────────────────────────────── @@ -149,7 +152,12 @@ async function searchSkillsMP(objective: string): Promise { // ── Tool factory ──────────────────────────────────────────────────────── -export function createTaskTrackerTool(agentDir: string, gitagentDir: string): AgentTool { +export function createTaskTrackerTool( + agentDir: string, + gitagentDir: string, + model?: Model, + onUsage?: (msg: GCAssistantMessage) => void, +): AgentTool { return { name: "task_tracker", label: "task_tracker", @@ -277,10 +285,33 @@ export function createTaskTrackerTool(agentDir: string, gitagentDir: string): Ag if (task.status !== "active") throw new Error(`Task ${params.task_id} is not active (status: ${task.status})`); const outcome = params.outcome as "success" | "failure" | "partial"; + + // Reflexion-style reflection: on any non-success outcome, replace the + // model's own one-line failure report with a grounded root-cause + + // next-strategy reflection generated from the task's actual recorded + // steps. Fails soft — any error here keeps the raw string exactly as + // before this reflection step existed. + let effectiveFailureReason = params.failure_reason; + if (outcome !== "success" && model) { + try { + effectiveFailureReason = await reflectOnFailure( + model, + { + objective: task.objective, + steps: task.steps.map((s) => s.description), + failureReason: params.failure_reason, + }, + onUsage, + ); + } catch { + // fail-soft — keep the raw one-liner + } + } + task.outcome = outcome; task.status = outcome === "success" ? "succeeded" : "failed"; task.ended_at = new Date().toISOString(); - task.failure_reason = params.failure_reason; + task.failure_reason = effectiveFailureReason; task.skill_used = params.skill_used; // Trigger reinforcement if a skill was used @@ -289,7 +320,7 @@ export function createTaskTrackerTool(agentDir: string, gitagentDir: string): Ag const skillDir = join(agentDir, "skills", params.skill_used); try { const stats = await loadSkillStats(skillDir); - const updated = adjustConfidence(stats, outcome, params.failure_reason); + const updated = adjustConfidence(stats, outcome, effectiveFailureReason); await saveSkillStats(skillDir, updated); reinforcementMsg = `\nSkill "${params.skill_used}" confidence: ${stats.confidence} → ${updated.confidence}`; } catch { @@ -312,7 +343,7 @@ export function createTaskTrackerTool(agentDir: string, gitagentDir: string): Ag return { content: [{ type: "text", - text: `Task ${task.id} ${outcome}. Reason: ${params.failure_reason || "not specified"}.${reinforcementMsg}\n\nConsider a different approach. Call task_tracker action "begin" with the same objective to retry.`, + text: `Task ${task.id} ${outcome}. Reason: ${effectiveFailureReason || "not specified"}.${reinforcementMsg}\n\nConsider a different approach. Call task_tracker action "begin" with the same objective to retry.`, }], details: { task_id: task.id }, }; From 396f3f3dc6cfa047fe29b66ece292a65e273bd18 Mon Sep 17 00:00:00 2001 From: Nivesh353 Date: Thu, 6 Aug 2026 13:55:01 +0530 Subject: [PATCH 2/4] Scaffold gitagent agent --- Documentation.md | 77 +++++ examples/skill-approval.ts | 149 ++++++++++ src/elicit.ts | 165 +++++++++++ src/index.ts | 21 +- src/sdk-types.ts | 7 + src/sdk.ts | 1 + src/text-diff.ts | 85 ++++++ src/tools/index.ts | 17 +- src/tools/skill-learner.ts | 119 +++++++- src/tools/task-tracker.ts | 95 ++++++- test/elicit.test.ts | 166 +++++++++++ test/learning.live.test.ts | 241 ++++++++++++++++ test/learning.test.ts | 557 +++++++++++++++++++++++++++++++++++++ 13 files changed, 1683 insertions(+), 17 deletions(-) create mode 100644 examples/skill-approval.ts create mode 100644 src/elicit.ts create mode 100644 src/text-diff.ts create mode 100644 test/elicit.test.ts create mode 100644 test/learning.live.test.ts create mode 100644 test/learning.test.ts diff --git a/Documentation.md b/Documentation.md index c795966..4f5e94a 100644 --- a/Documentation.md +++ b/Documentation.md @@ -552,6 +552,83 @@ The agent can learn new skills automatically: 5. Future tasks search for matching skills 6. Confidence adjusts based on success/failure outcomes +### Human-in-the-loop Skill Approval + +Two skill decisions belong to the user, not the model. When stdin is a TTY, the +agent pauses mid-tool-call and asks. + +**1. A matching skill is flagged unreliable** (confidence < 0.4), raised by +`task_tracker` action `begin`: + +``` +⚠️ Skill "laptop-setup-checklist" matches this task but is flagged as unreliable + laptop-setup-checklist — Generate a new laptop setup checklist… + confidence 0.3 · 2 success / 5 failure + recent failures: + - Saved the checklist outside the workspace directory + - Assumed macOS and emitted Windows-invalid steps + + [p] proceed — use the skill as-is (default) + [r] repair — rewrite its steps first (you review the change) + [s] skip — ignore the skill, solve from scratch +→ choose [p/r/s]: +``` + +The choice is handed to the model as a `USER DECISION:` instruction, so it can't +quietly repair a skill you told it to use, or use one you told it to skip. + +**2. A repair is proposed**, raised by `skill_learner` action `repair`. The +rewritten steps are generated but *nothing is written or committed* until you +accept: + +``` +Proposed repair for skill "laptop-setup-checklist" (attempt 1/3) + --- current steps + +++ proposed steps + - 1. Identify the workspace directory path… + + 1. Determine the absolute path of the workspace directory… + + based on 2 recorded failure(s): + - Saved the checklist outside the workspace directory + + [a] accept — write SKILL.md and commit (default) + [e] edit — open the proposed steps in $EDITOR first + [c] cancel — leave the skill unchanged +→ choose [a/e/c]: +``` + +- `[e]` opens the proposed steps in `$VISUAL`/`$EDITOR` (falls back to `vi`), + then re-shows the diff of what you saved so you confirm before it lands. +- `[c]` leaves `SKILL.md` untouched, does not increment `repair_count`, and tells + the model not to retry the repair. +- The repair history records who approved: `(user-approved)`, `(user-edited, + approved)`, or `(unattended)`. + +### Headless / SDK: `autoRepair` + +When nobody can be prompted — no TTY, `GITAGENT_APPROVAL=auto`, or programmatic +`query()` — a single flag decides whether the agent may rewrite its own skills: + +| | flagged skill match | `skill_learner` "repair" | +|---|---|---| +| **default** (`autoRepair` off) | reported as unreliable; repair declared off-limits | refused, no LLM call, `SKILL.md` untouched | +| `autoRepair` on | told to repair first, then use it | rewrites, commits, resets confidence to 0.6 | + +```ts +import { query } from "@open-gitagent/gitagent"; + +for await (const msg of query({ + prompt: "Set up a new laptop", + autoRepair: true, // omit (or false) and flagged skills are never modified +})) { /* … */ } +``` + +CLI equivalents: `gitagent --auto-repair` or `GITAGENT_AUTO_REPAIR=1` (useful for +cron/CI). On a TTY the interactive prompts take precedence over the flag. + +Runnable end-to-end demo of both paths, including the throwaway agent fixture: +[examples/skill-approval.ts](examples/skill-approval.ts). + --- ## Workflows & SkillFlows diff --git a/examples/skill-approval.ts b/examples/skill-approval.ts new file mode 100644 index 0000000..af40ac6 --- /dev/null +++ b/examples/skill-approval.ts @@ -0,0 +1,149 @@ +/** + * Unreliable-skill handling in SDK mode — runnable example. + * + * Scaffolds a throwaway agent whose single skill is deliberately flagged as + * unreliable (confidence 0.3), then runs a task that matches it. One option + * decides what happens: + * + * autoRepair: false (default) → the agent is told the skill is unreliable and + * repair is refused; SKILL.md is never touched. + * autoRepair: true → the agent rewrites the skill's steps from its + * own recorded failures, commits, then uses it. + * + * Usage (from the repo root, after `npm run build`): + * + * node --experimental-strip-types examples/skill-approval.ts # default: no repair + * node --experimental-strip-types examples/skill-approval.ts --auto # autoRepair: true + * + * Requires ANTHROPIC_API_KEY, or set GITAGENT_MODEL=provider:model plus that + * provider's key. + * + * (In the CLI — `gitagent --dir .` on a TTY — you instead get interactive + * prompts: proceed/repair/skip, then accept/edit/cancel on the diff.) + */ + +import { mkdtempSync, mkdirSync, writeFileSync, readFileSync } from "node:fs"; +import { execSync } from "node:child_process"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { query } from "../dist/exports.js"; + +const MODEL = process.env.GITAGENT_MODEL || "anthropic:claude-sonnet-4-6"; +const AUTO_REPAIR = process.argv.includes("--auto"); + +// ── Scaffold a throwaway agent with one flagged skill ─────────────────── + +function scaffoldAgent(): string { + const dir = mkdtempSync(join(tmpdir(), "skill-approval-")); + + writeFileSync(join(dir, "agent.yaml"), `spec_version: "0.1.0" +name: skill-approval-demo +version: 0.1.0 +description: Demo agent for unreliable-skill handling + +model: + preferred: "${MODEL}" + fallback: [] + +tools: [cli, read, write, memory] + +runtime: + max_turns: 12 + timeout: 120 +`); + + // The fixture: a skill that has failed more than it has worked, plus the + // failure lessons a repair would feed back to the model. + const skillDir = join(dir, "skills", "laptop-setup-checklist"); + mkdirSync(skillDir, { recursive: true }); + writeFileSync(join(skillDir, "SKILL.md"), `--- +name: laptop-setup-checklist +description: Generate a new laptop setup checklist covering OS updates, essential software installation, and backup/security configuration, saved as a Markdown file in the workspace directory. +confidence: 0.3 +usage_count: 7 +success_count: 2 +failure_count: 5 +negative_examples: + - Saved the checklist outside the workspace directory so the user could not find it + - Assumed macOS and emitted steps that are invalid on Windows +--- + +## Steps +1. Identify the workspace directory. +2. Write a checklist file covering OS updates, software and backups. +3. Tell the user it is done. +`); + + mkdirSync(join(dir, "workspace"), { recursive: true }); + + // A git repo so a repair commit actually lands and you can read it back. + execSync("git init -q && git add -A && git commit -q -m fixture --no-gpg-sign", { + cwd: dir, + stdio: "pipe", + env: { + ...process.env, + GIT_AUTHOR_NAME: "demo", GIT_AUTHOR_EMAIL: "demo@example.com", + GIT_COMMITTER_NAME: "demo", GIT_COMMITTER_EMAIL: "demo@example.com", + }, + }); + + return dir; +} + +// ── Run it ────────────────────────────────────────────────────────────── + +async function main() { + const dir = scaffoldAgent(); + console.log(`agent dir: ${dir}`); + console.log(`model: ${MODEL}`); + console.log(`autoRepair: ${AUTO_REPAIR}\n`); + + for await (const msg of query({ + prompt: + "Help me set up a new laptop. Start by tracking this with task_tracker " + + '(objective: "Help the user set up a new laptop checklist"), then follow whatever it tells you to do.', + dir, + model: MODEL, + autoRepair: AUTO_REPAIR, // ← the whole knob + })) { + switch (msg.type) { + case "delta": + if (msg.deltaType === "text") process.stdout.write(msg.content); + break; + case "tool_use": + console.log(`\n▶ ${msg.toolName}(${JSON.stringify(msg.args).slice(0, 120)})`); + break; + case "tool_result": { + const text = msg.content.length > 500 ? `${msg.content.slice(0, 500)}…` : msg.content; + console.log(text.split("\n").map((l) => ` ${l}`).join("\n")); + break; + } + case "assistant": + if (msg.errorMessage) console.error(`\n[error] ${msg.errorMessage}`); + break; + case "system": + console.log(`[${msg.subtype}] ${msg.content}`); + break; + } + } + + // ── What actually happened to the skill on disk ───────────────────── + const skillFile = join(dir, "skills", "laptop-setup-checklist", "SKILL.md"); + const after = readFileSync(skillFile, "utf-8"); + const confidence = after.match(/^confidence: (.+)$/m)?.[1]; + const repairs = after.match(/^repair_count: (.+)$/m)?.[1] ?? "0"; + const log = execSync("git log --oneline", { cwd: dir, encoding: "utf-8" }).trim(); + + console.log(`\n──────── result ────────`); + // 0.3 = never repaired. A repair resets it to 0.6, and a successful run nudges it up. + console.log(`confidence: ${confidence} (0.3 = untouched, >= 0.6 = repaired)`); + console.log(`repair_count: ${repairs}`); + console.log(`git log:\n${log.split("\n").map((l) => ` ${l}`).join("\n")}`); + console.log(`\nInspect the skill: cat ${skillFile}`); + console.log(`Inspect the output: ls ${join(dir, "workspace")}`); +} + +main().catch((err) => { + console.error(err); + process.exit(1); +}); diff --git a/src/elicit.ts b/src/elicit.ts new file mode 100644 index 0000000..d7c0280 --- /dev/null +++ b/src/elicit.ts @@ -0,0 +1,165 @@ +import { createInterface, type Interface } from "readline"; +import { spawnSync } from "child_process"; +import { mkdtempSync, readFileSync, writeFileSync, rmSync } from "fs"; +import { tmpdir } from "os"; +import { join } from "path"; + +// ANSI helpers (kept local, same style as index.ts) +const dim = (s: string) => `\x1b[2m${s}\x1b[0m`; +const bold = (s: string) => `\x1b[1m${s}\x1b[0m`; +const yellow = (s: string) => `\x1b[33m${s}\x1b[0m`; + +// ── Types ─────────────────────────────────────────────────────────────── + +export interface ElicitChoice { + /** Single character the user types to pick this option. */ + key: string; + /** Short imperative label, e.g. `accept — write the file and commit`. */ + label: string; +} + +export interface ElicitSelectRequest { + title: string; + /** Pre-rendered detail block (diff, stats, failure lessons) printed above the choices. */ + body?: string; + choices: ElicitChoice[]; + /** Key used when the user just presses Enter, and when nobody can answer. */ + defaultKey: string; + signal?: AbortSignal; +} + +/** + * A human-in-the-loop channel a tool can use mid-execution: it blocks the + * agent loop until the user picks an option, so the *user's* decision — not + * the model's guess — drives what happens next. + * + * `interactive` is false when nobody can answer (no TTY, or the approval + * policy is "auto"). Callers MUST check it and keep their previous autonomous + * behaviour in that case, so headless runs and CI don't hang or silently + * change meaning. + */ +export interface Elicitor { + readonly interactive: boolean; + /** Returns the chosen `key`. Rejects with "Operation aborted" if `signal` fires. */ + select(req: ElicitSelectRequest): Promise; + /** + * Opens `initial` in $VISUAL/$EDITOR for hand-editing. + * Returns the edited text, or null if no editor was available or nothing changed. + */ + edit(initial: string, opts?: { extension?: string }): Promise; +} + +// ── Console implementation ────────────────────────────────────────────── + +function firstWord(label: string): string { + return label.split(/[\s—-]/)[0].toLowerCase(); +} + +function askOn(rl: Interface, query: string, signal?: AbortSignal): Promise { + return new Promise((resolve, reject) => { + if (signal?.aborted) { + reject(new Error("Operation aborted")); + return; + } + const onAbort = () => reject(new Error("Operation aborted")); + signal?.addEventListener("abort", onAbort, { once: true }); + rl.question(query, { signal }, (answer) => { + signal?.removeEventListener("abort", onAbort); + resolve(answer); + }); + }); +} + +export class ConsoleElicitor implements Elicitor { + private rl: Interface | null = null; + + /** + * Share the REPL's readline interface. Required in REPL mode — two + * interfaces on the same stdin fight over input. In single-shot mode + * there is no REPL, so a short-lived interface is created per prompt. + */ + attach(rl: Interface | null): void { + this.rl = rl; + } + + get interactive(): boolean { + // Escape hatch for scripted/CI runs that want today's fully autonomous behaviour. + if (process.env.GITAGENT_APPROVAL === "auto") return false; + return process.stdin.isTTY === true; + } + + async select(req: ElicitSelectRequest): Promise { + if (!this.interactive) return req.defaultKey; + + const keys = req.choices.map((c) => c.key).join("/"); + let out = `\n${yellow(req.title)}\n`; + if (req.body) out += `${req.body}\n`; + out += "\n"; + for (const c of req.choices) { + const marker = c.key === req.defaultKey ? dim(" (default)") : ""; + out += ` ${bold(`[${c.key}]`)} ${c.label}${marker}\n`; + } + process.stdout.write(out); + + for (;;) { + const answer = (await this.question(`→ choose [${keys}]: `, req.signal)).trim().toLowerCase(); + if (!answer) return req.defaultKey; + const hit = req.choices.find( + (c) => c.key === answer || firstWord(c.label).startsWith(answer), + ); + if (hit) return hit.key; + process.stdout.write(dim(` "${answer}" is not one of ${keys} — try again.\n`)); + } + } + + async edit(initial: string, opts?: { extension?: string }): Promise { + if (!this.interactive) return null; + + const editorCmd = process.env.VISUAL || process.env.EDITOR || + (process.platform === "win32" ? "notepad" : "vi"); + const [cmd, ...cmdArgs] = editorCmd.split(/\s+/); + + const tmp = mkdtempSync(join(tmpdir(), "gitagent-edit-")); + const file = join(tmp, `proposal${opts?.extension ?? ".txt"}`); + try { + writeFileSync(file, initial, "utf-8"); + + // The child takes over the tty; pause the REPL's readline so it + // doesn't consume the editor's keystrokes. + this.rl?.pause(); + const res = spawnSync(cmd, [...cmdArgs, file], { stdio: "inherit" }); + this.rl?.resume(); + + if (res.error || res.status !== 0) { + const why = res.error?.message ?? `exited ${res.status}`; + process.stdout.write(dim(` Could not run "${editorCmd}" (${why}) — proposal left unchanged.\n`)); + return null; + } + + const edited = readFileSync(file, "utf-8"); + if (edited.trim() === initial.trim()) { + process.stdout.write(dim(" No changes saved.\n")); + return null; + } + return edited; + } finally { + try { + rmSync(tmp, { recursive: true, force: true }); + } catch { + // best-effort temp cleanup + } + } + } + + private question(query: string, signal?: AbortSignal): Promise { + const shared = this.rl; + if (shared) return askOn(shared, query, signal); + + const rl = createInterface({ input: process.stdin, output: process.stdout }); + return askOn(rl, query, signal).finally(() => rl.close()); + } +} + +export function createConsoleElicitor(): ConsoleElicitor { + return new ConsoleElicitor(); +} diff --git a/src/index.ts b/src/index.ts index 6ba2efa..30ff780 100644 --- a/src/index.ts +++ b/src/index.ts @@ -8,6 +8,7 @@ import { createBuiltinTools } from "./tools/index.js"; import { createSandboxContext } from "./sandbox.js"; import type { SandboxContext, SandboxConfig } from "./sandbox.js"; import { expandSkillCommand, refreshSkills } from "./skills.js"; +import { createConsoleElicitor } from "./elicit.js"; import { loadHooksConfig, runHooks, wrapToolWithHooks } from "./hooks.js"; import type { HooksConfig } from "./hooks.js"; import { loadDeclarativeTools } from "./tool-loader.js"; @@ -53,6 +54,7 @@ interface ParsedArgs { pat?: string; session?: string; voice?: string; + autoRepair?: boolean; } function parseArgs(argv: string[]): ParsedArgs { @@ -68,6 +70,7 @@ function parseArgs(argv: string[]): ParsedArgs { let pat: string | undefined; let session: string | undefined; let voice: string | undefined; + let autoRepair = false; for (let i = 0; i < args.length; i++) { switch (args[i]) { @@ -91,6 +94,9 @@ function parseArgs(argv: string[]): ParsedArgs { case "-s": sandbox = true; break; + case "--auto-repair": + autoRepair = true; + break; case "--sandbox-repo": sandboxRepo = args[++i]; break; @@ -124,7 +130,7 @@ function parseArgs(argv: string[]): ParsedArgs { } } - return { model, dir, prompt, env, sandbox, sandboxRepo, sandboxToken, repo, pat, session, voice }; + return { model, dir, prompt, env, sandbox, sandboxRepo, sandboxToken, repo, pat, session, voice, autoRepair }; } function handleEvent( @@ -321,7 +327,7 @@ async function main(): Promise { return; } - const { model, dir: rawDir, prompt, env, sandbox: useSandbox, sandboxRepo, sandboxToken, repo, pat, session: sessionBranch, voice } = parseArgs(process.argv); + const { model, dir: rawDir, prompt, env, sandbox: useSandbox, sandboxRepo, sandboxToken, repo, pat, session: sessionBranch, voice, autoRepair: autoRepairFlag } = parseArgs(process.argv); // If --repo is given, derive a default dir from the repo URL (skip interactive prompt) let dir = rawDir; @@ -535,6 +541,13 @@ async function main(): Promise { // same running total via onUsage below. let _totalCostUsd = 0; + // Human-in-the-loop channel for skill decisions. In REPL mode it gets the + // REPL's own readline interface attached below; in single-shot mode it opens + // a short-lived one per prompt. When it can't prompt (no TTY, GITAGENT_APPROVAL=auto), + // --auto-repair / GITAGENT_AUTO_REPAIR decides whether repair may run unattended. + const elicitor = createConsoleElicitor(); + const autoRepair = autoRepairFlag === true || process.env.GITAGENT_AUTO_REPAIR === "1"; + // Build tools — built-in + declarative let tools: AgentTool[] = createBuiltinTools({ dir, @@ -543,6 +556,8 @@ async function main(): Promise { gitagentDir, pluginMemoryLayers: pluginMemoryLayers.length > 0 ? pluginMemoryLayers : undefined, model: loaded.model, + elicit: elicitor, + autoRepair, onUsage: (msg) => { if (msg.usage) _totalCostUsd += msg.usage.costUsd ?? 0; }, @@ -687,6 +702,8 @@ async function main(): Promise { input: process.stdin, output: process.stdout, }); + // Share stdin with the REPL — a second interface would fight it for input. + elicitor.attach(rl); const ask = (): void => { rl.question(green("→ "), async (input) => { diff --git a/src/sdk-types.ts b/src/sdk-types.ts index 5ab79ca..8a7e564 100644 --- a/src/sdk-types.ts +++ b/src/sdk-types.ts @@ -143,6 +143,13 @@ export interface QueryOptions { repo?: LocalRepoOptions; sandbox?: SandboxOptions | boolean; hooks?: GCHooks; + /** + * Allow the agent to rewrite and commit its own unreliable skills (confidence < 0.4) + * without a human in the loop. Default false: a flagged skill is reported as + * unreliable and `skill_learner` "repair" refuses, leaving SKILL.md untouched. + * The CLI ignores this when stdin is a TTY — there it prompts instead. + */ + autoRepair?: boolean; maxTurns?: number; abortController?: AbortController; sessionId?: string; diff --git a/src/sdk.ts b/src/sdk.ts index 10a4b18..00d1b93 100644 --- a/src/sdk.ts +++ b/src/sdk.ts @@ -186,6 +186,7 @@ export function query(options: QueryOptions): Query { gitagentDir: loaded.gitagentDir, pluginMemoryLayers: pluginMemoryLayers.length > 0 ? pluginMemoryLayers : undefined, model: loaded.model, + autoRepair: options.autoRepair, onUsage: (msg) => { if (!msg.usage) return; costTracker.add(`${msg.provider}:${msg.model}`, msg.usage); diff --git a/src/text-diff.ts b/src/text-diff.ts new file mode 100644 index 0000000..7266a97 --- /dev/null +++ b/src/text-diff.ts @@ -0,0 +1,85 @@ +// Minimal line diff for approval previews — no dependency, no patch format. +// Inputs here are skill step lists (tens of short lines), so a plain LCS table +// is fine; anything larger falls back to a whole-block replace. + +const dim = (s: string) => `\x1b[2m${s}\x1b[0m`; +const red = (s: string) => `\x1b[31m${s}\x1b[0m`; +const green = (s: string) => `\x1b[32m${s}\x1b[0m`; + +const MAX_LCS_LINES = 400; + +export interface DiffLine { + sign: " " | "-" | "+"; + text: string; +} + +export function diffLines(before: string, after: string): DiffLine[] { + const a = before.split(/\r?\n/); + const b = after.split(/\r?\n/); + + if (a.length > MAX_LCS_LINES || b.length > MAX_LCS_LINES) { + return [ + ...a.map((text) => ({ sign: "-" as const, text })), + ...b.map((text) => ({ sign: "+" as const, text })), + ]; + } + + // lcs[i][j] = length of the longest common subsequence of a[i..] and b[j..] + const lcs: number[][] = Array.from({ length: a.length + 1 }, () => + new Array(b.length + 1).fill(0), + ); + for (let i = a.length - 1; i >= 0; i--) { + for (let j = b.length - 1; j >= 0; j--) { + lcs[i][j] = a[i] === b[j] + ? lcs[i + 1][j + 1] + 1 + : Math.max(lcs[i + 1][j], lcs[i][j + 1]); + } + } + + const out: DiffLine[] = []; + let i = 0; + let j = 0; + while (i < a.length && j < b.length) { + if (a[i] === b[j]) { + out.push({ sign: " ", text: a[i] }); + i++; + j++; + } else if (lcs[i + 1][j] >= lcs[i][j + 1]) { + out.push({ sign: "-", text: a[i] }); + i++; + } else { + out.push({ sign: "+", text: b[j] }); + j++; + } + } + while (i < a.length) out.push({ sign: "-", text: a[i++] }); + while (j < b.length) out.push({ sign: "+", text: b[j++] }); + + return out; +} + +export interface RenderDiffOptions { + beforeLabel?: string; + afterLabel?: string; + indent?: string; +} + +/** Colourised diff for terminal approval prompts. */ +export function renderDiff(before: string, after: string, opts: RenderDiffOptions = {}): string { + const indent = opts.indent ?? " "; + const lines = diffLines(before, after) + .filter((l) => !(l.text === "" && l.sign === " ")) + .map((l) => { + const text = `${indent}${l.sign} ${l.text}`; + if (l.sign === "-") return red(text); + if (l.sign === "+") return green(text); + return dim(text); + }); + + const header = [ + dim(`${indent}--- ${opts.beforeLabel ?? "before"}`), + dim(`${indent}+++ ${opts.afterLabel ?? "after"}`), + ]; + + return [...header, ...lines].join("\n"); +} diff --git a/src/tools/index.ts b/src/tools/index.ts index c2f0b43..e986107 100644 --- a/src/tools/index.ts +++ b/src/tools/index.ts @@ -3,6 +3,7 @@ import type { Model } from "@mariozechner/pi-ai"; import type { SandboxContext } from "../sandbox.js"; import type { MemoryLayerDef } from "../plugin-types.js"; import type { GCAssistantMessage } from "../sdk-types.js"; +import type { Elicitor } from "../elicit.js"; import { createCliTool } from "./cli.js"; import { createReadTool } from "./read.js"; import { createWriteTool } from "./write.js"; @@ -27,6 +28,18 @@ export interface BuiltinToolsConfig { model?: Model; /** Called with the reflection LLM call's usage, so callers can feed it into their own cost tracking. */ onUsage?: (msg: GCAssistantMessage) => void; + /** + * Terminal prompt channel for decisions the user should own: whether to use a + * flagged skill, and whether to accept a proposed skill repair. CLI-only — + * programmatic callers use `autoRepair` instead. + */ + elicit?: Elicitor; + /** + * Let the agent repair its own flagged skills unattended. Only consulted when + * `elicit` can't prompt (no TTY, or programmatic use). Default false: flagged + * skills are reported and "repair" refuses. + */ + autoRepair?: boolean; } /** @@ -56,8 +69,8 @@ export function createBuiltinTools(config: BuiltinToolsConfig): AgentTool[] // Add learning tools if gitagentDir is available if (config.gitagentDir) { - tools.push(createTaskTrackerTool(config.dir, config.gitagentDir, config.model, config.onUsage)); - tools.push(createSkillLearnerTool(config.dir, config.gitagentDir, config.model, config.onUsage)); + tools.push(createTaskTrackerTool(config.dir, config.gitagentDir, config.model, config.onUsage, config.elicit, config.autoRepair)); + tools.push(createSkillLearnerTool(config.dir, config.gitagentDir, config.model, config.onUsage, config.elicit, config.autoRepair)); } return tools; diff --git a/src/tools/skill-learner.ts b/src/tools/skill-learner.ts index 57f84e5..eed76ae 100644 --- a/src/tools/skill-learner.ts +++ b/src/tools/skill-learner.ts @@ -9,6 +9,8 @@ import { skillLearnerSchema } from "./shared.js"; import { loadSkillStats, isSkillFlagged } from "../learning/reinforcement.js"; import { repairSkillSteps } from "../learning/skill-repair.js"; import type { TaskRecord } from "./task-tracker.js"; +import type { Elicitor } from "../elicit.js"; +import { renderDiff } from "../text-diff.js"; import yaml from "js-yaml"; // Caps how many times a single skill can be auto-repaired before it must @@ -121,11 +123,35 @@ function gitCommit(agentDir: string, files: string[], message: string): void { // ── Tool factory ──────────────────────────────────────────────────────── +/** Diff + failure lessons block shown above the accept/edit/cancel choices. */ +function renderRepairPreview( + currentSteps: string, + proposedSteps: string, + negativeExamples: string[], +): string { + const dim = (s: string) => `\x1b[2m${s}\x1b[0m`; + const parts = [ + renderDiff(currentSteps, proposedSteps, { + beforeLabel: "current steps", + afterLabel: "proposed steps", + }), + ]; + if (negativeExamples.length > 0) { + const lessons = negativeExamples + .slice(-3) + .map((n) => ` - ${n.length > 160 ? `${n.slice(0, 160)}…` : n}`); + parts.push(dim(`\n based on ${negativeExamples.length} recorded failure(s):\n${lessons.join("\n")}`)); + } + return parts.join("\n"); +} + export function createSkillLearnerTool( agentDir: string, gitagentDir: string, model?: Model, onUsage?: (msg: GCAssistantMessage) => void, + elicit?: Elicitor, + autoRepair?: boolean, ): AgentTool { return { name: "skill_learner", @@ -295,7 +321,7 @@ export function createSkillLearnerTool( }; } - const skills: Array<{ name: string; confidence: number; usage: number; ratio: string; repairs: number }> = []; + const skills: Array<{ name: string; confidence: number; usage: number; ratio: string; repairs: number; flagged: boolean }> = []; for (const entry of entries) { if (!entry.isDirectory()) continue; @@ -309,6 +335,7 @@ export function createSkillLearnerTool( usage: stats.usage_count, ratio: `${stats.success_count}/${stats.success_count + stats.failure_count}`, repairs, + flagged: isSkillFlagged(stats), }); } @@ -321,10 +348,15 @@ export function createSkillLearnerTool( const lines = skills.map((s) => ` ${s.name}: confidence=${s.confidence}, usage=${s.usage}, success_ratio=${s.ratio}` + - (s.repairs > 0 ? `, repairs=${s.repairs}/${MAX_REPAIRS}` : ""), + (s.repairs > 0 ? `, repairs=${s.repairs}/${MAX_REPAIRS}` : "") + + (s.flagged ? ` ⚠️ FLAGGED — unreliable, consider action "repair"` : ""), ); + const flaggedCount = skills.filter((s) => s.flagged).length; + const footer = flaggedCount > 0 + ? `\n\n${flaggedCount} skill(s) flagged as unreliable. Before using one, call skill_learner action "repair" on it first, or proceed with caution.` + : ""; return { - content: [{ type: "text", text: `Skills:\n${lines.join("\n")}` }], + content: [{ type: "text", text: `Skills:\n${lines.join("\n")}${footer}` }], details: { skills }, }; } @@ -412,6 +444,21 @@ export function createSkillLearnerTool( ); } + // A repair rewrites and commits a file the agent will keep following, so + // it needs someone to sign off: a human at the terminal, or an explicit + // autoRepair opt-in. With neither, refuse before spending an LLM call. + if (!elicit?.interactive && !autoRepair) { + return { + content: [{ + type: "text", + text: `Repair of "${params.skill_name}" was NOT applied — repair needs approval, and this run has neither an interactive terminal nor autoRepair enabled. ` + + `skills/${params.skill_name}/SKILL.md is unchanged (confidence still ${stats.confidence}).\n\n` + + `Do NOT retry. Either use the skill as-is and report the real outcome, or solve the task from scratch.`, + }], + details: { skill_name: params.skill_name, approved: false, reason: "no_approval_channel" }, + }; + } + const stepsMatch = body.match(/## Steps\r?\n([\s\S]*?)(?=\r?\n## |\s*$)/); const currentSteps = stepsMatch ? stepsMatch[1].trim() : body.trim(); @@ -425,13 +472,58 @@ export function createSkillLearnerTool( onUsage, ); + // Human approval gate: nothing is written or committed until the user + // accepts. Skipped under autoRepair, which applies the rewrite unattended. + let finalSteps = repairedSteps; + let userEdited = false; + const attemptNo = repairCount + 1; + + if (elicit?.interactive) { + for (;;) { + const choice = await elicit.select({ + title: `Proposed repair for skill "${params.skill_name}" (attempt ${attemptNo}/${MAX_REPAIRS})`, + body: renderRepairPreview(currentSteps, finalSteps, stats.negative_examples), + choices: [ + { key: "a", label: "accept — write SKILL.md and commit" }, + { key: "e", label: "edit — open the proposed steps in $EDITOR first" }, + { key: "c", label: "cancel — leave the skill unchanged" }, + ], + defaultKey: "a", + signal, + }); + + if (choice === "a") break; + + if (choice === "c") { + return { + content: [{ + type: "text", + text: `Repair of "${params.skill_name}" was CANCELLED BY THE USER. ` + + `skills/${params.skill_name}/SKILL.md is unchanged (confidence still ${stats.confidence}).\n\n` + + `Do NOT retry the repair. Either use the skill as-is and report the real outcome, or solve the task from scratch.`, + }], + details: { skill_name: params.skill_name, approved: false, cancelled: true }, + }; + } + + const edited = await elicit.edit(finalSteps, { extension: ".md" }); + if (edited !== null) { + finalSteps = edited.trim(); + userEdited = true; + } + } + } + const historyMatch = body.match(/## Repair History\r?\n([\s\S]*?)(?=\r?\n## |\s*$)/); const existingHistory = historyMatch ? historyMatch[1].trim() : ""; - const attempt = repairCount + 1; + const attempt = attemptNo; const lessonLines = stats.negative_examples.length ? stats.negative_examples.map((n) => ` - ${n}`).join("\n") : " - (no specific lessons recorded)"; - const newEntry = `- Repair #${attempt} on ${new Date().toISOString()}:\n${lessonLines}`; + const approval = elicit?.interactive + ? userEdited ? " (user-edited, approved)" : " (user-approved)" + : " (autoRepair)"; + const newEntry = `- Repair #${attempt} on ${new Date().toISOString()}${approval}:\n${lessonLines}`; const historyBody = existingHistory ? `${existingHistory}\n${newEntry}` : newEntry; frontmatter.confidence = REPAIR_RESET_CONFIDENCE; @@ -441,7 +533,7 @@ export function createSkillLearnerTool( frontmatter.negative_examples = []; frontmatter.repair_count = attempt; - const newBody = `\n## Steps\n${repairedSteps}\n\n## Repair History\n${historyBody}\n`; + const newBody = `\n## Steps\n${finalSteps}\n\n## Repair History\n${historyBody}\n`; const yamlStr = yaml.dump(frontmatter, { lineWidth: -1, noRefs: true }).trimEnd(); const updated = `---\n${yamlStr}\n---\n${newBody}`; @@ -451,9 +543,20 @@ export function createSkillLearnerTool( return { content: [{ type: "text", - text: `Skill "${params.skill_name}" repaired (attempt ${attempt}/${MAX_REPAIRS}) and committed.\nConfidence reset to ${REPAIR_RESET_CONFIDENCE} (was ${stats.confidence}).\nSteps rewritten based on ${stats.negative_examples.length} recorded failure(s).`, + text: `Skill "${params.skill_name}" repaired (attempt ${attempt}/${MAX_REPAIRS}) and committed.` + + (elicit?.interactive + ? `\nThe user ${userEdited ? "edited and approved" : "approved"} the new steps.` + : "") + + `\nConfidence reset to ${REPAIR_RESET_CONFIDENCE} (was ${stats.confidence}).\nSteps rewritten based on ${stats.negative_examples.length} recorded failure(s).` + + `\n\nNow load skills/${params.skill_name}/SKILL.md and follow the repaired steps.`, }], - details: { skill_name: params.skill_name, repair_count: attempt, confidence: REPAIR_RESET_CONFIDENCE }, + details: { + skill_name: params.skill_name, + repair_count: attempt, + confidence: REPAIR_RESET_CONFIDENCE, + approved: true, + user_edited: userEdited, + }, }; } diff --git a/src/tools/task-tracker.ts b/src/tools/task-tracker.ts index beb638b..2a4da62 100644 --- a/src/tools/task-tracker.ts +++ b/src/tools/task-tracker.ts @@ -8,6 +8,7 @@ import type { GCAssistantMessage } from "../sdk-types.js"; import { taskTrackerSchema } from "./shared.js"; import { adjustConfidence, loadSkillStats, saveSkillStats } from "../learning/reinforcement.js"; import { reflectOnFailure } from "../learning/reflection.js"; +import type { Elicitor } from "../elicit.js"; import yaml from "js-yaml"; // ── Types ─────────────────────────────────────────────────────────────── @@ -75,6 +76,10 @@ interface SkillMatch { confidence?: number; source: "local" | "marketplace"; relevance: number; + /** Local skills only — shown to the user when a flagged match needs a decision. */ + successCount?: number; + failureCount?: number; + negativeExamples?: string[]; } async function searchLocalSkills(agentDir: string, objective: string): Promise { @@ -119,6 +124,11 @@ async function searchLocalSkills(agentDir: string, objective: string): Promise { // ── Tool factory ──────────────────────────────────────────────────────── +/** Renders the evidence a human needs to decide whether a flagged skill is worth using. */ +function describeFlaggedSkill(match: SkillMatch): string { + const dim = (s: string) => `\x1b[2m${s}\x1b[0m`; + const lines = [` ${match.name} — ${match.description}`]; + + const stats: string[] = []; + if (match.confidence !== undefined) stats.push(`confidence ${match.confidence}`); + if (match.successCount !== undefined && match.failureCount !== undefined) { + stats.push(`${match.successCount} success / ${match.failureCount} failure`); + } + if (stats.length > 0) lines.push(dim(` ${stats.join(" · ")}`)); + + const recent = (match.negativeExamples ?? []).slice(-3); + if (recent.length > 0) { + lines.push(dim(" recent failures:")); + for (const n of recent) { + const short = n.length > 160 ? `${n.slice(0, 160)}…` : n; + lines.push(dim(` - ${short}`)); + } + } + + return lines.join("\n"); +} + export function createTaskTrackerTool( agentDir: string, gitagentDir: string, model?: Model, onUsage?: (msg: GCAssistantMessage) => void, + elicit?: Elicitor, + autoRepair?: boolean, ): AgentTool { return { name: "task_tracker", @@ -232,13 +268,62 @@ export function createTaskTrackerTool( } } + // "p" | "r" | "s" once a human has ruled on a flagged match; null otherwise. + let flaggedDecision: string | null = null; + if (allMatches.length > 0) { const topMatch = allMatches[0]; const topConf = topMatch.confidence !== undefined ? ` (confidence: ${topMatch.confidence})` : ""; - response += `\n\n⚡ SKILL MATCH FOUND — YOU MUST USE IT:`; - response += `\n → ${topMatch.name}: ${topMatch.description}${topConf} [${topMatch.source}]`; - response += `\n\nACTION REQUIRED: Load skills/${topMatch.name}/SKILL.md NOW and follow its instructions.`; - response += `\nDo NOT proceed with a manual approach — the skill handles this task.`; + // Matches isSkillFlagged()'s threshold in reinforcement.ts — don't push + // a known-unreliable skill as mandatory without surfacing that first. + const topFlagged = topMatch.confidence !== undefined && topMatch.confidence < 0.4; + + if (topFlagged) { + // A flagged skill is a judgement call, not a mechanical one: ask the + // human which way to go and hand the model their decision, instead of + // letting it pick repair-vs-proceed on its own. With no terminal, the + // autoRepair flag decides whether repair is even on the table. + const decision = flaggedDecision = elicit?.interactive + ? await elicit.select({ + title: `⚠️ Skill "${topMatch.name}" matches this task but is flagged as unreliable`, + body: describeFlaggedSkill(topMatch), + choices: [ + { key: "p", label: "proceed — use the skill as-is" }, + { key: "r", label: "repair — rewrite its steps first (you review the change)" }, + { key: "s", label: "skip — ignore the skill, solve from scratch" }, + ], + defaultKey: "p", + signal, + }) + : null; + + response += `\n\n⚠️ SKILL MATCH FOUND, BUT IT'S FLAGGED AS UNRELIABLE:`; + response += `\n → ${topMatch.name}: ${topMatch.description}${topConf} [${topMatch.source}]`; + + if (decision === "p") { + response += `\n\nUSER DECISION: proceed with the flagged skill as-is.`; + response += `\nACTION REQUIRED: Load skills/${topMatch.name}/SKILL.md NOW and follow its instructions.`; + response += `\nDo NOT call skill_learner action "repair" — the user declined that. Report the real outcome via task_tracker "end" with skill_used "${topMatch.name}".`; + } else if (decision === "r") { + response += `\n\nUSER DECISION: repair the skill before using it.`; + response += `\nACTION REQUIRED: Call skill_learner action "repair" with skill_name "${topMatch.name}" NOW. The user reviews and approves the rewritten steps.`; + response += `\nIf the repair is approved, load skills/${topMatch.name}/SKILL.md and follow the repaired steps. If the user cancels the repair, do not use the skill — solve the task from scratch.`; + } else if (decision === "s") { + response += `\n\nUSER DECISION: skip the skill entirely.`; + response += `\nACTION REQUIRED: Do NOT read or use skills/${topMatch.name}/SKILL.md, and do NOT call skill_learner "repair". Solve this task from scratch and do not pass skill_used to task_tracker "end".`; + } else if (autoRepair) { + response += `\n\nThis skill has a low confidence score from repeated failures, and automatic repair is ENABLED for this run.`; + response += `\nACTION REQUIRED: Call skill_learner action "repair" with skill_name "${topMatch.name}" first, then load skills/${topMatch.name}/SKILL.md and follow the repaired steps.`; + } else { + response += `\n\nThis skill has a low confidence score from repeated failures, and repair is DISABLED for this run.`; + response += `\nDo NOT call skill_learner action "repair" — it will be refused. Either load skills/${topMatch.name}/SKILL.md and use it with caution (reporting the real outcome via task_tracker "end"), or solve the task from scratch.`; + } + } else { + response += `\n\n⚡ SKILL MATCH FOUND — YOU MUST USE IT:`; + response += `\n → ${topMatch.name}: ${topMatch.description}${topConf} [${topMatch.source}]`; + response += `\n\nACTION REQUIRED: Load skills/${topMatch.name}/SKILL.md NOW and follow its instructions.`; + response += `\nDo NOT proceed with a manual approach — the skill handles this task.`; + } if (allMatches.length > 1) { response += `\n\nOther matching skills:`; for (const m of allMatches.slice(1, 5)) { @@ -252,7 +337,7 @@ export function createTaskTrackerTool( return { content: [{ type: "text", text: response }], - details: { task_id: task.id, matches: allMatches }, + details: { task_id: task.id, matches: allMatches, flagged_decision: flaggedDecision ?? undefined }, }; } diff --git a/test/elicit.test.ts b/test/elicit.test.ts new file mode 100644 index 0000000..867e412 --- /dev/null +++ b/test/elicit.test.ts @@ -0,0 +1,166 @@ +import { describe, it, before } from "node:test"; +import assert from "node:assert/strict"; +import { mkdtempSync, mkdirSync, writeFileSync, readFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +let diffLines: typeof import("../dist/text-diff.js").diffLines; +let renderDiff: typeof import("../dist/text-diff.js").renderDiff; +let createConsoleElicitor: typeof import("../dist/elicit.js").createConsoleElicitor; +let createTaskTrackerTool: typeof import("../dist/tools/task-tracker.js").createTaskTrackerTool; +let createSkillLearnerTool: typeof import("../dist/tools/skill-learner.js").createSkillLearnerTool; + +before(async () => { + ({ diffLines, renderDiff } = await import("../dist/text-diff.js")); + ({ createConsoleElicitor } = await import("../dist/elicit.js")); + ({ createTaskTrackerTool } = await import("../dist/tools/task-tracker.js")); + ({ createSkillLearnerTool } = await import("../dist/tools/skill-learner.js")); +}); + +const strip = (s: string) => s.replace(/\x1b\[[0-9;]*m/g, ""); + +describe("diffLines", () => { + it("keeps common lines as context and marks the rest", () => { + const out = diffLines("a\nb\nc", "a\nB\nc"); + assert.deepEqual(out, [ + { sign: " ", text: "a" }, + { sign: "-", text: "b" }, + { sign: "+", text: "B" }, + { sign: " ", text: "c" }, + ]); + }); + + it("reports no changes for identical text", () => { + const out = diffLines("1. one\n2. two", "1. one\n2. two"); + assert.ok(out.every((l) => l.sign === " ")); + }); + + it("marks pure additions and pure deletions", () => { + assert.deepEqual( + diffLines("a", "a\nb").filter((l) => l.sign !== " "), + [{ sign: "+", text: "b" }], + ); + assert.deepEqual( + diffLines("a\nb", "a").filter((l) => l.sign !== " "), + [{ sign: "-", text: "b" }], + ); + }); +}); + +describe("renderDiff", () => { + it("prefixes labelled headers and signs each line", () => { + const text = strip(renderDiff("old", "new", { beforeLabel: "current", afterLabel: "proposed" })); + assert.match(text, /--- current/); + assert.match(text, /\+\+\+ proposed/); + assert.match(text, /^ {2}- old$/m); + assert.match(text, /^ {2}\+ new$/m); + }); +}); + +describe("ConsoleElicitor (non-interactive)", () => { + it("is non-interactive without a TTY and returns the default key without reading stdin", async () => { + const e = createConsoleElicitor(); + assert.equal(e.interactive, process.stdin.isTTY === true); + if (e.interactive) return; // only assert the headless contract under `node --test` + assert.equal(await e.select({ title: "t", choices: [{ key: "a", label: "accept" }], defaultKey: "a" }), "a"); + assert.equal(await e.edit("text"), null); + }); +}); + +describe("task_tracker flagged-skill gate", () => { + const makeAgentDir = (confidence: number) => { + const dir = mkdtempSync(join(tmpdir(), "gitagent-elicit-test-")); + const skillDir = join(dir, "skills", "widget-checklist"); + mkdirSync(skillDir, { recursive: true }); + writeFileSync( + join(skillDir, "SKILL.md"), + `---\nname: widget-checklist\ndescription: Generate a widget checklist file\nconfidence: ${confidence}\nusage_count: 6\nsuccess_count: 2\nfailure_count: 4\nnegative_examples:\n - wrote the file to the wrong directory\n---\n\n## Steps\n1. do the thing\n`, + ); + return dir; + }; + + it("routes a user 'repair' decision into the tool result", async () => { + const dir = makeAgentDir(0.3); + const elicit = { + interactive: true, + select: async () => "r", + edit: async () => null, + }; + const tool = createTaskTrackerTool(dir, dir, undefined, undefined, elicit); + const res = await tool.execute("c1", { action: "begin", objective: "Generate a widget checklist" }); + const text = res.content[0].text; + assert.match(text, /USER DECISION: repair the skill before using it/); + assert.match(text, /skill_learner action "repair" with skill_name "widget-checklist"/); + assert.equal(res.details?.flagged_decision, "r"); + }); + + it("routes a user 'skip' decision into the tool result", async () => { + const dir = makeAgentDir(0.3); + const elicit = { interactive: true, select: async () => "s", edit: async () => null }; + const tool = createTaskTrackerTool(dir, dir, undefined, undefined, elicit); + const res = await tool.execute("c1", { action: "begin", objective: "Generate a widget checklist" }); + assert.match(res.content[0].text, /USER DECISION: skip the skill entirely/); + }); + + it("tells the model repair is disabled when nobody can approve it", async () => { + const dir = makeAgentDir(0.3); + const elicit = { interactive: false, select: async () => "r", edit: async () => null }; + const tool = createTaskTrackerTool(dir, dir, undefined, undefined, elicit, false); + const res = await tool.execute("c1", { action: "begin", objective: "Generate a widget checklist" }); + const text = res.content[0].text; + assert.doesNotMatch(text, /USER DECISION/); + assert.match(text, /repair is DISABLED/); + assert.match(text, /Do NOT call skill_learner action "repair"/); + assert.equal(res.details?.flagged_decision, undefined); + }); + + it("points the model at repair when autoRepair is on and nobody can be asked", async () => { + const dir = makeAgentDir(0.3); + const tool = createTaskTrackerTool(dir, dir, undefined, undefined, undefined, true); + const res = await tool.execute("c1", { action: "begin", objective: "Generate a widget checklist" }); + const text = res.content[0].text; + assert.match(text, /automatic repair is ENABLED/); + assert.match(text, /skill_learner action "repair" with skill_name "widget-checklist"/); + }); + + it("prefers the interactive prompt over autoRepair when a human is present", async () => { + const dir = makeAgentDir(0.3); + const elicit = { interactive: true, select: async () => "s", edit: async () => null }; + const tool = createTaskTrackerTool(dir, dir, undefined, undefined, elicit, true); + const res = await tool.execute("c1", { action: "begin", objective: "Generate a widget checklist" }); + assert.match(res.content[0].text, /USER DECISION: skip the skill entirely/); + }); + + it("refuses skill_learner repair — without spending an LLM call — when nothing can approve it", async () => { + const dir = makeAgentDir(0.3); + const before = readFileSync(join(dir, "skills", "widget-checklist", "SKILL.md"), "utf-8"); + // A model object that would throw if the repair actually tried to use it: + // the refusal must short-circuit before any completion is requested. + const model = new Proxy({}, { get: () => { throw new Error("model must not be used"); } }) as any; + const tool = createSkillLearnerTool(dir, dir, model, undefined, undefined, false); + const res = await tool.execute("c1", { action: "repair", skill_name: "widget-checklist" }); + assert.match(res.content[0].text, /was NOT applied/); + assert.equal(res.details?.approved, false); + assert.equal(res.details?.reason, "no_approval_channel"); + assert.equal(readFileSync(join(dir, "skills", "widget-checklist", "SKILL.md"), "utf-8"), before); + }); + + it("does not gate a healthy skill match", async () => { + const dir = makeAgentDir(0.9); + let asked = false; + const elicit = { + interactive: true, + select: async () => { + asked = true; + return "p"; + }, + edit: async () => null, + }; + const tool = createTaskTrackerTool(dir, dir, undefined, undefined, elicit); + const res = await tool.execute("c1", { action: "begin", objective: "Generate a widget checklist" }); + assert.equal(asked, false); + assert.match(res.content[0].text, /YOU MUST USE IT/); + // sanity: the fixture is actually being read back + assert.ok(readFileSync(join(dir, "skills", "widget-checklist", "SKILL.md"), "utf-8").includes("confidence: 0.9")); + }); +}); diff --git a/test/learning.live.test.ts b/test/learning.live.test.ts new file mode 100644 index 0000000..df4be4c --- /dev/null +++ b/test/learning.live.test.ts @@ -0,0 +1,241 @@ +import { describe, it, before } from "node:test"; +import assert from "node:assert/strict"; +import { mkdtempSync, mkdirSync, writeFileSync, readFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +/** + * Live-API coverage for the parts of the learning layer that need a real model: + * the one-off completion helper, failure reflection, skill repair, and the full + * repair write path behind the approval gate. + * + * Skipped unless you opt in — `npm test` stays hermetic: + * + * GITAGENT_LIVE_TESTS=1 node --test test/learning.live.test.ts --experimental-strip-types + * + * Needs ANTHROPIC_API_KEY, or GITAGENT_MODEL=provider:model plus that key. + */ + +const LIVE = process.env.GITAGENT_LIVE_TESTS === "1"; +const MODEL_ID = process.env.GITAGENT_MODEL || "anthropic:claude-sonnet-4-6"; + +let runOneOffCompletion: typeof import("../dist/learning/llm-call.js").runOneOffCompletion; +let reflectOnFailure: typeof import("../dist/learning/reflection.js").reflectOnFailure; +let repairSkillSteps: typeof import("../dist/learning/skill-repair.js").repairSkillSteps; +let createSkillLearnerTool: typeof import("../dist/tools/skill-learner.js").createSkillLearnerTool; +let model: any; + +before(async () => { + if (!LIVE) return; + ({ runOneOffCompletion } = await import("../dist/learning/llm-call.js")); + ({ reflectOnFailure } = await import("../dist/learning/reflection.js")); + ({ repairSkillSteps } = await import("../dist/learning/skill-repair.js")); + ({ createSkillLearnerTool } = await import("../dist/tools/skill-learner.js")); + const { loadAgent } = await import("../dist/loader.js"); + + const dir = mkdtempSync(join(tmpdir(), "gitagent-live-agent-")); + writeFileSync(join(dir, "agent.yaml"), `spec_version: "0.1.0" +name: live-test-agent +version: 0.1.0 +description: Model holder for live learning tests +model: + preferred: "${MODEL_ID}" + fallback: [] +tools: [read] +runtime: + max_turns: 4 + timeout: 60 +`); + model = (await loadAgent(dir)).model; +}); + +// ── Fixtures ──────────────────────────────────────────────────────────── + +const FLAGGED_FM = `--- +name: flaky-checklist +description: Generate a laptop setup checklist covering OS updates, software installation and backup configuration, saved in the workspace directory. +confidence: 0.3 +usage_count: 7 +success_count: 2 +failure_count: 5 +negative_examples: + - Saved the checklist outside the workspace directory so the user could not find it + - Assumed macOS and emitted steps that are invalid on Windows +--- + +## Steps +1. Identify the workspace directory. +2. Write a checklist file covering OS updates, software and backups. +3. Tell the user it is done. +`; + +function flaggedAgentDir(): string { + const dir = mkdtempSync(join(tmpdir(), "gitagent-live-")); + mkdirSync(join(dir, "skills", "flaky-checklist"), { recursive: true }); + writeFileSync(join(dir, "skills", "flaky-checklist", "SKILL.md"), FLAGGED_FM); + return dir; +} + +const skillMd = (dir: string) => readFileSync(join(dir, "skills", "flaky-checklist", "SKILL.md"), "utf-8"); +const stepsOf = (md: string) => md.match(/## Steps\r?\n([\s\S]*?)(?=\r?\n## |\s*$)/)?.[1].trim() ?? ""; + +const acceptElicitor = () => ({ interactive: true, select: async () => "a", edit: async () => null }); + +// ── llm-call.ts ───────────────────────────────────────────────────────── + +describe("runOneOffCompletion (live)", { skip: !LIVE }, () => { + it("returns trimmed text and reports usage to onUsage", async () => { + const seen: any[] = []; + const out = await runOneOffCompletion( + model, + "Answer with a single word, no punctuation.", + "What is the capital of France?", + { maxTokens: 20 }, + (msg) => seen.push(msg), + ); + assert.equal(out, out.trim()); + assert.match(out, /Paris/i); + assert.equal(seen.length, 1); + assert.equal(seen[0].type, "assistant"); + assert.ok(seen[0].usage.outputTokens > 0, "output tokens should be reported"); + assert.equal(typeof seen[0].usage.costUsd, "number"); + }); + + it("throws instead of hanging when it times out", async () => { + await assert.rejects( + runOneOffCompletion(model, "You are terse.", "Write a 500 word essay about spline reticulation.", { timeoutMs: 1 }), + ); + }); +}); + +// ── reflection.ts ─────────────────────────────────────────────────────── + +describe("reflectOnFailure (live)", { skip: !LIVE }, () => { + it("returns one bounded single-line paragraph grounded in the steps", async () => { + const out = await reflectOnFailure(model, { + objective: "Save a laptop setup checklist for the user", + steps: [ + "Composed the checklist in memory", + "Wrote the file to /tmp instead of the workspace directory", + "Told the user the file was ready", + ], + failureReason: "user could not find the file", + }); + assert.ok(out.length > 0 && out.length <= 500, `length was ${out.length}`); + assert.doesNotMatch(out, /\n/, "must be a single paragraph"); + assert.doesNotMatch(out, /^(Root cause|Here is)/i, "no preamble"); + assert.doesNotMatch(out, /^[-*#]/, "no markdown bullets or headers"); + }); + + it("still reflects when no steps or reason were recorded", async () => { + const out = await reflectOnFailure(model, { objective: "Do something vague", steps: [] }); + assert.ok(out.length > 0 && out.length <= 500); + }); +}); + +// ── skill-repair.ts ───────────────────────────────────────────────────── + +describe("repairSkillSteps (live)", { skip: !LIVE }, () => { + it("returns a bare numbered list with no fences, within the char cap", async () => { + const out = await repairSkillSteps(model, { + skillDescription: "Generate a laptop setup checklist saved in the workspace directory", + currentSteps: "1. Identify the workspace directory.\n2. Write the checklist.\n3. Tell the user it is done.", + negativeExamples: [ + "Saved the checklist outside the workspace directory", + "Assumed macOS and emitted steps invalid on Windows", + ], + }); + assert.match(out, /^1\./, "should start at step 1"); + assert.doesNotMatch(out, /```/, "no code fences"); + assert.doesNotMatch(out, /^#/m, "no markdown headers"); + assert.ok(out.length <= 3000, `length was ${out.length}`); + assert.ok(out.split("\n").length >= 3, "should keep multiple steps"); + }); +}); + +// ── skill_learner repair: the full write path ─────────────────────────── + +describe("skill_learner repair (live)", { skip: !LIVE }, () => { + it("writes approved steps, resets stats, and records who approved", async () => { + const dir = flaggedAgentDir(); + const before = skillMd(dir); + const tool = createSkillLearnerTool(dir, dir, model, undefined, acceptElicitor(), false); + const res = await tool.execute("c", { action: "repair", skill_name: "flaky-checklist" }); + + assert.match(res.content[0].text, /repaired \(attempt 1\/3\) and committed/); + assert.equal((res.details as any).approved, true); + assert.equal((res.details as any).user_edited, false); + + const after = skillMd(dir); + assert.notEqual(after, before); + assert.match(after, /confidence: 0\.6/); + assert.match(after, /repair_count: 1/); + assert.match(after, /usage_count: 0/); + assert.match(after, /negative_examples: \[\]/); + assert.match(after, /## Repair History/); + assert.match(after, /Repair #1 on .* \(user-approved\)/); + // The recorded lessons are kept in the history even though the live array is cleared. + assert.match(after, /Saved the checklist outside the workspace directory/); + assert.ok(stepsOf(after).startsWith("1.")); + }); + + it("keeps the user's hand-edited steps and labels the history accordingly", async () => { + const dir = flaggedAgentDir(); + let edited = false; + const elicit = { + interactive: true, + // accept only after one edit round, mirroring the real re-preview loop + select: async () => (edited ? "a" : "e"), + edit: async (initial: string) => { edited = true; return `1. HAND-EDITED FIRST STEP.\n${initial}`; }, + }; + const tool = createSkillLearnerTool(dir, dir, model, undefined, elicit, false); + const res = await tool.execute("c", { action: "repair", skill_name: "flaky-checklist" }); + + assert.equal((res.details as any).user_edited, true); + assert.match(res.content[0].text, /The user edited and approved/); + const after = skillMd(dir); + assert.match(after, /HAND-EDITED FIRST STEP/); + assert.match(after, /\(user-edited, approved\)/); + }); + + it("leaves the file byte-identical when the user cancels", async () => { + const dir = flaggedAgentDir(); + const before = skillMd(dir); + const elicit = { interactive: true, select: async () => "c", edit: async () => null }; + const tool = createSkillLearnerTool(dir, dir, model, undefined, elicit, false); + const res = await tool.execute("c", { action: "repair", skill_name: "flaky-checklist" }); + + assert.match(res.content[0].text, /CANCELLED BY THE USER/); + assert.equal((res.details as any).cancelled, true); + assert.equal(skillMd(dir), before); + }); + + it("applies unattended under autoRepair and labels the history", async () => { + const dir = flaggedAgentDir(); + const tool = createSkillLearnerTool(dir, dir, model, undefined, undefined, true); + const res = await tool.execute("c", { action: "repair", skill_name: "flaky-checklist" }); + + assert.match(res.content[0].text, /repaired \(attempt 1\/3\)/); + assert.doesNotMatch(res.content[0].text, /The user/); + assert.match(skillMd(dir), /\(autoRepair\)/); + assert.match(skillMd(dir), /confidence: 0\.6/); + }); + + it("counts repairs across runs and stops at the third", async () => { + const dir = flaggedAgentDir(); + const tool = createSkillLearnerTool(dir, dir, model, undefined, undefined, true); + for (let i = 1; i <= 3; i++) { + const res = await tool.execute("c", { action: "repair", skill_name: "flaky-checklist" }); + assert.match(res.content[0].text, new RegExp(`attempt ${i}/3`)); + // A repair resets confidence to 0.6, so re-flag it to allow the next one. + writeFileSync( + join(dir, "skills", "flaky-checklist", "SKILL.md"), + skillMd(dir).replace(/confidence: 0\.6/, "confidence: 0.3"), + ); + } + await assert.rejects( + tool.execute("c", { action: "repair", skill_name: "flaky-checklist" }), + /already been repaired 3\/3 times/, + ); + }); +}); diff --git a/test/learning.test.ts b/test/learning.test.ts new file mode 100644 index 0000000..41902d1 --- /dev/null +++ b/test/learning.test.ts @@ -0,0 +1,557 @@ +import { describe, it, before } from "node:test"; +import assert from "node:assert/strict"; +import { mkdtempSync, mkdirSync, writeFileSync, readFileSync, existsSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +// Everything on this branch that runs without a model: the reinforcement math, +// task_tracker's full lifecycle, and skill_learner's actions and guard rails. +// The LLM-dependent parts (reflection text, repair rewriting) are covered by +// learning.live.test.ts, which needs an API key. + +let reinforcement: typeof import("../dist/learning/reinforcement.js"); +let createTaskTrackerTool: typeof import("../dist/tools/task-tracker.js").createTaskTrackerTool; +let createSkillLearnerTool: typeof import("../dist/tools/skill-learner.js").createSkillLearnerTool; + +before(async () => { + reinforcement = await import("../dist/learning/reinforcement.js"); + ({ createTaskTrackerTool } = await import("../dist/tools/task-tracker.js")); + ({ createSkillLearnerTool } = await import("../dist/tools/skill-learner.js")); +}); + +// ── Fixtures ──────────────────────────────────────────────────────────── + +const agentDir = () => mkdtempSync(join(tmpdir(), "gitagent-learning-")); + +function writeSkill(dir: string, name: string, frontmatter: Record, steps = "1. do the thing") { + const skillDir = join(dir, "skills", name); + mkdirSync(skillDir, { recursive: true }); + const fm = Object.entries(frontmatter) + .map(([k, v]) => (Array.isArray(v) + ? `${k}:\n${v.map((x) => ` - ${x}`).join("\n")}` + : `${k}: ${typeof v === "string" ? JSON.stringify(v) : v}`)) + .join("\n"); + writeFileSync(join(skillDir, "SKILL.md"), `---\n${fm}\n---\n\n## Steps\n${steps}\n`); + return skillDir; +} + +const skillFile = (dir: string, name: string) => join(dir, "skills", name, "SKILL.md"); +const text = (res: any) => res.content[0].text as string; + +/** A model that explodes on any use — proves a code path never reaches the LLM. */ +const explodingModel = () => + new Proxy({}, { get: () => { throw new Error("model must not be used"); } }) as any; + +const tracker = (dir: string, model?: any) => createTaskTrackerTool(dir, dir, model); +const learner = (dir: string, model?: any, elicit?: any, autoRepair?: boolean) => + createSkillLearnerTool(dir, dir, model, undefined, elicit, autoRepair); + +/** begin → N steps → end(outcome). Returns the task id. */ +async function runTask(dir: string, objective: string, steps: string[], outcome: string, extra: Record = {}) { + const t = tracker(dir); + const begun = await t.execute("c", { action: "begin", objective }); + const taskId = (begun.details as any).task_id as string; + for (const step of steps) await t.execute("c", { action: "update", task_id: taskId, step }); + await t.execute("c", { action: "end", task_id: taskId, outcome, ...extra }); + return taskId; +} + +// ── reinforcement.ts ──────────────────────────────────────────────────── + +describe("adjustConfidence", () => { + const base = { confidence: 0.5, usage_count: 3, success_count: 2, failure_count: 1, negative_examples: [] as string[] }; + + it("moves success asymptotically toward 1.0", () => { + const out = reinforcement.adjustConfidence(base, "success"); + assert.equal(out.confidence, 0.55); // 0.5 + 0.1 * (1 - 0.5) + assert.equal(out.success_count, 3); + assert.equal(out.usage_count, 4); + assert.equal(out.failure_count, 1); + }); + + it("never exceeds 1.0 on success", () => { + assert.equal(reinforcement.adjustConfidence({ ...base, confidence: 1 }, "success").confidence, 1); + }); + + it("penalises failure twice as hard as success rewards", () => { + const out = reinforcement.adjustConfidence(base, "failure", "wrote to the wrong path"); + assert.equal(out.confidence, 0.3); // 0.5 - 0.2 + assert.equal(out.failure_count, 2); + assert.deepEqual(out.negative_examples, ["wrote to the wrong path"]); + }); + + it("treats partial as a small penalty that still counts as a failure", () => { + const out = reinforcement.adjustConfidence(base, "partial", "half done"); + assert.equal(out.confidence, 0.45); + assert.equal(out.failure_count, 2); + assert.deepEqual(out.negative_examples, ["half done"]); + }); + + it("floors confidence at 0.0", () => { + assert.equal(reinforcement.adjustConfidence({ ...base, confidence: 0.1 }, "failure").confidence, 0); + }); + + it("records no lesson when no reason is given", () => { + assert.deepEqual(reinforcement.adjustConfidence(base, "failure").negative_examples, []); + }); + + it("caps negative_examples at 10, dropping the oldest", () => { + let stats = { ...base, negative_examples: [] as string[] }; + for (let i = 1; i <= 12; i++) stats = reinforcement.adjustConfidence(stats, "failure", `lesson ${i}`); + assert.equal(stats.negative_examples.length, 10); + assert.equal(stats.negative_examples[0], "lesson 3"); + assert.equal(stats.negative_examples.at(-1), "lesson 12"); + }); + + it("keeps confidence at two decimals (no float drift)", () => { + let stats = { ...base, confidence: 0.35 }; + for (let i = 0; i < 6; i++) stats = reinforcement.adjustConfidence(stats, "success"); + assert.equal(String(stats.confidence), String(Math.round(stats.confidence * 100) / 100)); + }); + + it("does not mutate the input stats", () => { + const input = { ...base, negative_examples: ["old"] }; + reinforcement.adjustConfidence(input, "failure", "new"); + assert.equal(input.usage_count, 3); + assert.deepEqual(input.negative_examples, ["old"]); + }); +}); + +describe("isSkillFlagged", () => { + it("flags below 0.4 only", () => { + const s = (confidence: number) => ({ confidence, usage_count: 0, success_count: 0, failure_count: 0, negative_examples: [] }); + assert.equal(reinforcement.isSkillFlagged(s(0.39)), true); + assert.equal(reinforcement.isSkillFlagged(s(0.4)), false); + assert.equal(reinforcement.isSkillFlagged(s(0)), true); + }); +}); + +describe("loadSkillStats / saveSkillStats", () => { + it("defaults to full confidence when there is no SKILL.md", async () => { + const stats = await reinforcement.loadSkillStats(join(agentDir(), "nope")); + assert.deepEqual(stats, { confidence: 1, usage_count: 0, success_count: 0, failure_count: 0, negative_examples: [] }); + }); + + it("fills in defaults for missing or malformed fields", async () => { + const dir = agentDir(); + const skillDir = writeSkill(dir, "partial", { name: "partial", confidence: 0.5, usage_count: "not-a-number" }); + const stats = await reinforcement.loadSkillStats(skillDir); + assert.equal(stats.confidence, 0.5); + assert.equal(stats.usage_count, 0); + assert.deepEqual(stats.negative_examples, []); + }); + + it("round-trips stats while preserving the skill body", async () => { + const dir = agentDir(); + const skillDir = writeSkill(dir, "rt", { name: "rt", confidence: 1 }, "1. keep me"); + await reinforcement.saveSkillStats(skillDir, { + confidence: 0.2, usage_count: 5, success_count: 1, failure_count: 4, negative_examples: ["a", "b"], + }); + const raw = readFileSync(join(skillDir, "SKILL.md"), "utf-8"); + assert.match(raw, /confidence: 0\.2/); + assert.match(raw, /1\. keep me/); + assert.deepEqual((await reinforcement.loadSkillStats(skillDir)).negative_examples, ["a", "b"]); + }); +}); + +// ── task_tracker ──────────────────────────────────────────────────────── + +describe("task_tracker begin", () => { + it("starts a task and reports no skill match", async () => { + const dir = agentDir(); + const res = await tracker(dir).execute("c", { action: "begin", objective: "Reticulate the splines" }); + assert.match(text(res), /Task started: /); + assert.match(text(res), /No matching skills found\. Solve from scratch\./); + assert.ok((res.details as any).task_id); + }); + + it("requires an objective", async () => { + await assert.rejects(tracker(agentDir()).execute("c", { action: "begin" }), /objective is required/); + }); + + it("resumes an active task with the same objective and bumps the attempt", async () => { + const dir = agentDir(); + const t = tracker(dir); + const first = await t.execute("c", { action: "begin", objective: "Same job" }); + const again = await t.execute("c", { action: "begin", objective: "Same job" }); + assert.match(text(again), /Resuming task/); + assert.match(text(again), /attempt #2/); + assert.equal((again.details as any).task_id, (first.details as any).task_id); + }); + + it("replays prior failure reasons on a fresh attempt", async () => { + const dir = agentDir(); + await runTask(dir, "Flaky job", ["step one"], "failure", { failure_reason: "picked the wrong directory" }); + const res = await tracker(dir).execute("c", { action: "begin", objective: "Flaky job" }); + assert.match(text(res), /Attempt #2/); + assert.match(text(res), /Prior failures:/); + assert.match(text(res), /picked the wrong directory/); + assert.match(text(res), /Avoid these approaches/); + }); + + it("orders a healthy skill match to be used", async () => { + const dir = agentDir(); + writeSkill(dir, "widget-checklist", { name: "widget-checklist", description: "Generate a widget checklist file", confidence: 0.9 }); + const res = await tracker(dir).execute("c", { action: "begin", objective: "Generate a widget checklist" }); + assert.match(text(res), /YOU MUST USE IT/); + assert.match(text(res), /Load skills\/widget-checklist\/SKILL\.md NOW/); + }); + + it("lists runner-up matches under the top one", async () => { + const dir = agentDir(); + writeSkill(dir, "widget-checklist", { name: "widget-checklist", description: "Generate a widget checklist file", confidence: 0.9 }); + writeSkill(dir, "widget-report", { name: "widget-report", description: "Generate a widget report file summary", confidence: 0.8 }); + const res = await tracker(dir).execute("c", { action: "begin", objective: "Generate a widget checklist file" }); + assert.match(text(res), /Other matching skills:/); + assert.match(text(res), /widget-report/); + }); +}); + +describe("task_tracker update", () => { + it("records numbered steps", async () => { + const dir = agentDir(); + const t = tracker(dir); + const begun = await t.execute("c", { action: "begin", objective: "Multi step job" }); + const id = (begun.details as any).task_id; + assert.match(text(await t.execute("c", { action: "update", task_id: id, step: "first" })), /Step 1 recorded/); + assert.match(text(await t.execute("c", { action: "update", task_id: id, step: "second" })), /Step 2 recorded/); + }); + + it("rejects unknown, inactive, and stepless updates", async () => { + const dir = agentDir(); + const t = tracker(dir); + await assert.rejects(t.execute("c", { action: "update", task_id: "nope", step: "x" }), /Task not found/); + const begun = await t.execute("c", { action: "begin", objective: "Short job" }); + const id = (begun.details as any).task_id; + await assert.rejects(t.execute("c", { action: "update", task_id: id }), /step is required/); + await t.execute("c", { action: "end", task_id: id, outcome: "success" }); + await assert.rejects(t.execute("c", { action: "update", task_id: id, step: "late" }), /is not active/); + }); +}); + +describe("task_tracker end", () => { + it("rewards the skill it used on success", async () => { + const dir = agentDir(); + writeSkill(dir, "widget-checklist", { name: "widget-checklist", description: "widget checklist", confidence: 0.5, usage_count: 1, success_count: 1, failure_count: 0 }); + const t = tracker(dir); + const begun = await t.execute("c", { action: "begin", objective: "Unrelated objective wording" }); + const id = (begun.details as any).task_id; + const res = await t.execute("c", { action: "end", task_id: id, outcome: "success", skill_used: "widget-checklist" }); + assert.match(text(res), /completed successfully/); + assert.match(text(res), /confidence: 0\.5 → 0\.55/); + assert.match(readFileSync(skillFile(dir, "widget-checklist"), "utf-8"), /confidence: 0\.55/); + assert.match(text(res), /skill_learner action "evaluate"/); + }); + + it("penalises the skill on failure and stores the lesson", async () => { + const dir = agentDir(); + writeSkill(dir, "widget-checklist", { name: "widget-checklist", description: "widget checklist", confidence: 0.5 }); + const t = tracker(dir); + const begun = await t.execute("c", { action: "begin", objective: "Unrelated objective wording" }); + const id = (begun.details as any).task_id; + const res = await t.execute("c", { + action: "end", task_id: id, outcome: "failure", + failure_reason: "saved the file outside the workspace", skill_used: "widget-checklist", + }); + assert.match(text(res), /confidence: 0\.5 → 0\.3/); + assert.match(text(res), /saved the file outside the workspace/); + assert.match(readFileSync(skillFile(dir, "widget-checklist"), "utf-8"), /saved the file outside the workspace/); + }); + + it("keeps the raw failure reason when reflection is unavailable (no model)", async () => { + const dir = agentDir(); + const t = tracker(dir); + const begun = await t.execute("c", { action: "begin", objective: "No model job" }); + const id = (begun.details as any).task_id; + const res = await t.execute("c", { action: "end", task_id: id, outcome: "failure", failure_reason: "raw reason" }); + assert.match(text(res), /Reason: raw reason/); + }); + + it("fails soft when reflection itself blows up, keeping the raw reason", async () => { + const dir = agentDir(); + // Tracks that reflection was really attempted — otherwise this test would + // pass just as happily if the reflection call were never made at all. + let touched = false; + const model = new Proxy({}, { + get: () => { touched = true; throw new Error("model exploded"); }, + }) as any; + const t = tracker(dir, model); + const begun = await t.execute("c", { action: "begin", objective: "Broken reflection job" }); + const id = (begun.details as any).task_id; + const res = await t.execute("c", { action: "end", task_id: id, outcome: "failure", failure_reason: "raw reason survives" }); + assert.equal(touched, true, "reflection should have been attempted"); + assert.match(text(res), /Reason: raw reason survives/); + assert.match(text(res), /Consider a different approach/); + }); + + it("reports a missing skill without failing the end call", async () => { + const dir = agentDir(); + const t = tracker(dir); + const begun = await t.execute("c", { action: "begin", objective: "Ghost skill job" }); + const id = (begun.details as any).task_id; + const res = await t.execute("c", { action: "end", task_id: id, outcome: "success", skill_used: "does-not-exist" }); + assert.match(text(res), /Could not update skill "does-not-exist" stats/); + }); + + it("validates its arguments", async () => { + const dir = agentDir(); + const t = tracker(dir); + await assert.rejects(t.execute("c", { action: "end", outcome: "success" }), /task_id is required/); + const begun = await t.execute("c", { action: "begin", objective: "Arg check job" }); + const id = (begun.details as any).task_id; + await assert.rejects(t.execute("c", { action: "end", task_id: id }), /outcome is required/); + await assert.rejects(t.execute("c", { action: "end", task_id: "nope", outcome: "success" }), /Task not found/); + }); +}); + +describe("task_tracker list", () => { + it("shows active tasks only", async () => { + const dir = agentDir(); + const t = tracker(dir); + assert.match(text(await t.execute("c", { action: "list" })), /No active tasks/); + await t.execute("c", { action: "begin", objective: "Still running" }); + await runTask(dir, "Already done", ["a"], "success"); + const res = await t.execute("c", { action: "list" }); + assert.match(text(res), /Still running/); + assert.doesNotMatch(text(res), /Already done/); + assert.equal((res.details as any).count, 1); + }); +}); + +// ── skill_learner ─────────────────────────────────────────────────────── + +describe("skill_learner evaluate", () => { + it("accepts a multi-step novel success", async () => { + const dir = agentDir(); + const id = await runTask(dir, "Assemble the quarterly widget report", ["one", "two", "three"], "success"); + const res = await learner(dir).execute("c", { action: "evaluate", task_id: id }); + assert.match(text(res), /Task IS worthy/); + assert.equal((res.details as any).worthy, true); + }); + + it("rejects a task that did not succeed", async () => { + const dir = agentDir(); + const id = await runTask(dir, "Failed job", ["one", "two", "three"], "failure", { failure_reason: "nope" }); + const res = await learner(dir).execute("c", { action: "evaluate", task_id: id }); + assert.match(text(res), /did not succeed/); + }); + + it("marks a task non-novel when an existing skill already covers it", async () => { + const dir = agentDir(); + writeSkill(dir, "quarterly-widget-report", { name: "quarterly-widget-report", description: "Assemble the quarterly widget report", confidence: 1 }); + const id = await runTask(dir, "Assemble the quarterly widget report", ["one", "two", "three"], "success"); + const res = await learner(dir).execute("c", { action: "evaluate", task_id: id }); + assert.equal((res.details as any).checks.novel, false); + }); + + it("honours override_heuristic for a thin task", async () => { + const dir = agentDir(); + const id = await runTask(dir, "Tiny job", ["only step"], "success"); + const plain = await learner(dir).execute("c", { action: "evaluate", task_id: id }); + assert.match(text(plain), /NOT worthy/); + const forced = await learner(dir).execute("c", { action: "evaluate", task_id: id, override_heuristic: true }); + assert.match(text(forced), /Task IS worthy/); + }); + + it("validates its arguments", async () => { + const dir = agentDir(); + await assert.rejects(learner(dir).execute("c", { action: "evaluate" }), /task_id is required/); + await assert.rejects(learner(dir).execute("c", { action: "evaluate", task_id: "nope" }), /Task not found/); + }); +}); + +describe("skill_learner crystallize", () => { + it("writes a SKILL.md with full confidence and the task's steps", async () => { + const dir = agentDir(); + const id = await runTask(dir, "Assemble the widget report", ["gather data", "render report", "verify output"], "success"); + const res = await learner(dir).execute("c", { + action: "crystallize", task_id: id, skill_name: "widget-report", skill_description: "Assemble a widget report", + }); + assert.match(text(res), /crystallized and committed/); + const md = readFileSync(skillFile(dir, "widget-report"), "utf-8"); + assert.match(md, /confidence: 1/); + assert.match(md, /usage_count: 0/); + assert.match(md, /learned_from: task:/); + assert.match(md, /1\. gather data/); + assert.match(md, /3\. verify output/); + assert.match(md, /## What Worked/); + }); + + it("carries prior failures into a What Did NOT Work section", async () => { + const dir = agentDir(); + await runTask(dir, "Repeat job", ["a"], "failure", { failure_reason: "used the wrong parser" }); + const id = await runTask(dir, "Repeat job", ["a", "b", "c"], "success"); + await learner(dir).execute("c", { + action: "crystallize", task_id: id, skill_name: "repeat-job", skill_description: "Do the repeat job", + }); + const md = readFileSync(skillFile(dir, "repeat-job"), "utf-8"); + assert.match(md, /## What Did NOT Work/); + assert.match(md, /used the wrong parser/); + }); + + it("refuses a failed task", async () => { + const dir = agentDir(); + const id = await runTask(dir, "Doomed job", ["a", "b", "c"], "failure", { failure_reason: "nope" }); + await assert.rejects( + learner(dir).execute("c", { action: "crystallize", task_id: id, skill_name: "doomed", skill_description: "d" }), + /Cannot crystallize failed task/, + ); + assert.equal(existsSync(skillFile(dir, "doomed")), false); + }); + + it("requires a kebab-case name and the other fields", async () => { + const dir = agentDir(); + const id = await runTask(dir, "Naming job", ["a", "b", "c"], "success"); + await assert.rejects( + learner(dir).execute("c", { action: "crystallize", task_id: id, skill_name: "Not Kebab", skill_description: "d" }), + /must be kebab-case/, + ); + await assert.rejects(learner(dir).execute("c", { action: "crystallize", task_id: id }), /skill_name is required/); + await assert.rejects( + learner(dir).execute("c", { action: "crystallize", task_id: id, skill_name: "fine-name" }), + /skill_description is required/, + ); + }); +}); + +describe("skill_learner status / review", () => { + it("marks flagged skills and counts repairs", async () => { + const dir = agentDir(); + writeSkill(dir, "good-skill", { name: "good-skill", description: "d", confidence: 0.8, usage_count: 4, success_count: 4, failure_count: 0 }); + writeSkill(dir, "bad-skill", { name: "bad-skill", description: "d", confidence: 0.2, usage_count: 6, success_count: 2, failure_count: 4, repair_count: 1 }); + const res = await learner(dir).execute("c", { action: "status" }); + const out = text(res); + assert.match(out, /good-skill: confidence=0\.8, usage=4, success_ratio=4\/4$/m); + assert.match(out, /bad-skill: .*repairs=1\/3 ⚠️ FLAGGED/); + assert.match(out, /1 skill\(s\) flagged as unreliable/); + }); + + it("reports an empty skills directory", async () => { + assert.match(text(await learner(agentDir()).execute("c", { action: "status" })), /No skills directory found/); + }); + + it("review lists only flagged skills, with their lessons", async () => { + const dir = agentDir(); + writeSkill(dir, "good-skill", { name: "good-skill", description: "d", confidence: 0.8 }); + writeSkill(dir, "bad-skill", { name: "bad-skill", description: "d", confidence: 0.2, negative_examples: ["wrong path", "wrong OS"] }); + const out = text(await learner(dir).execute("c", { action: "review" })); + assert.match(out, /bad-skill: confidence=0\.2/); + assert.doesNotMatch(out, /good-skill/); + assert.match(out, /wrong path; wrong OS/); + }); + + it("review says so when nothing is flagged", async () => { + const dir = agentDir(); + writeSkill(dir, "good-skill", { name: "good-skill", description: "d", confidence: 0.8 }); + assert.match(text(await learner(dir).execute("c", { action: "review" })), /No flagged skills/); + }); +}); + +describe("skill_learner repair guard rails", () => { + // Every case here must bail out before the model is touched. + it("requires a skill_name and a model", async () => { + const dir = agentDir(); + await assert.rejects(learner(dir).execute("c", { action: "repair" }), /skill_name is required/); + await assert.rejects( + learner(dir, undefined).execute("c", { action: "repair", skill_name: "whatever" }), + /Repair requires a model/, + ); + }); + + it("rejects an unknown skill and a malformed SKILL.md", async () => { + const dir = agentDir(); + await assert.rejects( + learner(dir, explodingModel()).execute("c", { action: "repair", skill_name: "ghost" }), + /Skill not found: ghost/, + ); + const badDir = join(dir, "skills", "no-frontmatter"); + mkdirSync(badDir, { recursive: true }); + writeFileSync(join(badDir, "SKILL.md"), "just a body, no frontmatter\n"); + await assert.rejects( + learner(dir, explodingModel()).execute("c", { action: "repair", skill_name: "no-frontmatter" }), + /Invalid SKILL\.md format/, + ); + }); + + it("refuses to repair a skill that is not flagged", async () => { + const dir = agentDir(); + writeSkill(dir, "healthy", { name: "healthy", description: "d", confidence: 0.8 }); + await assert.rejects( + learner(dir, explodingModel()).execute("c", { action: "repair", skill_name: "healthy" }), + /is not flagged \(confidence 0\.8 >= 0\.4\)/, + ); + }); + + it("stops after MAX_REPAIRS and points at update/delete", async () => { + const dir = agentDir(); + writeSkill(dir, "exhausted", { name: "exhausted", description: "d", confidence: 0.2, repair_count: 3 }); + await assert.rejects( + learner(dir, explodingModel()).execute("c", { action: "repair", skill_name: "exhausted" }), + /already been repaired 3\/3 times\. Use "update" or "delete"/, + ); + }); + + it("refuses when nothing can approve the repair, leaving the file untouched", async () => { + const dir = agentDir(); + writeSkill(dir, "flagged", { name: "flagged", description: "d", confidence: 0.2 }); + const before = readFileSync(skillFile(dir, "flagged"), "utf-8"); + const res = await learner(dir, explodingModel(), undefined, false).execute("c", { action: "repair", skill_name: "flagged" }); + assert.match(text(res), /was NOT applied/); + assert.equal((res.details as any).reason, "no_approval_channel"); + assert.equal(readFileSync(skillFile(dir, "flagged"), "utf-8"), before); + }); + + it("refuses the same way when a non-interactive elicitor is supplied", async () => { + const dir = agentDir(); + writeSkill(dir, "flagged", { name: "flagged", description: "d", confidence: 0.2 }); + const elicit = { interactive: false, select: async () => "a", edit: async () => null }; + const res = await learner(dir, explodingModel(), elicit, false).execute("c", { action: "repair", skill_name: "flagged" }); + assert.match(text(res), /was NOT applied/); + }); +}); + +describe("skill_learner update / delete", () => { + it("replaces the body and keeps the frontmatter", async () => { + const dir = agentDir(); + writeSkill(dir, "editable", { name: "editable", description: "keep me", confidence: 0.5 }, "1. old step"); + const res = await learner(dir).execute("c", { action: "update", skill_name: "editable", instructions: "## Steps\n1. brand new step" }); + assert.match(text(res), /updated and committed/); + const md = readFileSync(skillFile(dir, "editable"), "utf-8"); + assert.match(md, /description: keep me/); + assert.match(md, /confidence: 0\.5/); + assert.match(md, /1\. brand new step/); + assert.doesNotMatch(md, /old step/); + }); + + it("validates update arguments", async () => { + const dir = agentDir(); + await assert.rejects(learner(dir).execute("c", { action: "update", skill_name: "x" }), /instructions is required/); + await assert.rejects( + learner(dir).execute("c", { action: "update", skill_name: "ghost", instructions: "x" }), + /Skill not found: ghost/, + ); + }); + + it("deletes a skill directory and rejects an unknown one", async () => { + const dir = agentDir(); + writeSkill(dir, "doomed", { name: "doomed", description: "d", confidence: 0.5 }); + assert.match(text(await learner(dir).execute("c", { action: "delete", skill_name: "doomed" })), /deleted/); + assert.equal(existsSync(join(dir, "skills", "doomed")), false); + await assert.rejects(learner(dir).execute("c", { action: "delete", skill_name: "doomed" }), /Skill not found/); + }); +}); + +describe("both tools", () => { + it("reject unknown actions", async () => { + const dir = agentDir(); + await assert.rejects(tracker(dir).execute("c", { action: "nonsense" }), /Unknown action: nonsense/); + await assert.rejects(learner(dir).execute("c", { action: "nonsense" }), /Unknown action: nonsense/); + }); + + it("honour an already-aborted signal", async () => { + const dir = agentDir(); + const aborted = AbortSignal.abort(); + await assert.rejects(tracker(dir).execute("c", { action: "list" }, aborted), /Operation aborted/); + await assert.rejects(learner(dir).execute("c", { action: "status" }, aborted), /Operation aborted/); + }); +}); From 70f8d98fc8edf1efcb09dfd2bac68d42ae29580c Mon Sep 17 00:00:00 2001 From: Nivesh353 Date: Fri, 7 Aug 2026 11:21:48 +0530 Subject: [PATCH 3/4] fix(skills): crystallize must not overwrite an existing skill MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit crystallize wrote a fresh SKILL.md with confidence 1.0 and empty stats without checking whether the name was already taken. A live voice session hit this: blocked from repairing a flagged skill, it crystallized over the same name — resetting confidence 0.3 -> 1.0, erasing both recorded failure lessons, and replacing the steps with that task's step log. The result was a less useful skill that future runs trust completely, with no approval anywhere in the path. Refuse when skills//SKILL.md exists, report the existing skill's stats in the refusal, and point at "update", which preserves them. --- src/tools/skill-learner.ts | 20 ++++++++++++++++++++ test/learning.test.ts | 25 +++++++++++++++++++++++++ 2 files changed, 45 insertions(+) diff --git a/src/tools/skill-learner.ts b/src/tools/skill-learner.ts index eed76ae..21e35d4 100644 --- a/src/tools/skill-learner.ts +++ b/src/tools/skill-learner.ts @@ -1,4 +1,5 @@ import { readFile, writeFile, mkdir, readdir, rm } from "fs/promises"; +import { existsSync } from "fs"; import { join } from "path"; import { execSync } from "child_process"; import { type Static } from "@sinclair/typebox"; @@ -242,6 +243,25 @@ export function createSkillLearnerTool( throw new Error("skill_name must be kebab-case (e.g., deploy-staging)"); } + // Crystallize writes a fresh SKILL.md with confidence 1.0 and empty + // stats. Doing that over an existing skill would erase its whole track + // record — including the failures that flagged it — and swap its + // instructions for one task's step log, with none of the approval a + // "repair" needs. Refuse and point at the non-destructive action. + if (existsSync(join(agentDir, "skills", params.skill_name, "SKILL.md"))) { + const existing = await loadSkillStats(join(agentDir, "skills", params.skill_name)); + return { + content: [{ + type: "text", + text: `Skill "${params.skill_name}" already exists (confidence ${existing.confidence}, ` + + `${existing.success_count} success / ${existing.failure_count} failure). Crystallize does NOT overwrite — ` + + `that would erase its confidence, usage history, and recorded failures.\n\n` + + `Use action "update" to change its steps, or crystallize under a different skill_name.`, + }], + details: { skill_name: params.skill_name, created: false, reason: "already_exists" }, + }; + } + const store = await loadTasks(gitagentDir); const task = store.tasks.find((t) => t.id === params.task_id); if (!task) throw new Error(`Task not found: ${params.task_id}`); diff --git a/test/learning.test.ts b/test/learning.test.ts index 41902d1..958202a 100644 --- a/test/learning.test.ts +++ b/test/learning.test.ts @@ -389,6 +389,31 @@ describe("skill_learner crystallize", () => { assert.match(md, /used the wrong parser/); }); + it("refuses to overwrite an existing skill, preserving its record", async () => { + // Regression: a live voice session crystallized over a flagged skill, + // resetting confidence 0.3 → 1.0, wiping its recorded failures, and + // replacing its steps with the task's step log — all ungated. + const dir = agentDir(); + writeSkill(dir, "widget-report", { + name: "widget-report", description: "Assemble a widget report", + confidence: 0.3, usage_count: 7, success_count: 2, failure_count: 5, + negative_examples: ["used the wrong parser"], + }, "1. original instructions"); + const before = readFileSync(skillFile(dir, "widget-report"), "utf-8"); + + const id = await runTask(dir, "Assemble the widget report", ["gather", "render", "verify"], "success"); + const res = await learner(dir).execute("c", { + action: "crystallize", task_id: id, skill_name: "widget-report", skill_description: "Assemble a widget report", + }); + + assert.match(text(res), /already exists/); + assert.match(text(res), /does NOT overwrite/); + assert.match(text(res), /Use action "update"/); + assert.equal((res.details as any).created, false); + assert.equal((res.details as any).reason, "already_exists"); + assert.equal(readFileSync(skillFile(dir, "widget-report"), "utf-8"), before); + }); + it("refuses a failed task", async () => { const dir = agentDir(); const id = await runTask(dir, "Doomed job", ["a", "b", "c"], "failure", { failure_reason: "nope" }); From e93fc1632c4b70e3b35e4f497835fb9972eae358 Mon Sep 17 00:00:00 2001 From: Nivesh353 Date: Fri, 7 Aug 2026 11:51:45 +0530 Subject: [PATCH 4/4] fix(skills): validate skill_name on repair, update and delete Only crystallize checked the kebab-case pattern, so the other three actions accepted any string as a path segment. delete has no existence gate before rm(dir, { recursive: true }), so skill_name "../workspace" removed a directory outside the skills tree and committed it with git add -A. Verified against the pre-fix build. Also collapses repair's duplicate SKILL.md read into a single parse via statsFromFrontmatter(), and ignores an emptied $EDITOR buffer instead of writing a skill with no steps. --- src/elicit.ts | 6 ++++ src/learning/reinforcement.ts | 23 +++++++++---- src/tools/skill-learner.ts | 33 +++++++++++++----- test/learning.live.test.ts | 19 ++++++++++ test/learning.test.ts | 65 +++++++++++++++++++++++++++++++++++ 5 files changed, 131 insertions(+), 15 deletions(-) diff --git a/src/elicit.ts b/src/elicit.ts index d7c0280..0558628 100644 --- a/src/elicit.ts +++ b/src/elicit.ts @@ -141,6 +141,12 @@ export class ConsoleElicitor implements Elicitor { process.stdout.write(dim(" No changes saved.\n")); return null; } + // An empty buffer is a mistake, not an edit — callers would otherwise + // write out whatever "nothing" means for them. + if (edited.trim().length === 0) { + process.stdout.write(dim(" Saved file was empty — proposal left unchanged.\n")); + return null; + } return edited; } finally { try { diff --git a/src/learning/reinforcement.ts b/src/learning/reinforcement.ts index 49bab10..2a353c3 100644 --- a/src/learning/reinforcement.ts +++ b/src/learning/reinforcement.ts @@ -83,18 +83,27 @@ function serializeFrontmatter(frontmatter: Record, body: string): s return `---\n${yamlStr}\n---\n${body}`; } +/** + * Reads stats out of already-parsed frontmatter, filling defaults for missing or + * malformed fields. Lets callers that have parsed SKILL.md for other reasons get + * stats without a second read of the same file. + */ +export function statsFromFrontmatter(frontmatter: Record): SkillStats { + return { + confidence: typeof frontmatter.confidence === "number" ? frontmatter.confidence : DEFAULT_STATS.confidence, + usage_count: typeof frontmatter.usage_count === "number" ? frontmatter.usage_count : DEFAULT_STATS.usage_count, + success_count: typeof frontmatter.success_count === "number" ? frontmatter.success_count : DEFAULT_STATS.success_count, + failure_count: typeof frontmatter.failure_count === "number" ? frontmatter.failure_count : DEFAULT_STATS.failure_count, + negative_examples: Array.isArray(frontmatter.negative_examples) ? frontmatter.negative_examples : [], + }; +} + export async function loadSkillStats(skillDir: string): Promise { const skillFile = join(skillDir, "SKILL.md"); try { const content = await readFile(skillFile, "utf-8"); const { frontmatter } = parseFrontmatter(content); - return { - confidence: typeof frontmatter.confidence === "number" ? frontmatter.confidence : DEFAULT_STATS.confidence, - usage_count: typeof frontmatter.usage_count === "number" ? frontmatter.usage_count : DEFAULT_STATS.usage_count, - success_count: typeof frontmatter.success_count === "number" ? frontmatter.success_count : DEFAULT_STATS.success_count, - failure_count: typeof frontmatter.failure_count === "number" ? frontmatter.failure_count : DEFAULT_STATS.failure_count, - negative_examples: Array.isArray(frontmatter.negative_examples) ? frontmatter.negative_examples : [], - }; + return statsFromFrontmatter(frontmatter); } catch { return { ...DEFAULT_STATS }; } diff --git a/src/tools/skill-learner.ts b/src/tools/skill-learner.ts index 21e35d4..6a1c52a 100644 --- a/src/tools/skill-learner.ts +++ b/src/tools/skill-learner.ts @@ -7,7 +7,7 @@ import type { AgentTool } from "@mariozechner/pi-agent-core"; import type { Model } from "@mariozechner/pi-ai"; import type { GCAssistantMessage } from "../sdk-types.js"; import { skillLearnerSchema } from "./shared.js"; -import { loadSkillStats, isSkillFlagged } from "../learning/reinforcement.js"; +import { loadSkillStats, isSkillFlagged, statsFromFrontmatter } from "../learning/reinforcement.js"; import { repairSkillSteps } from "../learning/skill-repair.js"; import type { TaskRecord } from "./task-tracker.js"; import type { Elicitor } from "../elicit.js"; @@ -21,6 +21,20 @@ const MAX_REPAIRS = 3; // skill earns — a repaired skill has to re-earn trust, not start clean. const REPAIR_RESET_CONFIDENCE = 0.6; +// A skill name indexes straight into the filesystem — join(agentDir, "skills", +// name) — and "delete" removes that path recursively. Anything with a slash or +// a ".." reaches outside the skills tree, so every action that resolves a skill +// by name validates it, not just the one that creates skills. +const SKILL_NAME_PATTERN = /^[a-z0-9]+(-[a-z0-9]+)*$/; + +function assertValidSkillName(name: string): void { + if (!SKILL_NAME_PATTERN.test(name)) { + throw new Error( + `Invalid skill_name "${name}" — must be kebab-case (e.g., deploy-staging), with no path separators.`, + ); + } +} + // ── Helpers ───────────────────────────────────────────────────────────── interface TasksStore { @@ -238,10 +252,7 @@ export function createSkillLearnerTool( if (!params.skill_name) throw new Error("skill_name is required for crystallize action"); if (!params.skill_description) throw new Error("skill_description is required for crystallize action"); - // Validate kebab-case - if (!/^[a-z0-9]+(-[a-z0-9]+)*$/.test(params.skill_name)) { - throw new Error("skill_name must be kebab-case (e.g., deploy-staging)"); - } + assertValidSkillName(params.skill_name); // Crystallize writes a fresh SKILL.md with confidence 1.0 and empty // stats. Doing that over an existing skill would erase its whole track @@ -434,6 +445,7 @@ export function createSkillLearnerTool( case "repair": { if (!params.skill_name) throw new Error("skill_name is required for repair action"); + assertValidSkillName(params.skill_name); if (!model) throw new Error("Repair requires a model to be configured for this agent."); const skillFile = join(agentDir, "skills", params.skill_name, "SKILL.md"); @@ -449,8 +461,9 @@ export function createSkillLearnerTool( const frontmatter = yaml.load(fmMatch[1]) as Record; const body = fmMatch[2]; - const skillDir = join(agentDir, "skills", params.skill_name); - const stats = await loadSkillStats(skillDir); + // Stats come from the frontmatter just parsed above — no second read of + // the same file, so there's no window for it to change underneath us. + const stats = statsFromFrontmatter(frontmatter); if (!isSkillFlagged(stats)) { throw new Error( `Skill "${params.skill_name}" is not flagged (confidence ${stats.confidence} >= 0.4). Repair is only for flagged skills.`, @@ -527,7 +540,9 @@ export function createSkillLearnerTool( } const edited = await elicit.edit(finalSteps, { extension: ".md" }); - if (edited !== null) { + // An emptied buffer would otherwise write a skill with no steps at + // all. Treat it as "no usable edit" and re-show the proposal. + if (edited !== null && edited.trim().length > 0) { finalSteps = edited.trim(); userEdited = true; } @@ -582,6 +597,7 @@ export function createSkillLearnerTool( case "update": { if (!params.skill_name) throw new Error("skill_name is required for update action"); + assertValidSkillName(params.skill_name); if (!params.instructions) throw new Error("instructions is required for update action"); const skillFile = join(agentDir, "skills", params.skill_name, "SKILL.md"); @@ -610,6 +626,7 @@ export function createSkillLearnerTool( case "delete": { if (!params.skill_name) throw new Error("skill_name is required for delete action"); + assertValidSkillName(params.skill_name); const skillDir = join(agentDir, "skills", params.skill_name); try { diff --git a/test/learning.live.test.ts b/test/learning.live.test.ts index df4be4c..a3852bc 100644 --- a/test/learning.live.test.ts +++ b/test/learning.live.test.ts @@ -198,6 +198,25 @@ describe("skill_learner repair (live)", { skip: !LIVE }, () => { assert.match(after, /\(user-edited, approved\)/); }); + it("ignores an emptied editor buffer instead of writing a stepless skill", async () => { + const dir = flaggedAgentDir(); + let tried = false; + const elicit = { + interactive: true, + select: async () => (tried ? "a" : "e"), + // Simulates the user clearing the whole file and saving. + edit: async () => { tried = true; return " \n\n"; }, + }; + const tool = createSkillLearnerTool(dir, dir, model, undefined, elicit, false); + const res = await tool.execute("c", { action: "repair", skill_name: "flaky-checklist" }); + + assert.match(res.content[0].text, /repaired \(attempt 1\/3\)/); + assert.equal((res.details as any).user_edited, false, "an empty buffer is not an edit"); + const steps = stepsOf(skillMd(dir)); + assert.ok(steps.length > 0, "steps must not be empty"); + assert.ok(steps.startsWith("1."), `steps were: ${JSON.stringify(steps)}`); + }); + it("leaves the file byte-identical when the user cancels", async () => { const dir = flaggedAgentDir(); const before = skillMd(dir); diff --git a/test/learning.test.ts b/test/learning.test.ts index 958202a..b8ea030 100644 --- a/test/learning.test.ts +++ b/test/learning.test.ts @@ -126,6 +126,25 @@ describe("isSkillFlagged", () => { }); }); +describe("statsFromFrontmatter", () => { + it("matches loadSkillStats for the same file, without re-reading it", async () => { + const dir = agentDir(); + const skillDir = writeSkill(dir, "parity", { + name: "parity", confidence: 0.2, usage_count: 6, success_count: 2, failure_count: 4, + negative_examples: ["one", "two"], + }); + const raw = readFileSync(join(skillDir, "SKILL.md"), "utf-8"); + const frontmatter = (await import("js-yaml")).default.load(raw.match(/^---\r?\n([\s\S]*?)\r?\n---/)![1]) as any; + assert.deepEqual(reinforcement.statsFromFrontmatter(frontmatter), await reinforcement.loadSkillStats(skillDir)); + }); + + it("fills defaults for an empty frontmatter", () => { + assert.deepEqual(reinforcement.statsFromFrontmatter({}), { + confidence: 1, usage_count: 0, success_count: 0, failure_count: 0, negative_examples: [], + }); + }); +}); + describe("loadSkillStats / saveSkillStats", () => { it("defaults to full confidence when there is no SKILL.md", async () => { const stats = await reinforcement.loadSkillStats(join(agentDir(), "nope")); @@ -535,6 +554,52 @@ describe("skill_learner repair guard rails", () => { }); }); +describe("skill_learner skill_name validation", () => { + // Review follow-up: a skill name indexes into the filesystem, and "delete" + // removes that path recursively — so every action that resolves a skill by + // name must reject traversal, not just the one that creates skills. + const TRAVERSALS = ["../..", "../../etc", "skills/../../..", "/etc", "Not Kebab", "has_underscore"]; + + it("rejects traversing names on repair, update and delete", async () => { + const dir = agentDir(); + const l = learner(dir, explodingModel()); + for (const name of TRAVERSALS) { + await assert.rejects(l.execute("c", { action: "repair", skill_name: name }), /Invalid skill_name/, name); + await assert.rejects(l.execute("c", { action: "update", skill_name: name, instructions: "x" }), /Invalid skill_name/, name); + await assert.rejects(l.execute("c", { action: "delete", skill_name: name }), /Invalid skill_name/, name); + } + }); + + it("does not delete anything outside the skills directory", async () => { + const dir = agentDir(); + const victim = join(dir, "workspace"); + mkdirSync(victim, { recursive: true }); + writeFileSync(join(victim, "important.txt"), "do not delete me"); + + await assert.rejects( + learner(dir).execute("c", { action: "delete", skill_name: "../workspace" }), + /Invalid skill_name/, + ); + assert.equal(existsSync(join(victim, "important.txt")), true); + }); + + it("still rejects a bad name on crystallize", async () => { + const dir = agentDir(); + const id = await runTask(dir, "Naming job", ["a", "b", "c"], "success"); + await assert.rejects( + learner(dir).execute("c", { action: "crystallize", task_id: id, skill_name: "../escape", skill_description: "d" }), + /must be kebab-case/, + ); + }); + + it("accepts ordinary kebab-case names", async () => { + const dir = agentDir(); + writeSkill(dir, "deploy-staging-v2", { name: "deploy-staging-v2", description: "d", confidence: 0.5 }); + const res = await learner(dir).execute("c", { action: "update", skill_name: "deploy-staging-v2", instructions: "## Steps\n1. go" }); + assert.match(text(res), /updated and committed/); + }); +}); + describe("skill_learner update / delete", () => { it("replaces the body and keeps the frontmatter", async () => { const dir = agentDir();