Split CLI file-output modes (#362) - #406
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
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:
Summary
WalkthroughThe CLI now uses capability-scoped filesystem helpers: ChangesCLI output refactor
Sequence Diagram(s)sequenceDiagram
participant CLI
participant Rayon
participant OutputHelpers
participant StdoutStderr
CLI->>Rayon: process input paths in parallel
alt stdout mode
Rayon->>OutputHelpers: format_to_string for each file
OutputHelpers-->>Rayon: Result<String>
Rayon->>StdoutStderr: print results and errors in input order
else in-place mode
Rayon->>OutputHelpers: rewrite_in_place for each file
OutputHelpers-->>Rayon: Result<()>
Rayon->>StdoutStderr: print errors in input order
end
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 19 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (19 passed)
📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 090f872d2d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/architecture.md`:
- Around line 599-623: Add an italicized “Figure N: …” caption immediately after
the closing fence of the Mermaid diagram, describing the CLI’s parallel file
processing and ordered output behavior.
In `@tests/cli.rs`:
- Around line 107-111: Extend the --in-place assertions in the relevant CLI test
to verify in_place.stdout is empty after confirming in_place.status.success().
Keep the existing stderr failure diagnostic and rewritten-content validation
unchanged.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: a59eafea-bdee-4a79-906f-ce32111f32b2
⛔ Files ignored due to path filters (2)
tests/snapshots/cli__format_to_string_table_prose.snapis excluded by!**/*.snaptests/snapshots/cli__rewrite_in_place_table_prose.snapis excluded by!**/*.snap
📒 Files selected for processing (5)
docs/architecture.mddocs/developers-guide.mdsrc/main.rstests/cli.rstests/data/cli-output-parity.md
40ab249 to
58cf151
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
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 `@src/main.rs`:
- Around line 108-125: Update format_to_string and rewrite_in_place to use
cap_std/camino capability-scoped directory or file handles instead of direct
std::fs operations, preserving their current read, format, newline, and write
behavior. Refactor the file-output tests to use the same capability-scoped I/O
boundary and path types, removing direct filesystem access throughout these
paths.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 9f180a3e-417b-4924-b7e0-dab6a761d852
⛔ Files ignored due to path filters (2)
tests/snapshots/cli__format_to_string_table_prose.snapis excluded by!**/*.snaptests/snapshots/cli__rewrite_in_place_table_prose.snapis excluded by!**/*.snap
📒 Files selected for processing (5)
docs/architecture.mddocs/developers-guide.mdsrc/main.rstests/cli.rstests/data/cli-output-parity.md
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
|
@coderabbitai Have the following failed checks now been resolved? If further work is required, please provide an AI agent prompt for the remaining work to be done to address these failures. Do not treat warnings as optional or aspirational. Where a change is out of scope for this PR, propose a GitHub issue unless one exists already. (Treat documentation and validation coverage as in scope). ❌ Failed checks (2 errors)
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Current PR diff/stat =='
git diff --stat "$(git merge-base HEAD origin/main)" HEAD 2>/dev/null || git diff --stat HEAD~1 HEAD
echo '== Relevant file discovery =='
fd -t f -a '^(main\.rs|cli\.rs|Cargo\.toml|cli-output-parity\.md)$' . | sort
echo '== src/main.rs outline =='
ast-grep outline src/main.rs --items all --view expanded 2>/dev/null || true
echo '== File helper and filesystem call sites =='
rg -n -C 5 -P '\b(format_to_string|rewrite_in_place|handle_file)\s*\(|\b(std::)?fs::(read_to_string|write)\s*\(' src/main.rs tests/cli.rs
echo '== Test assertions related to output modes =='
rg -n -C 8 -P '\b(format_to_string|rewrite_in_place|assert_snapshot!?|stdout|read_to_string|write|cli_output_modes_snapshot_table_prose)\b' src/main.rs tests/cli.rs
echo '== Fixture contents =='
fixture="$(fd -t f -a '^cli-output-parity\.md$' tests | head -n1 || true)"
if [ -n "$fixture" ]; then
cat -n "$fixture"
fi
echo '== Snapshot files related to output modes =='
fd -t f -a . tests | rg 'format_to_string|rewrite_in_place|output.*parity|snap$' || true
echo '== Dependencies relevant to filesystem abstraction =='
if [ -f Cargo.toml ]; then
rg -n -C 2 -i 'cap-std|camino|tempfile|fs|dependency' Cargo.toml
fi
echo '== Issues matching filesystem injection / output mode architecture =='
gh issue list --repo leynos/mdtablefix --state all --limit 100 \
--search 'filesystem inject OR file I/O abstraction OR output mode OR handle_file' \
--json number,title,state,url --jq '.[] | "#\(.number) [\(.state)] \(.title) \(.url)"' || trueLength of output: 40329 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Input fixture: tests/data/cli-output-parity.dat =='
cat -n tests/data/cli-output-parity.dat
echo '== Independent expected oracle: tests/data/cli-output-parity.expected.md =='
cat -n tests/data/cli-output-parity.expected.md
echo '== Output-mode snapshots =='
for f in \
tests/snapshots/cli__format_to_string_table_prose.snap \
tests/snapshots/cli__rewrite_in_place_table_prose.snap
do
echo "-- $f"
cat -n "$f"
done
echo '== Helper contract documentation =='
rg -n -C 6 -P '\b(format_to_string|rewrite_in_place|capability|capability-scoped|Dir|filesystem)\b' \
docs/developers-guide.md docs/architecture.md src/main.rs
echo '== Exact diff for relevant production and integration-test code =='
git diff "$(git merge-base HEAD origin/main)" HEAD -- \
src/main.rs tests/cli.rs Cargo.toml docs/developers-guide.md docs/architecture.md \
tests/data/cli-output-parity.dat tests/data/cli-output-parity.expected.mdLength of output: 36169
No further implementation work or follow-up GitHub issue is required for these two failures. Issue Runtime validation was not executable in this inspection sandbox; rely on the PR’s reported full validation gate for execution confirmation. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
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 `@tests/data/cli-output-parity.expected.md`:
- Around line 5-7: Add a descriptive caption immediately before the fixture
table in the expected output, and update the corresponding paired input fixture
and snapshots so they produce the same captioned table consistently.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 59cf39b1-4151-4b2c-bd11-ebff4ec48620
⛔ Files ignored due to path filters (4)
Cargo.lockis excluded by!**/*.locktests/data/cli-output-parity.datis excluded by!**/*.dattests/snapshots/cli__format_to_string_table_prose.snapis excluded by!**/*.snaptests/snapshots/cli__rewrite_in_place_table_prose.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
Cargo.tomldocs/architecture.mddocs/developers-guide.mdsrc/main.rstests/cli.rstests/data/cli-output-parity.expected.md
e1cc849 to
0d0cee3
Compare
Replace the Boolean-controlled `Option<String>` contract with dedicated formatting and in-place rewrite functions. Keep both modes byte-identical, and cover the contract with property and CLI snapshot tests.
Update the concurrency diagram so both parallel CLI modes reference the current handlers and their concrete result contracts.
Caption the concurrency diagram and ensure the in-place CLI contract rejects unexpected stdout while retaining its rewrite snapshot coverage.
Keep the upstream before-and-after example intact after the concurrency diagram replay altered its surrounding Markdown.
Open each input file's parent directory once at the CLI boundary, then perform formatting and rewrites through `cap_std` UTF-8 directory capabilities. Keep the property and CLI file-output tests on the same boundary, and document the ownership rule for future callers.
Use an unformatted raw fixture and a separate expected-output oracle so stdout and in-place results each prove the formatting contract. Keep the raw fixture outside Markdown formatting to preserve this regression guard.
Keep the raw fixture, expected output, and both output-mode snapshots in sync with a descriptive caption immediately before the representative table.
Keep the labelled numbered-list input and converted footnote output separate so the architecture document remains valid Markdown and explains the transformation accurately.
a41b05d to
580363a
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/architecture.md`:
- Around line 619-645: The architecture diagram’s file-processing calls are
shown sequentially despite stating parallel execution. Update the Stdout and
In-place branches to model each file operation as explicit Mermaid parallel
branches (or a single Rayon batch submission), while preserving ordered result
handling and the existing error/exit behavior; include the required “Triage:”
paragraph for this documentation style change.
In `@src/main.rs`:
- Around line 105-107: Update open_file_parent to retain the original
std::path::Path and use the cap_std::fs/Path APIs instead of converting through
Utf8Path, allowing valid non-UTF-8 Unix paths to reach file I/O. Add a Unix-only
regression test that exercises opening a path containing non-UTF-8 bytes and
verifies it succeeds.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: a87f3bc8-6473-4747-babf-9e2065e61815
⛔ Files ignored due to path filters (4)
Cargo.lockis excluded by!**/*.locktests/data/cli-output-parity.datis excluded by!**/*.dattests/snapshots/cli__format_to_string_table_prose.snapis excluded by!**/*.snaptests/snapshots/cli__rewrite_in_place_table_prose.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
Cargo.tomldocs/architecture.mddocs/developers-guide.mdsrc/main.rstests/cli.rstests/data/cli-output-parity.expected.md
| participant Formatter as format_to_string | ||
| participant Rewriter as rewrite_in_place | ||
| participant Stdout as Stdout | ||
| participant Stderr as Stderr | ||
|
|
||
| User->>CLI: Run CLI with multiple files (not in-place) | ||
| CLI->>FileHandler: handle_file(file1) | ||
| CLI->>FileHandler: handle_file(file2) | ||
| CLI->>FileHandler: handle_file(file3) | ||
| Note over CLI,FileHandler: Files processed in parallel | ||
| FileHandler-->>CLI: Result (Ok(Some(output)) or Err(error)) | ||
| loop For each file in input order | ||
| CLI->>Stdout: Print output (if Ok) | ||
| CLI->>Stderr: Print error (if Err) | ||
| User->>CLI: Run CLI with multiple files | ||
| alt Stdout mode | ||
| CLI->>Formatter: format_to_string(file1) | ||
| CLI->>Formatter: format_to_string(file2) | ||
| CLI->>Formatter: format_to_string(file3) | ||
| Note over CLI,Formatter: Files processed in parallel | ||
| Formatter-->>CLI: Result<String> or Err(error) | ||
| loop For each file in input order | ||
| CLI->>Stdout: Print text (if Ok) | ||
| CLI->>Stderr: Print error (if Err) | ||
| end | ||
| else In-place mode | ||
| CLI->>Rewriter: rewrite_in_place(file1) | ||
| CLI->>Rewriter: rewrite_in_place(file2) | ||
| CLI->>Rewriter: rewrite_in_place(file3) | ||
| Note over CLI,Rewriter: Files processed in parallel | ||
| Rewriter-->>CLI: Result<()> or Err(error) | ||
| loop For each file in input order | ||
| CLI->>Stderr: Print error (if Err) | ||
| end | ||
| end | ||
| CLI-->>User: Exit (with error if any file errored) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Model the worker calls as parallel branches.
Represent each file operation with a Mermaid par branch and its own result, or
show one batch submission to Rayon. The current sequential arrows contradict
the stated parallel execution model.
Triage: [type:docstyle]
As per coding guidelines, “Make concurrency models explicit for shared state,
async execution, ordering, cancellation, locks, transactions, task lifetimes,
and parallelism.” As per path instructions, “Include a ‘Triage:’ paragraph”
for style comments.
🤖 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 `@docs/architecture.md` around lines 619 - 645, The architecture diagram’s
file-processing calls are shown sequentially despite stating parallel execution.
Update the Stdout and In-place branches to model each file operation as explicit
Mermaid parallel branches (or a single Rayon batch submission), while preserving
ordered result handling and the existing error/exit behavior; include the
required “Triage:” paragraph for this documentation style change.
Sources: Coding guidelines, Path instructions
| fn open_file_parent(path: &Path) -> anyhow::Result<(Dir, Utf8PathBuf)> { | ||
| let path = Utf8Path::from_path(path) | ||
| .with_context(|| format!("converting {} to a UTF-8 path", path.display()))?; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Files ==\n'
git ls-files | rg '^(src/main\.rs|Cargo\.toml|docs/|src/.*\.rs)$'
printf '\n== main.rs outline ==\n'
ast-grep outline src/main.rs --view expanded
printf '\n== Relevant lines in src/main.rs ==\n'
sed -n '1,220p' src/main.rs
printf '\n== Search for open_file_parent and cap_std usage ==\n'
rg -n 'open_file_parent|cap_std::fs_utf8|Utf8Path|Utf8PathBuf|PathBuf|Dir' src/main.rs src/**/*.rsRepository: leynos/mdtablefix
Length of output: 11962
🌐 Web query:
cap-std 4.0.2 cap_std::fs::Dir Path read write std::path::Path non-UTF-8
💡 Result:
In cap-std 4.0.2, the handling of non-UTF-8 paths depends on which filesystem module you use [1][2]. The primary module, cap_std::fs, is designed to support non-UTF-8 paths by using standard Rust types like std::path::Path and std::ffi::OsString, similar to the standard library's std::fs [3][1]. You should use this module if your application requires support for filenames that are not valid UTF-8 [1][2]. If you specifically require UTF-8-only path handling, cap-std provides an optional fs_utf8 module (enabled via the fs_utf8 Cargo feature) [1][4]. This module uses the camino crate's Utf8Path and Utf8PathBuf types [4]. If you attempt to use this module, it will enforce that all paths are valid UTF-8 [1][2]. Key points regarding path handling in cap-std: 1. Capability-based security: cap-std operates using Dir handles [4]. Methods on Dir (such as read, write, and open) accept paths that are relative to that directory, ensuring sandboxing [5][3]. 2. Path Types: When using cap_std::fs, you interact with standard std::path::Path types, which natively handle non-UTF-8 sequences found on various operating systems [3]. 3. DirEntry: In cap_std::fs, the DirEntry::file_name method returns an OsString, which correctly preserves non-UTF-8 data [6]. 4. Avoiding Restricted Modules: If your application must handle arbitrary non-UTF-8 filesystem paths, avoid the cap_std::fs_utf8 module, as it is explicitly intended to restrict operations to valid UTF-8 paths [1][2].
Citations:
- 1: https://docs.rs/cap-std/latest/cap_std/fs_utf8/index.html
- 2: https://docs.rs/cap-std/latest/x86_64-pc-windows-msvc/cap_std/fs_utf8/index.html
- 3: https://docs.rs/cap-std/latest/cap_std/fs/index.html
- 4: https://github.com/bytecodealliance/cap-std
- 5: https://docs.rs/crate/cap-std/4.0.2
- 6: https://docs.rs/cap-std/latest/cap_std/fs/struct.DirEntry.html
Preserve non-UTF-8 CLI paths.
Stop converting PathBuf into Utf8Path here; cap_std::fs_utf8::Dir rejects valid non-UTF-8 Unix filenames before any file I/O. Use the cap_std::fs/Path boundary instead and add a Unix-only regression test.
🤖 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 `@src/main.rs` around lines 105 - 107, Update open_file_parent to retain the
original std::path::Path and use the cap_std::fs/Path APIs instead of converting
through Utf8Path, allowing valid non-UTF-8 Unix paths to reach file I/O. Add a
Unix-only regression test that exercises opening a path containing non-UTF-8
bytes and verifies it succeeds.
3 participants
Summary
This branch replaces
handle_file's Boolean-controlledOption<String>contract with purpose-specific file-output functions.
Closes #362.
format_to_stringnow returns newline-terminated formatted text, andrewrite_in_placewrites that same text. The CLI's parallel paths collect theirnatural result types directly, without discarding an encoded mode sentinel.
Review walkthrough
direct parallel call sites, and property parity test.
fixture, and both
stdout snapshot
and in-place snapshot.
Validation
make check-fmt: passedmake lint: passedmake test: passedmake markdownlint: passedmake nixie: passedmbake validate Makefile: passedmake: passedcoderabbit review --agent: no findingsNotes
Model checking is not applicable: this API-shape refactor concerns unbounded
file content, while the property test directly compares the pure formatting
result with the bytes written in place across generated inputs.
References