mirror of
https://github.com/nexu-io/open-design.git
synced 2026-10-27 09:51:12 +01:00
fix(daemon): expose skill resources via cwd-relative aliases (#435)
* fix(daemon): expose skill resources via cwd-relative aliases (#430) Skill bodies referenced repository-absolute paths in their preamble and relied on every agent CLI honouring `--add-dir` (or its equivalent) to open them. Most agents (codex, gemini, opencode, cursor-agent, qwen, pi, hermes, kimi) have no such flag, and Claude Code's directory-access policy can still block these reads under specific permission/trust configurations even when `--add-dir` is set, so SKILL side files fail to load mid-turn. Stage `.od-skills` and `.od-design-systems` directory links inside the project cwd before spawning the agent, and rewrite the skill preamble to address resources via the cwd-relative alias path. The alias resolves through a directory junction (Windows) or symbolic link (POSIX), so resources are reachable from inside every agent's working directory without needing an extra-allowed-directories flag. `extraAllowedDirs` is kept as a belt-and-suspenders fallback for Claude/Copilot — if the alias cannot be created (e.g. the user has a pre-existing real entry under one of the reserved names), `--add-dir` still covers the absolute paths. `ensureCwdAliases` is idempotent, repoints when the target moves, and refuses to overwrite a non-symlink entry under the reserved name to avoid clobbering user data. Closes #430. * fix(daemon): address review feedback — copy active skill, dual preamble paths Round 2 of PR #435 addressing the unresolved P1/P2/P3 review threads from @chatgpt-codex-connector, @lefarcen and @mrcfps. - P1 writable-symlink. Replace the directory link with a per-project `fs.cp` of the active skill into `<cwd>/.od-skills/<folder>/`. Writes through the staged copy no longer mutate the shipped repo resource. A regression test pins this down so a future "optimisation" that re-introduces a symlink would fail loud. - P1 absolute fallback. Rewrite the skill preamble to advertise both the cwd-relative alias path (primary) and the absolute path (fallback). When `stageActiveSkill` is skipped (no project cwd) or errors, Claude/Copilot still reach the resource via the absolute path covered by `--add-dir`; other agents that fall back to `PROJECT_ROOT` as cwd reach it natively because the absolute path is in-cwd there. - P2 symlinked source root. Use `stat()` instead of `lstat()` on the source so a symlinked `SKILLS_DIR` or skill folder is followed rather than skipped. - P2 stale ordering. `ensureCwdAliases` is gone; staging now happens independently of prompt composition and never silently diverges from what the preamble advertises. - P3 unsafe spec name. Validate `folderName` against an explicit segment policy (no path separators, no dot segments, no absolute, no null bytes) so a malformed caller cannot escape the alias root. - P3 strong types. Drop the `@ts-nocheck` pragma on `cwd-aliases.ts` and type the public API (`SkillStagingResult`, `SkillStagingLogger`). Also drops the `.od-design-systems` alias entirely. Design-system bodies are read by the daemon and folded into the system prompt directly, so an agent never has to open them through the filesystem; exposing that tree as cwd-visible was unnecessary surface area. Test additions: 17 cases in `cwd-aliases.test.ts` covering the copy semantics, the write-barrier regression, symlinked-source handling, legacy-symlink upgrade, alias-root collision refusal, cwd=null guard, and a parameterised set of unsafe folder names. * fix(daemon): unify effectiveCwd and use junctions on Windows test fixtures Round 3 of PR #435 addressing two non-blocking follow-ups from @mrcfps: - server.ts: collapse the chat-handler `cwd` resolution into a single `effectiveCwd = cwd ?? PROJECT_ROOT` and feed it consistently to `buildArgs`, `spawn`, and the ACP/Pi RPC session adapters. Previously `spawn(... { cwd: cwd || undefined })` let no-project runs inherit whatever working directory the daemon process was started from (packaged daemons / service launches do not start in the workspace root), which made the "absolute path is in-cwd" guarantee in the new skill preamble untrue for that exact case the comment claimed to cover. With `effectiveCwd`, agents in no-project mode land in `PROJECT_ROOT` and the absolute fallback path emitted by the preamble is genuinely a relative-to-cwd path. - cwd-aliases.test.ts: replace `symlinkSync(..., 'dir')` fixtures with a platform-aware `dirLinkType` (`'junction'` on Windows, `'dir'` elsewhere). Directory symlinks on Windows require SeCreateSymbolicLinkPrivilege / Developer Mode, which is not enabled on most CI images, so the previous form would fail the daemon test suite on Windows runners even though the production code (which uses `fs.cp`) does not need symlinks at all.
This commit is contained in:
Xinmin Zeng
@@ -0,0 +1,254 @@
|
||||
import {
|
||||
lstatSync,
|
||||
mkdirSync,
|
||||
mkdtempSync,
|
||||
readFileSync,
|
||||
symlinkSync,
|
||||
writeFileSync,
|
||||
} from 'node:fs';
|
||||
import { realpath } from 'node:fs/promises';
|
||||
import { tmpdir } from 'node:os';
|
||||
import path from 'node:path';
|
||||
import { describe, expect, it } from 'vitest';
|
||||
import { SKILLS_CWD_ALIAS, stageActiveSkill } from '../src/cwd-aliases.js';
|
||||
|
||||
function fresh(): string {
|
||||
return mkdtempSync(path.join(tmpdir(), 'od-skill-stage-'));
|
||||
}
|
||||
|
||||
// On Windows, `fs.symlink(target, link, 'dir')` requires
|
||||
// SeCreateSymbolicLinkPrivilege / Developer Mode and fails on most CI
|
||||
// images. `'junction'` is the directory-only equivalent that does not
|
||||
// require elevated privileges, so we use it for fixtures so the daemon
|
||||
// suite stays green on Windows runners.
|
||||
const dirLinkType: 'dir' | 'junction' =
|
||||
process.platform === 'win32' ? 'junction' : 'dir';
|
||||
|
||||
function writeSampleSkill(root: string, folder: string): string {
|
||||
const dir = path.join(root, folder);
|
||||
mkdirSync(path.join(dir, 'assets'), { recursive: true });
|
||||
mkdirSync(path.join(dir, 'references'), { recursive: true });
|
||||
writeFileSync(path.join(dir, 'SKILL.md'), '# original SKILL\n');
|
||||
writeFileSync(
|
||||
path.join(dir, 'assets', 'template.html'),
|
||||
'<html>original</html>',
|
||||
);
|
||||
writeFileSync(path.join(dir, 'references', 'checklist.md'), '- original');
|
||||
return dir;
|
||||
}
|
||||
|
||||
describe('stageActiveSkill', () => {
|
||||
it('exposes the documented alias name so the skill preamble stays in sync', () => {
|
||||
expect(SKILLS_CWD_ALIAS).toBe('.od-skills');
|
||||
});
|
||||
|
||||
it('stages a per-project copy under <cwd>/.od-skills/<folder>/', async () => {
|
||||
const fs = fresh();
|
||||
const cwd = path.join(fs, 'project');
|
||||
const sourceRoot = path.join(fs, 'skills');
|
||||
const sourceDir = writeSampleSkill(sourceRoot, 'blog-post');
|
||||
mkdirSync(cwd);
|
||||
|
||||
const result = await stageActiveSkill(cwd, 'blog-post', sourceDir);
|
||||
|
||||
expect(result.staged).toBe(true);
|
||||
expect(result.stagedPath).toBe(
|
||||
path.join(cwd, SKILLS_CWD_ALIAS, 'blog-post'),
|
||||
);
|
||||
expect(
|
||||
readFileSync(
|
||||
path.join(result.stagedPath!, 'SKILL.md'),
|
||||
'utf8',
|
||||
),
|
||||
).toContain('original SKILL');
|
||||
expect(
|
||||
readFileSync(
|
||||
path.join(result.stagedPath!, 'assets', 'template.html'),
|
||||
'utf8',
|
||||
),
|
||||
).toContain('original');
|
||||
});
|
||||
|
||||
it('produces a real directory entry, not a symlink (write barrier)', async () => {
|
||||
const fs = fresh();
|
||||
const cwd = path.join(fs, 'project');
|
||||
const sourceDir = writeSampleSkill(path.join(fs, 'skills'), 'blog-post');
|
||||
mkdirSync(cwd);
|
||||
|
||||
await stageActiveSkill(cwd, 'blog-post', sourceDir);
|
||||
|
||||
const stagedSkill = path.join(cwd, SKILLS_CWD_ALIAS, 'blog-post');
|
||||
expect(lstatSync(stagedSkill).isSymbolicLink()).toBe(false);
|
||||
expect(lstatSync(stagedSkill).isDirectory()).toBe(true);
|
||||
|
||||
const stagedFile = path.join(stagedSkill, 'SKILL.md');
|
||||
expect(lstatSync(stagedFile).isSymbolicLink()).toBe(false);
|
||||
expect(lstatSync(stagedFile).isFile()).toBe(true);
|
||||
});
|
||||
|
||||
it('REGRESSION: writes through the staged copy do not mutate the source', async () => {
|
||||
// This is the P1 vulnerability lefarcen flagged on PR #435 round 1:
|
||||
// when `.od-skills` was a directory junction, an agent could
|
||||
// `Edit`/`Write` through the alias and overwrite the shipped repo
|
||||
// resource. The per-project copy is the structural fix; this test
|
||||
// pins it down so a future "optimisation" that re-introduces a
|
||||
// symlink would fail loud.
|
||||
const fs = fresh();
|
||||
const cwd = path.join(fs, 'project');
|
||||
const sourceDir = writeSampleSkill(path.join(fs, 'skills'), 'blog-post');
|
||||
mkdirSync(cwd);
|
||||
|
||||
await stageActiveSkill(cwd, 'blog-post', sourceDir);
|
||||
|
||||
const stagedSkillMd = path.join(
|
||||
cwd,
|
||||
SKILLS_CWD_ALIAS,
|
||||
'blog-post',
|
||||
'SKILL.md',
|
||||
);
|
||||
writeFileSync(stagedSkillMd, '# AGENT MUTATED');
|
||||
|
||||
expect(readFileSync(path.join(sourceDir, 'SKILL.md'), 'utf8')).toContain(
|
||||
'original SKILL',
|
||||
);
|
||||
expect(readFileSync(stagedSkillMd, 'utf8')).toContain('AGENT MUTATED');
|
||||
});
|
||||
|
||||
it('replaces a previous stage so removed files are not left behind', async () => {
|
||||
const fs = fresh();
|
||||
const cwd = path.join(fs, 'project');
|
||||
const sourceDir = writeSampleSkill(path.join(fs, 'skills'), 'blog-post');
|
||||
mkdirSync(cwd);
|
||||
await stageActiveSkill(cwd, 'blog-post', sourceDir);
|
||||
const stale = path.join(cwd, SKILLS_CWD_ALIAS, 'blog-post', 'stale.md');
|
||||
writeFileSync(stale, 'should be wiped on next stage');
|
||||
|
||||
await stageActiveSkill(cwd, 'blog-post', sourceDir);
|
||||
|
||||
expect(() => readFileSync(stale)).toThrow();
|
||||
});
|
||||
|
||||
it('follows a symlinked source root via stat() instead of skipping it', async () => {
|
||||
const fs = fresh();
|
||||
const cwd = path.join(fs, 'project');
|
||||
const realRoot = path.join(fs, 'skills-real');
|
||||
const linkedRoot = path.join(fs, 'skills');
|
||||
const realSkill = writeSampleSkill(realRoot, 'blog-post');
|
||||
symlinkSync(realRoot, linkedRoot, dirLinkType);
|
||||
mkdirSync(cwd);
|
||||
|
||||
const result = await stageActiveSkill(
|
||||
cwd,
|
||||
'blog-post',
|
||||
// simulate the daemon resolving SKILLS_DIR through a symlinked
|
||||
// mount.
|
||||
path.join(linkedRoot, 'blog-post'),
|
||||
);
|
||||
|
||||
expect(result.staged).toBe(true);
|
||||
expect(
|
||||
readFileSync(
|
||||
path.join(result.stagedPath!, 'SKILL.md'),
|
||||
'utf8',
|
||||
),
|
||||
).toContain('original SKILL');
|
||||
void realSkill;
|
||||
});
|
||||
|
||||
it('upgrades a legacy symlink left by an earlier daemon to a real directory', async () => {
|
||||
const fs = fresh();
|
||||
const cwd = path.join(fs, 'project');
|
||||
const sourceDir = writeSampleSkill(path.join(fs, 'skills'), 'blog-post');
|
||||
mkdirSync(cwd);
|
||||
// Earlier daemon versions staged the alias root as a directory link
|
||||
// that pointed at SKILLS_DIR. Make sure the new staging logic
|
||||
// detects and replaces that without panicking.
|
||||
symlinkSync(path.dirname(sourceDir), path.join(cwd, SKILLS_CWD_ALIAS), dirLinkType);
|
||||
|
||||
const messages: string[] = [];
|
||||
const result = await stageActiveSkill(
|
||||
cwd,
|
||||
'blog-post',
|
||||
sourceDir,
|
||||
(m) => messages.push(m),
|
||||
);
|
||||
|
||||
expect(result.staged).toBe(true);
|
||||
expect(
|
||||
lstatSync(path.join(cwd, SKILLS_CWD_ALIAS)).isSymbolicLink(),
|
||||
).toBe(false);
|
||||
expect(messages.some((m) => m.includes('replacing legacy symlink'))).toBe(
|
||||
true,
|
||||
);
|
||||
});
|
||||
|
||||
it('refuses to stage when the alias root is a regular file', async () => {
|
||||
const fs = fresh();
|
||||
const cwd = path.join(fs, 'project');
|
||||
const sourceDir = writeSampleSkill(path.join(fs, 'skills'), 'blog-post');
|
||||
mkdirSync(cwd);
|
||||
writeFileSync(path.join(cwd, SKILLS_CWD_ALIAS), 'user-content');
|
||||
|
||||
const messages: string[] = [];
|
||||
const result = await stageActiveSkill(
|
||||
cwd,
|
||||
'blog-post',
|
||||
sourceDir,
|
||||
(m) => messages.push(m),
|
||||
);
|
||||
|
||||
expect(result.staged).toBe(false);
|
||||
expect(result.reason).toMatch(/non-directory/);
|
||||
expect(
|
||||
readFileSync(path.join(cwd, SKILLS_CWD_ALIAS), 'utf8'),
|
||||
).toBe('user-content');
|
||||
expect(messages.some((m) => m.includes('refusing to stage'))).toBe(true);
|
||||
});
|
||||
|
||||
it('skips silently when the source directory does not exist', async () => {
|
||||
const fs = fresh();
|
||||
const cwd = path.join(fs, 'project');
|
||||
mkdirSync(cwd);
|
||||
|
||||
const result = await stageActiveSkill(
|
||||
cwd,
|
||||
'blog-post',
|
||||
path.join(fs, 'skills', 'missing'),
|
||||
);
|
||||
|
||||
expect(result.staged).toBe(false);
|
||||
expect(result.reason).toMatch(/source missing/);
|
||||
});
|
||||
|
||||
it('returns false without throwing when cwd is null', async () => {
|
||||
const result = await stageActiveSkill(
|
||||
null,
|
||||
'blog-post',
|
||||
'/does/not/matter',
|
||||
);
|
||||
expect(result.staged).toBe(false);
|
||||
expect(result.reason).toBe('no project cwd');
|
||||
});
|
||||
|
||||
it.each([
|
||||
['', 'unsafe folder name'],
|
||||
['.', 'unsafe folder name'],
|
||||
['..', 'unsafe folder name'],
|
||||
['../escape', 'unsafe folder name'],
|
||||
['nested/path', 'unsafe folder name'],
|
||||
['back\\slash', 'unsafe folder name'],
|
||||
['/abs/path', 'unsafe folder name'],
|
||||
])(
|
||||
'rejects unsafe folder name %j to keep the alias root sealed',
|
||||
async (folder, expectedReason) => {
|
||||
const fs = fresh();
|
||||
const cwd = path.join(fs, 'project');
|
||||
mkdirSync(cwd);
|
||||
|
||||
const result = await stageActiveSkill(cwd, folder, '/anywhere');
|
||||
|
||||
expect(result.staged).toBe(false);
|
||||
expect(result.reason).toContain(expectedReason);
|
||||
},
|
||||
);
|
||||
});
|
||||
@@ -0,0 +1,108 @@
|
||||
import { mkdirSync, mkdtempSync, writeFileSync } from 'node:fs';
|
||||
import { tmpdir } from 'node:os';
|
||||
import path from 'node:path';
|
||||
import { describe, expect, it } from 'vitest';
|
||||
import { listSkills } from '../src/skills.js';
|
||||
import { SKILLS_CWD_ALIAS } from '../src/cwd-aliases.js';
|
||||
|
||||
function fresh(): string {
|
||||
return mkdtempSync(path.join(tmpdir(), 'od-skills-'));
|
||||
}
|
||||
|
||||
function writeSkill(
|
||||
root: string,
|
||||
folder: string,
|
||||
options: {
|
||||
name?: string;
|
||||
description?: string;
|
||||
body?: string;
|
||||
withAttachments?: boolean;
|
||||
} = {},
|
||||
) {
|
||||
const dir = path.join(root, folder);
|
||||
mkdirSync(dir, { recursive: true });
|
||||
const fm = [
|
||||
'---',
|
||||
`name: ${options.name ?? folder}`,
|
||||
`description: ${options.description ?? 'A test skill.'}`,
|
||||
'---',
|
||||
'',
|
||||
options.body ?? '# Test skill body',
|
||||
'',
|
||||
].join('\n');
|
||||
writeFileSync(path.join(dir, 'SKILL.md'), fm);
|
||||
if (options.withAttachments) {
|
||||
mkdirSync(path.join(dir, 'assets'), { recursive: true });
|
||||
writeFileSync(
|
||||
path.join(dir, 'assets', 'template.html'),
|
||||
'<html><body>seed</body></html>',
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
describe('listSkills preamble', () => {
|
||||
it('emits both a cwd-relative skill root and an absolute fallback', async () => {
|
||||
const root = fresh();
|
||||
writeSkill(root, 'demo-skill', {
|
||||
withAttachments: true,
|
||||
body: 'Use `assets/template.html` to bootstrap.',
|
||||
});
|
||||
|
||||
const skills = await listSkills(root);
|
||||
expect(skills).toHaveLength(1);
|
||||
const [skill] = skills;
|
||||
|
||||
// The cwd-relative alias path is the primary one — that's what makes
|
||||
// the agent stay inside its working directory when reading skill
|
||||
// side files (issue #430).
|
||||
expect(skill.body).toContain(`${SKILLS_CWD_ALIAS}/demo-skill/`);
|
||||
expect(skill.body).toContain(
|
||||
`${SKILLS_CWD_ALIAS}/demo-skill/assets/template.html`,
|
||||
);
|
||||
|
||||
// The absolute fallback is required for two cases the relative path
|
||||
// cannot serve:
|
||||
// - calls without a project (cwd defaults to PROJECT_ROOT, where
|
||||
// the absolute path is in fact an in-cwd path);
|
||||
// - environments where `stageActiveSkill()` failed.
|
||||
// Claude/Copilot are additionally given `--add-dir` for that path.
|
||||
expect(skill.body).toContain(skill.dir);
|
||||
expect(skill.body).toMatch(/Skill root \(absolute fallback\)/);
|
||||
expect(skill.body).toMatch(/Skill root \(relative to project\)/);
|
||||
});
|
||||
|
||||
it('uses the on-disk folder name in the alias path even when `name` differs', async () => {
|
||||
const root = fresh();
|
||||
writeSkill(root, 'guizang-ppt', {
|
||||
name: 'magazine-web-ppt',
|
||||
withAttachments: true,
|
||||
});
|
||||
|
||||
const skills = await listSkills(root);
|
||||
expect(skills).toHaveLength(1);
|
||||
const [skill] = skills;
|
||||
|
||||
// `id`/`name` reflect the frontmatter value (used elsewhere as a stable
|
||||
// public id), but the on-disk alias path must use the actual folder
|
||||
// name — that is what the daemon-staged junction maps to.
|
||||
expect(skill.id).toBe('magazine-web-ppt');
|
||||
expect(skill.body).toContain(`${SKILLS_CWD_ALIAS}/guizang-ppt/`);
|
||||
expect(skill.body).not.toContain(`${SKILLS_CWD_ALIAS}/magazine-web-ppt/`);
|
||||
});
|
||||
|
||||
it('does not emit a preamble for skills without side files', async () => {
|
||||
const root = fresh();
|
||||
writeSkill(root, 'lone-skill', {
|
||||
withAttachments: false,
|
||||
body: 'Body without external files.',
|
||||
});
|
||||
|
||||
const skills = await listSkills(root);
|
||||
expect(skills).toHaveLength(1);
|
||||
const [skill] = skills;
|
||||
|
||||
expect(skill.body).not.toContain(SKILLS_CWD_ALIAS);
|
||||
expect(skill.body).not.toContain('Skill root');
|
||||
expect(skill.body).toContain('Body without external files.');
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user