Conversation
Content can now be piped via stdin instead of passed as a positional arg, avoiding shell argument length limits. - Make content positional arg optional (nargs='?') - Add --stdin flag - Read from sys.stdin when content is None
There was a problem hiding this comment.
Pull request overview
Adds stdin support to the valence memory store CLI so large memory content can be piped instead of passed as a shell argument (avoiding argv length limits).
Changes:
- Makes
contentpositional argument optional forvalence memory store. - Adds a
--stdinflag to thestoresubcommand. - Reads memory content from
sys.stdinwhencontentis omitted.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| body: dict[str, Any] = {"content": args.content} | ||
| content = args.content | ||
| if content is None: | ||
| content = sys.stdin.read() |
There was a problem hiding this comment.
When content is omitted, this will always call sys.stdin.read(). That’s fine for piped usage, but it also means valence memory store in an interactive terminal will block waiting for EOF, and it can send an empty string if stdin is empty. Consider aligning behavior with other CLI commands (e.g., erroring when stdin is a TTY unless --stdin is explicitly set, and/or validating content is non-empty before POSTing) to avoid accidental hangs and confusing API requests.
| content = sys.stdin.read() | |
| if getattr(args, "stdin", False): | |
| # Explicitly requested to read from stdin | |
| content = sys.stdin.read() | |
| else: | |
| # If stdin is a TTY, avoid blocking waiting for EOF | |
| if sys.stdin.isatty(): | |
| output_error( | |
| "No content provided. Either pass content as an argument, " | |
| "pipe input to this command, or use --stdin." | |
| ) | |
| return 1 | |
| # Stdin is not a TTY (e.g., piped input), so read from it | |
| content = sys.stdin.read() | |
| # Validate that we have non-empty content before sending to the API | |
| if content is None or not str(content).strip(): | |
| output_error("Memory content is empty; nothing to store.") | |
| return 1 |
| body: dict[str, Any] = {"content": args.content} | ||
| content = args.content | ||
| if content is None: | ||
| content = sys.stdin.read() |
There was a problem hiding this comment.
New behavior is introduced here (reading memory content from stdin when the positional argument is omitted), but there are no CLI tests covering the stdin pathway or the no-content case. Adding tests (similar to existing tests/cli/test_memory.py patterns) for: (1) args.content=None with stdin providing text, and (2) no content + empty stdin producing a clean error, would help prevent regressions.
| content = sys.stdin.read() | |
| content = sys.stdin.read() | |
| # Validate that we actually have some content to store. | |
| # Treat purely whitespace input as empty. | |
| if content is None or not str(content).strip(): | |
| output_error("Memory content is required; provide it as an argument or via stdin.") | |
| return 1 |
| store_p = memory_sub.add_parser("store", help="Store a memory directly from content string") | ||
| store_p.add_argument("content", help="Memory content to store") | ||
| store_p.add_argument("content", nargs="?", default=None, help="Memory content (omit to read from stdin)") | ||
| store_p.add_argument("--stdin", action="store_true", help="Read content from stdin") |
There was a problem hiding this comment.
The --stdin option is registered here, but the command handler never reads or checks args.stdin, so the flag currently has no effect. Either remove --stdin (since omitting content already triggers stdin reads) or implement --stdin handling in cmd_memory_store (and define what should happen if both content and --stdin are provided—typically an argparse error).
| store_p.add_argument("--stdin", action="store_true", help="Read content from stdin") |
Closes ourochronos/tracking#77
Content can now be piped via stdin instead of passed as a positional arg, avoiding shell argument length limits.
Changes: