From 609ed4f0b965e1d36f8c60a6dcf7a0653447bff3 Mon Sep 17 00:00:00 2001 From: CTO Date: Mon, 7 Sep 2026 05:31:50 +0000 Subject: [PATCH] fix(skills): publish runtime skill dirs atomically so no reader sees a tree without SKILL.md (BLO-32167) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `materializeRuntimeSkillFiles` published in place — `fs.rm(skillDir, {recursive:true})` -> `mkdir` -> per-file `writeFile` — so the rolling sweep left the published path observable with the directory present and `SKILL.md` absent. Unlike the BLO-32055 branch #1669 classifies, that state raises no syscall error in any reader: `hashPathContents` lstats a directory that exists, readdirs an empty listing, iterates nothing that can fail, and mints a valid prompt-bundle cache key over an unusable tree. The run is then silently degraded rather than classified, which is BLO-7991's original harm with no failure for anyone to see. Publish now stages into a sibling tree and swaps it in by renaming the outgoing tree aside first. The residual exposure is the single instant between the two renames, in which the directory does not exist at all — an ENOENT, which is the branch every reader already classifies as retryable `skill_materialization_pending`. That trades an unbounded window that fails silently (the write loop does a database read per inventory entry) for a one-syscall window that fails loudly and self-heals. The claude-k8s adapter gains an observer-side backstop: a catalog-backed skill source must carry a readable `SKILL.md` at its root. Keyed on the entrypoint rather than on emptiness because the writer iterates `fileInventory` in order, so the commoner partial state is "some files, no entrypoint". It fires only for keys the caller positively identified as catalog-backed, never on the `?? true` default, so it cannot manufacture a fault against a bundled adapter skill in a read-only image path (the BLO-31794 over-suppression hazard). Refs BLO-32167, BLO-32055, BLO-7991, BLO-31794 --- .../__tests__/company-skills-service.test.ts | 153 +++++++++++++++ server/src/services/company-skills.ts | 113 +++++++++-- .../PROVENANCE.md | 4 +- .../package-lock.json | 4 +- .../paperclip-adapter-claude-k8s/package.json | 2 +- .../src/server/prompt-cache.test.ts | 179 ++++++++++++++++++ .../src/server/prompt-cache.ts | 99 +++++++++- 7 files changed, 523 insertions(+), 31 deletions(-) diff --git a/server/src/__tests__/company-skills-service.test.ts b/server/src/__tests__/company-skills-service.test.ts index 390f3e71f919..aee8efd62cf3 100644 --- a/server/src/__tests__/company-skills-service.test.ts +++ b/server/src/__tests__/company-skills-service.test.ts @@ -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(); diff --git a/server/src/services/company-skills.ts b/server/src/services/company-skills.ts index 5739f3652541..c0fe769667ad 100644 --- a/server/src/services/company-skills.ts +++ b/server/src/services/company-skills.ts @@ -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) { diff --git a/vendor/paperclip-adapter-claude-k8s/PROVENANCE.md b/vendor/paperclip-adapter-claude-k8s/PROVENANCE.md index 264e04300e80..64fa669c2b7f 100644 --- a/vendor/paperclip-adapter-claude-k8s/PROVENANCE.md +++ b/vendor/paperclip-adapter-claude-k8s/PROVENANCE.md @@ -96,7 +96,7 @@ A manifest of `sha256(path)` over all 40 in-tree files, sorted by path under `LC_ALL=C`, itself hashes to: ``` -2de474bd5779272abf8686f3b4fad5b7fa90d1c7f658630f4437962336bb56f8 +dc8b199f975ae9d2af82a520f6b9b1bdd6e39dc86949826ef8e2a4cece8439e2 ``` Regenerate with: @@ -169,6 +169,8 @@ upstream**, so they are enumerated here rather than left implicit. | [BLO-31794](https://paperclip.blockcast.net/BLO/issues/BLO-31794) | `src/server/parse.ts`, `src/server/parse.test.ts` | Inverted that guard from a **blocklist** of stream-json event types to an **allowlist** of harness-authored ones, so an unrecognised type fails closed. The row above widened the same predicate twice for one reason: `parseClaudeStreamJson` branches on exactly three types (`system`+`init`, `assistant`, `result`) and ignores every other one, so each newly-appearing event shape slipped a role-blocklist by default. The hazard was one config edit from live rather than hypothetical — `job-manifest.ts:1256` appends `config.extraArgs` to the CLI argv verbatim (`:1125`, from an agent's `adapterConfig`), so `--include-partial-messages` on any single agent re-opens the guard with no code change, no diff and no review. Measured on the CLI this adapter runs (v2.1.210): that flag emits 9 `stream_event`s for a two-word prompt, each wrapping model prose in `event.delta.text_delta`, and `stream_event` was enumerated by no previous version of the guard. The allowlist is `{system, rate_limit_event}` with a stated membership criterion (payload must be entirely harness-authored scalars); `result` is deliberately excluded because a *truncated* one can reach the scan carrying the model's final message. Scoped per line and reading only the first `"type"` per line, so a nested type cannot veto its own line — measured as defence-in-depth rather than a live fix, since a real 1717-byte `init` line carries exactly ONE `"type"` (its `mcp_servers` entries are `{name, status}`, `output_style` a bare string). Detection is unchanged: the four existing guard cases pass unmodified, plus new cases for a production-shaped `init` line, `system:status` (which v2.1.210 emits pre-turn under `--include-partial-messages`), and `rate_limit_event` (the FAR-32 repro in `execute.test.ts`). Verified as a negative control — the new `stream_event` case FAILS against the previous blocklist while all 12 detection-preserving cases pass, so it discriminates the fix rather than merely passing alongside it. **Ally review follow-up on [#1650](https://github.com/Blockcast/paperclip/pull/1650):** the allowlist reaches the `system` *subtype* rather than admitting the type wholesale, because `system` is a multiplexer and admitting it whole reproduced this same defect one level down. Measured against the v2.1.210 binary, `system` carries at least `init`, `status`, `compact_boundary`, `hook_started`, `hook_response` and `mcp_status`; only the first two are admitted. `hook_response` is why this is a live hole rather than future-proofing — the binary builds it as `{type:"system",subtype:"hook_response",…,output,stdout,stderr}`, embedding a hook process's raw stdout, which is operator-configured. It is not merely reachable via `--settings`/`extraArgs`: **Paperclip provisions hooks itself**, and real pod logs on this instance carry a `SessionStart` `hook_response` whose `output` is an operator status message, and another whose `stdout` is an nginx 503 HTML page. (`hook_error`, listed in the review, appears in no v2.1.210 string table and is not a subtype at this version.) A `system` line with no readable subtype fails closed. **Second review round — the subtype gate as first shipped silently disabled detection in production, and this row previously claimed otherwise.** Because Paperclip provisions a `SessionStart` hook, `hook_started`/`hook_response` open the transcript *before* `init` on the large majority of real runs — measured on this instance's pod logs at **6510 of 8036** `init`-carrying logs (81%), with the hook line preceding `init` in **399/399** of a sample carrying both. A whole-transcript "every line must be harness-authored" veto therefore returned `false` on all of them, and the suite stayed green only because its sole positive fixture was a synthetic two-line shape no production run has — precisely the "fix the false positive by disabling detection entirely" failure mode this issue's own acceptance criteria warn about. The predicate now **attributes the phrase to its line** instead of demanding a globally clean transcript: the trigger phrase counts only when it sits on a line the harness authored (an allowlisted event, or a bare non-event line, which in stream-json mode is the CLI speaking outside the protocol — 0 of 6893 sampled production lines are bare). Every false positive in this family is the phrase *inside* an event payload, so attribution is the more faithful invariant and unknown types still fail closed. Detection now genuinely survives the production preamble, pinned by four new cases (full preamble; each hook line alone; an untrusted event *after* the death), and the negative-control discipline is unchanged — the production-preamble case FAILS against the whole-transcript veto. One deliberate narrowing is recorded in the source: the phrase regex's `\s+` can span a newline, so a phrase straddling two lines would no longer match; the CLI emits it on one line, and the direction is the safe one. Also note this row's own earlier "measured as defence-in-depth rather than a live fix" framing applied to *nested types on an init line*, which remains accurate; it did not license the detection-loss claim. The phrase test still runs before the per-line walk as a pre-filter — the walk re-tests each line and is what decides — skipping an eager `split` of the entire pod log on the common phrase-absent failure. | | [#1669](https://github.com/Blockcast/paperclip/pull/1669) | `src/server/prompt-cache.ts`, `src/server/prompt-cache.test.ts`, `src/server/execute.ts`, `src/server/execute.test.ts`, `src/server/execute-environment.test.ts` | Classified the OTHER path into BLO-7991's pathology, which the row above cannot see. [BLO-32055](https://paperclip.blockcast.net/BLO/issues/BLO-32055): a live run died with a bare Node `ENOENT ... open '<...>/__runtime__//SKILL.md'` and was reported as the anonymous `adapter_failed` — an agent-pool/adapter fault for what is a skill configuration fault, invisible to skill-health sweeps. The reader was not a skill loader but `hashPathContents`, which walks each declared skill's tree to derive the prompt-bundle **cache key**; its `readFile` was unguarded. That walk runs inside `prepareClaudePromptBundle`, i.e. BEFORE the CLI is spawned, so `stdoutExcerpt` and `stderrExcerpt` were both null and there was no `parsed`, no result event and no transcript — every classifier in `parse.ts` reads a Claude-CLI-authored surface, so all of them are **structurally** blind to it. Not a too-narrow regex and not a regression of #1525: a second real path into the same user-visible failure, on a layer #1525 never inspects. The file's absence is transient — `company-skills.ts materializeRuntimeSkillFiles` refreshes by `fs.rm(recursive)` -> `mkdir` -> per-file `writeFile`, so the sweep publishes a window where the directory exists and `SKILL.md` does not; measured live, the file appeared **43m36s** after the run died on it. Routing this to `skill_not_found` would therefore have been the WRONG fix — that code is in `NON_RETRYABLE_CONTINUATION_ERROR_CODES`, so it converts a self-healing condition into permanent retry suppression, the same over-suppression hazard as the two rows above. Split instead on the source of truth (which skill owns the path, never message text, so the BLO-31794 false-positive class cannot reach this branch): a catalog-backed key yields a new transient `skill_materialization_pending` and a non-catalog-backed one keeps the permanent `skill_not_found`. The new code joins `TRANSIENT_INFRA_CONTINUATION_ERROR_CODES` — **the set that already contained `adapter_failed`** — so retryability is preserved exactly rather than widened. The discriminator is load-bearing rather than cosmetic because `readPaperclipRuntimeSkillEntries` silently switches source: it returns the server-injected catalog entries OR, when config carries none, the adapter's own bundled on-disk skills, a read-only image path where a missing file is a packaging fault no retry can fix. Hashing is still fatal and the error re-thrown rather than swallowed — a half-written tree hashed into a key would mint a bundle whose skills are silently incomplete, which is BLO-7991's original harm traded for a failure nobody sees. **Ally review follow-up:** `readCatalogBackedSkillKeys` is now a literal transcription of the key-deriving half of `normalizeConfiguredPaperclipRuntimeSkills` (`server-utils.ts:2598`) rather than an approximation, which was wrong in both directions — `asString` falls back on an EMPTY string and not merely on a non-string, so `{key:"", name:"x"}` normalizes upstream to key `x` while a `typeof key === "string"` test resolved it to `""` and dropped the entry, marking a catalog-backed skill un-backed (permanent suppression, the one direction this change exists to avoid); and upstream DISCARDS any entry missing `runtimeName` or `source`, which the hand-rolled version contributed anyway, letting a source-less entry colliding with a bundled key mark an image-path fault retryable. Deriving from the same primitives closes both. Each new test is verified as a negative control — the empty-key case fails against the hand-rolled predicate, and the three `execute.ts` cases fail against an inverted ternary or a dropped `instanceof` guard, pinning the seam this change exists to produce (both halves were covered before; the join was not). Two test files convert `./prompt-cache.js` from a whole-module mock to an `importOriginal` partial: the old form replaced the module with a single export, so ADDING any export here broke 23 unrelated tests — a trap worth knowing before adding the next one. **Does not fix the underlying race**: porting `materializePaperclipSkillCopy`'s tmp-dir + rename + lock pattern into `materializeRuntimeSkillFiles` is the RCA fix and is deliberately out of scope here. | | [BLO-31665](https://paperclip.blockcast.net/BLO/issues/BLO-31665) | `src/server/execute.ts`, `src/server/execute.test.ts`, `src/server/secret-adopt.test.ts` (new) | Made an `AlreadyExists` 409 on a run-scoped Secret non-fatal. `execute.ts` creates three Secrets before the Job (prompt, env, mcp-config) and treated **any** throw from the create as fatal, returning `k8s_{prompt,env,mcp_config}_secret_create_failed` and killing the run — so a benign leftover from an earlier attempt of the *same* run stranded the agent. Adopting is safe because the name encodes the run: each Secret is `${jobName}-{prompt,env,mcp}` with `jobName = ac---` (`:1151`), so a full-name collision is a collision with this same `(agentId, runId)` and the contents are re-derived from the same config. A new `createOrAdoptRunSecret()` reads the colliding object and `replaceNamespacedSecret`s it. Replace-on-collision cannot yank a Secret from a pod still mounting it: the `k8s_concurrent_run_blocked` guard lists this agent's Jobs and returns **before** `buildJobManifest`, so by the time any of these creates runs there is no live Job for this agent and a colliding Secret is a leftover of a *dead* attempt by construction. **This is a second, parallel implementation of the operation [#1562](https://github.com/Blockcast/paperclip/pull/1562) fixed** in `packages/plugins/sandbox-providers/kubernetes/src/secret-manager.ts`; that PR never touched this call site, which is why the 409 kept hard-failing runs after it shipped (measured again 2026-09-05T17:03:47Z, run `591c1384`, with the fix live on both tiers). One deliberate divergence from that sibling, plus one measured non-difference. (a) The identity gate fails closed on *positive contradiction* only — a Secret whose `paperclip.io/run-id` or `app.kubernetes.io/managed-by` label **disagrees** with this run is never overwritten, but one **missing** them is adopted. A verbatim port of the sibling's gate would have been inert here regardless, since it requires `paperclip.io/managed-by: paperclip-k8s-plugin` while this adapter writes `app.kubernetes.io/managed-by: paperclip` — a different key *and* value, so it would reject every Secret this adapter writes. (b) `isK8s409` mirrors `isK8s404`'s shape, including its `HTTP-Code:` message probe — but that probe is **redundant, not load-bearing**, and an earlier version of this row claimed otherwise. Measured against the installed `@kubernetes/client-node` 1.4.0 by constructing a real `ApiException`: it is built as `super("HTTP-Code: " + code + ...)` and then sets **only** `this.code` — `statusCode` and `response` are both `undefined`. So the sibling's `code`/`statusCode` predicate **does** fire correctly on the production error, and the concern that it would silently miss it is **withdrawn**. Two consequences worth keeping: `code` is the reliable structured signal, and the pre-existing `isK8s404` does **not** check it, so that predicate works today purely on its message regex; and the tests here construct the genuine `ApiException` rather than a hand-rolled stand-in, so they are evidence about the real error shape rather than about an assumption. A create-409 followed by a read-404 (the adapter's own cleanup reaper racing a retry) now retakes the newly-free name instead of resurfacing the stale 409, which is what the sibling does. Each new `execute()` test is verified as a negative control: all three FAIL against the pre-change file with `k8s_prompt_secret_create_failed`, so they discriminate the fix rather than merely passing alongside it. **Ally review follow-up:** the re-create in the read-404 branch was itself an unguarded create, so a second racer retaking the name resurfaced the raw 409 and killed the run with the very code this change exists to prevent. The function is now a bounded two-pass loop — the retry adopts if the name is taken again, and gives up with the original 409 if create-409/read-404 repeats, so a create/delete duel cannot spin. Verified as a negative control: the second-racer case FAILS against the single-shot version. Also from that review: the log verb now distinguishes `Replaced` (leftover overwritten) from `Recreated` (leftover vanished, name retaken) rather than collapsing both to "Reclaimed", since which occurred is what you want when triaging the next one; `readNamespacedSecret`/`replaceNamespacedSecret` got `beforeEach` defaults in `execute.test.ts`, the same BLO-21858 unstubbed-mock trap its existing comment warns about; and a fourth end-to-end case drives the **env** Secret, since the other three drive the prompt path while the reported incident was `k8s_env_secret_create_failed`. **Second review round:** introducing the verb table between the JSDoc and the function silently **detached** the doc comment — JSDoc binds to the next declaration, so the whole safety rationale documented the `SecretDisposition` type alias and the function it describes had none. Reordered so it binds to the function again; pure reordering, no behaviour change, but it had quietly undone the two commits that made that comment accurate. The live-pod paragraph is also **corrected rather than merely softened**: the concurrency guard counts a Job as running only when it has no `deletionTimestamp` and no Complete/Failed condition, so a Job mid-deletion with a still-terminating pod passes it, and the claim "there is no live Job" was too strong. What actually closes the residual window is the consumption model — the env Secret is read via `secretKeyRef` at container start so a later replace cannot reach a running container, and the prompt/mcp Secrets are volume-mounted but re-derived byte-identically for the same `(agentId, runId)`. The end-to-end fixtures now also build a genuine `ApiException` rather than a plain Error described with the retracted "status only in the message" characterization. **Does not address** the orphaned-Secret leak also described on that issue — that is [#1459](https://github.com/Blockcast/paperclip/pull/1459) (BLO-21857), which edits this same file and will need a rebase against whichever of the two lands second. The issue's "stale error is never cleared" defect was **falsified** while working this: a company-wide census of all 15 agents found zero holding a 409 `errorReason`, and all three originally-named agents had heartbeated within ~20 minutes — the field is overwritten by the next run's outcome, not sticky. | +| [BLO-32167](https://paperclip.blockcast.net/BLO/issues/BLO-32167) | `src/server/prompt-cache.ts`, `src/server/prompt-cache.test.ts` | Closed the OTHER instant of the race the row above classifies — the one that raises no syscall error, so no `try/catch` can reach it. #1669 guards the walk's `readFile`, which only fires when a sweep lands between the `readdir` and the `readFile`. Sampled a moment earlier — after `mkdir`, before the first `writeFile` — `hashPathContents` `lstat`s a directory that exists, emits `dir:`, `readdir`s an empty listing, iterates nothing that can fail, and returns. A valid key is minted over an unusable tree, the bundle is cached under it, and the run proceeds **silently degraded**: BLO-7991's original harm (an agent behaves as though a declared skill does not exist), now with no failure for anyone to see, which is strictly more expensive than branch A's loud death. Live capture, CEO run `ff67a1b1` ~00:35Z 2026-09-06: the per-run pod copy of `investigate--9debdeaf08` was an empty directory while the shared store held all 24,209 bytes a minute later — anti-correlated samples a minute apart, and the run completed rather than dying. The pod copy is a snapshot, so the store's self-heal never reaches the running pod; meanwhile the catalogue advertised the skill with its full description throughout. The guard asserts the **positive shape** (`SKILL.md` present and readable at the source root) rather than testing for emptiness, deliberately: the writer iterates `fileInventory` in order and `SKILL.md` need not be first, so the far commoner partial state is *some files, no entrypoint*, which an emptiness test walks straight past. That is also the BLO-31794 inversion applied here — state what a valid tree must contain instead of enumerating how it can be broken. Fires **only** for keys the caller positively identified as catalog-backed, never on the `?? true` default the branch-A classifier uses: that default is right there because it classifies a fault that already happened and over-retrying beats permanently suppressing a self-healing one, whereas here the question is whether to *manufacture* a fault, and erring toward manufacturing would fail runs over bundled adapter skills in a read-only image path the sweep never rewrites (the BLO-31794 over-suppression hazard, in the direction this change could newly introduce it). Classification is unchanged in every other respect: retryable `skill_materialization_pending`, never `skill_not_found`, and `stat` rather than `lstat` so a symlinked entrypoint resolving to a real file stays usable. The **primary** fix is at the source and lives outside this directory — `server/src/services/company-skills.ts materializeRuntimeSkillFiles` now publishes by staging tree + rename, so neither branch should be reachable from that writer; this guard remains as the observer-side backstop because the pod-local bundle copy is a separate snapshot on a different code path and `materializeVersionSnapshot` still has the original non-atomic shape. Note the RCA framing in the row above ("porting `materializePaperclipSkillCopy`'s tmp-dir + rename + lock pattern") was followed in spirit but **not literally**: that pattern is `rm -rf targetRoot` then `rename`, and its lock serializes *writers* while `rm` is itself a walk — so a concurrent *reader* mid-`rm` still sees a shrinking tree, i.e. branch B survives the port. Renaming the outgoing tree aside instead is reader-atomic without needing writer exclusion. Every new test is verified as a negative control: the three behaviour-asserting cases fail against the pre-fix walk (the empty-dir and no-entrypoint cases mint a key; the EACCES case throws the wrong type), while the three no-regression guards — non-catalog-backed empty dir, byte-identical key on a healthy tree, symlinked entrypoint — pass both before and after, which is what they are for. | + The two cherry-picked commits in the composition above remain upstream commits authored against the fork, not Blockcast-local patches. diff --git a/vendor/paperclip-adapter-claude-k8s/package-lock.json b/vendor/paperclip-adapter-claude-k8s/package-lock.json index 5b99c72a5042..8f004b9d1b44 100644 --- a/vendor/paperclip-adapter-claude-k8s/package-lock.json +++ b/vendor/paperclip-adapter-claude-k8s/package-lock.json @@ -1,12 +1,12 @@ { "name": "paperclip-adapter-claude-k8s", - "version": "0.2.6-blockcast.4", + "version": "0.2.6-blockcast.5", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "paperclip-adapter-claude-k8s", - "version": "0.2.6-blockcast.4", + "version": "0.2.6-blockcast.5", "license": "MIT", "dependencies": { "@kubernetes/client-node": "^1.0.0", diff --git a/vendor/paperclip-adapter-claude-k8s/package.json b/vendor/paperclip-adapter-claude-k8s/package.json index 380d8faa53ce..93020bdfdf23 100644 --- a/vendor/paperclip-adapter-claude-k8s/package.json +++ b/vendor/paperclip-adapter-claude-k8s/package.json @@ -1,6 +1,6 @@ { "name": "paperclip-adapter-claude-k8s", - "version": "0.2.6-blockcast.4", + "version": "0.2.6-blockcast.5", "description": "Paperclip adapter plugin that runs Claude Code agents as Kubernetes Jobs", "license": "MIT", "repository": { diff --git a/vendor/paperclip-adapter-claude-k8s/src/server/prompt-cache.test.ts b/vendor/paperclip-adapter-claude-k8s/src/server/prompt-cache.test.ts index e37f35356251..98da8a65ce53 100644 --- a/vendor/paperclip-adapter-claude-k8s/src/server/prompt-cache.test.ts +++ b/vendor/paperclip-adapter-claude-k8s/src/server/prompt-cache.test.ts @@ -191,6 +191,185 @@ describe("prepareClaudePromptBundle skill-source materialization race (BLO-32055 }); }); +// BLO-32167. The other instant of the same race, and the one no `try/catch` can +// reach: sampled after the `mkdir` and before `SKILL.md` lands, the tree raises +// no syscall error at all. Every fixture below therefore asserts on a walk that +// *succeeds* — if these ever start passing by throwing an ENOENT, the fixture +// has drifted onto branch A and is no longer testing this issue. +describe("prepareClaudePromptBundle skill-source entrypoint assertion (BLO-32167)", () => { + const companyId = "acme-co"; + const skillKey = "garrytan/gstack/investigate"; + + async function withRoot(body: (root: string) => Promise): Promise { + const root = await fs.mkdtemp(path.join(os.tmpdir(), "blo32167-")); + vi.stubEnv("PAPERCLIP_HOME", path.join(root, "home")); + try { + return await body(root); + } finally { + vi.unstubAllEnvs(); + await fs.rm(root, { recursive: true, force: true }); + } + } + + const skillEntry = (source: string) => ({ + key: skillKey, + runtimeName: "investigate--9debdeaf08", + source, + required: false, + requiredReason: null, + }); + + it("refuses to mint a key over a catalog-backed skill whose directory is empty", async () => { + // The live capture: CEO run ff67a1b1, ~00:35Z 2026-09-06. The per-run pod + // copy of `investigate--9debdeaf08` was an empty directory while the shared + // store held all 24,209 bytes a minute later. The run did not die — it + // completed, silently degraded, and the catalogue advertised the skill + // throughout. Nothing here mocks `fs`: an empty directory is a state the + // filesystem produces natively, which is the whole point of the branch. + await withRoot(async (root) => { + const skillDir = path.join(root, "__runtime__", "investigate--9debdeaf08"); + await fs.mkdir(skillDir, { recursive: true }); + expect(await fs.readdir(skillDir)).toHaveLength(0); + + await expect(prepareClaudePromptBundle({ + companyId, + skills: [skillEntry(skillDir)], + instructionsContents: null, + catalogBackedSkillKeys: new Set([skillKey]), + onLog, + })).rejects.toMatchObject({ + name: "ClaudeSkillSourceUnavailableError", + skillKey, + // Retryable `skill_materialization_pending`, never `skill_not_found`: + // the condition self-heals on the next sweep, and `skill_not_found` is + // in NON_RETRYABLE_CONTINUATION_ERROR_CODES. + catalogBacked: true, + }); + }); + }); + + it("refuses a partially-written tree that has files but no SKILL.md", async () => { + // Strictly more common than the empty case and invisible to an emptiness + // test: the writer iterates `fileInventory` in order and SKILL.md need not + // be first, so "some files, no entrypoint" is the wider window. + await withRoot(async (root) => { + const skillDir = path.join(root, "__runtime__", "investigate--9debdeaf08"); + await fs.mkdir(path.join(skillDir, "references"), { recursive: true }); + await fs.writeFile(path.join(skillDir, "references", "playbook.md"), "x", "utf8"); + expect(await fs.readdir(skillDir)).not.toHaveLength(0); + + await expect(prepareClaudePromptBundle({ + companyId, + skills: [skillEntry(skillDir)], + instructionsContents: null, + catalogBackedSkillKeys: new Set([skillKey]), + onLog, + })).rejects.toMatchObject({ name: "ClaudeSkillSourceUnavailableError", catalogBacked: true }); + }); + }); + + it("leaves a non-catalog-backed empty directory classifying exactly as before", async () => { + // The BLO-31794 over-suppression hazard, in the direction this change could + // newly break: a bundled adapter skill lives in a read-only image path the + // sweep never rewrites, so its shape is not ours to police. Same on-disk + // state as the first case; only catalog membership moves the verdict, and + // here it must mint a key rather than manufacture a fault. + await withRoot(async (root) => { + const skillDir = path.join(root, "__runtime__", "investigate--9debdeaf08"); + await fs.mkdir(skillDir, { recursive: true }); + + const bundle = await prepareClaudePromptBundle({ + companyId, + skills: [skillEntry(skillDir)], + instructionsContents: null, + catalogBackedSkillKeys: new Set(), + onLog, + }); + expect(bundle.bundleKey).toMatch(/^[0-9a-f]{64}$/); + }); + }); + + it("leaves the bundle key of a populated catalog-backed tree byte-identical", async () => { + // The assertion must not perturb the healthy path. A changed key would + // invalidate every cached prompt bundle in the estate on deploy, so this + // compares the guarded key against the unguarded one rather than merely + // asserting it is well-formed. + await withRoot(async (root) => { + const skillDir = path.join(root, "__runtime__", "investigate--9debdeaf08"); + await fs.mkdir(skillDir, { recursive: true }); + await fs.writeFile(path.join(skillDir, "SKILL.md"), "---\nname: investigate\n---\n", "utf8"); + + const guarded = await prepareClaudePromptBundle({ + companyId, + skills: [skillEntry(skillDir)], + instructionsContents: null, + catalogBackedSkillKeys: new Set([skillKey]), + onLog, + }); + const unguarded = await prepareClaudePromptBundle({ + companyId, + skills: [skillEntry(skillDir)], + instructionsContents: null, + onLog, + }); + expect(guarded.bundleKey).toBe(unguarded.bundleKey); + }); + }); + + it("accepts a SKILL.md reached through a symlink", async () => { + // `stat`, not `lstat`: a symlinked entrypoint resolving to a real file is + // usable, and treating it as missing would fail a legitimate layout. + await withRoot(async (root) => { + const skillDir = path.join(root, "__runtime__", "investigate--9debdeaf08"); + await fs.mkdir(skillDir, { recursive: true }); + const realDoc = path.join(root, "real-skill.md"); + await fs.writeFile(realDoc, "---\nname: investigate\n---\n", "utf8"); + await fs.symlink(realDoc, path.join(skillDir, "SKILL.md")); + + const bundle = await prepareClaudePromptBundle({ + companyId, + skills: [skillEntry(skillDir)], + instructionsContents: null, + catalogBackedSkillKeys: new Set([skillKey]), + onLog, + }); + expect(bundle.bundleKey).toMatch(/^[0-9a-f]{64}$/); + }); + }); + + it("does not launder a non-ENOENT stat failure into a skill fault", async () => { + // Mirrors the branch-A EACCES case. A permissions fault on the entrypoint is + // a real fault against an unchanging cause; classifying it as + // materialization-pending would retry it forever. + await withRoot(async (root) => { + const skillDir = path.join(root, "__runtime__", "investigate--9debdeaf08"); + await fs.mkdir(skillDir, { recursive: true }); + await fs.writeFile(path.join(skillDir, "SKILL.md"), "body", "utf8"); + + const realStat = fs.stat; + const spy = vi.spyOn(fs, "stat").mockImplementation(async (target, ...rest) => { + if (typeof target === "string" && target.endsWith("SKILL.md")) { + const err = new Error("EACCES: permission denied") as NodeJS.ErrnoException; + err.code = "EACCES"; + throw err; + } + return (realStat as never)(target, ...rest); + }); + try { + await expect(prepareClaudePromptBundle({ + companyId, + skills: [skillEntry(skillDir)], + instructionsContents: null, + catalogBackedSkillKeys: new Set([skillKey]), + onLog, + })).rejects.toThrow(/EACCES/); + } finally { + spy.mockRestore(); + } + }); + }); +}); + // BLO-32055. `readPaperclipRuntimeSkillEntries` silently switches source: it // returns the server-injected catalog entries, OR — when the config carries none // — the adapter's own bundled on-disk skills. Only the first set lives under the diff --git a/vendor/paperclip-adapter-claude-k8s/src/server/prompt-cache.ts b/vendor/paperclip-adapter-claude-k8s/src/server/prompt-cache.ts index 7c8ca0ac96e3..9cf592c9a93e 100644 --- a/vendor/paperclip-adapter-claude-k8s/src/server/prompt-cache.ts +++ b/vendor/paperclip-adapter-claude-k8s/src/server/prompt-cache.ts @@ -24,17 +24,29 @@ export interface ClaudePromptBundle { const DEFAULT_PAPERCLIP_INSTANCE_ID = "default"; /** - * A declared skill's source tree lost a file out from under the bundle-key walk. + * A declared skill's source tree is not usable as a skill at bundle-key time. * - * BLO-32055: `company-skills.ts materializeRuntimeSkillFiles` refreshes a runtime - * skill by `fs.rm(skillDir, {recursive:true})` -> `mkdir` -> per-file `writeFile`. - * That is not atomic, so the rolling materialization sweep publishes a window in - * which the directory exists and `SKILL.md` does not. `hashPathContents` below - * walks that tree to derive the prompt-bundle cache key, and its `readFile` used - * to be unguarded — so a sweep landing between the `readdir` and the `readFile` - * threw a bare Node `ENOENT ... open '<...>/__runtime__//SKILL.md'` out of + * Two producers, both instants of the one materialization race: + * - BLO-32055 (branch A) — a file vanished mid-walk, raising `ENOENT`. + * - BLO-32167 (branch B) — the walk completed cleanly over a tree with no + * `SKILL.md`. Nothing throws; see `assertSkillEntrypointPresent`. + * + * BLO-32055: `company-skills.ts materializeRuntimeSkillFiles` used to refresh a + * runtime skill by `fs.rm(skillDir, {recursive:true})` -> `mkdir` -> per-file + * `writeFile`. That was not atomic, so the rolling materialization sweep + * published a window in which the directory exists and `SKILL.md` does not. + * `hashPathContents` below walks that tree to derive the prompt-bundle cache + * key, and its `readFile` used to be unguarded — so a sweep landing between the + * `readdir` and the `readFile` threw a bare Node + * `ENOENT ... open '<...>/__runtime__//SKILL.md'` out of * `prepareClaudePromptBundle`, i.e. before the Claude CLI was ever spawned. * + * (BLO-32167 has since made that publish atomic — staging tree, then rename — + * so both windows should now be closed at the writer. These guards remain as + * the observer-side backstop: the pod-local bundle copy is a separate snapshot + * on a different code path, and `materializeVersionSnapshot` still has the + * original non-atomic shape.) + * * That is why the live instance carried `errorCode: adapter_failed` with both * `stdoutExcerpt` and `stderrExcerpt` null: there was no transcript, no result * event, and no `parsed` for `isClaudeSkillNotFoundError` to read. The BLO-7991 @@ -202,6 +214,64 @@ async function hashPathContents( hash.update(`other:${relativePath}:${stat.mode}\n`); } +/** + * The file whose presence makes a skill directory a *skill* rather than a + * directory. Claude will not load a skill without it. + */ +const SKILL_ENTRYPOINT_FILENAME = "SKILL.md"; + +/** + * A catalog-backed skill source must contain a readable `SKILL.md` at its root. + * + * BLO-32167 — the second branch of the BLO-32055 race, and the one that raises + * no syscall error at all. `materializeRuntimeSkillFiles` used to publish + * `rm -rf` -> `mkdir` -> per-file `writeFile`, so a reader could sample the tree + * *after* the `mkdir` and *before* `SKILL.md` was written. `hashPathContents` + * handles that state perfectly happily: it `lstat`s a directory that exists, + * emits `dir:`, `readdir`s an empty (or SKILL.md-less) listing, iterates + * nothing that fails, and returns. The `isMissingEntryError` guard added for + * branch A is never reached because nothing throws. A valid key is minted over + * an unusable tree, the bundle is cached under it, and the run proceeds + * *silently degraded* — which is BLO-7991's original harm (an agent behaves as + * though a declared skill does not exist), now with no failure for anyone to + * see. It is strictly more expensive than branch A's loud death. + * + * The primary fix is at the source: the materializer now publishes by rename, + * so this state should no longer be reachable from that writer. This assertion + * is the observer-side backstop, and it is not redundant — the pod-local copy of + * the bundle is a *separate* snapshot taken by a different code path, and the + * version-snapshot materializer has the same non-atomic shape. A backstop that + * costs one `stat` per catalog-backed skill is worth having on the layer that + * mints the cache key. + * + * Deliberately keyed on `SKILL.md` rather than on "the directory is empty". + * The write loop iterates `fileInventory` in order and `SKILL.md` need not be + * first, so the far commoner partial state is *some files, no entrypoint* — an + * emptiness test would walk straight past it. Asserting the positive shape is + * also the inversion BLO-31794 asks for generally: state what a valid tree must + * contain, rather than enumerating the ways it can be broken. + */ +async function assertSkillEntrypointPresent(entry: PaperclipSkillEntry): Promise { + const entrypoint = path.join(entry.source, SKILL_ENTRYPOINT_FILENAME); + // `stat`, not `lstat`: a symlinked entrypoint that resolves to a real file is + // usable, and only the resolved target answers the question being asked. + const stat = await fs.stat(entrypoint).catch((err: unknown) => { + if (isMissingEntryError(err)) return null; + throw err; + }); + if (stat?.isFile()) return; + throw new ClaudeSkillSourceUnavailableError({ + skillKey: entry.key, + skillSource: entry.source, + missingPath: entrypoint, + // Only ever called for keys the caller resolved from a catalog row, so this + // is the transient class by construction: retryable, and self-healing on the + // next sweep. See the call site for why it is not called for anything else. + catalogBacked: true, + cause: null, + }); +} + async function buildClaudePromptBundleKey(input: { skills: PaperclipSkillEntry[]; instructionsContents: string | null; @@ -236,6 +306,19 @@ async function buildClaudePromptBundleKey(input: { cause: err, }); } + // BLO-32167. Only for keys KNOWN to be catalog-backed — never on the + // `?? true` default the branch-A classifier above uses. That default is + // correct there because it decides how to classify a fault that has already + // happened, and over-retrying beats permanently suppressing a self-healing + // one. Here the question is the opposite: whether to *manufacture* a fault. + // Erring toward manufacturing one would fail runs whose skill trees are + // legitimately shaped differently — bundled adapter skills in a read-only + // image path, which the sweep never rewrites and which no caller has told us + // about. That is the BLO-31794 over-suppression hazard, so this stays silent + // unless the caller positively identified the entry as catalog-backed. + if (input.catalogBackedSkillKeys?.has(entry.key)) { + await assertSkillEntrypointPresent(entry); + } } return hash.digest("hex"); }