Skip to content

fix(skills): publish runtime skill dirs atomically so no reader sees a tree without SKILL.md (BLO-32167) #1699

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
allyblockcast wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix(skills): publish runtime skill dirs atomically so no reader sees a tree without SKILL.md (BLO-32167) #1699

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
153 changes: 153 additions & 0 deletions server/src/__tests__/company-skills-service.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1841,6 +1841,159 @@ describeEmbeddedPostgres("companySkillService.list", () => {
);
});

// BLO-32167. `materializeRuntimeSkillFiles` used to publish in place:
// `fs.rm(skillDir, {recursive:true})` -> `mkdir` -> per-file `writeFile`. That
// left the *published* path observable in a state where the directory exists
// and `SKILL.md` does not — and unlike the BLO-32055 branch, that state raises
// no syscall error in any reader. `hashPathContents` in the claude-k8s adapter
// hashes it happily and mints a cache key over an unusable tree, so the run is
// silently degraded rather than classified (live capture: CEO run ff67a1b1,
// 2026-09-06T00:35Z, where a pod's copy of `investigate--9debdeaf08` was an
// empty directory while the shared store held all 24,209 bytes).
//
// The assertion is about what a CONCURRENT READER can see, not about how the
// writer is implemented — so it samples the published directory from inside
// `fs.writeFile`, before the write lands, and is agnostic to whether the fix
// stages elsewhere or writes in place.
it("never publishes a runtime skill directory without SKILL.md (BLO-32167)", async () => {
const companyId = randomUUID();
const skillId = randomUUID();
const skillKey = `company/${companyId}/atomic-publish`;
const missingSkillDir = path.join(await fs.mkdtemp(path.join(os.tmpdir(), "paperclip-atomic-publish-")), "gone");
cleanupDirs.add(path.dirname(missingSkillDir));

await db.insert(companies).values({
id: companyId,
name: "Paperclip",
issuePrefix: `T${companyId.replace(/-/g, "").slice(0, 6).toUpperCase()}`,
requireBoardApprovalForNewAgents: false,
});
await db.insert(companySkills).values({
id: skillId,
companyId,
key: skillKey,
slug: "atomic-publish",
name: "Atomic Publish",
description: null,
markdown: "# Atomic Publish\n\nMaterialized from DB.\n",
sourceType: "local_path",
sourceLocator: missingSkillDir,
trustLevel: "markdown_only",
compatibility: "compatible",
fileInventory: [{ path: "SKILL.md", kind: "skill" }],
metadata: { sourceKind: "local_path" },
});
await db.insert(agents).values({
id: randomUUID(),
companyId,
name: "Runner",
role: "engineer",
status: "active",
adapterType: "codex_local",
adapterConfig: { paperclipSkillSync: { desiredSkills: [skillKey] } },
});

// First pass establishes the published tree and tells us its path. The
// sampled window below is therefore a RE-materialization — the case that
// matters, because a reader mid-sweep is reading a skill that already
// worked a moment ago.
const first = await svc.listRuntimeSkillEntries(companyId);
const publishedDir = first.find((candidate) => candidate.key === skillKey)!.source;
await expect(fs.readFile(path.join(publishedDir, "SKILL.md"), "utf8")).resolves.toContain("Atomic Publish");

const samples: Array<{ exists: boolean; hasSkillFile: boolean; entries: string[] }> = [];
const realWriteFile = fs.writeFile;
const spy = vi.spyOn(fs, "writeFile").mockImplementation(async (target, ...rest) => {
const entries = await fs.readdir(publishedDir).catch(() => null);
samples.push({
exists: entries !== null,
hasSkillFile: entries?.includes("SKILL.md") ?? false,
entries: entries ?? [],
});
return (realWriteFile as never)(target, ...rest);
});

try {
const second = await svc.listRuntimeSkillEntries(companyId);
expect(second.find((candidate) => candidate.key === skillKey)).toMatchObject({
key: skillKey,
sourceStatus: "available",
});
} finally {
spy.mockRestore();
}

// Guards a vacuous pass: if the writer stops calling `fs.writeFile`, or the
// second listing stops re-materializing, there is no window being sampled
// and an all-clear below would mean nothing.
expect(samples.length).toBeGreaterThan(0);

// The invariant. The published directory may be absent (the one-syscall gap
// between the two renames — an ENOENT, which every reader already classifies
// as retryable `skill_materialization_pending` per BLO-32055/#1669), or it
// may be a complete tree. "Present but no SKILL.md" is the state that has no
// error for anyone to catch, and it must never be observable.
const degraded = samples.filter((sample) => sample.exists && !sample.hasSkillFile);
expect(degraded).toEqual([]);

await expect(fs.readFile(path.join(publishedDir, "SKILL.md"), "utf8")).resolves.toContain("Atomic Publish");
});

