fix: /dev/stdin works for all pipeline commands (#335)#411
Conversation
…stdin (#335) Every cmd_* function called p.read_text() to capture the source string and then called parse_file(path) which re-opened the same path a second time. For /dev/stdin the second open consumes an already-exhausted stream, returning empty content, causing the parse tree to be empty and the run command to fail with 'No exported functions to call'. Fix: replace parse_file(path) with parse(source, file=str(p)) in all seven affected commands (check, verify, compile, run, test, parse, ast). cmd_parse and cmd_ast previously had no source variable; they now read once before parsing. parse_file is no longer used in cli.py. Also adds: - TestStdinInput regression tests (4 tests) covering run, check, verify via both in-process and subprocess paths - Removes #335 from KNOWN_ISSUES.md Co-Authored-By: Claude <noreply@anthropic.invalid>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughCLI now reads source once into memory and passes that content plus a resolved logical path to the parser to avoid multiple file reads (fixing Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI
participant FileSystem
participant Parser
rect rgba(200,150,100,0.5)
Note over User,FileSystem: Before (Issue `#335`)
User->>CLI: vera run /dev/stdin
CLI->>FileSystem: read_text() [1st read]
FileSystem-->>CLI: source
CLI->>Parser: parse_file(path="/dev/stdin")
Parser->>FileSystem: read() [2nd read]
FileSystem-->>Parser: ∅
Parser-->>CLI: Error / no AST
end
rect rgba(100,200,100,0.5)
Note over User,FileSystem: After (Fixed)
User->>CLI: vera run /dev/stdin
CLI->>FileSystem: read_text() [single read]
FileSystem-->>CLI: source
CLI->>Parser: parse(source, file="/dev/stdin")
Parser-->>CLI: AST
CLI->>CLI: use logical path (cwd/stdin.vera) for resolution/output naming
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #411 +/- ##
==========================================
- Coverage 90.30% 90.30% -0.01%
==========================================
Files 49 49
Lines 19107 19105 -2
Branches 220 220
==========================================
- Hits 17255 17253 -2
Misses 1848 1848
Partials 4 4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_cli.py`:
- Line 2401: The test function test_check_reads_source_once currently declares
an unused parameter tmp_path which causes lint noise; update the function
signature in the test (def test_check_reads_source_once(...)) to remove the
tmp_path parameter so it no longer accepts or references tmp_path, and run the
tests/linter to confirm the unused-argument warning is resolved.
- Around line 2425-2433: In test_verify_dev_stdin_subprocess, add an assertion
on result.stdout to confirm the verify subprocess completed the expected
verification path (not just exit code); e.g. check result.stdout (or
result.stdout.lower()) contains a verification marker such as "verified",
"verification completed", or the tool's summary line (use result.stdout in the
assertion message for diagnostics) so the test verifies output content as well
as returncode.
- Around line 2388-2399: Rename the misnamed test_run_stdin to reflect that it
reads from a regular file (e.g., test_run_file) and update its docstring
accordingly; locate the test function named test_run_stdin (which calls cmd_run
on stdin_path and uses self.SIMPLE_PROGRAM/tmp_path) and change the name and
docstring so the test name and description match that it exercises file input
rather than /dev/stdin (do not modify cmd_run behavior in this change).
In `@vera/cli.py`:
- Around line 51-53: Create a small helper function (e.g., load_and_parse or
read_and_parse) that takes a path argument, constructs a Path, calls
read_text(encoding="utf-8"), and calls parse(source, file=str(p)), returning the
parsed tree (and optionally the source if callers need it); then replace the
duplicated patterns Path(path).read_text(...) + parse(...) in the CLI command
handlers with calls to this helper (update call sites at the locations using
Path and parse such as the blocks currently invoking Path(path) / read_text /
parse) to centralise reading/parsing and reduce duplication.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2f5c5516-9126-40a5-9b87-2958b2f57506
📒 Files selected for processing (4)
KNOWN_ISSUES.mdTESTING.mdtests/test_cli.pyvera/cli.py
💤 Files with no reviewable changes (1)
- KNOWN_ISSUES.md
- Rename test_run_stdin → test_run_file (it tests a regular file path)
- Remove unused tmp_path parameter from test_check_reads_source_once
- Add stdout assertion to test_verify_dev_stdin_subprocess ("verified")
- Extract _load_and_parse(path) helper in cli.py to centralise the
read-once pattern; replace all 7 duplicated 3-line blocks with a
single call; fixes mypy by typing return as Tree[object]
Co-Authored-By: Claude <noreply@anthropic.invalid>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
TESTING.md (1)
9-13:⚠️ Potential issue | 🟡 MinorThe example totals are still inconsistent in this document.
The overview now says
30example programs, but later sections of the same file still talk about29examples. Since this PR is already refreshing the headline metrics, please reconcile those counts in the same pass soTESTING.mdstays self-consistent. Based on learnings,scripts/check_doc_counts.pyautomatically validates that documented example and conformance program counts inTESTING.md,CLAUDE.md, and related docs match the actual file counts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TESTING.md` around lines 9 - 13, TESTING.md currently has inconsistent example counts (headline shows 30 examples while other sections still state 29); update all occurrences of the example and conformance program totals in TESTING.md so they match the refreshed headline metrics (e.g., change any "29" example references to "30" and verify the "65 programs" count is consistent), then run scripts/check_doc_counts.py to validate that TESTING.md, CLAUDE.md, and related docs pass the automated checks and adjust any remaining mismatches reported by that script.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_cli.py`:
- Around line 2370-2430: The stdin tests in TestStdinInput only use an
import-free SIMPLE_PROGRAM and never exercise module resolution or default
output-path logic (e.g., for compile), so add two tests: one variant of the
existing subprocess tests that supplies a program containing "import lib;" to
run/verify/check against /dev/stdin (to exercise module resolution), and one
test that runs "vera.cli" with the "compile /dev/stdin" command (capturing
stdout/stderr and returncode) to ensure compile-time default output path
behavior is correct; update or add tests alongside test_run_dev_stdin_subprocess
and test_check_reads_source_once so they pass the program string via subprocess
input and assert correct return codes and expected outputs.
In `@vera/cli.py`:
- Around line 49-60: The helper _load_and_parse currently returns
Path("/dev/stdin") which causes ModuleResolver(_root=p.parent) and cmd_compile
output logic to treat stdin as a filesystem path; change _load_and_parse so
after reading the source it detects stdin (e.g. path == "-" or path ==
"/dev/stdin" or p.is_fifo()) and in that case return a separate logical/base
Path for resolution/output (e.g. Path.cwd()/Path("stdin") or
Path.cwd()/Path("stdin.rs")) instead of the actual /dev/stdin Path; update
callers that pass p to ModuleResolver(_root=...) and cmd_compile output naming
to use this returned logical/base Path rather than the raw p so imports and
output filenames resolve relative to the caller's working directory rather than
/dev.
---
Outside diff comments:
In `@TESTING.md`:
- Around line 9-13: TESTING.md currently has inconsistent example counts
(headline shows 30 examples while other sections still state 29); update all
occurrences of the example and conformance program totals in TESTING.md so they
match the refreshed headline metrics (e.g., change any "29" example references
to "30" and verify the "65 programs" count is consistent), then run
scripts/check_doc_counts.py to validate that TESTING.md, CLAUDE.md, and related
docs pass the automated checks and adjust any remaining mismatches reported by
that script.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1fc69d15-d740-497e-aaad-3bcb63162274
📒 Files selected for processing (3)
TESTING.mdtests/test_cli.pyvera/cli.py
- _load_and_parse: detect /dev/stdin / - and return Path.cwd()/"stdin.vera" so module resolution (ModuleResolver _root) and compile output naming (stdin.wasm) use CWD rather than /dev/. Diagnostics retain the original path string for readable error locations. - Add test_compile_dev_stdin_wat and test_compile_dev_stdin_default_output covering the new compile-from-stdin behaviour. - TESTING.md: update three prose "29 examples" references to "30", and refresh test/line counts for test_cli.py (181→183, 2430→2455) and overall suite total (3108→3110). Co-Authored-By: Claude <noreply@anthropic.invalid>
|
@coderabbitai Fixed in commit 3bbff0e. Updated three prose references to '29 examples' in TESTING.md (lines 207, 242, 295) to '30'. All three instances were in coverage-table prose and the Ch3 feature matrix row. |
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
⛔ Learnings removed🧠 Learnings used |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/test_cli.py (1)
2370-2455:⚠️ Potential issue | 🟡 MinorCoverage gap: stdin with imports not tested.
The test suite covers the core regression but doesn't exercise module resolution when piping stdin with imports. The
_load_and_parsenormalisation toPath.cwd()/"stdin.vera"meansModuleResolverwould look for imports relative to CWD — this path is untested.A test like this would close the gap:
def test_check_dev_stdin_with_import(self, tmp_path: Path) -> None: """vera check /dev/stdin resolves imports relative to CWD.""" lib_src = """\ public fn helper(-> `@Int`) requires(true) ensures(true) effects(pure) { 1 } """ main_src = """\ import lib(helper); public fn main(-> `@Int`) requires(true) ensures(true) effects(pure) { helper() } """ (tmp_path / "lib.vera").write_text(lib_src) result = subprocess.run( [sys.executable, "-m", "vera.cli", "check", "/dev/stdin"], input=main_src, capture_output=True, text=True, cwd=tmp_path, ) assert result.returncode == 0, result.stderr🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_cli.py` around lines 2370 - 2455, Add a new test in TestStdinInput that exercises module resolution when reading /dev/stdin: create a helper module file (e.g., "lib.vera") in a tmp_path, write a main source that imports lib(helper) and call subprocess.run([sys.executable, "-m", "vera.cli", "check", "/dev/stdin"], input=main_src, capture_output=True, text=True, cwd=tmp_path) and assert returncode == 0; this ensures the _load_and_parse normalization to Path.cwd()/"stdin.vera" and ModuleResolver import lookup are covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@vera/cli.py`:
- Line 80: The unpacking of the tuple returned by _load_and_parse(path)
currently binds p and source which are unused; update the unpacking in
vera/cli.py to use underscore-prefixed names (e.g., _p, _source, tree or _, _,
tree) so unused variables are clearly marked and suppress Ruff RUF059; make the
same change in the cmd_ast code path where _load_and_parse is unpacked around
the referenced location (change the unused p and source there to
underscore-prefixed names as well).
---
Duplicate comments:
In `@tests/test_cli.py`:
- Around line 2370-2455: Add a new test in TestStdinInput that exercises module
resolution when reading /dev/stdin: create a helper module file (e.g.,
"lib.vera") in a tmp_path, write a main source that imports lib(helper) and call
subprocess.run([sys.executable, "-m", "vera.cli", "check", "/dev/stdin"],
input=main_src, capture_output=True, text=True, cwd=tmp_path) and assert
returncode == 0; this ensures the _load_and_parse normalization to
Path.cwd()/"stdin.vera" and ModuleResolver import lookup are covered.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bddee9e5-de0c-4a98-bde7-5eb1b0823aee
📒 Files selected for processing (3)
TESTING.mdtests/test_cli.pyvera/cli.py
…din test - cmd_parse and cmd_ast: rename unused `p, source` unpacking to `_p, _source` to clearly signal they are intentionally unused (suppresses RUF059). - Add test_check_dev_stdin_module_resolution: creates lib.vera in tmp_path, pipes a program with `import lib(helper)` to /dev/stdin with cwd=tmp_path, and asserts check succeeds — directly validates that the logical-path fix routes ModuleResolver to CWD instead of /dev/. - TESTING.md: update test counts (3110→3111, 183→184, 2455→2489). Co-Authored-By: Claude <noreply@anthropic.invalid>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@vera/cli.py`:
- Line 52: The return type annotation on _load_and_parse currently uses
unnecessary forward-reference quotes; update the signature of _load_and_parse to
use a normal annotation tuple[Path, str, Tree[object]] (no surrounding quotes),
relying on the module-level import of Tree so the annotation is a real type
rather than a string.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5ef65ecf-f2d4-4ac2-8a39-5feee6b45a24
📒 Files selected for processing (3)
TESTING.mdtests/test_cli.pyvera/cli.py
`from __future__ import annotations` (line 28) defers all annotations, so the string quotes around `tuple[Path, str, Tree[object]]` are redundant now that `Tree` is imported at module level. Co-Authored-By: Claude <noreply@anthropic.invalid>
Summary
cmd_*function calledp.read_text()to getsource, then immediately calledparse_file(path)which re-opened the same path. For/dev/stdinthe second open returns empty content (stream already consumed), so the parse tree was empty andvera runfailed withNo exported functions to callparse_file(path)withparse(source, file=str(p))in all seven affected commands (check,verify,compile,run,test,parse,ast).cmd_parseandcmd_asthad no source variable before; they now read once before parsing.parse_fileis no longer used incli.pyat allChanges
vera/cli.py— swapparse_file(path)→parse(source, file=str(p))in all commands; dropparse_fileimporttests/test_cli.py— newTestStdinInputclass with 4 regression tests (in-process + subprocess, coveringrun/check/verify)KNOWN_ISSUES.md— remove vera run /dev/stdin fails — stdin consumed before main compilation #335Test plan
echo '...' | vera run /dev/stdinproduces correct outputecho '...' | vera check /dev/stdinpassesecho '...' | vera verify /dev/stdinpassesCloses #335
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
Documentation