Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces several S7 property helpers and validation functions, including enum, check_logical_scalar, and check_pos_integer_scalar. It refactors the Field, ToolParameter, and Agent classes to utilize these new helpers, which simplifies their internal validation logic. Additionally, the PR implements a uniqueness check for tool parameter names and updates the available_tools function to use message() for output formatting. Review feedback identifies a missing definition for the check_integer_scalar function, suggests adding explicit NA handling to the enum validator, and recommends standardizing the naming of logical scalar check functions across the package.
| #' try(check_pos_integer_scalar(-1L)) | ||
| #' try(check_pos_integer_scalar(1.5)) | ||
| check_pos_integer_scalar <- function(x, arg_name = deparse(substitute(x))) { | ||
| check_integer_scalar(x, arg_name = arg_name) |
There was a problem hiding this comment.
Pull request overview
This PR expands internal S7 property helpers and validation to tighten schema/tool/agent correctness across the package.
Changes:
- Introduces an
enum()S7 property helper and new check helpers, and uses them inField/ToolParameter. - Adds additional validation (e.g., unique tool parameter names; new
create_agent()argument checks). - Adjusts
available_tools()output behavior (stdout → stderr) and tweaks an Agent duplicate-tool error message.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| r/R/00_init.R | Adds enum() + new check helpers; changes available_tools() to print via message() |
| r/R/01_Schema.R | Refactors Field to use enum(.SCHEMA_FIELD_TYPES) and simplifies validation |
| r/R/04_Tool.R | Refactors ToolParameter to use enum() and adds duplicate parameter-name validation in Tool |
| r/R/07_Agent.R | Adds argument checks in create_agent() and adjusts duplicate tool-name validator messaging |
Comments suppressed due to low confidence (1)
r/R/00_init.R:858
available_tools()switched fromcat()(stdout) tomessage()(stderr). This is a user-visible behavior change and can break callers that capture/pipe stdout (e.g.,capture.output()) while also making the output look like diagnostics. Consider reverting to stdout (cat()/cli::cat_line()/cli::cli_inform()depending on the intended UX) so the function remains a pure “print info” helper.
}
# %% available_tools ----
#' Print built-in tools available for use by agents
#'
#' Prints the R handle (`tool_*`), the `function_name` the model sees, and the
#' description of every built-in `Tool` exported by the package. Derived at
#' call time from the namespace — no hardcoded list.
#'
#' @param verbosity Integer: Verbosity level.
#'
#' @return A named list of `Tool` objects keyed by their R handle, invisibly.
#'
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| param_names <- vapply( | ||
| self@parameters, | ||
| function(p) p@name, | ||
| character(1L), | ||
| USE.NAMES = FALSE | ||
| ) | ||
| if (anyDuplicated(param_names)) { | ||
| cli::cli_abort( | ||
| "Tool parameter names must be unique. Rename duplicate parameters." | ||
| ) | ||
| } |
There was a problem hiding this comment.
New validation rejects duplicate tool parameter names, but there’s no test covering this behavior (and it’s easy to regress). Add a unit test that constructs a Tool with two parameters sharing the same name and asserts the expected error.
| ) { | ||
| check_optional_scalar_character(system_prompt, "system_prompt") | ||
| check_scalar_logical(use_memory, "use_memory") | ||
| check_pos_integer_scalar(max_tool_rounds, "max_tool_rounds") |
There was a problem hiding this comment.
create_agent() now validates max_tool_rounds via check_pos_integer_scalar(), but there’s no test asserting that invalid values (0, negative, non-integer) are rejected (or that valid values are accepted). Adding a small test around this new argument validation will help prevent accidental loosening/tightening of the API contract.
No description provided.