fix(cli): ship the #702 guardrail (lost in 0.1.4) + mount skills read-only

cli/__tests__/adapters.claude.environment.test.mjs

@@ -5,7 +5,7 @@
  *   - sandbox.mode='bwrap' → spawn binary is `bwrap`, not `claude`
  *   - mcp[]               → --mcp-config <path> added to inner argv,
  *                            and a mode-0600 temp file outside the workspace
- *   - skills.claude[]     → linkSkills called against the workspace
+ *   - skills.claude[]     → mountSkills called against the workspace
  *
  * Uses ctx._spawnImpl (the same test seam as adapters.claude.test.mjs) so
  * no real claude/bwrap binary runs.
@@ -127,7 +127,7 @@ describe('claude adapter — ctx.environment', () => {
     expect(fs.existsSync(path.dirname(calls[0].configPath))).toBe(false);
   });
 
-  test('symlinks skills.claude entries into <cwd>/.claude/skills/', async () => {
+  test('mounts skills.claude entries into <cwd>/.claude/skills/ as read-only copies', async () => {
     const { impl } = makeSpawnImpl();
     const skillSrc = fs.mkdtempSync(path.join(os.tmpdir(), 'cli-claude-skill-'));
     fs.writeFileSync(path.join(skillSrc, 'SKILL.md'), 'x', 'utf8');
@@ -139,8 +139,12 @@ describe('claude adapter — ctx.environment', () => {
       _spawnImpl: impl,
     });
 
-    const link = path.join(cwd, '.claude', 'skills', path.basename(skillSrc));
-    expect(fs.lstatSync(link).isSymbolicLink()).toBe(true);
+    const slot = path.join(cwd, '.claude', 'skills', path.basename(skillSrc));
+    // A copy, never a symlink — a symlink is writable through, which lets an
+    // agent edit its own governing skill in the operator's checkout (#702
+    // guardrail was lost from cli 0.1.4 exactly this way).
+    expect(fs.lstatSync(slot).isSymbolicLink()).toBe(false);
+    expect(fs.readFileSync(path.join(slot, 'SKILL.md'), 'utf8')).toBe('x');
 
     fs.rmSync(skillSrc, { recursive: true, force: true });
   });

cli/__tests__/environment.test.mjs

@@ -2,7 +2,7 @@
  * environment.test.mjs — ADR-008 Phase 1
  *
  * Covers parseEnvironmentFile, validateEnvironmentSpec, resolveWorkspace,
- * linkSkills. The module is pure I/O against the local FS — we use real temp
+ * mountSkills. The module is pure I/O against the local FS — we use real temp
  * dirs and a mocked `os.homedir` so `~` expansion and the default workspace
  * path land somewhere disposable.
  */
@@ -27,7 +27,7 @@ const {
   parseEnvironmentFile,
   validateEnvironmentSpec,
   resolveWorkspace,
-  linkSkills,
+  mountSkills,
 } = await import('../src/lib/environment.js');
 
 const writeJson = (dir, name, obj) => {
@@ -200,7 +200,7 @@ describe('resolveWorkspace', () => {
   });
 });
 
-describe('linkSkills', () => {
+describe('mountSkills', () => {
   let workspace;
   let skillSrc;
 
@@ -214,85 +214,97 @@ describe('linkSkills', () => {
     fs.rmSync(skillSrc, { recursive: true, force: true });
   });
 
-  test('creates .claude/skills/<basename> symlink to the source dir', async () => {
-    const { linked, skipped, conflicted } = await linkSkills(
+  const slotFor = (src) => path.join(workspace, '.claude', 'skills', path.basename(src));
+
+  test('mounts a real directory copy, not a symlink', async () => {
+    const { mounted, conflicted } = await mountSkills(
       { skills: { claude: [skillSrc] } },
       workspace,
     );
-    expect(linked).toEqual([skillSrc]);
-    expect(skipped).toEqual([]);
+    expect(mounted).toEqual([skillSrc]);
     expect(conflicted).toEqual([]);
 
-    const linkPath = path.join(workspace, '.claude', 'skills', path.basename(skillSrc));
-    expect(fs.lstatSync(linkPath).isSymbolicLink()).toBe(true);
-    expect(fs.readFileSync(path.join(linkPath, 'SKILL.md'), 'utf8')).toBe('# my skill');
+    const slot = slotFor(skillSrc);
+    expect(fs.lstatSync(slot).isSymbolicLink()).toBe(false);
+    expect(fs.lstatSync(slot).isDirectory()).toBe(true);
+    expect(fs.readFileSync(path.join(slot, 'SKILL.md'), 'utf8')).toBe('# my skill');
+  });
+
+  // The bug this function exists to prevent: @commonlyai/cli@0.1.4 shipped
+  // without the #702 "post as yourself" guardrail because an agent wrote
+  // through the old symlink into the operator's checkout, and `npm publish`
+  // ships the working tree rather than git HEAD.
+  test('writes into the mount CANNOT reach the source (regression: cli 0.1.4 guardrail loss)', async () => {
+    await mountSkills({ skills: { claude: [skillSrc] } }, workspace);
+    const slot = slotFor(skillSrc);
+
+    // Simulate an agent rewriting its own governing skill file. It must be
+    // able to affect at most its own copy — never the source of truth.
+    fs.chmodSync(path.join(slot, 'SKILL.md'), 0o644);
+    fs.writeFileSync(path.join(slot, 'SKILL.md'), '# guardrail deleted', 'utf8');
+
+    expect(fs.readFileSync(path.join(skillSrc, 'SKILL.md'), 'utf8')).toBe('# my skill');
+  });
+
+  test('mounted files are read-only (defense in depth)', async () => {
+    await mountSkills({ skills: { claude: [skillSrc] } }, workspace);
+    const mode = fs.statSync(path.join(slotFor(skillSrc), 'SKILL.md')).mode & 0o222;
+    expect(mode).toBe(0);
   });
 
-  test('idempotent: re-running with the same source reports as already-linked', async () => {
-    await linkSkills({ skills: { claude: [skillSrc] } }, workspace);
-    const second = await linkSkills({ skills: { claude: [skillSrc] } }, workspace);
-    expect(second.linked).toEqual([]);
-    expect(second.skipped).toEqual([skillSrc]);
+  test('re-mounting refreshes from source, so source edits still reach the agent', async () => {
+    await mountSkills({ skills: { claude: [skillSrc] } }, workspace);
+    fs.writeFileSync(path.join(skillSrc, 'SKILL.md'), '# updated upstream', 'utf8');
+
+    const second = await mountSkills({ skills: { claude: [skillSrc] } }, workspace);
+    expect(second.mounted).toEqual([skillSrc]);
     expect(second.conflicted).toEqual([]);
+    expect(fs.readFileSync(path.join(slotFor(skillSrc), 'SKILL.md'), 'utf8'))
+      .toBe('# updated upstream');
   });
 
-  test('refuses to overwrite a different existing symlink at the same slot — reports `different-target`', async () => {
-    // Two different source dirs, same basename — second one must surface as
-    // a conflict so the caller can warn the user (NOT silently merged into
-    // skipped, which would mask the user's intent being lost).
-    const otherSrc = fs.mkdtempSync(path.join(path.dirname(skillSrc),
-      `${path.basename(skillSrc).slice(0, -6)}-X-`));
-    const slotName = path.basename(skillSrc);
-    const slotPath = path.join(workspace, '.claude', 'skills', slotName);
-    fs.mkdirSync(path.dirname(slotPath), { recursive: true });
-    fs.symlinkSync(otherSrc, slotPath);
-
-    const { linked, skipped, conflicted } = await linkSkills(
+  test('a legacy symlink from CLI <=0.1.4 is replaced by a snapshot', async () => {
+    const slot = slotFor(skillSrc);
+    fs.mkdirSync(path.dirname(slot), { recursive: true });
+    fs.symlinkSync(skillSrc, slot);
+
+    const { mounted, conflicted } = await mountSkills(
       { skills: { claude: [skillSrc] } },
       workspace,
     );
-    expect(linked).toEqual([]);
-    expect(skipped).toEqual([]);
-    expect(conflicted).toEqual([{ path: skillSrc, reason: 'different-target' }]);
-    // Existing link untouched.
-    expect(fs.readlinkSync(slotPath)).toBe(otherSrc);
-
-    fs.rmSync(otherSrc, { recursive: true, force: true });
+    expect(mounted).toEqual([skillSrc]);
+    expect(conflicted).toEqual([]);
+    expect(fs.lstatSync(slot).isSymbolicLink()).toBe(false);
+    // The source survived the replacement (rm must not follow the symlink).
+    expect(fs.readFileSync(path.join(skillSrc, 'SKILL.md'), 'utf8')).toBe('# my skill');
   });
 
   test('returns empty buckets when no skills are declared', async () => {
-    expect(await linkSkills({}, workspace)).toEqual({
-      linked: [], skipped: [], conflicted: [],
-    });
+    expect(await mountSkills({}, workspace)).toEqual({ mounted: [], conflicted: [] });
   });
 
   test('non-existent source paths surface as `missing-source` conflicts (not silent skips)', async () => {
     const ghost = path.join(skillSrc, 'does-not-exist');
-    const { linked, skipped, conflicted } = await linkSkills(
+    const { mounted, conflicted } = await mountSkills(
       { skills: { claude: [ghost] } },
       workspace,
     );
-    expect(linked).toEqual([]);
-    expect(skipped).toEqual([]);
+    expect(mounted).toEqual([]);
     expect(conflicted).toEqual([{ path: ghost, reason: 'missing-source' }]);
   });
 
-  test('non-symlink occupants (real files/dirs) surface as `not-symlink` conflicts', async () => {
-    // A user might have hand-created a real dir at the slot — never overwrite.
-    const slotName = path.basename(skillSrc);
-    const slotPath = path.join(workspace, '.claude', 'skills', slotName);
-    fs.mkdirSync(path.dirname(slotPath), { recursive: true });
-    fs.mkdirSync(slotPath);
-    fs.writeFileSync(path.join(slotPath, 'hand-edited.md'), 'mine', 'utf8');
+  test('a user-authored dir at the slot is never clobbered — `not-a-mount`', async () => {
+    // No mount marker => we did not create it => it is real user work.
+    const slot = slotFor(skillSrc);
+    fs.mkdirSync(slot, { recursive: true });
+    fs.writeFileSync(path.join(slot, 'hand-edited.md'), 'mine', 'utf8');
 
-    const { linked, skipped, conflicted } = await linkSkills(
+    const { mounted, conflicted } = await mountSkills(
       { skills: { claude: [skillSrc] } },
       workspace,
     );
-    expect(linked).toEqual([]);
-    expect(skipped).toEqual([]);
-    expect(conflicted).toEqual([{ path: skillSrc, reason: 'not-symlink' }]);
-    // Hand-edited file untouched.
-    expect(fs.existsSync(path.join(slotPath, 'hand-edited.md'))).toBe(true);
+    expect(mounted).toEqual([]);
+    expect(conflicted).toEqual([{ path: skillSrc, reason: 'not-a-mount' }]);
+    expect(fs.readFileSync(path.join(slot, 'hand-edited.md'), 'utf8')).toBe('mine');
   });
 });

cli/__tests__/skill-sync.test.mjs

@@ -0,0 +1,49 @@
+/**
+ * Guard: the two checked-in copies of the commonly behaviour skill must stay
+ * byte-identical.
+ *
+ * There are two tracked copies of the same file:
+ *   - docs/agents/skills/commonly/SKILL.md — what README + docs-site tell users
+ *     to drop into their own agent's skills directory
+ *   - cli/skills/commonly/SKILL.md         — what ships inside the npm package
+ *
+ * `prepublishOnly` in cli/package.json copies the docs one OVER the cli one at
+ * publish time, which quietly makes docs/ the real source of truth. So an edit
+ * applied to only the cli copy is not merely duplicated-and-drifted — it is
+ * ERASED by the next publish, and the tarball ships without it.
+ *
+ * That is not hypothetical. PR #702 added the "post as yourself, never as your
+ * operator" guardrail to the cli copy only. @commonlyai/cli@0.1.4 therefore
+ * shipped with the guardrail missing, and every agent attached with it was
+ * instructed without the rule the PR existed to add.
+ *
+ * This test fails the moment the copies diverge, so the drift surfaces in CI
+ * instead of silently at publish.
+ */
+import fs from 'fs';
+import path from 'path';
+import { fileURLToPath } from 'url';
+
+const here = path.dirname(fileURLToPath(import.meta.url));
+const repoRoot = path.resolve(here, '..', '..');
+
+const PACKAGED = path.join(repoRoot, 'cli', 'skills', 'commonly', 'SKILL.md');
+const DOCS = path.join(repoRoot, 'docs', 'agents', 'skills', 'commonly', 'SKILL.md');
+
+describe('commonly skill copies stay in sync', () => {
+  test('both copies exist', () => {
+    expect(fs.existsSync(PACKAGED)).toBe(true);
+    expect(fs.existsSync(DOCS)).toBe(true);
+  });
+
+  test('packaged copy is byte-identical to the docs copy that prepublishOnly overwrites it with', () => {
+    // Compared as text so a failure prints a readable diff rather than a
+    // buffer-length mismatch.
+    expect(fs.readFileSync(PACKAGED, 'utf8')).toBe(fs.readFileSync(DOCS, 'utf8'));
+  });
+
+  test('the #702 attribution guardrail is present (it was lost in 0.1.4)', () => {
+    const body = fs.readFileSync(DOCS, 'utf8');
+    expect(body).toContain('Post as yourself, never as your operator');
+  });
+});

cli/package.json

@@ -1,6 +1,6 @@
 {
   "name": "@commonlyai/cli",
-  "version": "0.1.4",
+  "version": "0.1.5",
   "license": "Apache-2.0",
   "description": "The Commonly CLI — connect agents, manage pods, iterate fast",
   "type": "module",

cli/src/commands/agent.js

@@ -175,7 +175,7 @@ export const buildDefaultEnvironment = (adapterName) => {
     ],
   };
   // Only advertise the skill if it actually shipped (defensive: a broken
-  // package that dropped the skills/ dir shouldn't hand linkSkills a
+  // package that dropped the skills/ dir shouldn't hand mountSkills a
   // missing-source path every spawn).
   if (existsSync(BUNDLED_COMMONLY_SKILL_DIR)) {
     environment.skills = { claude: [BUNDLED_COMMONLY_SKILL_DIR] };

cli/src/lib/adapters/claude.js

@@ -60,7 +60,7 @@ import {
 import { homedir, tmpdir } from 'os';
 import { isAbsolute, join } from 'path';
 
-import { linkSkills } from '../environment.js';
+import { mountSkills } from '../environment.js';
 import { wrapArgvWithBwrap } from '../sandbox/bwrap.js';
 import {
   publicClaudeStateRoot,
@@ -468,11 +468,11 @@ export default {
     const baseArgs = ['-p', fullPrompt, '--output-format', 'text', sessionFlag, sessionId];
 
     if (ctx.environment && ctx.cwd) {
-      const skills = await linkSkills(ctx.environment, ctx.cwd);
+      const skills = await mountSkills(ctx.environment, ctx.cwd);
       if (skills.conflicted.length > 0) {
         for (const c of skills.conflicted) {
           // eslint-disable-next-line no-console
-          console.warn(`[claude] skill not linked (${c.reason}): ${c.path}`);
+          console.warn(`[claude] skill not mounted (${c.reason}): ${c.path}`);
         }
       }
       if (ctx.onSkillsLinked) ctx.onSkillsLinked(skills);