Skip to content

fix(core): handle io exceptions in subprocess #218

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
NickSeagull merged 2 commits into from
Nov 14, 2025
Merged

fix(core): handle io exceptions in subprocess #218

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1458537
fix(core): handle io exceptions in subprocess
Nov 12, 2025
0a1e2c4
Apply review fixes, better error messages
Nov 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions cli/src/Neo/Build.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ handle config = do

completion <-
Subprocess.openInherit "nix-build" (Array.fromLinkedList ["-E", buildExpression]) rootFolder Subprocess.InheritBOTH
|> Task.mapError (\_ -> CustomError "Failed to run nix-build")
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
if completion.exitCode != 0
then errorOut completion.stderr
else print completion.stdout
Comment on lines 85 to 87

@coderabbitai coderabbitai Bot Nov 13, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

Suggested change
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.

Expand Down
1 change: 1 addition & 0 deletions cli/src/Neo/Run.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ handle config = do
let rootFolder = [path|.|]
completion <-
Subprocess.openInherit [fmt|./result/bin/#{projectName}|] (Array.fromLinkedList []) rootFolder Subprocess.InheritBOTH
|> Task.mapError (\_ -> CustomError "Failed to run the built application")
if completion.exitCode != 0
then errorOut completion.stderr
else print completion.stdout
Comment on lines 26 to 28

@coderabbitai coderabbitai Bot Nov 13, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

Suggested change
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.

Expand Down
1 change: 1 addition & 0 deletions cli/src/Neo/Shell.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ handle config = do

completion <-
Subprocess.openInherit "nix-shell" (Array.fromLinkedList ["-E", shellExpression]) rootFolder Subprocess.InheritBOTH
|> Task.mapError (\_ -> CustomError "Failed to open the shell")

@coderabbitai coderabbitai Bot Nov 12, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

@NickSeagull NickSeagull Nov 13, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree

@coderabbitai coderabbitai Bot Nov 13, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 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:

  1. Simplicity: Each module has its own local Error type, making a shared utility function less beneficial
  2. Clarity: The pattern \err -> CustomError [fmt|context: #{err}|] is self-documenting and consistent
  3. 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!

if completion.exitCode != 0
then errorOut completion.stderr
else Task.yield ()
Expand Down
18 changes: 14 additions & 4 deletions core/system/Subprocess.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ module Subprocess (
import Array (Array)
import Array qualified
import Basics
import GHC.IO.Exception qualified as Exception
import Maybe qualified
import Path (Path)
import Path qualified
Expand Down Expand Up @@ -45,7 +46,12 @@ data InheritStream
deriving (Eq, Ord, Show)


openInherit :: Text -> Array Text -> Path -> InheritStream -> Task _ Completion
data Error
= NotExecutable
deriving (Show)
Comment thread
coderabbitai[bot] marked this conversation as resolved.


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)
Expand All @@ -69,8 +75,12 @@ openInherit executable arguments directory inheritStream = do
(_, _, _, ph) <-
-- System.Process.readCreateProcessWithExitCode processToExecute ""
System.Process.createProcess processToExecute
|> Task.fromIO
ec <- System.Process.waitForProcess ph |> Task.fromIO
|> Task.fromFailableIO @Exception.IOError
|> Task.mapError (\_ -> NotExecutable)

@NickSeagull NickSeagull Nov 13, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you map to the other types of errors described in the other comment?

ec <-
System.Process.waitForProcess ph
|> Task.fromFailableIO @Exception.IOError
|> Task.mapError (\_ -> NotExecutable)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
let exitCode = case ec of
System.Exit.ExitSuccess -> 0
System.Exit.ExitFailure code -> code
Expand All @@ -79,6 +89,6 @@ openInherit executable arguments directory inheritStream = do
Task.yield Completion {exitCode, stdout, stderr}


open :: Text -> Array Text -> Path -> Task _ Completion
open :: Text -> Array Text -> Path -> Task Error Completion
open executable arguments directory =
openInherit executable arguments directory InheritNONE
Loading