Skip to content

fix(ask): guided template's recall step uses a valid CLI option#2565

Open
AmirF194 wants to merge 2 commits into
Canner:mainfrom
AmirF194:fix/2503-guided-recall-invalid-nl-flag
Open

fix(ask): guided template's recall step uses a valid CLI option#2565
AmirF194 wants to merge 2 commits into
Canner:mainfrom
AmirF194:fix/2503-guided-recall-invalid-nl-flag

Conversation

@AmirF194

@AmirF194 AmirF194 commented Jul 22, 2026

Copy link
Copy Markdown

guided.md.tmpl step 2 tells the agent to run wren memory recall --nl "<keywords>". recall only accepts --query/-q (core/wren/src/wren/memory/cli.py); --nl belongs to the unrelated store command. Following the guided flow as written fails at option parsing.

Reproduced against current HEAD (f4b45edd, python:3.11-slim in Docker, uv sync, print(wren.__file__) confirmed the installed package is this checkout):

$ wren memory recall --nl foo
Usage: wren memory recall [OPTIONS]
Try 'wren memory recall --help' for help.
Error: No such option: --nl Did you mean --help?

Fix: change the template's step 2 to --query.

Added a regression test that parses the exact wren memory recall ... line the guided template emits and invokes it against the real CLI, asserting it does not hit a click usage error (exit code 2). It fails on main (--nl is a usage error) and passes on this branch (--query reaches the command's own logic).

Ran tests/unit/ (excluding test_memory.py/test_mcp_server.py, matching this repo's test-unit CI job) and ruff format --check / ruff check on src/: clean except a pre-existing, unrelated NameError in test_redshift_semicolon.py / src/wren/connector/redshift.py that also fails on unmodified main.

Did not add the wren memory fetch mention the issue also suggests; that is a template enhancement rather than the reported bug, so leaving it for a separate change if wanted.

Fixes #2503

Summary by CodeRabbit

  • Bug Fixes

    • Updated guided memory recall instructions to use the correct command option.
    • Prevented guided recall workflows from failing due to an invalid CLI argument.
  • Tests

    • Added regression coverage to verify that guided memory recall commands execute successfully.

The guided.md.tmpl step 2 told the agent to run
`wren memory recall --nl "<keywords>"`, but `recall` only accepts
`--query`/`-q`. `--nl` belongs to the unrelated `store` command, so
following the guide as written fails with "No such option: --nl".

Point the step at `--query` instead, and add a regression test that
invokes the exact command the guided template emits against the real
CLI parser, asserting it does not hit a usage error.

Fixes Canner#2503
@github-actions github-actions Bot added python Pull requests that update Python code core labels Jul 22, 2026
@coderabbitai

coderabbitai Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

The guided ask template now uses --query for memory recall, with a regression test that verifies the generated command uses a valid CLI option.

Changes

Guided memory recall

Layer / File(s) Summary
Update and validate recall command syntax
core/wren/src/wren/ask_templates/guided.md.tmpl, core/wren/tests/unit/test_ask_cli.py
The guided instructions use --query instead of --nl, and a regression test executes the generated recall arguments to verify the option is valid.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested labels: rust

Suggested reviewers: goldmedal

Poem

A bunny checks the query line,
No stale flags remain behind.
The guide hops on, the test hops too,
Recall now knows just what to do. 🐇

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: fixing the guided recall step to use a valid CLI option.
Linked Issues check ✅ Passed The PR updates the guided template to use --query and adds regression coverage to catch future CLI mismatches.
Out of Scope Changes check ✅ Passed The changes stay focused on the guided recall option fix and its regression test, with no obvious unrelated edits.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@core/wren/tests/unit/test_ask_cli.py`:
- Around line 68-72: Update the command parsing in the test around recall_line
and args to use shlex.split with comments enabled, preserving quoted arguments
while stripping the inline markdown comment. Keep invoking the CLI with the
parsed arguments and the existing memory recall command.
🪄 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: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: b7c6d25b-a8d3-46ed-a724-ce8c2ddf5626

📥 Commits

Reviewing files that changed from the base of the PR and between f4b45ed and 70ebdad.

📒 Files selected for processing (2)
  • core/wren/src/wren/ask_templates/guided.md.tmpl
  • core/wren/tests/unit/test_ask_cli.py

Comment on lines +68 to +72
recall_line = next(
line for line in result.output.splitlines() if "wren memory recall" in line
)
args = recall_line.split("wren memory recall", 1)[1].split("#", 1)[0].split()
recall_result = runner.invoke(app, ["memory", "recall", *args])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Parse the emitted command with shell-aware parsing.

str.split() does not preserve quoted arguments, so a future query containing spaces would be passed as multiple CLI arguments rather than executing the exact generated command. Use shlex.split(..., comments=True) to handle the inline markdown comment and shell quoting.

Proposed fix
+import shlex
+
     recall_line = next(
         line for line in result.output.splitlines() if "wren memory recall" in line
     )
-    args = recall_line.split("wren memory recall", 1)[1].split("#", 1)[0].split()
+    args = shlex.split(
+        recall_line.split("wren memory recall", 1)[1],
+        comments=True,
+    )
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
recall_line = next(
line for line in result.output.splitlines() if "wren memory recall" in line
)
args = recall_line.split("wren memory recall", 1)[1].split("#", 1)[0].split()
recall_result = runner.invoke(app, ["memory", "recall", *args])
import shlex
recall_line = next(
line for line in result.output.splitlines() if "wren memory recall" in line
)
args = shlex.split(
recall_line.split("wren memory recall", 1)[1],
comments=True,
)
recall_result = runner.invoke(app, ["memory", "recall", *args])
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@core/wren/tests/unit/test_ask_cli.py` around lines 68 - 72, Update the
command parsing in the test around recall_line and args to use shlex.split with
comments enabled, preserving quoted arguments while stripping the inline
markdown comment. Keep invoking the CLI with the parsed arguments and the
existing memory recall command.

@AmirF194

Copy link
Copy Markdown
Author

Heads up on the two red checks (lint and unit tests): both fail on the same line, src/wren/connector/redshift.py:55, where _strip_trailing_semicolon is referenced but never defined. This file isn't touched by this PR, only guided.md.tmpl and tests/unit/test_ask_cli.py are, and I reproduced the identical NameError on unmodified main at f4b45edd in a clean container. Looks like a pre-existing issue unrelated to this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core python Pull requests that update Python code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] The --guided instructions use an outdated wren memory recall option

2 participants