diff --git a/AGENTS.md b/AGENTS.md index 3a9445cf..cc7696a6 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -134,7 +134,7 @@ Follow these style rules: ... btw looks for these files in: 1. Current working directory or its parents (project-specific) -2. `~/.config/btw/btw.md` or `~/btw.md` (global defaults) +2. `~/.btw/btw.md`, `~/.config/btw/btw.md`, or `~/btw.md` (global defaults) Content becomes part of the system prompt. Use `` / `` comments to exclude sections from the prompt. @@ -302,6 +302,7 @@ Edit files in `inst/prompts/`. These are assembled in `btw_client()` based on co - **All tool wrappers accept `_intent` parameter** (added automatically by `wrap_with_intent()`) - **Snapshot tests for tool outputs** to catch formatting changes - **Use `check_*()` functions** argument validation, described below +- **Suppress cli messages in tests** - `expect_message(expr, pattern)` only absorbs the first *matching* message; non-matching messages (e.g. a `cli_alert_info()` emitted before the one under test) leak to the console even when the test passes. Wrap with `suppressMessages(expect_message(...))` when secondary messages are expected but unimportant, or use `suppressMessages(expr)` alone when the assertion is a side-effect rather than the message content. ### Type Check Functions @@ -339,6 +340,7 @@ Edit files in `inst/prompts/`. These are assembled in `btw_client()` based on co - **Git and GitHub commands** - `@git` and `@issue`/`@pr` commands require gert and gh packages respectively, plus appropriate repository access - **MCP server blocks** the R process - run non-interactively - **Test snapshots require frequent updates** as output formatting evolves +- **`btw_user_dirs()` is the single source of truth** for user-level config locations (`~/.btw`, `~/.config/btw`, `tools::R_user_dir("btw")`). Any code that discovers or installs user-level skills, agents, or `btw.md` files must use this helper — never hardcode individual paths ## Resources diff --git a/NEWS.md b/NEWS.md index b6ffa7c8..29901c59 100644 --- a/NEWS.md +++ b/NEWS.md @@ -2,6 +2,8 @@ ## Bug fixes +* User-level config locations are now consistent across skills, agents, and `btw.md` discovery. Skills installed with btw v1.2.0 are still discovered for backwards compatibility (#182). + * The `btw` CLI now loads `datasets`, `utils`, `stats`, and `methods` by declaring them in the Rapp `#| launcher:` frontmatter, reducing surprises for users who expect standard R packages to be available (#181). # btw 1.2.0 diff --git a/R/tool-skills.R b/R/tool-skills.R index 9b4ff5e2..e7393552 100644 --- a/R/tool-skills.R +++ b/R/tool-skills.R @@ -23,7 +23,10 @@ NULL #' 1. Skills bundled with the btw package itself #' 2. Skills from currently **attached** R packages — any package with an #' `inst/skills/` directory that is loaded via [library()] or [require()] -#' 3. User-level skills (`tools::R_user_dir("btw", "config")/skills`) +#' 3. User-level skills (`~/.btw/skills`, `~/.config/btw/skills`, +#' `tools::R_user_dir("btw")/skills`). For backwards compatibility, the +#' legacy `tools::R_user_dir("btw", "config")/skills` path used by briefly +#' by btw 1.2.0 is also included at lower priority. #' 4. Project-level skills (`.btw/skills/` or `.agents/skills/`) #' #' @param name The name of the skill to load, or `""` to list all available @@ -156,13 +159,19 @@ btw_skills_directories <- function(project_dir = getwd()) { # Skills from attached packages dirs <- c(dirs, attached_package_skill_dirs()) - # User-level skills (global installation) - user_skills_dir <- file.path( - tools::R_user_dir("btw", "config"), - "skills" - ) - if (dir.exists(user_skills_dir)) { - dirs <- c(dirs, user_skills_dir) + # Legacy: btw <= 1.2.0 install target — kept for backwards compatibility only, + # never written to by newer versions + legacy_skills_dir <- file.path(tools::R_user_dir("btw", "config"), "skills") + if (dir.exists(legacy_skills_dir)) { + dirs <- c(dirs, legacy_skills_dir) + } + + # User-level skills from btw_user_dirs() in increasing priority order + for (user_dir in rev(btw_user_dirs())) { + user_skills_dir <- file.path(user_dir, "skills") + if (dir.exists(user_skills_dir) && !user_skills_dir %in% dirs) { + dirs <- c(dirs, user_skills_dir) + } } # Project-level skills from multiple conventions @@ -313,8 +322,11 @@ btw_skills_list <- function() { find_skill <- function(skill_name) { skill_dirs <- btw_skills_directories() + # Search in reverse priority order so higher-priority sources win, consistent + # with btw_skills_list() which uses last-wins semantics. + # Fast path: directory named skill_name - for (dir in skill_dirs) { + for (dir in rev(skill_dirs)) { skill_dir <- file.path(dir, skill_name) skill_md_path <- file.path(skill_dir, "SKILL.md") if (dir.exists(skill_dir) && file.exists(skill_md_path)) { @@ -328,10 +340,9 @@ find_skill <- function(skill_name) { } # Slow path: scan all skills for a matching metadata$name (handles name/dir - # mismatch). We use extract_skill_metadata() first to avoid full validation # on every non-matching skill, then validate only when we find a match. - for (dir in skill_dirs) { + for (dir in rev(skill_dirs)) { subdirs <- list.dirs(dir, full.names = TRUE, recursive = FALSE) for (subdir in subdirs) { skill_md_path <- file.path(subdir, "SKILL.md") @@ -738,11 +749,23 @@ resolve_skill_scope <- function(scope, error_call = caller_env()) { switch( scope, project = resolve_project_skill_dir(error_call = error_call), - user = file.path(tools::R_user_dir("btw", "config"), "skills"), + user = resolve_user_skill_dir(), scope ) } +resolve_user_skill_dir <- function() { + candidates <- file.path(btw_user_dirs(), "skills") + existing <- candidates[dir.exists(candidates)] + # Require content, not just existence: an empty directory (e.g. one left + # behind after removing a skill) should not cause subsequent installs to + # land there instead of the preferred default (~/.btw/skills). + non_empty <- existing[map_lgl(existing, function(d) { + length(list.files(d)) > 0 + })] + if (length(non_empty) > 0) non_empty[[1]] else candidates[[1]] +} + select_skill_dir <- function( skill_dirs, skill = NULL, @@ -813,8 +836,9 @@ select_skill_dir <- function( #' chosen from `.btw/skills/` or `.agents/skills/` #' in that order. If one already exists, it is used; otherwise #' `.btw/skills/` is created. -#' - `"user"`: Installs to the user-level skills directory -#' (`tools::R_user_dir("btw", "config")/skills`). +#' - `"user"`: Installs to the first of `~/.btw/skills`, +#' `~/.config/btw/skills`, or `tools::R_user_dir("btw")/skills` that +#' already exists, defaulting to `~/.btw/skills` if none do. #' - A directory path: Installs to a custom directory, e.g. #' `scope = ".openhands/skills"`. Use `I("project")` or `I("user")` #' if you need a literal directory with those names. @@ -917,8 +941,9 @@ btw_skill_install_github <- function( #' chosen from `.btw/skills/` or `.agents/skills/` #' in that order. If one already exists, it is used; otherwise #' `.btw/skills/` is created. -#' - `"user"`: Installs to the user-level skills directory -#' (`tools::R_user_dir("btw", "config")/skills`). +#' - `"user"`: Installs to the first of `~/.btw/skills`, +#' `~/.config/btw/skills`, or `tools::R_user_dir("btw")/skills` that +#' already exists, defaulting to `~/.btw/skills` if none do. #' - A directory path: Installs to a custom directory, e.g. #' `scope = ".openhands/skills"`. Use `I("project")` or `I("user")` #' if you need a literal directory with those names. diff --git a/R/utils.R b/R/utils.R index a2299571..e018c37a 100644 --- a/R/utils.R +++ b/R/utils.R @@ -152,17 +152,28 @@ path_find_in_project <- function(filename, dir = getwd()) { path_find_in_project(filename, dirname(dir)) } +# Returns user-level config directories in decreasing priority order. +# Consumers iterate and use the first match (or rev() when building +# an increasing-priority list like btw_skills_directories()). +btw_user_dirs <- function() { + c( + fs::path_home(".btw"), + fs::path_home(".config", "btw"), + tools::R_user_dir("btw") + ) +} + path_find_user <- function(filename) { if (identical(Sys.getenv("TESTTHAT"), "true")) { # In testthat, we don't want to use the home directory return(NULL) } - possibilities <- c( + possibilities <- unique(c( fs::path_home(filename), fs::path_home_r(filename), - fs::path_home(".config", "btw", filename) - ) + file.path(btw_user_dirs(), filename) + )) for (path in possibilities) { if (fs::file_exists(path)) { @@ -191,19 +202,14 @@ find_project_agent_files <- function(dir = getwd()) { as.character(files) } -# Find agent-*.md files in user config directories (~/.btw/, ~/.config/btw/) +# Find agent-*.md files in user config directories find_user_agent_files <- function() { if (identical(Sys.getenv("TESTTHAT"), "true")) { return(character()) } - user_dirs <- c( - fs::path_home(".btw"), - fs::path_home(".config", "btw") - ) - files <- character() - for (dir in user_dirs) { + for (dir in btw_user_dirs()) { if (fs::dir_exists(dir)) { found <- fs::dir_ls(dir, regexp = "agent-.*\\.md$", type = "file") files <- c(files, as.character(found)) diff --git a/man/btw_skill_install_github.Rd b/man/btw_skill_install_github.Rd index 7b246b5f..c22ddf16 100644 --- a/man/btw_skill_install_github.Rd +++ b/man/btw_skill_install_github.Rd @@ -27,8 +27,9 @@ non-interactive sessions).} chosen from \verb{.btw/skills/} or \verb{.agents/skills/} in that order. If one already exists, it is used; otherwise \verb{.btw/skills/} is created. -\item \code{"user"}: Installs to the user-level skills directory -(\code{tools::R_user_dir("btw", "config")/skills}). +\item \code{"user"}: Installs to the first of \verb{~/.btw/skills}, +\verb{~/.config/btw/skills}, or \code{tools::R_user_dir("btw")/skills} that +already exists, defaulting to \verb{~/.btw/skills} if none do. \item A directory path: Installs to a custom directory, e.g. \code{scope = ".openhands/skills"}. Use \code{I("project")} or \code{I("user")} if you need a literal directory with those names. diff --git a/man/btw_skill_install_package.Rd b/man/btw_skill_install_package.Rd index 77310202..94ed132e 100644 --- a/man/btw_skill_install_package.Rd +++ b/man/btw_skill_install_package.Rd @@ -24,8 +24,9 @@ non-interactive sessions).} chosen from \verb{.btw/skills/} or \verb{.agents/skills/} in that order. If one already exists, it is used; otherwise \verb{.btw/skills/} is created. -\item \code{"user"}: Installs to the user-level skills directory -(\code{tools::R_user_dir("btw", "config")/skills}). +\item \code{"user"}: Installs to the first of \verb{~/.btw/skills}, +\verb{~/.config/btw/skills}, or \code{tools::R_user_dir("btw")/skills} that +already exists, defaulting to \verb{~/.btw/skills} if none do. \item A directory path: Installs to a custom directory, e.g. \code{scope = ".openhands/skills"}. Use \code{I("project")} or \code{I("user")} if you need a literal directory with those names. diff --git a/man/btw_tool_skill.Rd b/man/btw_tool_skill.Rd index a5a66190..83753f65 100644 --- a/man/btw_tool_skill.Rd +++ b/man/btw_tool_skill.Rd @@ -38,7 +38,10 @@ priority (later sources override earlier ones when skill names conflict): \item Skills bundled with the btw package itself \item Skills from currently \strong{attached} R packages — any package with an \verb{inst/skills/} directory that is loaded via \code{\link[=library]{library()}} or \code{\link[=require]{require()}} -\item User-level skills (\code{tools::R_user_dir("btw", "config")/skills}) +\item User-level skills (\verb{~/.btw/skills}, \verb{~/.config/btw/skills}, +\code{tools::R_user_dir("btw")/skills}). For backwards compatibility, the +legacy \code{tools::R_user_dir("btw", "config")/skills} path used by briefly +by btw 1.2.0 is also included at lower priority. \item Project-level skills (\verb{.btw/skills/} or \verb{.agents/skills/}) } } diff --git a/tests/testthat/test-tool_skills.R b/tests/testthat/test-tool_skills.R index 6d7f23bc..edb80476 100644 --- a/tests/testthat/test-tool_skills.R +++ b/tests/testthat/test-tool_skills.R @@ -355,6 +355,40 @@ test_that("find_skill() finds a valid skill", { expect_true(file.exists(result$path)) }) +test_that("find_skill() returns the highest-priority version when skill exists in multiple dirs", { + low_dir <- withr::local_tempdir() + high_dir <- withr::local_tempdir() + create_temp_skill(name = "my-skill", description = "Low priority version", dir = low_dir) + create_temp_skill(name = "my-skill", description = "High priority version", dir = high_dir) + # high_dir last = higher priority (consistent with btw_skills_directories() semantics) + local_skill_dirs(c(low_dir, high_dir)) + + result <- find_skill("my-skill") + fm <- frontmatter::read_front_matter(result$path) + expect_equal(fm$data$description, "High priority version") +}) + +# resolve_user_skill_dir() ------------------------------------------------- + +test_that("resolve_user_skill_dir() returns first non-empty existing candidate", { + dir1 <- withr::local_tempdir() + dir2 <- withr::local_tempdir() + skills2 <- file.path(dir2, "skills") + dir.create(skills2) + writeLines("x", file.path(skills2, "placeholder")) + + local_mocked_bindings(btw_user_dirs = function() c(dir1, dir2)) + expect_equal(resolve_user_skill_dir(), skills2) +}) + +test_that("resolve_user_skill_dir() falls back to first candidate when none have content", { + dir1 <- withr::local_tempdir() + dir2 <- withr::local_tempdir() + + local_mocked_bindings(btw_user_dirs = function() c(dir1, dir2)) + expect_equal(resolve_user_skill_dir(), file.path(dir1, "skills")) +}) + # extract_skill_metadata --------------------------------------------------- test_that("extract_skill_metadata() returns parsed frontmatter", {