feat(skills): custom skill directories via btw.skills.paths / BTW_SKILLS_PATHS#193
Merged
Conversation
Add `BTW_SKILLS_DIRS_USER` / `btw.skills.dirs_user` and `BTW_SKILLS_DIRS_PROJECT` / `btw.skills.dirs_project` envvar/option pairs to replace the default user-level and project-level skill directories. - Add `skill_dirs_from_option_or_envvar()`: reads the R option first, falls back to the envvar, splits on OS-native path separator, normalizes paths, returns `NULL` when neither is set. - Add `default_user_skill_dirs()`: encapsulates the legacy + current user-level defaults previously inlined in `btw_skills_directories()`. - Add `default_project_skill_dirs()`: encapsulates the project-level defaults previously inlined in `btw_skills_directories()`. - Update `btw_skills_directories()` to call the new helpers; resolution happens fresh on every call (tool-call time, not registration time). Tests: add 22 tests covering option > envvar > NULL precedence, path splitting/normalization, replace semantics, silent skipping of non-existent paths, and fallback to defaults.
Update the `btw_tool_skill` roxygen block (and its generated .Rd) to document: - The new `btw.skills.dirs_user`/`BTW_SKILLS_DIRS_USER` and `btw.skills.dirs_project`/`BTW_SKILLS_DIRS_PROJECT` pairs via a table - Replace semantics (new value replaces defaults, not appends) - Option > envvar precedence - OS-native path separator format - Resolution timing (at tool-call time, not registration time)
- Delete stale dead-code block at bottom of tool-skills.R (duplicate
definitions of default_user_skill_dirs, default_project_skill_dirs,
and two unused helpers resolve_skill_dirs_envvar_or_option /
parse_skill_dirs). The bottom copies were silently clobbering the
correct implementations above.
- Fix skill_dirs_from_option_or_envvar() to accept a character vector
option value (e.g. c('/a', '/b')) in addition to a path-separator-
delimited string. Character vectors are now the primary documented
idiom; the path-separator form remains available and is still required
for environment variables.
- Add missing %in% duplicate guard to the project-dir loop in
btw_skills_directories(), matching the existing guard on the user-dir
loop.
- Fix tool closing-over bug: the .btw_add_to_tools factory for
btw_tool_skill now captures the resolved option/envvar values at
registration time via local(). Previously, options set transiently by
btw_client() (via withr::local_options) were gone by the time the
tool was invoked, so custom dirs from btw.md were silently ignored.
- Fix misleading deduplication comment in default_user_skill_dirs().
- Update docs to reflect character-vector option support and
registration-time (not call-time) resolution semantics.
- Add 4 new tests (173 total, up from 169):
- char-vector option accepted by skill_dirs_from_option_or_envvar()
- package-bundled skills survive when custom user dirs are set
- no duplicate entry when user and project dirs point at same path
(the project-dir %in% guard is now exercised)
Fixes noted in _dev/agents/plan_skill-dir-options.md review section.
R1: Remove %||% getOption() fallback in tool impl closure. Only non-NULL captured values are now replayed via withr::with_options(); untouched slots resolve naturally from the live environment. This enforces the documented contract (registration-time capture wins when set; defers to live options otherwise). S1: Add two closure tests via btw_tools() to cover the registration- time capture behavior — one asserting frozen values survive option changes, one asserting deferred resolution when nothing was captured. S2: Strengthen NA filter in skill_dirs_from_option_or_envvar() from `nzchar()` to `!is.na() & nzchar()`. Add test for NA in char-vector. S3: Remove redundant local() IIFE wrapper around impl. The function closes correctly over captured_user_dirs / captured_project_dirs without it.
…W_SKILLS_PATHS Replace the separate user-dir and project-dir option/envvar pairs (btw.skills.dirs_user, BTW_SKILLS_DIRS_USER, btw.skills.dirs_project, BTW_SKILLS_DIRS_PROJECT) with a single btw.skills.paths / BTW_SKILLS_PATHS setting. When set, the new value entirely replaces all user-level and project-level skill directories. Package-bundled skills and skills from attached packages are always preserved regardless of the setting. The registration-time capture closure is simplified accordingly — only one captured_paths variable instead of two. Tests and docs updated to match.
…nvvar Also fix btw.md options key: skills.dirs_user → skills.paths
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a single
btw.skills.pathsR option andBTW_SKILLS_PATHSenvironment variable that replace all user-level and project-level skill search directories. Package-bundled skills and skills from attached packages are always preserved.Key design decisions:
c("/path/a", "/path/b")), OS-native path separator (:/;) for the env var. Non-existent paths are silently skipped.btw_tools()/btw_client()is called and closed over in the tool's function, so they survive after transient options set bybtw_client()go out of scope.btw.mdsupport: the new flat keyoptions.skills.pathsmaps directly tobtw.skills.paths.Verification