feat(browse-skills): atomic write helper for /skillify (D3)

stageSkill writes a candidate skill into ~/.gstack/.tmp/skillify-<spawnId>/
with restrictive perms. commitSkill does an atomic fs.renameSync into the
final tier path with realpath/lstat discipline (refuses symlinked staging
dirs, refuses to clobber existing skills). discardStaged is the cleanup
path for test failures and approval rejections, idempotent and bounded
to the per-spawn wrapper. validateSkillName enforces lowercase/digits/
dashes only, no path-escape characters.

Implements the D3 contract from the v1.19.0.0 plan review: never a
half-written skill on disk. Test fail or approval reject = rm -rf the
temp dir, no tombstone for never-approved skills.

Closes Codex finding #5 (atomic skill packaging) for Phase 2a.

34 unit assertions covering: stage validation, file-path escape rejection,
permission check, atomic rename, clobber refusal, symlink refusal, project
tier unresolved, idempotent discard, end-to-end happy + simulated test
failure + approval reject paths.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

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-<spawnId>/,
+ * 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<string, string | Buffer>;
+  /** 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:
+ *   <tmpRoot>/.gstack/.tmp/skillify-<spawnId>/<name>/
+ *
+ * The leaf <name> directory is what gets renamed during commit. The wrapper
+ * skillify-<spawnId>/ is per-spawn so concurrent /skillify invocations don't
+ * collide. Returns the absolute path to the staged skill dir (ending in <name>).
+ */
+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-<spawnId>/.
+  // 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-<id> 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)}`;
+}

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-<spawnId>/<name>/
+ *   commitSkill   → atomic rename to <tier-root>/<name>/, 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<string, string | Buffer> {
+  return new Map<string, string | Buffer>([
+    ['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', '<html></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<string, string | Buffer>([
+      ['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-<id>. 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);
+  });
+});