From 8b873e01121a80e563a8a02bf6a3219783de5476 Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Wed, 21 Oct 2026 00:14:53 -0700 Subject: [PATCH] fix(adapter-utils): skip the materialize lock for an already-current skill (#17061) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - A remote Claude ACP run stages the selected runtime skills into the sandbox. The host archives the materialized skills directory with tar and uploads it. > - Concurrent runs of one agent share that skills directory. The directory key is the selected skill set and its content. > - Each run calls `materializePaperclipSkillCopy` for each skill. The function always created and removed a sibling `.lock` directory, even when the copy was already current. > - The archive step lists the directory entries first and then gives the names to tar. When a concurrent run removed its lock between these two steps, tar failed with `Cannot stat` and the whole run failed as `adapter_failed`. > - This pull request checks the materialized fingerprint before it takes the lock. An up-to-date copy now leaves no transient entries in the shared directory. > - The benefit is that concurrent runs of one agent no longer fail each other's skill staging. ## Linked Issues or Issue Description **What happened?** A remote `claude_local` run failed before the agent started. The skill staging step ran `tar -c ... -T entries.nul` on the shared runtime-skills directory. Tar stopped with `tar: .lock: Cannot stat: No such file or directory` and exit status 2. Another run of the same agent was materializing the same skills at the same time. **Expected behavior** A run that finds its skills already materialized does not change the shared skills directory. Concurrent runs can archive that directory safely. **Steps to reproduce** 1. Materialize a skill into a skills directory. 2. Call `materializePaperclipSkillCopy` again in a loop for the same skill. 3. At the same time, call `createTarballFromDirectory` on the skills directory in a loop. 4. Before this change, most archives fail with `Cannot stat` on `.lock`. **Agent adapter(s) involved** `claude_local` (ACP, remote sandbox). The function is shared, so `grok_local` and the Codex and Gemini ACP skill paths also benefit. ## What Changed - `packages/adapter-utils/src/server-utils.ts`: `materializePaperclipSkillCopy` hashes the source and checks the materialized sentinel before it takes the lock. When the copy is current, it returns without creating the lock directory or a temporary copy. The existing check inside the lock stays, so a copy that is missing or out of date still uses the lock as before. The source is hashed once per call, the same as before. - `packages/adapter-utils/src/server-utils.test.ts`: a new test holds a live lock from another owner, then materializes an already-current skill. The call must return at once and must not change the lock or the directory. ## Verification - `cd packages/adapter-utils && npx vitest run src/server-utils.test.ts`: 132 passed. - Red/green: without the fix, the new test times out because the call waits on the live lock. With the fix, it passes. - `npx vitest run src/acpx-engine/execute.test.ts -t skill`: 15 passed. - `npx tsc --noEmit -p tsconfig.json` in `packages/adapter-utils`: passes. - Real-dependency race check (a temporary script, not committed). The script used real files and the system tar. It ran `materializePaperclipSkillCopy` for 5 skills in a loop while `createTarballFromDirectory` archived the same directory for 8 seconds: - Before the fix: 1994 of 2307 archives failed with `tar: skill-1.lock: Cannot stat: No such file or directory`. - After the fix: 0 of 2021 archives failed. - Assumption under test: the sentinel check outside the lock is safe. The sentinel is written into the temporary copy before the atomic rename, so a matching sentinel means a complete copy. The lock never protected readers: the caller already reads `SKILL.md` after the lock is released. - Remaining limit: the first materialization of a new skill set still uses the lock. A concurrent archive of the same directory is unlikely then, because a run archives the directory only after it has materialized every skill in it. ## Risks - Low risk. The change only skips the lock when the result is already correct. A missing, damaged, or out-of-date copy uses the same locked path as before. - No change to the materialized layout, the sentinel format, or the lock format. > For core feature work, check [`ROADMAP.md`](ROADMAP.md) first and discuss it in `#dev` before opening the PR. Feature PRs that overlap with planned core work may need to be redirected — check the roadmap first. See `CONTRIBUTING.md`. ## Model Used - Anthropic Claude Opus 5.5 (`claude-opus-5-5`) in Claude Code, with tool use (shell, file edits, test runs). The exact context-window size is not exposed in this session. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: devinfoley <139239+devinfoley@users.noreply.github.com> Co-authored-by: Claude Opus 5.5 --- .../adapter-utils/src/server-utils.test.ts | 31 +++++++++++++++++++ packages/adapter-utils/src/server-utils.ts | 5 ++- 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/packages/adapter-utils/src/server-utils.test.ts b/packages/adapter-utils/src/server-utils.test.ts index e8f524201d7..0523c4d09eb 100644 --- a/packages/adapter-utils/src/server-utils.test.ts +++ b/packages/adapter-utils/src/server-utils.test.ts @@ -345,6 +345,37 @@ describe("materializePaperclipSkillCopy", () => { } }); + it("does not lock an already-current materialized skill", async () => { + const root = await fs.mkdtemp( + path.join(os.tmpdir(), "paperclip-skill-copy-"), + ); + try { + const source = path.join(root, "source"); + const skillsHome = path.join(root, "skills"); + const target = path.join(skillsHome, "skill"); + const lock = `${target}.lock`; + await fs.mkdir(source, { recursive: true }); + await fs.writeFile(path.join(source, "SKILL.md"), "# skill\n", "utf8"); + await materializePaperclipSkillCopy(source, target); + + await fs.mkdir(lock); + const owner = path.join(lock, "owner.json"); + await fs.writeFile( + owner, + JSON.stringify({ pid: process.pid, createdAt: new Date().toISOString() }), + "utf8", + ); + + await expect( + materializePaperclipSkillCopy(source, target), + ).resolves.toMatchObject({ copiedFiles: 0 }); + expect((await fs.readdir(skillsHome)).sort()).toEqual(["skill", "skill.lock"]); + await expect(fs.readFile(owner, "utf8")).resolves.toContain(`"pid":${process.pid}`); + } finally { + await fs.rm(root, { recursive: true, force: true }); + } + }); + it("breaks stale materialization locks left by dead processes", async () => { const root = await fs.mkdtemp( path.join(os.tmpdir(), "paperclip-skill-copy-"), diff --git a/packages/adapter-utils/src/server-utils.ts b/packages/adapter-utils/src/server-utils.ts index 05a670d2724..26dac29cd9e 100644 --- a/packages/adapter-utils/src/server-utils.ts +++ b/packages/adapter-utils/src/server-utils.ts @@ -4563,6 +4563,10 @@ export async function materializePaperclipSkillCopy( skippedSymlinks: [], }; + const sourceFingerprint = await hashSkillDirectory(sourceRoot); + if (await materializedSkillFingerprintMatches(targetRoot, sourceFingerprint)) + return result; + const lockDir = `${targetRoot}.lock`; const releaseLock = await acquireMaterializeLock(lockDir); const tempRoot = `${targetRoot}.tmp-${process.pid}-${randomUUID()}`; @@ -4608,7 +4612,6 @@ export async function materializePaperclipSkillCopy( } try { - const sourceFingerprint = await hashSkillDirectory(sourceRoot); if ( await materializedSkillFingerprintMatches(targetRoot, sourceFingerprint) )