agentscope-ai / agentscope-ai/agentscope-java

fix(sandbox-e2b): E2bEnvdProcessClient drops the end event when exit_code is 0 - every successful command is reported as a failure

Đang mở
#2,793 0 bình luận 0 reaction 0 người được giao Xem trên GitHub
Ngôn ngữ chính
Java
Star
5.6k
Fork
1.3k
Merge trung bình
4 ngày 12 giờ
Pull request đã merge (30 ngày)
77

Mô tả

## Summary

`E2bEnvdProcessClient` (agentscope-extensions-sandbox-e2b) uses proto3 field-presence
APIs (`getAllFields()` / `hasField()`) to decide whether an `end` event exists and
whether `exit_code` was set. `exit_code` is a proto3 singular scalar **without
explicit presence**, so the value `0` is indistinguishable from "absent" through
those APIs. The result inverts the meaning of success:

- a command that exits with **0 (success)** → the `end` event is dropped / the exit
code is never read → the caller sees the initial sentinel `Integer.MIN_VALUE` →
`ExecException: Command exited with code -2147483648`;
- a command that exits with **non-zero (failure)** → everything works normally.

**Every successful command is reported as a failure.** Since sandbox/workspace
bootstrap itself runs commands that exit 0, the E2B backend cannot even hand out a
sandbox handle when the envd endpoint speaks JSON.

## Affected versions

- 2.0.1, 2.0.2, 2.0.2-subagent-bugfix — `E2bEnvdProcessClient.class` is byte-identical
across these three releases on Maven Central.
- Still present on `main` as of 2026-08-20 (checked at 643905e6).

## Environment where it bites

Any e2b-compatible envd that serves **JSON** frames. Concretely reproduced against
Alibaba Cloud FC's e2b-compatible service (`*.e2b.fc.aliyuncs.com`): FC's envd
rejects the PROTO codec with HTTP 400 and only speaks JSON, so the JSON parsing
path is mandatory there.

Server-side evidence that the endpoint is fine: the same request sent with `curl`
returns three complete frames —

```
{"event":{"start":{...}}}
{"event":{"data":{"stdout":"aGkK"}}}
{"event":{"end":{"exitCode":0}}}
```

The `end` frame is present with `exitCode: 0`; it is the client that discards it.

## Standalone reproduction (no network, unmodified Central artifact)

A minimal project with a single dependency on
`io.agentscope:agentscope-extensions-sandbox-e2b:2.0.1` feeds the exact envd JSON
frames above into `parseJsonStartResponse` via reflection. Output:

```
== Part 1: the proto3 fact the bug rests on (pure protobuf, no agentscope logic) ==
EndEvent builder after setField(exit_code, 0) -> getAllFields().isEmpty() = true
EndEvent builder after setField(exit_code, 1) -> getAllFields().isEmpty() = false

== Part 2: agentscope 2.0.1 parseJsonStartResponse on real envd JSON frames ==
envd frame: {"event":{"end":{"exitCode":0}}}
end event survives parsing : false <== END EVENT DROPPED BY THE CLIENT
exit code the caller sees : -2147483648 <== ExecException: Command exited with code -2147483648
envd frame: {"event":{"end":{"exitCode":1}}}
end event survives parsing : true
exit code the caller sees : 1
```

The `exitCode:1` control case shows the parser is otherwise fine — the failure is
specific to the success value `0`.

## Root cause — two spots, same mistake

**1. JSON → DynamicMessage assembly, `parseJsonStartResponse`:**

```java
JsonNode exitCodeNode = endNode.path("exitCode");
if (exitCodeNode.canConvertToInt()) {
endBuilder.setField(exitCodeField, exitCodeNode.intValue());
}
if (!endBuilder.getAllFields().isEmpty()) { // <-- bug
event.setField(processEventDesc.findFieldByName("end"), endBuilder.build());
}
```

`exit_code` is a proto3 singular int32 without explicit presence. After
`setField(exitCodeField, 0)`, `endBuilder.getAllFields()` is **empty** — that is
documented proto3 behavior, not a protobuf bug. So the presence of the `end` event
is judged from the wrong place, and the event is attached only when the exit code
is non-zero.

**2. Stream draining, `drainStartStream`:**

```java
if (pe.hasField(peEndF)) {
DynamicMessage end = (DynamicMessage) pe.getField(peEndF);
Descriptors.FieldDescriptor ec =
end.getDescriptorForType().findFieldByName("exit_code");
if (end.hasField(ec)) { // <-- bug
Object v = end.getField(ec);
exit = v instanceof Integer ? (Integer) v : ((Long) v).intValue();
}
}
```

Same presence trap on the read side: for `exit_code == 0`, `hasField()` is false and
`exit` keeps its `Integer.MIN_VALUE` initializer. Note this read-side guard is wrong
even for the binary PROTO path — proto3 wire format omits default-valued singular
scalars, so a server that legitimately encodes `exit_code = 0` also trips it.

## Suggested fix

**`parseJsonStartResponse`** — judge presence from the JSON, not from
`getAllFields()`:

```java
JsonNode exitCodeNode = endNode.path("exitCode");
boolean hasExitCode = exitCodeNode.canConvertToInt();
if (hasExitCode) {
endBuilder.setField(exitCodeField, exitCodeNode.intValue());
}
if (hasExitCode || !endBuilder.getAllFields().isEmpty()) {
event.setField(processEventDesc.findFieldByName("end"), endBuilder.build());
}
```

**`drainStartStream`** — read unconditionally; the proto3 default for an absent
field is `0`, which is exactly the right value here:

```java
Object v = end.getField(ec);
exit = v instanceof Integer ? (Integer) v : ((Long) v).intValue();
```

We have been running both changes as a local patch in production use against FC's
e2b service; happy to turn this into a PR if the approach looks right to you.

## Regression test suggestion

A unit test in the same package pins the behavior without any network:

```java
@Test
void anExitCodeOfZeroSurvivesJsonParsing() throws Exception {
E2bSandboxClientOptions options = new E2bSandboxClientOptions();
options.setApiKey("test-only-not-used");
E2bEnvdProcessClient client = new E2bEnvdProcessClient(options);

byte[] frame = "{\"event\":{\"end\":{\"exitCode\":0}}}"
.getBytes(StandardCharsets.UTF_8);
DynamicMessage response = /* invoke parseJsonStartResponse(frame) */;

Descriptors.FieldDescriptor eventField =
response.getDescriptorForType().findFieldByName("event");
assertThat(response.hasField(eventField)).isTrue();

DynamicMessage event = (DynamicMessage) response.getField(eventField);
Descriptors.FieldDescriptor endField =
event.getDescriptorForType().findFieldByName("end");
assertThat(event.hasField(endField)).isTrue(); // fails before the fix

DynamicMessage end = (DynamicMessage) event.getField(endField);
Object exitCode = end.getField(
end.getDescriptorForType().findFieldByName("exit_code"));
assertThat(((Number) exitCode).intValue()).isZero();
}
```

The negative control that documents the proto3 fact this bug rests on: build an
`EndEvent` via `DynamicMessage.newBuilder(endDesc)`, `setField(exit_code, 0)`, and
assert `builder.getAllFields()` is empty — while `setField(exit_code, 1)` makes it
non-empty. That is why the `getAllFields().isEmpty()` heuristic can never work for
this field.

Hướng dẫn đóng góp

Mở hướng dẫn đóng góp

Đánh giá

Issue này chưa được đánh giá.

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.