Skip to content

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

Merged
goldmedal merged 3 commits into
Canner:mainfrom
AmirF194:fix/2503-guided-recall-invalid-nl-flag
Jul 28, 2026
Merged

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

Conversation

@AmirF194

@AmirF194 AmirF194 commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

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 CLI short flag for keywords.
    • Prevented guided recall workflows from failing due to an invalid CLI argument.
  • Tests
    • Added a regression test to confirm the guided “step 2” emits a valid wren memory recall invocation and runs without a “no such option” error.

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

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 2ebfbafc-a2f1-4980-8a48-b66e1a215468

📥 Commits

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

📒 Files selected for processing (1)
  • core/wren/src/wren/ask_templates/guided.md.tmpl
🚧 Files skipped from review as they are similar to previous changes (1)
  • core/wren/src/wren/ask_templates/guided.md.tmpl

Walkthrough

The guided ask template now uses -q 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 -q 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

Poem

A bunny checks the recall 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 summarizes the main change: fixing the guided template's memory recall CLI option.
Linked Issues check ✅ Passed The template now uses a supported recall option and the new regression test covers the mismatch described in #2503.
Out of Scope Changes check ✅ Passed The changes stay focused on the guided recall option and its regression test, with no unrelated additions.
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
Contributor 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.

@goldmedal goldmedal left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks @AmirF194, make sense to me.

Comment thread core/wren/src/wren/ask_templates/guided.md.tmpl Outdated
@goldmedal
goldmedal merged commit f242be4 into Canner:main Jul 28, 2026
11 checks passed
@AmirF194
AmirF194 deleted the fix/2503-guided-recall-invalid-nl-flag branch July 28, 2026 06:11
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