Skip to content

feat(skills): custom skill directories via btw.skills.paths / BTW_SKILLS_PATHS#193

Merged
gadenbuie merged 7 commits into
mainfrom
feat/btw-help-topics
May 13, 2026
Merged

feat(skills): custom skill directories via btw.skills.paths / BTW_SKILLS_PATHS#193
gadenbuie merged 7 commits into
mainfrom
feat/btw-help-topics

Conversation

@gadenbuie
Copy link
Copy Markdown
Collaborator

Summary

Adds a single btw.skills.paths R option and BTW_SKILLS_PATHS environment 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:

  • Replace semantics: when set, the value entirely replaces user + project defaults (no merging). This is simpler to reason about and is the primary use case for agents and CI pointing at a specific skills directory.
  • Precedence: option beats envvar beats default.
  • Path format: character vector for R options (c("/path/a", "/path/b")), OS-native path separator (: / ;) for the env var. Non-existent paths are silently skipped.
  • Registration-time capture: values are read when btw_tools() / btw_client() is called and closed over in the tool's function, so they survive after transient options set by btw_client() go out of scope.
  • btw.md support: the new flat key options.skills.paths maps directly to btw.skills.paths.

Verification

# Point the skill tool at a custom directory
withr::with_options(
  list(btw.skills.paths = c("~/my-project/skills", "~/.agents/skills")),
  {
    tools <- btw_tools("btw_tool_skill")
    btw_skills_list()  # should include skills from custom dirs
  }
)

# Or via environment variable
Sys.setenv(BTW_SKILLS_PATHS = "~/.agents/skills")
btw_skills_list()

gadenbuie added 7 commits May 13, 2026 11:04
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
@gadenbuie gadenbuie marked this pull request as ready for review May 13, 2026 19:44
@gadenbuie gadenbuie merged commit 2f9660b into main May 13, 2026
9 checks passed
@gadenbuie gadenbuie deleted the feat/btw-help-topics branch May 13, 2026 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant