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"); }