// BLO-32167. The staging and retired trees are siblings of the published
// directory inside `__runtime__`, so a leak would accumulate there forever and
// — worse — could be picked up as a skill by anything that enumerates the
// root. Dot-prefixing is what makes them unmistakable; this asserts both that
// they are cleaned up and that nothing undotted is left behind.
it("leaves no staging or retired trees behind in __runtime__ (BLO-32167)", async () => {
const companyId = randomUUID();
const skillKey = `company/${companyId}/staging-cleanup`;
const missingSkillDir = path.join(await fs.mkdtemp(path.join(os.tmpdir(), "paperclip-staging-cleanup-")), "gone");
cleanupDirs.add(path.dirname(missingSkillDir));

await db.insert(companies).values({
id: companyId,
name: "Paperclip",
issuePrefix: `T${companyId.replace(/-/g, "").slice(0, 6).toUpperCase()}`,
requireBoardApprovalForNewAgents: false,
});
await db.insert(companySkills).values({
id: randomUUID(),
companyId,
key: skillKey,
slug: "staging-cleanup",
name: "Staging Cleanup",
description: null,
markdown: "# Staging Cleanup\n\nMaterialized from DB.\n",
sourceType: "local_path",
sourceLocator: missingSkillDir,
trustLevel: "markdown_only",
compatibility: "compatible",
fileInventory: [{ path: "SKILL.md", kind: "skill" }],
metadata: { sourceKind: "local_path" },
});
await db.insert(agents).values({
id: randomUUID(),
companyId,
name: "Runner",
role: "engineer",
status: "active",
adapterType: "codex_local",
adapterConfig: { paperclipSkillSync: { desiredSkills: [skillKey] } },
});

// Three passes: the first publishes, the rest exercise the retire-and-swap
// path where both a staging and a retired tree exist transiently.
let publishedDir = "";
for (let pass = 0; pass < 3; pass += 1) {
const entries = await svc.listRuntimeSkillEntries(companyId);
publishedDir = entries.find((candidate) => candidate.key === skillKey)!.source;
}

const runtimeRoot = path.dirname(publishedDir);
const leftovers = await fs.readdir(runtimeRoot);
expect(leftovers).toEqual([path.basename(publishedDir)]);
});

