fix(sdk): Connect-envelope process.Process/Start and parse exit-0 end…

[chaojixinren]

Summary

Two fixes on the Go SDK's Commands.Run (/process.Process/Start) path, both
required to run a process end-to-end against a real deployment:

1. Request framing ([Bug Report] Go SDK sends /process.Process/Start request body without a Connect envelope #825) — startProcess sent the streaming request body as
   raw JSON while advertising Content-Type: application/connect+json. A
   server-streaming Connect RPC requires a 5-byte length-prefixed enveloped
   message, so envd read the leading JSON bytes ({"pr…) as a frame header,
   decoded a bogus length, and aborted. Now wrapped via a new
   encodeConnectEnvelope helper + Connect-Content-Encoding: identity,
   matching the Python SDK and the Go PTY path. watchDir reuses the same
   helper (previously inlined identical framing).

2. Exit-0 end-event parsing — the end event is proto3 JSON, which omits a
   zero-valued exitCode entirely, so a successful (exit 0) process arrives as
   only {"exited":true,"status":"exit status 0"} and parseProcessStartStream
   failed with process EndEvent missing exit code. Now recovers the code from
   the status string and falls back to the exited flag, so exit-0 commands
   (the common case) no longer spuriously fail.

Changes

• sdk/go/envd.go
  • Add encodeConnectEnvelope; use it in startProcess + set
    Connect-Content-Encoding: identity; refactor watchDir to reuse it.
  • processEndEvent.exitCode(): fall back to parsing status (exit status N)
    then the exited flag when exitCode/exit_code are absent.
• sdk/go/sdk_test.go
  • TestCommandsRunUsesEnvdProcessStart: assert the request body is a valid
    Connect envelope and Connect-Content-Encoding: identity.
  • Add TestProcessEndEventExitCode (table-driven) and
    TestCommandsRunExitZeroStatusOnly (exit-0 stream reproduction).

Root cause (framing) — live A/B, same endpoint/port, only body framing differs

[FRAME] raw JSON      -> {"error":{"code":"invalid_argument","message":"protocol error: promised 577794671 bytes in enveloped message, got 50 bytes"}}

[FRAME] Connect frame -> {"event":{"start":{"pid":11}}}
                         {"event":{"data":{"stdout":"aGkK"}}}
                         {"event":{"end":{"exited":true,"status":"exit status 0"}}}

Raw JSON → envd's protocol framing error. The 5-byte-enveloped request runs
cleanly (pid=11, stdout=aGkK = base64 of hi\n, exit status 0). The
envelope is the only variable.

Testing

• go vet ./... clean; go test ./... green (unit tests, incl. the new
  envelope + exit-0 coverage).
• Live E2E against a real CubeSandbox (api=127.0.0.1:13000,
  proxy=127.0.0.1:11080, domain=cube.app), with the [Bug Report] Go SDK sends envd data-plane RPCs to the Jupyter port (49999) instead of envd (49983) #816 envd-port fix
  applied so requests reach envd (49983):

Framing fix — /process.Process/Start streams cleanly:

=== RUN   TestIntegrationProcessConnectEnvelope
OK  /process.Process/Start streamed cleanly -> stdout="envd-ok\n" stderr="e\n" exit=3
--- PASS: TestIntegrationProcessConnectEnvelope

Exit-0 parsing fix — exit 0 and exit 3 both return correctly:

=== RUN   TestIntegrationCommandsExitCodes
OK  exit 0 -> stdout="line-1\nline-2\nline-3\n" exit=0
OK  exit 3 -> stdout="bye\n" exit=3
--- PASS: TestIntegrationCommandsExitCodes (0.76s)
PASS

Notes

• Depends on [Bug Report] Go SDK sends envd data-plane RPCs to the Jupyter port (49999) instead of envd (49983) #816 (envd-port routing) to be exercisable end-to-end; the two are
  distinct bugs on the same code path. Unit tests do not catch either
  (httptest mocks never validate request framing and always include a nonzero
  exitCode).
• Response parsing was already Connect-aware (parseProcessStartStream →
  readConnectEnvelope); only the request side and the exit-0 case were wrong.

Closes #825

[wbzdssm]

Overall: fixes are correct and well-tested. A few notes:

1. watchDir missing Connect-Content-Encoding: identity
   startProcess sets this header after wrapping with encodeConnectEnvelope, but watchDir doesn't, even though it now uses the same helper. Both are server-streaming Connect RPCs — the request framing should be consistent.

2. Signal-killed exit code
   "signal: killed" falls back to exit code 0 via the exited flag, but the conventional mapping is 128 + signal_number. The Python SDK already handles this. Worth checking what envd actually puts in status for signal-terminated processes.

3. Nit: encodeConnectEnvelope returns *bytes.Buffer instead of io.Reader
   No bug, but returning the concrete type leaks an implementation detail.

[cubesandboxbot]

Suggestion: Add a comment explaining why Connect-Content-Encoding: identity is needed.

The Connect protocol supports gzip-compressed request bodies (via the compressed flag in the envelope + Connect-Content-Encoding: gzip). Setting this to identity explicitly signals to the server that the body is uncompressed. A brief comment here would help future readers understand why this header was added and when it matters.

// Explicitly signal uncompressed body so the Connect server does not
// attempt gzip decompression (the envelope flag byte is 0).
req.Header.Set("Connect-Content-Encoding", "identity")

[cubesandboxbot]

PR Review: fix(sdk): Connect-envelope process.Process/Start and parse exit-0 end events (#828)

Overview

Clean, focused fix addressing two bugs on the Go SDK's Commands.Run path. The PR is well-described and the E2E verification is thorough. Below is synthesized feedback from code quality, security, performance, test coverage, and documentation review dimensions.

---

Inline Comment

• sdk/go/envd.go:111 — Missing explanatory comment on the Connect-Content-Encoding: identity header (see inline).

---

Code Quality

1. Redundant error branching in readConnectEnvelope (envd.go:334-336)
The if err == io.EOF || err == io.ErrUnexpectedEOF branch performs the identical return as the fallthrough on line 337. Callers already differentiate the error type via io.EOF checks. This can be simplified to a bare if err != nil { return 0, nil, err }.

This is pre-existing code, not introduced by this PR, but worth cleaning up.

---

Security

2. Potential spurious error from Watcher.Close() race (envd.go:530-536 + readLoop at 591-634)
When Close() cancels the context and closes the body concurrently with readLoop blocked in readConnectEnvelope(w.body), the body-close may surface as a non-EOF error (e.g., os.ErrClosed) rather than io.EOF, producing a spurious error on the errs channel. Consider checking w.ctx.Err() after a read failure to suppress errors caused by intentional cancellation:

if err != nil {
    if errors.Is(err, io.EOF) || errors.Is(err, io.ErrUnexpectedEOF) {
        return
    }
    if w.ctx.Err() != nil {
        return // context was canceled — not a real error
    }
    w.sendErr(err)
    return
}

Pre-existing code, not introduced by this PR, but a meaningful API robustness improvement.

---

Test Coverage — Gaps Worth Filling

3. parseProcessStartStream error paths untested
The function has ~6 uncovered branches that are only reachable through direct unit tests (feed an io.Reader directly without spinning up an HTTP server per case):

Branch	What to test
flags & connectCompressedFlag != 0	Reject compressed stream
json.Unmarshal decode error	Malformed event JSON inside a valid envelope
response.Event == nil	Empty {} event, continue silently
response.Event.End.Error != ""	Process failure EndEvent
End event with exitCode() returning ok=false + no Error	Fallback exhausted
Stream ending without EndEvent (!sawEnd)	Abrupt stream termination

Adding table-driven tests for parseProcessStartStream with strings.NewReader would cover these compactly.

4. Missing exitCode() table entries
The new TestProcessEndEventExitCode covers 8 cases. Suggest adding:

• nil receiver: (*processEndEvent)(nil).exitCode() — the method has a nil guard at line 644 but no test exercises it
• both codes set: ExitCode: 1, ExitCodeSnake: 99 — validates camelCase priority
• status with extra whitespace: Status: " exit status 5 " — confirms TrimSpace + TrimPrefix work on non-canonical whitespace
• negative exit code: ExitCode: -2 — works but untested

---

Comments & Documentation

5. Comment on encodeConnectEnvelope is accurate (good)
6. exitCode() fallback comment correctly explains the proto3 JSON quirk (good)
7. PR description's claim that "response parsing was already Connect-aware" is accurate (good)

---

Summary

Category	Verdict
Correctness	✅ Fixes are correct; the root cause analysis (framing + exit-0 encoding) is solid
Security	No new vectors introduced; one pre-existing race noted for improvement
Performance	Neutral to slightly positive (watchDir reallocation eliminated)
Tests	Good coverage of the happy path; error-path unit tests for parseProcessStartStream would complete the picture
Documentation	One inline comment suggestion posted; otherwise clear and accurate

Overall this is a well-engineered fix with thorough E2E validation. The suggestions above are refinements, not blockers.