PR #21 Review Comments: hypeman build command improvements #22
Description
This issue tracks all review comments from PR #21: feat: add hypeman build command.
1. Client hangs indefinitely on failed build status
Severity: Medium
Location: pkg/cmd/build.go lines 378-380
Author: @cursor[bot]
The handling of the "failed" status in streamBuildEvents is inconsistent with "ready" and "cancelled". When "ready" is received, the function returns immediately with nil. When "cancelled" is received, it returns immediately with an error. However, when "failed" is received, the code only sets buildError and continues reading from the SSE stream, relying on the server to close the connection (trigger EOF). If the server keeps the connection alive after sending "failed" (e.g., continues sending heartbeat events or has networking issues), the client will hang indefinitely instead of returning the failure to the user.
2. SSE reader drops final event data at EOF
Severity: Medium
Location: pkg/cmd/build.go lines 331-338
Author: @cursor[bot]
When reader.ReadString('\n') encounters EOF before finding a newline, it returns both the partial data read and io.EOF as the error. The current code checks for EOF and immediately breaks without processing the line variable, which may contain valid event data. If the server closes the connection without sending a trailing newline after the final event (due to network issues or server behavior), that event is silently dropped. This could cause a final "ready" status to be missed, leading the client to incorrectly report the build failed or ended unexpectedly.
3. Should read .dockerignore instead of hardcoded skip list
Location: pkg/cmd/build.go line 157
Author: @rgarcia
Should we read .dockerignore to determine what to skip, rather than having a hardcoded list? That would match Docker's behavior and give users control over what gets excluded.
4. defer file.Close() inside loop could leak file descriptors
Location: pkg/cmd/build.go line 194
Author: @rgarcia
This defer file.Close() is inside the filepath.Walk callback loop—defers don't run until the function returns, so this could leak file descriptors if there are many files. Should close immediately after the copy instead.
5. Static "build failed" error message doesn't capture actual error
Location: pkg/cmd/build.go line 360
Author: @rgarcia
When the build fails, buildError is just set to the static string "build failed"—but the actual error message from the API isn't captured. Would be helpful to show the real error (maybe fetch the build status to get the error field, or parse it from an event if available).