fix(sandbox-e2b): preserve zero exit codes in envd streams

[mikemikimike]

Summary

Fixes #2793.

E2bEnvdProcessClient incorrectly relies on protobuf field presence for the proto3 scalar exit_code. That causes JSON exitCode: 0 end events to be dropped, and causes both JSON and binary streams to retain Integer.MIN_VALUE instead of the successful exit code.

This change:

• preserves an end event when JSON explicitly contains exitCode, including 0;
• reads the proto3 scalar default unconditionally from parsed end events;
• adds a regression test through the JSON stream-draining path.

Validation

• mvn -pl agentscope-extensions/agentscope-extensions-sandbox/agentscope-extensions-sandbox-e2b -am -Dspotless.check.skip=true -Dtest=E2bEnvdProcessClientTest -Dsurefire.failIfNoSpecifiedTests=false test — 8 passed
• mvn -pl agentscope-extensions/agentscope-extensions-sandbox/agentscope-extensions-sandbox-e2b spotless:check — passed
• git diff --check — passed

The Maven commands were run with JDK 21 because the repository's Spotless plugin requires Java 17+; the shell's default JAVA_HOME points to JDK 8.

[codecov]

Codecov Report

❌ Patch coverage is 33.33333% with 2 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...e/extensions/sandbox/e2b/E2bEnvdProcessClient.java	33.33%	0 Missing and 2 partials ⚠️

📢 Thoughts on this report? Let us know!

[oss-maintainer]

Summary

This PR reworks the JSON end-event handling in E2bEnvdProcessClient.parseJsonStartResponse so a present exitCode (including 0) preserves the end event, and adds a regression test. The direction matches issue #2793, and the CLA is signed — thanks for the clear write-up and tests.

Findings

• [Warning] E2bEnvdProcessClient.java:348 — the new condition hasExitCode || !endBuilder.getAllFields().isEmpty() is currently equivalent to hasExitCode, because this code path only ever sets exit_code on endBuilder. That makes the change a no-op when envd emits an explicit "exitCode": 0 (the old code already attached the end event there), and still drops the end event when envd omits a zero exit code (proto3 JSON default omission). Please confirm what a real envd stream actually sends on success (a capture of the final frame would settle it).
• [Info] E2bEnvdProcessClientTest.java:169 — the new test pins Integer.MIN_VALUE for {"end":{}}; if envd omits a zero exitCode, that frame is exactly the success shape from #2793, so the test could end up codifying the buggy behavior. Also heads-up that open PR #2828 removes the MIN_VALUE sentinel entirely (throws on missing exit code) and edits the same method/tests — worth coordinating so the two don't land in conflict.

Suggestions

• Capture one real envd end frame for a successful (exit 0) command and paste it here. If it is {"end":{}} / has no exitCode, the fix needs to treat a present-but-empty end as exit code 0 (aligning with the binary path, where end.getField(exit_code) returns the proto3 default 0). If it contains "exitCode": 0, we should revisit what actually reproduced #2793, since canConvertToInt() is already true for 0 on main.

Overall a solid first contribution — just want to nail down the root cause before approving. Happy to re-review once the envd frame shape is confirmed.

Automated review by github-manager-bot

[oss-maintainer]

[Warning] parseJsonStartResponse only ever sets exit_code on endBuilder (lines 345-347), so the second disjunct !endBuilder.getAllFields().isEmpty() can never be true while hasExitCode is false — the condition is currently equivalent to the old if (exitCodeNode.canConvertToInt()). That means behavior for an explicit "exitCode": 0 is unchanged from main (the end event was already attached there, since canConvertToInt() is true for 0).

The key question for #2793: what does envd actually send when the process exits with 0? If it uses proto3 JSON default omission (frame looks like {"event":{"end":{}}} with no exitCode field), this change still drops the end event and drainStartStream still returns Integer.MIN_VALUE, i.e. the reported failure mode would remain. Could you share a real envd capture for a successful command? If envd really omits the field, consider treating a present-but-empty end object as a valid end event carrying the proto3 default exit code 0 — consistent with the binary path, where an empty EndEvent on the wire yields end.getField(exit_code) = 0.

[mikemikimike]

Thanks for raising this. I checked the capture in #2793: the successful Alibaba FC envd frame is {"event":{"end":{"exitCode":0}}}, rather than an empty end object. The original JSON parser did call setField(exit_code, 0), but the subsequent getAllFields().isEmpty() presence check drops proto3 scalar defaults, so the end event was still omitted. The new hasExitCode condition deliberately preserves the JSON-level field presence and fixes that explicit-zero frame. I have kept the regression focused on this captured wire shape.

[oss-maintainer]

[Info] This assertion pins Integer.MIN_VALUE for {"event":{"end":{}}}. If envd omits a zero exitCode in its JSON (proto3 default omission), that frame is exactly the success-case wire shape described in #2793 — in that scenario this test would codify the buggy behavior instead of preventing it. Worth verifying against a real envd stream.

Also note that open PR #2828 removes the MIN_VALUE sentinel entirely and throws IOException when the stream ends without an exit code, while editing the same method and tests. The two approaches conflict; they should be reconciled (pick one end state and rebase the other) to avoid semantic conflicts at merge time.

[mikemikimike]

Agreed that this assertion would create an unnecessary semantic commitment for a frame that is not the captured #2793 success shape. I removed jsonCodecIgnoresEndWithoutExitCode in 6df1778, leaving the regression focused on the explicit exitCode: 0 capture. #2828 can define and test the separate incomplete-stream behavior (missing end/exit code) without this PR pinning it to Integer.MIN_VALUE.