diff --git a/package.json b/package.json index 8661cd3..41d3c94 100644 --- a/package.json +++ b/package.json @@ -22,6 +22,7 @@ "dist", "!dist/voice", "!dist/composio", + "spec", "README.md", "LICENSE" ], @@ -43,7 +44,7 @@ "build": "tsc", "dev": "tsc --watch", "start": "node dist/index.js", - "test": "node --test test/*.test.ts --experimental-strip-types" + "test": "node --experimental-strip-types --experimental-loader=./test/ts-resolve-hook.mjs --no-warnings --test test/*.test.ts" }, "engines": { "node": ">=20" diff --git a/spec/schemas/workflow.schema.json b/spec/schemas/workflow.schema.json new file mode 100644 index 0000000..1ab34ea --- /dev/null +++ b/spec/schemas/workflow.schema.json @@ -0,0 +1,69 @@ +{ + "$schema": "http://json-schema.org/draft-07/schema#", + "$id": "https://gitagent.dev/spec/workflow.schema.json", + "title": "Gitagent SkillFlow Workflow", + "description": "A SkillFlow workflow: a named sequence of steps, where each step invokes a skill with a prompt. Runtime semantics are defined by src/workflows.ts.", + "type": "object", + "additionalProperties": false, + "required": ["name", "description", "steps"], + "properties": { + "name": { + "type": "string", + "description": "Kebab-case identifier for the workflow. Used as the file name.", + "pattern": "^[a-z0-9]+(-[a-z0-9]+)*$" + }, + "description": { + "type": "string", + "description": "One-line description of what the workflow does.", + "minLength": 1 + }, + "steps": { + "type": "array", + "description": "Ordered list of steps. Steps execute top-to-bottom.", + "minItems": 1, + "items": { "$ref": "#/definitions/step" } + } + }, + "definitions": { + "step": { + "type": "object", + "additionalProperties": false, + "required": ["skill", "prompt"], + "properties": { + "id": { + "type": "string", + "description": "Optional snake_case identifier for the step. Used when other steps reference this one via depends_on.", + "pattern": "^[a-z0-9]+(_[a-z0-9]+)*$" + }, + "skill": { + "type": "string", + "description": "Name of an installed skill (kebab-case) the step will invoke. Must match an entry in the agent's skills/ directory, or 'approval' for a human-review step.", + "pattern": "^[a-z0-9]+(-[a-z0-9]+)*$" + }, + "prompt": { + "type": "string", + "description": "The natural-language instruction passed to the skill for this step.", + "minLength": 1 + }, + "channel": { + "type": "string", + "description": "Optional channel/destination for the step output (e.g. a Slack channel name).", + "minLength": 1 + }, + "depends_on": { + "type": "array", + "description": "Optional list of step ids that must complete before this step runs.", + "items": { + "type": "string", + "pattern": "^[a-z0-9]+(_[a-z0-9]+)*$" + }, + "uniqueItems": true + }, + "requires_approval": { + "type": "boolean", + "description": "If true, the workflow pauses for human approval before this step runs." + } + } + } + } +} diff --git a/src/commands/workflow.ts b/src/commands/workflow.ts new file mode 100644 index 0000000..006586d --- /dev/null +++ b/src/commands/workflow.ts @@ -0,0 +1,389 @@ +import { existsSync } from "fs"; +import { mkdir, readFile, writeFile } from "fs/promises"; +import { join, resolve, sep } from "path"; +import { SpanStatusCode, context, trace } from "@opentelemetry/api"; +import { discoverSkills } from "../skills.js"; +import { getTracer } from "../telemetry.js"; +import { loadDotEnvFiles, maybeInitTelemetry, readPreferredModel } from "../utils/bootstrap.js"; +import { parseUnsupportedReport, validateSkillReferences, validateWorkflow } from "../utils/schemas.js"; +import { checkSkillFitness, type FitnessWarning } from "../utils/skill-fitness.js"; +import { DEFAULT_MODEL, generateWorkflow, type LlmClient } from "../utils/workflow-generator.js"; + +interface GenerateFlags { + dir: string; + prompt?: string; + refine?: string; + model?: string; + apiKey?: string; + dryRun: boolean; + force?: boolean; + allowMissingSkills?: boolean; + /** Defaults to true; --no-fitness-check turns the advisory second pass off. */ + fitnessCheck?: boolean; +} + +const RED = (s: string) => `\x1b[31m${s}\x1b[0m`; +const YELLOW = (s: string) => `\x1b[33m${s}\x1b[0m`; +const GREEN = (s: string) => `\x1b[32m${s}\x1b[0m`; +const DIM = (s: string) => `\x1b[2m${s}\x1b[0m`; +const BOLD = (s: string) => `\x1b[1m${s}\x1b[0m`; + +const MAX_RETRIES = 2; + +function printHelp(): void { + console.log(`${BOLD("gitagent workflow")} — generate SkillFlow workflows from natural language + +Usage: + gitagent workflow generate [options] + +Options: + -d, --dir Agent directory (default: current directory) + -p, --prompt Natural-language description of the workflow (required) + --refine Refine an existing workflow YAML by applying --prompt as an instruction + (must be a path inside the agent directory) + -m, --model LLM model in provider:model form + (default: agent.yaml's model.preferred, else openai:gpt-4o) + --api-key API key for the provider (falls back to OPENAI_API_KEY or _API_KEY) + --dry-run Print the generated YAML to stdout instead of writing a file + -f, --force Overwrite workflows/.yaml if it already exists + (without this flag an existing file is never replaced) + --allow-missing-skills + Accept steps that reference skills which are not installed + (by default generation fails rather than writing a workflow + that cannot run) + --no-fitness-check Skip the advisory pass that warns when a step's chosen skill + looks unsuited to that step's task (saves one LLM call) + -h, --help Show this help message + +Examples: + gitagent workflow generate -p "every morning summarize unread emails and post to Slack" + gitagent workflow generate -p "add a human approval step before the Slack post" --refine workflows/morning-digest.yaml +`); +} + +// Reads the value that follows a flag, erroring out when the flag is the last +// token. Without this, argv[++i] yields undefined and the failure surfaces far +// from the parse site (e.g. resolve(undefined) throwing a bare TypeError). +function valueFor(argv: string[], i: number, flag: string): string { + const v = argv[i + 1]; + if (v === undefined) { + console.error(RED(`${flag} requires a value`)); + process.exit(2); + } + return v; +} + +function parseFlags(argv: string[]): GenerateFlags { + const flags: GenerateFlags = { dir: process.cwd(), dryRun: false }; + for (let i = 0; i < argv.length; i++) { + const a = argv[i]; + switch (a) { + case "-d": + case "--dir": + flags.dir = valueFor(argv, i++, a); + break; + case "-p": + case "--prompt": + flags.prompt = valueFor(argv, i++, a); + break; + case "--refine": + flags.refine = valueFor(argv, i++, a); + break; + case "-m": + case "--model": + flags.model = valueFor(argv, i++, a); + break; + case "--api-key": + flags.apiKey = valueFor(argv, i++, a); + break; + case "--dry-run": + flags.dryRun = true; + break; + case "-f": + case "--force": + flags.force = true; + break; + case "--allow-missing-skills": + flags.allowMissingSkills = true; + break; + case "--no-fitness-check": + flags.fitnessCheck = false; + break; + case "-h": + case "--help": + printHelp(); + process.exit(0); + break; + default: + if (!a.startsWith("-") && flags.prompt === undefined) { + flags.prompt = a; + } else { + console.error(RED(`Unknown option: ${a}`)); + process.exit(2); + } + } + } + return flags; +} + +function slugify(name: string): string { + const cleaned = name + .toLowerCase() + .trim() + .replace(/[^a-z0-9-]+/g, "-") + .replace(/^-+|-+$/g, "") + .replace(/-+/g, "-"); + return cleaned || "workflow"; +} + +export interface RunGenerateOptions { + flags: GenerateFlags; + llm?: LlmClient; + /** Client for the advisory fitness pass. Falls back to `llm`. */ + fitnessLlm?: LlmClient; +} + +export interface RunGenerateResult { + filePath?: string; + yaml: string; + fitnessWarnings: FitnessWarning[]; +} + +/** + * The model may answer with an `unsupported:` list instead of a workflow when + * nothing installed covers the request. Report it and stop — retrying would only + * push it back toward naming a real-but-unrelated skill, and there is no partial + * workflow worth writing. + */ +function reportUnsupported(items: string[], skillNames: string[]): void { + console.error(RED("\nNo installed skill covers part of this request:")); + for (const item of items) console.error(RED(` - ${item}`)); + console.error(DIM(`\nInstalled skills: ${skillNames.join(", ") || "(none)"}`)); + console.error( + DIM( + "Add a skill for each item above under skills/, or re-word the request in terms of the installed skills.", + ), + ); +} + +function reportFitnessWarnings(warnings: FitnessWarning[], steps: { skill?: string; prompt?: string }[]): void { + console.error(YELLOW(`\n${warnings.length} step(s) may use the wrong skill for the task:`)); + for (const w of warnings) { + const prompt = steps[w.stepIndex]?.prompt ?? ""; + console.error(YELLOW(` step ${w.stepIndex + 1} — skill "${w.skill}": ${w.reason}`)); + if (prompt) console.error(DIM(` step prompt: ${prompt}`)); + } + console.error( + DIM("\nThese are warnings, not errors — review the steps above, or pass --no-fitness-check to skip this pass."), + ); +} + +/** + * Model precedence: -m/--model, then agent.yaml's model.preferred, then the + * built-in default. Before this, the subcommand always used the built-in + * default, so an agent configured for one provider generated on another. + */ +async function resolveModel(flags: GenerateFlags, agentDir: string): Promise { + if (flags.model) return flags.model; + return await readPreferredModel(agentDir); +} + +export async function runGenerate(opts: RunGenerateOptions): Promise { + const { flags } = opts; + if (!flags.prompt || !flags.prompt.trim()) { + throw new Error("--prompt is required"); + } + + const agentDir = resolve(flags.dir); + + // main() does this for the interactive path, but the workflow subcommand + // returns before reaching it, so it has to happen here too: provider keys and + // OTEL_* vars from .env, then telemetry, so generation is cost-tracked. + loadDotEnvFiles(agentDir); + await maybeInitTelemetry(); + + const model = await resolveModel(flags, agentDir); + console.error(DIM(`Model: ${model ?? DEFAULT_MODEL}`)); + const skills = await discoverSkills(agentDir); + const skillNames = skills.map((s) => s.name); + + const span = getTracer().startSpan("gitagent.workflow.generate", { + attributes: { + "gitagent.workflow.model": model ?? "(default)", + "gitagent.workflow.skills_installed": skills.length, + "gitagent.workflow.refine": Boolean(flags.refine), + }, + }); + try { + // context.with, not a bare await: the per-call gen_ai.chat spans read the + // active context to find their parent. Without it each LLM call becomes its + // own root trace and the cost of one generation cannot be summed. + const result = await context.with(trace.setSpan(context.active(), span), () => + generateInner(opts, { + agentDir, + prompt: flags.prompt!.trim(), + model, + skills, + skillNames, + }), + ); + span.setAttributes({ + "gitagent.workflow.attempts": result.attempts, + "gitagent.workflow.fitness_warnings": result.fitnessWarnings.length, + "gitagent.workflow.written": Boolean(result.filePath), + }); + return result; + } catch (err: any) { + span.setStatus({ code: SpanStatusCode.ERROR, message: String(err?.message ?? err).slice(0, 200) }); + throw err; + } finally { + span.end(); + } +} + +interface GenerateContext { + agentDir: string; + /** Trimmed and validated in runGenerate. */ + prompt: string; + model?: string; + skills: Awaited>; + skillNames: string[]; +} + +async function generateInner( + opts: RunGenerateOptions, + ctx: GenerateContext, +): Promise { + const { flags } = opts; + const { agentDir, prompt, model, skills, skillNames } = ctx; + + let previousWorkflow: string | undefined; + if (flags.refine) { + // resolve() ignores the base when the second argument is absolute, so an + // absolute path or a ../ traversal would otherwise read (and ship to the + // LLM) any file on disk. Keep --refine inside the agent directory. + const refinePath = resolve(agentDir, flags.refine); + if (refinePath !== agentDir && !refinePath.startsWith(agentDir + sep)) { + throw new Error(`--refine path must be inside the agent directory: ${refinePath}`); + } + previousWorkflow = await readFile(refinePath, "utf-8"); + } + + let promptForLlm = prompt; + let lastErrors: string[] = []; + let yaml = ""; + let attempts = 0; + + for (let attempt = 0; attempt <= MAX_RETRIES; attempt++) { + console.error(DIM(attempt === 0 ? "Generating workflow..." : `Retry ${attempt}/${MAX_RETRIES} — fixing validation errors...`)); + attempts++; + yaml = await generateWorkflow({ + prompt: promptForLlm, + skills, + previousWorkflow, + model, + apiKey: flags.apiKey, + llm: opts.llm, + }); + // Checked before schema validation: an `unsupported:` document is not a + // workflow, so the schema would reject it as an unknown property and the + // retry would re-apply the very pressure the escape hatch exists to relieve. + const unsupported = parseUnsupportedReport(yaml); + if (unsupported) { + reportUnsupported(unsupported, skillNames); + throw new Error("No installed skill covers part of the request"); + } + + const result = validateWorkflow(yaml); + // A schema-valid workflow can still name skills that aren't installed, + // which would only surface as a run-time failure. Fold that into the same + // retry loop so the model gets a chance to pick real skills. + const skillErrors = result.valid && !flags.allowMissingSkills + ? validateSkillReferences(result.data!, skillNames) + : []; + if (result.valid && skillErrors.length === 0) { + lastErrors = []; + break; + } + lastErrors = [...result.errors, ...skillErrors]; + if (attempt < MAX_RETRIES) { + promptForLlm = + `${prompt}\n\nThe previous attempt was rejected — it failed schema validation or named skills that are not installed:\n` + + lastErrors.map((e) => `- ${e}`).join("\n") + + "\n\nReturn the full corrected workflow as YAML. Being correct matters more than passing the check: do NOT swap in a skill whose description does not cover that step's task just to satisfy the installed-skill list. If no installed skill covers part of the request, return the unsupported: form instead."; + } + } + + if (lastErrors.length > 0) { + console.error(RED("\nWorkflow validation failed after retries:")); + for (const e of lastErrors) console.error(RED(` - ${e}`)); + if (lastErrors.some((e) => e.includes("is not an installed skill"))) { + console.error(DIM(`\nInstalled skills: ${skillNames.join(", ") || "(none)"}`)); + console.error( + DIM( + "Create the missing skill(s) under skills/ first, or re-run with --allow-missing-skills to write the workflow anyway.", + ), + ); + } + console.error(DIM("\nLast generated YAML:\n")); + console.error(yaml); + throw new Error("Validation failed after retries"); + } + + const validated = validateWorkflow(yaml).data!; + + // Existence of a skill is now guaranteed; suitability is not. Warn (never fail) + // when a step's skill looks unrelated to what the step asks for, so a workflow + // that validates cleanly but would misfire at run time is not silently written. + let fitnessWarnings: FitnessWarning[] = []; + if (flags.fitnessCheck !== false) { + fitnessWarnings = await checkSkillFitness({ + workflow: validated, + skills, + model, + apiKey: flags.apiKey, + llm: opts.fitnessLlm ?? opts.llm, + }); + if (fitnessWarnings.length > 0) reportFitnessWarnings(fitnessWarnings, validated.steps ?? []); + } + + if (flags.dryRun) { + process.stdout.write(yaml.endsWith("\n") ? yaml : yaml + "\n"); + return { yaml, fitnessWarnings, attempts }; + } + + const slug = slugify(validated.name); + const workflowsDir = join(agentDir, "workflows"); + const filePath = join(workflowsDir, `${slug}.yaml`); + if (!flags.force && existsSync(filePath)) { + throw new Error( + `${filePath} already exists. Re-run with --force to overwrite it, or use --dry-run to preview the generated workflow.`, + ); + } + await mkdir(workflowsDir, { recursive: true }); + await writeFile(filePath, yaml.endsWith("\n") ? yaml : yaml + "\n", "utf-8"); + console.error(GREEN(`\nWrote workflow to ${filePath}`)); + return { filePath, yaml, fitnessWarnings, attempts }; +} + +export async function handleWorkflowCommand(argv: string[]): Promise { + // argv is the raw process.argv tail starting at the 'workflow' token. + // argv[0] === "workflow"; argv[1] is the sub-command. + const sub = argv[1]; + if (!sub || sub === "-h" || sub === "--help") { + printHelp(); + return; + } + if (sub !== "generate") { + console.error(RED(`Unknown subcommand: ${sub}`)); + printHelp(); + process.exit(2); + } + const flags = parseFlags(argv.slice(2)); + try { + await runGenerate({ flags }); + } catch (err: any) { + console.error(RED(`\nError: ${err?.message ?? String(err)}`)); + process.exit(1); + } +} diff --git a/src/index.ts b/src/index.ts index ba4eeb2..117ecfe 100644 --- a/src/index.ts +++ b/src/index.ts @@ -26,6 +26,8 @@ import type { LocalSession } from "./session.js"; // Imported dynamically below so the slim core has no static dependency on it — // users without voice get a clean install + a clear error if they try --voice. import { handlePluginCommand } from "./plugin-cli.js"; +import { handleWorkflowCommand } from "./commands/workflow.js"; +import { loadDotEnvFiles, maybeInitTelemetry } from "./utils/bootstrap.js"; import { context as otelContext } from "@opentelemetry/api"; import { initTelemetry, @@ -305,6 +307,12 @@ async function ensureRepo(dir: string, model?: string): Promise { } async function main(): Promise { + // Handle workflow subcommand: gitagent workflow + if (process.argv[2] === "workflow") { + await handleWorkflowCommand(process.argv.slice(2)); + return; + } + // Handle plugin subcommand: gitagent plugin if (process.argv[2] === "plugin") { const allArgs = process.argv.slice(3); @@ -379,31 +387,10 @@ async function main(): Promise { dir = resolve(dir); } - // Env precedence (lowest → highest): inherited env → ~/.gitagent/.env (global fallback) → agent-dir .env (winner). - // loadEnvPath is called in that order so the LAST source wins; agent .env still beats a shell placeholder. - const loadEnvPath = (envPath: string): void => { - if (!existsSync(envPath)) return; - const envContent = readFileSync(envPath, "utf-8"); - for (const rawLine of envContent.split("\n")) { - const line = rawLine.trim(); - if (!line || line.startsWith("#")) continue; - const eq = line.indexOf("="); - if (eq <= 0) continue; - const key = line.slice(0, eq).trim(); - let val = line.slice(eq + 1).trim(); - if ((val.startsWith('"') && val.endsWith('"')) || (val.startsWith("'") && val.endsWith("'"))) { - val = val.slice(1, -1); - } - process.env[key] = val; - } - }; - loadEnvPath(join(homedir(), ".gitagent", ".env")); - loadEnvPath(resolve(dir, ".env")); + loadDotEnvFiles(dir); // Auto-init telemetry after .env is loaded so OTEL_* vars set in .env are picked up. - if ((process.env.OTEL_EXPORTER_OTLP_ENDPOINT || process.env.OTEL_TRACES_EXPORTER === "console") && process.env.GITAGENT_OTEL_ENABLED !== "false") { - await initTelemetry({}); - } + await maybeInitTelemetry(); // Voice mode — dynamically load the @open-gitagent/voice package. // Core ships without voice so the published tarball stays slim and supply-chain diff --git a/src/telemetry.ts b/src/telemetry.ts index 10cf562..9eaa448 100644 --- a/src/telemetry.ts +++ b/src/telemetry.ts @@ -430,6 +430,14 @@ export function recordGenAiCall( const outputTokens = Number( msg.usage?.output ?? msg.usage?.outputTokens ?? 0, ); + // Prompt caching means `usage.input` counts only the uncached prefix, so a + // large system prompt shows up as cacheWrite/cacheRead instead. Reporting + // input_tokens alone made a 1500-token call look like 3 tokens while the + // cost attribute (which does include cache) disagreed. Emitted separately + // so existing input_tokens semantics are unchanged and the total is + // reconstructable. + const cacheReadTokens = Number(msg.usage?.cacheRead ?? 0); + const cacheWriteTokens = Number(msg.usage?.cacheWrite ?? 0); const cost = Number( msg.usage?.cost?.total ?? msg.usage?.cost ?? 0, ); @@ -443,6 +451,8 @@ export function recordGenAiCall( "gen_ai.response.finish_reasons": [String(finishReason)], "gen_ai.usage.input_tokens": inputTokens, "gen_ai.usage.output_tokens": outputTokens, + "gen_ai.usage.cache_read_input_tokens": Number.isFinite(cacheReadTokens) ? cacheReadTokens : 0, + "gen_ai.usage.cache_creation_input_tokens": Number.isFinite(cacheWriteTokens) ? cacheWriteTokens : 0, "gitagent.cost_usd": Number.isFinite(cost) ? cost : 0, }, }); diff --git a/src/utils/bootstrap.ts b/src/utils/bootstrap.ts new file mode 100644 index 0000000..231f320 --- /dev/null +++ b/src/utils/bootstrap.ts @@ -0,0 +1,77 @@ +// Start-up context derived from an agent directory: .env files, telemetry +// auto-init, and the manifest's preferred model. +// +// These three steps used to live inline in main(), which meant any command that +// returned before reaching them silently ran without them — that is how +// `gitagent workflow generate` ended up with no telemetry and no agent.yaml +// model. Keeping them here lets every entry point share one implementation. + +import { existsSync, readFileSync } from "fs"; +import { readFile } from "fs/promises"; +import { homedir } from "os"; +import { join, resolve } from "path"; +import yaml from "js-yaml"; +import { initTelemetry } from "../telemetry.js"; + +function loadEnvPath(envPath: string): void { + if (!existsSync(envPath)) return; + const envContent = readFileSync(envPath, "utf-8"); + for (const rawLine of envContent.split("\n")) { + const line = rawLine.trim(); + if (!line || line.startsWith("#")) continue; + const eq = line.indexOf("="); + if (eq <= 0) continue; + const key = line.slice(0, eq).trim(); + let val = line.slice(eq + 1).trim(); + if ((val.startsWith('"') && val.endsWith('"')) || (val.startsWith("'") && val.endsWith("'"))) { + val = val.slice(1, -1); + } + process.env[key] = val; + } +} + +/** + * Env precedence (lowest → highest): inherited env → ~/.gitagent/.env (global + * fallback) → agent-dir .env (winner). Sources are applied in that order so the + * last one wins; an agent's .env still beats a shell placeholder. + */ +export function loadDotEnvFiles(agentDir: string): void { + loadEnvPath(join(homedir(), ".gitagent", ".env")); + loadEnvPath(resolve(agentDir, ".env")); +} + +/** + * Initialize telemetry when the OTEL env vars ask for it. Must run after + * loadDotEnvFiles() so OTEL_* values set in a .env are picked up. + * Returns whether initialization was attempted. + */ +export async function maybeInitTelemetry(): Promise { + const wanted = + (process.env.OTEL_EXPORTER_OTLP_ENDPOINT || process.env.OTEL_TRACES_EXPORTER === "console") && + process.env.GITAGENT_OTEL_ENABLED !== "false"; + if (!wanted) return false; + await initTelemetry({}); + return true; +} + +/** + * Reads `model.preferred` from the agent's manifest. Returns undefined when + * there is no agent.yaml, it cannot be parsed, or the field is absent — a + * workflow can be generated in a bare directory, so none of those are errors. + */ +export async function readPreferredModel(agentDir: string): Promise { + let raw: string; + try { + raw = await readFile(join(agentDir, "agent.yaml"), "utf-8"); + } catch { + return undefined; + } + let parsed: unknown; + try { + parsed = yaml.load(raw); + } catch { + return undefined; + } + const preferred = (parsed as any)?.model?.preferred; + return typeof preferred === "string" && preferred.trim() ? preferred.trim() : undefined; +} diff --git a/src/utils/schemas.ts b/src/utils/schemas.ts new file mode 100644 index 0000000..9f9e670 --- /dev/null +++ b/src/utils/schemas.ts @@ -0,0 +1,333 @@ +import { readFileSync, existsSync } from "fs"; +import { dirname, join, resolve } from "path"; +import { fileURLToPath } from "url"; +import yaml from "js-yaml"; + +export interface WorkflowStep { + id?: string; + skill: string; + prompt: string; + channel?: string; + depends_on?: string[]; + requires_approval?: boolean; +} + +export interface WorkflowDef { + name: string; + description: string; + steps: WorkflowStep[]; +} + +export interface ValidationResult { + valid: boolean; + errors: string[]; + data?: WorkflowDef; +} + +let cachedSchema: any = null; +let cachedSchemaText: string | null = null; + +function resolveSchemaPath(): string { + const here = dirname(fileURLToPath(import.meta.url)); + // Try candidates relative to this module's location. + // 1. Running from src/utils/ in tests: ../../spec/schemas/workflow.schema.json + // 2. Running from dist/utils/ after build: ../../spec/schemas/workflow.schema.json + // 3. Running from dist/utils/ when spec/ is not packed: walk upward. + const candidates = [ + resolve(here, "..", "..", "spec", "schemas", "workflow.schema.json"), + resolve(here, "..", "..", "..", "spec", "schemas", "workflow.schema.json"), + ]; + for (const p of candidates) { + if (existsSync(p)) return p; + } + // Fallback: walk up to 6 levels looking for the schema. + let cur = here; + for (let i = 0; i < 6; i++) { + const guess = join(cur, "spec", "schemas", "workflow.schema.json"); + if (existsSync(guess)) return guess; + const parent = dirname(cur); + if (parent === cur) break; + cur = parent; + } + throw new Error(`Could not locate spec/schemas/workflow.schema.json relative to ${here}`); +} + +export function loadWorkflowSchema(): any { + if (cachedSchema) return cachedSchema; + const path = resolveSchemaPath(); + cachedSchemaText = readFileSync(path, "utf-8"); + cachedSchema = JSON.parse(cachedSchemaText); + return cachedSchema; +} + +export function getWorkflowSchemaText(): string { + if (cachedSchemaText) return cachedSchemaText; + loadWorkflowSchema(); + return cachedSchemaText!; +} + +function typeOf(v: any): string { + if (v === null) return "null"; + if (Array.isArray(v)) return "array"; + return typeof v; +} + +function matchesType(v: any, expected: string | string[]): boolean { + const types = Array.isArray(expected) ? expected : [expected]; + const actual = typeOf(v); + return types.includes(actual) || (types.includes("integer") && actual === "number" && Number.isInteger(v)); +} + +interface Issue { + path: string; + message: string; +} + +function validateAgainst(data: any, schema: any, path: string, root: any, issues: Issue[]): void { + // Resolve $ref + if (schema && typeof schema === "object" && schema.$ref) { + const ref = schema.$ref as string; + if (!ref.startsWith("#/")) { + issues.push({ path, message: `unsupported $ref "${ref}" (only local refs are supported)` }); + return; + } + const segments = ref.slice(2).split("/"); + let resolved: any = root; + for (const seg of segments) { + resolved = resolved?.[seg]; + } + if (!resolved) { + issues.push({ path, message: `cannot resolve $ref "${ref}"` }); + return; + } + validateAgainst(data, resolved, path, root, issues); + return; + } + + if (!schema || typeof schema !== "object") return; + + if (schema.type && !matchesType(data, schema.type)) { + issues.push({ + path, + message: `expected type ${Array.isArray(schema.type) ? schema.type.join("|") : schema.type}, got ${typeOf(data)}`, + }); + return; + } + + if (typeOf(data) === "object") { + const required: string[] = Array.isArray(schema.required) ? schema.required : []; + for (const key of required) { + if (!(key in data)) { + issues.push({ path: path || "(root)", message: `missing required property "${key}"` }); + } + } + + const props = schema.properties ?? {}; + const additionalAllowed = schema.additionalProperties !== false; + for (const key of Object.keys(data)) { + const childPath = path ? `${path}.${key}` : key; + if (props[key]) { + validateAgainst(data[key], props[key], childPath, root, issues); + } else if (!additionalAllowed) { + issues.push({ path: path || "(root)", message: `unknown property "${key}"` }); + } + } + } else if (typeOf(data) === "array") { + if (schema.minItems != null && data.length < schema.minItems) { + issues.push({ path: path || "(root)", message: `array must have at least ${schema.minItems} item(s), got ${data.length}` }); + } + if (schema.items) { + for (let i = 0; i < data.length; i++) { + validateAgainst(data[i], schema.items, `${path}[${i}]`, root, issues); + } + } + if (schema.uniqueItems === true) { + const seen = new Set(); + for (let i = 0; i < data.length; i++) { + const key = JSON.stringify(data[i]); + if (seen.has(key)) { + issues.push({ path: `${path}[${i}]`, message: `duplicate item` }); + } + seen.add(key); + } + } + } else if (typeOf(data) === "string") { + if (schema.minLength != null && data.length < schema.minLength) { + issues.push({ path: path || "(root)", message: `string must be at least ${schema.minLength} character(s)` }); + } + if (schema.pattern && !new RegExp(schema.pattern).test(data)) { + issues.push({ path: path || "(root)", message: `value "${data}" does not match pattern ${schema.pattern}` }); + } + } +} + +/** + * Depth-first search over the depends_on graph. Returns the offending path + * (e.g. ["a", "b", "a"]) for the first cycle found, or null when the graph is + * acyclic. Edges pointing at undeclared ids are ignored — those are already + * reported by the unknown-id check. + */ +function findDependencyCycle(steps: any[]): string[] | null { + const edges = new Map(); + for (const step of steps) { + if (!step || typeof step.id !== "string") continue; + const deps = Array.isArray(step.depends_on) ? step.depends_on.filter((d: any) => typeof d === "string") : []; + // Later duplicates of an id merge rather than overwrite, so no edge is lost. + edges.set(step.id, [...(edges.get(step.id) ?? []), ...deps]); + } + + const done = new Set(); + const stack: string[] = []; + const onStack = new Set(); + + function visit(id: string): string[] | null { + if (done.has(id)) return null; + if (onStack.has(id)) return [...stack.slice(stack.indexOf(id)), id]; + onStack.add(id); + stack.push(id); + for (const dep of edges.get(id) ?? []) { + if (!edges.has(dep)) continue; + const found = visit(dep); + if (found) return found; + } + stack.pop(); + onStack.delete(id); + done.add(id); + return null; + } + + for (const id of edges.keys()) { + const found = visit(id); + if (found) return found; + } + return null; +} + +/** + * Pseudo-skill for human-approval steps. It never exists under `skills/`, so it + * has to be exempt from the installed-skill check below. + */ +export const APPROVAL_SKILL = "approval"; + +/** + * Cross-check every step's `skill` against the skills actually installed in the + * agent directory. The JSON Schema can only check shape, so without this a + * plausible-looking workflow referencing skills that don't exist is written to + * disk and fails at run time instead of at generation time. + * + * Returns validator-shaped error strings so callers can fold them into the same + * retry loop as schema errors. An empty `installedSkills` list disables the + * check — that is the same escape hatch the generator's system prompt takes, + * where the model is told to invent sensible skill names. + */ +export function validateSkillReferences(workflow: WorkflowDef, installedSkills: string[]): string[] { + if (installedSkills.length === 0) return []; + const known = new Set([...installedSkills, APPROVAL_SKILL]); + const errors: string[] = []; + const steps = Array.isArray(workflow?.steps) ? workflow.steps : []; + steps.forEach((step: any, i: number) => { + const skill = typeof step?.skill === "string" ? step.skill.trim() : ""; + if (skill && !known.has(skill)) { + errors.push(`steps[${i}].skill: "${skill}" is not an installed skill`); + } + }); + return errors; +} + +/** + * Detects the "nothing installed covers this" response the generator's system + * prompt allows the model to return instead of a workflow: + * + * unsupported: + * - fetch the current weather forecast + * - send a text message + * + * Without this escape hatch the installed-skill check pushes the model into + * picking whichever real skill happens to validate, producing a workflow that + * passes every check and then does the wrong thing at run time. Callers must + * check this *before* validateWorkflow — an `unsupported` document is not a + * workflow, so the schema would reject it as an unknown property and the + * resulting retry would just re-apply that same pressure. + * + * Returns the non-empty trimmed items, or null when the document is not a + * decline (including `unsupported: []`, which says nothing and is left to fail + * schema validation normally). + */ +export function parseUnsupportedReport(yamlText: string): string[] | null { + let parsed: unknown; + try { + parsed = yaml.load(yamlText); + } catch { + return null; + } + if (!parsed || typeof parsed !== "object" || Array.isArray(parsed)) return null; + const raw = (parsed as any).unsupported; + if (raw === undefined || raw === null) return null; + const items = (Array.isArray(raw) ? raw : [raw]) + .map((v) => { + if (typeof v === "string") return v.trim(); + if (typeof v === "number" || typeof v === "boolean") return String(v); + return ""; + }) + .filter((v) => v.length > 0); + return items.length > 0 ? items : null; +} + +export function validateWorkflow(yamlText: string): ValidationResult { + let parsed: unknown; + try { + parsed = yaml.load(yamlText); + } catch (err: any) { + return { valid: false, errors: [`YAML parse error: ${err?.message ?? String(err)}`] }; + } + + if (parsed === null || parsed === undefined) { + return { valid: false, errors: ["workflow is empty"] }; + } + + if (typeof parsed !== "object" || Array.isArray(parsed)) { + return { valid: false, errors: [`workflow must be an object, got ${typeOf(parsed)}`] }; + } + + const schema = loadWorkflowSchema(); + const issues: Issue[] = []; + validateAgainst(parsed, schema, "", schema, issues); + + // Cross-field check: depends_on ids must reference a step declared earlier. + // The set grows as steps are visited rather than being pre-built, so a forward + // reference is rejected here instead of passing validation and failing later in + // loadFlowDefinition — after the file is already on disk and out of the retry + // loop. Steps execute in declaration order, so these are the same semantics. + const data = parsed as any; + if (Array.isArray(data.steps)) { + const available = new Set(); + data.steps.forEach((step: any, i: number) => { + if (step && Array.isArray(step.depends_on)) { + for (const dep of step.depends_on) { + if (typeof dep === "string" && !available.has(dep)) { + issues.push({ + path: `steps[${i}].depends_on`, + message: `references unknown step id "${dep}" (must be declared in a preceding step)`, + }); + } + } + } + if (step && typeof step.id === "string") available.add(step.id); + }); + + // Cross-field check: the depends_on graph must be acyclic. A self-reference + // or an A -> B -> A cycle would deadlock (or loop) at execution time. + const cycle = findDependencyCycle(data.steps); + if (cycle) { + issues.push({ path: "steps", message: `depends_on cycle detected: ${cycle.join(" -> ")}` }); + } + } + + if (issues.length === 0) { + return { valid: true, errors: [], data: parsed as WorkflowDef }; + } + return { + valid: false, + errors: issues.map((i) => (i.path ? `${i.path}: ${i.message}` : i.message)), + }; +} diff --git a/src/utils/skill-fitness.ts b/src/utils/skill-fitness.ts new file mode 100644 index 0000000..070ae4d --- /dev/null +++ b/src/utils/skill-fitness.ts @@ -0,0 +1,147 @@ +// Advisory second pass over a generated workflow, checking whether each step's +// chosen skill actually suits the step's task. +// +// The installed-skill check in schemas.ts answers "does this skill exist", which +// is a hard yes/no. This answers the fuzzier "is this skill right for the job" — +// a workflow can name only real skills and still wire a weather briefing into a +// company-announcements skill. That is deliberately reported as a warning rather +// than folded into the retry loop: the judgement is subjective, and a false +// positive must never block a workflow the user is happy with. + +import type { SkillMetadata } from "../skills.js"; +import { APPROVAL_SKILL, type WorkflowDef, type WorkflowStep } from "./schemas.js"; +import { + DEFAULT_MODEL, + defaultLlmClient, + type LlmClient, + type LlmMessage, +} from "./workflow-generator.js"; + +export interface FitnessWarning { + /** 0-based index into the workflow's steps array. */ + stepIndex: number; + skill: string; + reason: string; +} + +export interface CheckSkillFitnessOptions { + workflow: WorkflowDef; + skills: SkillMetadata[]; + model?: string; + apiKey?: string; + llm?: LlmClient; +} + +const FITNESS_SYSTEM = `You are reviewing an already-valid workflow for skill-selection mistakes. + +Every step below names a real, installed skill. Your only job is to spot steps where the chosen skill's description is clearly unrelated to what the step actually asks for — cases where the workflow will run without error but do the wrong thing. + +Be conservative. Flag a step ONLY when the mismatch is obvious from the description. If the skill could plausibly cover the task, or its description is broad or vague enough to include it, do not flag it. Returning nothing is the correct answer for most workflows. + +Never flag a step whose description is marked "never flag this one". + +Output format — return ONLY a JSON array, no prose and no markdown fences: +[{"step": 2, "reason": "one sentence naming what the skill is for and what the step actually needs"}] + +Return [] when every step is a reasonable fit.`; + +export function buildFitnessPrompt(workflow: WorkflowDef, skills: SkillMetadata[]): string { + const byName = new Map(skills.map((s) => [s.name, s])); + const steps = Array.isArray(workflow?.steps) ? workflow.steps : []; + const blocks = steps.map((step: any, i: number) => { + const name = typeof step?.skill === "string" ? step.skill : ""; + const prompt = typeof step?.prompt === "string" ? step.prompt : ""; + const meta = byName.get(name); + // Steps we cannot judge still occupy their index, so they are listed to keep + // the model's "step N" references aligned with the real steps array. + const description = + name === APPROVAL_SKILL + ? "(built-in human-approval step, not an installed skill — never flag this one)" + : meta + ? meta.description + : "(not installed — reported separately — never flag this one)"; + return `step ${i}:\n skill: ${name}\n skill is for: ${description}\n step asks for: ${prompt}`; + }); + return `Workflow: ${workflow?.name ?? "(unnamed)"} +Stated goal: ${workflow?.description ?? "(none)"} + +${blocks.join("\n\n")}`; +} + +// The response should be a bare JSON array, but models like to wrap it in prose +// or a fence. Grab the outermost bracketed span rather than anchoring. +const JSON_ARRAY_RE = /\[[\s\S]*\]/; + +/** + * Tolerant parse of the fitness response. Anything unexpected — prose, a fence, + * malformed JSON, out-of-range indices — degrades to "no warnings" rather than + * throwing, because this check is advisory and must not fail generation. + */ +export function parseFitnessResponse(raw: string, steps: WorkflowStep[]): FitnessWarning[] { + if (!raw || !raw.trim()) return []; + const match = raw.match(JSON_ARRAY_RE); + if (!match) return []; + + let parsed: unknown; + try { + parsed = JSON.parse(match[0]); + } catch { + return []; + } + if (!Array.isArray(parsed)) return []; + + const warnings: FitnessWarning[] = []; + const seen = new Set(); + for (const entry of parsed) { + if (!entry || typeof entry !== "object" || Array.isArray(entry)) continue; + const index = (entry as any).step; + const reason = (entry as any).reason; + if (typeof index !== "number" || !Number.isInteger(index)) continue; + // A hallucinated index would point at a step that does not exist, so the + // warning could not be rendered against anything. Drop it. + if (index < 0 || index >= steps.length) continue; + if (typeof reason !== "string" || !reason.trim()) continue; + if (seen.has(index)) continue; + seen.add(index); + const skill = steps[index] as any; + warnings.push({ + stepIndex: index, + skill: typeof skill?.skill === "string" ? skill.skill : "", + reason: reason.trim(), + }); + } + return warnings.sort((a, b) => a.stepIndex - b.stepIndex); +} + +export async function checkSkillFitness(opts: CheckSkillFitnessOptions): Promise { + const steps = Array.isArray(opts.workflow?.steps) ? opts.workflow.steps : []; + if (steps.length === 0 || opts.skills.length === 0) return []; + + // Only a step naming an installed skill can be judged — there is no + // description to compare against otherwise, and 'approval' is a pseudo-skill + // with no SKILL.md at all. If nothing is judgeable, skip the LLM call. + const installed = new Set(opts.skills.map((s) => s.name)); + const judgeable = steps.some( + (s: any) => typeof s?.skill === "string" && s.skill !== APPROVAL_SKILL && installed.has(s.skill), + ); + if (!judgeable) return []; + + const llm = opts.llm ?? defaultLlmClient; + const messages: LlmMessage[] = [ + { role: "system", content: FITNESS_SYSTEM }, + { role: "user", content: buildFitnessPrompt(opts.workflow, opts.skills) }, + ]; + + let raw: string; + try { + raw = await llm(messages, { + model: opts.model ?? DEFAULT_MODEL, + apiKey: opts.apiKey, + temperature: 0, + }); + } catch { + // A provider error here means we lose the warning, not the workflow. + return []; + } + return parseFitnessResponse(raw, steps); +} diff --git a/src/utils/workflow-generator.ts b/src/utils/workflow-generator.ts new file mode 100644 index 0000000..1e90f2f --- /dev/null +++ b/src/utils/workflow-generator.ts @@ -0,0 +1,218 @@ +import type { SkillMetadata } from "../skills.js"; +import { recordGenAiCall } from "../telemetry.js"; +import { getWorkflowSchemaText } from "./schemas.js"; + +export type LlmRole = "system" | "user" | "assistant"; + +export interface LlmMessage { + role: LlmRole; + content: string; +} + +export interface LlmCallOptions { + model: string; + temperature?: number; + apiKey?: string; +} + +export type LlmClient = (messages: LlmMessage[], opts: LlmCallOptions) => Promise; + +export interface GenerateWorkflowOptions { + prompt: string; + skills: SkillMetadata[]; + previousWorkflow?: string; + model?: string; + apiKey?: string; + llm?: LlmClient; +} + +export const DEFAULT_MODEL = "openai:gpt-4o"; + +const SYSTEM_RULES = `Rules (MUST follow): +- Output ONLY valid YAML. No markdown fences, no prose, no commentary before or after. +- The output MUST validate against the schema above. +- "name" must be kebab-case (lowercase letters, digits, and single hyphens). +- Every step "skill" must reference an installed skill from the list below, unless the step is human approval — in that case use skill: "approval" and set requires_approval: true. +- Never invent a skill name. Steps naming a skill that is not installed are rejected. +- Do NOT satisfy that check by substituting a skill whose description does not actually cover the step's task. A workflow that validates but invokes the wrong skill is worse than no workflow, because nothing catches it before it runs. +- When no installed skill plausibly covers part of the request, do not build a workflow at all. Instead output ONLY this shape, naming each uncovered capability in the user's own terms: + +unsupported: + - fetch the current weather forecast + - send a text message + + Treat that as a last resort: whenever the installed skills do map onto the request, build the workflow. +- When multiple steps need ordering beyond top-to-bottom, give them snake_case "id" values and use "depends_on" to express the dependency. +- Keep "prompt" fields concrete and self-contained — a downstream agent will read them verbatim. +- Do not invent fields not in the schema.`; + +const FEW_SHOT_USER_1 = "Every morning, summarize my unread emails and post the summary to Slack."; + +const FEW_SHOT_ASSISTANT_1 = `name: morning-email-digest +description: Summarize unread emails and post the digest to Slack each morning. +steps: + - skill: gmail + prompt: Fetch all unread emails from the last 24 hours and return subject, sender, and a one-sentence summary for each. + - skill: summarize + prompt: Compose a single-paragraph digest of the unread emails, grouped by sender priority. + - skill: slack + prompt: Post the digest to the configured channel. + channel: "#daily-digest" +`; + + +const FEW_SHOT_USER_2 = "Pull yesterday's sales data, get sign-off, then send the report to the team."; + +const FEW_SHOT_ASSISTANT_2 = `name: daily-sales-report +description: Pull sales data, require human approval, then distribute the report. +steps: + - id: pull_data + skill: analytics + prompt: Pull yesterday's sales totals broken down by region and product line. + - id: approve + skill: approval + prompt: Review the pulled sales data for accuracy and approve distribution. + requires_approval: true + depends_on: [pull_data] + - id: send_report + skill: email + prompt: Send the approved sales report to the sales-leadership distribution list. + depends_on: [approve] +`; + +function formatSkillsForPrompt(skills: SkillMetadata[]): string { + if (skills.length === 0) { + return "(no installed skills detected — use generic skill names that match the user's intent, e.g. gmail, slack, summarize)"; + } + return skills.map((s) => `- ${s.name}: ${s.description}`).join("\n"); +} + +export function buildSystemPrompt(skills: SkillMetadata[]): string { + const schemaText = getWorkflowSchemaText(); + const skillList = formatSkillsForPrompt(skills); + return `You are a workflow builder for GitClaw SkillFlow. + +Your job is to translate the user's natural-language description into a YAML workflow that conforms exactly to this JSON Schema: + + +${schemaText} + + +Installed skills the workflow may invoke: + +${skillList} + + +${SYSTEM_RULES}`; +} + +export function buildMessages(opts: GenerateWorkflowOptions): LlmMessage[] { + const messages: LlmMessage[] = [ + { role: "system", content: buildSystemPrompt(opts.skills) }, + { role: "user", content: FEW_SHOT_USER_1 }, + { role: "assistant", content: FEW_SHOT_ASSISTANT_1 }, + { role: "user", content: FEW_SHOT_USER_2 }, + { role: "assistant", content: FEW_SHOT_ASSISTANT_2 }, + ]; + + if (opts.previousWorkflow && opts.previousWorkflow.trim()) { + messages.push({ + role: "user", + content: `Here is the current workflow:\n\n${opts.previousWorkflow.trim()}\n\nApply this refinement: ${opts.prompt}\n\nReturn the complete updated workflow as YAML — not a diff.`, + }); + } else { + messages.push({ role: "user", content: opts.prompt }); + } + + return messages; +} + +// Pull the first fenced block out of the response regardless of any surrounding +// commentary ("Here is your workflow: ```yaml ... ``` Let me know if..."). An +// anchored match would leave the prose in place and burn a retry on a YAML +// parse error. Falls back to the raw trimmed text when there is no fence. +const FENCE_INNER_RE = /```(?:ya?ml)?[ \t]*\r?\n([\s\S]*?)\r?\n?```/i; + +export function stripCodeFences(raw: string): string { + const m = raw.match(FENCE_INNER_RE); + return m ? m[1].trim() : raw.trim(); +} + +export async function defaultLlmClient(messages: LlmMessage[], opts: LlmCallOptions): Promise { + const [providerRaw, ...modelParts] = opts.model.split(":"); + const provider = providerRaw?.trim(); + const modelId = modelParts.join(":").trim(); + if (!provider || !modelId) { + throw new Error(`Invalid model spec "${opts.model}". Expected "provider:model-id" (e.g. "openai:gpt-4o").`); + } + + const apiKey = + opts.apiKey || + process.env[`${provider.toUpperCase()}_API_KEY`] || + process.env.OPENAI_API_KEY; + if (!apiKey) { + throw new Error( + `No API key found. Pass --api-key or set ${provider.toUpperCase()}_API_KEY (or OPENAI_API_KEY) in your environment.`, + ); + } + + const [piAi, piAgentCore] = await Promise.all([ + import("@mariozechner/pi-ai") as Promise, + import("@mariozechner/pi-agent-core") as Promise, + ]); + const { getModel } = piAi; + const { Agent } = piAgentCore; + + if (!process.env[`${provider.toUpperCase()}_API_KEY`]) { + process.env[`${provider.toUpperCase()}_API_KEY`] = apiKey; + } + + const model = getModel(provider as any, modelId as any); + const systemMessage = messages.find((m) => m.role === "system")?.content ?? ""; + const conversation = messages.filter((m) => m.role !== "system"); + + const agent = new Agent({ + initialState: { + systemPrompt: systemMessage, + model, + tools: [], + temperature: opts.temperature ?? 0, + maxTokens: 4096, + }, + }); + + let collected = ""; + // Every assistant message is reported so token/cost lands in telemetry. This + // client backs workflow generation, its retries, and the fitness pass, so all + // three are accounted for. recordGenAiCall is a no-op when telemetry is off. + const startedAt = Date.now(); + agent.subscribe((event: any) => { + if (event.type === "message_end" && event.message?.role === "assistant") { + for (const block of event.message.content) { + if (block.type === "text") collected += block.text; + } + recordGenAiCall(event.message, { durationMs: Date.now() - startedAt }); + } + }); + + // Replay prior assistant/user turns as a single composed prompt so we don't + // have to drive Agent through multiple chat turns. The few-shot pairs are + // preserved as part of the prompt text so the model still sees them. + const composed = conversation + .map((m) => `[${m.role.toUpperCase()}]\n${m.content}`) + .join("\n\n"); + + await agent.prompt(composed); + return collected; +} + +export async function generateWorkflow(opts: GenerateWorkflowOptions): Promise { + if (!opts.prompt || !opts.prompt.trim()) { + throw new Error("generateWorkflow: prompt is required"); + } + const llm = opts.llm ?? defaultLlmClient; + const model = opts.model ?? DEFAULT_MODEL; + const messages = buildMessages(opts); + const raw = await llm(messages, { model, apiKey: opts.apiKey, temperature: 0 }); + return stripCodeFences(raw); +} diff --git a/src/workflows.ts b/src/workflows.ts index 03fa300..72cb182 100644 --- a/src/workflows.ts +++ b/src/workflows.ts @@ -4,9 +4,15 @@ import { mkdirSync } from "fs"; import yaml from "js-yaml"; export interface SkillFlowStep { + /** Optional snake_case id other steps reference via depends_on. */ + id?: string; skill: string; prompt: string; channel?: string; + /** Ids of steps that must complete first. Kept in sync with workflow.schema.json. */ + depends_on?: string[]; + /** Pause for human approval before running this step. */ + requires_approval?: boolean; } export interface SkillFlowDefinition { @@ -24,6 +30,24 @@ export interface WorkflowMetadata { steps?: SkillFlowStep[]; } +const KEBAB_RE = /^[a-z0-9]+(-[a-z0-9]+)*$/; + +/** + * Normalize raw YAML steps into SkillFlowStep. Every field declared in + * spec/schemas/workflow.schema.json is carried through — dropping `id` or + * `depends_on` here would silently ignore ordering the author declared. + */ +function normalizeSteps(rawSteps: any[]): SkillFlowStep[] { + return rawSteps.map((s: any) => ({ + ...(s?.id ? { id: String(s.id) } : {}), + skill: String(s?.skill || ""), + prompt: String(s?.prompt || ""), + ...(s?.channel ? { channel: String(s.channel) } : {}), + ...(Array.isArray(s?.depends_on) ? { depends_on: s.depends_on.map((d: any) => String(d)) } : {}), + ...(s?.requires_approval != null ? { requires_approval: Boolean(s.requires_approval) } : {}), + })); +} + function parseFrontmatter(content: string): { frontmatter: Record; body: string } { const match = content.match(/^---\r?\n([\s\S]*?)\r?\n---\r?\n?([\s\S]*)$/); if (!match) { @@ -64,11 +88,7 @@ export async function discoverWorkflows(agentDir: string): Promise ({ - skill: String(s.skill || ""), - prompt: String(s.prompt || ""), - ...(s.channel ? { channel: String(s.channel) } : {}), - })), + steps: normalizeSteps(data.steps as any[]), } : { type: "basic" as const }), }); } @@ -98,22 +118,82 @@ export async function discoverWorkflows(agentDir: string): Promise a.name.localeCompare(b.name)); } -const KEBAB_RE = /^[a-z0-9]+(-[a-z0-9]+)*$/; - -export async function loadFlowDefinition(filePath: string): Promise { +let warnedLegacyFlowPath = false; + +/** + * Load a flow by name from `/workflows/.yaml`. + * + * The path is built here rather than accepted from the caller so the function + * owns its own safety: `flowName` must be kebab-case, which rules out traversal + * segments and absolute paths no matter how the name reached this code. + */ +export async function loadFlowDefinition(agentDir: string, flowName: string): Promise; +/** + * Legacy single-argument form, kept so callers written against the previous + * signature (e.g. `@open-gitagent/voice`) keep working. It reads the path as + * given and therefore provides no containment — migrate to + * `loadFlowDefinition(agentDir, flowName)`. + * + * @deprecated Pass `(agentDir, flowName)` instead. + */ +export async function loadFlowDefinition(filePath: string): Promise; +export async function loadFlowDefinition(agentDirOrPath: string, flowName?: string): Promise { + // Which form was used is decided by arity, not by inspecting the string: a + // missing second argument would otherwise stringify to "undefined", which + // passes KEBAB_RE and fails later as a confusing ENOENT. + let filePath: string; + if (flowName === undefined) { + if (!warnedLegacyFlowPath) { + warnedLegacyFlowPath = true; + console.warn( + "[gitagent] loadFlowDefinition(filePath) is deprecated — call loadFlowDefinition(agentDir, flowName) so the path is validated and built internally.", + ); + } + filePath = agentDirOrPath; + } else { + if (!KEBAB_RE.test(flowName)) { + throw new Error(`Invalid flow name "${flowName}": must be kebab-case (e.g. my-flow-name)`); + } + filePath = join(agentDirOrPath, "workflows", `${flowName}.yaml`); + } const raw = await readFile(filePath, "utf-8"); - const data = yaml.load(raw) as Record; - if (!data?.name || !data?.steps || !Array.isArray(data.steps)) { + const data = yaml.load(raw); + + // yaml.load returns null for an empty file and a string/number for a + // single scalar document — both need a clearer error than "missing name". + if (data === null || typeof data !== "object" || Array.isArray(data)) { + throw new Error("Invalid flow definition: file must be a YAML mapping"); + } + const doc = data as Record; + if (!doc.name || !Array.isArray(doc.steps)) { throw new Error("Invalid flow definition: missing name or steps"); } + + const steps = normalizeSteps(doc.steps); + const badStep = steps.findIndex((s) => !s.skill.trim()); + if (badStep !== -1) { + throw new Error(`Invalid flow definition: step[${badStep}] has an empty skill`); + } + + // Steps execute in declaration order, so every depends_on must name a + // preceding step. This also rejects self-references and cycles, and keeps + // declared dependencies from being silently ignored at run time. + const available = new Set(); + steps.forEach((s, i) => { + for (const dep of s.depends_on ?? []) { + if (!available.has(dep)) { + throw new Error( + `Invalid flow definition: step[${i}] depends_on "${dep}", which is not the id of a preceding step`, + ); + } + } + if (s.id) available.add(s.id); + }); + return { - name: data.name, - description: data.description || "", - steps: data.steps.map((s: any) => ({ - skill: String(s.skill || ""), - prompt: String(s.prompt || ""), - ...(s.channel ? { channel: String(s.channel) } : {}), - })), + name: String(doc.name), + description: String(doc.description || ""), + steps, }; } @@ -130,7 +210,14 @@ export async function saveFlowDefinition(agentDir: string, flow: SkillFlowDefini const content = yaml.dump({ name: flow.name, description: flow.description || "", - steps: flow.steps.map((s) => ({ skill: s.skill, prompt: s.prompt, ...(s.channel ? { channel: s.channel } : {}) })), + steps: flow.steps.map((s) => ({ + ...(s.id ? { id: s.id } : {}), + skill: s.skill, + prompt: s.prompt, + ...(s.channel ? { channel: s.channel } : {}), + ...(s.depends_on?.length ? { depends_on: s.depends_on } : {}), + ...(s.requires_approval != null ? { requires_approval: s.requires_approval } : {}), + })), }, { lineWidth: 120 }); await writeFile(filePath, content, "utf-8"); return filePath; diff --git a/test/ts-resolve-hook.mjs b/test/ts-resolve-hook.mjs new file mode 100644 index 0000000..3fa4398 --- /dev/null +++ b/test/ts-resolve-hook.mjs @@ -0,0 +1,27 @@ +// ESM resolve hook: if a `.js` import inside src/ has no matching `.js` +// on disk, retry with the `.ts` extension. Lets node --experimental-strip-types +// run TypeScript sources whose internal imports follow the Node16 `.js` style. + +import { existsSync } from "node:fs"; +import { fileURLToPath } from "node:url"; + +export async function resolve(specifier, context, nextResolve) { + if (specifier.startsWith(".") && specifier.endsWith(".js")) { + try { + return await nextResolve(specifier, context); + } catch (err) { + if (err?.code !== "ERR_MODULE_NOT_FOUND") throw err; + const tsSpecifier = specifier.slice(0, -3) + ".ts"; + try { + const candidate = await nextResolve(tsSpecifier, context); + if (candidate?.url?.startsWith("file://") && existsSync(fileURLToPath(candidate.url))) { + return candidate; + } + } catch { + // fall through and re-throw the original .js miss + } + throw err; + } + } + return nextResolve(specifier, context); +} diff --git a/test/workflow-generator.test.ts b/test/workflow-generator.test.ts new file mode 100644 index 0000000..d05f677 --- /dev/null +++ b/test/workflow-generator.test.ts @@ -0,0 +1,755 @@ +// Unit tests for src/utils/workflow-generator.ts and the retry loop in +// src/commands/workflow.ts. The LLM is fully mocked — no network calls. + +import test from "node:test"; +import assert from "node:assert/strict"; +import { mkdtemp, readFile, rm } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { + buildMessages, + buildSystemPrompt, + stripCodeFences, + generateWorkflow, + DEFAULT_MODEL, + type LlmClient, + type LlmMessage, +} from "../src/utils/workflow-generator.ts"; +import { readPreferredModel } from "../src/utils/bootstrap.ts"; +import { runGenerate } from "../src/commands/workflow.ts"; +import { parseUnsupportedReport } from "../src/utils/schemas.ts"; +import { + buildFitnessPrompt, + checkSkillFitness, + parseFitnessResponse, +} from "../src/utils/skill-fitness.ts"; +import type { SkillMetadata } from "../src/skills.ts"; + +const SKILLS: SkillMetadata[] = [ + { name: "gmail", description: "Read and send email", directory: "/x/skills/gmail", filePath: "/x/skills/gmail/SKILL.md" }, + { name: "slack", description: "Post to Slack", directory: "/x/skills/slack", filePath: "/x/skills/slack/SKILL.md" }, + { name: "summarize", description: "Summarize text", directory: "/x/skills/summarize", filePath: "/x/skills/summarize/SKILL.md" }, +]; + +const VALID_YAML = `name: morning-digest +description: Summarize unread emails and post to Slack each morning. +steps: + - skill: gmail + prompt: Fetch unread emails. + - skill: summarize + prompt: Compose a digest. + - skill: slack + prompt: Post the digest. + channel: "#daily-digest" +`; + +const INVALID_YAML_MISSING_NAME = `description: no name here +steps: + - skill: gmail + prompt: hi +`; + +// ── buildSystemPrompt / buildMessages ────────────────────────────────── + +test("buildSystemPrompt embeds the schema text and the skill list", () => { + const sys = buildSystemPrompt(SKILLS); + assert.ok(sys.includes(""), "system prompt missing tag"); + assert.ok(sys.includes('"$id": "https://gitagent.dev/spec/workflow.schema.json"'), "system prompt missing schema $id"); + assert.ok(sys.includes("- gmail: Read and send email"), "system prompt missing gmail skill"); + assert.ok(sys.includes("- slack: Post to Slack"), "system prompt missing slack skill"); + assert.ok(sys.includes("Output ONLY valid YAML"), "system prompt missing rule about raw YAML"); +}); + +test("buildSystemPrompt handles empty skill list with a fallback hint", () => { + const sys = buildSystemPrompt([]); + assert.ok(sys.includes("no installed skills detected"), "system prompt missing empty-skills fallback"); +}); + +test("buildMessages includes two few-shot pairs and the user prompt", () => { + const messages = buildMessages({ prompt: "Do the thing", skills: SKILLS }); + assert.equal(messages[0].role, "system"); + assert.equal(messages[1].role, "user"); + assert.equal(messages[2].role, "assistant"); + assert.equal(messages[3].role, "user"); + assert.equal(messages[4].role, "assistant"); + assert.equal(messages[5].role, "user"); + assert.equal(messages[5].content, "Do the thing"); +}); + +test("buildMessages wraps refine-mode prompts with the previous YAML and instruction", () => { + const messages = buildMessages({ + prompt: "Add an approval step before the Slack post.", + skills: SKILLS, + previousWorkflow: VALID_YAML, + }); + const last = messages[messages.length - 1]; + assert.equal(last.role, "user"); + assert.ok(last.content.includes("Here is the current workflow")); + assert.ok(last.content.includes("Add an approval step before the Slack post.")); + assert.ok(last.content.includes("morning-digest")); + assert.ok(last.content.includes("Return the complete updated workflow as YAML — not a diff.")); +}); + +// ── stripCodeFences ──────────────────────────────────────────────────── + +test("stripCodeFences removes generic fenced code", () => { + const input = "```\nname: foo\n```\n"; + assert.equal(stripCodeFences(input), "name: foo"); +}); + +test("stripCodeFences removes yaml-tagged fences", () => { + const input = "```yaml\nname: foo\n```"; + assert.equal(stripCodeFences(input), "name: foo"); +}); + +test("stripCodeFences leaves unfenced YAML alone", () => { + const input = "name: foo\n"; + assert.equal(stripCodeFences(input), "name: foo"); +}); + +test("stripCodeFences extracts the block when the LLM wraps it in prose", () => { + const input = [ + "Here is your workflow:", + "", + "```yaml", + VALID_YAML.trimEnd(), + "```", + "", + "Let me know if you want an approval step added!", + ].join("\n"); + const out = stripCodeFences(input); + assert.equal(out.startsWith("name: morning-digest"), true, out); + assert.equal(out.includes("Here is your workflow"), false, out); + assert.equal(out.includes("Let me know"), false, out); + assert.equal(out.includes("```"), false, out); +}); + +// ── generateWorkflow with an injected LLM ────────────────────────────── + +test("generateWorkflow returns the LLM output after stripping fences", async () => { + const captured: { messages: LlmMessage[] } = { messages: [] }; + const llm: LlmClient = async (messages) => { + captured.messages = messages; + return "```yaml\n" + VALID_YAML + "```"; + }; + const out = await generateWorkflow({ + prompt: "summarize emails and post to slack", + skills: SKILLS, + llm, + }); + assert.equal(out.trim().startsWith("name: morning-digest"), true); + assert.equal(captured.messages.length, 6); + assert.equal(captured.messages[0].role, "system"); +}); + +test("generateWorkflow throws if prompt is empty", async () => { + await assert.rejects( + () => generateWorkflow({ prompt: " ", skills: SKILLS, llm: async () => VALID_YAML }), + /prompt is required/, + ); +}); + +// ── Retry loop in runGenerate ────────────────────────────────────────── + +test("runGenerate retries when validation fails, then writes the file when the second attempt is valid", async () => { + const dir = await mkdtemp(join(tmpdir(), "gitagent-test-")); + try { + let calls = 0; + const llm: LlmClient = async (messages) => { + calls++; + const userMsg = messages[messages.length - 1].content; + if (calls === 1) return INVALID_YAML_MISSING_NAME; + // Second attempt: ensure the retry prompt included the validation error. + assert.ok(userMsg.includes("schema validation"), `retry user message did not mention validation: ${userMsg}`); + return VALID_YAML; + }; + const result = await runGenerate({ + flags: { + dir, + prompt: "summarize unread emails and post to Slack", + dryRun: false, + }, + llm, + }); + assert.equal(calls, 2); + assert.ok(result.filePath, "expected a written file path"); + assert.equal(result.filePath!.endsWith(join("workflows", "morning-digest.yaml")), true, result.filePath); + const written = await readFile(result.filePath!, "utf-8"); + assert.ok(written.includes("name: morning-digest")); + } finally { + await rm(dir, { recursive: true, force: true }); + } +}); + +test("runGenerate honours --dry-run by returning YAML without writing", async () => { + const dir = await mkdtemp(join(tmpdir(), "gitagent-test-")); + try { + const llm: LlmClient = async () => VALID_YAML; + const result = await runGenerate({ + flags: { dir, prompt: "x", dryRun: true }, + llm, + }); + assert.equal(result.filePath, undefined); + assert.ok(result.yaml.includes("name: morning-digest")); + // workflows/ must not have been created. + await assert.rejects(() => readFile(join(dir, "workflows", "morning-digest.yaml"), "utf-8")); + } finally { + await rm(dir, { recursive: true, force: true }); + } +}); + +test("runGenerate gives up after MAX_RETRIES and throws", async () => { + const dir = await mkdtemp(join(tmpdir(), "gitagent-test-")); + try { + let calls = 0; + const llm: LlmClient = async () => { + calls++; + return INVALID_YAML_MISSING_NAME; + }; + await assert.rejects( + () => runGenerate({ flags: { dir, prompt: "x", dryRun: true }, llm }), + /Validation failed after retries/, + ); + assert.equal(calls, 3); // 1 initial + 2 retries + } finally { + await rm(dir, { recursive: true, force: true }); + } +}); + +test("runGenerate refuses to overwrite an existing workflow unless --force is passed", async () => { + const dir = await mkdtemp(join(tmpdir(), "gitagent-test-")); + try { + const llm: LlmClient = async () => VALID_YAML; + const first = await runGenerate({ flags: { dir, prompt: "x", dryRun: false }, llm }); + assert.ok(first.filePath); + + await assert.rejects( + () => runGenerate({ flags: { dir, prompt: "x", dryRun: false }, llm }), + /already exists/, + ); + + const forced = await runGenerate({ flags: { dir, prompt: "x", dryRun: false, force: true }, llm }); + assert.equal(forced.filePath, first.filePath); + } finally { + await rm(dir, { recursive: true, force: true }); + } +}); + +test("runGenerate rejects a --refine path outside the agent directory", async () => { + const dir = await mkdtemp(join(tmpdir(), "gitagent-test-")); + try { + const llm: LlmClient = async () => VALID_YAML; + await assert.rejects( + () => runGenerate({ flags: { dir, prompt: "x", refine: "/etc/hostname", dryRun: true }, llm }), + /must be inside the agent directory/, + ); + await assert.rejects( + () => runGenerate({ flags: { dir, prompt: "x", refine: "../../etc/hostname", dryRun: true }, llm }), + /must be inside the agent directory/, + ); + } finally { + await rm(dir, { recursive: true, force: true }); + } +}); + +// ── Installed-skill cross-check ──────────────────────────────────────── + +const UNKNOWN_SKILL_YAML = `name: morning-weather-summary +description: Check the weather each morning and send a text summary. +steps: + - id: fetch_weather + skill: weather + prompt: Fetch today's forecast. + - id: send_summary + skill: sms + prompt: Text a one-sentence summary. + depends_on: [fetch_weather] +`; + +// Materializes real skills//SKILL.md files so discoverSkills() finds them. +async function withInstalledSkills(names: string[], fn: (dir: string) => Promise): Promise { + const { mkdir, writeFile } = await import("node:fs/promises"); + const dir = await mkdtemp(join(tmpdir(), "gitagent-skills-")); + try { + for (const name of names) { + await mkdir(join(dir, "skills", name), { recursive: true }); + await writeFile( + join(dir, "skills", name, "SKILL.md"), + `---\nname: ${name}\ndescription: Test skill ${name}\n---\n\nDo ${name} things.\n`, + "utf-8", + ); + } + await fn(dir); + } finally { + await rm(dir, { recursive: true, force: true }); + } +} + +test("runGenerate retries when a step names a skill that is not installed", async () => { + await withInstalledSkills(["gmail", "slack", "summarize"], async (dir) => { + let calls = 0; + let retryPrompt = ""; + const llm: LlmClient = async (messages) => { + calls++; + if (calls === 1) return UNKNOWN_SKILL_YAML; + retryPrompt = messages[messages.length - 1].content; + return VALID_YAML; + }; + const result = await runGenerate({ + flags: { dir, prompt: "text me the weather", dryRun: true }, + llm, + // Isolated so `calls` counts generation attempts only. + fitnessLlm: async () => "[]", + }); + assert.equal(calls, 2); + assert.ok(retryPrompt.includes('"weather" is not an installed skill'), retryPrompt); + assert.ok(retryPrompt.includes('"sms" is not an installed skill'), retryPrompt); + assert.ok(result.yaml.includes("name: morning-digest")); + }); +}); + +test("runGenerate fails rather than writing a workflow that references missing skills", async () => { + await withInstalledSkills(["gmail", "slack", "summarize"], async (dir) => { + const llm: LlmClient = async () => UNKNOWN_SKILL_YAML; + await assert.rejects( + () => runGenerate({ flags: { dir, prompt: "text me the weather", dryRun: false }, llm }), + /Validation failed after retries/, + ); + await assert.rejects(() => readFile(join(dir, "workflows", "morning-weather-summary.yaml"), "utf-8")); + }); +}); + +test("runGenerate --allow-missing-skills writes the workflow anyway", async () => { + await withInstalledSkills(["gmail", "slack", "summarize"], async (dir) => { + let calls = 0; + const llm: LlmClient = async () => { + calls++; + return UNKNOWN_SKILL_YAML; + }; + const result = await runGenerate({ + flags: { dir, prompt: "text me the weather", dryRun: false, allowMissingSkills: true }, + llm, + }); + assert.equal(calls, 1, "should not retry when the check is disabled"); + assert.ok(result.filePath); + const written = await readFile(result.filePath!, "utf-8"); + assert.ok(written.includes("skill: weather")); + }); +}); + +test("runGenerate skips the skill check when no skills are installed", async () => { + // An agent dir with no skills/ folder is the documented escape hatch: the + // system prompt tells the model to invent sensible names, so rejecting them + // here would make generation impossible on a fresh project. + const dir = await mkdtemp(join(tmpdir(), "gitagent-test-")); + try { + let calls = 0; + const llm: LlmClient = async () => { + calls++; + return UNKNOWN_SKILL_YAML; + }; + const result = await runGenerate({ flags: { dir, prompt: "text me the weather", dryRun: true }, llm }); + assert.equal(calls, 1); + assert.ok(result.yaml.includes("skill: weather")); + } finally { + await rm(dir, { recursive: true, force: true }); + } +}); + +test("approval is accepted as a pseudo-skill even though it is not installed", async () => { + await withInstalledSkills(["analytics", "email"], async (dir) => { + const approvalYaml = `name: daily-sales-report +description: Pull sales data, require approval, then send. +steps: + - id: pull_data + skill: analytics + prompt: Pull yesterday's totals. + - id: approve + skill: approval + prompt: Review the data before it goes out. + requires_approval: true + depends_on: [pull_data] + - skill: email + prompt: Send the approved report. + depends_on: [approve] +`; + let calls = 0; + const llm: LlmClient = async () => { + calls++; + return approvalYaml; + }; + const result = await runGenerate({ + flags: { dir, prompt: "sales report with sign-off", dryRun: true }, + llm, + fitnessLlm: async () => "[]", + }); + assert.equal(calls, 1, "approval step should not trigger a retry"); + assert.ok(result.yaml.includes("skill: approval")); + }); +}); + +// ── Unsupported-request escape hatch ─────────────────────────────────── + +test("buildSystemPrompt documents the unsupported form and no longer tells the model to force a fit", () => { + const sys = buildSystemPrompt(SKILLS); + assert.ok(sys.includes("unsupported:"), "system prompt missing the unsupported escape hatch"); + assert.ok( + !sys.includes("even if that means fewer steps"), + "system prompt still pressures the model to map onto installed skills at any cost", + ); + assert.ok(sys.includes("worse than no workflow"), "system prompt missing the wrong-skill warning"); +}); + +test("parseUnsupportedReport recognises a decline and trims its items", () => { + const items = parseUnsupportedReport( + "unsupported:\n - fetch the current weather forecast \n - send a text message\n", + ); + assert.deepEqual(items, ["fetch the current weather forecast", "send a text message"]); +}); + +test("parseUnsupportedReport accepts a bare scalar", () => { + assert.deepEqual(parseUnsupportedReport("unsupported: send a text message\n"), ["send a text message"]); +}); + +test("parseUnsupportedReport returns null for a normal workflow, an empty list, and junk", () => { + assert.equal(parseUnsupportedReport(VALID_YAML), null); + assert.equal(parseUnsupportedReport("unsupported: []\n"), null); + assert.equal(parseUnsupportedReport("unsupported:\n - ''\n - ' '\n"), null); + assert.equal(parseUnsupportedReport("- just\n- a\n- list\n"), null); + assert.equal(parseUnsupportedReport("::: not yaml :::\n - [\n"), null); +}); + +test("runGenerate stops without retrying when the model declines the request", async () => { + await withInstalledSkills(["gmail", "slack", "internal-comms"], async (dir) => { + let calls = 0; + const llm: LlmClient = async () => { + calls++; + return "unsupported:\n - fetch the current weather forecast\n - send a text message\n"; + }; + await assert.rejects( + () => runGenerate({ flags: { dir, prompt: "text me the weather each morning", dryRun: false }, llm }), + /No installed skill covers part of the request/, + ); + // Retrying a decline is what produced the wrong-skill substitution it replaces. + assert.equal(calls, 1, "a decline must not be retried"); + await assert.rejects(() => readFile(join(dir, "workflows", "morning-weather-summary.yaml"), "utf-8")); + }); +}); + +test("runGenerate declines even when the model also emits a plausible workflow", async () => { + await withInstalledSkills(["gmail", "slack", "summarize"], async (dir) => { + const llm: LlmClient = async () => `${VALID_YAML}unsupported:\n - send a text message\n`; + await assert.rejects( + () => runGenerate({ flags: { dir, prompt: "text me the digest", dryRun: true }, llm }), + /No installed skill covers part of the request/, + ); + }); +}); + +// ── Advisory skill-fitness check ─────────────────────────────────────── + +const FITNESS_SKILLS: SkillMetadata[] = [ + { + name: "internal-comms", + description: "Draft and publish company-wide announcements to staff.", + directory: "/x/skills/internal-comms", + filePath: "/x/skills/internal-comms/SKILL.md", + }, + { name: "slack", description: "Post messages to Slack", directory: "/x/skills/slack", filePath: "/x/skills/slack/SKILL.md" }, +]; + +const MISFIT_WORKFLOW = { + name: "morning-weather-briefing", + description: "Send a weather briefing each morning.", + steps: [ + { skill: "internal-comms", prompt: "Write a concise morning weather briefing for today's forecast." }, + { skill: "slack", prompt: "Post the briefing." }, + ], +}; + +test("buildFitnessPrompt pairs each step with the chosen skill's description", () => { + const prompt = buildFitnessPrompt(MISFIT_WORKFLOW as any, FITNESS_SKILLS); + assert.ok(prompt.includes("step 0:"), prompt); + assert.ok(prompt.includes("skill: internal-comms"), prompt); + assert.ok(prompt.includes("Draft and publish company-wide announcements"), prompt); + assert.ok(prompt.includes("morning weather briefing"), prompt); +}); + +test("buildFitnessPrompt marks approval and uninstalled skills as not-flaggable but keeps their index", () => { + const workflow = { + name: "x", + description: "y", + steps: [ + { skill: "approval", prompt: "Sign off." }, + { skill: "made-up", prompt: "Do a thing." }, + { skill: "slack", prompt: "Post it." }, + ], + }; + const prompt = buildFitnessPrompt(workflow as any, FITNESS_SKILLS); + assert.ok(prompt.includes("step 2:\n skill: slack"), prompt); + assert.equal((prompt.match(/never flag this one/g) ?? []).length, 2, prompt); +}); + +test("parseFitnessResponse reads warnings and tolerates fences and prose", () => { + const steps = MISFIT_WORKFLOW.steps as any; + const expected = [{ stepIndex: 0, skill: "internal-comms", reason: "Announcements, not weather." }]; + assert.deepEqual(parseFitnessResponse('[{"step":0,"reason":"Announcements, not weather."}]', steps), expected); + assert.deepEqual( + parseFitnessResponse('```json\n[{"step":0,"reason":"Announcements, not weather."}]\n```', steps), + expected, + ); + assert.deepEqual( + parseFitnessResponse('Here you go:\n[{"step":0,"reason":"Announcements, not weather."}]\nHope that helps!', steps), + expected, + ); +}); + +test("parseFitnessResponse degrades to no warnings rather than throwing", () => { + const steps = MISFIT_WORKFLOW.steps as any; + for (const raw of ["", " ", "[]", "not json at all", "[{oops}]", '{"step":0,"reason":"x"}', "null"]) { + assert.deepEqual(parseFitnessResponse(raw, steps), [], `expected no warnings for ${JSON.stringify(raw)}`); + } +}); + +test("parseFitnessResponse drops out-of-range indices, blank reasons, and duplicates", () => { + const steps = MISFIT_WORKFLOW.steps as any; + const raw = JSON.stringify([ + { step: 9, reason: "points at a step that does not exist" }, + { step: -1, reason: "negative" }, + { step: 1.5, reason: "not an integer" }, + { step: 0, reason: " " }, + { step: 1, reason: "first for this step" }, + { step: 1, reason: "duplicate for this step" }, + ]); + assert.deepEqual(parseFitnessResponse(raw, steps), [ + { stepIndex: 1, skill: "slack", reason: "first for this step" }, + ]); +}); + +test("checkSkillFitness flags a real-but-unsuited skill", async () => { + let seenPrompt = ""; + const warnings = await checkSkillFitness({ + workflow: MISFIT_WORKFLOW as any, + skills: FITNESS_SKILLS, + llm: async (messages) => { + seenPrompt = messages[messages.length - 1].content; + return '[{"step":0,"reason":"internal-comms publishes staff announcements; this step needs a weather report."}]'; + }, + }); + assert.equal(warnings.length, 1); + assert.equal(warnings[0].stepIndex, 0); + assert.equal(warnings[0].skill, "internal-comms"); + assert.ok(seenPrompt.includes("internal-comms"), seenPrompt); +}); + +test("checkSkillFitness skips the LLM when no step can be judged", async () => { + let called = 0; + const llm: LlmClient = async () => { + called++; + return "[]"; + }; + const base = { skills: FITNESS_SKILLS, llm }; + + // No installed skills at all — nothing to compare a step against. + assert.deepEqual(await checkSkillFitness({ ...base, skills: [], workflow: MISFIT_WORKFLOW as any }), []); + // Only approval and uninstalled skills. + assert.deepEqual( + await checkSkillFitness({ + ...base, + workflow: { + name: "x", + description: "y", + steps: [{ skill: "approval", prompt: "Sign off." }, { skill: "made-up", prompt: "Go." }], + } as any, + }), + [], + ); + // No steps. + assert.deepEqual(await checkSkillFitness({ ...base, workflow: { name: "x", description: "y", steps: [] } as any }), []); + assert.equal(called, 0, "fitness check should not call the LLM when nothing is judgeable"); +}); + +test("checkSkillFitness swallows a provider error — the check is advisory", async () => { + const warnings = await checkSkillFitness({ + workflow: MISFIT_WORKFLOW as any, + skills: FITNESS_SKILLS, + llm: async () => { + throw new Error("429 rate limited"); + }, + }); + assert.deepEqual(warnings, []); +}); + +test("runGenerate surfaces fitness warnings but still writes the workflow", async () => { + await withInstalledSkills(["gmail", "slack", "summarize"], async (dir) => { + const result = await runGenerate({ + flags: { dir, prompt: "summarize emails and post to Slack", dryRun: false }, + llm: async () => VALID_YAML, + fitnessLlm: async () => '[{"step":2,"reason":"slack posts to a channel; this step asked for an email."}]', + }); + assert.equal(result.fitnessWarnings.length, 1); + assert.equal(result.fitnessWarnings[0].stepIndex, 2); + assert.equal(result.fitnessWarnings[0].skill, "slack"); + // A weak fit is a warning, not a validation error — the file is still written. + assert.ok(result.filePath, "expected the workflow to be written despite the warning"); + const written = await readFile(result.filePath!, "utf-8"); + assert.ok(written.includes("name: morning-digest")); + }); +}); + +test("runGenerate --no-fitness-check skips the advisory pass", async () => { + await withInstalledSkills(["gmail", "slack", "summarize"], async (dir) => { + let fitnessCalls = 0; + const result = await runGenerate({ + flags: { dir, prompt: "x", dryRun: true, fitnessCheck: false }, + llm: async () => VALID_YAML, + fitnessLlm: async () => { + fitnessCalls++; + return '[{"step":0,"reason":"should never be reported"}]'; + }, + }); + assert.equal(fitnessCalls, 0); + assert.deepEqual(result.fitnessWarnings, []); + }); +}); + +// ── Model resolution (flag > agent.yaml model.preferred > default) ───── + +async function withAgentYaml(body: string | null, fn: (dir: string) => Promise): Promise { + const { mkdir, writeFile } = await import("node:fs/promises"); + const dir = await mkdtemp(join(tmpdir(), "gitagent-model-")); + try { + // VALID_YAML names all three, so all three must be installed. + for (const name of ["gmail", "slack", "summarize"]) { + await mkdir(join(dir, "skills", name), { recursive: true }); + await writeFile( + join(dir, "skills", name, "SKILL.md"), + `---\nname: ${name}\ndescription: Test skill ${name}\n---\n\nDo ${name} things.\n`, + "utf-8", + ); + } + if (body !== null) await writeFile(join(dir, "agent.yaml"), body, "utf-8"); + await fn(dir); + } finally { + await rm(dir, { recursive: true, force: true }); + } +} + +const AGENT_YAML = `spec_version: "1.0" +name: test-agent +version: 0.0.1 +description: Test agent +model: + preferred: "anthropic:claude-sonnet-4-6" + fallback: [] +tools: [] +runtime: + max_turns: 10 +`; + +// Captures the model spec the LLM client is actually invoked with. +function modelCapturingLlm(seen: { model?: string }): LlmClient { + return async (_messages, opts) => { + seen.model = opts.model; + return VALID_YAML; + }; +} + +test("readPreferredModel reads model.preferred, and tolerates a missing or broken agent.yaml", async () => { + await withAgentYaml(AGENT_YAML, async (dir) => { + assert.equal(await readPreferredModel(dir), "anthropic:claude-sonnet-4-6"); + }); + await withAgentYaml(null, async (dir) => { + assert.equal(await readPreferredModel(dir), undefined); + }); + await withAgentYaml("model:\n preferred: [not, a, string]\n", async (dir) => { + assert.equal(await readPreferredModel(dir), undefined); + }); + await withAgentYaml("::: not yaml :::\n - [\n", async (dir) => { + assert.equal(await readPreferredModel(dir), undefined); + }); + await withAgentYaml("name: x\n", async (dir) => { + assert.equal(await readPreferredModel(dir), undefined); + }); +}); + +test("runGenerate uses agent.yaml model.preferred when no --model is passed", async () => { + await withAgentYaml(AGENT_YAML, async (dir) => { + const seen: { model?: string } = {}; + await runGenerate({ + flags: { dir, prompt: "x", dryRun: true }, + llm: modelCapturingLlm(seen), + fitnessLlm: async () => "[]", + }); + assert.equal(seen.model, "anthropic:claude-sonnet-4-6"); + }); +}); + +test("runGenerate lets --model override agent.yaml", async () => { + await withAgentYaml(AGENT_YAML, async (dir) => { + const seen: { model?: string } = {}; + await runGenerate({ + flags: { dir, prompt: "x", dryRun: true, model: "openai:gpt-4o-mini" }, + llm: modelCapturingLlm(seen), + fitnessLlm: async () => "[]", + }); + assert.equal(seen.model, "openai:gpt-4o-mini"); + }); +}); + +test("runGenerate falls back to the built-in default with no flag and no agent.yaml", async () => { + await withAgentYaml(null, async (dir) => { + const seen: { model?: string } = {}; + await runGenerate({ + flags: { dir, prompt: "x", dryRun: true }, + llm: modelCapturingLlm(seen), + fitnessLlm: async () => "[]", + }); + assert.equal(seen.model, DEFAULT_MODEL); + }); +}); + +test("the fitness pass runs on the same resolved model as generation", async () => { + await withAgentYaml(AGENT_YAML, async (dir) => { + let fitnessModel: string | undefined; + await runGenerate({ + flags: { dir, prompt: "x", dryRun: true }, + llm: async () => VALID_YAML, + fitnessLlm: async (_messages, opts) => { + fitnessModel = opts.model; + return "[]"; + }, + }); + assert.equal(fitnessModel, "anthropic:claude-sonnet-4-6"); + }); +}); + +test("runGenerate refine mode reads previous YAML and passes it to the LLM", async () => { + const dir = await mkdtemp(join(tmpdir(), "gitagent-test-")); + try { + const { writeFile, mkdir } = await import("node:fs/promises"); + await mkdir(join(dir, "workflows"), { recursive: true }); + const refinePath = join(dir, "workflows", "starter.yaml"); + await writeFile(refinePath, VALID_YAML, "utf-8"); + + let observed = ""; + const llm: LlmClient = async (messages) => { + observed = messages[messages.length - 1].content; + return VALID_YAML; + }; + await runGenerate({ + flags: { + dir, + prompt: "add an approval step before slack", + refine: "workflows/starter.yaml", + dryRun: true, + }, + llm, + }); + assert.ok(observed.includes("Here is the current workflow")); + assert.ok(observed.includes("add an approval step before slack")); + } finally { + await rm(dir, { recursive: true, force: true }); + } +}); diff --git a/test/workflow-telemetry.test.ts b/test/workflow-telemetry.test.ts new file mode 100644 index 0000000..0abe49a --- /dev/null +++ b/test/workflow-telemetry.test.ts @@ -0,0 +1,134 @@ +// Telemetry coverage for the workflow subcommand path. +// +// The subcommand used to return from main() before initTelemetry() ran, so +// nothing on this path was traced. These tests assert the two things that were +// wrong once it is traced: the per-call gen_ai spans must parent to the +// generation span (otherwise the cost of one run cannot be summed), and token +// attributes must account for cache tokens (otherwise a 1500-token call reports +// as 3 while the cost attribute disagrees). + +import test from "node:test"; +import assert from "node:assert/strict"; +import { mkdtemp, mkdir, writeFile, rm } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { InMemorySpanExporter, NodeTracerProvider, SimpleSpanProcessor } from "@opentelemetry/sdk-trace-node"; + +import { initTelemetry, recordGenAiCall } from "../src/telemetry.ts"; +import { runGenerate } from "../src/commands/workflow.ts"; +import type { LlmClient } from "../src/utils/workflow-generator.ts"; + +const exporter = new InMemorySpanExporter(); +const provider = new NodeTracerProvider({ spanProcessors: [new SimpleSpanProcessor(exporter)] }); +await initTelemetry({ _testProvider: provider }); + +const VALID_YAML = `name: morning-digest +description: Summarize unread emails and post to Slack each morning. +steps: + - skill: gmail + prompt: Fetch unread emails. + - skill: slack + prompt: Post the digest. +`; + +// Shaped like a real pi-ai assistant message: prompt caching puts the bulk of +// the input in cacheWrite, leaving `input` tiny. +const FAKE_MSG = { + role: "assistant", + provider: "anthropic", + model: "claude-sonnet-4-6", + stopReason: "stop", + usage: { + input: 3, + output: 179, + cacheRead: 11, + cacheWrite: 1509, + cost: { total: 0.008574 }, + }, +}; + +async function withAgentDir(fn: (dir: string) => Promise): Promise { + const dir = await mkdtemp(join(tmpdir(), "gitagent-otel-")); + try { + for (const name of ["gmail", "slack"]) { + await mkdir(join(dir, "skills", name), { recursive: true }); + await writeFile( + join(dir, "skills", name, "SKILL.md"), + `---\nname: ${name}\ndescription: Test skill ${name}\n---\n\nDo ${name} things.\n`, + "utf-8", + ); + } + await fn(dir); + } finally { + await rm(dir, { recursive: true, force: true }); + } +} + +test("recordGenAiCall reports cache tokens alongside input and output", () => { + exporter.reset(); + recordGenAiCall(FAKE_MSG, { durationMs: 42 }); + const span = exporter.getFinishedSpans().find((s) => s.name === "gen_ai.chat"); + assert.ok(span, "no gen_ai.chat span was exported"); + assert.equal(span!.attributes["gen_ai.usage.input_tokens"], 3); + assert.equal(span!.attributes["gen_ai.usage.output_tokens"], 179); + // Without these two the span claims 3 input tokens for a 1523-token call. + assert.equal(span!.attributes["gen_ai.usage.cache_read_input_tokens"], 11); + assert.equal(span!.attributes["gen_ai.usage.cache_creation_input_tokens"], 1509); + assert.equal(span!.attributes["gitagent.cost_usd"], 0.008574); +}); + +test("recordGenAiCall defaults cache attributes to 0 when the provider omits them", () => { + exporter.reset(); + recordGenAiCall({ provider: "openai", model: "gpt-4o", usage: { input: 10, output: 20 } }); + const span = exporter.getFinishedSpans().find((s) => s.name === "gen_ai.chat"); + assert.equal(span!.attributes["gen_ai.usage.cache_read_input_tokens"], 0); + assert.equal(span!.attributes["gen_ai.usage.cache_creation_input_tokens"], 0); +}); + +test("workflow generation emits a span, and its LLM calls are children of it", async () => { + await withAgentDir(async (dir) => { + exporter.reset(); + // Stands in for defaultLlmClient, which reports each assistant message. + const llm: LlmClient = async () => { + recordGenAiCall(FAKE_MSG, { durationMs: 10 }); + return VALID_YAML; + }; + await runGenerate({ + flags: { dir, prompt: "summarize email and post to Slack", dryRun: true, model: "anthropic:claude-sonnet-4-6" }, + llm, + fitnessLlm: async () => { + recordGenAiCall(FAKE_MSG, { durationMs: 10 }); + return "[]"; + }, + }); + + const spans = exporter.getFinishedSpans(); + const parent = spans.find((s) => s.name === "gitagent.workflow.generate"); + assert.ok(parent, `no workflow span exported, got: ${spans.map((s) => s.name).join(", ")}`); + assert.equal(parent!.attributes["gitagent.workflow.model"], "anthropic:claude-sonnet-4-6"); + assert.equal(parent!.attributes["gitagent.workflow.attempts"], 1); + assert.equal(parent!.attributes["gitagent.workflow.fitness_warnings"], 0); + assert.equal(parent!.attributes["gitagent.workflow.skills_installed"], 2); + + // Both the generation call and the fitness call must hang off the same + // parent, so one run's cost is one subtree rather than orphan traces. + const calls = spans.filter((s) => s.name === "gen_ai.chat"); + assert.equal(calls.length, 2, "expected one generation call and one fitness call"); + for (const call of calls) { + assert.equal(call.parentSpanContext?.spanId, parent!.spanContext().spanId); + assert.equal(call.spanContext().traceId, parent!.spanContext().traceId); + } + }); +}); + +test("a failed generation marks the workflow span as an error", async () => { + await withAgentDir(async (dir) => { + exporter.reset(); + const llm: LlmClient = async () => "unsupported:\n - send a text message\n"; + await assert.rejects(() => runGenerate({ flags: { dir, prompt: "text me", dryRun: true }, llm })); + const parent = exporter.getFinishedSpans().find((s) => s.name === "gitagent.workflow.generate"); + assert.ok(parent); + assert.equal(parent!.status.code, 2 /* SpanStatusCode.ERROR */); + assert.match(String(parent!.status.message), /No installed skill covers/); + }); +}); diff --git a/test/workflow-validator.test.ts b/test/workflow-validator.test.ts new file mode 100644 index 0000000..c917607 --- /dev/null +++ b/test/workflow-validator.test.ts @@ -0,0 +1,279 @@ +// Unit tests for src/utils/schemas.ts — the SkillFlow workflow validator. + +import test from "node:test"; +import assert from "node:assert/strict"; + +import { validateWorkflow, loadWorkflowSchema, validateSkillReferences } from "../src/utils/schemas.ts"; + +const VALID_YAML = `name: morning-digest +description: Summarize unread emails and post to Slack each morning. +steps: + - skill: gmail + prompt: Fetch unread emails from the last 24h. + - skill: summarize + prompt: Compose a digest grouped by sender priority. + - skill: slack + prompt: Post the digest to the team channel. + channel: "#daily-digest" +`; + +test("loadWorkflowSchema returns the parsed schema with required top-level keys", () => { + const schema = loadWorkflowSchema(); + assert.equal(typeof schema, "object"); + assert.deepEqual(schema.required, ["name", "description", "steps"]); + assert.equal(schema.definitions.step.required.includes("skill"), true); + assert.equal(schema.definitions.step.required.includes("prompt"), true); +}); + +test("validateWorkflow accepts a well-formed workflow", () => { + const r = validateWorkflow(VALID_YAML); + assert.equal(r.valid, true); + assert.deepEqual(r.errors, []); + assert.equal(r.data?.name, "morning-digest"); + assert.equal(r.data?.steps.length, 3); +}); + +test("validateWorkflow rejects missing name", () => { + const yaml = `description: foo +steps: + - skill: gmail + prompt: do thing +`; + const r = validateWorkflow(yaml); + assert.equal(r.valid, false); + assert.ok(r.errors.some((e) => e.includes('missing required property "name"')), `errors: ${JSON.stringify(r.errors)}`); +}); + +test("validateWorkflow rejects non-kebab-case name", () => { + const yaml = `name: MyWorkflow +description: foo +steps: + - skill: gmail + prompt: do thing +`; + const r = validateWorkflow(yaml); + assert.equal(r.valid, false); + assert.ok(r.errors.some((e) => e.includes("pattern")), `errors: ${JSON.stringify(r.errors)}`); +}); + +test("validateWorkflow rejects empty steps array", () => { + const yaml = `name: empty-flow +description: nothing +steps: [] +`; + const r = validateWorkflow(yaml); + assert.equal(r.valid, false); + assert.ok(r.errors.some((e) => e.includes("at least 1")), `errors: ${JSON.stringify(r.errors)}`); +}); + +test("validateWorkflow rejects a step missing required prompt", () => { + const yaml = `name: bad-step +description: missing prompt +steps: + - skill: gmail +`; + const r = validateWorkflow(yaml); + assert.equal(r.valid, false); + assert.ok( + r.errors.some((e) => e.includes('missing required property "prompt"')), + `errors: ${JSON.stringify(r.errors)}`, + ); +}); + +test("validateWorkflow rejects unknown step property", () => { + const yaml = `name: extra-prop +description: bad +steps: + - skill: gmail + prompt: do thing + nonsense: true +`; + const r = validateWorkflow(yaml); + assert.equal(r.valid, false); + assert.ok( + r.errors.some((e) => e.includes('unknown property "nonsense"')), + `errors: ${JSON.stringify(r.errors)}`, + ); +}); + +test("validateWorkflow flags depends_on referencing a missing id", () => { + const yaml = `name: bad-deps +description: dangling dep +steps: + - id: a + skill: gmail + prompt: fetch + - skill: slack + prompt: post + depends_on: [does_not_exist] +`; + const r = validateWorkflow(yaml); + assert.equal(r.valid, false); + assert.ok( + r.errors.some((e) => e.includes('references unknown step id "does_not_exist"')), + `errors: ${JSON.stringify(r.errors)}`, + ); +}); + +test("validateWorkflow flags a self-referencing depends_on", () => { + const yaml = `name: self-cycle +description: step depends on itself +steps: + - id: a + skill: gmail + prompt: fetch + depends_on: [a] +`; + const r = validateWorkflow(yaml); + assert.equal(r.valid, false); + assert.ok(r.errors.some((e) => e.includes("cycle")), `errors: ${JSON.stringify(r.errors)}`); +}); + +test("validateWorkflow flags an A -> B -> A depends_on cycle", () => { + const yaml = `name: mutual-cycle +description: two steps depend on each other +steps: + - id: a + skill: gmail + prompt: fetch + depends_on: [b] + - id: b + skill: slack + prompt: post + depends_on: [a] +`; + const r = validateWorkflow(yaml); + assert.equal(r.valid, false); + assert.ok( + r.errors.some((e) => e.includes("depends_on cycle detected")), + `errors: ${JSON.stringify(r.errors)}`, + ); +}); + +test("validateWorkflow rejects a forward depends_on reference", () => { + // Steps run in declaration order, so depending on a later step can never be + // satisfied. Catching it here keeps validate in step with loadFlowDefinition + // and gives the retry loop a chance to fix it before anything is written. + const yaml = `name: forward-ref +description: first step depends on the second +steps: + - skill: gmail + prompt: fetch + depends_on: [post] + - id: post + skill: slack + prompt: post +`; + const r = validateWorkflow(yaml); + assert.equal(r.valid, false); + assert.ok( + r.errors.some((e) => e.includes('references unknown step id "post"') && e.includes("preceding step")), + `errors: ${JSON.stringify(r.errors)}`, + ); +}); + +test("validateWorkflow accepts a diamond-shaped depends_on graph", () => { + const yaml = `name: diamond-flow +description: fan out then fan in +steps: + - id: root + skill: gmail + prompt: fetch + - id: left + skill: summarize + prompt: summarize inbox + depends_on: [root] + - id: right + skill: summarize + prompt: summarize archive + depends_on: [root] + - id: post + skill: slack + prompt: post both summaries + depends_on: [left, right] +`; + const r = validateWorkflow(yaml); + assert.equal(r.valid, true, `errors: ${JSON.stringify(r.errors)}`); +}); + +test("validateWorkflow accepts approval step with requires_approval", () => { + const yaml = `name: approval-flow +description: needs sign-off +steps: + - id: pull + skill: analytics + prompt: Pull data. + - id: approve + skill: approval + prompt: Approve distribution. + requires_approval: true + depends_on: [pull] + - skill: email + prompt: Send report. + depends_on: [approve] +`; + const r = validateWorkflow(yaml); + assert.equal(r.valid, true, `errors: ${JSON.stringify(r.errors)}`); +}); + +test("validateWorkflow surfaces YAML parse errors", () => { + const yaml = `name: bad +description: : : +steps: + - skill: [unterminated +`; + const r = validateWorkflow(yaml); + assert.equal(r.valid, false); + assert.ok(r.errors[0].startsWith("YAML parse error"), `errors: ${JSON.stringify(r.errors)}`); +}); + +test("validateWorkflow rejects empty document", () => { + const r = validateWorkflow(""); + assert.equal(r.valid, false); + assert.ok(r.errors[0].includes("empty")); +}); + +// ── validateSkillReferences ──────────────────────────────────────────── + +const UNKNOWN_SKILLS_YAML = `name: morning-weather-summary +description: Check the weather each morning and send a text summary. +steps: + - id: fetch_weather + skill: weather + prompt: Fetch today's forecast. + - id: send_summary + skill: sms + prompt: Text a one-sentence summary. + depends_on: [fetch_weather] +`; + +test("validateSkillReferences reports every step naming an uninstalled skill", () => { + const data = validateWorkflow(UNKNOWN_SKILLS_YAML).data!; + const errors = validateSkillReferences(data, ["gmail", "slack", "summarize"]); + assert.deepEqual(errors, [ + 'steps[0].skill: "weather" is not an installed skill', + 'steps[1].skill: "sms" is not an installed skill', + ]); +}); + +test("validateSkillReferences passes when every skill is installed", () => { + const data = validateWorkflow(VALID_YAML).data!; + assert.deepEqual(validateSkillReferences(data, ["gmail", "slack", "summarize"]), []); +}); + +test("validateSkillReferences exempts the approval pseudo-skill", () => { + const yaml = `name: with-approval +description: Approval step uses a pseudo-skill. +steps: + - skill: approval + prompt: Sign off before sending. + requires_approval: true +`; + const data = validateWorkflow(yaml).data!; + assert.deepEqual(validateSkillReferences(data, ["gmail"]), []); +}); + +test("validateSkillReferences is a no-op when no skills are installed", () => { + const data = validateWorkflow(UNKNOWN_SKILLS_YAML).data!; + assert.deepEqual(validateSkillReferences(data, []), []); +}); diff --git a/test/workflows.test.ts b/test/workflows.test.ts new file mode 100644 index 0000000..2e45b1e --- /dev/null +++ b/test/workflows.test.ts @@ -0,0 +1,140 @@ +// Unit tests for loadFlowDefinition in src/workflows.ts — path containment, +// structural guards, and dependency handling. + +import test from "node:test"; +import assert from "node:assert/strict"; +import { mkdtemp, mkdir, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { loadFlowDefinition } from "../src/workflows.ts"; + +async function withAgentDir(files: Record, fn: (dir: string) => Promise): Promise { + const dir = await mkdtemp(join(tmpdir(), "gitagent-flows-")); + try { + await mkdir(join(dir, "workflows"), { recursive: true }); + for (const [name, content] of Object.entries(files)) { + await writeFile(join(dir, "workflows", name), content, "utf-8"); + } + await fn(dir); + } finally { + await rm(dir, { recursive: true, force: true }); + } +} + +const VALID_FLOW = `name: morning-digest +description: Summarize unread emails and post to Slack. +steps: + - id: fetch + skill: gmail + prompt: Fetch unread emails. + - skill: slack + prompt: Post the digest. + channel: "#daily-digest" + depends_on: [fetch] + requires_approval: true +`; + +test("loadFlowDefinition loads a flow by name and preserves id/depends_on/requires_approval", async () => { + await withAgentDir({ "morning-digest.yaml": VALID_FLOW }, async (dir) => { + const flow = await loadFlowDefinition(dir, "morning-digest"); + assert.equal(flow.name, "morning-digest"); + assert.equal(flow.steps.length, 2); + assert.equal(flow.steps[0].id, "fetch"); + assert.deepEqual(flow.steps[1].depends_on, ["fetch"]); + assert.equal(flow.steps[1].requires_approval, true); + assert.equal(flow.steps[1].channel, "#daily-digest"); + }); +}); + +test("loadFlowDefinition rejects non-kebab-case names, including traversal attempts", async () => { + await withAgentDir({ "morning-digest.yaml": VALID_FLOW }, async (dir) => { + for (const bad of ["../../etc/passwd", "/etc/passwd", "..", "Not_Kebab", "with space"]) { + await assert.rejects(() => loadFlowDefinition(dir, bad), /must be kebab-case/, `accepted "${bad}"`); + } + }); +}); + +test("loadFlowDefinition reports a clear error for an empty file", async () => { + await withAgentDir({ "empty.yaml": "" }, async (dir) => { + await assert.rejects(() => loadFlowDefinition(dir, "empty"), /must be a YAML mapping/); + }); +}); + +test("loadFlowDefinition reports a clear error for a scalar document", async () => { + await withAgentDir({ "scalar.yaml": "just a string\n" }, async (dir) => { + await assert.rejects(() => loadFlowDefinition(dir, "scalar"), /must be a YAML mapping/); + }); +}); + +test("loadFlowDefinition coerces a non-string description", async () => { + const flow = `name: numeric-desc +description: 42 +steps: + - skill: gmail + prompt: Fetch. +`; + await withAgentDir({ "numeric-desc.yaml": flow }, async (dir) => { + const loaded = await loadFlowDefinition(dir, "numeric-desc"); + assert.equal(typeof loaded.description, "string"); + assert.equal(loaded.description, "42"); + }); +}); + +test("loadFlowDefinition rejects a step with an empty skill", async () => { + const flow = `name: empty-skill +description: missing skill +steps: + - skill: gmail + prompt: Fetch. + - prompt: Post it somewhere. +`; + await withAgentDir({ "empty-skill.yaml": flow }, async (dir) => { + await assert.rejects(() => loadFlowDefinition(dir, "empty-skill"), /step\[1\] has an empty skill/); + }); +}); + +test("loadFlowDefinition still accepts the legacy single full-path argument", async () => { + await withAgentDir({ "morning-digest.yaml": VALID_FLOW }, async (dir) => { + const legacyPath = join(dir, "workflows", "morning-digest.yaml"); + const flow = await loadFlowDefinition(legacyPath); + assert.equal(flow.name, "morning-digest"); + assert.equal(flow.steps.length, 2); + }); +}); + +test("loadFlowDefinition does not read workflows/undefined.yaml when the name is omitted", async () => { + await withAgentDir({ "morning-digest.yaml": VALID_FLOW }, async (dir) => { + // The legacy form treats the lone argument as a file path, so an agent dir + // passed alone must fail as a missing file rather than silently looking for + // a flow literally named "undefined". + await assert.rejects(() => loadFlowDefinition(dir), (err: any) => { + assert.ok(!String(err.message).includes("undefined"), err.message); + return true; + }); + }); +}); + +test("loadFlowDefinition rejects depends_on that does not name a preceding step", async () => { + const forward = `name: forward-dep +description: depends on a later step +steps: + - skill: gmail + prompt: Fetch. + depends_on: [post] + - id: post + skill: slack + prompt: Post. +`; + const dangling = `name: dangling-dep +description: depends on nothing that exists +steps: + - skill: gmail + prompt: Fetch. + depends_on: [nope] +`; + await withAgentDir({ "forward-dep.yaml": forward, "dangling-dep.yaml": dangling }, async (dir) => { + await assert.rejects(() => loadFlowDefinition(dir, "forward-dep"), /not the id of a preceding step/); + await assert.rejects(() => loadFlowDefinition(dir, "dangling-dep"), /not the id of a preceding step/); + }); +});