Skip to content

Commit e522856

Browse files
author
SqlRush
committed
fix(exec): emit turn.failed (not turn.completed) when a turn ends in a critical API error
Error-path differential parity (TestParityTurnError) drives both the real codex binary and codexgo through a fake /v1/responses endpoint that returns a non-retryable HTTP 400. codex terminates the exec --json stream with a `turn.failed` event; codexgo was emitting `turn.completed`. collectTurnComplete now checks lastCritical and emits TurnFailedEvent on the error path. The terminal event type, exit code, and event-type sequence now match codex. The error *message* text still differs (codexgo leaks internal `core: ...` wrapping vs codex's clean upstream body) — documented as a tracked deviation (DEVIATIONS.md, spec 34).
1 parent dc3a8bd commit e522856

3 files changed

Lines changed: 141 additions & 0 deletions

File tree

DEVIATIONS.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,8 @@ Categories: `format` (on-disk), `protocol` (wire), `behavioral`, `cosmetic`.
3232
| 04 config | behavioral | layer-state/fingerprint/origins, macOS MDM managed prefs, cloud-requirements, git-trust project loading, keymap parsing carried as opaque TOML trees / omitted | large peripheral surface; load/merge/validate + typed schema are faithful | accepted |
3333
| 32 app-server-protocol | behavioral | ts-rs/schemars schema-export generator deferred; a few cross-area fields carried as `json.RawMessage` | the runtime types/methods are faithful; the codegen tool is non-runtime | accepted |
3434

35+
| 34 exec | behavioral | On an API error, the error *message* text differs: codex surfaces the clean upstream error body; codexgo leaks internal wrapping (`core: model stream failed: …`). The event shape, exit code, and terminal `turn.failed` event all match (verified by `TestParityTurnError`). | error-message cleanup needs threading the upstream API error through core without the `%w` chain prefix; tracked | review |
36+
3537
> Entries are appended as each spec lands and reports its deviations.
3638
> **`review (must finish)`** items are genuine gaps to close before claiming the
3739
> corresponding spec complete — they are not permanent accepted deviations.

internal/exec/processor.go

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -309,6 +309,15 @@ func (p *JSONLProcessor) collectTurnComplete(n *protocol.TurnCompleteEvent) Coll
309309
p.finalMessage = &msg
310310
}
311311
p.emitFinalOnExit = true
312+
// When a critical error occurred during the turn, codex emits a terminal
313+
// turn.failed (carrying the error) rather than turn.completed. Mirror that.
314+
if p.lastCritical != nil {
315+
events = append(events, ThreadEvent{
316+
Kind: ThreadEventKindTurnFailed,
317+
TurnFailed: &TurnFailedEvent{Error: *p.lastCritical},
318+
})
319+
return CollectedThreadEvents{Events: events, Status: StatusInitiateShutdown}
320+
}
312321
events = append(events, ThreadEvent{
313322
Kind: ThreadEventKindTurnCompleted,
314323
TurnCompleted: &TurnCompletedEvent{Usage: p.usageFromLastTotal()},
Lines changed: 130 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,130 @@
1+
package paritytest
2+
3+
// Error-path parity: when the model endpoint returns a non-retryable API error,
4+
// codexgo must surface the SAME terminal error behavior as codex — same exit code
5+
// and the same kind of error event in the exec --json stream. Robust differential:
6+
// asserts codexgo == codex regardless of the exact mapping, and flags a genuine
7+
// divergence if they differ. Env-gated on CODEX_PARITY_BIN.
8+
9+
import (
10+
"bytes"
11+
"io"
12+
"net/http"
13+
"net/http/httptest"
14+
"os"
15+
"os/exec"
16+
"strings"
17+
"testing"
18+
)
19+
20+
// newErrorServer returns a server that answers /responses with a non-retryable
21+
// HTTP 400 + an OpenAI-style error body (4xx other than 429 is not retried, so the
22+
// turn fails deterministically on the first request).
23+
func newErrorServer(t *testing.T) *httptest.Server {
24+
t.Helper()
25+
return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
26+
if r.Method == http.MethodPost && strings.HasSuffix(strings.TrimSuffix(r.URL.Path, "/"), "responses") {
27+
_, _ = io.Copy(io.Discard, r.Body)
28+
w.Header().Set("Content-Type", "application/json")
29+
w.WriteHeader(http.StatusBadRequest)
30+
_, _ = io.WriteString(w, `{"error":{"message":"parity boom","type":"invalid_request_error","code":"bad_parity"}}`)
31+
return
32+
}
33+
w.Header().Set("Content-Type", "application/json")
34+
_, _ = io.WriteString(w, `{"models":[]}`)
35+
}))
36+
}
37+
38+
// runErrorTurn runs the binary's exec --json against srv, tolerating (and
39+
// returning) a non-zero exit code instead of failing the test.
40+
func runErrorTurn(t *testing.T, who, bin, serverURL string) (string, int) {
41+
t.Helper()
42+
home := t.TempDir()
43+
work := t.TempDir()
44+
writeToolCallConfig(t, home, serverURL)
45+
46+
cmd := exec.Command(bin, "exec", "--json", "--skip-git-repo-check", "-C", work, toolTurnPrompt)
47+
cmd.Env = append(os.Environ(),
48+
"CODEX_HOME="+home,
49+
fakeEnvKey+"="+fakeAPIKey,
50+
"OPENAI_API_KEY=",
51+
"CODEX_API_KEY=",
52+
"CODEX_ACCESS_TOKEN=",
53+
)
54+
cmd.Stdin = bytes.NewReader(nil)
55+
var stdout, stderr bytes.Buffer
56+
cmd.Stdout = &stdout
57+
cmd.Stderr = &stderr
58+
err := cmd.Run()
59+
code := 0
60+
if ee, ok := err.(*exec.ExitError); ok {
61+
code = ee.ExitCode()
62+
} else if err != nil {
63+
t.Fatalf("%s exec (error turn) failed to run: %v", who, err)
64+
}
65+
return stdout.String(), code
66+
}
67+
68+
// TestParityTurnError asserts codexgo surfaces the same exit code and the same
69+
// normalized error event(s) as codex when the API returns a non-retryable error.
70+
func TestParityTurnError(t *testing.T) {
71+
refBin := referenceBin(t)
72+
cgoBin := buildCodexgo(t)
73+
74+
run := func(who, bin string) ([]map[string]any, int) {
75+
srv := newErrorServer(t)
76+
defer srv.Close()
77+
out, code := runErrorTurn(t, who, bin, srv.URL)
78+
return parseJSONL(t, who, out), code
79+
}
80+
refEvents, refCode := run("codex", refBin)
81+
cgoEvents, cgoCode := run("codexgo", cgoBin)
82+
83+
// Both must fail with a non-zero exit, and the SAME code.
84+
if refCode == 0 {
85+
t.Fatalf("codex unexpectedly exited 0 on API error")
86+
}
87+
if cgoCode != refCode {
88+
t.Errorf("exit code mismatch on API error: codex=%d codexgo=%d", refCode, cgoCode)
89+
}
90+
91+
// Both must emit at least one terminal error event.
92+
hasError := func(evs []map[string]any) bool {
93+
for _, ev := range evs {
94+
if tag, _ := ev["type"].(string); strings.Contains(tag, "error") {
95+
return true
96+
}
97+
}
98+
return false
99+
}
100+
if !hasError(refEvents) {
101+
t.Fatalf("codex emitted no error event:\n%v", refEvents)
102+
}
103+
if !hasError(cgoEvents) {
104+
t.Errorf("codexgo emitted no error event (codex did):\n%v", cgoEvents)
105+
}
106+
107+
// Both must emit the SAME terminal event type. codex emits turn.failed on a
108+
// failed turn; codexgo previously emitted turn.completed (now fixed).
109+
if a, b := terminalEventType(refEvents), terminalEventType(cgoEvents); a != b {
110+
t.Errorf("terminal event mismatch: codex=%q codexgo=%q", a, b)
111+
} else if a != "turn.failed" {
112+
t.Errorf("expected terminal turn.failed on API error, got %q", a)
113+
}
114+
if a, b := eventTypeSequence(refEvents), eventTypeSequence(cgoEvents); strings.Join(a, ",") != strings.Join(b, ",") {
115+
t.Errorf("error-turn event-type sequence mismatch:\n codex: %v\n codexgo: %v", a, b)
116+
}
117+
// NOTE (documented deviation, docs/PARITY.md): the error *message* text differs
118+
// — codex surfaces the clean upstream error body, while codexgo currently leaks
119+
// internal wrapping ("core: model stream failed: ..."). The event SHAPE, exit
120+
// code, and terminal turn.failed all match; the message-text cleanup is tracked.
121+
}
122+
123+
// terminalEventType returns the "type" of the last event in the stream.
124+
func terminalEventType(events []map[string]any) string {
125+
if len(events) == 0 {
126+
return ""
127+
}
128+
t, _ := events[len(events)-1]["type"].(string)
129+
return t
130+
}

0 commit comments

Comments
 (0)