feat: support connectionFilePath and skills overhaul for secrets management#1432
Merged
douenergy merged 14 commits intoCanner:mainfrom Mar 9, 2026
Merged
feat: support connectionFilePath and skills overhaul for secrets management#1432douenergy merged 14 commits intoCanner:mainfrom
douenergy merged 14 commits intoCanner:mainfrom
Conversation
…LLM context Add optional `connectionFilePath` parameter to query/validate/metadata DTOs. When provided, ibis-server reads the JSON file directly instead of requiring inline `connectionInfo`, so passwords and credentials never pass through the LLM. Raises `WrenError(INVALID_CONNECTION_INFO)` when neither field is given. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…w (v1.2) Update skill to never ask for or handle sensitive credentials. LLM leaves sensitive fields as TODO comments; ibis-server reads connection.json directly via connectionFilePath so secrets stay out of the conversation. Adds BigQuery base64 credential instructions and opt-in inline mode for test environments. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…d generate-mdl Connection info details (per-connector fields, secrets policy, BigQuery base64 instructions, Mode A/B workflow) were duplicated across both skills. Consolidate into a new wren-connection-info skill. generate-mdl now delegates Step 1 to wren-connection-info before introspecting the schema. wren-project Typical Workflow is reordered so connection setup always comes first. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…cy auto-install - Add wren-usage skill as the main entry point for day-to-day Wren usage; it handles skill installation (Step 0) and delegates to focused sub-skills - Add wren-connection-info and wren-usage to install.sh ALL_SKILLS - Add dependency auto-install to install.sh: installing a skill now automatically installs its declared dependencies from index.json - Add skills/index.json as hub catalog with tags, dependencies, and repository links per skill (replaces sparse versions.json for hub use) - Add skills/check-versions.sh to verify versions.json ↔ SKILL.md parity - Add .github/workflows/skills-check.yml to run parity check on PRs - Add license: Apache-2.0 to all SKILL.md frontmatter - Move skill installation phase from wren-quickstart to wren-usage; wren-quickstart now starts from workspace creation (Phase 1) - Update AUTHORING.md with registration checklist and release workflow - Update README.md and SKILLS.md to reflect new skill catalog Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ntial access control In security mode (default), the LLM must never read connection.yml or target/connection.json without explicit user confirmation. Inline mode allows free access for test/dev environments. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
github-actions
bot
added
documentation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds support for file-based connection information ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Router as Router (v2/v3)
participant DTO as DTO
participant Resolver as resolve_connection_info
participant FS as FileSystem
participant DataSource as DataSource
Client->>Router: POST /query (body with connectionInfo or connectionFilePath)
Router->>DTO: parse → DTO instance
Router->>Resolver: resolve_connection_info(DTO)
alt connectionFilePath present
Resolver->>FS: open/read JSON file (path must be inside CONNECTION_FILE_ROOT)
FS-->>Resolver: return connectionInfo dict
else inline connectionInfo present
Resolver-->>Resolver: return DTO.connection_info
end
Resolver-->>Router: resolved connection_info dict or raise WrenError(INVALID_CONNECTION_INFO)
Router->>DataSource: data_source.get_connection_info(connection_info, headers)
DataSource-->>Router: result or error
Router-->>Client: response (results or WrenError payload)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
skills/wren-quickstart/SKILL.md (1)
15-16:⚠️ Potential issue | 🟠 MajorUpdate the version-check instructions to use
skills/index.json.Line 15 and Line 16 still point to
versions.jsonand awren-quickstartkey, but this PR moves skill metadata toskills/index.json. As written, the update check will read the wrong file/shape and can’t reliably detect newer quickstart versions.Suggested doc fix
-Silently fetch `https://raw.githubusercontent.com/Canner/wren-engine/main/skills/versions.json`. -Compare the `wren-quickstart` key with this skill's version (`1.0`). +Silently fetch `https://raw.githubusercontent.com/Canner/wren-engine/main/skills/index.json`. +Find the entry whose `name` is `wren-quickstart`, then compare its `version` with this skill's version (`1.0`).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/wren-quickstart/SKILL.md` around lines 15 - 16, The instructions currently tell readers to fetch versions.json and check a wren-quickstart key; update them to fetch skills/index.json instead and describe comparing this skill's entry in that index (look up the skill by its canonical id/name used in skills/index.json) against the local version (`1.0`) to detect updates; specifically replace references to `https://raw.githubusercontent.com/Canner/wren-engine/main/skills/versions.json` and the `wren-quickstart` key with the `skills/index.json` URL and the appropriate skill identifier (as used in skills/index.json) and note the field name to compare (version) so the check reads the correct file shape.
🧹 Nitpick comments (6)
skills/check-versions.sh (3)
10-13: Simplify skill name parsing.The current parsing (removing quotes, splitting on colon, trimming spaces) is fragile. Since you're already using Python to read the JSON, consider extracting skill names directly:
♻️ Cleaner approach
-while IFS= read -r skill; do - skill_name="${skill//\"/}" - skill_name="${skill_name%%:*}" - skill_name="${skill_name// /}" +while IFS= read -r skill_name; doThe loop input at line 32 already prints just the keys, so the additional parsing may be unnecessary. Verify that Python outputs clean key names without quotes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/check-versions.sh` around lines 10 - 13, The parsing of skill names in the while loop (variable skill -> skill_name) is fragile; instead, rely on the Python step that prints keys so you can remove the manual quote/colon/space stripping. Update the loop that reads "skill" to assume clean key names and delete the transformations using skill_name="${skill//\"/}", skill_name="${skill_name%%:*}", and skill_name="${skill_name// /}"; if necessary, adjust the Python command that generates the loop input to emit plain keys (no quotes or colons) so the rest of the script can use the loop variable directly.
24-24: Version extraction may match non-frontmatter lines.The
grep -m1 'version:'approach assumes the first occurrence is in the YAML frontmatter. If a SKILL.md containsversion:in a code block or example before the frontmatter ends, this could extract the wrong value.Consider a more robust approach that only parses within the frontmatter delimiters (
---).♻️ More robust frontmatter parsing
- md_version=$(grep -m1 'version:' "$skill_file" | sed 's/.*version: *"\{0,1\}\([^"]*\)"\{0,1\}/\1/' | tr -d ' "') + # Extract version from YAML frontmatter only (between first two --- lines) + md_version=$(awk '/^---$/{if(++c==2)exit} c==1 && /version:/{gsub(/.*version: *"?|"? *$/,""); print; exit}' "$skill_file")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/check-versions.sh` at line 24, The current extraction using md_version=$(grep -m1 'version:' "$skill_file" ...) can pick up a later "version:" outside YAML frontmatter; update check-versions.sh to first isolate the frontmatter region delimited by --- from the beginning of $skill_file (e.g., read until the second ---) and then search that isolated block for the version key, assigning the result to md_version; reference the existing md_version variable and $skill_file when implementing the change so you only grep/sed/awk within the frontmatter slice rather than the whole file.
1-41: Consider addingindex.jsonvalidation.Per
AUTHORING.mdline 107, bothversions.jsonandindex.jsonmust stay in sync with SKILL.md versions. This script currently only validatesversions.json. Consider extending it to also checkindex.jsonentries, or document thatindex.jsonvalidation is out of scope.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/check-versions.sh` around lines 1 - 41, The script only validates VERSIONS_JSON; update check-versions.sh to also load and validate index.json (e.g., add INDEX_JSON variable) and compare the SKILL.md frontmatter versions against entries in both JSON files (the same loop that reads keys from VERSIONS_JSON should be extended or duplicated to read keys/values from INDEX_JSON using the same python/json logic), increment ERRORS and emit clear MISMATCH or MISSING messages for index.json just like for versions.json, and handle the case where index.json is missing or an entry is absent (treat as MISSING); reference variables/functions to change: VERSIONS_JSON, the python3 key-loading invocation at the bottom, the json_version lookup line, and the ERRORS handling so index.json validation integrates with the existing exit behavior (and optionally mention AUTHORING.md line 107 in the message or comments).skills/AUTHORING.md (1)
103-105: Minor style note: consider varying sentence structure.Three successive steps begin with "Add". While this is standard for procedural documentation, you could optionally vary the phrasing (e.g., "Register the skill in..." or "Include an entry in...") for readability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/AUTHORING.md` around lines 103 - 105, Revise the three consecutive step lines that all start with "Add" to vary sentence structure for readability: change the step referencing versions.json to something like "Register the skill name and version in versions.json", change the step for index.json to "Include an entry in index.json with name, version, description, tags, dependencies (if any), and repository", and change the install.sh step to "Append the skill to the ALL_SKILLS array in install.sh"; keep the same required fields and meaning but vary the verbs to avoid repetition.skills/generate-mdl/SKILL.md (1)
52-59: Consider adding a language specifier to the fenced code block.The code block starting at line 52 lacks a language identifier. Adding
httportextwould improve syntax highlighting and satisfy markdown linting.📝 Suggested fix
-``` +```http POST http://localhost:8000/v3/connector/<data_source>/metadata/tables🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/generate-mdl/SKILL.md` around lines 52 - 59, The fenced code block containing the HTTP request example (the block starting with "POST http://localhost:8000/v3/connector/<data_source>/metadata/tables") lacks a language specifier; update the opening fence from ``` to ```http (or ```text) so the block uses HTTP syntax highlighting and satisfies markdown linting rules.skills/install.sh (1)
67-71: Minor edge case in empty array check.The
${result[@]:-}syntax for defaulting an empty array may behave unexpectedly in older Bash versions (< 4.4) withset -uenabled. If the array is empty, some Bash versions treat${result[@]}as an unbound variable error even with the default.Consider using explicit length check for broader compatibility:
♻️ Suggested improvement
skill_in_result() { local s="$1" + [ "${`#result`[@]}" -eq 0 ] && return 1 - for r in "${result[@]:-}"; do [ "$r" = "$s" ] && return 0; done + for r in "${result[@]}"; do [ "$r" = "$s" ] && return 0; done return 1 }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/install.sh` around lines 67 - 71, The skill_in_result function uses the parameter expansion ${result[@]:-} which can trigger unbound variable errors in older Bash with set -u; change the logic to first check the array length using the explicit length expansion (use ${`#result`[@]} with a safe default) and return/fall through if empty, then iterate over "${result[@]}" only when length > 0; update function skill_in_result to use this explicit length check to ensure compatibility across Bash versions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ibis-server/app/util.py`:
- Around line 58-64: The resolve_connection_info function should catch file I/O
and JSON parsing errors and raise a structured WrenError instead of letting
exceptions propagate: wrap the open/json.load block in try/except that catches
FileNotFoundError and json.JSONDecodeError (and optionally
PermissionError/OSError) and raise WrenError(INVALID_CONNECTION_INFO, f"...",
details=...) with a clear message and any useful detail (e.g., the path and
original exception message); keep returning dto.connection_info when no
connection_file_path is provided. Ensure you reference resolve_connection_info
and use the INVALID_CONNECTION_INFO WrenError code so callers receive the
expected structured error.
- Around line 60-61: Validate that dto.connection_file_path resolves inside an
allowed base directory before using it: compute the resolved path (path) as
shown, compute/obtain the allowed base directory (e.g., a constant or config
like ALLOWED_CONNECTION_DIR), and verify the resolved path is a descendant of
that base (use pathlib.Path(...).resolve() and Path.is_relative_to(base) or
compare os.path.commonpath). If the check fails, raise an exception or log and
refuse to open the file; only proceed to open/read the file when the path
validation passes. Reference dto.connection_file_path and the local variable
path in your change.
In `@skills/AUTHORING.md`:
- Line 107: The documentation and tooling are out of sync: update the validation
script check-versions.sh to also verify that index.json matches the SKILL.md
frontmatter version (in addition to versions.json) so both files are enforced,
by adding logic to read and compare the version field from SKILL.md against the
version entries in versions.json and index.json and fail with a clear message
when they differ; alternatively, if you prefer not to change the script, update
AUTHORING.md to clearly state that check-versions.sh only validates
versions.json and document the manual step to keep index.json in sync.
In `@skills/wren-connection-info/SKILL.md`:
- Around line 193-198: The link fragment `#field-mapping` is incorrect because
the actual header is "## Field mapping (YAML → JSON)"; update the reference in
the Mode B output section to match the generated slug or simplify the header to
a plain "## Field mapping" so the anchor resolves. Specifically, either change
the link `#field-mapping` to the correct slug derived from "Field mapping (YAML
→ JSON)" or rename the header to "Field mapping" (or add an explicit anchor) so
readers clicking the link land on the intended section.
In `@skills/wren-project/SKILL.md`:
- Around line 15-18: Update the example update notification in SKILL.md so the
installed version matches the frontmatter (change the example "installed: 1.3"
to "installed: 1.4"); locate the `wren-project` key and the notification example
string ("A newer version of the **wren-project** skill is available (remote:
X.Y, installed: 1.3).") and replace the installed version to "1.4" for
consistency with the frontmatter.
In `@skills/wren-quickstart/SKILL.md`:
- Around line 64-66: The subsection label "3a — Generate MDL" is stale under the
"Phase 2 — Generate MDL and save project" heading; update that subsection title
(the line containing "3a — Generate MDL") to match Phase 2 numbering (e.g.,
change to "2a — Generate MDL") so the walkthrough and quick reference are
consistent with the surrounding Phase 2 headings and references in SKILL.md.
---
Outside diff comments:
In `@skills/wren-quickstart/SKILL.md`:
- Around line 15-16: The instructions currently tell readers to fetch
versions.json and check a wren-quickstart key; update them to fetch
skills/index.json instead and describe comparing this skill's entry in that
index (look up the skill by its canonical id/name used in skills/index.json)
against the local version (`1.0`) to detect updates; specifically replace
references to
`https://raw.githubusercontent.com/Canner/wren-engine/main/skills/versions.json`
and the `wren-quickstart` key with the `skills/index.json` URL and the
appropriate skill identifier (as used in skills/index.json) and note the field
name to compare (version) so the check reads the correct file shape.
---
Nitpick comments:
In `@skills/AUTHORING.md`:
- Around line 103-105: Revise the three consecutive step lines that all start
with "Add" to vary sentence structure for readability: change the step
referencing versions.json to something like "Register the skill name and version
in versions.json", change the step for index.json to "Include an entry in
index.json with name, version, description, tags, dependencies (if any), and
repository", and change the install.sh step to "Append the skill to the
ALL_SKILLS array in install.sh"; keep the same required fields and meaning but
vary the verbs to avoid repetition.
In `@skills/check-versions.sh`:
- Around line 10-13: The parsing of skill names in the while loop (variable
skill -> skill_name) is fragile; instead, rely on the Python step that prints
keys so you can remove the manual quote/colon/space stripping. Update the loop
that reads "skill" to assume clean key names and delete the transformations
using skill_name="${skill//\"/}", skill_name="${skill_name%%:*}", and
skill_name="${skill_name// /}"; if necessary, adjust the Python command that
generates the loop input to emit plain keys (no quotes or colons) so the rest of
the script can use the loop variable directly.
- Line 24: The current extraction using md_version=$(grep -m1 'version:'
"$skill_file" ...) can pick up a later "version:" outside YAML frontmatter;
update check-versions.sh to first isolate the frontmatter region delimited by
--- from the beginning of $skill_file (e.g., read until the second ---) and then
search that isolated block for the version key, assigning the result to
md_version; reference the existing md_version variable and $skill_file when
implementing the change so you only grep/sed/awk within the frontmatter slice
rather than the whole file.
- Around line 1-41: The script only validates VERSIONS_JSON; update
check-versions.sh to also load and validate index.json (e.g., add INDEX_JSON
variable) and compare the SKILL.md frontmatter versions against entries in both
JSON files (the same loop that reads keys from VERSIONS_JSON should be extended
or duplicated to read keys/values from INDEX_JSON using the same python/json
logic), increment ERRORS and emit clear MISMATCH or MISSING messages for
index.json just like for versions.json, and handle the case where index.json is
missing or an entry is absent (treat as MISSING); reference variables/functions
to change: VERSIONS_JSON, the python3 key-loading invocation at the bottom, the
json_version lookup line, and the ERRORS handling so index.json validation
integrates with the existing exit behavior (and optionally mention AUTHORING.md
line 107 in the message or comments).
In `@skills/generate-mdl/SKILL.md`:
- Around line 52-59: The fenced code block containing the HTTP request example
(the block starting with "POST
http://localhost:8000/v3/connector/<data_source>/metadata/tables") lacks a
language specifier; update the opening fence from ``` to ```http (or ```text) so
the block uses HTTP syntax highlighting and satisfies markdown linting rules.
In `@skills/install.sh`:
- Around line 67-71: The skill_in_result function uses the parameter expansion
${result[@]:-} which can trigger unbound variable errors in older Bash with set
-u; change the logic to first check the array length using the explicit length
expansion (use ${`#result`[@]} with a safe default) and return/fall through if
empty, then iterate over "${result[@]}" only when length > 0; update function
skill_in_result to use this explicit length check to ensure compatibility across
Bash versions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8543e873-ac59-4a32-837f-fa0f905a8b29
📒 Files selected for processing (23)
.github/workflows/skills-check.ymlibis-server/app/dependencies.pyibis-server/app/model/__init__.pyibis-server/app/model/metadata/dto.pyibis-server/app/routers/v2/connector.pyibis-server/app/routers/v3/connector.pyibis-server/app/util.pyibis-server/tests/routers/v2/connector/test_postgres.pyibis-server/tests/routers/v3/connector/postgres/test_fallback_v2.pyibis-server/tests/routers/v3/connector/postgres/test_query.pyskills/AUTHORING.mdskills/README.mdskills/SKILLS.mdskills/check-versions.shskills/generate-mdl/SKILL.mdskills/index.jsonskills/install.shskills/versions.jsonskills/wren-connection-info/SKILL.mdskills/wren-mcp-setup/SKILL.mdskills/wren-project/SKILL.mdskills/wren-quickstart/SKILL.mdskills/wren-usage/SKILL.md
…rror format Update test_query_without_connection_info in oracle and trino to expect the WrenError INVALID_CONNECTION_INFO response instead of the old Pydantic 422 detail format, matching the change made to postgres in bb95d1a. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tionInfo error format Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add FileNotFoundError/JSONDecodeError handling in resolve_connection_info to return structured WrenError instead of HTTP 500 - Fix stale version in wren-project SKILL.md example notification (1.3→1.4) - Fix stale subsection label in wren-quickstart SKILL.md (3a→2a) - Simplify field mapping header in wren-connection-info SKILL.md so #field-mapping anchor resolves correctly - Clarify AUTHORING.md that check-versions.sh only validates versions.json Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@skills/wren-connection-info/SKILL.md`:
- Around line 1-8: The frontmatter in SKILL.md sets version "1.1" while the
skills registry still lists "wren-connection-info" as "1.0"; update the entry
for "wren-connection-info" in skills/index.json to "1.1" so the registry matches
the SKILL.md frontmatter (verify the key name "wren-connection-info" and the
version string are identical).
- Around line 202-213: The field mapping table is incomplete: add the missing
snake_case→camelCase mappings used by the ConnectionInfo models (include
ssl_mode → sslMode and ssl_ca → sslCA for MySqlConnectionInfo, and
connection_url → connectionUrl for ConnectionUrl) and update the explanatory
note so it no longer claims "all other fields remain unchanged"; alternatively
replace the explicit table with a concise general rule ("convert all snake_case
keys to camelCase for JSON output") and list exceptions if any, ensuring
references to MySqlConnectionInfo and ConnectionUrl are mentioned so reviewers
can locate the affected mappings.
In `@skills/wren-project/SKILL.md`:
- Around line 7-18: The SKILL.md declares the skill version as "1.4" but the
project's versions index still lists the wren-project entry as "1.3", causing
the version-check logic to misreport updates; open the versions index that
contains the "wren-project" key and update its value from "1.3" to "1.4" so the
remote versions.json matches the SKILL.md frontmatter and the comparison logic
(the version check that prints "A newer version of the **wren-project** skill is
available (remote: X.Y, installed: 1.4).") behaves correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1b5f443e-6284-484e-b6a5-31a777eb9078
📒 Files selected for processing (10)
ibis-server/app/util.pyibis-server/tests/routers/v2/connector/test_clickhouse.pyibis-server/tests/routers/v2/connector/test_mssql.pyibis-server/tests/routers/v2/connector/test_mysql.pyibis-server/tests/routers/v2/connector/test_oracle.pyibis-server/tests/routers/v2/connector/test_trino.pyskills/AUTHORING.mdskills/wren-connection-info/SKILL.mdskills/wren-project/SKILL.mdskills/wren-quickstart/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (1)
- ibis-server/app/util.py
- Bump wren-connection-info in index.json 1.0→1.1 - Bump wren-project in index.json 1.3→1.4 - Add missing snake_case→camelCase mappings to wren-connection-info field mapping table (sslMode, sslCA, connectionUrl, httpPath, accessToken, s3StagingDir, awsAccessKeyId, awsSecretAccessKey) - Clarify that only fields without underscores remain unchanged Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Previously the script only compared versions.json against SKILL.md, leaving index.json out-of-sync mismatches undetected by CI. Now both files are validated in the same loop. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
json.dump without ensure_ascii=False escaped — to \u2014. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@skills/check-versions.sh`:
- Around line 12-48: The loop currently iterates only over keys from
$VERSIONS_JSON, missing skills that exist only in index.json or as skill dirs;
change the iteration to build a unified set of skill names (union of keys from
$VERSIONS_JSON, the 'name' entries from $INDEX_JSON, and directory names under
"$SCRIPT_DIR") and iterate over that set. Concretely, replace the current
process-substitution that reads only versions.json with a generator that uses
python3 (or shell + python) to load keys from VERSIONS_JSON, names from
INDEX_JSON['skills'], and also list "$SCRIPT_DIR" subdirectories, deduplicate
them (e.g., with a set) and print one-per-line; keep the rest of the loop logic
(variables like skill_name, versions_version, index_version, SKILL.md checks)
unchanged.
In `@skills/index.json`:
- Around line 42-54: The wren-project skill entry currently lacks the extracted
connection dependency; update the JSON object for the skill whose "name" is
"wren-project" to include "wren-connection-info" in its dependencies array so
the installer (which reads index.json.dependencies) will auto-expand the
connection/security instructions when wren-project is selected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e5438259-3881-4483-a986-933934e08f32
📒 Files selected for processing (4)
skills/AUTHORING.mdskills/check-versions.shskills/index.jsonskills/wren-connection-info/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (1)
- skills/wren-connection-info/SKILL.md
- util.py: add optional CONNECTION_FILE_ROOT env var to restrict connectionFilePath to an allowed directory, preventing path traversal - check-versions.sh: iterate over union of versions.json keys, index.json names, and SKILL.md dirs so skills missing from any single file are still caught - index.json: add wren-connection-info as wren-project dependency so install.sh auto-pulls it when wren-project is selected Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
ibis-server/app/util.py (2)
64-70:⚠️ Potential issue | 🟠 MajorFail closed when
CONNECTION_FILE_ROOTis unset.This still allows arbitrary local JSON reads whenever the env var is missing, so the traversal guard becomes opt-in rather than enforced. Reject
connectionFilePathuntil a root is configured, or default to a safe fixed root.Suggested fix
def resolve_connection_info(dto) -> dict: """Return connection info dict from either a file path or the inline DTO field.""" import os if getattr(dto, "connection_file_path", None): path = pathlib.Path(dto.connection_file_path).resolve() allowed_root = os.environ.get("CONNECTION_FILE_ROOT") - if allowed_root: - if not path.is_relative_to(pathlib.Path(allowed_root).resolve()): - raise WrenError( - ErrorCode.INVALID_CONNECTION_INFO, - f"Connection file path is outside the allowed directory: {dto.connection_file_path}", - ) + if not allowed_root: + raise WrenError( + ErrorCode.INVALID_CONNECTION_INFO, + "connectionFilePath is disabled until CONNECTION_FILE_ROOT is configured", + ) + allowed_root_path = pathlib.Path(allowed_root).resolve() + if not path.is_relative_to(allowed_root_path): + raise WrenError( + ErrorCode.INVALID_CONNECTION_INFO, + f"Connection file path is outside the allowed directory: {dto.connection_file_path}", + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ibis-server/app/util.py` around lines 64 - 70, The current guard is opt-in because allowed_root = os.environ.get("CONNECTION_FILE_ROOT") permits any path when unset; change the behavior in the connection-file validation so that if CONNECTION_FILE_ROOT is not set you reject the request instead of allowing it (or alternatively set a safe default root), i.e. when allowed_root is falsy raise WrenError(ErrorCode.INVALID_CONNECTION_INFO, ...) for dto.connection_file_path; when allowed_root is present continue to validate with pathlib.Path(allowed_root).resolve() and path.is_relative_to(...) as currently written to prevent directory traversal.
71-83:⚠️ Potential issue | 🟠 MajorMap the remaining file-read failures to
INVALID_CONNECTION_INFO.Bad inputs like a directory path or an unreadable file still bubble out as uncaught
OSErrorsubclasses and turn into 500s. Keep these on the same structured error path as the other invalid file cases.Suggested fix
try: - with open(path) as f: + with open(path, encoding="utf-8") as f: return json.load(f) except FileNotFoundError: raise WrenError( ErrorCode.INVALID_CONNECTION_INFO, f"Connection file not found: {dto.connection_file_path}", ) except json.JSONDecodeError as e: raise WrenError( ErrorCode.INVALID_CONNECTION_INFO, f"Invalid JSON in connection file: {e}", ) + except OSError as e: + raise WrenError( + ErrorCode.INVALID_CONNECTION_INFO, + f"Unable to read connection file: {dto.connection_file_path}", + ) from e🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ibis-server/app/util.py` around lines 71 - 83, The code currently only catches FileNotFoundError and json.JSONDecodeError when reading connection files; update the try/except around open(path) and json.load(f) to also catch any other file-read OSError (e.g., PermissionError, IsADirectoryError) and re-raise a WrenError with ErrorCode.INVALID_CONNECTION_INFO using the same message pattern (include dto.connection_file_path or the caught exception message), so all file-access failures map to INVALID_CONNECTION_INFO instead of bubbling as 500s; locate the block that opens path and raises WrenError and add a broad except OSError as e: that raises WrenError(ErrorCode.INVALID_CONNECTION_INFO, f"Connection file not found or unreadable: {dto.connection_file_path}: {e}") (or similar) to standardize error handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@ibis-server/app/util.py`:
- Around line 64-70: The current guard is opt-in because allowed_root =
os.environ.get("CONNECTION_FILE_ROOT") permits any path when unset; change the
behavior in the connection-file validation so that if CONNECTION_FILE_ROOT is
not set you reject the request instead of allowing it (or alternatively set a
safe default root), i.e. when allowed_root is falsy raise
WrenError(ErrorCode.INVALID_CONNECTION_INFO, ...) for dto.connection_file_path;
when allowed_root is present continue to validate with
pathlib.Path(allowed_root).resolve() and path.is_relative_to(...) as currently
written to prevent directory traversal.
- Around line 71-83: The code currently only catches FileNotFoundError and
json.JSONDecodeError when reading connection files; update the try/except around
open(path) and json.load(f) to also catch any other file-read OSError (e.g.,
PermissionError, IsADirectoryError) and re-raise a WrenError with
ErrorCode.INVALID_CONNECTION_INFO using the same message pattern (include
dto.connection_file_path or the caught exception message), so all file-access
failures map to INVALID_CONNECTION_INFO instead of bubbling as 500s; locate the
block that opens path and raises WrenError and add a broad except OSError as e:
that raises WrenError(ErrorCode.INVALID_CONNECTION_INFO, f"Connection file not
found or unreadable: {dto.connection_file_path}: {e}") (or similar) to
standardize error handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a63ee882-1f63-457b-8bb2-7cdbebbc0074
📒 Files selected for processing (3)
ibis-server/app/util.pyskills/check-versions.shskills/index.json
🚧 Files skipped from review as they are similar to previous changes (1)
- skills/check-versions.sh
…ILE_ROOT unset Without the env var set, connectionFilePath is rejected entirely instead of allowing arbitrary file reads. When set, the resolved path must still be within that root. Update the three connectionFilePath tests to inject CONNECTION_FILE_ROOT via monkeypatch. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- compose.yaml: add CONNECTION_FILE_ROOT=/workspace so connectionFilePath works out of the box with the mounted workspace volume - wren-mcp-setup/SKILL.md: add -e CONNECTION_FILE_ROOT=/workspace to both docker run examples - wren-connection-info/SKILL.md: document the server requirement and clarify that the path passed as connectionFilePath must be the container-internal path when ibis-server runs in Docker Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
ibis-server/app/util.py (1)
80-92: Consider catchingPermissionErrorfor robustness.If the process lacks read permissions on the connection file, a
PermissionErrorwill propagate as HTTP 500 instead of a structuredWrenError. This is an edge case but worth handling for better error messages.♻️ Proposed enhancement
try: with open(path) as f: return json.load(f) except FileNotFoundError: raise WrenError( ErrorCode.INVALID_CONNECTION_INFO, f"Connection file not found: {dto.connection_file_path}", ) except json.JSONDecodeError as e: raise WrenError( ErrorCode.INVALID_CONNECTION_INFO, f"Invalid JSON in connection file: {e}", ) + except PermissionError: + raise WrenError( + ErrorCode.INVALID_CONNECTION_INFO, + f"Permission denied reading connection file: {dto.connection_file_path}", + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ibis-server/app/util.py` around lines 80 - 92, The file-read block that opens and parses the connection file currently catches FileNotFoundError and json.JSONDecodeError but not PermissionError, so add a PermissionError except branch in the same try/except around the open(json.load) call (the code handling in util.py that references dto.connection_file_path) and raise a WrenError with ErrorCode.INVALID_CONNECTION_INFO and a clear message like "Permission denied reading connection file: {dto.connection_file_path}" including the original exception details.skills/wren-connection-info/SKILL.md (1)
73-79: Add language identifiers to fenced code blocks.The credential schema examples lack language specifiers. Adding
yamlwould improve syntax highlighting and satisfy markdown linting.📝 Example fix for one block (apply pattern to others)
-``` +```yaml host: <hostname or IP> port: <port> user: <username> password: <SECRET> database: <database name></details> Also applies to: 85-91, 95-99, 111-117, 121-123, 127-132, 136-140 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@skills/wren-connection-info/SKILL.mdaround lines 73 - 79, Update the fenced
credential examples in SKILL.md to include a language identifier for syntax
highlighting: replace each triple-backtick fence that wraps the
host/port/user/password/database examples withyaml (e.g., changeto"user:", "password:", "database:") so markdown linters and syntax highlighting pick up the YAML formatting.skills/wren-mcp-setup/SKILL.md (2)
76-76: Consider documenting CONNECTION_FILE_ROOT's purpose.While the environment variable is correctly set, users might benefit from a brief explanation of what
CONNECTION_FILE_ROOTdoes. This is a security feature that restrictsconnectionFilePathto read files only from within the specified directory, preventing arbitrary file system access.You could add a note in Step 2 explaining:
- `CONNECTION_FILE_ROOT=/workspace` — Restricts file-based connection info to the workspace directory for security🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/wren-mcp-setup/SKILL.md` at line 76, Add a short explanatory note in Step 2 documenting the environment variable CONNECTION_FILE_ROOT and its purpose: explain that CONNECTION_FILE_ROOT defines the root directory (e.g., /workspace) used to restrict file-based connection lookups (connectionFilePath) to that directory to prevent arbitrary filesystem access and improve security; place this one-line bullet near the existing env var list so users see its security implication.
245-258: Add missing environment variables for consistency.The troubleshooting docker run command omits
CONNECTION_FILE_ROOT,MDL_PATH, andCONNECTION_INFO_FILEthat are present in the main examples. Users copying this command will lose file-based connection support and auto-loading capabilities.📝 Proposed fix to add missing environment variables
docker run -d \ --name wren-mcp \ -p 18000:8000 \ -p 19000:9000 \ -e ENABLE_MCP_SERVER=true \ -e MCP_TRANSPORT=streamable-http \ -e MCP_HOST=0.0.0.0 \ -e MCP_PORT=9000 \ -e WREN_URL=localhost:8000 \ + -e CONNECTION_FILE_ROOT=/workspace \ + -e MDL_PATH=/workspace/target/mdl.json \ + -e CONNECTION_INFO_FILE=/workspace/target/connection.json \ -v <WORKSPACE_PATH>:/workspace \ ghcr.io/canner/wren-engine-ibis:latest🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/wren-mcp-setup/SKILL.md` around lines 245 - 258, The Docker troubleshooting snippet for starting the Wren MCP container is missing the environment variables CONNECTION_FILE_ROOT, MDL_PATH, and CONNECTION_INFO_FILE, so update the docker run command (the block that creates the wren-mcp container) to include -e CONNECTION_FILE_ROOT=<...>, -e MDL_PATH=<...>, and -e CONNECTION_INFO_FILE=<...> (matching the names used elsewhere) so file-based connections and auto-loading behave consistently with the main examples; ensure values or placeholders mirror the main examples' usage and keep the other MCP_* and WREN_URL env vars unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ibis-server/app/util.py`:
- Around line 80-92: The file-read block that opens and parses the connection
file currently catches FileNotFoundError and json.JSONDecodeError but not
PermissionError, so add a PermissionError except branch in the same try/except
around the open(json.load) call (the code handling in util.py that references
dto.connection_file_path) and raise a WrenError with
ErrorCode.INVALID_CONNECTION_INFO and a clear message like "Permission denied
reading connection file: {dto.connection_file_path}" including the original
exception details.
In `@skills/wren-connection-info/SKILL.md`:
- Around line 73-79: Update the fenced credential examples in SKILL.md to
include a language identifier for syntax highlighting: replace each
triple-backtick fence that wraps the host/port/user/password/database examples
with ```yaml (e.g., change ``` to ```yaml) for every credential block (the
blocks containing "host:", "port:", "user:", "password:", "database:") so
markdown linters and syntax highlighting pick up the YAML formatting.
In `@skills/wren-mcp-setup/SKILL.md`:
- Line 76: Add a short explanatory note in Step 2 documenting the environment
variable CONNECTION_FILE_ROOT and its purpose: explain that CONNECTION_FILE_ROOT
defines the root directory (e.g., /workspace) used to restrict file-based
connection lookups (connectionFilePath) to that directory to prevent arbitrary
filesystem access and improve security; place this one-line bullet near the
existing env var list so users see its security implication.
- Around line 245-258: The Docker troubleshooting snippet for starting the Wren
MCP container is missing the environment variables CONNECTION_FILE_ROOT,
MDL_PATH, and CONNECTION_INFO_FILE, so update the docker run command (the block
that creates the wren-mcp container) to include -e CONNECTION_FILE_ROOT=<...>,
-e MDL_PATH=<...>, and -e CONNECTION_INFO_FILE=<...> (matching the names used
elsewhere) so file-based connections and auto-loading behave consistently with
the main examples; ensure values or placeholders mirror the main examples' usage
and keep the other MCP_* and WREN_URL env vars unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cf38102a-238c-4620-8d2b-02a2bf713f57
📒 Files selected for processing (7)
ibis-server/app/util.pyibis-server/tests/routers/v2/connector/test_postgres.pyibis-server/tests/routers/v3/connector/postgres/test_fallback_v2.pyibis-server/tests/routers/v3/connector/postgres/test_query.pymcp-server/docker/compose.yamlskills/wren-connection-info/SKILL.mdskills/wren-mcp-setup/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (1)
- ibis-server/tests/routers/v3/connector/postgres/test_fallback_v2.py
This was referenced Mar 9, 2026
5 tasks
This was referenced Mar 20, 2026
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
2 participants
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
connectionFilePathparameter to query/validate/metadata DTOs so credentials are read directly from disk, never passed through the LLM contextwren-connection-infoas a dedicated skill consolidating connection setup (secrets policy, per-connector fields, BigQuery base64 instructions, Mode A/B workflow) previously duplicated acrosswren-projectandgenerate-mdlwren-usageas the primary hub entry point for day-to-day usage with automatic dependency installationconnection_modefield towren_project.ymlfor access control —securitymode (default) prevents LLM from readingconnection.yml/target/connection.jsonwithout explicit user confirmationskills/check-versions.shand.github/workflows/skills-check.ymlto verifyversions.json↔SKILL.mdversion parity on PRsTest plan
connectionFilePathworks end-to-end: place credentials in a JSON file, pass the path in the request, confirm query executes without inlineconnectionInfoWrenError(INVALID_CONNECTION_INFO)is raised when neitherconnectionInfonorconnectionFilePathis providedjust test postgresto validate ibis-server postgres connector tests passskills/check-versions.shto verify skill version paritywren-usageskill and confirm it auto-installs dependencies🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores