From 6d1ef03db0cd173cd951efcdb848c50c604ade0a Mon Sep 17 00:00:00 2001 From: tim-inkeep <132074086+tim-inkeep@users.noreply.github.com> Date: Fri, 26 Jun 2026 17:32:17 -0400 Subject: [PATCH] fix(skills): don't conflate fs errors with binary/not-found in skill reads (#2203) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(skills): don't conflate fs errors with binary/not-found in skill reads Two post-merge review findings from PR #1662: - readSkillBundledFiles returned text=null for BOTH binary/oversize files AND genuine IO errors (EACCES/EIO/EISDIR). The cross-scope move treats a null-text file as binary, SKIPS it, then deletes the source — so a read error became silent data loss. It now throws on a real IO error (only binary/oversize and a vanished ENOENT file stay null), which fails GET /api/skill and aborts the move before any delete. +test (guarded for root, which bypasses perms). - skill-restore's ls-tree catch mapped every git failure to version-not-found (HTTP 404), so a git I/O error / corrupt repo was masked as a stale-version 404. It now classifies a bad-object/revision as version-not-found (404) and any other git failure as io-error (500), matching the sibling git-show catch. * test+refactor(skills): pin the git not-found/io-error boundary, align catch Addresses the two review suggestions on this PR: - Extract the ls-tree error classifier to an exported isGitObjectNotFound() so the load-bearing 404-vs-500 regex is single-sourced and unit-tested (a bad object/revision stays 404; corrupt-repo / missing-binary / EACCES → 500). - Align the sibling git-show catch to the safe e instanceof Error ? e.message : String(e) extraction so a non-Error rejection can't surface as 'undefined'. * style: single-quote the test fixture strings (biome format) --------- GitOrigin-RevId: 5a50169587c6e470a9b5a1b321b24f32245bca57 --- packages/server/src/skill-projection.test.ts | 15 ++++++++++ packages/server/src/skill-projection.ts | 3 +- packages/server/src/skill-restore.test.ts | 31 ++++++++++++++++++++ packages/server/src/skill-restore.ts | 24 ++++++++++----- 4 files changed, 65 insertions(+), 8 deletions(-) create mode 100644 packages/server/src/skill-restore.test.ts diff --git a/packages/server/src/skill-projection.test.ts b/packages/server/src/skill-projection.test.ts index 1f4bf5266..43208846a 100644 --- a/packages/server/src/skill-projection.test.ts +++ b/packages/server/src/skill-projection.test.ts @@ -1,5 +1,6 @@ import { afterEach, beforeEach, describe, expect, test } from 'bun:test'; import { + chmodSync, existsSync, lstatSync, mkdirSync, @@ -172,6 +173,20 @@ describe('readSkillBundledFiles', () => { test('absent skill dir returns empty', () => { expect(readSkillBundledFiles(join(root, 'nope'))).toEqual([]); }); + + const isRoot = typeof process.getuid === 'function' && process.getuid() === 0; + test.skipIf(isRoot)('a genuine IO error THROWS rather than masquerading as binary', () => { + const dir = makeSkill('locked', '# Body'); + mkdirSync(join(dir, 'reference'), { recursive: true }); + const secret = join(dir, 'reference', 'secret.md'); + writeFileSync(secret, '# Secret', 'utf-8'); + chmodSync(secret, 0o000); + try { + expect(() => readSkillBundledFiles(dir)).toThrow(); + } finally { + chmodSync(secret, 0o644); // restore so afterEach can clean up + } + }); }); describe('hostSkillsRootEscapes', () => { diff --git a/packages/server/src/skill-projection.ts b/packages/server/src/skill-projection.ts index 93e6901ac..1dc90a2ed 100644 --- a/packages/server/src/skill-projection.ts +++ b/packages/server/src/skill-projection.ts @@ -233,7 +233,8 @@ export function readSkillBundledFiles( if (buf.length <= MAX_BUNDLED_FILE_BYTES && !buf.includes(0)) { text = buf.toString('utf-8'); } - } catch { + } catch (err) { + if ((err as NodeJS.ErrnoException)?.code !== 'ENOENT') throw err; text = null; } out.push({ path: rel, text }); diff --git a/packages/server/src/skill-restore.test.ts b/packages/server/src/skill-restore.test.ts new file mode 100644 index 000000000..4f0a79a3d --- /dev/null +++ b/packages/server/src/skill-restore.test.ts @@ -0,0 +1,31 @@ +import { describe, expect, test } from 'bun:test'; +import { isGitObjectNotFound } from './skill-restore.ts'; + +describe('isGitObjectNotFound', () => { + test('classifies a genuinely-missing object/revision as not-found (→ 404)', () => { + for (const msg of [ + "fatal: not a valid object name 'abc123'", + 'fatal: bad revision', + 'unknown revision or path not in the working tree', + 'fatal: Not a valid object name HEAD~5', + 'fatal: invalid object name deadbeef', + 'fatal: not a tree object', + ]) { + expect(isGitObjectNotFound(msg)).toBe(true); + } + }); + + test('does NOT match genuine git I/O / server faults (→ 500)', () => { + for (const msg of [ + 'fatal: unable to read source tree', + 'git binary not found', + 'fatal: object file is empty', + 'repository is corrupt', + 'fatal: unable to read tree', + 'EACCES: permission denied', + 'spawn git ENOENT', + ]) { + expect(isGitObjectNotFound(msg)).toBe(false); + } + }); +}); diff --git a/packages/server/src/skill-restore.ts b/packages/server/src/skill-restore.ts index 4d9e3336e..a0cfc2560 100644 --- a/packages/server/src/skill-restore.ts +++ b/packages/server/src/skill-restore.ts @@ -16,6 +16,12 @@ export type RestoreSkillResult = error: string; }; +export function isGitObjectNotFound(message: string): boolean { + return /not a valid object name|not a tree object|bad revision|unknown revision|invalid object name/i.test( + message, + ); +} + export async function restoreSkillVersion(opts: { shadow: ShadowHandle; contentDir: string; @@ -33,12 +39,15 @@ export async function restoreSkillVersion(opts: { let fileList: string; try { fileList = await sg.raw('ls-tree', '-r', '--name-only', version, '--', shadowPath); - } catch { - return { - ok: false, - code: 'version-not-found', - error: `Version ${version.slice(0, 8)} not found.`, - }; + } catch (e) { + const msg = e instanceof Error ? e.message : String(e); + return isGitObjectNotFound(msg) + ? { ok: false, code: 'version-not-found', error: `Version ${version.slice(0, 8)} not found.` } + : { + ok: false, + code: 'io-error', + error: `Failed to read version ${version.slice(0, 8)}: ${msg}`, + }; } const files = fileList .split('\n') @@ -68,10 +77,11 @@ export async function restoreSkillVersion(opts: { try { staged.push({ rel, destAbs, content: await sg.raw('show', `${version}:${shadowFile}`) }); } catch (e) { + const msg = e instanceof Error ? e.message : String(e); return { ok: false, code: 'io-error', - error: `Failed reading ${rel} at ${version.slice(0, 8)}: ${(e as Error).message}`, + error: `Failed reading ${rel} at ${version.slice(0, 8)}: ${msg}`, }; } }