Repository navigation
Split commons() into two - #414
simonpcouch wants to merge 16 commits into
Conversation
|
Will need to think through DuckDB and context search:
|
This comment was marked as outdated.
This comment was marked as outdated.
|
MCP-CONSOLE ALLOWS US TO EDIT THE TOOL DESCRIPTION NOW YIPPEE |
# Conflicts: # pkg-r/R/commons.R
|
Preview deployed to Connect ( Deployed from commit 0a7131f. |
|
Preview deployed to Connect ( Deployed from commit 0a7131f. |
|
In devrel-agent with Posit Assistant: commons-in-posit-assistant.mov |
|
We should talk about how to make multiple commons agents available in one R session in the docs. |
|
Regardless of anything else you should recommend a fully named call for |
|
[EDIT: this is now done] Should rename commons <- commons(data_sources = data_sources, semantic_layer = semantic_layer, context_layer = context_layer)
agent <- agent(client, commons = commons)
For now, this should be backward-compatible, raising a |
`commons()` now returns the commons: data sources, semantic layer, and context layer, callable without a model. Its arguments must be named. `agent(client, commons = )` builds the agent. Calls that pass a client to `commons()` still return an agent, with a deprecation warning.
There was a problem hiding this comment.
See here for the write-up of the changes.
| #' If the commons has a context layer, `$context()` needs one more step for | ||
| #' now. It relies on DuckDB's `fts` extension, which MCP Console's sandbox | ||
| #' can't download. MCP Console can install the extension itself, but only | ||
| #' into its own cache, which R's duckdb doesn't read by default. Point R at | ||
| #' that cache with an absolute path (`~` and environment variables aren't | ||
| #' expanded): | ||
| #' | ||
| #' ```yaml | ||
| #' environment: | ||
| #' # macOS; on Linux, /home/you/.cache/mcp-console/dependencies/duckdb | ||
| #' DUCKDB_R_HOME: /Users/you/Library/Caches/mcp-console/dependencies/duckdb | ||
| #' ``` | ||
| #' | ||
| #' Then add one more sentence to both `tool.description` and | ||
| #' `tool.instructions`, so the agent asks MCP Console to install the | ||
| #' extension: | ||
| #' | ||
| #' ```markdown | ||
| #' The first time you run `commons$context()`, include | ||
| #' `"requirements": {"duckdb": ["fts"]}` in the same call. | ||
| #' ``` |
There was a problem hiding this comment.
There's a comment above about an issue with the needed extensions for DuckDB and context search. I decided to just temporarily document the workaround for users in favor of #455, removing the needed for ragnar/duckdb in context search altogether.
Another approach could be to have Tomasz add a configuration option in mcp-console that would allow us to pre-install extensions on install.
Agent-written code review from GPT 6.1 Sol — two findingsReviewed ee1da47. [P2] Preserve quoted literals in [P2] Load pending pins before returning their connection ( Both reproduced locally. Focused Edit: addressed both. |
|
I will take a closer look at this in a bit, but what do you think about also having a way to resolve custom SQL that uses dictionary definitions? Ordinary SQL can already be run with I have not thought about this much from an interoperability angle, but it would be helpful (but not critical) for the reproducibility work in #428. It would be useful from an interoperability stance if you want to write custom SQL that uses a dictionary definition without copying and pasting from the dictionary. |
Thanks, this was an oversight on my part. |
$run_sql() expands governed definitions as {{name}} tokens and refuses
writes, as the agent's run_sql tool does. MCP Console now builds commons
from .Rprofile, since its startup block must select a SQL connection.
jat255
left a comment
jat255
left a comment
There was a problem hiding this comment.
Generally, I like this direction. Some small comments and a few little R things identified by Opus (I verified, but definitely wouldn't have found them without it :D).
I'll open an issue to bring Python in sync with this architecture.
| ) | ||
| guarded <- Commons$new( | ||
| client = test_client(), | ||
| test_that("commons() runs measures", { |
There was a problem hiding this comment.
(Called out by Opus 5.5 -- take it or leave it) $calculation(), $context() and $describe() have no tests. All three are new user-facing methods, and should probably have at least one live-path test. For $context(), the test could cover the dictionary-only case, because the docs say it searches dictionaries even without a context layer.
| ) | ||
| ) | ||
| } | ||
| if (!inherits(commons, "commons")) { |
There was a problem hiding this comment.
(Found by Opus 5.5) Calling agent(client) without commons currently fails with base R's argument "commons" is missing, with no default, so the helpful message below never shows. That's probably the first error someone migrating from the one-call commons(client, ...) form will see. This could be fixed by adding rlang::check_required(commons) just before this check, or routing a missing commons into this same message so the commons(data_sources = ...) hint still appears.
| condition <- paste0( | ||
| "^\\s*\"?([A-Za-z_][A-Za-z0-9_]*)\"?\\s*(<=|>=|!=|<>|=|<|>)\\s*", | ||
| "('(?:[^']|'')*'|-?[0-9]+(?:\\.[0-9]+)?)\\s*(?:(?i:AND)\\s+|$)" | ||
| ) |
There was a problem hiding this comment.
(Found by Opus 5.5) The where parser here accepts some malformed input without an error. Each of these passes locally:
"a = 1 AND "(trailingANDwith nothing after it)"a = 1AND b = 2"(no space beforeAND)"\"a = 1"(unbalanced quote around the column name)
None of these is unsafe, but a typo goes through without warning. This version requires the column quotes to come as a pair, and requires whitespace on both sides of AND with another condition after it. The (?|...) branch reset keeps the column in capture group 1, so the rest of the function doesn't need to change:
| condition <- paste0( | |
| "^\\s*\"?([A-Za-z_][A-Za-z0-9_]*)\"?\\s*(<=|>=|!=|<>|=|<|>)\\s*", | |
| "('(?:[^']|'')*'|-?[0-9]+(?:\\.[0-9]+)?)\\s*(?:(?i:AND)\\s+|$)" | |
| ) | |
| condition <- paste0( | |
| "^\\s*(?|\"([A-Za-z_][A-Za-z0-9_]*)\"|([A-Za-z_][A-Za-z0-9_]*))", | |
| "\\s*(<=|>=|!=|<>|=|<|>)\\s*", | |
| "('(?:[^']|'')*'|-?[0-9]+(?:\\.[0-9]+)?)", | |
| "(?:\\s+(?i:AND)\\s+(?=\\S)|\\s*$)" | |
| ) |
There was a problem hiding this comment.
(Found by Opus 5.5) With no context available, $context() on a commons object returns the message "No context layer is configured for this agent." (line 840).
This message can now be reached from the new commons object without an agent configured, so it should say something that fits both callers (maybe just dropping the agent reference)?
(this line wasn't in the diff, so I couldn't put the comment on the exact line)
There was a problem hiding this comment.
(Found by Opus 5.5) On a new commons object with no calculations, $calculation("nope") errors with a message "Available calculations: ." and an empty list (line 154). $metrics("x") does the same with "Available metrics: ." (pool.R:230). The wording predates this PR, but it was only reachable through the agent's tools before. The new $calculation() and $metrics() methods make it something a person could see in the console. Either the hint should be dropped, or replaced with something like "No trusted calculations are defined", when the list is empty.
(These lines weren't in the change set, so this is a file level comment rather than on those specific line.)
| #' argument to [rsconnect::deployApp()] so \pkg{ellmer} includes message | ||
| #' content. A server administrator must also set `OpenTelemetry.Enabled = true` | ||
| #' and `OpenTelemetry.AllowContentInstrumentation = true`. | ||
| #' @section Using commons with other coding agents: |
There was a problem hiding this comment.
This section restates a fair amount of MCP Console configuration (the YAML keys, the config resolve command, and the cache path for DUCKDB_R_HOME), which will probably drift as mcp-console changes (perhaps rapidly in the coming weeks/months). Would it age better to link to the mcp-console docs for the setup and keep only the sentence to add to the tool description here?
There was a problem hiding this comment.
I wonder if in this file we should describe in a dedicated what we mean by a "commons" now (by that I mean the collection of layers, etc.). That's a fairly cute concept that I think is worth calling out explicitly. Not sure if this is the right place for it, or somewhere else in the docs, but I think the concept needs documentation.
...with interoperability in mind. Closes #410. The recommended way to write:
becomes:
Implemented backward-compatibly.
commons()'s output is "just" the data structure that holds the trusted code and output, and not the stuff that hooks into an agent harness so external agents can invoke search and call trusted code and context via methods on the output.For external coding agents, the flow is that they'd register
mcp-consolewithcommons <- commons()in the startup script and a sentence added to the tool descriptuon. For Posit Assistant, this would be an entry in.Rprofileand a sentence in AGENTS.md.