fix(skills): inject active skill roots

[zerob13]

close #1409

Summary by CodeRabbit

• Refactor

  • Updated runtime instructions to use skill metadata for path guidance and script execution recommendations
  • Redesigned file system access controls with improved skill-root based permission validation and path resolution
  • Modified command execution handling and directory allowlist construction for enhanced skill scoping

• Tests

  • Added comprehensive test suite for skill file access permissions and command execution constraints

[coderabbitai]

📝 Walkthrough

Walkthrough

Modified skill runtime instruction generation to use metadata-based skill root references instead of environment variables, restructured agent tool execution to handle exec commands earlier with skill-root-based allowlist building, and added comprehensive tests for skill-constrained file access and execution permissions.

Changes

Cohort / File(s)	Summary
Skill Runtime Instructions src/main/presenter/skillPresenter/index.ts	Updated runtime context to reference metadata.skillRoot instead of environment variables, replaced specific base_directory guidance with general relative path and skill_run preference messaging.
Agent Tool Execution Logic src/main/presenter/toolPresenter/agentTools/agentToolManager.ts	Moved exec handling before filesystem handler initialization; made buildAllowedDirectories asynchronous; replaced getSkillsPath() with resolveActiveSkillRoots(conversationId) for dynamic skill-root-based allowlisting; introduced explicitBaseDirectory parameter for path resolution.
Skill Presenter Tests test/main/presenter/skillPresenter/skillPresenter.test.ts	Updated assertions to verify new skill-content guidance format with backticked skill root values and advisory lines about relative-path behavior and skill_run execution preference.
Agent Tool Manager Skill Access Tests test/main/presenter/toolPresenter/agentTools/agentToolManagerSkillAccess.test.ts	New test suite verifying skill-root-constrained file reading, write permission checks with base_directory validation, and rejection of exec commands with working directory restrictions on active skill roots.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• feat(skill): harden runtime execution #1343: Modifies skill runtime instruction rendering in skillPresenter and adjusts agent tool execution logic for skill-root hardening.
• fix(agent): honor session workdir defaults #1380: Touches the same skill runtime messaging and agent tool execution/working-directory logic across skillPresenter and agentToolManager.
• fix: window and toolcall bug #1252: Introduces base/working-directory handling and restructures exec handling and allowed-directory resolution in agent tool flows.

Poem

🐰 Metadata whispers where envvars once spoke,
Early exec catches prevent the slow folk,
Skill roots now guide, with tests standing tall,
A hop through the permissions—we've secured it all! 🌿✨

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix(skills): inject active skill roots' directly corresponds to the core change: injecting active skill roots into the agent tool manager's allowed directories allowlist instead of using the global skills path.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch codex/skilloops

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

test/main/presenter/toolPresenter/agentTools/agentToolManagerSkillAccess.test.ts (1)

77-116: Consider adding cleanup in afterEach.

The test creates temporary directories that aren't cleaned up after each test. While os.tmpdir() is eventually cleared by the OS, explicit cleanup improves test hygiene and prevents potential issues with test isolation.

♻️ Suggested cleanup

   beforeEach(async () => {
     vi.clearAllMocks()
 
     workspaceDir = await fs.mkdtemp(path.join(os.tmpdir(), 'deepchat-skill-workspace-'))
     skillsDir = await fs.mkdtemp(path.join(os.tmpdir(), 'deepchat-custom-skills-'))
     // ... rest of setup
   })
+
+  afterEach(async () => {
+    await fs.rm(workspaceDir, { recursive: true, force: true }).catch(() => {})
+    await fs.rm(skillsDir, { recursive: true, force: true }).catch(() => {})
+  })

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@test/main/presenter/toolPresenter/agentTools/agentToolManagerSkillAccess.test.ts`
around lines 77 - 116, Add an afterEach cleanup to remove the temporary
directories created in beforeEach (workspaceDir and skillsDir) to avoid leftover
files; implement an async afterEach that checks for non-null workspaceDir and
skillsDir and calls fs.rm (or fs.rmSync) with recursive: true and force: true
(or equivalent) to delete them, ensuring any errors are handled or ignored, and
include this cleanup alongside the existing vi.clearAllMocks to fully restore
test state for the tests in agentToolManagerSkillAccess.test.ts.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In
`@test/main/presenter/toolPresenter/agentTools/agentToolManagerSkillAccess.test.ts`:
- Around line 77-116: Add an afterEach cleanup to remove the temporary
directories created in beforeEach (workspaceDir and skillsDir) to avoid leftover
files; implement an async afterEach that checks for non-null workspaceDir and
skillsDir and calls fs.rm (or fs.rmSync) with recursive: true and force: true
(or equivalent) to delete them, ensuring any errors are handled or ignored, and
include this cleanup alongside the existing vi.clearAllMocks to fully restore
test state for the tests in agentToolManagerSkillAccess.test.ts.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: ba2bd952-7ab7-40d3-ace1-19953539fdf7

📥 Commits

Reviewing files that changed from the base of the PR and between 7625da7 and 5e8e9bf.

📒 Files selected for processing (4)

• src/main/presenter/skillPresenter/index.ts
• src/main/presenter/toolPresenter/agentTools/agentToolManager.ts
• test/main/presenter/skillPresenter/skillPresenter.test.ts
• test/main/presenter/toolPresenter/agentTools/agentToolManagerSkillAccess.test.ts