feat(wren): add context validate with view dry-plan and description checks#1515
feat(wren): add context validate with view dry-plan and description checks#1515goldmedal merged 4 commits intofeat/cli-0.2.0from
Conversation
|
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 MDL manifest validation: new Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI as Root CLI / context_cli.validate()
participant Context as wren.context.validate()
participant Engine as WrenEngine
participant DS as DataSource
User->>CLI: Invoke `wren context validate` (--mdl, --datasource, --level)
CLI->>CLI: Load/parse MDL manifest (file / base64)
CLI->>CLI: Resolve datasource / connection file
CLI->>DS: Construct/coerce DataSource
CLI->>Context: Call validate(manifest_str, ds, level)
Context->>Context: Decode base64 JSON manifest
Context->>Engine: Instantiate WrenEngine with manifest & datasource
loop per view
Context->>Engine: dry_plan(view.statement)
Engine-->>Context: plan result or raise error
end
Context->>Context: Accumulate errors & warnings (incl. descriptions)
Context-->>CLI: Return {errors, warnings}
CLI->>User: Print results and exit (code 0 or 1)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 |
c037e7d to
5861c2d
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
wren/tests/unit/test_context.py (1)
78-199: Add regression tests for invalid-input contract paths.Given
validate()is a library entry point, it would help to add tests for invalid datasource strings and invalidlevelvalues to guarantee structurederrorsoutput instead of exceptions.🤖 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 78 - 199, Add two regression tests that call the library entrypoint validate() with invalid inputs: one passing an invalid datasource string (e.g., "not-a-datasource") and one passing an invalid level value (e.g., "nope"); name them test_validate_invalid_datasource and test_validate_invalid_level. Each test should call validate(_b64(manifest), <invalid_value>) using the existing manifest fixtures (e.g., _BASE_MANIFEST or _MODEL_WITHOUT_DESC), assert that validate returns a structured result (no exception) and that result["errors"] is non-empty and contains a clear error token like "invalid_datasource" or "invalid_level" (or other consistent error text your validate() produces). Ensure tests live alongside the other tests in the same test module and follow the same pattern (use DataSource.duckdb where appropriate for the valid case to mirror existing tests).wren/src/wren/context_cli.py (1)
47-52: Consider moving shared CLI helpers into a dedicated module.Importing private functions from
wren.clicouples two command modules tightly. A small shared utility module would reduce fragility and keep command registration concerns separate from helper logic.🤖 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 47 - 52, The imports of private helpers (_load_conn, _load_manifest, _require_mdl, _resolve_datasource) from wren.cli should be relocated to a dedicated shared helper module to avoid coupling; create a new module (e.g., wren.cli_helpers) that exports these functions (or thin wrappers) and update context_cli.py to import them from wren.cli_helpers instead of wren.cli; also update other CLI modules that use these symbols to import from the new module and add tests or an __init__ export to ensure public API stability.
🤖 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_cli.py`:
- Line 63: The code currently calls _load_manifest(_require_mdl(mdl)) which can
attempt a base64 decode on a non-existent path and produce confusing errors;
update the input validation so that before decoding you explicitly check that
the provided --mdl path exists and is a file (use pathlib.Path(mdl).exists() /
is_file() or os.path.exists) and raise/return a clear error (FileNotFoundError
or a CLI-friendly argparse/Click error) from _require_mdl or at the call site in
context_cli.py so users see "file not found" instead of a decode failure.
In `@wren/src/wren/context.py`:
- Around line 21-22: The f-strings that build guard/warning messages access
model['name'] directly which can raise KeyError for malformed manifests; update
those messages in wren/src/wren/context.py to use model.get("name", "<unknown>")
instead of model['name'] (apply to the occurrences around the current f-strings
and the other instances mentioned), so validation remains resilient and the logs
show "<unknown>" when name is missing.
- Around line 45-46: Validate the level parameter in the functions in
wren/src/wren/context.py that declare level: str = "warning" -> dict: check
against an explicit allowed set (e.g.
{"debug","info","warning","error","critical"}), and if the value is not in that
set return a clear error dict (for example {"error":"invalid level","allowed":
[...]} ) instead of silently proceeding; update both occurrences (the function
with the signature containing level: str = "warning" and the other similar
function referenced in the review) to perform this validation at the start and
return the error dict immediately when invalid.
- Around line 81-83: The check for empty view statements currently treats only
exactly empty strings as empty; update the logic that reads stmt =
view.get("statement", "") in context.py (the variable stmt and list errors) to
trim whitespace (e.g., call strip() on stmt) before testing, so whitespace-only
statements are classified as empty and trigger errors.append(f"View '{name}':
empty statement").
- Around line 74-75: Wrap the DataSource(...) construction in a try/except so
invalid datasource strings don't raise out of the function; when
isinstance(data_source, str) and you call DataSource(data_source), catch the
exception (ValueError/TypeError or general Exception), convert it into the
function's standard validation result (e.g., append/return a validation error
message or ValidationResult indicating invalid datasource) and continue flow
instead of re-raising; reference the DataSource constructor and the local
variable data_source so the change is applied where that conversion occurs.
---
Nitpick comments:
In `@wren/src/wren/context_cli.py`:
- Around line 47-52: The imports of private helpers (_load_conn, _load_manifest,
_require_mdl, _resolve_datasource) from wren.cli should be relocated to a
dedicated shared helper module to avoid coupling; create a new module (e.g.,
wren.cli_helpers) that exports these functions (or thin wrappers) and update
context_cli.py to import them from wren.cli_helpers instead of wren.cli; also
update other CLI modules that use these symbols to import from the new module
and add tests or an __init__ export to ensure public API stability.
In `@wren/tests/unit/test_context.py`:
- Around line 78-199: Add two regression tests that call the library entrypoint
validate() with invalid inputs: one passing an invalid datasource string (e.g.,
"not-a-datasource") and one passing an invalid level value (e.g., "nope"); name
them test_validate_invalid_datasource and test_validate_invalid_level. Each test
should call validate(_b64(manifest), <invalid_value>) using the existing
manifest fixtures (e.g., _BASE_MANIFEST or _MODEL_WITHOUT_DESC), assert that
validate returns a structured result (no exception) and that result["errors"] is
non-empty and contains a clear error token like "invalid_datasource" or
"invalid_level" (or other consistent error text your validate() produces).
Ensure tests live alongside the other tests in the same test module and follow
the same pattern (use DataSource.duckdb where appropriate for the valid case to
mirror existing tests).
🪄 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: 7f51d912-6f32-4110-aa63-4632ecc7a4d8
📒 Files selected for processing (4)
wren/src/wren/cli.pywren/src/wren/context.pywren/src/wren/context_cli.pywren/tests/unit/test_context.py
5861c2d to
285aa00
Compare
d7e9834 to
bc78712
Compare
da32b1f to
edc8002
Compare
…hecks Upgrades `wren context validate` from a structural-only check to a semantic quality gate with three levels: - **error** (CI/CD): structural errors + view SQL dry-plan via wren-core - **warning** (default): + model/view missing description warnings - **strict**: + column-level missing description warnings View dry-plan catches broken model references and SQL syntax errors without touching the data source. Exit code 1 on any error; warnings-only exits 0. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Fix E402 lint: move context_cli import to top of cli.py
- Use .get("name", "<unknown>") for malformed manifest resilience
- Validate level param in validate() and return error dict if invalid
- Wrap DataSource() construction in try/except for invalid datasource strings
- Strip whitespace from view statement before empty check
- Add test_validate_invalid_datasource and test_validate_invalid_level
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
edc8002 to
a43dadf
Compare
_load_manifest silently treated a missing path as a base64 string, producing a confusing decode error. Now detects .json suffix + missing file and exits with "Error: MDL file not found: <path>". Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
a43dadf to
2aafea5
Compare
index_manifest now seeds canonical NL-SQL pairs into query_history, so recall_queries returns seeded results alongside user-stored ones. Assert >= 1 and verify the user query is present instead of exact count. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
wren context validateCLI command (newcontext_appsub-application)--level strict)--levelflag:error(CI/CD),warning(default),strict(all checks); exit 1 only on errorsChanges
wren/src/wren/context.pyvalidate()core logic +_check_descriptions()wren/src/wren/context_cli.pycontext_appTyper sub-app withvalidatecommandwren/src/wren/cli.pycontext_appinto main CLIwren/tests/unit/test_context.pyTest plan
test_validate_view_dry_plan_pass— valid view SQL → no errorstest_validate_view_dry_plan_error— view referencing deleted model → errortest_validate_view_empty_statement— empty statement → errortest_validate_model_no_description_warning— model without description → warningtest_validate_view_no_description_warning— view without description → warningtest_validate_level_error_suppresses_warnings—--level error→ no warningstest_validate_strict_column_warning—--level strict→ column warningstest_validate_errors_exit_1— any error → exit code 1test_validate_warnings_exit_0— warnings only → exit code 0🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests