feat(wren): add context command group for YAML-based MDL project management#1511
Merged
goldmedal merged 11 commits intofeat/cli-0.2.0from Apr 4, 2026
Merged
feat(wren): add context command group for YAML-based MDL project management#1511goldmedal merged 11 commits intofeat/cli-0.2.0from
goldmedal merged 11 commits intofeat/cli-0.2.0from
Conversation
github-actions
bot
added
the
dependencies
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a Wren project context subsystem: project discovery, YAML/SQL MDL loaders for schema v1/v2, validation, manifest assembly and JSON output, a Typer Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/CLI
participant MainCLI as Main CLI
participant ContextApp as context_app
participant Discover as discover_project_path()
participant Validator as validate_project()
participant Loader as build_json()/load_*
participant FS as Filesystem
User->>MainCLI: wren context build
MainCLI->>ContextApp: dispatch build
ContextApp->>Discover: discover_project_path()
Discover->>FS: search for `wren_project.yml` (cwd → parents → WREN_PROJECT_HOME/config)
FS-->>Discover: project_path
ContextApp->>Validator: validate_project(project_path)
Validator->>FS: read wren_project.yml, models/, views/, relationships.yml, instructions.md
FS-->>Validator: YAML/SQL contents
Validator-->>ContextApp: errors/warnings
ContextApp->>Loader: build_json(project_path)
Loader->>FS: load metadata and external SQL files
FS-->>Loader: file contents
Loader->>Loader: assemble manifest, convert snake_case→camelCase
Loader->>FS: write target/mdl.json (or --output)
FS-->>ContextApp: write confirmation
ContextApp-->>User: report counts and output path
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: 1
🧹 Nitpick comments (3)
wren/src/wren/cli.py (1)
35-47: Consider reusingdiscover_project_pathfromwren.contextto avoid duplication.This logic duplicates the upward-search algorithm in
wren/src/wren/context.py:40-57(discover_project_path). Both walk up from cwd using[current, *current.parents], check forwren_project.yml, and stop at home or root.To maintain consistency and reduce maintenance burden:
♻️ Proposed refactor
+from wren.context import discover_project_path # noqa: PLC0415 + def _require_mdl(mdl: str | None) -> str: ... if _DEFAULT_MDL.exists(): return str(_DEFAULT_MDL) - # Walk up from cwd looking for wren_project.yml → target/mdl.json - current = Path.cwd() - for parent in [current, *current.parents]: - target = parent / "target" / "mdl.json" - if (parent / "wren_project.yml").exists() and target.exists(): - return str(target) - if parent == Path.home() or parent == parent.parent: - break + # Try project discovery → target/mdl.json + project_path = discover_project_path() + target = project_path / "target" / "mdl.json" + if target.exists(): + return str(target) # Global default project ...🤖 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 35 - 47, The current upward-search logic in wren/cli.py duplicates discover_project_path from wren.context; update cli.py to import and call discover_project_path() (from wren.context) to obtain the project path instead of reimplementing the loop, then translate its returned path to the target mdl.json (or fall back to the existing global default _WREN_HOME/project/target/mdl.json if discover_project_path returns None); remove the duplicated loop and keep behavior identical (stop at home/root and prefer local project over global) while referencing discover_project_path to locate the project root.wren/src/wren/context_cli.py (1)
164-170: Minor: Redundant entity loading when validation passes.When validation succeeds,
load_models,load_views, andload_relationshipsare called again here, but they were already called insidevalidate_project(). For a validation command this is acceptable overhead, but if performance becomes a concern, consider havingvalidate_projectreturn the loaded entities.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wren/src/wren/context_cli.py` around lines 164 - 170, The validation code is reloading models/views/relationships after validate_project() already loaded them; modify validate_project to return the loaded entities (models, views, relationships) or change the caller to accept and use those returned values instead of calling load_models, load_views, and load_relationships again; update the call site in context_cli.py where validate_project() is invoked so it captures (models, views, rels, errors) or similar, and use those variables for the typer.echo message instead of calling the loader functions again.wren/src/wren/memory/cli.py (1)
150-151: Broad exception swallowing hides potential issues.
except Exception: passsilently swallows all exceptions, including unexpected ones likePermissionError,MemoryError, or bugs in the context module. Consider logging at debug level to aid troubleshooting:🔧 Proposed improvement
+ from loguru import logger # noqa: PLC0415 + project_path = discover_project_path() instructions = load_instructions(project_path) if instructions: manifest["_instructions"] = instructions - except Exception: - pass # instructions are optional; never fail index because of them + except Exception as e: + logger.debug(f"Could not load instructions: {e}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wren/src/wren/memory/cli.py` around lines 150 - 151, The bare "except Exception: pass" in wren/src/wren/memory/cli.py silently swallows all errors; replace it with a guarded handler that logs the exception at debug level and only swallows expected/benign errors while re-raising critical ones: change the block around the "except Exception: pass" to "except Exception as e:" then use the module logger (e.g. logger = logging.getLogger(__name__)) to call logger.debug("...%s", e, exc_info=True), and re-raise for fatal exceptions such as MemoryError or PermissionError (e.g. if isinstance(e, (MemoryError, PermissionError)): raise) so unexpected bugs are not hidden.
🤖 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/memory/cli.py`:
- Around line 139-151: The manifest injection of "_instructions" causes
manifest_hash() used by extract_schema_items() -> index_schema() to change on
instruction edits, triggering unnecessary re-indexing; remove or avoid adding
"_instructions" to the manifest that is passed into index_schema() (or
explicitly strip manifest["_instructions"] before calling
extract_schema_items()/index_schema()) and change the broad except Exception:
pass in the CLI block that loads instructions to catch only expected errors
(e.g., FileNotFoundError, ImportError, ModuleNotFoundError) so we don't silently
swallow programming errors; update the code around the manifest population in
the CLI (where "_instructions" is set) and the call sites using
extract_schema_items()/index_schema()/manifest_hash() accordingly.
---
Nitpick comments:
In `@wren/src/wren/cli.py`:
- Around line 35-47: The current upward-search logic in wren/cli.py duplicates
discover_project_path from wren.context; update cli.py to import and call
discover_project_path() (from wren.context) to obtain the project path instead
of reimplementing the loop, then translate its returned path to the target
mdl.json (or fall back to the existing global default
_WREN_HOME/project/target/mdl.json if discover_project_path returns None);
remove the duplicated loop and keep behavior identical (stop at home/root and
prefer local project over global) while referencing discover_project_path to
locate the project root.
In `@wren/src/wren/context_cli.py`:
- Around line 164-170: The validation code is reloading
models/views/relationships after validate_project() already loaded them; modify
validate_project to return the loaded entities (models, views, relationships) or
change the caller to accept and use those returned values instead of calling
load_models, load_views, and load_relationships again; update the call site in
context_cli.py where validate_project() is invoked so it captures (models,
views, rels, errors) or similar, and use those variables for the typer.echo
message instead of calling the loader functions again.
In `@wren/src/wren/memory/cli.py`:
- Around line 150-151: The bare "except Exception: pass" in
wren/src/wren/memory/cli.py silently swallows all errors; replace it with a
guarded handler that logs the exception at debug level and only swallows
expected/benign errors while re-raising critical ones: change the block around
the "except Exception: pass" to "except Exception as e:" then use the module
logger (e.g. logger = logging.getLogger(__name__)) to call logger.debug("...%s",
e, exc_info=True), and re-raise for fatal exceptions such as MemoryError or
PermissionError (e.g. if isinstance(e, (MemoryError, PermissionError)): raise)
so unexpected bugs are not hidden.
🪄 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: fc4423b8-6bde-4454-96a6-04428a5a7f24
⛔ Files ignored due to path filters (1)
wren/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
wren/pyproject.tomlwren/src/wren/cli.pywren/src/wren/context.pywren/src/wren/context_cli.pywren/src/wren/memory/cli.pywren/tests/unit/test_context.pywren/tests/unit/test_context_cli.py
goldmedal
added a commit
that referenced
this pull request
Apr 2, 2026
…scope - Strip `_`-prefixed keys from manifest before passing to index_schema() so instruction changes don't invalidate the schema hash and trigger unnecessary full re-indexing of models/views/relationships - Narrow `except Exception` to FileNotFoundError/ImportError/ModuleNotFoundError to avoid silently swallowing programming errors Addresses CodeRabbit review on #1511. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wren/src/wren/memory/cli.py`:
- Around line 154-157: The CLI currently strips keys starting with "_"
unconditionally before calling store.index_schema, so the --instructions flag
(which loads _instructions into manifest) has no effect; update the manifest
filtering logic in the CLI (the index_manifest creation before
store.index_schema) to conditionally retain the "_instructions" key when the
--instructions flag is enabled (or remove the flag entirely if you choose to
disable instruction indexing). Locate the variables/operations manifest,
index_manifest, and the call store.index_schema(...) and change the filter to
check the flag (e.g., include "_instructions" when instructions flag is true) so
instruction content is indexed when requested.
- Around line 139-151: The manifest population block that calls
discover_project_path() and load_instructions() currently only catches
FileNotFoundError, ImportError, and ModuleNotFoundError, so read failures from
load_instructions() (which uses read_text()) like PermissionError,
IsADirectoryError, and UnicodeDecodeError can propagate and abort the index
command; update the except clause in the include_instructions branch (the block
referencing discover_project_path and load_instructions) to also catch
PermissionError, IsADirectoryError, and UnicodeDecodeError (or replace with a
broader OSError/Exception group if you prefer) and silently ignore them so
unreadable/invalid instructions.md never fail the index.
🪄 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: da404e4b-9782-4408-8bea-1139cf10b61e
📒 Files selected for processing (1)
wren/src/wren/memory/cli.py
goldmedal
added a commit
that referenced
this pull request
Apr 2, 2026
…ctions - manifest_hash() now excludes _-prefixed keys so instruction changes don't invalidate the schema cache and trigger unnecessary re-indexing - Remove the _-key stripping before index_schema() — instructions are now properly passed through and indexed when --instructions is enabled - Broaden exception handler to also catch PermissionError, IsADirectoryError, and UnicodeDecodeError from read_text() on instructions.md Addresses second CodeRabbit review on #1511. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
goldmedal
added a commit
that referenced
this pull request
Apr 2, 2026
…scope - Strip `_`-prefixed keys from manifest before passing to index_schema() so instruction changes don't invalidate the schema hash and trigger unnecessary full re-indexing of models/views/relationships - Narrow `except Exception` to FileNotFoundError/ImportError/ModuleNotFoundError to avoid silently swallowing programming errors Addresses CodeRabbit review on #1511. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
goldmedal
added a commit
that referenced
this pull request
Apr 2, 2026
…ctions - manifest_hash() now excludes _-prefixed keys so instruction changes don't invalidate the schema cache and trigger unnecessary re-indexing - Remove the _-key stripping before index_schema() — instructions are now properly passed through and indexed when --instructions is enabled - Broaden exception handler to also catch PermissionError, IsADirectoryError, and UnicodeDecodeError from read_text() on instructions.md Addresses second CodeRabbit review on #1511. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wren/src/wren/context.py`:
- Around line 386-423: The validator assumes model["columns"] is a list and each
column is a dict, causing AttributeError on malformed YAML; update the block
that handles columns in wren/src/wren/context.py to (1) check
isinstance(columns, list) and append a ValidationError on model path if not, (2)
when iterating ensure each col is a dict (append ValidationError at f"{src_path}
> {name} > columns[{j}]" if not) before calling col.get, (3) validate that
col_name is a non-empty string before using it (emit "column missing 'name'" and
skip duplicate checks), and (4) only perform duplicate checks and type checks
(for col.get("type")) after these type validations so ValidationError (class
ValidationError, variables src_path/name/col_names) is used rather than letting
AttributeError abort validation.
- Around line 71-75: The code currently calls int(config.get("schema_version",
1)) in get_schema_version and again at the other read site (lines ~323-324),
which raises ValueError on non-numeric YAML values; wrap the int() conversion in
a try/except to catch ValueError, then handle it by reporting a clear validation
error (e.g., raise a ConfigValidationError or a user-facing exception with a
message like "invalid schema_version in wren_project.yml: must be an integer")
or fallback to the default 1 while logging a validation warning; apply the same
guarded conversion and error handling to both get_schema_version and the other
schema_version read site so invalid values produce a controlled validation error
instead of crashing.
🪄 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: 2af155b0-3a9f-472e-aed4-05a3eccde2b4
⛔ Files ignored due to path filters (1)
wren/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
wren/pyproject.tomlwren/src/wren/cli.pywren/src/wren/context.pywren/src/wren/context_cli.pywren/src/wren/memory/cli.pywren/src/wren/memory/schema_indexer.pywren/tests/unit/test_context.pywren/tests/unit/test_context_cli.py
✅ Files skipped from review due to trivial changes (2)
- wren/pyproject.toml
- wren/tests/unit/test_context.py
🚧 Files skipped from review as they are similar to previous changes (3)
- wren/src/wren/memory/schema_indexer.py
- wren/src/wren/cli.py
- wren/src/wren/context_cli.py
goldmedal
added a commit
that referenced
this pull request
Apr 2, 2026
…ntext - get_schema_version() now raises a clear SystemExit on non-integer schema_version values instead of crashing with ValueError - validate_project() reports a validation error instead of crashing when schema_version is non-integer - validate_project() adds type guards: columns must be a list, each column and relationship entry must be a dict — malformed YAML reports errors instead of raising AttributeError Addresses CodeRabbit review on #1511. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
wren/src/wren/context.py (1)
496-528:⚠️ Potential issue | 🟡 MinorMissing type check for
ref_modelscan cause incorrect validation on malformed YAML.If
modelsin a relationship is a scalar (e.g.,models: "Customer"instead ofmodels: ["Customer"]), iterating over it will yield individual characters, generating spurious "references unknown model" errors for each character.🛡️ Proposed fix to validate ref_models is a list
rel_name = rel.get("name", f"relationships[{i}]") ref_models = rel.get("models", []) + if not isinstance(ref_models, list): + errors.append( + ValidationError( + "error", + f"relationships > {rel_name}", + "'models' must be a list", + ) + ) + ref_models = [] for m in ref_models:🤖 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 496 - 528, The loop that validates each relationship should guard against non-list `models` to avoid iterating scalars (e.g., strings) and producing spurious errors; inside the relationships loop where `rel`, `ref_models`, `rel_name`, `errors`, and `ValidationError` are used, add a type check like `if not isinstance(ref_models, list):` and append a ValidationError (severity "error" or "warning") for `f"relationships > {rel_name}"` with a message such as "models must be a list", then skip iterating that `ref_models` (continue) so only proper list values are checked against `all_entity_names`.
🧹 Nitpick comments (1)
wren/src/wren/context.py (1)
230-259: In-place mutation of loaded models/views may cause subtle bugs.Lines 243-246 modify the original
modelsandviewslists returned by the loaders by callingm.pop("_source_dir", None). If any caller retrieves models/views separately and then callsbuild_manifest(), the_source_dirmetadata will be unexpectedly removed from those previously-loaded objects.Consider working on copies to avoid side effects:
♻️ Suggested fix using shallow copies
- models = load_models(project_path) - views = load_views(project_path) + models = [dict(m) for m in load_models(project_path)] + views = [dict(v) for v in load_views(project_path)]🤖 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 230 - 259, build_manifest is mutating the dicts returned by load_models/load_views by popping "_source_dir", causing side effects for callers; instead, construct new lists of shallow copies without the internal key (e.g., iterate over models and views, copy each dict and remove "_source_dir" from the copy) and use those copies when building manifest so the original returned objects remain unchanged; update references to the new local variables (e.g., copied_models, copied_views) when constructing the manifest.
🤖 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/context.py`:
- Around line 496-528: The loop that validates each relationship should guard
against non-list `models` to avoid iterating scalars (e.g., strings) and
producing spurious errors; inside the relationships loop where `rel`,
`ref_models`, `rel_name`, `errors`, and `ValidationError` are used, add a type
check like `if not isinstance(ref_models, list):` and append a ValidationError
(severity "error" or "warning") for `f"relationships > {rel_name}"` with a
message such as "models must be a list", then skip iterating that `ref_models`
(continue) so only proper list values are checked against `all_entity_names`.
---
Nitpick comments:
In `@wren/src/wren/context.py`:
- Around line 230-259: build_manifest is mutating the dicts returned by
load_models/load_views by popping "_source_dir", causing side effects for
callers; instead, construct new lists of shallow copies without the internal key
(e.g., iterate over models and views, copy each dict and remove "_source_dir"
from the copy) and use those copies when building manifest so the original
returned objects remain unchanged; update references to the new local variables
(e.g., copied_models, copied_views) when constructing the manifest.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
wren/src/wren/context.py (1)
522-531:⚠️ Potential issue | 🟡 MinorAdd type guard for
ref_modelsto prevent crashes on malformed YAML.Similar to other type guards added in this validation function,
ref_modelsshould be validated as a list before iteration. If a user writesmodels: orders(string) instead ofmodels: [orders](list), the loop will raiseTypeError.🛡️ Proposed fix
rel_name = rel.get("name", f"relationships[{i}]") ref_models = rel.get("models", []) + if not isinstance(ref_models, list): + errors.append( + ValidationError( + "error", + f"relationships > {rel_name}", + "'models' must be a list", + ) + ) + ref_models = [] for m in ref_models: if m not in all_entity_names:🤖 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 522 - 531, The loop assumes ref_models is iterable/list and will crash on malformed YAML; add a type guard after ref_models = rel.get("models", []) to check isinstance(ref_models, list) and if not, append a ValidationError (using the same ValidationError constructor and context like "relationships > {rel_name}") to errors, then continue/skip iterating; keep the rest of the logic (checking each m against all_entity_names and appending errors) unchanged so only valid lists are iterated.
🧹 Nitpick comments (1)
cli-skills/wren-usage/SKILL.md (1)
33-39: LGTM on the new project-local path and instructions step.The documentation correctly reflects the new
target/mdl.jsonlocation and thewren context instructionsworkflow step. The indented code block style is consistent with the project's approach to avoid security scanner false positives.However, note that Line 78 still references
ls ~/.wren/mdl.jsonfor the "table not found" error recovery workflow. Consider updating it to align with the project-based discovery or clarifying that both paths are valid fallbacks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli-skills/wren-usage/SKILL.md` around lines 33 - 39, The doc still references the legacy fallback path "ls ~/.wren/mdl.json" in the "table not found" recovery workflow; update that reference to either use the new project-local path "target/mdl.json" or explicitly document both as valid fallbacks (e.g., show "target/mdl.json" first and then mention "~/.wren/mdl.json" as a legacy fallback), ensuring the example command and explanatory text match the new discovery behavior and the earlier instruction to run "wren context instructions".
🤖 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/context.py`:
- Around line 522-531: The loop assumes ref_models is iterable/list and will
crash on malformed YAML; add a type guard after ref_models = rel.get("models",
[]) to check isinstance(ref_models, list) and if not, append a ValidationError
(using the same ValidationError constructor and context like "relationships >
{rel_name}") to errors, then continue/skip iterating; keep the rest of the logic
(checking each m against all_entity_names and appending errors) unchanged so
only valid lists are iterated.
---
Nitpick comments:
In `@cli-skills/wren-usage/SKILL.md`:
- Around line 33-39: The doc still references the legacy fallback path "ls
~/.wren/mdl.json" in the "table not found" recovery workflow; update that
reference to either use the new project-local path "target/mdl.json" or
explicitly document both as valid fallbacks (e.g., show "target/mdl.json" first
and then mention "~/.wren/mdl.json" as a legacy fallback), ensuring the example
command and explanatory text match the new discovery behavior and the earlier
instruction to run "wren context instructions".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7ff909d9-64d0-426f-bbaf-100ab6fc6d68
📒 Files selected for processing (13)
cli-skills/wren-usage/SKILL.mdwren/src/wren/cli.pywren/src/wren/context.pywren/src/wren/context_cli.pywren/src/wren/memory/cli.pywren/tests/connectors/test_duckdb.pywren/tests/connectors/test_postgres.pywren/tests/suite/query.pywren/tests/unit/test_context.pywren/tests/unit/test_context_cli.pywren/tests/unit/test_cte_rewriter.pywren/tests/unit/test_engine.pywren/tests/unit/test_memory.py
💤 Files with no reviewable changes (1)
- wren/tests/unit/test_engine.py
✅ Files skipped from review due to trivial changes (6)
- wren/tests/unit/test_memory.py
- wren/tests/connectors/test_duckdb.py
- wren/tests/connectors/test_postgres.py
- wren/tests/suite/query.py
- wren/tests/unit/test_cte_rewriter.py
- wren/src/wren/context_cli.py
🚧 Files skipped from review as they are similar to previous changes (2)
- wren/src/wren/cli.py
- wren/src/wren/memory/cli.py
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
wren/src/wren/context.py (1)
521-531:⚠️ Potential issue | 🟡 MinorAdd type guard for
ref_modelsto prevent iteration over non-list values.If
rel.get("models")returns a non-list value (e.g., a string due to malformed YAML likemodels: ordersinstead ofmodels: [orders]), thefor m in ref_modelsloop will iterate over individual characters, producing confusing validation errors.🛡️ Proposed fix
rel_name = rel.get("name", f"relationships[{i}]") ref_models = rel.get("models", []) + if not isinstance(ref_models, list): + errors.append( + ValidationError( + "error", + f"relationships > {rel_name}", + "'models' must be a list", + ) + ) + ref_models = [] for m in ref_models: if m not in all_entity_names:🤖 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 521 - 531, The loop iterates ref_models without checking its type, so non-list values (e.g., a string) get iterated character-by-character; add a type guard before the loop: check if ref_models is an instance of list (use isinstance(ref_models, list)); if not, append a ValidationError referencing "relationships > {rel_name}" with a clear message like "models must be a list" and skip the for m in ref_models loop (continue) so only actual lists are iterated; keep using the existing variables rel_name, ref_models and all_entity_names to locate and validate entries.
🤖 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/context.py`:
- Around line 521-531: The loop iterates ref_models without checking its type,
so non-list values (e.g., a string) get iterated character-by-character; add a
type guard before the loop: check if ref_models is an instance of list (use
isinstance(ref_models, list)); if not, append a ValidationError referencing
"relationships > {rel_name}" with a clear message like "models must be a list"
and skip the for m in ref_models loop (continue) so only actual lists are
iterated; keep using the existing variables rel_name, ref_models and
all_entity_names to locate and validate entries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6a1a3499-1ee8-4ebf-93f6-5c5fa11051a2
📒 Files selected for processing (14)
cli-skills/wren-usage/SKILL.mdwren/docs/wren_project.mdwren/src/wren/cli.pywren/src/wren/context.pywren/src/wren/context_cli.pywren/src/wren/memory/cli.pywren/tests/connectors/test_duckdb.pywren/tests/connectors/test_postgres.pywren/tests/suite/query.pywren/tests/unit/test_context.pywren/tests/unit/test_context_cli.pywren/tests/unit/test_cte_rewriter.pywren/tests/unit/test_engine.pywren/tests/unit/test_memory.py
💤 Files with no reviewable changes (1)
- wren/tests/unit/test_engine.py
✅ Files skipped from review due to trivial changes (7)
- wren/tests/unit/test_memory.py
- wren/tests/suite/query.py
- wren/tests/connectors/test_postgres.py
- wren/tests/unit/test_cte_rewriter.py
- wren/tests/connectors/test_duckdb.py
- wren/docs/wren_project.md
- wren/tests/unit/test_context.py
goldmedal
added a commit
to goldmedal/wren-engine
that referenced
this pull request
Apr 4, 2026
…scope - Strip `_`-prefixed keys from manifest before passing to index_schema() so instruction changes don't invalidate the schema hash and trigger unnecessary full re-indexing of models/views/relationships - Narrow `except Exception` to FileNotFoundError/ImportError/ModuleNotFoundError to avoid silently swallowing programming errors Addresses CodeRabbit review on Canner#1511. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
goldmedal
added a commit
to goldmedal/wren-engine
that referenced
this pull request
Apr 4, 2026
…ctions - manifest_hash() now excludes _-prefixed keys so instruction changes don't invalidate the schema cache and trigger unnecessary re-indexing - Remove the _-key stripping before index_schema() — instructions are now properly passed through and indexed when --instructions is enabled - Broaden exception handler to also catch PermissionError, IsADirectoryError, and UnicodeDecodeError from read_text() on instructions.md Addresses second CodeRabbit review on Canner#1511. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
goldmedal
added a commit
to goldmedal/wren-engine
that referenced
this pull request
Apr 4, 2026
…ntext - get_schema_version() now raises a clear SystemExit on non-integer schema_version values instead of crashing with ValueError - validate_project() reports a validation error instead of crashing when schema_version is non-integer - validate_project() adds type guards: columns must be a list, each column and relationship entry must be a dict — malformed YAML reports errors instead of raising AttributeError Addresses CodeRabbit review on Canner#1511. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
goldmedal
force-pushed
the
feat/cli-0.2.0
branch
from
d7e9834 to
bc78712
Compare
…gement Adds `wren context` subcommands (init/validate/build/show) that manage MDL definitions as a structured project directory. Models and views are authored in snake_case YAML and compiled to camelCase JSON via `wren context build`. - context.py: loaders, snake→camel conversion, validation (8 checks), build - context_cli.py: Typer sub-app wired into main CLI - _require_mdl() now auto-discovers target/mdl.json from wren_project.yml walk-up - memory index auto-loads instructions.md from nearest project - 52 unit + CLI tests, pyyaml>=6.0 added as dependency Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…scope - Strip `_`-prefixed keys from manifest before passing to index_schema() so instruction changes don't invalidate the schema hash and trigger unnecessary full re-indexing of models/views/relationships - Narrow `except Exception` to FileNotFoundError/ImportError/ModuleNotFoundError to avoid silently swallowing programming errors Addresses CodeRabbit review on #1511. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ctions - manifest_hash() now excludes _-prefixed keys so instruction changes don't invalidate the schema cache and trigger unnecessary re-indexing - Remove the _-key stripping before index_schema() — instructions are now properly passed through and indexed when --instructions is enabled - Broaden exception handler to also catch PermissionError, IsADirectoryError, and UnicodeDecodeError from read_text() on instructions.md Addresses second CodeRabbit review on #1511. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ntext - get_schema_version() now raises a clear SystemExit on non-integer schema_version values instead of crashing with ValueError - validate_project() reports a validation error instead of crashing when schema_version is non-integer - validate_project() adds type guards: columns must be a list, each column and relationship entry must be a dict — malformed YAML reports errors instead of raising AttributeError Addresses CodeRabbit review on #1511. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…L, add context instructions subcommand - Remove _instructions from build_manifest()/build_json() — instructions are agent-layer only - Rewrite _require_mdl() to use project discovery (no more ~/.wren/mdl.json fallback) - Add WREN_PROJECT_HOME env var and ~/.wren/config.yml default_project to discover_project_path() - Memory path defaults to <project>/.wren/memory/ instead of global ~/.wren/memory/ - Add wren context instructions subcommand for LLM instruction consumption - Rewrite docs/wren_project.md with project concept, discovery, lifecycle, instructions - Update SKILL.md Step 1 to run wren context instructions once per session - 59 unit tests, all passing Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
docs/wren_project.md is for the MCP server workflow. The CLI-focused project documentation belongs in wren/docs/wren_project.md. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Remove stale reference to ~/.wren/mdl.json default. The CLI now discovers MDL from the project's target/mdl.json. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Covers project structure, discovery priority, lifecycle, project vs global scope, and how agents consume instructions. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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 contextsubcommands (init,validate,build,show) for managing MDL definitions as a structured YAML project directorywren context build → target/mdl.jsonmodels/<name>/metadata.yml,views/<name>/metadata.yml)_require_mdl()now auto-discoverstarget/mdl.jsonby walking up from cwd before falling back to~/.wren/project/target/mdl.jsonwren memory indexauto-loadsinstructions.mdfrom nearest project directoryFiles changed
wren/src/wren/context.pywren/src/wren/context_cli.pycontextTyper sub-app (init/validate/build/show)wren/src/wren/cli.py_require_mdl()fallback chainwren/src/wren/memory/cli.pyindexauto-loads instructions from project dirwren/pyproject.tomlpyyaml>=6.0dependencywren/tests/unit/test_context.pywren/tests/unit/test_context_cli.pyTest plan
uv run python -m pytest tests/unit/test_context.py tests/unit/test_context_cli.py)wren context init && wren context validate && wren context build🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
Chores