it("falls back to stored markdown when reading SKILL.md from a missing local source", async () => {
const companyId = randomUUID();
const skillId = randomUUID();
Expand Down
113 changes: 94 additions & 19 deletions server/src/services/company-skills.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5241,30 +5241,105 @@ export function companySkillService(db: Db) {
};
}

async function materializeRuntimeSkillFiles(companyId: string, skill: CompanySkill) {
const runtimeRoot = path.resolve(resolveManagedSkillsRoot(companyId), "__runtime__");
const skillDir = path.resolve(runtimeRoot, buildSkillRuntimeName(skill.key, skill.slug));
await fs.rm(skillDir, { recursive: true, force: true });
await fs.mkdir(skillDir, { recursive: true });

let wroteSkillFile = false;
for (const entry of skill.fileInventory) {
const normalizedPath = normalizePortablePath(entry.path);
const detail = await readFile(companyId, skill.id, normalizedPath).catch(() => null);
const content = detail?.content ?? (normalizedPath === "SKILL.md" ? skill.markdown : null);
if (content === null) continue;
const targetPath = path.resolve(skillDir, entry.path);
await fs.mkdir(path.dirname(targetPath), { recursive: true });
await fs.writeFile(targetPath, content, "utf8");
if (normalizedPath === "SKILL.md") wroteSkillFile = true;
/**
* Swap a fully-written staging tree into place under `skillDir`.
*
* BLO-32167. The publish must never leave `skillDir` observable in a
* half-written state, because several readers sample it concurrently and none
* of them can tell "still being written" from "this is all there is":
* `resolveExistingSkillDirectory` (the available-skills catalogue),
* `hashPathContents` in the claude-k8s adapter (the prompt-bundle cache key),
* and the per-run copy that snapshots the tree into a pod.
*
* Renaming the outgoing tree aside *before* renaming the new one in is what
* makes that true. The obvious alternative — `rm -rf` the old tree first —
* reintroduces the same defect from the other end: `rm` is itself a walk, so a
* reader landing mid-`rm` sees a shrinking directory and hashes it happily.
*
* The residual exposure is the single instant between the two renames, in
* which `skillDir` does not exist at all. That is deliberate: a *missing*
* source is the branch every reader already handles correctly (`ENOENT` ->
* `ClaudeSkillSourceUnavailableError` -> retryable `skill_materialization_pending`
* per BLO-32055 / #1669), whereas a *present but incomplete* source is the
* silent one. So this trades an unbounded window that fails silently for a
* one-syscall window that fails loudly and self-heals.
*
* "Unbounded" is not rhetorical: the write loop performs a database read per
* inventory entry, so the pre-BLO-32167 window spanned N round-trips.
*/
async function publishStagedRuntimeSkillDir(stagingDir: string, skillDir: string) {
const retiredDir = path.resolve(
path.dirname(skillDir),
`.retired-${path.basename(skillDir)}-${process.pid}-${randomUUID()}`,
);
let retired = false;
try {
await fs.rename(skillDir, retiredDir);
retired = true;
} catch (error) {
// First materialization of this skill: nothing to retire.
if ((error as NodeJS.ErrnoException).code !== "ENOENT") throw error;
}

if (!wroteSkillFile) {
try {
await fs.rename(stagingDir, skillDir);
} catch (error) {
const code = (error as NodeJS.ErrnoException).code;
// A concurrent materializer published between our two renames. Its tree is
// complete by construction, so this is a correctness-preserving collision
// rather than a corrupt state; ours is at least as fresh, so take the slot.
if (code !== "ENOTEMPTY" && code !== "EEXIST") {
if (retired) await fs.rename(retiredDir, skillDir).catch(() => {});
throw error;
}
await fs.rm(skillDir, { recursive: true, force: true });
throw unprocessable("Company skill could not be materialized because its stored SKILL.md copy is missing.");
await fs.rename(stagingDir, skillDir);
} finally {
if (retired) await fs.rm(retiredDir, { recursive: true, force: true }).catch(() => {});
}
}

return skillDir;
async function materializeRuntimeSkillFiles(companyId: string, skill: CompanySkill) {
const runtimeRoot = path.resolve(resolveManagedSkillsRoot(companyId), "__runtime__");
const runtimeName = buildSkillRuntimeName(skill.key, skill.slug);
const skillDir = path.resolve(runtimeRoot, runtimeName);
// Sibling of the destination so the publish below is a rename (atomic, same
// filesystem) rather than a copy. Dot-prefixed because a runtime name never
// starts with a dot, so neither a staging nor a retired tree can be mistaken
// for a skill by anything that enumerates `__runtime__`.
const stagingDir = path.resolve(runtimeRoot, `.staging-${runtimeName}-${process.pid}-${randomUUID()}`);

await fs.mkdir(runtimeRoot, { recursive: true });
await fs.mkdir(stagingDir, { recursive: true });

try {
let wroteSkillFile = false;
for (const entry of skill.fileInventory) {
const normalizedPath = normalizePortablePath(entry.path);
const detail = await readFile(companyId, skill.id, normalizedPath).catch(() => null);
const content = detail?.content ?? (normalizedPath === "SKILL.md" ? skill.markdown : null);
if (content === null) continue;
const targetPath = path.resolve(stagingDir, entry.path);
await fs.mkdir(path.dirname(targetPath), { recursive: true });
await fs.writeFile(targetPath, content, "utf8");
if (normalizedPath === "SKILL.md") wroteSkillFile = true;
}

if (!wroteSkillFile) {
// Deliberately still removes the published tree, exactly as before. A
// skill whose stored SKILL.md has gone must stop being served, and the
// callers rely on that: `resolveRuntimeSkillSource` would otherwise keep
// resolving the stale directory through `resolveExistingSkillDirectory`.
await fs.rm(skillDir, { recursive: true, force: true });
throw unprocessable("Company skill could not be materialized because its stored SKILL.md copy is missing.");
}

await publishStagedRuntimeSkillDir(stagingDir, skillDir);
return skillDir;
} finally {
// No-op on the success path — the staging tree was renamed away.
await fs.rm(stagingDir, { recursive: true, force: true }).catch(() => {});
}
}

function resolveVersionSnapshotPath(skillDir: string, relativePath: string) {
Expand Down
Loading
Loading