feat(wren): add store-tip hint after successful query#1508
Merged
goldmedal merged 3 commits intoCanner:feat/cli-0.2.0from Apr 4, 2026
Merged
feat(wren): add store-tip hint after successful query#1508goldmedal merged 3 commits intoCanner:feat/cli-0.2.0from
goldmedal merged 3 commits intoCanner:feat/cli-0.2.0from
Conversation
Print a `wren memory store` hint to stderr after a non-exploratory query succeeds, so agents and humans get an explicit nudge to save the NL-SQL pair. A `--quiet`/`-q` flag suppresses the tip for automation pipelines. Exploratory peek queries (bare SELECT + LIMIT, no WHERE/GROUP BY/HAVING/aggregate/CTE) are filtered via a lightweight sqlglot AST check and never shown the tip. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a CLI Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI
participant Engine
participant Classifier
participant StdErr
User->>CLI: invoke command / query
CLI->>Engine: execute SQL
Engine-->>CLI: return results
CLI->>User: print results (stdout)
CLI->>Classifier: is_exploratory(sql)
Classifier-->>CLI: exploratory? (True/False)
alt not exploratory and not quiet
CLI->>StdErr: print "wren memory store" tip (escaped SQL)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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)
266-270: Deduplicate post-query tip gating into a helper.Both command paths repeat the same
quiet + is_exploratorylogic. Extracting this keeps behavior aligned and easier to maintain.Proposed refactor
+def _maybe_print_store_tip(sql: str, quiet: bool) -> None: + if quiet: + return + from wren.sql_classify import is_exploratory # noqa: PLC0415 + if not is_exploratory(sql): + _print_store_tip(sql) @@ - if not quiet: - from wren.sql_classify import is_exploratory # noqa: PLC0415 - - if not is_exploratory(sql): - _print_store_tip(sql) + _maybe_print_store_tip(sql, quiet) @@ - if not quiet: - from wren.sql_classify import is_exploratory # noqa: PLC0415 - - if not is_exploratory(sql): - _print_store_tip(sql) + _maybe_print_store_tip(sql, quiet)Also applies to: 294-298
🤖 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 266 - 270, Both command paths duplicate the gating logic that checks quiet and is_exploratory before calling _print_store_tip; extract this into a small helper (e.g., _should_print_store_tip or maybe print_store_tip_if_applicable) that takes the sql and quiet flag, imports/uses is_exploratory and calls _print_store_tip when appropriate, and then replace the duplicated blocks at the two locations (the current checks around _print_store_tip at lines shown) with a single call to that helper to keep behavior aligned and easier to maintain.wren/tests/unit/test_cli_store_tip.py (1)
34-34: Use explicitNonehandling for mock result fallback.
result or _MOCK_TABLEcan overwrite intentional falsey test values. Preferis Nonefor deterministic fixture behavior.Proposed tweak
- mock_engine.query.return_value = result or _MOCK_TABLE + mock_engine.query.return_value = _MOCK_TABLE if result is None else result🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wren/tests/unit/test_cli_store_tip.py` at line 34, The test currently sets mock_engine.query.return_value = result or _MOCK_TABLE which treats falsey but valid values (e.g., empty list, 0, False) as missing; change the fallback to explicit None checking so mock_engine.query.return_value is set to result if result is not None, otherwise to _MOCK_TABLE (reference symbols: mock_engine.query.return_value, result, _MOCK_TABLE).wren/tests/unit/test_sql_classify.py (1)
13-37: Add a regression case for nestedLIMITin subqueries.Please add a case where only an inner subquery has
LIMIT; this guards the classifier against descendant-vs-top-level clause confusion.Suggested test case
[ @@ # SUM ("SELECT SUM(total) FROM orders", False), + # Inner LIMIT only (outer SELECT has no LIMIT) + ("SELECT * FROM (SELECT * FROM orders LIMIT 5) t", False), ], )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wren/tests/unit/test_sql_classify.py` around lines 13 - 37, Add a regression case to the test tuple list that ensures LIMIT inside a nested subquery does not mark the top-level query exploratory: add a case where the top-level SELECT has no LIMIT but contains a subquery with LIMIT (for example a query selecting from "(SELECT ... LIMIT 1)") and assert the classifier returns False; update the list of cases in test_sql_classify.py (the parameterized test/cases list shown in the diff) with this new tuple.
🤖 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/tests/unit/test_cli_store_tip.py`:
- Around line 120-129: The test test_tip_escapes_single_quotes_in_sql currently
has a weak/invalid assertion (a stray `}` and not matching the actual escaped
payload). Update the assertion to check for the correctly escaped --sql argument
in result.stderr by asserting the exact escaped payload generated by the CLI,
e.g. that the stderr contains the string "--sql 'SELECT * FROM orders WHERE
status = '\\''open'\\''" (remove the stray `}` and replace the loose
"'\\''open'\\''" check with a full exact match of the escaped --sql value so
regressions in quote-escaping will fail the test).
---
Nitpick comments:
In `@wren/src/wren/cli.py`:
- Around line 266-270: Both command paths duplicate the gating logic that checks
quiet and is_exploratory before calling _print_store_tip; extract this into a
small helper (e.g., _should_print_store_tip or maybe
print_store_tip_if_applicable) that takes the sql and quiet flag, imports/uses
is_exploratory and calls _print_store_tip when appropriate, and then replace the
duplicated blocks at the two locations (the current checks around
_print_store_tip at lines shown) with a single call to that helper to keep
behavior aligned and easier to maintain.
In `@wren/tests/unit/test_cli_store_tip.py`:
- Line 34: The test currently sets mock_engine.query.return_value = result or
_MOCK_TABLE which treats falsey but valid values (e.g., empty list, 0, False) as
missing; change the fallback to explicit None checking so
mock_engine.query.return_value is set to result if result is not None, otherwise
to _MOCK_TABLE (reference symbols: mock_engine.query.return_value, result,
_MOCK_TABLE).
In `@wren/tests/unit/test_sql_classify.py`:
- Around line 13-37: Add a regression case to the test tuple list that ensures
LIMIT inside a nested subquery does not mark the top-level query exploratory:
add a case where the top-level SELECT has no LIMIT but contains a subquery with
LIMIT (for example a query selecting from "(SELECT ... LIMIT 1)") and assert the
classifier returns False; update the list of cases in test_sql_classify.py (the
parameterized test/cases list shown in the diff) with this new tuple.
🪄 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: 6b5af503-4053-403e-a80b-60a926c6682f
📒 Files selected for processing (4)
wren/src/wren/cli.pywren/src/wren/sql_classify.pywren/tests/unit/test_cli_store_tip.pywren/tests/unit/test_sql_classify.py
- Extract _maybe_print_store_tip() helper to deduplicate the quiet+is_exploratory gating in main() and query() - Fix is_exploratory() to check top-level clause args (stmt.args) instead of find() so an inner LIMIT inside a subquery doesn't incorrectly mark the outer query as exploratory; CTE detection now uses stmt.args["with_"] key - Fix mock result fallback to use explicit is None check - Strengthen quote-escaping assertion to match exact --sql payload in stderr - Add regression test: SELECT * FROM (subquery LIMIT n) returns False Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
A plain SELECT with no WHERE/GROUP BY/HAVING/aggregate is exploratory whether or not it has LIMIT — both are just "show me what's there". Remove the LIMIT requirement from the is_exploratory heuristic. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
goldmedal
added a commit
that referenced
this pull request
Apr 4, 2026
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 --sqlorwren query --sqlexecution, print awren memory storehint to stderr so agents and humans get an explicit nudge to save the NL-SQL pair--quiet/-qflag suppresses the tip for automation pipelines (stdout remains clean regardless)SELECT … LIMITwith noWHERE/GROUP BY/HAVING/ aggregate / CTE) are filtered via a lightweight sqlglot AST check and never shown the tipNew files
wren/src/wren/sql_classify.pyis_exploratory(sql)— sqlglot AST heuristicwren/tests/unit/test_sql_classify.pywren/tests/unit/test_cli_store_tip.pyTest plan
just test-unit— all 22 new tests passjust lint— ruff format + check cleanwren --sql "SELECT COUNT(*) FROM orders")--quietsuppresses tip (wren --sql "..." -q)wren --sql "SELECT * FROM orders LIMIT 5")🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
--quietflag to suppress optional CLI informational tips for cleaner scripting.--quiet).Tests