feat(wren): add profile management for named connection profiles#1509
Open
goldmedal wants to merge 8 commits intoCanner:mainfrom
Open
feat(wren): add profile management for named connection profiles#1509goldmedal wants to merge 8 commits intoCanner:mainfrom
goldmedal wants to merge 8 commits intoCanner:mainfrom
Conversation
Adds a DBT-like profile system so users can define named connection profiles in ~/.wren/profiles.yml and avoid passing --connection-file / --datasource on every command. - New `wren profile` sub-app: list, add, rm, switch, debug - Active profile auto-supplies datasource + connection_info to all commands; explicit flags always win for backward compatibility - WREN_HOME env var overrides the ~/.wren base path - Sensitive fields (password, token, credentials, etc.) masked in debug - 34 unit + CLI integration tests Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
github-actions
bot
added
the
dependencies
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 YAML-backed profile persistence under ~/.wren/profiles.yml, a new Changes
Sequence DiagramsequenceDiagram
participant User as CLI User
participant CLI as "wren/cli.py"
participant ProfileMgr as "wren/profile.py"
participant Storage as "~/.wren/profiles.yml"
participant Engine as "DataSource / WrenEngine"
User->>CLI: invoke CLI command (engine or profile subcommand)
CLI->>ProfileMgr: resolve_connection(explicit args) / get_active_profile()
ProfileMgr->>Storage: read or write profiles.yml
Storage-->>ProfileMgr: profiles data (or missing)
ProfileMgr-->>CLI: (datasource, connection_info)
alt datasource or connection_info found
CLI->>Engine: construct DataSource / WrenEngine with resolved data
Engine-->>CLI: engine instance / result
CLI-->>User: run command / print output
else no datasource found
CLI-->>User: print error and exit
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
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: 4
🤖 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/cli.py`:
- Around line 146-168: dry-plan currently bypasses the active-profile resolution
and uses its own datasource path in dry_plan(), so when no --datasource or
connection file is provided it fails; modify dry_plan() to use the same
resolution as _build_engine() (or call _build_engine() directly) so the
active-profile fallback (using get_active_profile(), DataSource parsing, and
returning WrenEngine with prof_dict) is applied, ensuring it uses
_load_conn/_resolve_datasource only as the downstream path; also add a
regression test that sets an active profile (via get_active_profile or test
fixture) and asserts that invoking dry-plan picks up the profile datasource
without explicit flags.
In `@wren/src/wren/profile_cli.py`:
- Around line 60-74: The imported profile JSON/YAML from --from-file is stored
raw into profile_data so later callers (e.g., _build_engine()) see
connection_info={"properties": ...} instead of the flattened {"datasource":...,
"properties":...} envelope produced by the --connection-file path; update the
from_file handling in profile_cli.py to normalize the loaded profile into the
same envelope shape as the connection-file flow: validate required keys,
move/flatten fields into a top-level "datasource" and "properties" structure (or
reject and raise typer.Exit(1) with a clear error) and ensure parsing errors and
post-normalization validation produce user-facing typer.echo messages before
exiting so downstream callers like _build_engine() receive a consistent
connection_info shape.
In `@wren/src/wren/profile.py`:
- Around line 15-22: The _load_raw function should reject malformed or
mis-shaped profiles.yml rather than letting callers fail; catch yaml.YAMLError
when reading and re-raise a clear exception, then validate the loaded value is a
dict with keys "active" and "profiles" where "profiles" is itself a dict (and
"active" is either None or a string), and raise a deterministic ValueError (or
custom error) with a helpful message if validation fails; update the
implementation in _load_raw (referencing _PROFILES_FILE and _load_raw) to
perform these checks and raise clear errors instead of returning unexpected
shapes.
- Around line 25-30: The current _save_raw uses Path.write_text which is not
atomic and respects process umask; change _save_raw to write to a temporary file
in the same directory (use _PROFILES_FILE.parent/_WREN_HOME), open the temp file
with os.open using flags (O_CREAT|O_WRONLY|O_TRUNC) and mode 0o600 to force file
permissions, write the YAML bytes, call file.flush() and os.fsync(fd) (and
os.fchmod(fd, 0o600) for extra safety), close the fd and then atomically replace
the target with os.replace(temp_path, _PROFILES_FILE) to avoid partial writes
and ensure 0600 permissions; ensure _WREN_HOME.mkdir(parents=True,
exist_ok=True) runs before creating the temp file.
🪄 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: 3cd0454f-ae1a-4df0-8b22-3f26ad4c82b1
⛔ Files ignored due to path filters (1)
wren/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
wren/pyproject.tomlwren/src/wren/cli.pywren/src/wren/profile.pywren/src/wren/profile_cli.pywren/tests/test_profile.pywren/tests/test_profile_cli.py
- dry-plan: apply active profile fallback (was bypassing profile resolution)
- profile_cli: normalize MCP envelope {datasource, properties:{}} on --from-file import
- profile_cli: validate imported file has a 'datasource' key; better parse error messages
- profile: validate profiles.yml shape in _load_raw (YAML parse errors, wrong types)
- profile: write profiles.yml atomically with 0600 permissions via tempfile + os.replace
- tests: add 6 new tests covering permissions, malformed YAML, envelope normalization
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
wren/src/wren/cli.py (1)
145-164: Consider consolidating profile resolution logic.Both
_build_engine()(lines 147-164) anddry_plan()(lines 331-354) duplicate the profile fallback pattern. Theresolve_connection()function inprofile.pywas designed for this purpose but isn't used. Consolidating would reduce code duplication and ensure consistent behavior.Also applies to: 329-354
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wren/src/wren/cli.py` around lines 145 - 164, The profile fallback logic in _build_engine() and dry_plan() is duplicated; replace those blocks with a single call to the existing resolve_connection() in profile.py (instead of manually calling get_active_profile and validating datasource) so both functions use the same resolution path. Locate the duplicated code in _build_engine() and dry_plan(), call resolve_connection(manifest_str, datasource, connection_file, connection_info) (or the appropriate signature in profile.py), handle any returned datasource enum conversion (DataSource) and errors the same way resolve_connection documents, and pass the returned connection_info and data_source into WrenEngine (the same parameters used previously) to preserve behavior. Ensure imports reference resolve_connection and remove the old manual get_active_profile / DataSource ValueError handling to avoid duplication.wren/src/wren/profile.py (1)
80-87: Silent fallback when active profile is missing.If
activereferences a profile name that doesn't exist inprofiles, this returns(name, {}). Callers likecli.pycheckif prof_dict:(line 151), so they silently fall through. Consider logging a warning here or returning(None, {})for consistency with "no active profile".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wren/src/wren/profile.py` around lines 80 - 87, get_active_profile currently returns (name, {}) when the "active" name is missing from "profiles", causing callers that check "if prof_dict:" to silently treat it as no profile; update get_active_profile to detect when name is not a key in profiles and treat that case like "no active profile" by returning (None, {}) and emitting a warning via the module logger (e.g. logging.getLogger(__name__).warning) mentioning the missing profile name; modify the logic in get_active_profile to first load data, read name, check profiles = data.get("profiles", {}), and if name is None or name not in profiles return (None, {}) after logging the missing reference, otherwise return (name, dict(profiles[name])).
🤖 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/cli.py`:
- Around line 162-164: Wrap the WrenEngine creation so any
pydantic.ValidationError raised while converting profile connection info is
caught and turned into a user-friendly error: import pydantic.ValidationError
and catch it around the call that passes prof_dict to WrenEngine (the return
WrenEngine(... manifest_str=manifest_str, data_source=ds,
connection_info=prof_dict) line), then raise a clear CLI-friendly exception
(e.g., click.ClickException or re-raise ValueError with a concise message) that
includes the validation errors from the ValidationError instead of letting the
traceback propagate.
In `@wren/src/wren/profile.py`:
- Around line 157-163: The current masking in profile.py uses _SENSITIVE and
leaks fields like access_key, key_id, client_id, bucket, endpoint,
s3_staging_dir; update the masking logic by replacing the blacklist with an
explicit whitelist of safe fields (e.g., SAFE_FIELDS =
{"name","region","profile_name",...}) and mask everything not in SAFE_FIELDS, or
if you prefer to keep a blacklist, expand _SENSITIVE to include substrings like
"access", "key", "key_id", "client_id", "bucket", "endpoint", "s3_staging_dir",
"staging_dir" and use the existing any(...) check; modify the loop over
profile.items() (the masked dict creation) to consult the new SAFE_FIELDS
(allow) or expanded _SENSITIVE (deny) so that access_key, key_id, client_id and
other secret-like keys are always replaced with "***".
- Around line 122-140: The docstring for resolve_connection() is incorrect about
falling back to a legacy ~/.wren/connection_info.json; update the docstring to
state the actual behavior: "Priority: explicit flags > active profile" and that
legacy file handling is performed elsewhere (cli.py::_load_conn), or remove the
legacy fallback clause entirely; ensure the docstring continues to describe the
returned tuple (datasource_or_None, connection_dict) and reference
resolve_connection and get_active_profile so readers know where the
active-profile comes from and that legacy fallback lives in cli.py::_load_conn.
---
Nitpick comments:
In `@wren/src/wren/cli.py`:
- Around line 145-164: The profile fallback logic in _build_engine() and
dry_plan() is duplicated; replace those blocks with a single call to the
existing resolve_connection() in profile.py (instead of manually calling
get_active_profile and validating datasource) so both functions use the same
resolution path. Locate the duplicated code in _build_engine() and dry_plan(),
call resolve_connection(manifest_str, datasource, connection_file,
connection_info) (or the appropriate signature in profile.py), handle any
returned datasource enum conversion (DataSource) and errors the same way
resolve_connection documents, and pass the returned connection_info and
data_source into WrenEngine (the same parameters used previously) to preserve
behavior. Ensure imports reference resolve_connection and remove the old manual
get_active_profile / DataSource ValueError handling to avoid duplication.
In `@wren/src/wren/profile.py`:
- Around line 80-87: get_active_profile currently returns (name, {}) when the
"active" name is missing from "profiles", causing callers that check "if
prof_dict:" to silently treat it as no profile; update get_active_profile to
detect when name is not a key in profiles and treat that case like "no active
profile" by returning (None, {}) and emitting a warning via the module logger
(e.g. logging.getLogger(__name__).warning) mentioning the missing profile name;
modify the logic in get_active_profile to first load data, read name, check
profiles = data.get("profiles", {}), and if name is None or name not in profiles
return (None, {}) after logging the missing reference, otherwise return (name,
dict(profiles[name])).
🪄 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: 6f0cf955-56cd-4632-98cb-b89e4dbcf412
📒 Files selected for processing (5)
wren/src/wren/cli.pywren/src/wren/profile.pywren/src/wren/profile_cli.pywren/tests/test_profile.pywren/tests/test_profile_cli.py
✅ Files skipped from review due to trivial changes (1)
- wren/tests/test_profile_cli.py
🚧 Files skipped from review as they are similar to previous changes (2)
- wren/tests/test_profile.py
- wren/src/wren/profile_cli.py
- cli: catch pydantic.ValidationError when building WrenEngine from profile connection info, surface friendly error instead of raw traceback - profile: fix misleading docstring on resolve_connection() — legacy connection_info.json fallback lives in cli._load_conn(), not here - profile: expand _SENSITIVE mask set (access_key, key_id, client_id, bucket, endpoint, staging_dir, hostname, http_path, role_arn) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
wren/src/wren/profile.py (1)
145-181: Consider hoisting_SENSITIVEto module level.The
_SENSITIVEset is recreated on everydebug_profile()call. Moving it to module scope avoids repeated allocation. This is a minor optimization but improves clarity for maintainers.♻️ Suggested refactor
+_SENSITIVE = frozenset({ + "password", + "credentials", + "secret", + "token", + "private_key", + "access_key", + "key_id", + "client_id", + "bucket", + "endpoint", + "staging_dir", + "hostname", + "http_path", + "role_arn", +}) + + def debug_profile(name: str | None = None) -> dict[str, Any]: """Return diagnostic info for a profile (or the active one). Masks sensitive fields (password, credentials, secret, token). """ if name is None: name = get_active_name() if name is None: return {"error": "no active profile"} data = _load_raw() profile = data.get("profiles", {}).get(name) if profile is None: return {"error": f"profile '{name}' not found"} - _SENSITIVE = { - "password", - "credentials", - ... - } masked = {} for k, v in profile.items(): if k.lower() in _SENSITIVE or any(s in k.lower() for s in _SENSITIVE): masked[k] = "***" else: masked[k] = v return {"name": name, "active": data.get("active") == name, "config": masked}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wren/src/wren/profile.py` around lines 145 - 181, The _SENSITIVE set is defined inside debug_profile() causing it to be reallocated on every call; hoist it to module scope (e.g., define SENSITIVE or _SENSITIVE at top of wren/src/wren/profile.py) and update debug_profile to reference that module-level constant, leaving the masking logic in debug_profile (for keys and any(...) checks) unchanged so behavior remains identical.wren/src/wren/cli.py (1)
335-360: Consider extracting shared profile-resolution logic.The active-profile loading pattern (lines 337-360) is largely duplicated from
_build_engine()(lines 147-170). A shared helper like_resolve_from_profile(manifest_str) -> tuple[DataSource, dict] | Nonecould reduce duplication and ensure consistent behavior.This is a good-to-have improvement that can be deferred.
♻️ Possible helper extraction
def _try_active_profile_datasource() -> tuple[DataSource, dict] | None: """Return (DataSource, conn_dict) from active profile or None.""" from wren.profile import get_active_profile # noqa: PLC0415 from wren.model.data_source import DataSource # noqa: PLC0415 _name, prof_dict = get_active_profile() if not prof_dict: return None ds_str = prof_dict.pop("datasource", None) if ds_str is None: typer.echo("Error: no datasource in active profile.", err=True) raise typer.Exit(1) try: ds = DataSource(ds_str.lower()) except ValueError: typer.echo(f"Error: unknown datasource '{ds_str}'", err=True) raise typer.Exit(1) return ds, prof_dictThen both
_build_engine()anddry_plan()can call this helper.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wren/src/wren/cli.py` around lines 335 - 360, Extract the duplicated active-profile resolution into a helper (e.g., _try_active_profile_datasource) that imports get_active_profile and DataSource, returns (DataSource, dict) or None, and replicates current behavior: if no profile return None, if profile lacks "datasource" echo the same error and raise typer.Exit(1), and if the datasource string is invalid echo the unknown-datasource error and raise typer.Exit(1); then replace the duplicated blocks in _build_engine() and the dry_plan CLI block (the code that constructs DataSource from prof_dict and opens WrenEngine) to call this helper and use its returned (ds, conn_dict) when present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@wren/src/wren/cli.py`:
- Around line 335-360: Extract the duplicated active-profile resolution into a
helper (e.g., _try_active_profile_datasource) that imports get_active_profile
and DataSource, returns (DataSource, dict) or None, and replicates current
behavior: if no profile return None, if profile lacks "datasource" echo the same
error and raise typer.Exit(1), and if the datasource string is invalid echo the
unknown-datasource error and raise typer.Exit(1); then replace the duplicated
blocks in _build_engine() and the dry_plan CLI block (the code that constructs
DataSource from prof_dict and opens WrenEngine) to call this helper and use its
returned (ds, conn_dict) when present.
In `@wren/src/wren/profile.py`:
- Around line 145-181: The _SENSITIVE set is defined inside debug_profile()
causing it to be reallocated on every call; hoist it to module scope (e.g.,
define SENSITIVE or _SENSITIVE at top of wren/src/wren/profile.py) and update
debug_profile to reference that module-level constant, leaving the masking logic
in debug_profile (for keys and any(...) checks) unchanged so behavior remains
identical.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d813a613-ac78-4309-98a9-326c3c051ec1
📒 Files selected for processing (2)
wren/src/wren/cli.pywren/src/wren/profile.py
typer.Choice does not exist; Click is the underlying library. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
wren/src/wren/profile_cli.py (1)
60-94: Reject conflictingaddmodes explicitly.Currently, conflicting flags are silently resolved by precedence (
--from-filewins). Returning an explicit error is clearer and prevents accidental misconfiguration.Suggested fix
+ if from_file and interactive: + typer.echo( + "Error: --from-file and --interactive cannot be used together.", + err=True, + ) + raise typer.Exit(1) + if from_file and datasource: + typer.echo( + "Error: --datasource cannot be combined with --from-file.", + err=True, + ) + raise typer.Exit(1) + if from_file:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wren/src/wren/profile_cli.py` around lines 60 - 94, Detect and reject conflicting add modes by adding an explicit check before the existing mode selection: if both from_file and interactive are truthy, call typer.echo with a clear error like "Error: cannot use --from-file and --interactive together." to stderr and raise typer.Exit(1). Make the change in the same function that references from_file, interactive, datasource, _interactive_add and profile_data so the new check runs before the current "if from_file:" branch that currently gives precedence to --from-file.
🤖 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_cli.py`:
- Around line 132-135: The interactive prompt currently calls typer.prompt in
the fields loop and accepts sensitive inputs in clear text; update the loop in
profile_cli.py that iterates over fields (the for field in fields block) to
detect sensitive field names (e.g., "password", "pass", "secret", "credentials",
"api_key", "token") and call typer.prompt with input masking (hide_input=True or
equivalent) for those fields (or use getpass()) while leaving non-sensitive
prompts unchanged; ensure the collected value is still assigned into the profile
dict (profile[field] = value) only when a value is provided.
- Around line 67-75: The code calls path.read_text() outside the try/except, so
IO or decode errors can crash the CLI; wrap the file read in the same guarded
block (or add a separate try/except around path.read_text()) and on any
exception from path.read_text() echo a descriptive error with the filename
(using typer.echo or process logger) and raise typer.Exit(1). Specifically
update the logic around path.read_text(), the try that currently parses into raw
using yaml.safe_load/json.loads, and ensure both read and parse failures are
handled the same way (referencing path.read_text(), the variable text, and the
existing except Exception as exc handling).
---
Nitpick comments:
In `@wren/src/wren/profile_cli.py`:
- Around line 60-94: Detect and reject conflicting add modes by adding an
explicit check before the existing mode selection: if both from_file and
interactive are truthy, call typer.echo with a clear error like "Error: cannot
use --from-file and --interactive together." to stderr and raise typer.Exit(1).
Make the change in the same function that references from_file, interactive,
datasource, _interactive_add and profile_data so the new check runs before the
current "if from_file:" branch that currently gives precedence to --from-file.
🪄 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: f1db000d-4050-4619-9ca3-227be1c0d80a
📒 Files selected for processing (1)
wren/src/wren/profile_cli.py
wren/src/wren/profile_cli.py
Outdated
Comment on lines
+67
to
+75
| text = path.read_text() | ||
| try: | ||
| if path.suffix in (".yml", ".yaml"): | ||
| raw = yaml.safe_load(text) | ||
| else: | ||
| raw = json.loads(text) | ||
| except Exception as exc: | ||
| typer.echo(f"Error: could not parse {from_file}: {exc}", err=True) | ||
| raise typer.Exit(1) |
Contributor
There was a problem hiding this comment.
Handle file read failures before parse to avoid traceback exits.
Line 67 (path.read_text()) is outside the guarded parse block, so unreadable files (permissions, directory path, decode errors) can crash the CLI instead of returning a clean typer.Exit(1).
Suggested fix
- text = path.read_text()
try:
+ text = path.read_text()
if path.suffix in (".yml", ".yaml"):
raw = yaml.safe_load(text)
else:
raw = json.loads(text)
except Exception as exc:
typer.echo(f"Error: could not parse {from_file}: {exc}", err=True)
raise typer.Exit(1)📝 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
| text = path.read_text() | |
| try: | |
| if path.suffix in (".yml", ".yaml"): | |
| raw = yaml.safe_load(text) | |
| else: | |
| raw = json.loads(text) | |
| except Exception as exc: | |
| typer.echo(f"Error: could not parse {from_file}: {exc}", err=True) | |
| raise typer.Exit(1) | |
| try: | |
| text = path.read_text() | |
| if path.suffix in (".yml", ".yaml"): | |
| raw = yaml.safe_load(text) | |
| else: | |
| raw = json.loads(text) | |
| except Exception as exc: | |
| typer.echo(f"Error: could not parse {from_file}: {exc}", err=True) | |
| raise typer.Exit(1) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wren/src/wren/profile_cli.py` around lines 67 - 75, The code calls
path.read_text() outside the try/except, so IO or decode errors can crash the
CLI; wrap the file read in the same guarded block (or add a separate try/except
around path.read_text()) and on any exception from path.read_text() echo a
descriptive error with the filename (using typer.echo or process logger) and
raise typer.Exit(1). Specifically update the logic around path.read_text(), the
try that currently parses into raw using yaml.safe_load/json.loads, and ensure
both read and parse failures are handled the same way (referencing
path.read_text(), the variable text, and the existing except Exception as exc
handling).
DuckDB uses LocalFileConnectionInfo which expects url (directory path) and format (defaults to 'duckdb'), not a 'path' field. Also gives local_file its own entry and shows field defaults in interactive prompts. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
wren/src/wren/profile_cli.py (2)
67-75:⚠️ Potential issue | 🟠 MajorHandle file read failures inside the guarded parse block.
Line 67 reads the file outside the
try, so unreadable paths (permissions, directory input, decode errors) can still crash with a traceback instead of clean CLI exit handling.Proposed fix
- text = path.read_text() try: + text = path.read_text() if path.suffix in (".yml", ".yaml"): raw = yaml.safe_load(text) else: raw = json.loads(text) except Exception as exc: typer.echo(f"Error: could not parse {from_file}: {exc}", err=True) raise typer.Exit(1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wren/src/wren/profile_cli.py` around lines 67 - 75, The file read operation (path.read_text()) is outside the try/except so IO or decode errors escape the CLI; move the read into the same try block that parses into raw (the block that handles yaml.safe_load/json.loads) and catch exceptions from both reading and parsing, using the existing typer.echo(f"Error: could not parse {from_file}: {exc}", err=True) and raise typer.Exit(1) on failure so unreadable files or decode errors are handled cleanly; update references to text/raw inside that try and keep the existing error message and exit behavior.
154-157:⚠️ Potential issue | 🟠 MajorMask sensitive values during interactive prompts.
Lines 154-157 currently echo sensitive inputs (for example
password/credentials) in clear text duringwren profile add --interactive.Proposed fix
+ _SENSITIVE_FIELDS = { + "password", + "pass", + "secret", + "credentials", + "api_key", + "token", + "private_key", + } for field, default in fields: - value = typer.prompt(f" {field}", default=default, show_default=bool(default)) + value = typer.prompt( + f" {field}", + default=default, + show_default=bool(default), + hide_input=field.lower() in _SENSITIVE_FIELDS, + ) if value: profile[field] = valueIn the Typer version used by this repository, does `typer.prompt()` support `hide_input` and pass it to Click’s prompt implementation?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wren/src/wren/profile_cli.py` around lines 154 - 157, The interactive prompt loop over fields (the for field, default in fields block that calls typer.prompt and assigns into profile[field]) is echoing sensitive values; detect sensitive field names (e.g., contains "password", "secret", "token", "api_key", "credentials") and call the prompt with hidden input for those entries (use typer.prompt(..., hide_input=True) if the Typer version supports hide_input, otherwise call click.prompt(hide_input=True) or fallback to getpass.getpass) so sensitive values are not echoed; ensure the logic still respects the default and show_default behavior and only assigns profile[field] when a value is provided.
🧹 Nitpick comments (1)
wren/src/wren/profile_cli.py (1)
107-108: Differentiate “added” vs “overwritten” profile output.
add_profileoverwrites existing names, but Line 108 always prints “added.” Consider surfacing overwrite explicitly (or requiring a confirm flag) to avoid accidental silent replacement.Proposed refactor
- from wren.profile import add_profile # noqa: PLC0415 + from wren.profile import add_profile, list_profiles # noqa: PLC0415 ... - add_profile(name, profile_data, activate=activate) - typer.echo(f"Profile '{name}' added.") + existed = name in list_profiles() + add_profile(name, profile_data, activate=activate) + typer.echo(f"Profile '{name}' {'updated' if existed else 'added'}.")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wren/src/wren/profile_cli.py` around lines 107 - 108, The CLI always prints "Profile 'name' added." even when add_profile(name, profile_data, activate=activate) overwrites an existing profile; change the flow to detect whether the name already existed before calling add_profile (or have add_profile return a status like created/overwritten) and print "added" or "overwritten" accordingly, and optionally add a --confirm/--force flag to require explicit consent for overwrites; update the echo logic that currently prints Profile '{name}' added. to reflect the actual outcome (created vs overwritten).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@wren/src/wren/profile_cli.py`:
- Around line 67-75: The file read operation (path.read_text()) is outside the
try/except so IO or decode errors escape the CLI; move the read into the same
try block that parses into raw (the block that handles
yaml.safe_load/json.loads) and catch exceptions from both reading and parsing,
using the existing typer.echo(f"Error: could not parse {from_file}: {exc}",
err=True) and raise typer.Exit(1) on failure so unreadable files or decode
errors are handled cleanly; update references to text/raw inside that try and
keep the existing error message and exit behavior.
- Around line 154-157: The interactive prompt loop over fields (the for field,
default in fields block that calls typer.prompt and assigns into profile[field])
is echoing sensitive values; detect sensitive field names (e.g., contains
"password", "secret", "token", "api_key", "credentials") and call the prompt
with hidden input for those entries (use typer.prompt(..., hide_input=True) if
the Typer version supports hide_input, otherwise call
click.prompt(hide_input=True) or fallback to getpass.getpass) so sensitive
values are not echoed; ensure the logic still respects the default and
show_default behavior and only assigns profile[field] when a value is provided.
---
Nitpick comments:
In `@wren/src/wren/profile_cli.py`:
- Around line 107-108: The CLI always prints "Profile 'name' added." even when
add_profile(name, profile_data, activate=activate) overwrites an existing
profile; change the flow to detect whether the name already existed before
calling add_profile (or have add_profile return a status like
created/overwritten) and print "added" or "overwritten" accordingly, and
optionally add a --confirm/--force flag to require explicit consent for
overwrites; update the echo logic that currently prints Profile '{name}' added.
to reflect the actual outcome (created vs overwritten).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 15f74774-1498-45c4-8a85-f03b4713a8bd
📒 Files selected for processing (1)
wren/src/wren/profile_cli.py
local_file still prompts for format since the user needs to specify csv/parquet/json. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
wren/src/wren/profile_cli.py (2)
67-75:⚠️ Potential issue | 🟠 MajorWrap
path.read_text()in the guarded block.Line 67 is outside
try, so read/decode failures can crash the command instead of returningExit(1).Fix
- text = path.read_text() try: + text = path.read_text() if path.suffix in (".yml", ".yaml"): raw = yaml.safe_load(text) else: raw = json.loads(text)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wren/src/wren/profile_cli.py` around lines 67 - 75, The call to path.read_text() is outside the try/except so IO or decoding errors can escape; move the read into the guarded block so any exception from reading or parsing is caught and handled the same way as parsing errors: wrap path.read_text() together with the existing yaml.safe_load/json.loads logic inside the try that currently catches Exception as exc, and on exception call typer.echo(f"Error: could not parse {from_file}: {exc}", err=True) and then raise typer.Exit(1) (preserving the current error message behavior).
157-160:⚠️ Potential issue | 🟠 MajorMask sensitive values in interactive prompts.
password/credentials-like fields are currently entered in clear text. Usehide_input=Truefor sensitive keys.Fix
+ sensitive = {"password", "pass", "secret", "credentials", "api_key", "token", "private_key"} for field, default in fields: - value = typer.prompt(f" {field}", default=default, show_default=bool(default)) + value = typer.prompt( + f" {field}", + default=default, + show_default=bool(default), + hide_input=field.lower() in sensitive, + ) if value: profile[field] = valueIn the Typer version used by this repository, does typer.prompt() support the hide_input parameter?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wren/src/wren/profile_cli.py` around lines 157 - 160, The interactive prompt loop that builds profile entries (the for field, default in fields: block that assigns profile[field]) must mask sensitive keys: detect keys like "password", "credentials", "secret", "api_key", "token" (case-insensitive) and call typer.prompt(..., hide_input=True) for those fields; for non-sensitive fields keep the current behavior. Also add a fallback: if the Typer version in use does not accept hide_input, use getpass.getpass to read hidden input and fall back to typer.prompt for visible input. Update the loop that writes into profile[field] accordingly.
🧹 Nitpick comments (1)
wren/src/wren/profile_cli.py (1)
102-105: Avoid hardcoding the profile path in user guidance.This message is inaccurate when
WREN_HOMEis set. Prefer a dynamic path or generic wording.Lightweight wording change
- "Edit ~/.wren/profiles.yml to add connection fields." + "Edit your profiles.yml (WREN_HOME or ~/.wren) to add connection fields."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wren/src/wren/profile_cli.py` around lines 102 - 105, The echoed guidance hardcodes "~/.wren/profiles.yml"; update the message in profile_cli.py (where typer.echo is called, e.g., in the profile creation handler) to compute the actual profiles file path dynamically and display that instead of the literal tilde, by checking WREN_HOME (os.environ.get("WREN_HOME") or the module's WREN_HOME constant) and joining "profiles.yml" with that directory (fallback to Path.home()/".wren"/"profiles.yml" if unset), then include the computed path in the user-facing string.
🤖 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_cli.py`:
- Around line 22-23: Wrap calls to list_profiles() and get_active_name() in a
try/except that catches ValueError (originating from _load_raw()) and raise
typer.Exit(code=1) to present a clean user error; apply the same pattern around
add_profile(...), remove_profile(...), switch_profile(...), and
debug_profile(...) invocations so any malformed profiles.yml ValueError is
caught and converted into typer.Exit(1). Make sure to reference these functions
(list_profiles, get_active_name, add_profile, remove_profile, switch_profile,
debug_profile) when adding the try/except and do not change other error
handling.
---
Duplicate comments:
In `@wren/src/wren/profile_cli.py`:
- Around line 67-75: The call to path.read_text() is outside the try/except so
IO or decoding errors can escape; move the read into the guarded block so any
exception from reading or parsing is caught and handled the same way as parsing
errors: wrap path.read_text() together with the existing
yaml.safe_load/json.loads logic inside the try that currently catches Exception
as exc, and on exception call typer.echo(f"Error: could not parse {from_file}:
{exc}", err=True) and then raise typer.Exit(1) (preserving the current error
message behavior).
- Around line 157-160: The interactive prompt loop that builds profile entries
(the for field, default in fields: block that assigns profile[field]) must mask
sensitive keys: detect keys like "password", "credentials", "secret", "api_key",
"token" (case-insensitive) and call typer.prompt(..., hide_input=True) for those
fields; for non-sensitive fields keep the current behavior. Also add a fallback:
if the Typer version in use does not accept hide_input, use getpass.getpass to
read hidden input and fall back to typer.prompt for visible input. Update the
loop that writes into profile[field] accordingly.
---
Nitpick comments:
In `@wren/src/wren/profile_cli.py`:
- Around line 102-105: The echoed guidance hardcodes "~/.wren/profiles.yml";
update the message in profile_cli.py (where typer.echo is called, e.g., in the
profile creation handler) to compute the actual profiles file path dynamically
and display that instead of the literal tilde, by checking WREN_HOME
(os.environ.get("WREN_HOME") or the module's WREN_HOME constant) and joining
"profiles.yml" with that directory (fallback to
Path.home()/".wren"/"profiles.yml" if unset), then include the computed path in
the user-facing string.
🪄 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: 45dcd31b-b365-453c-922f-751b439d68a0
📒 Files selected for processing (1)
wren/src/wren/profile_cli.py
wren/src/wren/profile_cli.py
Outdated
Comment on lines
+22
to
+23
| profiles = list_profiles() | ||
| active = get_active_name() |
Contributor
There was a problem hiding this comment.
Handle malformed profiles.yml errors consistently across commands.
These calls can raise ValueError (via _load_raw()), which currently bubbles as a traceback. Please catch and convert to a clean typer.Exit(1) user error.
Suggested pattern
+def _exit_profile_error(exc: Exception) -> None:
+ typer.echo(f"Error: {exc}", err=True)
+ raise typer.Exit(1)
+
`@profile_app.command`("list")
def list_cmd() -> None:
@@
- profiles = list_profiles()
- active = get_active_name()
+ try:
+ profiles = list_profiles()
+ active = get_active_name()
+ except ValueError as exc:
+ _exit_profile_error(exc)Apply the same guard around add_profile, remove_profile, switch_profile, and debug_profile calls.
Also applies to: 107-107, 179-179, 193-193, 209-209
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wren/src/wren/profile_cli.py` around lines 22 - 23, Wrap calls to
list_profiles() and get_active_name() in a try/except that catches ValueError
(originating from _load_raw()) and raise typer.Exit(code=1) to present a clean
user error; apply the same pattern around add_profile(...), remove_profile(...),
switch_profile(...), and debug_profile(...) invocations so any malformed
profiles.yml ValueError is caught and converted into typer.Exit(1). Make sure to
reference these functions (list_profiles, get_active_name, add_profile,
remove_profile, switch_profile, debug_profile) when adding the try/except and do
not change other error handling.
- profile_cli: move path.read_text() inside try block so I/O errors (bad permissions, encoding) produce a clean error instead of a traceback - profile_cli: hide_input=True for sensitive fields (password, credentials, token, secret, private_key) in interactive add prompts - profile_cli: catch ValueError from malformed profiles.yml in list, rm, switch, debug commands — previously produced a raw Python traceback Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
Author
Code reviewFound 3 issues (all fixed in commit f9cd1c2):
🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
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/profiles.ymlprofile system so users can define named connection profiles and avoid passing--connection-file/--datasourceon every commandwren profilesub-app withlist,add,rm,switch, anddebugcommandsdatasource+connection_infoto all CLI commands; explicit flags always take priority (backward compatible)WREN_HOMEenv var overrides the~/.wrenbase pathpassword,token,credentials,private_key,secret) are masked as***indebugoutputExample usage
profiles.yml schema
Test plan
uv run pytest tests/test_profile.py tests/test_profile_cli.py -v— 34 tests, all passuv run ruff format --check src/ && uv run ruff check src/— cleanwren profile add/wren queryend-to-end against a live DuckDB instance🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Dependencies
Tests