Avoid creating unwritable HOME skills dir in SkillLoader#103
Avoid creating unwritable HOME skills dir in SkillLoader#103SergioChan merged 4 commits intopockebot:mainfrom
Conversation
…x runs don’t crash when OPENPOCKET_HOME isn’t writable - keep bundled and workspace skills loading unchanged
There was a problem hiding this comment.
Pull request overview
Adjusts skill source directory initialization so skill loading doesn’t fail when the local skills directory under OPENPOCKET_HOME can’t be created (e.g., sandboxed/CI environments), while still loading bundled/workspace skills.
Changes:
- Build the list of skill source directories incrementally instead of returning a fixed array.
- Wrap creation of the local skills directory (
OPENPOCKET_HOME/skills) in atry/catchand skip local skills on failure. - Always include the workspace skills directory as a source.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // this path may not be writable. If mkdir fails, skip it so bundled + | ||
| // workspace skills still load. | ||
| try { | ||
| const localDir = ensureDir(path.join(openpocketHome(), "skills")); | ||
| dirs.push({ source: "local", dir: localDir }); | ||
| } catch { | ||
| // ignore – lack of local skills should not be fatal |
There was a problem hiding this comment.
The blanket catch {} will silently swallow any failure from ensureDir(openpocketHome()/skills), including non-permission errors (e.g., OPENPOCKET_HOME pointing at a file, invalid path, etc.). That makes local-skill loading failures hard to diagnose and can hide real misconfiguration. Consider catching the error object and only skipping on expected non-writable cases (EACCES/EPERM/EROFS), and either rethrow or emit a warning/debug log for unexpected errors.
| // this path may not be writable. If mkdir fails, skip it so bundled + | |
| // workspace skills still load. | |
| try { | |
| const localDir = ensureDir(path.join(openpocketHome(), "skills")); | |
| dirs.push({ source: "local", dir: localDir }); | |
| } catch { | |
| // ignore – lack of local skills should not be fatal | |
| // this path may not be writable. If mkdir fails due to permission issues, | |
| // skip it so bundled + workspace skills still load. Other errors should | |
| // surface to aid diagnosis (e.g., misconfigured OPENPOCKET_HOME). | |
| try { | |
| const localDir = ensureDir(path.join(openpocketHome(), "skills")); | |
| dirs.push({ source: "local", dir: localDir }); | |
| } catch (err) { | |
| const code = | |
| err && typeof err === "object" && "code" in err | |
| ? (err as { code?: unknown }).code | |
| : undefined; | |
| if (code === "EACCES" || code === "EPERM" || code === "EROFS") { | |
| // ignore – lack of local skills due to non-writable home is not fatal | |
| } else { | |
| // Unexpected error (e.g., invalid path, OPENPOCKET_HOME misconfigured) | |
| throw err; | |
| } |
| // Local skills live under OPENPOCKET_HOME. In sandboxed or CI environments | ||
| // this path may not be writable. If mkdir fails, skip it so bundled + | ||
| // workspace skills still load. | ||
| try { | ||
| const localDir = ensureDir(path.join(openpocketHome(), "skills")); | ||
| dirs.push({ source: "local", dir: localDir }); | ||
| } catch { |
There was a problem hiding this comment.
This new behavior (skipping local skills when ensureDir fails) isn’t covered by the existing SkillLoader tests. Add a test that forces ensureDir(path.join(openpocketHome(), "skills")) to throw (e.g., set OPENPOCKET_HOME to a temp file path so mkdir hits ENOTDIR) and assert SkillLoader still loads bundled/workspace skills without throwing.
|
@SergioChan is attempting to deploy a commit to the t54 labs Team on Vercel. A member of the Team first needs to authorize it. |
Summary
guard SkillLoader’s local skills path with a try/catch so CI/sandbox runs
don’t crash when OPENPOCKET_HOME isn’t writable
keep bundled and workspace skills loading unchanged
Why
found this problem while setting up the project on my machine
Changes
Added a try/catch around local skills directory creation so SkillLoader skips OPENPOCKET_HOME/skills when that path isn’t writable; bundled + workspace skills still load (src/skills/skill-loader.ts).
Kept behavior unchanged for workspace skills (still ensured) and bundled skills (always loaded).
##Migrations / rollout
No config or data migrations required.
Safer in CI/sandboxed environments: nothing to opt-in; just rebuild/redeploy.
Optional: rerun npm run build and the affected suite (node --test test/agent-runtime.test.mjs) to confirm in your environment.
Testing
npm install
npm run build
node --test test/agent-runtime.test.mjs
Checklist