feat: add -d @- and -d @file for Unix pipeline composability#3
feat: add -d @- and -d @file for Unix pipeline composability#3bowlofarugula wants to merge 2 commits intomasterfrom
Conversation
Enables Unix pipeline composability — pipe JSON args from stdin or read
from a file, following the curl @ convention:
echo '{"q":"foo"}' | murl $S/tools/search -d @-
murl $S/tools/get -d @params.json
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe changes add functionality to the CLI to load JSON data from external sources via the Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/test_cli.py (1)
185-210: Add a negative test for non-object JSON from@path.Great coverage overall. One gap remains: reject array/scalar JSON when source is a file (
-d@file``), not just stdin.Suggested test addition
+def test_parse_data_flags_file_non_object(tmp_path): + """@path with non-object JSON raises ValueError.""" + f = tmp_path / "args.json" + f.write_text('[1, 2, 3]') + with pytest.raises(ValueError, match="must be an object, not list"): + parse_data_flags((f'@{f}',))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_cli.py` around lines 185 - 210, Add a negative test that mirrors the stdin non-object case but reads from a file: create a temp file containing non-object JSON (e.g., "[1,2,3]"), call parse_data_flags with the file path prefixed by '@' (e.g., f'@{f}'), and assert it raises ValueError with the same message used for non-object input (e.g., matching "must be an object, not list"). Name the test similar to test_parse_data_flags_file_non_object so it pairs with the existing test_parse_data_flags_file and ensures parse_data_flags rejects arrays/scalars from files as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@murl/cli.py`:
- Line 395: The help text for the CLI option "-d, --data" contains a typo: the
stdin source token is written as "@->" but should be "@-". Update the help
string associated with the "-d, --data" option in murl/cli.py (the
usage/description for the Request data flag) to replace "@->" with "@-" so the
documentation and parser expectations match.
- Around line 122-123: The except blocks that currently do "except OSError as e:
raise ValueError(f'Cannot read {source}: {e}')" (and the analogous JSON parsing
handler) must preserve the original exception context by re-raising with "from
e" so the traceback retains the root cause; update both raises to "raise
ValueError(f'Cannot read {source}: {e}') from e" (and for the JSON handler,
"raise ValueError(f'Invalid JSON in {source}: {e}') from e") so the original
OSError/JSONDecodeError is chained into the new ValueError.
---
Nitpick comments:
In `@tests/test_cli.py`:
- Around line 185-210: Add a negative test that mirrors the stdin non-object
case but reads from a file: create a temp file containing non-object JSON (e.g.,
"[1,2,3]"), call parse_data_flags with the file path prefixed by '@' (e.g.,
f'@{f}'), and assert it raises ValueError with the same message used for
non-object input (e.g., matching "must be an object, not list"). Name the test
similar to test_parse_data_flags_file_non_object so it pairs with the existing
test_parse_data_flags_file and ensures parse_data_flags rejects arrays/scalars
from files as well.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9b70da9c-dbea-487b-b6de-b72aacaff5ca
📒 Files selected for processing (2)
murl/cli.pytests/test_cli.py
- Chain original exceptions with `raise ... from e` in _read_json_source - Clarify -d help text to avoid @-> ambiguity Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Addressed both CodeRabbit findings in 4b5c454:
|
turlockmike
left a comment
turlockmike
left a comment
There was a problem hiding this comment.
Two-lens review: Unix design, security.
Overall: Clean feature that follows the curl convention correctly. The _read_json_source extraction, JSON-object-only enforcement, and flag ordering semantics (later overrides earlier) are well done. A few items to address.
Key findings:
-
@detection precedence —-d email=@userwould be parsed as a@-source, not a key=value pair. Check for=first — if the token contains=, it's a key=value pair regardless of what comes after. See inline. -
Double stdin consumption — Passing
-d @- -d @-silently fails on the secondsys.stdin.read()(reads empty string). Detect multiple@-uses before the loop and error early. That's what curl does. See inline. -
Unbounded file read —
open(path).read()with no size limit. If murl is composed into pipelines or CI that accepts user-supplied paths,-d @/dev/zeroexhausts memory. A simple 10 MB ceiling closes this. See inline.
Additional observations:
- No end-to-end test through
CliRunner.invokeagainst themcp_serverfixture — all stdin tests exerciseparse_data_flagsdirectly. A test likerunner.invoke(main, [url, "-d", "@-", "--no-auth"], input='{"message":"hi"}')would prove the feature works through the full CLI path. - The
@pathfile-read behavior is worth documenting in the README so integrators who pipe user-controlled values into-d @<path>understand they're responsible for path sanitization.
What's good: The _read_json_source helper is the right extraction. Error messages are specific and actionable ("JSON from @- must be an object, not list"). The flag ordering semantics (later overrides earlier, so @- can combine with explicit overrides) mirrors curl correctly.
— Mike's AI Clone 🧬
| JSON objects (starting with '{') are merged into the result. | ||
| JSON arrays (starting with '[') are not supported as they don't | ||
| represent key-value pairs needed for MCP arguments. | ||
| Supports three formats (processed in order, later values override earlier): |
There was a problem hiding this comment.
[Unix Design] Warning — @ Detection Fires Before = Check
stripped.startswith('@') runs on the full argument before checking for =. This means -d email=@user would be parsed as a @-source (trying to read file email=@user), not as a key=value pair where the value is the literal string @user.
Curl handles this because @ only activates when it's the entire -d value. Fix: check for = in the token first — if it contains =, it's a key=value pair regardless of what comes after.
| for data in data_flags: | ||
| stripped = data.strip() | ||
| if stripped.startswith('{'): | ||
| if stripped.startswith('@'): |
There was a problem hiding this comment.
[Unix Design] Warning — Double Consumption of Stdin
Pass -d @- -d @- and the second sys.stdin.read() reads an empty string, raising ValueError("Empty input from @-") — a confusing error when the user's intent was clear. Stdin is a one-shot resource.
Detect multiple @- uses before the loop and fail immediately with "@- may only appear once". That's what curl does.
| if path == '-': | ||
| content = sys.stdin.read() | ||
| else: | ||
| with open(path) as f: |
There was a problem hiding this comment.
[Security] Warning — Unbounded File Read
open(path).read() with no size limit means -d @/dev/zero or a very large file will exhaust memory before JSON parsing even runs. For a local CLI this is primarily self-inflicted, but if murl is composed into pipelines or CI that accepts user-supplied arguments, an attacker who controls the path can force unbounded memory consumption.
A simple guard — read up to a reasonable ceiling (e.g., 10 MB) and raise a clear error if exceeded — closes this cleanly.
2 participants
Summary
@-(stdin) and@path(file) support to the-d/--dataflag, following the curl conventionmurl $S/tools/search -d q=foo | jq '{id:.[0].text}' | murl $S/tools/get -d @--dflags override earlier ones, so@-can be combined with explicitkey=valueoverridesTest plan
@-reads JSON object from stdin@pathreads JSON object from file@-merges correctly with explicit-d key=valueflags (later wins)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
-d/--dataflagDocumentation