feat: implement PipeHandler for shell piping support#52
feat: implement PipeHandler for shell piping support#52jhawpetoss6-collab wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughA new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/shell/pipes.go (1)
15-17: Consider handling partial writes and adding error context.
os.Stdout.Writecan return a short write (n < len(data)) without an error in some edge cases. Also, wrapping the error with context would help users diagnose issues faster.✏️ Suggested improvement
func (h *PipeHandler) WriteToStdout(data []byte) error { - _, err := os.Stdout.Write(data) - return err + n, err := h.Out.Write(data) + if err != nil { + return fmt.Errorf("failed to write to stdout: %w", err) + } + if n < len(data) { + return fmt.Errorf("incomplete write to stdout: wrote %d of %d bytes", n, len(data)) + } + return nil }As per coding guidelines: "Errors should help the user fix the problem, not just describe it".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/shell/pipes.go` around lines 15 - 17, The WriteToStdout implementation can return a short write without an error; update PipeHandler.WriteToStdout to loop until all bytes are written (handling n < len(data) by advancing the slice) and on any write error return a wrapped error that includes context (e.g., "failed writing to stdout" and how many bytes were written vs expected) so callers can diagnose partial-write failures; locate and modify the WriteToStdout method to implement the loop and use fmt.Errorf or the project’s error-wrapping helper to add that context.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/shell/pipes.go`:
- Around line 8-9: The PipeHandler currently relies on global
os.Stdin/os.Stdout; change it to accept I/O dependencies by adding fields (e.g.,
In io.Reader, Out io.Writer) to the PipeHandler struct and provide a constructor
like NewPipeHandler(in io.Reader, out io.Writer) that sets those fields
(optionally defaulting to os.Stdin/os.Stdout when nil). Update any PipeHandler
methods to use PipeHandler.In and PipeHandler.Out instead of os.Stdin/os.Stdout
so callers and tests can inject mock streams.
---
Nitpick comments:
In `@pkg/shell/pipes.go`:
- Around line 15-17: The WriteToStdout implementation can return a short write
without an error; update PipeHandler.WriteToStdout to loop until all bytes are
written (handling n < len(data) by advancing the slice) and on any write error
return a wrapped error that includes context (e.g., "failed writing to stdout"
and how many bytes were written vs expected) so callers can diagnose
partial-write failures; locate and modify the WriteToStdout method to implement
the loop and use fmt.Errorf or the project’s error-wrapping helper to add that
context.
| // PipeHandler enables piping data into and out of Axe agents via standard streams. | ||
| type PipeHandler struct{} |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Inject I/O dependencies instead of using global os.Stdin/os.Stdout.
The empty struct relies on global state (os.Stdin, os.Stdout), which makes testing tricky and violates the coding guideline about passing dependencies explicitly. Consider accepting io.Reader and io.Writer in the struct or constructor so callers can control the streams.
🔧 Suggested refactor
-// PipeHandler enables piping data into and out of Axe agents via standard streams.
-type PipeHandler struct{}
+// PipeHandler enables piping data into and out of Axe agents via standard streams.
+type PipeHandler struct {
+ In io.Reader
+ Out io.Writer
+}
+
+// NewPipeHandler returns a PipeHandler wired to the given streams.
+// Pass os.Stdin and os.Stdout for real usage.
+func NewPipeHandler(in io.Reader, out io.Writer) *PipeHandler {
+ return &PipeHandler{In: in, Out: out}
+}As per coding guidelines: "No global state — pass dependencies explicitly".
📝 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.
| // PipeHandler enables piping data into and out of Axe agents via standard streams. | |
| type PipeHandler struct{} | |
| // PipeHandler enables piping data into and out of Axe agents via standard streams. | |
| type PipeHandler struct { | |
| In io.Reader | |
| Out io.Writer | |
| } | |
| // NewPipeHandler returns a PipeHandler wired to the given streams. | |
| // Pass os.Stdin and os.Stdout for real usage. | |
| func NewPipeHandler(in io.Reader, out io.Writer) *PipeHandler { | |
| return &PipeHandler{In: in, Out: out} | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/shell/pipes.go` around lines 8 - 9, The PipeHandler currently relies on
global os.Stdin/os.Stdout; change it to accept I/O dependencies by adding fields
(e.g., In io.Reader, Out io.Writer) to the PipeHandler struct and provide a
constructor like NewPipeHandler(in io.Reader, out io.Writer) that sets those
fields (optionally defaulting to os.Stdin/os.Stdout when nil). Update any
PipeHandler methods to use PipeHandler.In and PipeHandler.Out instead of
os.Stdin/os.Stdout so callers and tests can inject mock streams.
Summary
This PR introduces shell piping support to Axe by adding a new
PipeHandlertype in thepkg/shellpackage. This enables Axe agents to consume input from stdin and emit results to stdout, allowing them to participate in shell pipelines. The implementation includes buffering capabilities for handling large data streams.Changelog
Added
PipeHandlertype inpkg/shellpackage for standard-stream I/OReadFromStdin()method to read all available data from stdin and return as a byte sliceWriteToStdout(data []byte)method to write byte data to stdout