fix(core): handle io exceptions in subprocess#218
Conversation
WalkthroughBehold: a typed subprocess error is added in Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Command (Build / Run / Shell)
participant Sub as Subprocess (open / openInherit)
participant OS as OS (createProcess / waitForProcess)
CLI->>Sub: call openInherit/open(args)
note right of Sub `#EFEFEF`: Returns Task Error Completion\nIOErrors → ProcessError(Text)
Sub->>OS: createProcess(...)
OS-->>Sub: success or IOError
alt IOError
Sub-->>CLI: Task(Error = ProcessError)
else success
Sub->>OS: waitForProcess(pid)
OS-->>Sub: ExitCode
alt ExitSuccess
Sub-->>CLI: Completion (ok)
else non-zero
Sub-->>CLI: Completion (non-zero)
end
end
note right of CLI `#EFEFEF`: CLI applies Task.mapError → CustomError("...")\nand continues existing handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ 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.
Actionable comments posted: 4
♻️ Duplicate comments (1)
cli/src/Neo/Run.hs (1)
25-25: Preserve the original error information for diagnostics.The prophecy repeats! This is the third manifestation of error detail abandonment in thy codebase.
Apply this correction:
- |> Task.mapError (\_ -> CustomError "Failed to run the built application") + |> Task.mapError (\err -> CustomError [fmt|Failed to run the built application: #{err}|])As decreed in Shell.hs review, consider the shared helper function to eliminate this triplication.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (4)
cli/src/Neo/Build.hs(1 hunks)cli/src/Neo/Run.hs(1 hunks)cli/src/Neo/Shell.hs(1 hunks)core/system/Subprocess.hs(4 hunks)
🔇 Additional comments (1)
core/system/Subprocess.hs (1)
87-88: Clarify the intent of empty stdout/stderr strings.Behold these lines where stdout and stderr are ordained as empty strings! When
InheritNONEmode is active, the streams are captured viaCreatePipe, yet their content is not retrieved. WhenInheritmodes are used, empty strings are semantically correct since output flows to the parent's streams.This implementation is consistent with the current behavior, but the commented code at line 76 suggests
readCreateProcessWithExitCodewas previously considered, which would have captured the output.Verify this is intentional: if capturing output for
InheritNONEis desired in the future, the process handles fromcreateProcesswould need to be read. If the current behavior (always empty strings) is correct, consider adding a comment to prevent future confusion:+ -- stdout and stderr are always empty as streams are either inherited + -- or not captured; use readCreateProcessWithExitCode if capture is needed let stdout = "" let stderr = ""
|
|
||
| completion <- | ||
| Subprocess.openInherit "nix-shell" (Array.fromLinkedList ["-E", shellExpression]) rootFolder Subprocess.InheritBOTH | ||
| |> Task.mapError (\_ -> CustomError "Failed to open the shell") |
There was a problem hiding this comment.
Preserve the original error information for diagnostics.
By the divine decree previously spoken! This lambda also discards the specific error details with (\_ -> ...). The pattern repeats across thy codebase - Build, Run, and Shell all commit this same transgression.
Apply this correction:
- |> Task.mapError (\_ -> CustomError "Failed to open the shell")
+ |> Task.mapError (\err -> CustomError [fmt|Failed to open the shell: #{err}|])Consider extracting a shared helper function to avoid this duplication across Build.hs, Run.hs, and Shell.hs:
mapSubprocessError :: Text -> Subprocess.Error -> Error
mapSubprocessError context err = CustomError [fmt|#{context}: #{err}|]🤖 Prompt for AI Agents
In cli/src/Neo/Shell.hs around line 84, the Task.mapError currently discards the
original subprocess error with (\_ -> CustomError "Failed to open the shell");
replace that with a mapping that preserves error details by using a helper like
mapSubprocessError which accepts a context string and the Subprocess.Error and
returns CustomError including the original error text. Add this helper (e.g.,
mapSubprocessError :: Text -> Subprocess.Error -> Error) in a shared module (or
a common Utils file) and import it into Shell.hs, Build.hs and Run.hs, then
change the lambda to Task.mapError (mapSubprocessError "Failed to open the
shell") so the original error is logged for diagnostics.
There was a problem hiding this comment.
🧩 Analysis chain
@NickSeagull By thy divine approval, I shall illuminate the path to righteous error handling! Let me first examine the current state of Build.hs and Run.hs to provide thee with complete guidance.
🏁 Script executed:
#!/bin/bash
# Examine the current error handling in Build.hs
cat cli/src/Neo/Build.hs | grep -A 2 "Task.mapError"
Length of output: 722
🏁 Script executed:
#!/bin/bash
# Examine the current error handling in Run.hs
cat cli/src/Neo/Run.hs | grep -A 2 "Task.mapError"
Length of output: 146
🏁 Script executed:
#!/bin/bash
# Check if there's already a common utilities module
fd -t f "Util" cli/src/Neo/
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check the Error type definition in Build.hs
rg -n "^data Error" cli/src/Neo/Build.hs -A 3
Length of output: 79
🏁 Script executed:
#!/bin/bash
# Check the Error type definition in Run.hs
rg -n "^data Error" cli/src/Neo/Run.hs -A 3
Length of output: 79
@NickSeagull Behold! I have surveyed thy domain and shall now provide the complete implementation to preserve error diagnostics across all affected modules.
Changes Required
1. cli/src/Neo/Shell.hs (Line 84)
completion <-
Subprocess.openInherit "nix-shell" (Array.fromLinkedList ["-E", shellExpression]) rootFolder Subprocess.InheritBOTH
|> Task.mapError (\err -> CustomError [fmt|Failed to open the shell: #{err}|])2. cli/src/Neo/Build.hs (around line where nix-build is called)
completion <-
Subprocess.openInherit "nix-build" (Array.fromLinkedList ["-E", buildExpression, "--no-out-link"]) rootFolder Subprocess.InheritBOTH
|> Task.mapError (\err -> CustomError [fmt|Failed to run nix-build: #{err}|])3. cli/src/Neo/Run.hs (around line where the built application runs)
completion <-
Subprocess.openInherit exeName arguments rootFolder Subprocess.InheritBOTH
|> Task.mapError (\err -> CustomError [fmt|Failed to run the built application: #{err}|])Rationale
By thy divine wisdom, I have chosen the inline approach rather than extracting a shared helper because:
- Simplicity: Each module has its own local
Errortype, making a shared utility function less beneficial - Clarity: The pattern
\err -> CustomError [fmt|context: #{err}|]is self-documenting and consistent - Maintainability: Direct error message construction keeps the context and error together
The Show instance on Subprocess.Error (which is NotExecutable) will automatically be interpolated into the error message, providing thee with precise diagnostics when subprocess operations fail!
/\_/\
( o.o ) Verily! The divine order of error
> ^ < preservation shall now reign!
/| |
(| |)
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
NickSeagull
left a comment
NickSeagull
left a comment
There was a problem hiding this comment.
Hey, thank you so much for the quick PR!
It looks good, I just agreed upon some comments by CodeRabbit.
Can you take a look? Thanks 🙏
|
|
||
| completion <- | ||
| Subprocess.openInherit "nix-shell" (Array.fromLinkedList ["-E", shellExpression]) rootFolder Subprocess.InheritBOTH | ||
| |> Task.mapError (\_ -> CustomError "Failed to open the shell") |
| |> Task.fromIO | ||
| ec <- System.Process.waitForProcess ph |> Task.fromIO | ||
| |> Task.fromFailableIO @Exception.IOError | ||
| |> Task.mapError (\_ -> NotExecutable) |
There was a problem hiding this comment.
Can you map to the other types of errors described in the other comment?
DeviousStoat
force-pushed
the
io-subprocess
branch
from
519d665 to
0a1e2c4
Compare
|
Sure! Hopefully it should be better now |
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 (2)
core/system/Subprocess.hs (2)
1-7: Export the Error type for transparency.Observe, great architect! Thy
Errortype (defined at lines 49-51) is referenced in the signatures ofopenandopenInherit, yet it remaineth absent from the module's export list. While thy current callers transform it immediately viamapError, future maintainers or external modules may wish to pattern-match uponProcessErroror handle it specifically.Apply this enhancement:
module Subprocess ( OpenOptions (..), Completion (..), InheritStream (..), + Error (..), open, openInherit, ) whereThis follows Haskell convention and improves API transparency.
54-90: Critical: The publicopenfunction uses a broken InheritNONE mode that causes process blockage.Behold what thy verification hath revealed: the broken
InheritStreammodes are not mere theoretical defects. On line 94, thy codebase exposes a publicopenfunction that directly callsopenInheritwithInheritNONE—one of the three fatally flawed modes.open :: Text -> Array Text -> Path -> Task Error Completion open executable arguments directory = openInherit executable arguments directory InheritNONEWhen
InheritNONEexecutes, both stdout and stderr pipes are created (line 60), yet thy code discards the handles (line 75) and never reads them. The subprocess shall write to these pipes, the buffers shall fill, and the process shall hang indefinitely.The modes
InheritSTDOUTandInheritSTDERRsuffer identical fates—defined but broken, creating pipes that are never consumed.Thy current external callers use only
InheritBOTH(the safe path), but this public API is a timebomb awaiting use.Remedy this immediately with Option 1: Remove the three broken modes and simplify the implementation:
data InheritStream - = InheritSTDOUT - | InheritSTDERR - | InheritBOTH - | InheritNONE + = InheritBOTH deriving (Eq, Ord, Show)openInherit :: Text -> Array Text -> Path -> InheritStream -> Task Error Completion openInherit executable arguments directory inheritStream = do - let (stdoutStream, stderrStream) = case inheritStream of - InheritSTDOUT -> (System.Process.Inherit, System.Process.CreatePipe) - InheritSTDERR -> (System.Process.CreatePipe, System.Process.Inherit) - InheritBOTH -> (System.Process.Inherit, System.Process.Inherit) - InheritNONE -> (System.Process.CreatePipe, System.Process.CreatePipe) + let (stdoutStream, stderrStream) = + (System.Process.Inherit, System.Process.Inherit)open :: Text -> Array Text -> Path -> Task Error Completion open executable arguments directory = - openInherit executable arguments directory InheritNONE + openInherit executable arguments directory InheritBOTHAlternatively, if partial inheritance proves necessary, implement proper pipe reading before exposing such modes.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (4)
cli/src/Neo/Build.hs(1 hunks)cli/src/Neo/Run.hs(1 hunks)cli/src/Neo/Shell.hs(1 hunks)core/system/Subprocess.hs(4 hunks)
🔇 Additional comments (4)
cli/src/Neo/Build.hs (1)
84-84: Excellent error preservation! The divine decree has been fulfilled.Behold, the subprocess error is now captured and interpolated into thy CustomError message. When nix-build fails to execute, the underlying cause shall no longer vanish into the void. Diagnosticians and maintainers shall have actionable context at their fingertips.
cli/src/Neo/Shell.hs (1)
82-87: Exemplary implementation! This is the righteous path.By the divine architecture, this module demonstrates the proper handling of inherited streams! When
InheritBOTHdirects output to the terminal, thou wiselyTask.yield ()on line 87 rather than attempting to print the void. The error preservation on line 84 is equally sound.cli/src/Neo/Run.hs (1)
25-25: Error preservation achieved!The subprocess error is now faithfully captured and embedded within thy CustomError message. When the application fails to execute, the underlying cause shall be revealed to those who seek understanding.
core/system/Subprocess.hs (1)
78-79: Divine error handling! The IOError is now preserved for eternity.By thy wisdom, the subprocess errors are no longer cast into the abyss! Both process creation failures (line 78-79) and wait failures (line 82-83) now capture the underlying
IOErrorand embed it withinProcessErrorvia format interpolation. Diagnosticians shall rejoice at this abundance of context.Also applies to: 82-83
| if completion.exitCode != 0 | ||
| then errorOut completion.stderr | ||
| else print completion.stdout |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove the vestigial print statement.
Mortal architect! When thou invokest InheritBOTH on line 83, the subprocess output flows directly unto the terminal in real-time. The completion.stdout field remains an empty string, rendering thy print completion.stdout on line 87 without effect—a ghost of intention past.
Apply this refinement:
if completion.exitCode != 0
then errorOut completion.stderr
- else print completion.stdout
+ else Task.yield ()Observe how Shell.hs (line 87) correctly uses Task.yield () in the same scenario.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if completion.exitCode != 0 | |
| then errorOut completion.stderr | |
| else print completion.stdout | |
| if completion.exitCode != 0 | |
| then errorOut completion.stderr | |
| else Task.yield () |
🤖 Prompt for AI Agents
In cli/src/Neo/Build.hs around lines 85 to 87, the code prints completion.stdout
after running the subprocess, but when using InheritBOTH the subprocess streams
directly to the terminal and completion.stdout is empty; remove the vestigial
`print completion.stdout` and replace it with a no-op like `Task.yield ()` (or
simply omit any action) so the successful branch does not attempt to print an
empty string and mirrors the pattern used in Shell.hs.
| if completion.exitCode != 0 | ||
| then errorOut completion.stderr | ||
| else print completion.stdout |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove the phantom print statement.
Behold the same transgression as in Build.hs! When InheritBOTH channels the subprocess output directly to the terminal (line 24), thy completion.stdout remains eternally empty. The print completion.stdout on line 28 is thus a no-op—executing, yet accomplishing naught.
Apply this correction:
if completion.exitCode != 0
then errorOut completion.stderr
- else print completion.stdout
+ else Task.yield ()Align with the wisdom demonstrated in Shell.hs (line 87).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if completion.exitCode != 0 | |
| then errorOut completion.stderr | |
| else print completion.stdout | |
| if completion.exitCode != 0 | |
| then errorOut completion.stderr | |
| else Task.yield () |
🤖 Prompt for AI Agents
In cli/src/Neo/Run.hs around lines 26 to 28, the code prints completion.stdout
even though the process was run with InheritBOTH and stdout will be empty;
remove the phantom print statement and replace it with the same no-op/success
behavior used in Shell.hs (line 87) so the function doesn't attempt to print
completion.stdout when output has already been inherited.
2 participants
As discussed in #197
Summary by CodeRabbit