feat(mcp-server): add Web UI for connection info management with read-only mode#1447
Merged
douenergy merged 8 commits intoCanner:mainfrom Mar 16, 2026
Merged
Conversation
Adds a Starlette/Jinja2 web UI (port 9001) for configuring data source connection credentials and deploying MDL without touching config files. - Per-datasource form fields for 14 connectors (Postgres, MySQL, DuckDB, BigQuery, Snowflake, Clickhouse, Trino, MSSQL, Oracle, Redshift, Athena, Doris, local/S3/MinIO/GCS file) - Monaco JSON editor for live MDL deploy - Connection test against ibis-server on save - Docker detection via /.dockerenv: shows yellow banner and inline hint on host fields to use host.docker.internal instead of localhost - connection_info_path defaults to ~/.wren/connection_info.json; file is created/updated on save - WEB_UI_ENABLED=false env var to disable the UI Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds a persistent read-only mode flag that can only be toggled via the Web UI. When enabled, deploy, deploy_manifest, list_remote_tables, and list_remote_constraints tools return an error instead of executing. State is persisted to ~/.wren/mcp-ui-settings.json and restored on startup. UI shows an iOS-style toggle switch at the top of the page. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- wren-mcp-setup v1.3: expose port 9001, remove CONNECTION_INFO_FILE from workspace layout, add Web UI steps for connection config and read-only mode - generate-mdl v1.3: replace direct ibis-server REST calls with MCP tools (list_remote_tables, list_remote_constraints, deploy_manifest, dry_run), remove wren-connection-info dependency - wren-project v1.5: fix version string, fix Web UI port (8077 → 9001), remove connection.yml / target/connection.json from project workflow - wren-connection-info: clarify file path approach is for direct API calls only - index.json: bump versions, remove stale wren-connection-info dependencies Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace the agent workflow (credential gathering, file generation) with a pure reference guide explaining required fields per connector, sensitive field handling, Docker host hints, and BigQuery base64 encoding. Connection info is now configured by the user via the Web UI, not generated by the agent. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Remove connection.yml / target/connection.json references throughout. Add port 9001 (Web UI) to docker run commands and port tables. Update generate-mdl flow to use MCP tools instead of direct API calls. Update wren_project.md to remove connection_mode and connection.yml sections. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
github-actions
Bot
added
documentation
Contributor
📝 WalkthroughWalkthroughAdds a Starlette-based MCP Web UI (port 9001) for connection and MDL management, integrates the UI with MCP runtime (read-only mode, state callbacks, conditional UI startup), updates templates/utilities, adds Jinja2 multipart deps, and converts docs/skills to a Web UI–driven connection/MDL workflow. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (Browser)
participant WebUI as Web UI Server (localhost:9001)
participant MCP as MCP Server (wren)
participant Ibis as ibis-server
participant Engine as Wren Engine (MDL)
User->>WebUI: GET /
WebUI->>MCP: request state (_web_get_state)
MCP-->>WebUI: return datasource, connection, MDL status
WebUI-->>User: render dashboard
User->>WebUI: POST /connection (save)
WebUI->>MCP: _web_set_connection(ds, conn_info)
MCP-->>WebUI: persist state
WebUI->>Ibis: test connection
Ibis-->>WebUI: test result
WebUI-->>User: display success/error
User->>WebUI: POST /mdl (save & deploy)
WebUI->>MCP: _web_deploy_from_dict(mdl_dict)
MCP->>Engine: deploy_manifest / dry_run
Engine-->>MCP: deployment status / counts
MCP-->>WebUI: status update
WebUI-->>User: show deployment status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
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)
📝 Coding Plan
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 |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![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: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
skills/index.json (1)
22-37:⚠️ Potential issue | 🟠 MajorRestore the
wren-connection-infodependency forgenerate-mdl.
skills/install.sh:59-114only auto-expands prerequisites fromindex.json.dependencies. With that field gone here, installinggenerate-mdldirectly no longer brings in the connection-info reference, even though this skill still starts from a live datasource connection. That also breaks the transitive install path forwren-quickstartandwren-usage, which depend ongenerate-mdl.skills/AUTHORING.md:73-81also documentsdependenciesas the supported way to model this prerequisite.Suggested fix
{ "name": "generate-mdl", "version": "1.3", "description": "Generate a Wren MDL manifest from a live database using MCP server introspection tools.", "tags": [ "wren", "mdl", "database", "introspection", "postgres", "bigquery", "snowflake", "mysql", "clickhouse", "trino" ], + "dependencies": [ + "wren-connection-info" + ], "repository": "https://github.com/Canner/wren-engine/tree/main/skills/generate-mdl" },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/index.json` around lines 22 - 37, The generate-mdl skill entry is missing its dependencies declaration, so restore an index.json.dependencies array on the "generate-mdl" object that includes the "wren-connection-info" dependency (use the correct package name and appropriate version/string used by other skills), so skills/install.sh auto-expands prerequisites and transitive installs (also update any README/AUTHORING references if needed); locate the "generate-mdl" object in skills/index.json and add the dependencies key mirroring the format used by other skills.skills/wren-connection-info/SKILL.md (1)
18-33:⚠️ Potential issue | 🟠 MajorAlign the documented connector list with the actual Web UI.
This guide now says to use the Web UI, but the connector set here has drifted from
mcp-server/app/web.py:25-120: it documentsDATABRICKSwhile omittingREDSHIFTandDORIS. That will send users looking for options the UI does not expose, and hide ones it does.Also applies to: 115-121
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/wren-connection-info/SKILL.md` around lines 18 - 33, The documented data source table in skills/wren-connection-info/SKILL.md is out of sync with the actual connectors defined in mcp-server/app/web.py (lines ~25-120); update the table to match the Web UI by removing the DATABRICKS entry and adding REDSHIFT and DORIS (and any other connectors present in web.py), ensuring the Value column matches the connector keys used in web.py and the Database column matches their human-readable names.skills/wren-quickstart/SKILL.md (1)
93-95:⚠️ Potential issue | 🟠 MajorThis phase ordering breaks on a fresh workspace.
These lines tell users to start the container before generating
target/mdl.json, but Phase 3 later says the setup always exportsMDL_PATH=/workspace/target/mdl.json. Inmcp-server/app/wren.py:148-157, startup opens that file unconditionally whenMDL_PATHis set, so a first-time setup withouttarget/mdl.jsonwill fail before users ever reach ibis-server. Please either require Phase 2 before Phase 3, or document a startup path that does not setMDL_PATHuntil the compiled MDL exists.mcp-server/app/wren.py (1)
356-369:⚠️ Potential issue | 🟡 MinorAnnotation
readOnlyHint=Truecontradicts read-only mode blocking.Both
list_remote_constraintsandlist_remote_tablesare annotated withreadOnlyHint=True, indicating they don't modify data. However, they are blocked whenread_only_modeis active (lines 366-367, 382-383).This creates a confusing user experience—tools marked as "read-only safe" are blocked in read-only mode. If the intent is to prevent schema introspection for security reasons, consider:
- Changing the annotation to
readOnlyHint=False, or- Documenting why read-only tools are blocked (e.g., "read-only mode also restricts schema discovery")
📝 Option A: Update annotations to reflect actual behavior
`@mcp.tool`( annotations=ToolAnnotations( title="List Remote Constraints", - readOnlyHint=True, + readOnlyHint=False, ), ) async def list_remote_constraints() -> str:`@mcp.tool`( annotations=ToolAnnotations( title="List Remote Tables", - readOnlyHint=True, + readOnlyHint=False, ), ) async def list_remote_tables() -> str:Also applies to: 372-385
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp-server/app/wren.py` around lines 356 - 369, The ToolAnnotations for list_remote_constraints (and similarly list_remote_tables) incorrectly declare readOnlyHint=True while the functions early-return when read_only_mode is set; update the annotations to reflect actual behavior by changing readOnlyHint=True to readOnlyHint=False in the `@mcp.tool` ToolAnnotations for both functions (or alternatively remove the read_only_mode guard if the intent is truly read-only), and adjust the function docstrings or comments to document that read-only mode blocks schema discovery.docs/getting_started_with_claude_code.md (1)
159-191:⚠️ Potential issue | 🟠 MajorMove MCP registration ahead of manual MDL generation.
Lines 171-173 say
/generate-mdluses MCP tools, but theclaude mcp add ...step does not appear until Phase 4. In a fresh Claude Code setup, those tools will not exist yet when the reader reaches Phase 3, so the manual flow can fail before registration ever happens.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/getting_started_with_claude_code.md` around lines 159 - 191, Documentation places the MCP registration step after the /generate-mdl step, which breaks fresh setups because /generate-mdl requires the MCP; move or duplicate the claude mcp add instruction so registration happens before running /generate-mdl. Update the guide so Phase 3 (where /generate-mdl is run) includes the command `claude mcp add --transport http wren http://localhost:9000/mcp` (or a note to perform the Phase 4 MCP registration first), and ensure the text mentions that MCP must be registered prior to using /generate-mdl and related MCP-backed commands like list_remote_tables() and list_remote_constraints().docs/quickstart.md (1)
145-175:⚠️ Potential issue | 🟠 MajorRegister MCP before the first
/generate-mdlstep.This manual flow asks readers to run
/generate-mdlbefore they add the MCP server in Lines 173-180. On a clean setup, the MCP-backed tools needed for MDL generation are not available in Claude Code yet, so the manual path can fail on first use. Please move theclaude mcp add ...step earlier, or make MCP registration an explicit prerequisite for/generate-mdl.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/quickstart.md` around lines 145 - 175, The docs currently instruct running /generate-mdl before registering the MCP server; change the manual flow so the MCP registration command (claude mcp add --transport http wren http://localhost:9000/mcp) appears before any /generate-mdl steps or add an explicit prerequisite note that the MCP server must be registered/available prior to running /generate-mdl (update the Phase 2 sequence around the /generate-mdl command and include the claude mcp add command or prerequisite statement so the MCP-backed tools are available).
🧹 Nitpick comments (5)
mcp-server/README.md (1)
11-36: Consider mentioning WEB_UI_ENABLED to explain how to disable the Web UI.The documentation explains the Web UI features clearly, but doesn't mention that users can disable the Web UI entirely by setting
WEB_UI_ENABLED=false. Adding a brief note in the introduction would improve discoverability for users looking to disable the feature, rather than just finding it in the environment variables table.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp-server/README.md` around lines 11 - 36, Add a short sentence to the Web UI section intro explaining that the Web UI can be disabled by setting the environment variable WEB_UI_ENABLED=false; update the paragraph that currently mentions WEB_UI_PORT to also reference WEB_UI_ENABLED and where to find persisted settings (e.g., ~/.wren/mcp-ui-settings.json) so readers can quickly discover how to turn the UI off without scanning the env-vars table.skills/wren-mcp-setup/SKILL.md (1)
91-95: Add language specifiers to fenced code blocks.The code blocks at lines 91-95 and 329-333 are missing language specifiers, which affects syntax highlighting and linter compliance.
📝 Proposed fix
Line 91:
-``` +```text <WORKSPACE_PATH>/ └── target/ └── mdl.json # Compiled MDL (from wren-project build)Line 331:
-``` +```text Loaded MDL /workspace/target/mdl.json (9 models, 47 columns)Also applies to: 329-333
🤖 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 91 - 95, Add a language specifier (e.g., "text") to the two fenced code blocks in SKILL.md so they become ```text blocks; specifically update the block showing the workspace tree ("<WORKSPACE_PATH>/ ... mdl.json") and the block showing the loader output ("Loaded MDL /workspace/target/mdl.json (9 models, 47 columns)") to include the language specifier to enable proper syntax highlighting and satisfy the linter.mcp-server/app/web.py (1)
256-261: Move import to module level.The
Responseimport insideget_mdl()is unconventional and incurs a small overhead on each call. Consider moving it to the top-level imports with other Starlette components.♻️ Proposed fix
At the top of the file (around line 12):
from starlette.responses import HTMLResponse +from starlette.responses import ResponseThen in the function:
async def get_mdl(request: Request): - from starlette.responses import Response - state = _get_state()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp-server/app/web.py` around lines 256 - 261, The local import of Response inside get_mdl is unnecessary; move "from starlette.responses import Response" to the module-level imports with the other Starlette imports and remove the inline import from the get_mdl function so get_mdl simply uses Response directly (refer to the get_mdl function and the Response symbol when making the change).mcp-server/app/templates/index.html (1)
7-8: Consider adding Subresource Integrity (SRI) hashes for CDN resources.External scripts loaded without integrity hashes could be compromised if the CDN is breached. While this is a low-risk internal tool, SRI provides defense-in-depth.
🔒 Example with SRI hashes
- <link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/@picocss/pico@2/css/pico.min.css"> - <script src="https://unpkg.com/htmx.org@1.9.12"></script> + <link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/@picocss/pico@2/css/pico.min.css" integrity="sha384-..." crossorigin="anonymous"> + <script src="https://unpkg.com/htmx.org@1.9.12" integrity="sha384-..." crossorigin="anonymous"></script>Generate hashes using https://www.srihash.org/ or similar tools.
Also applies to: 190-190
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp-server/app/templates/index.html` around lines 7 - 8, The HTML includes external resources (<link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/@picocss/pico@2/css/pico.min.css"> and <script src="https://unpkg.com/htmx.org@1.9.12"></script>) without Subresource Integrity (SRI) or crossorigin attributes; update the <link> and <script> tags that load PicoCSS and htmx (the href/src values above) to include appropriate integrity="sha384-..." attributes (generated from the exact file contents) and add crossorigin="anonymous" so the browser can verify the resources, ensuring you generate the correct SRI hashes (e.g., via https://www.srihash.org/) for those exact versions before committing.mcp-server/app/wren.py (1)
786-798:_web_deploy_from_dictalways returns success—consider failure scenarios.The function always returns
(True, ...)regardless of whethermdl_cache.set_mdl()succeeds or whether the MDL dict is valid. Whileset_mdlmay not fail, consider adding basic validation (e.g., check for requireddataSourcefield) to provide meaningful feedback.♻️ Proposed enhancement with basic validation
def _web_deploy_from_dict(mdl_dict: dict) -> tuple[bool, str]: global data_source + if not isinstance(mdl_dict, dict): + return False, "MDL must be a JSON object" + if not mdl_dict.get("dataSource"): + return False, "MDL must have a 'dataSource' field" data_source = mdl_dict.get("dataSource", "").lower() mdl_cache.set_mdl(dict_to_base64_string(mdl_dict)) models = mdl_dict.get("models", []) total_columns = sum(len(m.get("columns", [])) for m in models)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp-server/app/wren.py` around lines 786 - 798, _web_deploy_from_dict currently always returns success; add basic validation and error handling: validate required keys (e.g., "dataSource" present/non-empty and "models" is a list) and return (False, error_message) on invalid input, wrap mdl_cache.set_mdl in try/except and return failure with the exception message if it raises, and if writing to mdl_path fails (inside the existing try/except) surface a warning or failure depending on importance; reference the function _web_deploy_from_dict, the global data_source assignment, mdl_cache.set_mdl, dict_to_base64_string, mdl_path, and the models/columns aggregation to locate where to add the checks and exception handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/getting_started_with_claude_code.md`:
- Around line 177-181: The fenced code block containing the single line
`/wren-project` is unlabeled and triggers markdownlint MD040; update the fence
to include a language tag by changing the opening fence from ``` to ```text so
the block becomes ```text then `/wren-project` and the closing ``` remains the
same; target the fenced block that contains `/wren-project` in the
docs/getting_started_with_claude_code.md content.
In `@docs/quickstart.md`:
- Around line 157-166: The two new fenced code blocks containing the slash
commands "/generate-mdl" and "/wren-project" are unlabeled and trigger
markdownlint MD040; update those fences to include a language tag (e.g., use
"text" for slash-command examples or "bash" if you prefer shell highlighting) so
the blocks become "```text" (or "```bash") before the command and "```" after,
ensuring Markdown linting passes.
In `@docs/wren_project.md`:
- Around line 140-184: The three newly added fenced code blocks (the deploy
example "deploy(mdl_file_path=\"/workspace/target/mdl.json\")", the inline
workflow snippet showing "target/" and the "target/" .gitignore example) need
explicit language labels to satisfy markdownlint MD040; update the first and the
MDL/workflow examples to use a text fence (```text) and label the gitignore
snippet as ```gitignore so each fenced block has a language identifier.
In `@mcp-server/app/templates/index.html`:
- Around line 178-188: The handler for the 'htmx:responseError' listener
currently builds an HTML string (msg) containing unescaped
evt.detail.xhr.responseText and assigns it to el.innerHTML, which allows XSS;
change this to avoid innerHTML by creating/setting safe DOM nodes and
textContent instead: obtain el (as currently), create or reuse a wrapper element
(e.g., a small/span) set its textContent to a descriptive message plus the
truncated response body (use the existing body variable but do not interpolate
it into HTML), and apply the red color via style or a pre-defined CSS class on
that element; ensure anywhere msg or innerHTML is used in this
'htmx:responseError' listener is replaced with textContent/textNode-based
updates.
In `@mcp-server/app/web.py`:
- Around line 226-231: The code writes connection info to disk using conn_path
(from state.get) but doesn't handle failures from os.makedirs, open, or
json.dump, so persistence can silently fail; wrap the directory creation and
file write for the conn_path block in a try/except that catches
OSError/IOError/Exception, and on failure ensure you log the error (including
the exception message) and either return a failure response or raise a
controlled exception so callers know persistence failed; specifically modify the
block that references conn_path, ds and conn_info to perform the write inside a
try: with open(...) and json.dump(...) and handle exceptions in the except: to
call your module logger (or raise a descriptive RuntimeError) so write errors
are surfaced.
In `@skills/generate-mdl/SKILL.md`:
- Around line 204-206: Update the "Connection setup" section in SKILL.md to
document the fallback when the Web UI is disabled: mention the
WEB_UI_ENABLED=false configuration and describe the non-UI alternative (e.g.,
using the CLI or the reference workflow) or add a link to the wren-mcp-setup
reference workflow that shows how to configure connection info without the Web
UI; ensure you reference the WEB_UI_ENABLED flag and the wren-mcp-setup skill so
readers can find the non-UI steps when the Web UI at http://localhost:9001 is
unavailable.
- Around line 59-69: Update the schema discovery docs to explicitly handle MCP
server read-only mode: when using list_remote_tables() (filtering by
properties.schema) and list_remote_constraints() to build Relationship entries
for the MDL, add a troubleshooting note that these calls will fail if the server
is in read-only mode and instruct the user to disable read-only mode in the Web
UI (http://localhost:9001) before retrying; keep the existing advice about
checking connection info but add this additional check so agents don’t
misdiagnose read-only failures as connectivity issues.
- Around line 39-41: Several fenced code blocks (e.g., the ones showing
health_check(), list_remote_tables(), list_remote_constraints(),
deploy_manifest(mdl=<manifest dict>) and dry_run(sql="SELECT * FROM
<any_model_name> LIMIT 1")) are missing a language tag and trigger markdownlint
MD040; update each triple-backtick fence around those snippets to include a
language (suggested: "text") so they read ```text ... ``` to keep the docs
lint-clean while preserving content.
In `@skills/wren-connection-info/SKILL.md`:
- Around line 80-113: The connector field tables in SKILL.md must be reconciled
with the backend DATASOURCE_FIELDS definitions in mcp-server/app/web.py; update
the Snowflake table to list fields user, password, account, database, schema
(named "schema" not "sf_schema"), and warehouse; change DuckDB docs to use a
single "path" field (not format/url); and adjust Athena to use "region_name" and
the exact AWS key names (aws_access_key_id, aws_secret_access_key) to match
DATASOURCE_FIELDS so the docs reflect the UI/backend contract.
- Around line 62-76: The SKILL.md BigQuery fields use incorrect names; update
the table and explanatory text to match the web UI schema by replacing
`project_id` → `project`, `dataset_id` → `dataset`, and
`credentials_json_string` → `credentials_base64`, and ensure the description
explicitly states the field expects the base64-encoded service account JSON
(keep the base64 command examples but refer to the `credentials_base64` field).
Also verify any mention of "credentials_json_string" elsewhere in SKILL.md is
renamed to `credentials_base64` so the produced properties shape matches the
schema that expects `project`, `dataset`, and `credentials_base64`.
- Around line 143-150: Update the sensitive-fields table in SKILL.md so it
matches the UI form schema: remove keys the server UI doesn't support (drop
`private_key`, `aws_session_token`, and `client_secret`), rename the BigQuery
key to the form’s current field name (replace `credentials_json_string` with the
UI-backed key), and list only the actual supported keys per connector (e.g., for
Postgres/MySQL/MSSQL/ClickHouse/Oracle keep `password`; for BigQuery use the
updated credentials field; for Snowflake keep `password`; for Athena keep
`aws_access_key_id` and `aws_secret_access_key`; for S3/Minio/GCS keep
`access_key` and `secret_key`; for Databricks keep `access_token`). Ensure the
table rows use the exact field names found in the web UI schema to keep docs in
lockstep with the form.
In `@skills/wren-mcp-setup/SKILL.md`:
- Around line 97-98: Update the note to clarify that connection info is now
primarily managed via the MCP server Web UI but the server still persists/loads
connection data from ~/.wren/connection_info.json (the same file the Web UI
writes to), so the file can be pre-populated for startup; reference the server's
connection load behavior (the code in wren.py that reads
~/.wren/connection_info.json) and the Web UI save behavior when wording the new
sentence.
In `@skills/wren-project/SKILL.md`:
- Around line 47-52: Update the fenced code block in SKILL.md that shows the
output tree (the block starting with "my_project/") to include a fence language
specifier by changing the opening ``` to ```text so the snippet is fenced as
```text ... ``` to satisfy the MD040 linter rule.
- Around line 196-198: Move the connection configuration step to precede the
deploy call and remove the hardcoded localhost URL; update the steps so the
"Connection info" instruction appears before step 6 and refer to the MCP server
address generically (e.g., "configure via the MCP server Web UI at your MCP
host:PORT" or "configure via the MCP server Web UI using your MCP_HOST and
MCP_PORT") instead of using http://localhost:9001, and ensure the deploy call
deploy(mdl_file_path="./target/mdl.json") remains after the connection has been
configured.
In `@skills/wren-quickstart/SKILL.md`:
- Around line 16-19: Update the sample upgrade notice for the wren-quickstart
skill so the installed version matches the bumped skill version: change the
displayed "installed: 1.1" to "installed: 1.2" in the notice string that reads
"A newer version of the **wren-quickstart** skill is available (remote: X.Y,
installed: 1.1)". Ensure the notice now reads "... (remote: X.Y, installed:
1.2)" so the version-check output is consistent with the SKILL.md bump.
- Around line 140-144: Update the Web UI URL in the quickstart docs: replace the
hardcoded URL "http://localhost:8077" in SKILL.md with "http://localhost:9001"
so it matches the configured WEB_UI_PORT (default 9001), the Docker container
exposure, and other docs (wren-mcp-setup, wren-connection-info, wren-project,
generate-mdl); ensure the single URL instance in the quickstart section is
updated to 9001.
---
Outside diff comments:
In `@docs/getting_started_with_claude_code.md`:
- Around line 159-191: Documentation places the MCP registration step after the
/generate-mdl step, which breaks fresh setups because /generate-mdl requires the
MCP; move or duplicate the claude mcp add instruction so registration happens
before running /generate-mdl. Update the guide so Phase 3 (where /generate-mdl
is run) includes the command `claude mcp add --transport http wren
http://localhost:9000/mcp` (or a note to perform the Phase 4 MCP registration
first), and ensure the text mentions that MCP must be registered prior to using
/generate-mdl and related MCP-backed commands like list_remote_tables() and
list_remote_constraints().
In `@docs/quickstart.md`:
- Around line 145-175: The docs currently instruct running /generate-mdl before
registering the MCP server; change the manual flow so the MCP registration
command (claude mcp add --transport http wren http://localhost:9000/mcp) appears
before any /generate-mdl steps or add an explicit prerequisite note that the MCP
server must be registered/available prior to running /generate-mdl (update the
Phase 2 sequence around the /generate-mdl command and include the claude mcp add
command or prerequisite statement so the MCP-backed tools are available).
In `@mcp-server/app/wren.py`:
- Around line 356-369: The ToolAnnotations for list_remote_constraints (and
similarly list_remote_tables) incorrectly declare readOnlyHint=True while the
functions early-return when read_only_mode is set; update the annotations to
reflect actual behavior by changing readOnlyHint=True to readOnlyHint=False in
the `@mcp.tool` ToolAnnotations for both functions (or alternatively remove the
read_only_mode guard if the intent is truly read-only), and adjust the function
docstrings or comments to document that read-only mode blocks schema discovery.
In `@skills/index.json`:
- Around line 22-37: The generate-mdl skill entry is missing its dependencies
declaration, so restore an index.json.dependencies array on the "generate-mdl"
object that includes the "wren-connection-info" dependency (use the correct
package name and appropriate version/string used by other skills), so
skills/install.sh auto-expands prerequisites and transitive installs (also
update any README/AUTHORING references if needed); locate the "generate-mdl"
object in skills/index.json and add the dependencies key mirroring the format
used by other skills.
In `@skills/wren-connection-info/SKILL.md`:
- Around line 18-33: The documented data source table in
skills/wren-connection-info/SKILL.md is out of sync with the actual connectors
defined in mcp-server/app/web.py (lines ~25-120); update the table to match the
Web UI by removing the DATABRICKS entry and adding REDSHIFT and DORIS (and any
other connectors present in web.py), ensuring the Value column matches the
connector keys used in web.py and the Database column matches their
human-readable names.
---
Nitpick comments:
In `@mcp-server/app/templates/index.html`:
- Around line 7-8: The HTML includes external resources (<link rel="stylesheet"
href="https://cdn.jsdelivr.net/npm/@picocss/pico@2/css/pico.min.css"> and
<script src="https://unpkg.com/htmx.org@1.9.12"></script>) without Subresource
Integrity (SRI) or crossorigin attributes; update the <link> and <script> tags
that load PicoCSS and htmx (the href/src values above) to include appropriate
integrity="sha384-..." attributes (generated from the exact file contents) and
add crossorigin="anonymous" so the browser can verify the resources, ensuring
you generate the correct SRI hashes (e.g., via https://www.srihash.org/) for
those exact versions before committing.
In `@mcp-server/app/web.py`:
- Around line 256-261: The local import of Response inside get_mdl is
unnecessary; move "from starlette.responses import Response" to the module-level
imports with the other Starlette imports and remove the inline import from the
get_mdl function so get_mdl simply uses Response directly (refer to the get_mdl
function and the Response symbol when making the change).
In `@mcp-server/app/wren.py`:
- Around line 786-798: _web_deploy_from_dict currently always returns success;
add basic validation and error handling: validate required keys (e.g.,
"dataSource" present/non-empty and "models" is a list) and return (False,
error_message) on invalid input, wrap mdl_cache.set_mdl in try/except and return
failure with the exception message if it raises, and if writing to mdl_path
fails (inside the existing try/except) surface a warning or failure depending on
importance; reference the function _web_deploy_from_dict, the global data_source
assignment, mdl_cache.set_mdl, dict_to_base64_string, mdl_path, and the
models/columns aggregation to locate where to add the checks and exception
handling.
In `@mcp-server/README.md`:
- Around line 11-36: Add a short sentence to the Web UI section intro explaining
that the Web UI can be disabled by setting the environment variable
WEB_UI_ENABLED=false; update the paragraph that currently mentions WEB_UI_PORT
to also reference WEB_UI_ENABLED and where to find persisted settings (e.g.,
~/.wren/mcp-ui-settings.json) so readers can quickly discover how to turn the UI
off without scanning the env-vars table.
In `@skills/wren-mcp-setup/SKILL.md`:
- Around line 91-95: Add a language specifier (e.g., "text") to the two fenced
code blocks in SKILL.md so they become ```text blocks; specifically update the
block showing the workspace tree ("<WORKSPACE_PATH>/ ... mdl.json") and the
block showing the loader output ("Loaded MDL /workspace/target/mdl.json (9
models, 47 columns)") to include the language specifier to enable proper syntax
highlighting and satisfy the linter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 62163be8-93c2-48e5-aa5b-e440c73c1e77
⛔ Files ignored due to path filters (1)
mcp-server/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
docs/getting_started_with_claude_code.mddocs/quickstart.mddocs/wren_project.mdmcp-server/README.mdmcp-server/app/templates/_fields.htmlmcp-server/app/templates/index.htmlmcp-server/app/utils.pymcp-server/app/web.pymcp-server/app/wren.pymcp-server/pyproject.tomlskills/generate-mdl/SKILL.mdskills/index.jsonskills/versions.jsonskills/wren-connection-info/SKILL.mdskills/wren-mcp-setup/SKILL.mdskills/wren-project/SKILL.mdskills/wren-quickstart/SKILL.md
Comment on lines
+196
to
+198
| 6. Deploy: deploy(mdl_file_path="./target/mdl.json") | ||
|
|
||
| 7. Deploy: deploy(mdl_file_path="./target/mdl.json") | ||
| use connectionFilePath="<absolute_path>/target/connection.json" in API requests | ||
| (ibis-server reads the file directly — secrets stay out of this conversation) | ||
| 7. Connection info: configure via the MCP server Web UI (http://localhost:9001) |
Contributor
There was a problem hiding this comment.
Move connection setup before deploy, and avoid a localhost-only instruction.
As written, the workflow tells users to deploy before configuring the prerequisite connection. It also hardcodes localhost, which conflicts with the new containerized-client guidance added in this PR.
Suggested fix
-6. Deploy: deploy(mdl_file_path="./target/mdl.json")
-
-7. Connection info: configure via the MCP server Web UI (http://localhost:9001)
+6. Connection info: configure via the MCP server Web UI
+ (typically http://localhost:9001; use the Docker host hint when running in a container)
+
+7. Deploy: deploy(mdl_file_path="./target/mdl.json")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/wren-project/SKILL.md` around lines 196 - 198, Move the connection
configuration step to precede the deploy call and remove the hardcoded localhost
URL; update the steps so the "Connection info" instruction appears before step 6
and refer to the MCP server address generically (e.g., "configure via the MCP
server Web UI at your MCP host:PORT" or "configure via the MCP server Web UI
using your MCP_HOST and MCP_PORT") instead of using http://localhost:9001, and
ensure the deploy call deploy(mdl_file_path="./target/mdl.json") remains after
the connection has been configured.
- Add language tags to all fenced code blocks (MD040 lint) - Fix wren-quickstart Web UI port: 8077 → 9001 - Fix wren-quickstart installed version in upgrade notice: 1.1 → 1.2 - Move connection info step before deploy in wren-project skill - Clarify connection info persisted to ~/.wren/connection_info.json - Add read-only mode troubleshooting to generate-mdl skill - Document Web UI disabled fallback in generate-mdl skill - Fix XSS: escape responseText before inserting into DOM - Handle OSError on connection info file write in web.py Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
skills/wren-mcp-setup/SKILL.md (2)
331-333: Add a language tag to this fenced block.The log output snippet is missing a fence language, triggering MD040.
Suggested fix
-``` +```text Loaded MDL /workspace/target/mdl.json (9 models, 47 columns)</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@skills/wren-mcp-setup/SKILL.mdaround lines 331 - 333, The fenced code block
that contains the log line "Loaded MDL /workspace/target/mdl.json (9 models, 47
columns)" is missing a language tag which triggers MD040; update that fenced
block by adding a language identifier (e.g., "text") after the opening backticks
so it reads ```text to satisfy the linter and preserve plain-text formatting for
the log snippet.</details> --- `91-95`: **Add a language tag to this fenced block.** The workspace layout snippet is missing a fence language, triggering MD040. <details> <summary>Suggested fix</summary> ```diff -``` +```text <WORKSPACE_PATH>/ └── target/ └── mdl.json # Compiled MDL (from wren-project build) ``` ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@skills/wren-mcp-setup/SKILL.mdaround lines 91 - 95, The fenced code block
showing the workspace layout (the block containing "<WORKSPACE_PATH>/", "└──
target/", "└── mdl.json # Compiled MDL (from wren-project build)") is
missing a language tag which triggers MD040; edit SKILL.md to add a fence
language (e.g., use ```text) to the opening triple-backtick so the block becomes
a language-tagged fenced code block.</details> </blockquote></details> <details> <summary>mcp-server/app/templates/index.html (1)</summary><blockquote> `222-235`: **Minor: Consider escaping error messages in MDL save handler.** The OOB swap handling at line 225 uses `innerHTML` to parse the server response. While the server controls this content, the error path at line 233 directly interpolates the caught exception into HTML. If a JSON parsing error contains angle brackets, they would be rendered as HTML. The risk is low since `json.JSONDecodeError` messages typically don't echo raw input, but for defense in depth: <details> <summary>Suggested fix for the catch block</summary> ```diff } catch (e) { - result.innerHTML = `<small style="color:var(--pico-color-red-500)">✗ ${e}</small>`; + const errSmall = document.createElement('small'); + errSmall.style.color = 'var(--pico-color-red-500)'; + errSmall.textContent = `✗ ${e}`; + result.textContent = ''; + result.appendChild(errSmall); } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@mcp-server/app/templates/index.html` around lines 222 - 235, The catch block currently injects the caught exception into HTML via result.innerHTML using the caught variable e; change this to safely render the error as text instead of raw HTML (e.g., create a small element or set result.textContent) so angle brackets in e cannot be interpreted as HTML; update the catch handling that assigns to result.innerHTML and reference the catch variable e and the result element to perform a safe text-only insertion. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In@mcp-server/app/templates/index.html:
- Around line 222-235: The catch block currently injects the caught exception
into HTML via result.innerHTML using the caught variable e; change this to
safely render the error as text instead of raw HTML (e.g., create a small
element or set result.textContent) so angle brackets in e cannot be interpreted
as HTML; update the catch handling that assigns to result.innerHTML and
reference the catch variable e and the result element to perform a safe
text-only insertion.In
@skills/wren-mcp-setup/SKILL.md:
- Around line 331-333: The fenced code block that contains the log line "Loaded
MDL /workspace/target/mdl.json (9 models, 47 columns)" is missing a language tag
which triggers MD040; update that fenced block by adding a language identifier
(e.g., "text") after the opening backticks so it reads ```text to satisfy the
linter and preserve plain-text formatting for the log snippet.- Around line 91-95: The fenced code block showing the workspace layout (the
block containing "<WORKSPACE_PATH>/", "└── target/", "└── mdl.json #
Compiled MDL (from wren-project build)") is missing a language tag which
triggers MD040; edit SKILL.md to add a fence language (e.g., use ```text) to the
opening triple-backtick so the block becomes a language-tagged fenced code
block.</details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `5dd3fc4d-f2e5-4f29-b38f-eafce73c8ff8` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 286141c391c94db88e033dd0aa9ad7575472eb78 and 7eacd9c70358975cb9847e873c72f0e84191c66a. </details> <details> <summary>📒 Files selected for processing (9)</summary> * `docs/getting_started_with_claude_code.md` * `docs/quickstart.md` * `docs/wren_project.md` * `mcp-server/app/templates/index.html` * `mcp-server/app/web.py` * `skills/generate-mdl/SKILL.md` * `skills/wren-mcp-setup/SKILL.md` * `skills/wren-project/SKILL.md` * `skills/wren-quickstart/SKILL.md` </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (1)</summary> * skills/wren-quickstart/SKILL.md </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
douenergy
approved these changes
Mar 16, 2026
2 tasks
6 tasks
nhaluc1005
pushed a commit
to nhaluc1005/text2sql-practice
that referenced
this pull request
Apr 3, 2026
…-only mode (Canner#1447) Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
5 tasks
This was referenced Apr 14, 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
Screenshot
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Documentation