diff --git a/AGENTS.md b/AGENTS.md index 5a4753d..2afc33f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -159,6 +159,22 @@ is confined to the skill's own directory — real paths are resolved, so a symli refused, not followed — and a malformed `SKILL.md` is warned about and skipped rather than failing the run. +**Skill trust** (`src/skill-trust.ts`) decides whether a skill is offered at all, which +is the control that matters for the real threat: a `SKILL.md` is instructions the model +obeys. An untrusted skill is **neither listed in the prompt nor readable** — the listing +is itself a foothold, since names and descriptions are injected for every skill — so +unvetted text never enters the context window. Trust is _declared by the operator, never +by the skill_ (a malicious `SKILL.md` would simply claim to be trusted), so the record +lives outside it, in `$XDG_CONFIG_HOME/agent/skills.trust.json`, keyed by the skill's +**content hash**: "I reviewed this" is worth nothing if the bytes can change underneath +the review. The operator's own trees are trusted by default — `--skills`, the project's +`./skills`, the user-level shelf; nothing arrives without them putting it there — a +directory they got elsewhere is named with `--skills-untrusted`, and a **blocked** +verdict refuses a skill whatever else says. `--skills-trust ` vouches for one by +recording its hash, and `--skills-list` prints the discovered skills with their level, +hash and status. A scan (a separate, out-of-band step) is what turns "unvetted" into a +recorded verdict — the runtime never fetches and never scans. + **Delegation** (`src/delegation.ts`) is a `task` tool, off unless `--task`: a sub-agent that runs in its own session and returns only its result. Two rules bound it — the child never auto-approves (delegation must not amplify authority) diff --git a/README.md b/README.md index 4a5d812..de4d699 100644 --- a/README.md +++ b/README.md @@ -105,6 +105,12 @@ do nothing at all in the REPL.) large skill library costs almost nothing until it is used. A malformed `SKILL.md` is reported and skipped rather than failing the run, and the read tool cannot leave a skill's own directory — real paths are resolved, so a symlink out is refused. + A skill you got from somewhere else is kept out of the model's reach entirely: + name its directory with `--skills-untrusted` and it is neither listed in the + prompt nor readable, until you vouch for it (`--skills-trust ` records its + content hash in `$XDG_CONFIG_HOME/agent/skills.trust.json`) or a scan clears it. + `--skills-list` prints what was found, with each skill's level, hash and trust + status, and needs no MCP server. - **Delegation.** `--task` exposes a `task` tool: the model can hand a self-contained job to a **sub-agent** that runs in its own session and returns only its result — so a broad search or a long review cannot flood the caller's diff --git a/docs/archetypes/skills-agent.md b/docs/archetypes/skills-agent.md index a29eb74..2f21fd6 100644 --- a/docs/archetypes/skills-agent.md +++ b/docs/archetypes/skills-agent.md @@ -88,6 +88,22 @@ skill; "read a file" is a tool. own permissions — or the reverse, a procedure that has to run in the caller's context when it needed isolation. +### Trust and containment + +- **Purpose.** Decide whether a skill's instructions may enter the model's context at + all, and bound what reading one can reach. +- **Contract (sketch).** Containment: resolve real paths and refuse anything that is + not a regular file inside the skill's own directory. Trust: declared by the operator + (never by the skill — a hostile `SKILL.md` would simply claim to be trusted), keyed + by the skill's content hash, with a scan verdict able to refuse a skill regardless. +- **Reference.** `opencode`'s discovery service trusts a publisher-declared `version` + string and downloads over HTTPS — the weakest useful model; `llama.cpp` ships skills + as local files and never fetches, so it has no trust model to get wrong. In this host + the untrusted case is a skill _copied_ in — a vendored or submoduled pack — which is + where both a hostile instruction and a symlink arrive. +- **Failure mode prevented.** A hostile procedure entering the context window, and a + read escaping the skill's directory through a link. + ## Our coverage | Component | Status | Evidence | @@ -95,6 +111,7 @@ skill; "read a file" is a tool. | `SKILL.md` definition + trigger description | ✅ | `src/skills.ts`: frontmatter `name`/`description`, the `description` used as the trigger | | Progressive disclosure | ✅ | the prompt carries names + descriptions only; the body loads through `skills_read_skill`, bundled files by name | | Discovery (user / project / remote) | 🟡 | `--skills ` (repeatable), the `./skills` convention, and a user-level default; nothing remote | +| Trust and containment | ✅ | operator-declared and hash-keyed (`src/skill-trust.ts`); an untrusted skill is neither listed nor readable, and the read tool resolves real paths, refusing anything that is not a regular file inside the skill | | Supply chain (fetch / cache / version) | ❌ | local directories only — nothing is fetched, so nothing needs signing | | Invocation (context injection vs sub-session) | ✅ | both, explicitly: the body enters the current context via `skills_read_skill`, or the skill runs as an isolated sub-session when a `task` names it (`agent`), where its `tools`/`deny` narrows what that child may use | diff --git a/docs/architecture.md b/docs/architecture.md index e8ba490..05e440b 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -69,14 +69,15 @@ ordinary **toolsets**, so using them takes the same path as any MCP tool — app gate, middleware, execution graph and telemetry all apply unchanged, and none adds a protocol concept. -| Capability | Module | What it is | -| ---------------- | ------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| **Skills** | `src/skills.ts` | `/SKILL.md` instructions, found at three levels: `--skills `, the project's `./skills`, and the user-level `$XDG_CONFIG_HOME/agent/skills` — a project skill wins over a user one. Only names and descriptions enter the system prompt; `skills_read_skill` loads the body on demand and cannot leave the skill's directory. Frontmatter may also declare `tools`/`deny`, making the skill an **agent definition**. | -| **Pause/resume** | `src/runtime.ts` | With `--pause`, a `require_confirmation` call is recorded as a suspension and the run stops with exit `5` instead of denying it; `--resume ` continues the saved conversation so the blocked call simply runs. The record is `Suspension` (`@adaptivemcp/spec`), persisted by `memory`. | -| **Delegation** | `src/delegation.ts` | `--task` exposes `task`: a sub-agent in its own session that returns a **result, never its transcript**. It shares the parent's store (same graph and learned state, its own session) and inherits no authority — it never auto-approves and never receives `task` itself. `agent: ""` names a definition whose policy is intersected with the parent's (`narrowTools`), so it can only narrow. | -| **Task list** | `src/tasks.ts` | `--tasks` exposes the shared list: `tasks_create` writes down intended work and the ids it `dependsOn`, `tasks_list` reads it back (or only what is _ready_), and `tasks_update` moves a task on — reporting what it left unmet or unblocked. Transitions are not policed: the record says what was decided, and the store's `readyTasks` is what orders it, treating an unknown dependency id as unfinished so a typo blocks rather than unblocks. | -| **Modes** | `src/modes.ts` | `--mode ` names the coordination pattern (`coordinate`/`route`/`broadcast`/`tasks`), implies the toolsets it rests on, and adds its paragraph to the system prompt — so the shape of the work is a choice rather than an emergent accident. `tasks` goes furthest and passes a **stop guard** to the loop. | -| **Typed steps** | `src/steps.ts` | One typed history per session: an `ActionStep` for every attempted call (a gate refusal or a pause is an attempt, not a failure), and — with `--plan-interval `, off by default — a **planning pass** that records a `PlanStep` asked for from the _recorded_ state and injects it for the step that follows. `/steps` renders tasks, actions and plans in order. | +| Capability | Module | What it is | +| ---------------- | -------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| **Skills** | `src/skills.ts` | `/SKILL.md` instructions, found at three levels: `--skills `, the project's `./skills`, and the user-level `$XDG_CONFIG_HOME/agent/skills` — a project skill wins over a user one. Only names and descriptions enter the system prompt; `skills_read_skill` loads the body on demand and cannot leave the skill's directory. Frontmatter may also declare `tools`/`deny`, making the skill an **agent definition**. | +| **Skill trust** | `src/skill-trust.ts` | Trust is declared by the operator, never by the skill, and keyed by the skill's content hash (`$XDG_CONFIG_HOME/agent/skills.trust.json`): an untrusted skill is neither listed in the prompt nor readable, so unvetted instructions never enter the context window. `--skills-untrusted ` marks a directory from elsewhere; `--skills-trust ` vouches for one; a blocked scan verdict refuses it whatever else says. | +| **Pause/resume** | `src/runtime.ts` | With `--pause`, a `require_confirmation` call is recorded as a suspension and the run stops with exit `5` instead of denying it; `--resume ` continues the saved conversation so the blocked call simply runs. The record is `Suspension` (`@adaptivemcp/spec`), persisted by `memory`. | +| **Delegation** | `src/delegation.ts` | `--task` exposes `task`: a sub-agent in its own session that returns a **result, never its transcript**. It shares the parent's store (same graph and learned state, its own session) and inherits no authority — it never auto-approves and never receives `task` itself. `agent: ""` names a definition whose policy is intersected with the parent's (`narrowTools`), so it can only narrow. | +| **Task list** | `src/tasks.ts` | `--tasks` exposes the shared list: `tasks_create` writes down intended work and the ids it `dependsOn`, `tasks_list` reads it back (or only what is _ready_), and `tasks_update` moves a task on — reporting what it left unmet or unblocked. Transitions are not policed: the record says what was decided, and the store's `readyTasks` is what orders it, treating an unknown dependency id as unfinished so a typo blocks rather than unblocks. | +| **Modes** | `src/modes.ts` | `--mode ` names the coordination pattern (`coordinate`/`route`/`broadcast`/`tasks`), implies the toolsets it rests on, and adds its paragraph to the system prompt — so the shape of the work is a choice rather than an emergent accident. `tasks` goes furthest and passes a **stop guard** to the loop. | +| **Typed steps** | `src/steps.ts` | One typed history per session: an `ActionStep` for every attempted call (a gate refusal or a pause is an attempt, not a failure), and — with `--plan-interval `, off by default — a **planning pass** that records a `PlanStep` asked for from the _recorded_ state and injects it for the step that follows. `/steps` renders tasks, actions and plans in order. | All of these are opt-in, so a host that enables none behaves exactly as it did before. A host must therefore **forward the loop's options rather than re-list the diff --git a/src/agent-config.test.ts b/src/agent-config.test.ts index d8efc28..e43582f 100644 --- a/src/agent-config.test.ts +++ b/src/agent-config.test.ts @@ -58,6 +58,16 @@ describe("loadAgentConfig", () => { expect(() => loadAgentConfig(write("s3.yaml", "skills: 5"))).toThrow(/skills/); }); + it("accepts skillsUntrusted as a string or a list", () => { + expect(loadAgentConfig(write("u.yaml", 'skillsUntrusted: "vendor"')).skillsUntrusted).toEqual([ + "vendor", + ]); + expect( + loadAgentConfig(write("u2.yaml", "skillsUntrusted:\n - a\n - b")).skillsUntrusted, + ).toEqual(["a", "b"]); + expect(() => loadAgentConfig(write("u3.yaml", "skillsUntrusted: 5"))).toThrow(/skillsUntrusted/); + }); + it("treats an empty file as no options", () => { expect(loadAgentConfig(write("empty.yaml", ""))).toEqual({}); }); diff --git a/src/agent-config.ts b/src/agent-config.ts index 47b25f0..0fab2cf 100644 --- a/src/agent-config.ts +++ b/src/agent-config.ts @@ -92,6 +92,16 @@ export function loadAgentConfig(path: string): Partial { out.skills = value as string[]; else throw new Error(`config "${path}": "skills" must be a string or a list of strings`); } + if (doc.skillsUntrusted !== undefined) { + const value = doc.skillsUntrusted; + if (typeof value === "string") out.skillsUntrusted = [value]; + else if (Array.isArray(value) && value.every((s) => typeof s === "string")) + out.skillsUntrusted = value as string[]; + else + throw new Error( + `config "${path}": "skillsUntrusted" must be a string or a list of strings`, + ); + } for (const key of NUMBER_KEYS) { const value = doc[key]; if (value === undefined) continue; @@ -158,6 +168,7 @@ export function loadAgentConfig(path: string): Partial { const known = new Set([ "servers", "skills", + "skillsUntrusted", "defaultRisk", "system1Backend", "mode", diff --git a/src/cli-args.test.ts b/src/cli-args.test.ts index e06a967..e6547f5 100644 --- a/src/cli-args.test.ts +++ b/src/cli-args.test.ts @@ -116,6 +116,11 @@ describe("parseArgs coverage of the full flag surface", () => { "skills", "--skills", "/opt/skills", + "--skills-untrusted", + "/vendor/skills", + "--skills-trust", + "deploy", + "--skills-list", "--compress", "--report", "--no-server-policy", @@ -148,6 +153,9 @@ describe("parseArgs coverage of the full flag surface", () => { history: "hist.json", workflow: "wf1", skills: ["skills", "/opt/skills"], + skillsUntrusted: ["/vendor/skills"], + skillsTrust: ["deploy"], + skillsList: true, compress: true, report: true, serverPolicy: false, diff --git a/src/cli-args.ts b/src/cli-args.ts index 60a0400..3cf326e 100644 --- a/src/cli-args.ts +++ b/src/cli-args.ts @@ -46,6 +46,15 @@ export interface CliArgs { * `./skills` is added automatically when it exists. */ skills: string[]; + /** + * Skill directories whose skills are untrusted: not listed in the prompt and + * not readable until vouched for by name or by a scan verdict. + */ + skillsUntrusted: string[]; + /** Skill names to vouch for (records their hash in the trust file), then exit. */ + skillsTrust: string[]; + /** Print the discovered skills with their trust status, then exit. */ + skillsList: boolean; /** Local fallback risk for tools whose server ships no MCP annotations. */ defaultRisk: "low" | "medium" | "high" | "none"; /** Poll long-running tool jobs (payloads carrying a job handle) to completion. */ @@ -138,6 +147,9 @@ export function parseTierList(raw: string, source: string): System1Tier[] { const DEFAULT_ARGS: CliArgs = { servers: [], skills: [], + skillsUntrusted: [], + skillsTrust: [], + skillsList: false, listTools: false, interactive: false, defaultRisk: "medium", @@ -218,6 +230,15 @@ export function parseFlags(argv: string[]): Partial { case "--skills": (args.skills ??= []).push(next()); break; + case "--skills-untrusted": + (args.skillsUntrusted ??= []).push(next()); + break; + case "--skills-trust": + (args.skillsTrust ??= []).push(next()); + break; + case "--skills-list": + args.skillsList = true; + break; case "--default-risk": { const value = next(); if (value !== "low" && value !== "medium" && value !== "high" && value !== "none") { @@ -482,6 +503,12 @@ Options: ./skills and $XDG_CONFIG_HOME/agent/skills are searched too, a project skill winning over a user one. Only names and descriptions enter the prompt; bodies load on demand + --skills-untrusted Treat the skills under as untrusted: not listed in + the prompt and not readable, until vouched for (repeatable) + --skills-trust Vouch for a skill by recording its content hash in + $XDG_CONFIG_HOME/agent/skills.trust.json, then exit + --skills-list Print the discovered skills, their level, hash and trust + status, then exit (needs no MCP server) --prompt One-shot prompt instead of the interactive REPL -i, --interactive Force the interactive REPL --model Pin one catalog model (disables adaptive selection) diff --git a/src/cli.e2e.test.ts b/src/cli.e2e.test.ts index ba422df..5810db2 100644 --- a/src/cli.e2e.test.ts +++ b/src/cli.e2e.test.ts @@ -1,5 +1,5 @@ import { spawn, type ChildProcess } from "node:child_process"; -import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { fileURLToPath } from "node:url"; @@ -719,6 +719,138 @@ describe("cli, end to end and offline", () => { rmSync(configHome, { recursive: true, force: true }); }, 30_000); + it("lists skills, with their level and hash, and needs no MCP server", async () => { + const configHome = mkdtempSync(join(tmpdir(), "agent-e2e-skill-list-")); + const skillsDir = mkdtempSync(join(tmpdir(), "agent-e2e-skill-list-src-")); + mkdirSync(join(skillsDir, "deploy"), { recursive: true }); + writeFileSync( + join(skillsDir, "deploy", "SKILL.md"), + "---\nname: deploy\ndescription: Ship it\n---\n\nGo.\n", + ); + + // No --server and no model: inspecting skills is a host act, not a run. + const run = await runCli(["--skills-list", "--skills", skillsDir], { + XDG_CONFIG_HOME: configHome, + }); + + expect(run.code).toBe(0); + expect(run.stdout).toContain("deploy"); + expect(run.stdout).toContain("trusted"); + expect(run.stdout).toMatch(/sha256:[0-9a-f]{64}/); + + rmSync(configHome, { recursive: true, force: true }); + rmSync(skillsDir, { recursive: true, force: true }); + }, 30_000); + + it("withholds a skill from a directory marked untrusted, and says why", async () => { + const configHome = mkdtempSync(join(tmpdir(), "agent-e2e-skill-trust-config-")); + const skillsDir = mkdtempSync(join(tmpdir(), "agent-e2e-skill-trust-src-")); + mkdirSync(join(skillsDir, "sketchy"), { recursive: true }); + writeFileSync( + join(skillsDir, "sketchy", "SKILL.md"), + "---\nname: sketchy\ndescription: From somewhere else\n---\n\nTrust me.\n", + ); + model = new StubModelServer({ respond: () => ({ text: "done" }) }); + const baseUrl = await model.start(); + + const run = await runCli( + [ + "--prompt", + "say hi", + "--skills", + skillsDir, + "--skills-untrusted", + skillsDir, + "--server", + serverSpec(), + "--base-url", + baseUrl, + "--api-key", + "test-key", + "--no-ollama", + "--no-stream", + "--db", + ":memory:", + "--session", + "cli-e2e-skill-trust", + "--max-steps", + "1", + ], + { XDG_CONFIG_HOME: configHome }, + ); + + expect(run.code).toBe(0); + // Not in the prompt listing — the listing is itself a foothold — and not offered. + const system = model.requests[0]?.messages.find((message) => message.role === "system"); + expect(system?.content ?? "").not.toContain("sketchy"); + expect(offeredTools(model.requests[0]!)).not.toContain("skills_read_skill"); + // The operator is told what was withheld, and how to vouch for it. + expect(run.stderr).toContain("withheld as untrusted"); + expect(run.stderr).toContain("sketchy"); + + rmSync(configHome, { recursive: true, force: true }); + rmSync(skillsDir, { recursive: true, force: true }); + }, 30_000); + + it("vouches for a skill by hash, so a marked directory becomes usable", async () => { + const configHome = mkdtempSync(join(tmpdir(), "agent-e2e-skill-vouch-config-")); + const skillsDir = mkdtempSync(join(tmpdir(), "agent-e2e-skill-vouch-src-")); + mkdirSync(join(skillsDir, "review"), { recursive: true }); + writeFileSync( + join(skillsDir, "review", "SKILL.md"), + "---\nname: review\ndescription: Review a diff\n---\n\nRead it, then report.\n", + ); + const env = { XDG_CONFIG_HOME: configHome }; + + // Vouching is a config act of its own: it records the hash and exits. + const vouched = await runCli(["--skills", skillsDir, "--skills-trust", "review"], env); + expect(vouched.code).toBe(0); + expect(vouched.stdout).toContain("vouched for review"); + const record = JSON.parse( + readFileSync(join(configHome, "agent", "skills.trust.json"), "utf8"), + ) as { entries: Array<{ name: string; hash: string }> }; + expect(record.entries[0]?.name).toBe("review"); + expect(record.entries[0]?.hash).toMatch(/^sha256:/); + + // Now the same directory, marked untrusted, still offers the skill it vouched for. + model = new StubModelServer({ respond: () => ({ text: "done" }) }); + const baseUrl = await model.start(); + const run = await runCli( + [ + "--prompt", + "review it", + "--skills", + skillsDir, + "--skills-untrusted", + skillsDir, + "--server", + serverSpec(), + "--base-url", + baseUrl, + "--api-key", + "test-key", + "--no-ollama", + "--no-stream", + "--db", + ":memory:", + "--session", + "cli-e2e-skill-vouch", + "--max-steps", + "1", + ], + env, + ); + + expect(run.code).toBe(0); + const system = model.requests[0]?.messages.find((message) => message.role === "system"); + expect(system?.content).toContain("review: Review a diff"); + expect(offeredTools(model.requests[0]!)).toContain("skills_read_skill"); + expect(run.stderr).not.toContain("withheld"); + + rmSync(configHome, { recursive: true, force: true }); + rmSync(skillsDir, { recursive: true, force: true }); + }, 30_000); + it("pauses a high-risk call, then resumes the run that stopped on it", async () => { // A file-backed store on purpose: a paused run has to outlive the process // that paused it, which is the whole point of the feature. diff --git a/src/cli.ts b/src/cli.ts index c59d4bf..2b90e5a 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -46,6 +46,15 @@ import { skillsToolset, type AgentDefinition, } from "./skills.js"; +import { + addTrustEntry, + decideTrust, + readTrust, + skillHash, + trustFilePath, + writeTrust, + type TrustRecord, +} from "./skill-trust.js"; import { tasksPrompt, tasksToolset } from "./tasks.js"; import { modePrompt, modeTools, tasksStopGuard } from "./modes.js"; import { PlanningPass, PlanUsage, formatSteps } from "./steps.js"; @@ -131,6 +140,58 @@ async function main(): Promise { return; } + // Inspecting and vouching for skills are host config acts, so they need no MCP + // server and no model: discover, report, write the record, and stop. + if (args.skillsList || args.skillsTrust.length > 0) { + const discovered = discoverSkills(skillDirs(args.skills), (message) => + console.warn(`[skills] ${message}`), + ); + const trustFile = trustFilePath(); + let record = loadTrustRecord(trustFile); + if (!record) return; + + if (args.skillsTrust.length > 0) { + for (const name of args.skillsTrust) { + const skill = discovered.find((candidate) => candidate.name === name); + const hash = skill ? skillHash(skill) : undefined; + if (!hash) { + console.error(`error: --skills-trust: no readable skill named "${name}"`); + process.exitCode = ExitCode.Usage; + return; + } + record = addTrustEntry(record, { + name, + hash, + status: "trusted", + note: "vouched for on the command line", + at: new Date().toISOString(), + }); + } + writeTrust(record, trustFile); + console.log( + `[skills] vouched for ${args.skillsTrust.join(", ")} — recorded in ${trustFile}`, + ); + if (!args.skillsList) return; + } + + if (discovered.length === 0) console.log("[skills] none discovered"); + const withheld = new Map( + decideTrust(discovered, { untrustedDirs: args.skillsUntrusted, record }).withheld.map( + ({ skill, reason }) => [skill, reason], + ), + ); + for (const skill of discovered) { + const status = withheld.has(skill) ? "untrusted" : "trusted"; + const reason = withheld.get(skill); + console.log( + [status, skill.origin, skill.name, skillHash(skill) ?? "unreadable", reason] + .filter((field) => field !== undefined) + .join("\t"), + ); + } + return; + } + const fallbackServer = exampleServerOptions(); const serverSpecs: ServerSpec[] = args.servers.length > 0 @@ -211,9 +272,24 @@ async function main(): Promise { // Skills are host-owned instructions exposed as an ordinary toolset, so // reading one takes the same path as any MCP tool (gate, telemetry, graph). // Only names and descriptions reach the prompt; the body is fetched on demand. - const skills = discoverSkills(skillDirs(args.skills), (message) => { + // Trust decides which skills are offered at all: a withheld skill is neither + // listed (the listing is itself a foothold — names and descriptions steer the + // model) nor readable, so unvetted text never enters the context window. + const discovered = discoverSkills(skillDirs(args.skills), (message) => { if (args.verbose) console.warn(`[skills] ${message}`); }); + const trustRecord = loadTrustRecord(trustFilePath()); + if (!trustRecord) return; + const { trusted: skills, withheld } = decideTrust(discovered, { + untrustedDirs: args.skillsUntrusted, + record: trustRecord, + }); + if (withheld.length > 0) { + console.error( + `[skills] ${withheld.length} withheld as untrusted: ` + + withheld.map(({ skill, reason }) => `${skill.name} (${reason})`).join("; "), + ); + } const sessionId = args.session ?? `cli-${Date.now()}`; // Delegation is wired lazily: the toolset joins the aggregate now (so `task` is // in the tool list the model sees), while its dependencies — the parent @@ -674,6 +750,17 @@ async function main(): Promise { } } +/** Read the trust record, reporting a malformed one as a config error. */ +function loadTrustRecord(path: string): TrustRecord | undefined { + try { + return readTrust(path); + } catch (error) { + console.error(`error: ${error instanceof Error ? error.message : String(error)}`); + process.exitCode = ExitCode.Config; + return undefined; + } +} + /** Child step cap: a sub-agent is for one bounded piece of work, not a second run. */ const TASK_MAX_STEPS = 6; diff --git a/src/skill-trust.test.ts b/src/skill-trust.test.ts new file mode 100644 index 0000000..0e89800 --- /dev/null +++ b/src/skill-trust.test.ts @@ -0,0 +1,175 @@ +import { describe, it, expect, beforeEach, afterEach } from "vitest"; +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { SKILL_FILE, discoverSkills, type Skill } from "./skills.js"; +import { + addTrustEntry, + decideTrust, + readTrust, + skillHash, + trustFilePath, + writeTrust, + type TrustRecord, +} from "./skill-trust.js"; + +describe("skill trust", () => { + let root: string; + + beforeEach(() => { + root = mkdtempSync(join(tmpdir(), "agent-skill-trust-")); + }); + + afterEach(() => { + rmSync(root, { recursive: true, force: true }); + }); + + /** Write one skill under `root` and return it as discovery sees it. */ + function makeSkill(name: string, body = "Do the thing."): Skill { + const dir = join(root, name); + mkdirSync(dir, { recursive: true }); + writeFileSync( + join(dir, SKILL_FILE), + `---\nname: ${name}\ndescription: ${name} does a thing\n---\n\n${body}\n`, + ); + return discover(root).find((skill) => skill.name === name)!; + } + + function discover(from: string): Skill[] { + return discoverSkills([{ path: from, origin: "project" }]); + } + + describe("skillHash", () => { + it("is stable for the same content", () => { + const skill = makeSkill("deploy"); + expect(skillHash(skill)).toBe(skillHash(discover(root)[0]!)); + expect(skillHash(skill)).toMatch(/^sha256:[0-9a-f]{64}$/); + }); + + it("changes with the instructions, and with a bundled file", () => { + const skill = makeSkill("deploy"); + const before = skillHash(skill)!; + + writeFileSync(join(skill.dir, "notes.md"), "- item"); + const withBundle = discover(root)[0]!; + const after = skillHash(withBundle)!; + expect(after).not.toBe(before); // a verdict must cover the files, not just SKILL.md + + makeSkill("deploy", "Do something else."); + expect(skillHash(discover(root)[0]!)).not.toBe(after); + }); + + it("is undefined when the skill cannot be read back", () => { + const skill = makeSkill("deploy"); + rmSync(skill.dir, { recursive: true, force: true }); + expect(skillHash(skill)).toBeUndefined(); + }); + }); + + describe("decideTrust", () => { + it("trusts the operator's own trees by default", () => { + const skill = makeSkill("deploy"); + expect(decideTrust([skill])).toEqual({ trusted: [skill], withheld: [] }); + }); + + it("withholds everything under a directory marked untrusted", () => { + const skill = makeSkill("deploy"); + const { trusted, withheld } = decideTrust([skill], { untrustedDirs: [root] }); + + expect(trusted).toEqual([]); + expect(withheld[0]?.skill).toEqual(skill); + expect(withheld[0]?.reason).toContain("marked untrusted"); + }); + + it("vouches for exactly those bytes — an edit is no longer vouched for", () => { + const skill = makeSkill("deploy"); + const record: TrustRecord = { + version: 1, + entries: [{ name: "deploy", hash: skillHash(skill)!, status: "trusted", at: "now" }], + }; + + expect(decideTrust([skill], { untrustedDirs: [root], record }).trusted).toEqual([skill]); + + // Same name, different content: "I reviewed this" stops being true. + makeSkill("deploy", "Now with an exfiltration step."); + const edited = discover(root)[0]!; + expect(decideTrust([edited], { untrustedDirs: [root], record }).trusted).toEqual([]); + }); + + it("refuses a blocked skill even inside a trusted directory", () => { + const skill = makeSkill("deploy"); + const record: TrustRecord = { + version: 1, + entries: [ + { name: "deploy", hash: skillHash(skill)!, status: "blocked", note: "exfil rule", at: "now" }, + ], + }; + + const { trusted, withheld } = decideTrust([skill], { record }); + + expect(trusted).toEqual([]); + expect(withheld[0]?.reason).toContain("exfil rule"); + }); + + it("will not vouch for a skill it cannot read back", () => { + const skill = makeSkill("deploy"); + rmSync(skill.dir, { recursive: true, force: true }); + + const { trusted, withheld } = decideTrust([skill], { untrustedDirs: [root] }); + + expect(trusted).toEqual([]); + expect(withheld).toHaveLength(1); + }); + }); + + describe("the record on disk", () => { + it("lives beside the user skills directory", () => { + expect(trustFilePath({ XDG_CONFIG_HOME: "/opt/cfg" }, "/home/u")).toBe( + "/opt/cfg/agent/skills.trust.json", + ); + expect(trustFilePath({}, "/home/u")).toBe("/home/u/.config/agent/skills.trust.json"); + }); + + it("round-trips, and a missing file means no record yet", () => { + const path = join(root, "skills.trust.json"); + expect(readTrust(path)).toEqual({ version: 1, entries: [] }); + + const record: TrustRecord = { + version: 1, + entries: [{ name: "a", hash: "sha256:x", status: "trusted", note: "scanned", at: "now" }], + }; + writeTrust(record, path); + expect(readTrust(path)).toEqual(record); + }); + + it("refuses a record it cannot understand, rather than quietly un-blocking", () => { + const path = join(root, "skills.trust.json"); + + writeFileSync(path, "{ not json"); + expect(() => readTrust(path)).toThrow(/not valid JSON/); + + writeFileSync(path, JSON.stringify({ version: 2, entries: [] })); + expect(() => readTrust(path)).toThrow(/unsupported version/); + + writeFileSync(path, JSON.stringify({ version: 1, entries: [{ name: "a" }] })); + expect(() => readTrust(path)).toThrow(/malformed entry/); + }); + + it("replaces an entry for a skill instead of accumulating hashes", () => { + const first = addTrustEntry( + { version: 1, entries: [] }, + { name: "a", hash: "sha256:1", status: "trusted", at: "t1" }, + ); + const second = addTrustEntry(first, { + name: "a", + hash: "sha256:2", + status: "trusted", + at: "t2", + }); + + expect(second.entries).toEqual([ + { name: "a", hash: "sha256:2", status: "trusted", at: "t2" }, + ]); + }); + }); +}); diff --git a/src/skill-trust.ts b/src/skill-trust.ts new file mode 100644 index 0000000..a163608 --- /dev/null +++ b/src/skill-trust.ts @@ -0,0 +1,214 @@ +/** + * Skill trust: whether the host offers a skill to the model at all. + * + * Containment (`src/skills.ts`) bounds what a skill can *reach*; trust decides + * whether it is offered in the first place, and that is the control that matters + * for the real threat — a `SKILL.md` is instructions the model obeys. So an + * untrusted skill is **not listed in the prompt and not readable**: it never enters + * the context window, rather than entering it with a warning attached. (The prompt + * listing is itself a foothold — only `name` and `description` are needed to steer + * a model, and those are injected for every discovered skill.) + * + * Trust is **declared by the operator, never by the skill**: a `SKILL.md` cannot + * vouch for itself, because a malicious one would simply say it is trusted. The + * record therefore lives outside the skill, in the host's config directory, and is + * keyed by the skill's **content hash** — "I reviewed this" is worth nothing if the + * thing can change underneath the review. + * + * The default is that the operator's own trees are trusted: the `--skills` + * directories they pointed at, the project's `./skills`, and their user-level + * shelf. Nothing arrives here without them putting it there (there is no fetch), + * so requiring a promotion for every skill they wrote would be friction with no + * threat behind it. A directory they *did* get from elsewhere is named with + * `--skills-untrusted`, and a blocking verdict from a scan refuses a skill + * whatever else says. + */ +import { createHash } from "node:crypto"; +import { mkdirSync, readFileSync, realpathSync, writeFileSync } from "node:fs"; +import { homedir } from "node:os"; +import { dirname, join, sep } from "node:path"; +import { SKILL_FILE, userSkillsDir, type Skill } from "./skills.js"; + +/** What a trust entry says about one content hash. */ +export type TrustStatus = "trusted" | "blocked"; + +export interface TrustEntry { + name: string; + /** `sha256:…` over the skill's instructions and bundled files. */ + hash: string; + status: TrustStatus; + /** Why, in the operator's words — a scan verdict, a date, a note. */ + note?: string; + /** When the entry was written. */ + at: string; +} + +/** The host's trust record, as stored on disk. */ +export interface TrustRecord { + version: 1; + entries: TrustEntry[]; +} + +export function emptyTrust(): TrustRecord { + return { version: 1, entries: [] }; +} + +/** The record's path: `$XDG_CONFIG_HOME/agent/skills.trust.json`, beside the user skills dir. */ +export function trustFilePath( + env: NodeJS.ProcessEnv = process.env, + home: string = homedir(), +): string { + return join(dirname(userSkillsDir(env, home)), "skills.trust.json"); +} + +/** + * Read the trust record. A file that cannot be understood is an **error**, not an + * empty record: silently ignoring it would quietly un-block whatever it blocked. + */ +export function readTrust(path: string): TrustRecord { + let text: string; + try { + text = readFileSync(path, "utf8"); + } catch { + return emptyTrust(); // no record yet is the normal first-run case + } + let parsed: unknown; + try { + parsed = JSON.parse(text); + } catch { + throw new Error(`trust record "${path}" is not valid JSON`); + } + if (typeof parsed !== "object" || parsed === null) { + throw new Error(`trust record "${path}" must be a JSON object`); + } + const record = parsed as { version?: unknown; entries?: unknown }; + if (record.version !== 1) { + throw new Error(`trust record "${path}" has an unsupported version: ${String(record.version)}`); + } + if (!Array.isArray(record.entries)) { + throw new Error(`trust record "${path}" needs an "entries" array`); + } + const entries: TrustEntry[] = []; + for (const raw of record.entries) { + const entry = raw as Partial; + const ok = + typeof entry?.name === "string" && + typeof entry.hash === "string" && + (entry.status === "trusted" || entry.status === "blocked") && + typeof entry.at === "string" && + (entry.note === undefined || typeof entry.note === "string"); + if (!ok) throw new Error(`trust record "${path}" has a malformed entry`); + entries.push({ + name: entry.name as string, + hash: entry.hash as string, + status: entry.status as TrustStatus, + note: entry.note, + at: entry.at as string, + }); + } + return { version: 1, entries }; +} + +export function writeTrust(record: TrustRecord, path: string): void { + mkdirSync(dirname(path), { recursive: true }); // the config dir may not exist yet + writeFileSync(path, `${JSON.stringify(record, null, 2)}\n`); +} + +/** Record `entry`, replacing any earlier entry for the same skill name. */ +export function addTrustEntry(record: TrustRecord, entry: TrustEntry): TrustRecord { + return { + version: 1, + entries: [...record.entries.filter((existing) => existing.name !== entry.name), entry], + }; +} + +/** + * A skill's identity for a trust decision: its instructions, plus the bundled + * files the read tool advertises, hashed in a fixed order. `undefined` when the + * skill cannot be read back — a skill that cannot be vouched for is not vouched + * for. + */ +export function skillHash(skill: Skill): string | undefined { + try { + const hash = createHash("sha256"); + hash.update(`skill: ${skill.name}\n`); + hash.update(readFileSync(join(skill.dir, SKILL_FILE))); + for (const file of [...skill.files].sort()) { + hash.update(`\nfile: ${file}\n`); + hash.update(readFileSync(join(skill.dir, file))); + } + return `sha256:${hash.digest("hex")}`; + } catch { + return undefined; + } +} + +export interface WithheldSkill { + skill: Skill; + /** Why it was withheld, for the host to say out loud. */ + reason: string; +} + +export interface TrustDecision { + /** Skills the model may see: listed in the prompt, readable by the tool. */ + trusted: Skill[]; + /** Skills withheld, with the reason, so the host can report them. */ + withheld: WithheldSkill[]; +} + +/** + * Split discovered skills into those the model may see and those it may not. + * + * Precedence, in order: a **blocked** verdict refuses regardless of anything else + * (a positive finding is not overridden by a directory being trusted); an explicit + * **trusted** entry vouches for exactly those bytes; a directory named with + * `--skills-untrusted` withholds what is under it; everything else is trusted. + */ +export function decideTrust( + skills: Skill[], + { + untrustedDirs = [], + record = emptyTrust(), + }: { untrustedDirs?: string[]; record?: TrustRecord } = {}, +): TrustDecision { + const marked = untrustedDirs.map(realDir).filter((dir): dir is string => dir !== undefined); + const trusted: Skill[] = []; + const withheld: WithheldSkill[] = []; + + for (const skill of skills) { + const hash = skillHash(skill); + const entry = hash + ? record.entries.find((candidate) => candidate.name === skill.name && candidate.hash === hash) + : undefined; + if (entry?.status === "blocked") { + withheld.push({ + skill, + reason: entry.note ? `blocked by the trust record (${entry.note})` : "blocked by the trust record", + }); + } else if (entry?.status === "trusted") { + trusted.push(skill); + } else if (marked.some((dir) => inside(dir, skill.dir))) { + withheld.push({ + skill, + reason: "in a directory marked untrusted — vouch for it with --skills-trust or scan it", + }); + } else { + trusted.push(skill); + } + } + return { trusted, withheld }; +} + +/** The real path of a directory, or `undefined` when it does not exist. */ +function realDir(path: string): string | undefined { + try { + return realpathSync(path); + } catch { + return undefined; + } +} + +/** Whether `path` is `root` or sits under it. Both must already be real paths. */ +function inside(root: string, path: string): boolean { + return path === root || path.startsWith(root + sep); +}