feat(wren): CLI 0.2.0 — context management, profiles, strict mode & memory#1522
Merged
feat(wren): CLI 0.2.0 — context management, profiles, strict mode & memory#1522
Conversation
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…#1510) Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…ll (#1514) Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…e-mdl skill (#1513) Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…profile add --ui) (#1512) Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…gement (#1511) Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…hecks (#1515) Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…ettings (#1517) Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…oject (#1516) Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…skills (#1518) Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…#1520) Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Replace connection_info.json/mdl.json references with profile + project flow - Update error recovery to use `wren profile debug` instead of cat connection file - Update Workflow 4 to use validate → build → index instead of manual cp - Fix memory default path description (project-local, not ~/.wren/) - Add profile and context commands to decision tree Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Clarify that DuckDB data sources should use the DB file name without extension as the catalog value (e.g. jaffle_shop.duckdb → jaffle_shop). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Reflect new wren CLI subsystems (context, profile, config, policy, field registry, utils, docs) and skills directory restructuring. Remove static Package Structure section in favor of navigable sections. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
github-actions
bot
added
documentation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a CLI-first MDL project system (YAML per-entity), named connection profiles with optional browser UI, a centralized datasource field registry, seed-query memory seeding, strict SQL policy checks configurable via WrenConfig, connector refactors (SecretStr→str), CLI utilities, packaging/CI updates, a large test suite, and skills/docs reorganization. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as "wren CLI"
participant Profile as "Profile Web"
participant Context as "Context Manager"
participant Engine as "WrenEngine"
participant Memory as "Memory Store"
User->>CLI: wren profile add --ui
CLI->>Profile: start(profile_name, activate?, port, open)
Profile->>User: render form (datasource options from field_registry)
User->>Profile: submit form
Profile->>CLI: save profile -> ~/.wren/profiles.yml
User->>CLI: wren context init / build
CLI->>Context: scaffold or build manifest
Context->>CLI: write target/mdl.json
User->>CLI: wren --sql "Natural language question"
CLI->>CLI: resolve active profile -> connection_info
CLI->>Context: discover/load target/mdl.json
CLI->>Engine: WrenEngine(manifest, data_source, conn_info, config)
Engine->>Engine: parse SQL -> validate_sql_policy(ast, model_names, config)
Engine->>Engine: plan & execute
Engine->>CLI: results
CLI->>CLI: is_exploratory(sql)?
alt non-exploratory
CLI->>Memory: store query (may include seed entries)
CLI->>User: print results + store-tip (unless --quiet)
else exploratory
CLI->>User: print results
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![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: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wren/src/wren/model/__init__.py (1)
18-23:⚠️ Potential issue | 🟠 Major
to_key_string()now ignores non-secret fields, risking cache key collisions.After this refactor, fields like
host,port,database, anduserare no longerSecretStr, so they won't be included in the key string. Two connections differing only by host or port would produce identical keys.Consider including all relevant fields in the key, not just secrets:
🔧 Proposed fix
def to_key_string(self) -> str: key_parts = [] for _, field_value in self: if isinstance(field_value, SecretStr): key_parts.append(field_value.get_secret_value()) + elif isinstance(field_value, str): + key_parts.append(field_value) return "|".join(key_parts)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wren/src/wren/model/__init__.py` around lines 18 - 23, The to_key_string method currently only appends SecretStr values which causes collisions when non-secret connection fields change; update to_key_string to include all relevant field values (convert SecretStr via get_secret_value and use str(...) for other types) and preserve a deterministic order (use field names or the class's iteration order) so that values like host, port, database, and user are part of the produced key; reference the to_key_string method and SecretStr handling when making this change.
🧹 Nitpick comments (14)
wren/src/wren/memory/embeddings.py (1)
10-13: Environment-based model configuration is useful, but dimension is hardcoded.The env-var override for
_DEFAULT_MODELis helpful for CI testing with smaller models. However,_DEFAULT_DIM = 384remains hardcoded and may not match all models that could be configured viaWREN_EMBEDDING_MODEL. If a user specifies a model with different dimensions,default_dimension()would return an incorrect value.Consider dynamically fetching the dimension from the model or documenting that
_DEFAULT_DIMonly applies to the default model family.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wren/src/wren/memory/embeddings.py` around lines 10 - 13, The hardcoded _DEFAULT_DIM may not match an override of _DEFAULT_MODEL; change default_dimension() to derive the embedding size from the configured model string instead of using the fixed _DEFAULT_DIM: read _DEFAULT_MODEL (or os.getenv("WREN_EMBEDDING_MODEL")) and map known model names to their dims (or attempt to load model metadata to get dimensionality) and fall back to _DEFAULT_DIM only if no mapping/metadata is available; update references to _DEFAULT_DIM, _DEFAULT_MODEL, and the default_dimension() helper so the returned dimension matches the selected model.skills-archive/SKILLS.md (1)
32-35: Removebashlanguage identifier from code fence inside blockquote.Based on learnings, code fences inside blockquotes in this repository intentionally omit the
bashlanguage identifier to avoid supply-chain security scanners (Snyk/Socket) flagging them as executable shell commands.Proposed fix
> Installing `wren-usage` via `install.sh` automatically installs all dependent skills: -> ```bash +> ``` > bash skills-archive/install.sh wren-usage > ```🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills-archive/SKILLS.md` around lines 32 - 35, In the SKILLS.md blockquote that shows the install example, remove the "bash" language identifier from the inline code fence around the install command (the fenced block containing "bash skills-archive/install.sh wren-usage") so the fence is just ``` rather than ```bash; update the quoted lines in that block (the lines beginning with "> ```bash" and ending with "> ```") to use a plain code fence to avoid marking the snippet as an executable shell block.wren/tests/unit/test_engine.py (1)
118-122:_COMBINED_CONFIGis defined but never used.The combined configuration preset is declared but not referenced by any test. Consider either adding a test that uses it or removing the unused definition.
Option 1: Remove unused config
_STRICT_CONFIG = WrenConfig(strict_mode=True) _BLACKLIST_CONFIG = WrenConfig(denied_functions=frozenset(["pg_read_file"])) -_COMBINED_CONFIG = WrenConfig( - strict_mode=True, denied_functions=frozenset(["pg_read_file"]) -)Option 2: Add test using combined config
def test_combined_strict_and_blacklist(): """Verify both strict mode and denied functions work together.""" conn_info = {"url": "/tmp", "format": "duckdb"} with WrenEngine( _MANIFEST_STR, DataSource.duckdb, conn_info, fallback=False, config=_COMBINED_CONFIG, ) as engine: # Should block unknown table (strict mode) with pytest.raises(WrenError) as exc_info: engine.dry_plan("SELECT * FROM secret_table") assert exc_info.value.error_code == ErrorCode.MODEL_NOT_FOUND # Should also block denied function with pytest.raises(WrenError) as exc_info: engine.dry_plan("SELECT pg_read_file('/etc/passwd')") assert exc_info.value.error_code == ErrorCode.BLOCKED_FUNCTION🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wren/tests/unit/test_engine.py` around lines 118 - 122, Add a unit test that uses the previously defined _COMBINED_CONFIG to verify strict_mode and denied_functions enforce together: create a test function named test_combined_strict_and_blacklist that opens a WrenEngine with _MANIFEST_STR, DataSource.duckdb, conn_info={"url": "/tmp", "format": "duckdb"}, fallback=False and config=_COMBINED_CONFIG; assert that engine.dry_plan("SELECT * FROM secret_table") raises WrenError with error_code ErrorCode.MODEL_NOT_FOUND and that engine.dry_plan("SELECT pg_read_file('/etc/passwd')") raises WrenError with error_code ErrorCode.BLOCKED_FUNCTION so both behaviors are validated together.wren/src/wren/templates/profile_form.html (1)
37-41: Expose HTMX updates as live regions.
#fieldsand#resultchange asynchronously, but assistive tech will not be notified today. Add live-region semantics so newly injected fields and save results are announced.♿ Proposed fix
- <div id="fields"></div> + <div id="fields" aria-live="polite"></div> ... - <div id="result" style="min-height:1.4em;margin-top:.4rem"></div> + <div id="result" role="status" aria-live="polite" aria-atomic="true" style="min-height:1.4em;margin-top:.4rem"></div>🤖 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 37 - 41, The two async-updated containers "#fields" and "#result" are not announced to assistive tech; add ARIA live-region semantics by giving the fields container a role="region" and aria-live="polite" aria-atomic="true" (so injected form fields are exposed) and give the result container aria-live="polite" aria-atomic="true" (or role="status" if you want higher priority) so HTMX updates to `#fields` and `#result` are announced; update the template elements with those attributes on the divs with id="fields" and id="result".skills-archive/check-versions.sh (2)
26-26: Version extraction regex may be fragile with edge cases.The sed pattern
's/.*version: *"\{0,1\}\([^"]*\)"\{0,1\}/\1/'handles both quoted and unquoted versions, but could fail if:
- The frontmatter has multiline values before
version:- The version string contains special characters
Consider using a YAML parser (via Python) for robustness, similar to how you handle JSON parsing.
♻️ Proposed fix using Python YAML parsing
- md_version=$(grep -m1 'version:' "$skill_file" | sed 's/.*version: *"\{0,1\}\([^"]*\)"\{0,1\}/\1/' | tr -d ' "') + md_version=$(python3 -c " +import re +with open('$skill_file') as f: + content = f.read() +# Extract YAML frontmatter between --- markers +match = re.search(r'^---\s*\n(.*?)\n---', content, re.DOTALL) +if match: + for line in match.group(1).split('\n'): + if line.strip().startswith('version:'): + v = line.split(':', 1)[1].strip().strip('\"') + print(v) + break + else: + print('MISSING') +else: + print('MISSING') +")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills-archive/check-versions.sh` at line 26, The current brittle sed extraction for md_version in check-versions.sh (the md_version assignment that reads from skill_file) should be replaced with a YAML parse using Python: call a short Python snippet that reads skill_file, extracts the YAML frontmatter (between the leading and trailing '---'), uses yaml.safe_load to parse it, and prints the "version" value so md_version is assigned robustly; update references to md_version to rely on that parsed value and ensure the script checks for missing/empty result and errors out if no version is found.
12-15: Skill name extraction could be simplified.The current approach uses multiple string substitutions. Consider using a JSON-aware approach consistent with the rest of the script.
♻️ Optional simplification
- skill_name="${skill//\"/}" - skill_name="${skill_name%%:*}" - skill_name="${skill_name// /}" + skill_name=$(echo "$skill" | tr -d '": ' | cut -d',' -f1)Or better, move the skill iteration into the Python block that generates the union.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills-archive/check-versions.sh` around lines 12 - 15, The skill-name extraction loop uses brittle string substitutions; replace the multi-step parsing inside the while IFS= read -r skill; do ... skill_name=... block with a JSON-aware extraction (e.g. set skill_name="$(jq -r '.name' <<<"$skill")") or move the entire skill iteration into the Python block that generates the union so names are obtained with Python's JSON parsing; update references to the skill_name variable and remove the three substitution lines (skill_name="${skill//\"/}", skill_name="${skill_name%%:*}", skill_name="${skill_name// /}") accordingly.skills-archive/wren-generate-mdl/SKILL.md (1)
210-215: Consider updating the/wren-connection-inforeference.Line 214 references
/wren-connection-infofor connection field documentation. Given the PR's shift to CLI-first architecture withwren profile addandwren docs connection-info, verify this reference is still the preferred approach for archived MCP-based skills, or update to point to the CLI alternative.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills-archive/wren-generate-mdl/SKILL.md` around lines 210 - 215, Update the /wren-connection-info reference in the Connection setup section of SKILL.md to reflect the current CLI-first flow: verify whether archived MCP-based skills should still point to the Web UI note or be changed to the CLI commands; if the CLI is preferred, replace the /wren-connection-info mention with instructions to use "wren profile add" to pre-configure connection info and/or reference "wren docs connection-info" for field details, and ensure the note still mentions the Web UI fallback when WEB_UI_ENABLED=false for backward compatibility.docs/quickstart.md (2)
80-88: Consider security implications of the curl|bash install option.While the
curl|bashpattern is common, it's worth noting that Option 1 (npx skills add) is the safer installation method. The current documentation already lists it first, which is good. Consider adding a brief note that Option 1 is preferred for security-conscious users.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/quickstart.md` around lines 80 - 88, Add a brief security note recommending the first install method as safer: indicate that using "npx skills add Canner/wren-engine --skill '*' --agent claude-code" is preferred for security-conscious users and that the "curl -fsSL ... | bash" option runs remote code and should be used with caution (verify the script first or use an alternative). Mention both commands (npx skills add and curl ... | bash) so reviewers can locate the lines to update.
109-114: Important clarification on DuckDB path — consider emphasizing further.The note that the database path is the directory containing
.duckdbfiles (not the.duckdbfile itself) is a common point of confusion. Consider making this more prominent (e.g., bold or a warning callout) to prevent user errors.📝 Optional enhancement
-- **Database path:** `/Users/you/jaffle_shop_duckdb` — the **directory** containing `.duckdb` files, not the `.duckdb` file itself (your absolute path from Step 1) +- **Database path:** `/Users/you/jaffle_shop_duckdb` — ⚠️ This must be the **directory** containing `.duckdb` files, **not** the `.duckdb` file itself (your absolute path from Step 1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/quickstart.md` around lines 109 - 114, Emphasize that the "Database path" field expects the directory containing .duckdb files (not the .duckdb file) by making that sentence more prominent in docs/quickstart.md: update the "Database path:" line to include a bolded or warning-style callout around "directory containing .duckdb files (not the .duckdb file itself)" and optionally add a short example or note under the "Database path" bullet to show an absolute directory path vs. a file path to prevent confusion.wren/src/wren/memory/seed_queries.py (1)
40-74: PotentialKeyErroron missing dictionary keys.The function accesses
model["name"]andcol["name"]directly without defensive checks. If a malformed manifest is passed (e.g., a model or column missing thenamekey), this will raise an unhandledKeyError.Consider using
.get()with a fallback or adding validation. Given that the AI summary indicates this module is "consumed byMemoryStore._upsert_seed_queries()" which processes user-provided manifests, defensive handling may be warranted.🛡️ Optional defensive fix
def _model_seeds(model: dict) -> list[dict]: - name = model["name"] + name = model.get("name") + if not name: + return [] columns = model.get("columns", []) pairs = []🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wren/src/wren/memory/seed_queries.py` around lines 40 - 74, The _model_seeds function assumes model["name"] and col["name"] exist and can raise KeyError for malformed manifests; update _model_seeds to read model_name = model.get("name") and skip/return early if falsy, and for each column use col_name = col.get("name") and skip columns without a name before using them in logic (also keep existing model.get("primaryKey") checks). Ensure pairs.append templates only use a valid model_name and that numeric_col/group_col assignment uses col_name (and continues to check isCalculated via col.get("isCalculated", False)). This defensive validation prevents KeyError when MemoryStore._upsert_seed_queries() passes incomplete manifests.wren/src/wren/memory/store.py (1)
158-182: Consider batch insertion for seed queries.The current implementation calls
store_query()in a loop (lines 175-180), which computes embeddings and performs table operations individually for each seed pair. For manifests with many models/relationships, this could be slow.If performance becomes an issue, consider batching: compute all embeddings in one call, then insert all records at once.
♻️ Sketch of batch approach
def _upsert_seed_queries(self, manifest: dict) -> int: from wren.memory.seed_queries import SEED_TAG, generate_seed_queries if _QUERY_TABLE in _table_names(self._db): table = self._db.open_table(_QUERY_TABLE) table.delete(f"tags = '{SEED_TAG}'") pairs = generate_seed_queries(manifest) if not pairs: return 0 # Batch embed all NL queries texts = [p["nl"] for p in pairs] vectors = self._embed_fn.compute_source_embeddings(texts) now = datetime.now(timezone.utc) records = [ { "text": p["nl"], "vector": vec, "nl_query": p["nl"], "sql_query": p["sql"], "datasource": "", "created_at": now, "tags": SEED_TAG, } for p, vec in zip(pairs, vectors) ] if _QUERY_TABLE in _table_names(self._db): self._db.open_table(_QUERY_TABLE).add(records) else: self._db.create_table(_QUERY_TABLE, records, schema=self._query_table_schema()) return len(pairs)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wren/src/wren/memory/store.py` around lines 158 - 182, The loop in _upsert_seed_queries calls store_query for each seed which computes embeddings and does table ops per-item; replace this with a batched path: after generate_seed_queries, build a list of texts, call self._embed_fn.compute_source_embeddings once to get vectors, assemble record dicts (include fields used by store_query: text/vector/nl_query/sql_query/datasource/created_at/tags), then use self._db.open_table(_QUERY_TABLE).add(records) or self._db.create_table(_QUERY_TABLE, records, schema=self._query_table_schema()) depending on _table_names(this._db) to insert all at once; keep the initial deletion of SEED_TAG and return len(pairs). Ensure you reference SEED_TAG, _QUERY_TABLE, generate_seed_queries, _embed_fn.compute_source_embeddings, _query_table_schema in the change.wren/tests/unit/test_context.py (1)
545-552: Redundant imports at module level after initial imports.Lines 545-549 re-import modules that are already imported at the top of the file (
pytestat line 5,jsonat line 5). Thejson as _jsonalias is never used. Consider removing these redundant imports.Proposed cleanup
-import base64 -import json as _json - -import orjson -import pytest +import base64 +import orjson + from wren.context import validate_manifest🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wren/tests/unit/test_context.py` around lines 545 - 552, Remove the redundant module-level imports in the test file: eliminate the repeated imports of pytest and json (the alias json as _json) shown in the diff so only the original top-of-file imports remain; specifically remove the duplicated "import base64 / import json as _json / import orjson / import pytest" block and any unused alias references (there are none for _json), keeping only the single canonical imports used by tests and leaving validate_manifest and DataSource imports intact.wren/src/wren/profile_web.py (2)
202-206: Minor: potential race condition in_free_port.Between releasing the socket and the server binding to the port, another process could claim it. This is a known TOCTOU pattern, but acceptable for a development/CLI tool context since the window is tiny and failure would be immediately visible.
🤖 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 202 - 206, The _free_port function risks a TOCTOU race by releasing the socket before the server binds; to fix, change the design so the socket remains bound until the server takes it: modify _free_port to return the open socket object (not just the port) or add a helper like reserve_port_and_start that accepts a server-start callback and binds the socket (using socket.socket/AF_INET, SOCK_STREAM) then calls the callback while the socket is still held; update all callers of _free_port to either accept the socket return value or use the new reserve_port_and_start function so the port cannot be claimed between release and server bind.
160-165: Bareexcept Exceptionis appropriate here but consider narrowing scope.The catch-all exception handler with logging is reasonable for a user-facing web endpoint to prevent stack traces from leaking. However, if there are specific known exceptions from
add_profile, consider catching those explicitly before the generic handler.🤖 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 160 - 165, The current user-facing handler for saving profiles uses a broad "except Exception" around the add_profile call (logger.exception and HTMLResponse) — narrow this by identifying and adding explicit except clauses for the known exceptions raised by add_profile (e.g., validation errors, duplicate/profile-specific errors) before the final generic except; keep the existing logger.exception and 500 HTMLResponse for the fallback, but handle each specific exception with an appropriate log level/message and user-facing response (using the same HTMLResponse pattern) so that add_profile, logger.exception, and the HTMLResponse fallback remain intact while known errors are handled explicitly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mcp-server/app/web.py`:
- Around line 87-89: The current except ImportError around calling
_build_datasource_fields() hides any ImportError raised from
wren.model.field_registry or its dependencies; change the fallback so only a
genuine missing-package error triggers the hardcoded map. Replace the broad
except ImportError with a narrower check (e.g. except ModuleNotFoundError as e)
or inspect the ImportError to re-raise unless the missing module is the optional
registry package, so that failures inside wren.model.field_registry surface;
keep references to DATASOURCE_FIELDS and _build_datasource_fields and ensure
other ImportError causes are not silently swallowed.
In `@skills-archive/install.sh`:
- Around line 53-57: The script currently only sets INDEX_JSON from a local
checkout (variable INDEX_JSON in install.sh) so subset installs from stdin or
environments without python3 never expand dependencies; update install.sh to
detect when INDEX_JSON is unset and either (a) fetch the remote
skills-archive/index.json (using curl/wget) and assign it to INDEX_JSON before
resolving dependencies for piped or subset installs, or (b) if network fetch
fails or python3 is unavailable, exit with a non-zero error and a clear message
that dependency resolution cannot proceed; ensure this change is applied to all
dependency-resolution paths that reference INDEX_JSON (including the piped
installer path and the other blocks noted) so installs never silently skip
prerequisites.
In `@skills-archive/README.md`:
- Around line 9-18: Update the README commands to point at the archive directory
instead of the active skills directory: change the marketplace add command's
--path from "skills" to "skills-archive" and update the local development test
command from "claude --plugin-dir ./skills" to "claude --plugin-dir
./skills-archive" so the archived skill set is loaded; ensure the plugin install
instruction ("/plugin install wren@wren") remains unchanged but is documented
alongside the corrected --path target.
- Around line 52-54: The bulk-copy example in README.md mixes source and
destination directories and omits two skill folders; replace the current cp -r
line so all sources consistently come from skills-archive and include the
missing wren-quickstart and wren-http-api directories (e.g., copy
skills-archive/wren-usage skills-archive/wren-generate-mdl
skills-archive/wren-project skills-archive/wren-sql
skills-archive/wren-mcp-setup skills-archive/wren-connection-info
skills-archive/wren-quickstart skills-archive/wren-http-api into
~/.claude/skills/), ensuring the command sources only from skills-archive and
installs every listed wren-* skill.
In `@skills/wren-usage/SKILL.md`:
- Around line 215-220: Insert the missing build step so the startup flow
generates the manifest before indexing: update the sequence to run "wren context
build" immediately after "wren context init" and before "wren memory index"
(this ensures target/mdl.json is produced/updated); reference the commands "wren
context init", "wren context build", and "wren memory index" when updating the
README steps to prevent indexing against a missing or stale manifest.
In `@wren/README.md`:
- Around line 98-106: The README has duplicated step numbering: the heading
"**4. Run queries**" and the following heading "**4. Index schema for semantic
search**" should be sequential; update the second heading to "**5. Index schema
for semantic search**" (edit the markdown heading text for "Index schema for
semantic search" in README.md so the quick start steps are numbered correctly).
In `@wren/src/wren/cli.py`:
- Around line 533-535: The code registers the same sub-app twice via
app.add_typer(context_app), causing duplicate command registration; locate the
duplicate call to app.add_typer(context_app) (the third occurrence) and remove
it or replace it with the intended sub-app variable (e.g., the correct Typer
instance instead of context_app) so that context_app is added only once and no
duplicate commands are registered.
- Around line 16-17: The _WREN_HOME variable is being assigned from the
environment and then immediately overwritten, nullifying WREN_HOME; update the
_WREN_HOME initialization in wren/src/wren/cli.py so it reads the environment
variable with a default (e.g., Path(os.environ.get("WREN_HOME", Path.home() /
".wren")).expanduser()) and remove the subsequent hardcoded assignment that
resets _WREN_HOME, ensuring the environment override is honored.
In `@wren/src/wren/config.py`:
- Around line 40-42: The config loader currently only catches
json.JSONDecodeError and OSError, but config_path.read_text() can raise
UnicodeDecodeError which should be normalized to the same WrenError path; update
the try/except in wren/src/wren/config.py around config_path.read_text() and
json.loads() (the block that currently uses except (json.JSONDecodeError,
OSError) as e) to also catch UnicodeDecodeError (or catch exceptions from
read_text separately) and re-raise or convert the error into the same WrenError
used for JSON decode failures so all decode/read failures present a consistent
CLI error.
In `@wren/src/wren/context_cli.py`:
- Around line 303-310: The validate branch only calls
validate_project(project_path) and must also run the semantic dry-plan checks
used by the dry-run path so broken view SQL isn't written; update the
validate_first branch to invoke the same semantic/dry-plan routine used by the
--dry-run flow (the module's dry-plan/semantic validation function) after
validate_project(project_path), collect its errors alongside hard_errors, echo
any semantic errors and raise typer.Exit(1) when present so the validate path
prevents building/writing broken SQL (refer to validate_first, validate_project,
and the project's dry-run/dry-plan function used elsewhere).
In `@wren/src/wren/context.py`:
- Around line 94-109: The MDL import code in context.py builds project_config
without setting the required "name" field, causing validate_project() to fail;
update the block that constructs project_config (the dict passed into yaml.dump
for the ProjectFile with relative_path "wren_project.yml") to populate
project_config["name"] from the MDL (e.g., use mdl_json["name"] if present,
otherwise fallback to mdl_json.get("projectName") or a sensible default/derived
value) so the generated wren_project.yml includes a valid name entry.
- Around line 214-217: The loop writing files uses f.relative_path verbatim
which allows directory traversal; before creating/writing the file (the code
around the variables files, f.relative_path, output_dir, path.write_text),
validate and sanitize the target path: reject absolute paths and paths with
parent-traversal, compute the resolved target (from output_dir /
f.relative_path), ensure the resolved target is a child of output_dir.resolve()
(e.g., startswith output_dir.resolve() + path separator) and raise/log an error
or skip the file if it escapes, only then call path.parent.mkdir(...) and
path.write_text(...). Ensure the check covers symlinks by using resolve() on
both sides.
- Around line 209-217: When --force is used the code currently only skips the
existence guard but leaves stale managed files on disk; update the block around
project_file to, when project_file.exists() and force is True, remove the
existing managed tree under output_dir (e.g. delete the previous generated
subfolders such as models/, views/, relationships/ or the set of files that
correspond to the previously managed output) before writing new files; implement
this by using pathlib/shutil to recursively remove the relevant directories or
files referenced by output_dir (taking care to only remove the managed/generated
locations, not the whole user directory), then proceed with creating parents and
writing each file from files as currently done.
- Around line 527-551: The function collects schema_version parsing/unsupported
errors into the errors list but then proceeds to call
load_models/load_views/load_relationships (which call get_schema_version() and
can sys.exit), so change validate_project to short-circuit and return the
collected errors immediately after checking sv (i.e., after the block that
appends ValidationError for non-int or unsupported sv and before calling
load_models/load_views/load_relationships); reference the errors list, the local
sv variable, _SUPPORTED_SCHEMA_VERSIONS, and the loaders
load_models/load_views/load_relationships (and get_schema_version indirectly) to
locate where to insert the early return.
In `@wren/src/wren/memory/cli.py`:
- Around line 163-183: The code currently always calls discover_project_path()
to load instructions, which causes instructions from the current working dir to
be merged into an explicitly passed MDL; change the logic so that when an
explicit mdl path/option is provided (the variable that holds the selected MDL),
derive the project root from that MDL path when it matches the pattern
"<project>/target/mdl.json" (e.g., Path(mdl).endswith("target/mdl.json") ->
project_root = Path(mdl).parents[1]) and pass that project_root to
load_instructions(project_root); only call discover_project_path() when no
explicit mdl is given, and keep the existing exception swallowing around
load_instructions and the manifest["_instructions"] assignment.
In `@wren/src/wren/model/field_registry.py`:
- Around line 357-365: The selection logic silently falls back to models[0] when
an unknown variant is requested; update the branch that computes model_cls to
explicitly reject unknown variants: if multiple models exist and variant is
provided, search models using _get_variant_name and if no match is found raise a
clear ValueError (or custom exception) naming the requested variant and listing
available variants (derive names via _get_variant_name(models[i])); ensure this
change touches the code that computes model_cls so callers get an immediate
error instead of silently using the first model.
In `@wren/src/wren/profile_cli.py`:
- Around line 214-221: The prompt currently uses f.placeholder as the default so
pressing Enter persists example placeholders; change prompt behavior to never
use f.placeholder as the default: set prompt_default = f.default or "" (do not
fall back to f.placeholder), and if f.placeholder exists append it to the prompt
label (e.g., " {f.label} (e.g. {f.placeholder})") so it remains a hint; finally
ensure you only persist values that are non-empty and not equal to f.placeholder
(i.e., check value and value != f.placeholder before assigning profile[f.name] =
value).
In `@wren/src/wren/templates/_profile_fields.html`:
- Around line 19-35: Replace the current hidden-input fallback and add value
binding for visible inputs so default values are actually submitted: in the
branch where field.input_type == 'hidden' change the input value from value="{{
field.default or '' }}" to value="{{ field.default }}" to preserve falsey
defaults, and in the generic input branch (the <input type="{{ field.input_type
}}" name="{{ field.name }}"> line) add a value attribute value="{{ field.default
}}" (keeping placeholder and other attributes) so visible fields are prefilled
with field.default; leave the file_base64 block (ids like fb64-{{ field.name }}
/ fb64-file-{{ field.name }}) unchanged.
In `@wren/src/wren/templates/profile_form.html`:
- Around line 7-8: Replace the external CDN references to Pico and HTMX in the
profile_form.html (the <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> tags) with locally
served, vendored copies: download the specific versions used, add them to the
app’s static assets, update the href/src to point to those local asset paths,
and ensure the server serves them with appropriate caching headers; optionally
pin the files to exact versions and remove or avoid relying on external CDNs.
In `@wren/tests/unit/test_cli_store_tip.py`:
- Around line 24-25: Replace the POSIX-only "/dev/null" sentinel used in
module-level constants _CONN_FILE_ARGS and _MDL_ARGS with real temporary files
provided by pytest's tmp_path fixture: stop using hard-coded module constants
and instead construct the argument lists inside tests (or in a small helper like
make_args(tmp_path)) by creating tmp files (e.g., tmp_path.joinpath("conn.json")
and tmp_path.joinpath("mdl")) and passing their paths into
_CONN_FILE_ARGS/_MDL_ARGS equivalents; update any tests that reference the
module-level _CONN_FILE_ARGS/_MDL_ARGS to accept tmp_path and build the args at
test time so the suite is portable to Windows.
---
Outside diff comments:
In `@wren/src/wren/model/__init__.py`:
- Around line 18-23: The to_key_string method currently only appends SecretStr
values which causes collisions when non-secret connection fields change; update
to_key_string to include all relevant field values (convert SecretStr via
get_secret_value and use str(...) for other types) and preserve a deterministic
order (use field names or the class's iteration order) so that values like host,
port, database, and user are part of the produced key; reference the
to_key_string method and SecretStr handling when making this change.
---
Nitpick comments:
In `@docs/quickstart.md`:
- Around line 80-88: Add a brief security note recommending the first install
method as safer: indicate that using "npx skills add Canner/wren-engine --skill
'*' --agent claude-code" is preferred for security-conscious users and that the
"curl -fsSL ... | bash" option runs remote code and should be used with caution
(verify the script first or use an alternative). Mention both commands (npx
skills add and curl ... | bash) so reviewers can locate the lines to update.
- Around line 109-114: Emphasize that the "Database path" field expects the
directory containing .duckdb files (not the .duckdb file) by making that
sentence more prominent in docs/quickstart.md: update the "Database path:" line
to include a bolded or warning-style callout around "directory containing
.duckdb files (not the .duckdb file itself)" and optionally add a short example
or note under the "Database path" bullet to show an absolute directory path vs.
a file path to prevent confusion.
In `@skills-archive/check-versions.sh`:
- Line 26: The current brittle sed extraction for md_version in
check-versions.sh (the md_version assignment that reads from skill_file) should
be replaced with a YAML parse using Python: call a short Python snippet that
reads skill_file, extracts the YAML frontmatter (between the leading and
trailing '---'), uses yaml.safe_load to parse it, and prints the "version" value
so md_version is assigned robustly; update references to md_version to rely on
that parsed value and ensure the script checks for missing/empty result and
errors out if no version is found.
- Around line 12-15: The skill-name extraction loop uses brittle string
substitutions; replace the multi-step parsing inside the while IFS= read -r
skill; do ... skill_name=... block with a JSON-aware extraction (e.g. set
skill_name="$(jq -r '.name' <<<"$skill")") or move the entire skill iteration
into the Python block that generates the union so names are obtained with
Python's JSON parsing; update references to the skill_name variable and remove
the three substitution lines (skill_name="${skill//\"/}",
skill_name="${skill_name%%:*}", skill_name="${skill_name// /}") accordingly.
In `@skills-archive/SKILLS.md`:
- Around line 32-35: In the SKILLS.md blockquote that shows the install example,
remove the "bash" language identifier from the inline code fence around the
install command (the fenced block containing "bash skills-archive/install.sh
wren-usage") so the fence is just ``` rather than ```bash; update the quoted
lines in that block (the lines beginning with "> ```bash" and ending with ">
```") to use a plain code fence to avoid marking the snippet as an executable
shell block.
In `@skills-archive/wren-generate-mdl/SKILL.md`:
- Around line 210-215: Update the /wren-connection-info reference in the
Connection setup section of SKILL.md to reflect the current CLI-first flow:
verify whether archived MCP-based skills should still point to the Web UI note
or be changed to the CLI commands; if the CLI is preferred, replace the
/wren-connection-info mention with instructions to use "wren profile add" to
pre-configure connection info and/or reference "wren docs connection-info" for
field details, and ensure the note still mentions the Web UI fallback when
WEB_UI_ENABLED=false for backward compatibility.
In `@wren/src/wren/memory/embeddings.py`:
- Around line 10-13: The hardcoded _DEFAULT_DIM may not match an override of
_DEFAULT_MODEL; change default_dimension() to derive the embedding size from the
configured model string instead of using the fixed _DEFAULT_DIM: read
_DEFAULT_MODEL (or os.getenv("WREN_EMBEDDING_MODEL")) and map known model names
to their dims (or attempt to load model metadata to get dimensionality) and fall
back to _DEFAULT_DIM only if no mapping/metadata is available; update references
to _DEFAULT_DIM, _DEFAULT_MODEL, and the default_dimension() helper so the
returned dimension matches the selected model.
In `@wren/src/wren/memory/seed_queries.py`:
- Around line 40-74: The _model_seeds function assumes model["name"] and
col["name"] exist and can raise KeyError for malformed manifests; update
_model_seeds to read model_name = model.get("name") and skip/return early if
falsy, and for each column use col_name = col.get("name") and skip columns
without a name before using them in logic (also keep existing
model.get("primaryKey") checks). Ensure pairs.append templates only use a valid
model_name and that numeric_col/group_col assignment uses col_name (and
continues to check isCalculated via col.get("isCalculated", False)). This
defensive validation prevents KeyError when MemoryStore._upsert_seed_queries()
passes incomplete manifests.
In `@wren/src/wren/memory/store.py`:
- Around line 158-182: The loop in _upsert_seed_queries calls store_query for
each seed which computes embeddings and does table ops per-item; replace this
with a batched path: after generate_seed_queries, build a list of texts, call
self._embed_fn.compute_source_embeddings once to get vectors, assemble record
dicts (include fields used by store_query:
text/vector/nl_query/sql_query/datasource/created_at/tags), then use
self._db.open_table(_QUERY_TABLE).add(records) or
self._db.create_table(_QUERY_TABLE, records, schema=self._query_table_schema())
depending on _table_names(this._db) to insert all at once; keep the initial
deletion of SEED_TAG and return len(pairs). Ensure you reference SEED_TAG,
_QUERY_TABLE, generate_seed_queries, _embed_fn.compute_source_embeddings,
_query_table_schema in the change.
In `@wren/src/wren/profile_web.py`:
- Around line 202-206: The _free_port function risks a TOCTOU race by releasing
the socket before the server binds; to fix, change the design so the socket
remains bound until the server takes it: modify _free_port to return the open
socket object (not just the port) or add a helper like reserve_port_and_start
that accepts a server-start callback and binds the socket (using
socket.socket/AF_INET, SOCK_STREAM) then calls the callback while the socket is
still held; update all callers of _free_port to either accept the socket return
value or use the new reserve_port_and_start function so the port cannot be
claimed between release and server bind.
- Around line 160-165: The current user-facing handler for saving profiles uses
a broad "except Exception" around the add_profile call (logger.exception and
HTMLResponse) — narrow this by identifying and adding explicit except clauses
for the known exceptions raised by add_profile (e.g., validation errors,
duplicate/profile-specific errors) before the final generic except; keep the
existing logger.exception and 500 HTMLResponse for the fallback, but handle each
specific exception with an appropriate log level/message and user-facing
response (using the same HTMLResponse pattern) so that add_profile,
logger.exception, and the HTMLResponse fallback remain intact while known errors
are handled explicitly.
In `@wren/src/wren/templates/profile_form.html`:
- Around line 37-41: The two async-updated containers "#fields" and "#result"
are not announced to assistive tech; add ARIA live-region semantics by giving
the fields container a role="region" and aria-live="polite" aria-atomic="true"
(so injected form fields are exposed) and give the result container
aria-live="polite" aria-atomic="true" (or role="status" if you want higher
priority) so HTMX updates to `#fields` and `#result` are announced; update the
template elements with those attributes on the divs with id="fields" and
id="result".
In `@wren/tests/unit/test_context.py`:
- Around line 545-552: Remove the redundant module-level imports in the test
file: eliminate the repeated imports of pytest and json (the alias json as
_json) shown in the diff so only the original top-of-file imports remain;
specifically remove the duplicated "import base64 / import json as _json /
import orjson / import pytest" block and any unused alias references (there are
none for _json), keeping only the single canonical imports used by tests and
leaving validate_manifest and DataSource imports intact.
In `@wren/tests/unit/test_engine.py`:
- Around line 118-122: Add a unit test that uses the previously defined
_COMBINED_CONFIG to verify strict_mode and denied_functions enforce together:
create a test function named test_combined_strict_and_blacklist that opens a
WrenEngine with _MANIFEST_STR, DataSource.duckdb, conn_info={"url": "/tmp",
"format": "duckdb"}, fallback=False and config=_COMBINED_CONFIG; assert that
engine.dry_plan("SELECT * FROM secret_table") raises WrenError with error_code
ErrorCode.MODEL_NOT_FOUND and that engine.dry_plan("SELECT
pg_read_file('/etc/passwd')") raises WrenError with error_code
ErrorCode.BLOCKED_FUNCTION so both behaviors are validated together.
🪄 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: 23f43d8a-5524-4e0a-b526-fc03239792f8
⛔ Files ignored due to path filters (1)
wren/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (100)
.claude/CLAUDE.md.github/workflows/wren-ci.ymlcli-skills/wren-usage/SKILL.mddocs/README.mddocs/getting_started_with_claude_code.mddocs/mdl/model.mddocs/mdl/view.mddocs/quickstart.mddocs/wren_project.mdmcp-server/app/web.pyskills-archive/.claude-plugin/marketplace.jsonskills-archive/.claude-plugin/plugin.jsonskills-archive/AUTHORING.mdskills-archive/README.mdskills-archive/SKILLS.mdskills-archive/check-versions.shskills-archive/index.jsonskills-archive/install.shskills-archive/versions.jsonskills-archive/wren-connection-info/SKILL.mdskills-archive/wren-connection-info/references/databases.mdskills-archive/wren-connection-info/references/file-sources.mdskills-archive/wren-generate-mdl/SKILL.mdskills-archive/wren-http-api/SKILL.mdskills-archive/wren-http-api/references/response-format.mdskills-archive/wren-http-api/references/tools.mdskills-archive/wren-http-api/scripts/session.shskills-archive/wren-mcp-setup/SKILL.mdskills-archive/wren-project/SKILL.mdskills-archive/wren-quickstart/SKILL.mdskills-archive/wren-sql/SKILL.mdskills-archive/wren-sql/references/bigquery.mdskills-archive/wren-sql/references/correction.mdskills-archive/wren-sql/references/datetime.mdskills-archive/wren-sql/references/types.mdskills-archive/wren-usage/SKILL.mdskills/.claude-plugin/marketplace.jsonskills/.claude-plugin/plugin.jsonskills/AUTHORING.mdskills/README.mdskills/SKILLS.mdskills/index.jsonskills/install.shskills/versions.jsonskills/wren-generate-mdl/SKILL.mdskills/wren-usage/SKILL.mdskills/wren-usage/references/memory.mdskills/wren-usage/references/wren-sql.mdwren/.claude/CLAUDE.mdwren/README.mdwren/docs/cli.mdwren/pyproject.tomlwren/src/wren/__init__.pywren/src/wren/cli.pywren/src/wren/config.pywren/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/context.pywren/src/wren/context_cli.pywren/src/wren/docs.pywren/src/wren/engine.pywren/src/wren/mdl/cte_rewriter.pywren/src/wren/memory/__init__.pywren/src/wren/memory/cli.pywren/src/wren/memory/embeddings.pywren/src/wren/memory/schema_indexer.pywren/src/wren/memory/seed_queries.pywren/src/wren/memory/store.pywren/src/wren/model/__init__.pywren/src/wren/model/data_source.pywren/src/wren/model/error.pywren/src/wren/model/field_registry.pywren/src/wren/policy.pywren/src/wren/profile_cli.pywren/src/wren/profile_web.pywren/src/wren/sql_classify.pywren/src/wren/templates/_profile_fields.htmlwren/src/wren/templates/profile_form.htmlwren/src/wren/type_mapping.pywren/src/wren/utils_cli.pywren/tests/connectors/test_duckdb.pywren/tests/connectors/test_postgres.pywren/tests/suite/query.pywren/tests/test_field_registry.pywren/tests/test_profile_web.pywren/tests/unit/test_cli_store_tip.pywren/tests/unit/test_config.pywren/tests/unit/test_context.pywren/tests/unit/test_context_cli.pywren/tests/unit/test_convert_mdl.pywren/tests/unit/test_cte_rewriter.pywren/tests/unit/test_engine.pywren/tests/unit/test_memory.pywren/tests/unit/test_policy.pywren/tests/unit/test_seed_queries.pywren/tests/unit/test_sql_classify.pywren/tests/unit/test_type_mapping.py
💤 Files with no reviewable changes (2)
- cli-skills/wren-usage/SKILL.md
- docs/getting_started_with_claude_code.md
Fix critical bugs: WREN_HOME env var override nullified by duplicate assignment, context_app registered twice, and directory traversal vulnerability in MDL import file writing. Fix major issues: cache key collisions in to_key_string(), broad ImportError hiding regressions in mcp-server, missing project name in MDL import, schema version errors causing abrupt SystemExit, wrong instructions merged with explicit --mdl, placeholder persisted as profile value, silent fallback on unknown variant, and HTML form default rendering. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wren/src/wren/model/__init__.py (1)
18-25:⚠️ Potential issue | 🟠 Major
to_key_string()still collapses distinct connection configs.Only
SecretStrandstrvalues contribute to the key, so configs that differ only inenable_ssl,secure,ssl_enabled,settings, orkwargsstill produce the same string. If this key backs connector caching, you'll reuse the wrong connection settings for some profiles.🧩 Serialize the whole normalized config instead of only strings
+import json from enum import Enum from typing import Annotated, Literal, Union @@ def to_key_string(self) -> str: - key_parts = [] - for _, field_value in self: - if isinstance(field_value, SecretStr): - key_parts.append(field_value.get_secret_value()) - elif isinstance(field_value, str): - key_parts.append(field_value) - return "|".join(key_parts) + def _normalize(value): + if isinstance(value, SecretStr): + return value.get_secret_value() + if isinstance(value, dict): + return {k: _normalize(v) for k, v in sorted(value.items())} + return value + + return json.dumps( + {name: _normalize(value) for name, value in self}, + sort_keys=True, + separators=(",", ":"), + default=str, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wren/src/wren/model/__init__.py` around lines 18 - 25, to_key_string currently only includes SecretStr and str fields so distinct connection configs that differ in booleans or nested dicts collapse; update to_key_string to serialize the entire normalized config (not just string fields) into a deterministic string (e.g., JSON with sorted keys) while unwrapping SecretStr via get_secret_value so secrets are included; reference the to_key_string method and ensure any iteration over self uses the full field values (including bools, dicts, kwargs, etc.) before serializing to produce a unique, stable cache key.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@skills/wren-usage/SKILL.md`:
- Around line 247-261: The fenced code block that lists Wren CLI commands (the
triple-backtick block containing "Get data back → wren --sql ..." through
"Switch profile → wren profile switch <name>") is missing a language tag; add a
neutral tag such as text (or bash if preferred) to the opening fence (e.g.,
change ``` to ```text) so the Markdown linter (MD040) is satisfied.
- Line 53: Replace the indented code block for the single-command snippet "wren
context instructions" with a fenced code block using triple backticks; locate
the snippet labeled "wren context instructions" in SKILL.md and surround that
single command with ``` on its own lines so the MD046 lint rule is satisfied.
In `@wren/README.md`:
- Around line 98-104: Update the Quick Start text to reflect that wren now
auto-discovers the project's target/mdl.json (not ~/.wren/mdl.json), mention the
new wren context workflow and show that users can either run wren in a project
(so it finds target/mdl.json) or explicitly pass --mdl /path/to/mdl.json (or set
WREN_HOME if that override is still supported); adjust the example command (wren
--sql ...) and the surrounding prose to instruct users to use wren context /
ensure target/mdl.json exists or include --mdl when running the command.
In `@wren/src/wren/cli.py`:
- Around line 197-207: The active-profile code paths currently instantiate
WrenEngine without passing the loaded CLI config, so settings like strict_mode
and denied_functions are skipped; update the branches that select the active
profile to call load_config(_WREN_HOME) and pass the resulting config into the
WrenEngine constructor (same as the explicit --connection-file/--datasource
branches), i.e., locate the active-profile instantiation sites and add
config=config (using load_config(_WREN_HOME) and error handling analogous to the
shown try/except) so both places (the earlier active-profile branch and the
similar block around the other occurrence) consistently wire config into
WrenEngine.
In `@wren/src/wren/context.py`:
- Around line 745-748: The file defines _VALID_LEVELS twice (duplicate
frozenset({"error", "warning", "strict"})); remove the redundant definition so
there is only a single _VALID_LEVELS constant. Locate the duplicate occurrences
of the symbol _VALID_LEVELS in this module (the identical frozenset lines) and
delete the extra line, leaving one canonical declaration.
In `@wren/src/wren/model/field_registry.py`:
- Around line 1-7: The mcp-server should explicitly declare the runtime
dependency on the package that provides wren.model.field_registry so the web UI
doesn't rely on fragile fallback data; update mcp-server/pyproject.toml to add
the appropriate dependency (the package providing module
wren.model.field_registry, e.g., wren-engine) or add a clear install-time
requirement note in mcp-server documentation, and ensure imports referenced in
mcp-server/app/web.py (which imports wren.model.field_registry) are validated at
startup to fail fast if the package is missing.
In `@wren/src/wren/profile_cli.py`:
- Around line 181-224: The interactive prompt currently ignores
FieldDef.required and silently drops unreadable file_base64 inputs, allowing
creation of an unusable profile; update the field loop in profile_cli.py (the
block iterating "for f in fields") to enforce f.required: for normal and
sensitive prompts re-prompt or error if the user provides an empty value when
f.required is True (use typer.prompt to loop or raise a clear error), and for
file_base64 inputs treat read errors or empty paths as validation failures for
required fields (either re-prompt until a readable path is provided or fail the
interactive flow with a descriptive message); ensure valid values are only added
to profile[f.name] after passing these required checks so the saved/activated
profile cannot omit required fields.
- Around line 74-83: The import guard around "from wren.profile_web import start
as web_start" is too broad; change it to catch ImportError, inspect the missing
module name or exception message and only convert it into the "install
wren-engine[ui]" message (and call typer.Exit) when the missing module is one of
the optional UI deps (e.g., "starlette", "uvicorn", "jinja2"); for any other
ImportError raised from inside wren.profile_web re-raise the exception so real
errors aren't masked.
---
Outside diff comments:
In `@wren/src/wren/model/__init__.py`:
- Around line 18-25: to_key_string currently only includes SecretStr and str
fields so distinct connection configs that differ in booleans or nested dicts
collapse; update to_key_string to serialize the entire normalized config (not
just string fields) into a deterministic string (e.g., JSON with sorted keys)
while unwrapping SecretStr via get_secret_value so secrets are included;
reference the to_key_string method and ensure any iteration over self uses the
full field values (including bools, dicts, kwargs, etc.) before serializing to
produce a unique, stable cache key.
🪄 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: a257ad22-aae9-453a-9d4e-752cce605f21
📒 Files selected for processing (13)
mcp-server/app/web.pyskills-archive/README.mdskills/wren-usage/SKILL.mdwren/README.mdwren/src/wren/cli.pywren/src/wren/config.pywren/src/wren/context.pywren/src/wren/memory/cli.pywren/src/wren/model/__init__.pywren/src/wren/model/field_registry.pywren/src/wren/profile_cli.pywren/src/wren/templates/_profile_fields.htmlwren/tests/unit/test_engine.py
✅ Files skipped from review due to trivial changes (2)
- skills-archive/README.md
- wren/src/wren/memory/cli.py
🚧 Files skipped from review as they are similar to previous changes (2)
- mcp-server/app/web.py
- wren/src/wren/config.py
Fix to_key_string() to serialize all fields via JSON (not just str/SecretStr) to prevent cache collisions on bool/dict fields. Wire load_config() into active-profile code paths so strict_mode and denied_functions are enforced. Remove duplicate _VALID_LEVELS constant. Narrow ImportError guard in profile --ui to only catch missing optional UI deps. Fix markdown lint issues in wren-usage skill. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wren/src/wren/profile_cli.py (1)
74-145:⚠️ Potential issue | 🟡 MinorReject conflicting add modes explicitly.
--ui,--from-file, and--interactiveare treated as alternative flows here, but the current branch order silently ignores whichever lower-priority flags the user also passed. Failing fast is safer than creating a profile from an unexpected path.Proposed fix
+ selected_modes = sum(bool(flag) for flag in (ui, from_file, interactive)) + if selected_modes > 1: + typer.echo( + "Error: choose only one of --ui, --from-file, or --interactive.", + err=True, + ) + raise typer.Exit(1) + if ui: try: from wren.profile_web import start as web_start # noqa: PLC0415🤖 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 74 - 145, The CLI currently treats --ui, --from-file, and --interactive as alternatives but silently ignores lower-priority flags; add an explicit mutual-exclusion check at the start of the add flow (before the if ui: branch) that detects when more than one of the booleans ui, from_file, interactive is True and immediately typer.echo a clear error like "Error: --ui, --from-file and --interactive are mutually exclusive; pass only one." and raise typer.Exit(1). Modify the top-level logic around the ui/from_file/interactive branches in profile_cli.py (the code that calls _interactive_add and web_start and reads Path) to short-circuit on this validation so the existing branches remain unchanged beyond the early reject.
♻️ Duplicate comments (1)
wren/src/wren/profile_cli.py (1)
183-226:⚠️ Potential issue | 🟠 MajorInteractive mode still ignores required fields.
FieldDef.requiredis never consulted in any branch here, and unreadablefile_base64inputs only emit a warning. That means required credentials can still be dropped fromprofilewith no immediate failure.
🤖 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 27-41: The current try/except only handles SystemExit so failures
from discover_project_path() surface as raw tracebacks; add an additional except
Exception as e block around the discover_project_path() / target check to catch
non-SystemExit errors, call typer.echo with a concise CLI-friendly message that
includes the exception text (e) and then raise typer.Exit(1); keep the existing
except SystemExit handler intact so SystemExit is still handled specially.
In `@wren/src/wren/model/__init__.py`:
- Line 13: The StrPort annotated validator currently coerces ints to strings but
treats booleans as ints (since bool subclasses int), so update the
BeforeValidator used in StrPort to only convert when the value is an actual int
(exclude booleans) — e.g., check type or ensure not isinstance(v, bool) before
applying str(v); modify the lambda in StrPort's BeforeValidator accordingly so
boolean inputs are not coerced to strings and will be rejected by port
validation.
In `@wren/src/wren/profile_cli.py`:
- Around line 86-89: The CLI always prints "Opening browser... (press Ctrl+C to
cancel)" regardless of the --no-open flag; update the code around typer.echo and
the web_start call so the message reflects the open_browser behavior: check the
no_open variable (or invert it to open_browser) and only print "Opening
browser..." when open_browser is true, otherwise print a neutral message like
"Starting UI without opening a browser..." (or omit the banner), then call
web_start(name, activate=activate, port=ui_port, open_browser=not no_open) as
before; adjust the conditional near typer.echo and reference to
no_open/open_browser and the web_start invocation to keep behavior consistent.
---
Outside diff comments:
In `@wren/src/wren/profile_cli.py`:
- Around line 74-145: The CLI currently treats --ui, --from-file, and
--interactive as alternatives but silently ignores lower-priority flags; add an
explicit mutual-exclusion check at the start of the add flow (before the if ui:
branch) that detects when more than one of the booleans ui, from_file,
interactive is True and immediately typer.echo a clear error like "Error: --ui,
--from-file and --interactive are mutually exclusive; pass only one." and raise
typer.Exit(1). Modify the top-level logic around the ui/from_file/interactive
branches in profile_cli.py (the code that calls _interactive_add and web_start
and reads Path) to short-circuit on this validation so the existing branches
remain unchanged beyond the early reject.
🪄 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: d8f78276-2685-41c0-9456-c4629947198b
📒 Files selected for processing (5)
skills/wren-usage/SKILL.mdwren/src/wren/cli.pywren/src/wren/context.pywren/src/wren/model/__init__.pywren/src/wren/profile_cli.py
✅ Files skipped from review due to trivial changes (1)
- wren/src/wren/context.py
- Catch non-SystemExit exceptions in _require_mdl() for CLI-friendly errors - Reject bool values in StrPort validator (bool is subclass of int) - Add mutual exclusion check for --ui/--from-file/--interactive flags - Suppress "Opening browser" message when --no-open is passed - Enforce FieldDef.required in interactive profile creation - Clean stale managed files on --force in write_project_files() - Fix skills-archive README "all at once" to include all skills Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
wren/src/wren/context.py (1)
248-253: Consider catching YAML parse errors for clearer user feedback.If
config.ymlcontains malformed YAML,yaml.safe_loadraises ayaml.YAMLErrorwith a traceback rather than a clean CLI error. Similar concern applies toload_project_configat line 299.♻️ Suggested improvement
def load_global_config() -> dict: """Load ~/.wren/config.yml (global preferences).""" config_file = _WREN_HOME / "config.yml" if not config_file.exists(): return {} - return yaml.safe_load(config_file.read_text()) or {} + try: + return yaml.safe_load(config_file.read_text()) or {} + except yaml.YAMLError as e: + raise SystemExit(f"Error: invalid YAML in {config_file}: {e}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wren/src/wren/context.py` around lines 248 - 253, load_global_config (and similarly load_project_config) currently calls yaml.safe_load directly and will raise a raw yaml.YAMLError on malformed YAML; catch yaml.YAMLError around yaml.safe_load, include the file path (config_file or project config path) in a clear error message, and surface it as a user-friendly CLI error (e.g., raise a ClickException or SystemExit with a concise message) so the traceback is replaced with a readable parse error; reference load_global_config and load_project_config to locate the fix.skills-archive/README.md (2)
73-80: Clarify theClawHubtags in the skills table.Lines 73 and 80 have
`ClawHub`appearing at the end of the description cells without explanation. While these two skills are indeed published on ClawHub (per lines 35-37), the inline code formatting makes it look like a formatting artifact rather than a meaningful badge or tag.Consider using a clearer notation such as:
- A dedicated column:
| ClawHub |with ✓/✗ indicators- A proper badge:
- A footnote marker:
†with explanation below the table- Or simply noting ClawHub availability in the description prose
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills-archive/README.md` around lines 73 - 80, The README table uses inline code `ClawHub` at the end of two description cells which reads like a formatting artifact; update the table to clearly indicate ClawHub publication—for example, add a dedicated "ClawHub" column with ✓/✗ and move the markers from the descriptions for the rows referencing wren-usage and wren-http-api, or replace the inline `` `ClawHub` `` with a visible badge or a footnote marker and explanatory footnote below the table; modify the table header and the rows for the entries named "wren-usage" and "wren-http-api" accordingly so the marking is consistent and unambiguous.
67-67: Relocate the tip closer to the npx skills section.The tip about using
--skill '*'would be more helpful immediately after the npx skills introduction (around line 26) rather than after the unrelated invocation examples.📋 Suggested reorganization
Move line 67 to appear right after line 27 in the "Option 2 — npx skills" section:
### Option 2 — npx skills Install all skills for Claude Code: ```bash npx skills add Canner/wren-engine --skill '*' --agent claude-codeTip: Use
--skill '*'to install all skills at once, or specify individual skills (e.g.,--skill wren-generate-mdl --skill wren-sql).
npx skillsalso supports Cursor, Windsurf, and 30+ other agent tools — replace--agent claude-codewith your agent of choice.</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@skills-archive/README.mdat line 67, Move the tip about using --skill ''
from its current location after the invocation examples to directly under the
"Option 2 — npx skills" section (immediately after the npx skills example block
that shows npx skills add Canner/wren-engine --skill '' --agent claude-code);
specifically relocate the line starting with "> Tip: Use--skill '*'..."
so it appears right after the npx example and before the sentence "npx skills
also supports Cursor, Windsurf...", keeping the exact tip text intact.</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.
Inline comments:
In@skills-archive/README.md:
- Line 20: Update the README to correct the namespace example and clarify
invocation syntax: change the incorrect/wren:wren-sqlexample to/wren:sql,
and explicitly state that the colon form (e.g.,/wren:generate-mdl,
/wren:sql) is the Claude Code Plugin invocation while the hyphen form (e.g.,
/wren-usage,/wren-quickstart,/wren-sql) is used for the npx skills
option; alternatively, pick one consistent invocation pattern (colon or hyphen)
and update all examples and headings (references: the examples
/wren:generate-mdl,/wren:sql,/wren-usage,/wren-quickstart,
/wren-sql) so the documentation no longer contradicts itself.In
@wren/src/wren/context.py:
- Around line 115-137: The loop that processes models accesses
model_snake["name"] directly (in the models loop) which can raise KeyError for
malformed MDL; modify the code in the models processing block to validate that
"name" exists on model_snake (e.g., if "name" not in model_snake: raise
ValueError with a clear message including the model content or index) before
using it, and apply the same validation to the views processing block (the
analogous view_snake["name"] access) so both cases produce a descriptive error
instead of an unhandled KeyError.
Nitpick comments:
In@skills-archive/README.md:
- Around line 73-80: The README table uses inline code
ClawHubat the end of
two description cells which reads like a formatting artifact; update the table
to clearly indicate ClawHub publication—for example, add a dedicated "ClawHub"
column with ✓/✗ and move the markers from the descriptions for the rows
referencing wren-usage and wren-http-api, or replace the inline`ClawHub`
with a visible badge or a footnote marker and explanatory footnote below the
table; modify the table header and the rows for the entries named "wren-usage"
and "wren-http-api" accordingly so the marking is consistent and unambiguous.- Line 67: Move the tip about using --skill '' from its current location after
the invocation examples to directly under the "Option 2 — npx skills" section
(immediately after the npx skills example block that shows npx skills add
Canner/wren-engine --skill '' --agent claude-code); specifically relocate the
line starting with "> Tip: Use--skill '*'..." so it appears right after
the npx example and before the sentence "npx skills also supports Cursor,
Windsurf...", keeping the exact tip text intact.In
@wren/src/wren/context.py:
- Around line 248-253: load_global_config (and similarly load_project_config)
currently calls yaml.safe_load directly and will raise a raw yaml.YAMLError on
malformed YAML; catch yaml.YAMLError around yaml.safe_load, include the file
path (config_file or project config path) in a clear error message, and surface
it as a user-friendly CLI error (e.g., raise a ClickException or SystemExit with
a concise message) so the traceback is replaced with a readable parse error;
reference load_global_config and load_project_config to locate the fix.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `aceb0646-888b-460f-9f1e-8e1a73df801a` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 6f4e9f9fdf66db287c81c3305b367db41ad95534 and d949e4e0a97e8abf28b17fe735c398736b5759ef. </details> <details> <summary>📒 Files selected for processing (5)</summary> * `skills-archive/README.md` * `wren/src/wren/cli.py` * `wren/src/wren/context.py` * `wren/src/wren/model/__init__.py` * `wren/src/wren/profile_cli.py` </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (1)</summary> * wren/src/wren/cli.py </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
…ce example Add explicit name-field validation in convert_mdl_to_project() for both models and views to produce clear errors instead of KeyError. Fix /wren:wren-sql typo to /wren:sql in skills-archive README. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Update README quick start to use `wren context init` + YAML models + `wren context build` instead of manually creating ~/.wren/mdl.json. Reflects the 0.2.0 CLI flow where wren auto-discovers target/mdl.json from the project directory. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
douenergy
approved these changes
Apr 7, 2026
This was referenced Apr 7, 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
wren contextcommand group for YAML-based MDL project management (init,validate,init --from-mdl)wren profilefor named connection profiles with browser-based UI (profile add --ui)wren docs connection-infoCLI command andwren utilswith type_mapping modulewren-usageandwren-generate-mdlto profile-based CLI workflowHighlights
Test plan
just testinwren/passesjust lintinwren/passesjust test-memory)wren context init,wren profile add,wren --sql🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Changes