diff --git a/browse/src/browser-skill-write.ts b/browse/src/browser-skill-write.ts new file mode 100644 index 00000000..55ffd9e2 --- /dev/null +++ b/browse/src/browser-skill-write.ts @@ -0,0 +1,215 @@ +/** + * Atomic-write helper for agent-authored browser-skills (D3 from Phase 2 plan). + * + * /skillify stages a candidate skill into ~/.gstack/.tmp/skillify-/, + * runs $B skill test against it, and only renames the directory into its final + * tier path on success + user approval. On failure or rejection, the staged + * directory is removed entirely — no half-written skill ever appears in + * $B skill list, no tombstone for something the user never approved. + * + * stageSkill — write all files into the staging dir, return its path + * commitSkill — atomic rename into the final tier path; refuses to clobber + * discardStaged — rm -rf the staged dir (called on test fail or reject) + * + * Symlink discipline: lstat() the staging dir before rename to refuse moves + * through symlinks; realpath() the final tier root to ensure the destination + * lands inside the expected directory tree. + */ + +import * as fs from 'fs'; +import * as path from 'path'; +import * as os from 'os'; +import { isPathWithin } from './platform'; +import type { TierPaths } from './browser-skills'; +import { defaultTierPaths } from './browser-skills'; + +// ─── Naming validation ────────────────────────────────────────── + +/** + * Skill names must be safe directory names: lowercase letters, digits, dashes. + * Starts with a letter, no consecutive dashes, no trailing dash, ≤64 chars. + * Rejects '..', leading dots, slashes, anything that could escape the tier dir. + */ +const SKILL_NAME_PATTERN = /^[a-z][a-z0-9]*(-[a-z0-9]+)*$/; + +export function validateSkillName(name: string): void { + if (!name) throw new Error('Skill name is empty.'); + if (name.length > 64) throw new Error(`Skill name too long (${name.length} > 64).`); + if (!SKILL_NAME_PATTERN.test(name)) { + throw new Error( + `Invalid skill name "${name}". Must be lowercase letters/digits/dashes, ` + + `start with a letter, no leading/trailing/consecutive dashes.`, + ); + } +} + +// ─── Staging ──────────────────────────────────────────────────── + +export interface StageSkillOptions { + name: string; + /** Map of relative path → contents. Path may contain '/' for nested dirs. */ + files: Map; + /** Optional override (tests pass synthetic spawn ids). */ + spawnId?: string; + /** Optional override (tests pass a fake tmp root). */ + tmpRoot?: string; +} + +/** + * Stage a skill into the staging tree: + * /.gstack/.tmp/skillify-// + * + * The leaf directory is what gets renamed during commit. The wrapper + * skillify-/ is per-spawn so concurrent /skillify invocations don't + * collide. Returns the absolute path to the staged skill dir (ending in ). + */ +export function stageSkill(opts: StageSkillOptions): string { + validateSkillName(opts.name); + if (opts.files.size === 0) { + throw new Error('stageSkill: files map is empty.'); + } + + const spawnId = opts.spawnId ?? generateSpawnId(); + const tmpRoot = opts.tmpRoot ?? path.join(os.homedir(), '.gstack', '.tmp'); + const wrapperDir = path.join(tmpRoot, `skillify-${spawnId}`); + const stagedDir = path.join(wrapperDir, opts.name); + + fs.mkdirSync(wrapperDir, { recursive: true, mode: 0o700 }); + fs.mkdirSync(stagedDir, { recursive: true, mode: 0o700 }); + + for (const [relPath, contents] of opts.files) { + if (relPath.startsWith('/') || relPath.includes('..')) { + // Defense in depth: validateSkillName above bounds the leaf, but a + // bad relPath in files could still write outside the staged dir. + throw new Error(`Invalid file path in stageSkill: "${relPath}".`); + } + const filePath = path.join(stagedDir, relPath); + const fileDir = path.dirname(filePath); + fs.mkdirSync(fileDir, { recursive: true }); + fs.writeFileSync(filePath, contents); + } + + return stagedDir; +} + +// ─── Commit (atomic rename) ───────────────────────────────────── + +export interface CommitSkillOptions { + name: string; + tier: 'project' | 'global'; + stagedDir: string; + /** Optional override (tests pass synthetic tier paths). */ + tiers?: TierPaths; +} + +/** + * Atomically move the staged skill into its final tier path. Refuses to + * clobber an existing skill at the same path — the agent's approval gate + * MUST surface name collisions before calling this. + * + * Returns the absolute path of the committed skill dir. + * + * Throws when: + * - tier path is unresolved (project tier with no project root) + * - destination already exists + * - staged dir is a symlink (refuses to follow) + * - resolved destination escapes the tier root (defense in depth) + */ +export function commitSkill(opts: CommitSkillOptions): string { + validateSkillName(opts.name); + + const tiers = opts.tiers ?? defaultTierPaths(); + const tierRoot = opts.tier === 'project' ? tiers.project : tiers.global; + if (!tierRoot) { + throw new Error(`commitSkill: tier "${opts.tier}" has no resolved path.`); + } + + // Refuse to follow a symlinked staging dir — caller should hand us the path + // returned by stageSkill, which is always a real directory. + let stagedStat: fs.Stats; + try { + stagedStat = fs.lstatSync(opts.stagedDir); + } catch (err: any) { + throw new Error(`commitSkill: staged dir "${opts.stagedDir}" not accessible: ${err.code ?? err.message}`); + } + if (stagedStat.isSymbolicLink()) { + throw new Error(`commitSkill: staged dir "${opts.stagedDir}" is a symlink — refusing to commit.`); + } + if (!stagedStat.isDirectory()) { + throw new Error(`commitSkill: staged path "${opts.stagedDir}" is not a directory.`); + } + + // Ensure the tier root exists, then resolve its real path so the final + // destination check defends against tierRoot itself being a symlink. + fs.mkdirSync(tierRoot, { recursive: true, mode: 0o755 }); + const realTierRoot = fs.realpathSync(tierRoot); + + const dest = path.join(realTierRoot, opts.name); + if (!isPathWithin(dest, realTierRoot)) { + // Should be impossible after validateSkillName, but defense in depth. + throw new Error(`commitSkill: destination "${dest}" escapes tier root.`); + } + + // Refuse to clobber. Both regular dirs and symlinks count. + let destExists = false; + try { + fs.lstatSync(dest); + destExists = true; + } catch (err: any) { + if (err.code !== 'ENOENT') throw err; + } + if (destExists) { + throw new Error( + `commitSkill: a skill named "${opts.name}" already exists at ${dest}. ` + + `Pick a different name or remove the existing skill first ` + + `($B skill rm ${opts.name}${opts.tier === 'global' ? ' --global' : ''}).`, + ); + } + + fs.renameSync(opts.stagedDir, dest); + return dest; +} + +// ─── Discard (cleanup on failure or reject) ───────────────────── + +/** + * Remove the staged skill directory and its per-spawn wrapper. Called on + * test failure (step 8 of /skillify) or approval rejection (step 9). + * + * Idempotent: missing dirs are not an error. Best-effort: failures are + * swallowed (cleanup is fire-and-forget, not load-bearing). + */ +export function discardStaged(stagedDir: string): void { + // Remove the leaf skill dir first, then the wrapper skillify-/. + // If the wrapper was the only thing inside it, this tidies up that too. + try { + fs.rmSync(stagedDir, { recursive: true, force: true }); + } catch { + // best effort + } + const wrapperDir = path.dirname(stagedDir); + if (path.basename(wrapperDir).startsWith('skillify-')) { + try { + // Only remove the wrapper if it's now empty — concurrent /skillify + // invocations get their own wrappers, but if a buggy caller passed + // a stagedDir not under a skillify- wrapper we should not nuke + // an unrelated parent. + const remaining = fs.readdirSync(wrapperDir); + if (remaining.length === 0) { + fs.rmdirSync(wrapperDir); + } + } catch { + // best effort + } + } +} + +// ─── Spawn id ─────────────────────────────────────────────────── + +/** Per-spawn id matching the format used by skill-token.ts. */ +function generateSpawnId(): string { + // 8 random hex chars + millis suffix — collision risk negligible across + // concurrent /skillify invocations on a single machine. + const rand = Math.floor(Math.random() * 0xffffffff).toString(16).padStart(8, '0'); + return `${rand}-${Date.now().toString(36)}`; +} diff --git a/browse/test/browser-skill-write.test.ts b/browse/test/browser-skill-write.test.ts new file mode 100644 index 00000000..dbdb147f --- /dev/null +++ b/browse/test/browser-skill-write.test.ts @@ -0,0 +1,350 @@ +/** + * D3 helper tests — staging, atomic commit, and discard for /skillify. + * + * These tests use synthetic tier paths and a synthetic tmp root so they + * never touch the user's real ~/.gstack/ tree. The contract under test: + * + * stageSkill → writes files into ~/.gstack/.tmp/skillify-// + * commitSkill → atomic rename to //, refuses to clobber + * discardStaged → rm -rf the staged dir + per-spawn wrapper, idempotent + * + * Failure-mode coverage: + * - simulated test failure between stage and commit → discardStaged leaves + * no on-disk artifact (the bug class the helper exists to prevent) + * - commit refuses to clobber an existing skill dir + * - commit refuses to follow a symlinked staging dir + * - discardStaged is idempotent (safe to call twice) + */ + +import { describe, it, expect, beforeEach, afterEach } from 'bun:test'; +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +import { + stageSkill, + commitSkill, + discardStaged, + validateSkillName, +} from '../src/browser-skill-write'; +import type { TierPaths } from '../src/browser-skills'; + +let tmpRoot: string; +let tiers: TierPaths; +let stagingTmpRoot: string; + +beforeEach(() => { + tmpRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'browser-skill-write-test-')); + tiers = { + project: path.join(tmpRoot, 'project', '.gstack', 'browser-skills'), + global: path.join(tmpRoot, 'home', '.gstack', 'browser-skills'), + bundled: path.join(tmpRoot, 'gstack-install', 'browser-skills'), + }; + // Synthetic tmp root keeps tests off the real ~/.gstack/.tmp/. + stagingTmpRoot = path.join(tmpRoot, 'home', '.gstack', '.tmp'); +}); + +afterEach(() => { + fs.rmSync(tmpRoot, { recursive: true, force: true }); +}); + +function sampleFiles(): Map { + return new Map([ + ['SKILL.md', '---\nname: test-skill\nhost: example.com\ntriggers: []\nargs: []\ntrusted: false\n---\nbody\n'], + ['script.ts', 'console.log("hi");\n'], + ['_lib/browse-client.ts', '// fake SDK\n'], + ['fixtures/example-com-2026-04-27.html', '\n'], + ['script.test.ts', 'import { describe, it, expect } from "bun:test"; describe("x", () => { it("y", () => expect(1).toBe(1)); });\n'], + ]); +} + +// ─── validateSkillName ────────────────────────────────────────── + +describe('validateSkillName', () => { + it.each([ + ['hackernews-frontpage'], + ['scrape'], + ['lobsters-frontpage-v2'], + ['a'], + ['a1'], + ])('accepts valid name: %s', (name) => { + expect(() => validateSkillName(name)).not.toThrow(); + }); + + it.each([ + [''], + ['UPPERCASE'], + ['has space'], + ['../escape'], + ['/abs/path'], + ['-leading-dash'], + ['trailing-dash-'], + ['double--dash'], + ['1starts-with-digit'], + ['has.dot'], + ['has_underscore'], + ['a'.repeat(65)], + ])('rejects invalid name: %s', (name) => { + expect(() => validateSkillName(name)).toThrow(); + }); +}); + +// ─── stageSkill ───────────────────────────────────────────────── + +describe('stageSkill', () => { + it('writes all files into the staged dir and returns the path', () => { + const stagedDir = stageSkill({ + name: 'test-skill', + files: sampleFiles(), + spawnId: 'aaaa1111-test', + tmpRoot: stagingTmpRoot, + }); + + expect(stagedDir).toBe(path.join(stagingTmpRoot, 'skillify-aaaa1111-test', 'test-skill')); + expect(fs.existsSync(path.join(stagedDir, 'SKILL.md'))).toBe(true); + expect(fs.existsSync(path.join(stagedDir, 'script.ts'))).toBe(true); + expect(fs.existsSync(path.join(stagedDir, '_lib', 'browse-client.ts'))).toBe(true); + expect(fs.existsSync(path.join(stagedDir, 'fixtures', 'example-com-2026-04-27.html'))).toBe(true); + expect(fs.readFileSync(path.join(stagedDir, 'script.ts'), 'utf-8')).toContain('hi'); + }); + + it('creates the wrapper dir with restrictive perms', () => { + const stagedDir = stageSkill({ + name: 'test-skill', + files: sampleFiles(), + spawnId: 'bbbb2222-test', + tmpRoot: stagingTmpRoot, + }); + const wrapperDir = path.dirname(stagedDir); + const stat = fs.statSync(wrapperDir); + // 0o700 = owner-only; mode mask off everything else. + expect((stat.mode & 0o077)).toBe(0); + }); + + it('rejects empty file maps', () => { + expect(() => + stageSkill({ + name: 'test-skill', + files: new Map(), + spawnId: 'cccc3333-test', + tmpRoot: stagingTmpRoot, + }), + ).toThrow(/files map is empty/); + }); + + it('rejects file paths that try to escape', () => { + const bad = new Map([ + ['SKILL.md', 'ok\n'], + ['../escape.ts', 'bad\n'], + ]); + expect(() => + stageSkill({ + name: 'test-skill', + files: bad, + spawnId: 'dddd4444-test', + tmpRoot: stagingTmpRoot, + }), + ).toThrow(/Invalid file path/); + }); + + it('rejects invalid skill names', () => { + expect(() => + stageSkill({ + name: 'BAD/NAME', + files: sampleFiles(), + spawnId: 'eeee5555-test', + tmpRoot: stagingTmpRoot, + }), + ).toThrow(/Invalid skill name/); + }); + + it('keeps concurrent stages isolated by spawnId', () => { + const a = stageSkill({ name: 'shared-name', files: sampleFiles(), spawnId: 'spawn-a', tmpRoot: stagingTmpRoot }); + const b = stageSkill({ name: 'shared-name', files: sampleFiles(), spawnId: 'spawn-b', tmpRoot: stagingTmpRoot }); + expect(a).not.toBe(b); + expect(fs.existsSync(a)).toBe(true); + expect(fs.existsSync(b)).toBe(true); + }); +}); + +// ─── commitSkill ──────────────────────────────────────────────── + +describe('commitSkill', () => { + it('atomically renames staged dir into the global tier path', () => { + const stagedDir = stageSkill({ + name: 'test-skill', + files: sampleFiles(), + spawnId: 'commit-1', + tmpRoot: stagingTmpRoot, + }); + + const dest = commitSkill({ + name: 'test-skill', + tier: 'global', + stagedDir, + tiers, + }); + + expect(dest).toBe(path.join(fs.realpathSync(tiers.global), 'test-skill')); + expect(fs.existsSync(dest)).toBe(true); + expect(fs.existsSync(path.join(dest, 'SKILL.md'))).toBe(true); + // The staged dir is gone (rename moved it). + expect(fs.existsSync(stagedDir)).toBe(false); + }); + + it('refuses to clobber an existing skill at the same path', () => { + // Pre-create a colliding skill at the global tier. + fs.mkdirSync(path.join(tiers.global, 'collide-skill'), { recursive: true }); + fs.writeFileSync(path.join(tiers.global, 'collide-skill', 'marker.txt'), 'existing\n'); + + const stagedDir = stageSkill({ + name: 'collide-skill', + files: sampleFiles(), + spawnId: 'commit-2', + tmpRoot: stagingTmpRoot, + }); + + expect(() => + commitSkill({ name: 'collide-skill', tier: 'global', stagedDir, tiers }), + ).toThrow(/already exists/); + + // Existing skill is untouched. + expect(fs.readFileSync(path.join(tiers.global, 'collide-skill', 'marker.txt'), 'utf-8')).toBe('existing\n'); + // Staged dir is still there (caller decides whether to discard or rename). + expect(fs.existsSync(stagedDir)).toBe(true); + }); + + it('refuses to follow a symlinked staging dir', () => { + const realDir = path.join(tmpRoot, 'real-staging'); + fs.mkdirSync(realDir, { recursive: true }); + fs.writeFileSync(path.join(realDir, 'SKILL.md'), 'fake\n'); + const symlink = path.join(tmpRoot, 'symlinked-staging'); + fs.symlinkSync(realDir, symlink); + + expect(() => + commitSkill({ name: 'sym-skill', tier: 'global', stagedDir: symlink, tiers }), + ).toThrow(/symlink/); + }); + + it('throws when project tier is unresolved', () => { + const stagedDir = stageSkill({ + name: 'test-skill', + files: sampleFiles(), + spawnId: 'commit-3', + tmpRoot: stagingTmpRoot, + }); + + const tiersNoProject: TierPaths = { project: null, global: tiers.global, bundled: tiers.bundled }; + expect(() => + commitSkill({ name: 'test-skill', tier: 'project', stagedDir, tiers: tiersNoProject }), + ).toThrow(/has no resolved path/); + }); + + it('rejects invalid skill names at commit time too', () => { + // Caller could pass a bad name even after a successful stage. + const stagedDir = stageSkill({ + name: 'good-name', + files: sampleFiles(), + spawnId: 'commit-4', + tmpRoot: stagingTmpRoot, + }); + expect(() => + commitSkill({ name: 'BAD/NAME', tier: 'global', stagedDir, tiers }), + ).toThrow(/Invalid skill name/); + }); +}); + +// ─── discardStaged ────────────────────────────────────────────── + +describe('discardStaged', () => { + it('removes the staged dir and the wrapper when no siblings remain', () => { + const stagedDir = stageSkill({ + name: 'test-skill', + files: sampleFiles(), + spawnId: 'discard-1', + tmpRoot: stagingTmpRoot, + }); + const wrapperDir = path.dirname(stagedDir); + expect(fs.existsSync(stagedDir)).toBe(true); + expect(fs.existsSync(wrapperDir)).toBe(true); + + discardStaged(stagedDir); + + expect(fs.existsSync(stagedDir)).toBe(false); + expect(fs.existsSync(wrapperDir)).toBe(false); + }); + + it('is idempotent — safe to call twice', () => { + const stagedDir = stageSkill({ + name: 'test-skill', + files: sampleFiles(), + spawnId: 'discard-2', + tmpRoot: stagingTmpRoot, + }); + discardStaged(stagedDir); + expect(() => discardStaged(stagedDir)).not.toThrow(); + }); + + it('does not nuke unrelated parents when stagedDir is not under a skillify wrapper', () => { + // Synthetic: stagedDir parent is just /tmp/xxx, not skillify-. discardStaged + // should clean the leaf only and leave the parent alone (defense in depth + // against a buggy caller passing a path outside the staging tree). + const lonelyParent = path.join(tmpRoot, 'unrelated-parent'); + const lonelyChild = path.join(lonelyParent, 'leaf'); + fs.mkdirSync(lonelyChild, { recursive: true }); + fs.writeFileSync(path.join(lonelyParent, 'sibling.txt'), 'do not touch\n'); + + discardStaged(lonelyChild); + + expect(fs.existsSync(lonelyChild)).toBe(false); + expect(fs.existsSync(path.join(lonelyParent, 'sibling.txt'))).toBe(true); + expect(fs.existsSync(lonelyParent)).toBe(true); + }); +}); + +// ─── End-to-end failure flow (D3 contract) ────────────────────── + +describe('D3 contract: simulated test failure leaves no on-disk artifact', () => { + it('stage → simulated test fail → discard → no skill at final path', () => { + const stagedDir = stageSkill({ + name: 'failing-skill', + files: sampleFiles(), + spawnId: 'd3-fail-1', + tmpRoot: stagingTmpRoot, + }); + const finalPath = path.join(tiers.global, 'failing-skill'); + + // Simulate $B skill test failing — caller's catch block runs discardStaged. + discardStaged(stagedDir); + + // Final tier path never received the skill. + expect(fs.existsSync(finalPath)).toBe(false); + // Staging is cleaned. + expect(fs.existsSync(stagedDir)).toBe(false); + }); + + it('stage → user rejects in approval gate → discard → no skill at final path', () => { + const stagedDir = stageSkill({ + name: 'rejected-skill', + files: sampleFiles(), + spawnId: 'd3-reject-1', + tmpRoot: stagingTmpRoot, + }); + + // Tests passed but user said no in the approval gate. + discardStaged(stagedDir); + + expect(fs.existsSync(path.join(tiers.global, 'rejected-skill'))).toBe(false); + }); + + it('stage → tests pass → commit succeeds → skill is at final path', () => { + const stagedDir = stageSkill({ + name: 'happy-skill', + files: sampleFiles(), + spawnId: 'd3-happy-1', + tmpRoot: stagingTmpRoot, + }); + const dest = commitSkill({ name: 'happy-skill', tier: 'global', stagedDir, tiers }); + expect(fs.existsSync(dest)).toBe(true); + expect(fs.existsSync(path.join(dest, 'SKILL.md'))).toBe(true); + }); +});