feat(wren): shared field registry + browser-based profile form (wren profile add --ui)#1512
Open
feat(wren): shared field registry + browser-based profile form (wren profile add --ui)#1512
Conversation
…connection fields Fields like host, port, database, user, url, account, etc. are not secrets and don't need SecretStr protection. This simplifies connector code by removing unnecessary .get_secret_value() calls while keeping SecretStr for truly sensitive fields (password, credentials, tokens, keys). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Centralizes datasource field definitions (name, label, type, sensitivity) in a single registry used by profile_cli, docs, and mcp-server web UI, eliminating duplicated hardcoded field lists across modules. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…FIELDS Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Temporarily remove app/ from sys.path before importing wren.model.field_registry so Python finds the installed wren package instead of app/wren.py. This eliminates duplicate startup log messages without renaming the entry point. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Adds a temporary local Starlette server that renders a connection form in the browser, saves the completed profile to ~/.wren/profiles.yml, then shuts itself down — same pattern as `gh auth login`. - profile_web.py: create_app() factory + start() blocking entry point; _ProfileServer subclasses uvicorn.Server to open browser only after the server is ready - templates/profile_form.html: PicoCSS + HTMX single-page form - templates/_profile_fields.html: HTMX partial driven by get_fields(); handles hidden / file_base64 / password / text field types and renders a variant selector for bigquery / redshift / databricks - profile_cli.py: adds --ui / --port / --no-open flags to `wren profile add`; graceful ImportError message when [ui] extra is not installed - pyproject.toml: new [ui] extra (starlette, uvicorn, jinja2, python-multipart) - tests: 23 TestClient-based tests covering GET / and /fields, all POST /save paths, variant handling, activate flag, and empty-field filtering Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Quick start step 2 now shows all three profile creation methods (--ui browser form, --interactive, --from-file) before the manual JSON option - New "Connection profiles" section documents wren profile add/list/switch/debug/rm - Installation table adds wren-engine[ui] entry; [all] updated to include ui - Development section notes that test_profile_web.py requires [ui] extra Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
github-actions
bot
added
documentation
Contributor
📝 WalkthroughWalkthroughCentralizes datasource field metadata into a new field registry, converts many connection model fields from secret types to plain strings, updates connectors/docs to use raw fields, and adds a Starlette-based browser UI (with CLI flags) for creating connection profiles plus templates and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as 🌐 Browser
participant Server as 🖥️ Starlette Server
participant Registry as 📋 Field Registry
participant Storage as 💾 Profiles Storage
participant CLI as ⌨️ CLI
CLI->>Browser: open browser URL /
Browser->>Server: GET /
Server->>Registry: get_datasource_options()
Registry-->>Server: datasource list
Server-->>Browser: render form with datasource select
Browser->>Server: GET /fields?datasource=ds&_variant=opt
Server->>Registry: get_fields(ds, variant=opt)
Registry-->>Server: FieldDef list
Server-->>Browser: render fields fragment
Browser->>Server: POST /save (form/_json)
Server->>Storage: add_profile(name, profile_dict)
Storage-->>Server: saved
Server-->>Browser: success + shutdown signal
Server->>CLI: return result (profile created)
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 docstrings
🧪 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 |
test-ui: runs tests/test_profile_web.py and tests/test_field_registry.py with --extra ui (starlette, uvicorn, jinja2, python-multipart). test-memory: runs tests/unit/test_memory.py with --extra memory (lancedb, sentence-transformers). Caches ~/.cache/huggingface to avoid re-downloading the paraphrase-multilingual-MiniLM-L12-v2 model on every run. Both jobs follow the same build pattern as test-unit (Cargo cache + maturin build + uv sync). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The default paraphrase-multilingual-MiniLM-L12-v2 model is ~470 MB. CI tests only verify index counts, search keys, and store/recall round-trips — they don't require multilingual or high-quality embeddings. paraphrase-MiniLM-L3-v2 (~61 MB, dim=384) is a drop-in replacement that is 8x smaller and still produces valid 384-dim vectors. Adds WREN_EMBEDDING_MODEL env var support to embeddings.py so the model can be overridden without changing test code. Production default (paraphrase-multilingual-MiniLM-L12-v2) is unchanged. 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wren/README.md (1)
58-84:⚠️ Potential issue | 🟡 MinorSeparate the new profile flow from the legacy
connection_info.jsonpath.This section says there are three supported setup paths, but Lines 71-84 immediately reintroduce manual
~/.wren/connection_info.jsoncreation and saywrenauto-discovers “both files.” That muddies the newprofiles.ymlworkflow. Either remove that block from quick start or label it explicitly as a backward-compatible/legacy path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wren/README.md` around lines 58 - 84, The README currently mixes the new profile workflow with the legacy connection_info.json path; update the "Configure a connection profile" section to clearly separate them by either removing the manual ~/.wren/connection_info.json example or explicitly marking it as legacy/backward-compatible (mentioning "legacy connection_info.json" and that current recommended flow is profiles.yml via the `wren profile add` commands). Ensure you reference the new workflow commands (`wren profile add my-db --ui`, `--interactive`, `--from-file`) and the legacy filename (`connection_info.json` or `~/.wren/connection_info.json`) so readers understand which is recommended and which is deprecated.
🧹 Nitpick comments (4)
wren/src/wren/templates/profile_form.html (1)
7-8: Consider pinning CDN resources with integrity hashes for supply chain security.Loading external scripts without Subresource Integrity (SRI) hashes from CDNs poses a supply chain risk if the CDN is compromised. While this is a local temporary server for profile creation, consider adding
integrityattributes for 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" crossorigin="anonymous"> + <script src="https://unpkg.com/htmx.org@1.9.12" integrity="sha384-..." crossorigin="anonymous"></script>You can generate SRI hashes using
openssl dgst -sha384 -binary <file> | openssl base64 -Aor online tools.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wren/src/wren/templates/profile_form.html` around lines 7 - 8, Add Subresource Integrity attributes and crossorigin to the external CDN tags in profile_form.html: compute SRI hashes (e.g., sha384) for the pico.min.css and htmx.org@1.9.12 files and add integrity="..." and crossorigin="anonymous" to the corresponding <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> elements so the browser verifies the files; ensure the integrity values match the exact files served and update those tags in the template accordingly.mcp-server/app/web.py (1)
38-40: Sys.path removal usingreversed()is correct but could be fragile.The use of
reversed(saved_paths)ensures indices remain valid during removal. However, this approach modifiessys.pathin-place, which could cause issues if another thread imports modules concurrently. Since this is called at module initialization time, the risk is low, but worth noting.🤖 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 38 - 40, The current removal loop that builds saved_paths and then iterates with reversed(saved_paths) to remove entries from sys.path (the saved_paths variable and the for _, p in reversed(saved_paths) loop) mutates sys.path in-place which can be fragile; instead replace the in-place removals with a safe, atomic reassignment that filters out matching paths (e.g., compute a new list using os.path.realpath and app_dir and assign it back to sys.path) so you avoid concurrent-modification issues and keep behavior equivalent without iteratively calling sys.path.remove.wren/src/wren/model/field_registry.py (2)
194-204: Redundant local import ofUnion.
Unionis already imported at module level (line 6), so the local import on line 197 is unnecessary. Only thetypesimport is needed fortypes.UnionType.🧹 Suggested cleanup
def _union_args(annotation) -> tuple | None: """Return type args if annotation is Union/UnionType, else None.""" import types # noqa: PLC0415 - from typing import Union # noqa: PLC0415 if isinstance(annotation, types.UnionType): return annotation.__args__🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wren/src/wren/model/field_registry.py` around lines 194 - 204, In _union_args remove the redundant local import "from typing import Union" and rely on the module-level Union import already present; keep the local "import types" for types.UnionType and leave the rest of the function logic unchanged so behavior is preserved (function name: _union_args, local symbol: types.UnionType, module-level symbol: Union).
359-365: Silent fallback when variant is unknown could mask user errors.When a variant is provided but doesn't match any model, the code silently falls back to
models[0]. This could hide configuration errors where users specify an invalid variant name. Consider raising aValueErrorsimilar to the unknown datasource case, or at minimum logging a warning.♻️ Proposed fix to raise on invalid variant
if len(models) == 1: model_cls = models[0] elif variant is None: model_cls = models[0] else: - model_cls = next( + model_cls_match = next( (m for m in models if _get_variant_name(m) == variant), - models[0], + None, ) + if model_cls_match is None: + valid = [_get_variant_name(m) for m in models] + raise ValueError(f"Unknown variant {variant!r} for {datasource!r}. Valid: {valid}") + model_cls = model_cls_match🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wren/src/wren/model/field_registry.py` around lines 359 - 365, The code currently silently falls back to models[0] when a provided variant doesn't match any model; update the branch that sets model_cls (the elif/else that uses next((m for m in models if _get_variant_name(m) == variant), models[0])) so that if variant is not None and no match is found you raise a ValueError (or at minimum log a warning) instead of defaulting to models[0]; ensure the error message mentions the invalid variant and available _get_variant_name(m) values to aid debugging, mirroring the behavior used for unknown datasources.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wren/src/wren/connector/databricks.py`:
- Around line 32-33: The azure_tenant_id value is being set as a 1-tuple due to
a trailing comma; change the assignment that sets kwargs["azure_tenant_id"] =
(connection_info.azure_tenant_id,) to assign the plain string value from
connection_info.azure_tenant_id so DbConfig.azure_tenant_id receives a string
(remove the trailing comma where kwargs["azure_tenant_id"] is populated).
In `@wren/src/wren/model/__init__.py`:
- Around line 76-78: The type annotation for region_name is inconsistent with
its default; update region_name's annotation to allow None (e.g., change
region_name: str to region_name: str | None or Optional[str]) so it matches
Field(default=None), ensuring static type checkers and Pydantic agree; keep the
Field(...) call (examples=["us-west-2"], default=None) and leave
role_session_name and schema_name annotations unchanged.
In `@wren/src/wren/profile_cli.py`:
- Around line 92-93: The current message always suggests running `wren profile
switch {result['name']}` even when activate=True; update the logic in the
profile save/creation flow (the block referencing the local variable `activate`
and `result['name']`) so that if activate is True you print a confirmation that
the profile was activated (e.g., "Profile '{result['name']}' activated.") and
only show the "Run `wren profile switch ...`" hint when activate is False.
- Around line 188-197: Wrap the file read and base64 encoding in a try/except
around the call to Path(path_str).expanduser().read_bytes() inside the
file_base64 branch so invalid or inaccessible paths don't crash the CLI; catch
FileNotFoundError, PermissionError and a general OSError, report a clear
user-facing error via typer (e.g., typer.secho or typer.echo) and either
re-prompt or skip setting profile[f.name] when an error occurs, ensuring you
only set profile[f.name] with base64.b64encode(content).decode() on successful
read.
In `@wren/src/wren/profile_web.py`:
- Around line 34-37: The return type annotation of create_app is incorrect: it
declares tuple[Starlette, dict[str, Any]] but the function actually returns
three values (app, result, server_ref); update the create_app return annotation
to reflect the true tuple type (e.g., tuple[Starlette, dict[str, Any],
ServerRefType] or appropriate type for server_ref) and remove the # type:
ignore[return-value] on the actual return; ensure you import or define the
correct type for server_ref and update any callers if necessary.
- Line 109: The call to add_profile(name, profile, activate=activate) can raise
ValueError or filesystem errors (OSError/PermissionError) and must be wrapped in
a try/except; update the handler in profile_web.py to catch ValueError and
OSError/PermissionError around the add_profile(...) invocation, log the
exception, and return a clear user-facing HTTP error response (e.g., 400 for
ValueError with the validation message, 500 or 403 for filesystem/permission
errors) instead of allowing the exception to propagate; ensure you reference and
handle exceptions thrown by functions in wren/src/wren/profile.py and maintain
existing request/response conventions used in this module.
---
Outside diff comments:
In `@wren/README.md`:
- Around line 58-84: The README currently mixes the new profile workflow with
the legacy connection_info.json path; update the "Configure a connection
profile" section to clearly separate them by either removing the manual
~/.wren/connection_info.json example or explicitly marking it as
legacy/backward-compatible (mentioning "legacy connection_info.json" and that
current recommended flow is profiles.yml via the `wren profile add` commands).
Ensure you reference the new workflow commands (`wren profile add my-db --ui`,
`--interactive`, `--from-file`) and the legacy filename (`connection_info.json`
or `~/.wren/connection_info.json`) so readers understand which is recommended
and which is deprecated.
---
Nitpick comments:
In `@mcp-server/app/web.py`:
- Around line 38-40: The current removal loop that builds saved_paths and then
iterates with reversed(saved_paths) to remove entries from sys.path (the
saved_paths variable and the for _, p in reversed(saved_paths) loop) mutates
sys.path in-place which can be fragile; instead replace the in-place removals
with a safe, atomic reassignment that filters out matching paths (e.g., compute
a new list using os.path.realpath and app_dir and assign it back to sys.path) so
you avoid concurrent-modification issues and keep behavior equivalent without
iteratively calling sys.path.remove.
In `@wren/src/wren/model/field_registry.py`:
- Around line 194-204: In _union_args remove the redundant local import "from
typing import Union" and rely on the module-level Union import already present;
keep the local "import types" for types.UnionType and leave the rest of the
function logic unchanged so behavior is preserved (function name: _union_args,
local symbol: types.UnionType, module-level symbol: Union).
- Around line 359-365: The code currently silently falls back to models[0] when
a provided variant doesn't match any model; update the branch that sets
model_cls (the elif/else that uses next((m for m in models if
_get_variant_name(m) == variant), models[0])) so that if variant is not None and
no match is found you raise a ValueError (or at minimum log a warning) instead
of defaulting to models[0]; ensure the error message mentions the invalid
variant and available _get_variant_name(m) values to aid debugging, mirroring
the behavior used for unknown datasources.
In `@wren/src/wren/templates/profile_form.html`:
- Around line 7-8: Add Subresource Integrity attributes and crossorigin to the
external CDN tags in profile_form.html: compute SRI hashes (e.g., sha384) for
the pico.min.css and htmx.org@1.9.12 files and add integrity="..." and
crossorigin="anonymous" to the corresponding <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> elements so the
browser verifies the files; ensure the integrity values match the exact files
served and update those tags in the template accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1f225faf-bf35-4048-9d9a-356d9ee2a95e
⛔ Files ignored due to path filters (1)
wren/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
mcp-server/app/web.pywren/README.mdwren/pyproject.tomlwren/src/wren/connector/databricks.pywren/src/wren/connector/duckdb.pywren/src/wren/connector/oracle.pywren/src/wren/connector/redshift.pywren/src/wren/connector/spark.pywren/src/wren/docs.pywren/src/wren/model/__init__.pywren/src/wren/model/data_source.pywren/src/wren/model/field_registry.pywren/src/wren/profile_cli.pywren/src/wren/profile_web.pywren/src/wren/templates/_profile_fields.htmlwren/src/wren/templates/profile_form.htmlwren/tests/test_field_registry.pywren/tests/test_profile_web.py
- fix: databricks azure_tenant_id had spurious trailing comma wrapping
value in a tuple instead of passing the string directly
- fix: AthenaConnectionInfo region_name type annotation str -> str | None
to match default=None
- fix: profile_cli activation message was misleading ("run switch" hint
shown even when --activate was passed); now says "is now active"
- fix: profile_cli --from-file file read wrapped in try/except for
FileNotFoundError/PermissionError
- fix: profile_web.py return type annotation corrected to
tuple[Starlette, dict[str, Any], list]
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
starlette.testclient requires httpx at import time; CI test-ui job was failing with ModuleNotFoundError. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wren/src/wren/model/__init__.py`:
- Line 12: BaseConnectionInfo.to_key_string currently only includes SecretStr
fields when building cache keys, so demoting connection fields to str/StrPort
(e.g., StrPort) causes empty or non-unique keys; update to_key_string in
BaseConnectionInfo to include identity-critical connection fields (not just
SecretStr) by either: 1) consulting the field registry sensitivity marker in
field_registry.py (add or use a non-secret "identity" flag) and concatenating
those fields' values into the key, or 2) explicitly including known identity
fields (e.g., host, port, user, database, file_path) alongside SecretStr values;
ensure you reference StrPort and other annotated types so these string-valued
fields are included in the generated key and keep sensitivity/secret handling
separate from cache-key generation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6c07d12e-ad5d-49e6-b63d-5552168cdc7e
📒 Files selected for processing (7)
.github/workflows/wren-ci.ymlwren/pyproject.tomlwren/src/wren/connector/databricks.pywren/src/wren/memory/embeddings.pywren/src/wren/model/__init__.pywren/src/wren/profile_cli.pywren/src/wren/profile_web.py
✅ Files skipped from review due to trivial changes (3)
- wren/src/wren/connector/databricks.py
- .github/workflows/wren-ci.yml
- wren/src/wren/profile_cli.py
🚧 Files skipped from review as they are similar to previous changes (2)
- wren/src/wren/profile_web.py
- wren/pyproject.toml
add_profile can raise ValueError (malformed profiles.yml) or OSError (filesystem permission error); surface these as friendly error messages instead of unhandled HTTP 500s. 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 `@wren/src/wren/profile_web.py`:
- Around line 115-127: The HTML responses interpolate user-controlled values
({exc} and {name}) without escaping; modify the handler in profile_web.py to
HTML-escape these values before interpolation (e.g., use html.escape on exc and
name), update the HTMLResponse calls to use the escaped variables, and add the
necessary import (from html import escape) so that error messages and profile
names cannot inject raw HTML.
- Line 7: The usage example currently unpacks two values from
create_app("my-profile") but create_app now returns three values; update the
example to unpack and name the third return value (e.g., change the unpacking of
app, result to app, result, server or app, result, config depending on the
actual third return) so the call matches create_app's signature and all returned
values are handled; update any subsequent references in profile_web.py that
expect the third value.
- Around line 99-109: The form parsing in profile_web.py should be hardened:
first ensure form.get("_json") is a str and non-empty before calling json.loads;
catch json.JSONDecodeError as before and also verify the parsed value is a dict
(only then call profile.update(parsed_json)); in the fallback loop over
form.items() (the code handling profile population), skip values that are not
plain strings (i.e., check isinstance(v, str) before calling v.strip()) so
file-like uploads or other non-string form fields are ignored; reference the
existing variables/profile and _INTERNAL to apply these guards around the json
branch and the for k, v in form.items() branch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ba29ffc5-3e91-487a-a7a4-788a6c76f3eb
📒 Files selected for processing (1)
wren/src/wren/profile_web.py
- fix docstring example: create_app returns 3 values, not 2 - escape user-controlled values (name, exc) in HTMLResponse to prevent XSS - guard _json form field: check isinstance str before strip/parse, verify parsed value is a dict before profile.update - guard form loop: skip non-str values (e.g. file uploads) before strip Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
wren/src/wren/profile_web.py (1)
116-121:⚠️ Potential issue | 🟠 MajorAdd a final fallback for unexpected save failures
add_profile(...)handling is better now, but this block still lets unexpected exceptions escape as raw 500s. Add a finalexcept Exceptionwith server-side logging and a generic user message (and set explicit error status codes).Proposed fix
+import logging @@ +logger = logging.getLogger(__name__) @@ - try: - add_profile(name, profile, activate=activate) - except (ValueError, OSError) as exc: + try: + add_profile(name, profile, activate=activate) + except ValueError as exc: return HTMLResponse( - f'<small style="color:var(--pico-color-red-500)">✗ Failed to save profile: {escape(str(exc))}</small>' + f'<small style="color:var(--pico-color-red-500)">✗ Failed to save profile: {escape(str(exc))}</small>', + status_code=400, + ) + except OSError as exc: + return HTMLResponse( + f'<small style="color:var(--pico-color-red-500)">✗ Failed to save profile: {escape(str(exc))}</small>', + status_code=500, + ) + except Exception: + logger.exception("Unexpected error while saving profile") + return HTMLResponse( + '<small style="color:var(--pico-color-red-500)">✗ Failed to save profile due to an unexpected error.</small>', + status_code=500, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wren/src/wren/profile_web.py` around lines 116 - 121, The current try/except around add_profile(name, profile, activate=activate) only returns HTMLResponses without explicit status codes and lacks a generic catcher, so add: keep the existing except (ValueError, OSError) but return explicit status_code values (e.g., 400 for ValueError, 500 for OSError) and then add a final except Exception as exc that logs the full exception server-side (use the module logger or logger.exception) and returns a generic HTMLResponse with a 500 status_code and non-sensitive message (use escape for any displayed text). Ensure you reference add_profile, HTMLResponse, escape and the module logger when making the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wren/src/wren/profile_web.py`:
- Around line 86-89: The code calls .strip() on form values without ensuring
they are strings, causing AttributeError for UploadFile objects; update the
reads for datasource, _profile_name, and _variant so you first retrieve the raw
value (e.g., v = form.get("datasource", "")), check if isinstance(v, str) (or
otherwise coerce non-strings to ""), then call .strip(). Apply the same
defensive normalization to name (from "_profile_name" with fallback
profile_name) and variant_key (from "_variant") before using them so file
uploads won't trigger 500 errors.
---
Duplicate comments:
In `@wren/src/wren/profile_web.py`:
- Around line 116-121: The current try/except around add_profile(name, profile,
activate=activate) only returns HTMLResponses without explicit status codes and
lacks a generic catcher, so add: keep the existing except (ValueError, OSError)
but return explicit status_code values (e.g., 400 for ValueError, 500 for
OSError) and then add a final except Exception as exc that logs the full
exception server-side (use the module logger or logger.exception) and returns a
generic HTMLResponse with a 500 status_code and non-sensitive message (use
escape for any displayed text). Ensure you reference add_profile, HTMLResponse,
escape and the module logger when making the changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b005b968-7f3f-446e-81c7-092af40497be
📒 Files selected for processing (1)
wren/src/wren/profile_web.py
Comment on lines
+86
to
+89
| ds = form.get("datasource", "").strip().lower() | ||
| name = form.get("_profile_name", profile_name) | ||
| variant_key = form.get("_variant") | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify direct scalar extraction sites in profile_web save handler.
rg -n -C3 'form\.get\("datasource"|form\.get\("_profile_name"|form\.get\("_variant"' wren/src/wren/profile_web.pyRepository: Canner/wren-engine
Length of output: 394
🌐 Web query:
Starlette form API request.form() multipart values non-string types
💡 Result:
In Starlette, request.form for multipart/form-data requests returns a FormData object, which is an immutable multidict. The values in this multidict are either str for regular text fields or UploadFile objects for file uploads. UploadFile is a non-string type that wraps a SpooledTemporaryFile (file-like object) with additional attributes like filename (str|None), content_type (str|None), headers (Headers), and size (int). It provides async methods: read, write, seek, and close. Text fields are always strings. Files are detected based on the Content-Disposition header having a filename parameter (per RFC-7578). If missing enctype="multipart/form-data" in HTML forms or no filename, files may parse as strings (filenames only). Example usage: async with request.form as form: text_value: str = form["text_field"] file_value: UploadFile = form["file_field"] contents = await file_value.read filename = file_value.filename Requires python-multipart installed. Limits: max_files, max_fields, max_part_size for security. No other non-string types (e.g., int, bool) in FormData; all non-file values are str.
Citations:
- 1: https://www.starlette.io/requests
- 2: https://starlette.dev/requests/
- 3: https://github.com/encode/starlette/blob/master/starlette/requests.py
- 4: https://docs.plateforme.io/latest/reference/api/data
- 5: https://github.com/encode/starlette/blob/f6ea760a80d8b109fb6afd1c03e9a33754e6bb5f/starlette/datastructures.py
🏁 Script executed:
#!/bin/bash
# Search for all form.get() calls in the file to identify the scope of this issue
rg -n 'form\.get\(' wren/src/wren/profile_web.py
# Also check if there are any existing type guards for form values in this file
rg -n 'isinstance.*form|form.*isinstance' wren/src/wren/profile_web.pyRepository: Canner/wren-engine
Length of output: 265
Guard scalar form fields before calling .strip() to prevent 500 errors from file uploads
Line 86 calls .strip() directly on form.get("datasource", "") without type checking. Starlette's multipart form API can return UploadFile objects (not strings) for file uploads, which lack a .strip() method and cause an AttributeError → 500 error. Normalize datasource, _profile_name, and _variant defensively at read time.
Proposed fix
- ds = form.get("datasource", "").strip().lower()
- name = form.get("_profile_name", profile_name)
- variant_key = form.get("_variant")
+ raw_ds = form.get("datasource")
+ ds = raw_ds.strip().lower() if isinstance(raw_ds, str) else ""
+
+ raw_name = form.get("_profile_name")
+ name = (
+ raw_name.strip()
+ if isinstance(raw_name, str) and raw_name.strip()
+ else profile_name
+ )
+
+ raw_variant = form.get("_variant")
+ variant_key = (
+ raw_variant.strip()
+ if isinstance(raw_variant, str) and raw_variant.strip()
+ else None
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ds = form.get("datasource", "").strip().lower() | |
| name = form.get("_profile_name", profile_name) | |
| variant_key = form.get("_variant") | |
| raw_ds = form.get("datasource") | |
| ds = raw_ds.strip().lower() if isinstance(raw_ds, str) else "" | |
| raw_name = form.get("_profile_name") | |
| name = ( | |
| raw_name.strip() | |
| if isinstance(raw_name, str) and raw_name.strip() | |
| else profile_name | |
| ) | |
| raw_variant = form.get("_variant") | |
| variant_key = ( | |
| raw_variant.strip() | |
| if isinstance(raw_variant, str) and raw_variant.strip() | |
| else None | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wren/src/wren/profile_web.py` around lines 86 - 89, The code calls .strip()
on form values without ensuring they are strings, causing AttributeError for
UploadFile objects; update the reads for datasource, _profile_name, and _variant
so you first retrieve the raw value (e.g., v = form.get("datasource", "")),
check if isinstance(v, str) (or otherwise coerce non-strings to ""), then call
.strip(). Apply the same defensive normalization to name (from "_profile_name"
with fallback profile_name) and variant_key (from "_variant") before using them
so file uploads won't trigger 500 errors.
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
1 participant
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
wren/model/field_registry.py): single source of truth for all 20+ datasource connection fields, derived from Pydantic models. Eliminates the three separate hardcoded field dicts that existed inprofile_cli.py,mcp-server/app/web.py, anddocs.py.wren profile add --ui): opens a temporary local web server, renders a connection form in the browser, saves the completed profile to~/.wren/profiles.yml, then shuts down automatically — same pattern asgh auth login.wren profile add --interactivenow covers all 20+ datasources (previously only 5), with correct password hiding, file upload for BigQuery credentials, and variant selection for Databricks/Redshift/BigQuery.Changes
wren/src/wren/model/field_registry.pyDATASOURCE_MODELS,FieldDef,get_fields(),get_variants(),get_datasource_options()wren/src/wren/docs.pyDATASOURCE_MODELSfromfield_registryinstead of defining it locallywren/src/wren/profile_cli.py_COMMON_FIELDSwithget_fields(); add--ui/--port/--no-openflagswren/src/wren/profile_web.pycreate_app()factory andstart()wren/src/wren/templates/profile_form.htmlwren/src/wren/templates/_profile_fields.htmlmcp-server/app/web.pyDATASOURCE_FIELDSwith_build_datasource_fields()from field registry; graceful fallback ifwrennot installedwren/pyproject.toml[ui]optional extra: starlette, uvicorn, jinja2, python-multipartwren/tests/test_field_registry.pywren/tests/test_profile_web.pyTestClient-based tests: form rendering, field partials, all POST /save pathswren/README.mdwren profilecommands and--uimodeTest plan
PYTHONPATH=src pytest tests/test_field_registry.py— 31 tests, no Docker neededPYTHONPATH=src pytest tests/test_profile_web.py— 23 tests (requiresuv sync --extra dev --extra ui)PYTHONPATH=src pytest tests/test_profile.py tests/test_profile_cli.py— existing profile tests still passwren profile add test --ui --no-open— confirm URL is printed and form workswren profile add test --interactive— confirm all datasources are available and passwords are hidden🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Packaging
Tests
Chores