Avoid /bin/sh -c#7787
Conversation
|
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)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThis PR replaces shell-based subprocess execution in two locations. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
framework/src/main/java/org/checkerframework/framework/stub/AnnotationFileElementTypes.java (1)
897-898: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant re-sorting of jar entry names.
entriesis already sorted byComparator.comparing(Object::toString)at Line 898, thenjarEntryNames(built from the sameentrieslist) is sorted again insideprintSortedIndented. Since this is debug-only output on a rarely-hit path, the duplication is harmless but avoidable.♻️ Optional simplification
- List<String> jarEntryNames = new ArrayList<>(entries.size()); - for (JarEntry jarEntry : entries) { - jarEntryNames.add(jarEntry.getName()); - } - printSortedIndented(jarEntryNames); + List<String> jarEntryNames = + entries.stream().map(JarEntry::getName).collect(Collectors.toList()); + printSortedIndented(jarEntryNames);Also applies to: 930-935
🤖 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 `@framework/src/main/java/org/checkerframework/framework/stub/AnnotationFileElementTypes.java` around lines 897 - 898, The jar entry list in AnnotationFileElementTypes is being sorted twice: once when building entries with Comparator.comparing(Object::toString) and again later when jarEntryNames is passed to printSortedIndented. Remove the redundant second sort by reusing the already-sorted entries list (and the same pattern in the later matching block), keeping the debug-only output unchanged while avoiding duplicate work.dataflow/src/main/java/org/checkerframework/dataflow/cfg/visualize/CFGVisualizeLauncher.java (1)
328-337: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winProcessBuilder replacement looks correct.
Equivalent behavior to the prior
/bin/sh -cinvocation, and avoids shell injection risk sincefileis passed as a discrete argument rather than interpolated into a shell command string.One gap (pre-existing, not introduced by this diff):
child.waitFor()'s exit code is never checked, so a failingdotinvocation (e.g., binary missing or malformed dot file) silently produces no PDF without any error message.♻️ Optional: surface non-zero exit codes
ProcessBuilder pb = new ProcessBuilder("dot", "-Tpdf", file, "-o", file + ".pdf"); Process child = pb.start(); - child.waitFor(); + int exitCode = child.waitFor(); + if (exitCode != 0) { + System.err.println("dot exited with status " + exitCode); + } } catch (InterruptedException | IOException e) {🤖 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 `@dataflow/src/main/java/org/checkerframework/dataflow/cfg/visualize/CFGVisualizeLauncher.java` around lines 328 - 337, The producePDF method starts the dot process and waits for it, but it never checks the child exit status, so failures are silent. Update producePDF to inspect the result of child.waitFor() after launching the ProcessBuilder and, when the exit code is non-zero, report an error and exit similarly to the existing exception handling; use the producePDF and child symbols to locate the change.
🤖 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.
Outside diff comments:
In
`@dataflow/src/main/java/org/checkerframework/dataflow/cfg/visualize/CFGVisualizeLauncher.java`:
- Around line 328-337: The producePDF method starts the dot process and waits
for it, but it never checks the child exit status, so failures are silent.
Update producePDF to inspect the result of child.waitFor() after launching the
ProcessBuilder and, when the exit code is non-zero, report an error and exit
similarly to the existing exception handling; use the producePDF and child
symbols to locate the change.
In
`@framework/src/main/java/org/checkerframework/framework/stub/AnnotationFileElementTypes.java`:
- Around line 897-898: The jar entry list in AnnotationFileElementTypes is being
sorted twice: once when building entries with
Comparator.comparing(Object::toString) and again later when jarEntryNames is
passed to printSortedIndented. Remove the redundant second sort by reusing the
already-sorted entries list (and the same pattern in the later matching block),
keeping the debug-only output unchanged while avoiding duplicate work.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fcf92db5-e4d6-47a0-8e26-758bd5d176d2
📒 Files selected for processing (2)
dataflow/src/main/java/org/checkerframework/dataflow/cfg/visualize/CFGVisualizeLauncher.javaframework/src/main/java/org/checkerframework/framework/stub/AnnotationFileElementTypes.java
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
framework/src/main/java/org/checkerframework/framework/stub/AnnotationFileElementTypes.java (1)
921-939: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify: avoid collecting into a separate list then re-sorting.
entriesis already sorted at line 899, andprintSortedIndentedre-sorts again at line 952. The intermediatejarEntryNamescollection could be built with a stream and directly mapped, skipping the redundant sort logic in this debug-only path.♻️ Optional simplification
- List<String> jarEntryNames = new ArrayList<>(entries.size()); - for (JarEntry jarEntry : entries) { - jarEntryNames.add(jarEntry.getName()); - } - printSortedIndented(jarEntryNames); + printSortedIndented( + entries.stream().map(JarEntry::getName).collect(Collectors.toList()));Behavior is otherwise correct and matches the flush/sleep pattern used elsewhere.
🤖 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 `@framework/src/main/java/org/checkerframework/framework/stub/AnnotationFileElementTypes.java` around lines 921 - 939, The debug-only jar-entry printing in AnnotationFileElementTypes still builds a separate jarEntryNames list and then re-sorts it via printSortedIndented even though entries is already sorted. Simplify the block inside the stubDebug branch by mapping entries directly to names (for example in the existing loop area) and pass that result to printSortedIndented, or otherwise remove the redundant intermediate collection and extra sort while keeping the existing debug output structure intact.
🤖 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.
Outside diff comments:
In
`@framework/src/main/java/org/checkerframework/framework/stub/AnnotationFileElementTypes.java`:
- Around line 921-939: The debug-only jar-entry printing in
AnnotationFileElementTypes still builds a separate jarEntryNames list and then
re-sorts it via printSortedIndented even though entries is already sorted.
Simplify the block inside the stubDebug branch by mapping entries directly to
names (for example in the existing loop area) and pass that result to
printSortedIndented, or otherwise remove the redundant intermediate collection
and extra sort while keeping the existing debug output structure intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c12a4151-4a03-49cc-a090-23ea2dd502a7
📒 Files selected for processing (1)
framework/src/main/java/org/checkerframework/framework/stub/AnnotationFileElementTypes.java
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dataflow/src/main/java/org/checkerframework/dataflow/cfg/visualize/CFGVisualizeLauncher.java (1)
328-340: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAvoid waiting on unread
dotstreamsProcessBuilderleaves stdout/stderr piped here, but nothing drains them beforewaitFor(). Ifdotwrites enough output or hangs, this can block indefinitely. UseinheritIO()or redirect/drain the streams, and add a timeout if possible.🤖 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 `@dataflow/src/main/java/org/checkerframework/dataflow/cfg/visualize/CFGVisualizeLauncher.java` around lines 328 - 340, The producePDF method launches dot and then waits without consuming its stdout/stderr, which can deadlock the process if buffers fill. Update producePDF to either call ProcessBuilder.inheritIO() or explicitly redirect/drain the child streams before waitFor, and add a timeout around the process wait if feasible. Keep the fix localized to producePDF and the ProcessBuilder/Process child handling in CFGVisualizeLauncher.
🤖 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.
Outside diff comments:
In
`@dataflow/src/main/java/org/checkerframework/dataflow/cfg/visualize/CFGVisualizeLauncher.java`:
- Around line 328-340: The producePDF method launches dot and then waits without
consuming its stdout/stderr, which can deadlock the process if buffers fill.
Update producePDF to either call ProcessBuilder.inheritIO() or explicitly
redirect/drain the child streams before waitFor, and add a timeout around the
process wait if feasible. Keep the fix localized to producePDF and the
ProcessBuilder/Process child handling in CFGVisualizeLauncher.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7e856a02-0afb-4a63-ba05-47820401097a
📒 Files selected for processing (1)
dataflow/src/main/java/org/checkerframework/dataflow/cfg/visualize/CFGVisualizeLauncher.java
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
`@framework/src/main/java/org/checkerframework/framework/stub/AnnotationFileElementTypes.java`:
- Around line 933-934: Remove the leftover synchronous-output workaround in
AnnotationFileElementTypes, specifically the System.out.flush() and
SystemPlume.sleep(1) calls near the listing output path. Update the code so the
listing prints directly without any forced flush or pause, and keep the change
localized around the method that emits the output so the in-process behavior
remains clean.
🪄 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: 471612d5-9704-4e15-9003-7e983d16904f
📒 Files selected for processing (1)
framework/src/main/java/org/checkerframework/framework/stub/AnnotationFileElementTypes.java
|
Wait to merge until the next release of plume-util. |
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)
dataflow/src/main/java/org/checkerframework/dataflow/cfg/visualize/CFGVisualizeLauncher.java (1)
335-337: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPropagate failed PDF generation to the caller.
This only prints the non-zero exit status and returns normally, so
.dotand🤖 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 `@dataflow/src/main/java/org/checkerframework/dataflow/cfg/visualize/CFGVisualizeLauncher.java` around lines 335 - 337, Update the non-zero exit handling in CFGVisualizeLauncher so failed PDF generation is propagated to the caller instead of only being printed. Preserve the status context in the error, and ensure the CLI returns a non-zero result when the dot process fails.
🤖 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
`@dataflow/src/main/java/org/checkerframework/dataflow/cfg/visualize/CFGVisualizeLauncher.java`:
- Around line 330-331: Update the PATH diagnostic in CFGVisualizeLauncher so the
environment value is never passed as a printf format string; remove the
diagnostic or print the concatenated value with System.out.println instead,
preserving normal dot startup.
- Around line 332-334: Update the process execution in CFGVisualizeLauncher
around ProcessBuilder and child.waitFor(): redirect or inherit dot’s stdout and
stderr to prevent pipe saturation, replace the unbounded wait with a timed wait,
and forcibly terminate the child when the timeout expires before handling the
exit result.
---
Outside diff comments:
In
`@dataflow/src/main/java/org/checkerframework/dataflow/cfg/visualize/CFGVisualizeLauncher.java`:
- Around line 335-337: Update the non-zero exit handling in CFGVisualizeLauncher
so failed PDF generation is propagated to the caller instead of only being
printed. Preserve the status context in the error, and ensure the CLI returns a
non-zero result when the dot process fails.
🪄 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: 1f3636b7-9d2e-4cfc-be75-db87450252ee
📒 Files selected for processing (1)
dataflow/src/main/java/org/checkerframework/dataflow/cfg/visualize/CFGVisualizeLauncher.java
1 participant
No description provided.