harden codex ws/compat paths from PR #86 review

Resolve CodeRabbit review findings:
- tool-call ID canonicalization: remap provisional item-only IDs onto the
  learned call_id so args-before-added no longer fragments one tool call
  into two (openairesponsestream.RemapToolCallID + stream.go)
- parameterless object tool schemas inject additionalProperties:false and
  required:[] so strict:true is accepted by the Responses API
- no-tools payloads omit parallel_tool_calls (tool-protocol signal leak)
- authjson rejects non-regular token files (FIFO/device)
- managed oauth files count skipped symlinks for an accurate error
- codexclientcompat drops tool-use instructions when no tools; add nil guard
- streamdebug Close always emits the terminal debug record with correct status
- gemini handler trims ALegID consistently across EnsureCallDiag calls
- openailegacy decode drops dead cp=nil assignment
- diag/streamdebug use sort.Strings instead of duplicated insertion sorts
- ws dialer documents custom-RoundTripper fallback; drop dead close branch
- refbackend SSE event name derived from frame type; WS tests add read deadlines

Co-authored-by: Cursor <cursoragent@cursor.com>

Author: Mateusz

internal/core/diag/debug_summary.go

@@ -3,6 +3,7 @@ package diag
 import (
 	"log/slog"
 	"os"
+	"sort"
 	"strconv"
 	"strings"
 	"sync"
@@ -35,11 +36,7 @@ func StableCounts(counts map[string]int) []string {
 	for k := range counts {
 		keys = append(keys, k)
 	}
-	for i := 1; i < len(keys); i++ {
-		for j := i; j > 0 && keys[j] < keys[j-1]; j-- {
-			keys[j], keys[j-1] = keys[j-1], keys[j]
-		}
-	}
+	sort.Strings(keys)
 	out := make([]string, 0, len(keys))
 	for _, k := range keys {
 		out = append(out, k+"="+strconv.Itoa(counts[k]))

internal/plugins/backends/openaicodex/authjson.go

@@ -139,6 +139,9 @@ func checkTokenFilePermissions(path string) error {
 	if err != nil {
 		return nil // let the caller's ReadFile produce the canonical not-exist error
 	}
+	if !info.Mode().IsRegular() {
+		return fmt.Errorf("%s: token file %q is not a regular file", ID, path)
+	}
 	if info.Mode().Perm()&0o077 != 0 {
 		return fmt.Errorf("%s: token file %q is group/other accessible (mode %o); expected 0600", ID, path, info.Mode().Perm())
 	}

internal/plugins/backends/openaicodex/managed_oauth_files.go

@@ -25,6 +25,7 @@ func loadManagedAccounts(dir string, filter []string) ([]managedAccount, error)
 		if ent.Type()&os.ModeSymlink != 0 {
 			// security: skip symlinked account files so a planted symlink cannot
 			// read targets outside the managed-oauth storage directory.
+			skipped++
 			continue
 		}
 		path := filepath.Join(dir, ent.Name())

internal/plugins/backends/openaicodex/payload.go

@@ -98,7 +98,7 @@ func PayloadForCall(call *lipapi.Call, cand routing.AttemptCandidate, cfg Config
 	}
 	if call.Options.ParallelToolCalls != nil {
 		p.ParallelToolCalls = call.Options.ParallelToolCalls
-	} else if p.ParallelToolCalls == nil {
+	} else if p.ParallelToolCalls == nil && len(call.Tools) > 0 {
 		v := false
 		p.ParallelToolCalls = &v
 	}

internal/plugins/backends/openaicodex/payload_test.go

@@ -217,6 +217,9 @@ func TestPayloadForCall_noToolsOmitsToolChoice(t *testing.T) {
 	if strings.Contains(string(raw), `"tool_choice"`) {
 		t.Fatalf("no-tools payload must omit tool_choice: %s", raw)
 	}
+	if strings.Contains(string(raw), `"parallel_tool_calls"`) {
+		t.Fatalf("no-tools payload must omit parallel_tool_calls: %s", raw)
+	}
 }
 
 func TestPayloadForCall_modelInstructionsReasoningTemperatureToolsMultimodal(t *testing.T) {
@@ -814,6 +817,44 @@ func TestPayloadForCall_strictCompatibleToolSchemaUsesStrictTrue(t *testing.T) {
 	}
 }
 
+func TestPayloadForCall_parameterlessObjectSchemaGetsAdditionalPropertiesAndRequired(t *testing.T) {
+	t.Parallel()
+	strict, raw := codexToolStrict(t, `{"type":"object"}`)
+	if !strict {
+		t.Fatalf("parameterless object schema must stay strict=true after normalization: %s", raw)
+	}
+	var decoded struct {
+		Tools []struct {
+			Parameters map[string]any `json:"parameters"`
+		} `json:"tools"`
+	}
+	if err := json.Unmarshal(raw, &decoded); err != nil {
+		t.Fatal(err)
+	}
+	if len(decoded.Tools) != 1 {
+		t.Fatalf("tools: %v", decoded.Tools)
+	}
+	params := decoded.Tools[0].Parameters
+	if ap, ok := params["additionalProperties"].(bool); !ok || ap {
+		t.Fatalf("parameterless object must have additionalProperties:false: %#v", params)
+	}
+	req, ok := params["required"].([]any)
+	if !ok || len(req) != 0 {
+		t.Fatalf("parameterless object must have required:[]: %#v", params)
+	}
+}
+
+func TestPayloadForCall_parameterlessObjectWithAdditionalPropertiesTrueIsStrictFalse(t *testing.T) {
+	t.Parallel()
+	// A parameterless object that explicitly allows additional properties is not
+	// strict-compatible; it must be sent strict:false so the upstream does not
+	// reject it.
+	strict, _ := codexToolStrict(t, `{"type":"object","additionalProperties":true}`)
+	if strict {
+		t.Fatal("parameterless object with additionalProperties:true must use strict=false")
+	}
+}
+
 func TestPayloadForCall_composedLooseToolSchemaUsesStrictFalse(t *testing.T) {
 	t.Parallel()
 	cases := []struct {

internal/plugins/backends/openaicodex/stream.go

@@ -219,6 +219,7 @@ func (m *codexEventMapper) handleOutputItemDone(data string) error {
 		return nil
 	}
 	m.rememberToolCallID(ev.Item.ID, ev.Item.CallID)
+	m.remapProvisionalToolCall(ev.Item.ID, ev.Item.CallID)
 	if item, ok := outputFunctionCallInputItem(ev.Item.Type, ev.Item.ID, ev.Item.CallID, ev.Item.Name, ev.Item.Arguments); ok {
 		m.outputItems = append(m.outputItems, item)
 	}
@@ -296,6 +297,7 @@ func (m *codexEventMapper) handleOutputItemAdded(data string) error {
 		return nil
 	}
 	m.rememberToolCallID(ev.Item.ID, ev.Item.CallID)
+	m.remapProvisionalToolCall(ev.Item.ID, ev.Item.CallID)
 	return m.mapper.ToolCallAdded(codexCanonicalToolCallID(ev.Item.ID, ev.Item.CallID), ev.Item.Name)
 }
 
@@ -341,17 +343,40 @@ func (m *codexEventMapper) rememberToolCallID(itemID, callID string) {
 		return
 	}
 	m.toolCallIDs[itemID] = callID
+	// Once the real call_id is known, drop the provisional flag so toolCallID
+	// stops returning the item-only ID and all subsequent events canonicalize
+	// onto the call_id.
+	delete(m.provisional, itemID)
 }
 
-func (m *codexEventMapper) toolCallID(itemID, callID string) string {
+// remapProvisionalToolCall moves any mapper state buffered under the
+// provisional item-only ID onto the real call_id once it is learned. Without
+// this, argument deltas that arrived before output_item.added stay buffered
+// under the item ID while ToolCallAdded targets the call_id, fragmenting one
+// logical tool call into two.
+func (m *codexEventMapper) remapProvisionalToolCall(itemID, callID string) {
 	itemID = strings.TrimSpace(itemID)
 	callID = strings.TrimSpace(callID)
-	if itemID != "" && m.provisional[itemID] {
-		return itemID
+	if itemID == "" || callID == "" || callID == itemID {
+		return
 	}
+	m.mapper.RemapToolCallID(itemID, callID)
+}
+
+func (m *codexEventMapper) toolCallID(itemID, callID string) string {
+	itemID = strings.TrimSpace(itemID)
+	callID = strings.TrimSpace(callID)
+	// Prefer a learned call_id over the provisional item-only ID so deltas and
+	// completion events resolve to the same canonical ID as output_item.added.
 	if callID == "" {
 		callID = strings.TrimSpace(m.toolCallIDs[itemID])
 	}
+	if callID != "" {
+		return codexCanonicalToolCallID(itemID, callID)
+	}
+	if itemID != "" && m.provisional[itemID] {
+		return itemID
+	}
 	if callID == "" && itemID != "" {
 		m.provisional[itemID] = true
 	}

internal/plugins/backends/openaicodex/stream_internal_test.go

@@ -403,21 +403,36 @@ func TestHandleData_toolArgsBeforeAddedWaitForToolName(t *testing.T) {
 	}
 
 	events := stream.DrainPending(&s.mapper.pending)
-	var toolStarted *lipapi.Event
+	var startedCount int
+	var startedID, startedName string
 	var args strings.Builder
+	var finishedIDs []string
 	for i := range events {
 		ev := events[i]
 		switch ev.Kind {
 		case lipapi.EventToolCallStarted:
-			toolStarted = &ev
+			startedCount++
+			startedID = ev.ToolCallID
+			startedName = ev.ToolName
 		case lipapi.EventToolCallArgsDelta:
 			args.WriteString(ev.Delta)
+		case lipapi.EventToolCallFinished:
+			finishedIDs = append(finishedIDs, ev.ToolCallID)
 		}
 	}
-	if toolStarted == nil || toolStarted.ToolName != "read" {
-		t.Fatalf("tool started = %+v, want name read; events=%+v", toolStarted, events)
+	if startedCount != 1 {
+		t.Fatalf("tool call started count = %d, want 1 (no provisional/real duplicate); events=%+v", startedCount, events)
+	}
+	if startedID != "call_late" {
+		t.Fatalf("tool call started id = %q, want call_late; events=%+v", startedID, events)
+	}
+	if startedName != "read" {
+		t.Fatalf("tool started name = %q, want read; events=%+v", startedName, events)
 	}
 	if got := args.String(); got != `{"filePath":` {
-		t.Fatalf("args = %q", got)
+		t.Fatalf("args = %q, want incremental delta preserved after remap onto call_id", got)
+	}
+	if len(finishedIDs) != 1 || finishedIDs[0] != "call_late" {
+		t.Fatalf("tool call finished ids = %v, want [call_late]; events=%+v", finishedIDs, events)
 	}
 }

internal/plugins/backends/openaicodex/toolschema.go

@@ -5,16 +5,18 @@ package openaicodex
 // additionalProperties:false and list all of its properties in required. Schemas
 // that do not comply must be sent with strict:false, otherwise the upstream
 // rejects the request (e.g. "additionalProperties is required to be supplied
-// and to be false"). Parameterless schemas (no properties) are treated as
-// compatible so they keep strict:true. The check is conservative: when in
-// doubt it returns false, which only relaxes strict mode (safe) and never
+// and to be false"). Parameterless object schemas are normalized by
+// addStrictAdditionalProperties to include additionalProperties:false (and an
+// empty required list) so they remain strict-compatible instead of leaking a
+// strict:true tool that the upstream rejects. The check is conservative: when
+// in doubt it returns false, which only relaxes strict mode (safe) and never
 // causes an upstream rejection.
 func isStrictCompatibleSchema(schema map[string]any) bool {
 	if hasRef(schema) || !strictCompatibleCompositions(schema) {
 		return false
 	}
-	props, _ := schema["properties"].(map[string]any)
-	if len(props) == 0 {
+	if !isObjectSchema(schema) {
+		// Non-object root (array/primitive): only its array items must comply.
 		return strictCompatibleArrayItems(schema)
 	}
 	ap, ok := schema["additionalProperties"]
@@ -24,6 +26,7 @@ func isStrictCompatibleSchema(schema map[string]any) bool {
 	if asBool, ok := ap.(bool); !ok || asBool {
 		return false
 	}
+	props, _ := schema["properties"].(map[string]any)
 	reqRaw, _ := schema["required"].([]any)
 	required := make(map[string]bool, len(reqRaw))
 	for _, r := range reqRaw {
@@ -51,17 +54,37 @@ func normalizeToolSchemaForCodex(schema map[string]any) (map[string]any, bool) {
 	return schema, isStrictCompatibleSchema(schema)
 }
 
+// isObjectSchema reports whether a node is a JSON object schema: either it
+// declares type "object" or it carries a non-empty properties map. Empty
+// properties maps without an explicit object type are not treated as objects so
+// that a truly empty schema ({}) is left untouched.
+func isObjectSchema(node map[string]any) bool {
+	if t, _ := node["type"].(string); t == "object" {
+		return true
+	}
+	props, ok := node["properties"].(map[string]any)
+	return ok && len(props) > 0
+}
+
 func addStrictAdditionalProperties(v any) {
 	switch x := v.(type) {
 	case map[string]any:
 		for k, child := range x {
 			addStrictAdditionalProperties(child)
 			x[k] = child
 		}
-		if props, ok := x["properties"].(map[string]any); ok && len(props) > 0 {
+		if isObjectSchema(x) {
 			if _, ok := x["additionalProperties"]; !ok {
 				x["additionalProperties"] = false
 			}
+			// Parameterless objects must also carry an explicit required:[] for the
+			// Responses API strict mode; inject it only when no properties and no
+			// required are already declared.
+			if props, _ := x["properties"].(map[string]any); len(props) == 0 {
+				if _, ok := x["required"]; !ok {
+					x["required"] = []any{}
+				}
+			}
 		}
 	case []any:
 		for i, child := range x {

internal/plugins/backends/openaicodex/ws.go

@@ -253,6 +253,10 @@ func newWSDialer(client *http.Client) *websocket.Dialer {
 				d.TLSClientConfig = &tls.Config{MinVersion: tls.VersionTLS12}
 			}
 		}
+		// When client.Transport is a custom RoundTripper (e.g. instrumentation)
+		// rather than *http.Transport, proxy/TLS settings cannot be introspected
+		// generically, so the WS dialer falls back to default networking. This
+		// differs from the HTTPS path that uses the same client.
 	}
 	return d
 }
@@ -568,9 +572,6 @@ func readFirstNonEmptyWSMessage(ctx context.Context, conn *websocket.Conn, timeo
 			if ctx != nil && ctx.Err() != nil {
 				return nil, ctx.Err()
 			}
-			if websocket.IsCloseError(err, websocket.CloseNormalClosure, websocket.CloseGoingAway) {
-				return nil, newWSTransportError(fmt.Errorf("read websocket: %w", err))
-			}
 			return nil, newWSTransportError(fmt.Errorf("read websocket: %w", err))
 		}
 		if len(strings.TrimSpace(string(data))) > 0 {

internal/plugins/backends/protocols/openairesponsestream/mapper.go

@@ -265,6 +265,34 @@ func (m *Mapper) flushPendingToolArgs(id string) error {
 	return nil
 }
 
+// RemapToolCallID consolidates tool-call state buffered under oldID onto newID.
+// It is used when a tool call's real call_id is learned after argument deltas
+// were already buffered under the provisional item-only ID, so pending args and
+// started/arg-delta/finished flags all move onto the canonical ID instead of
+// fragmenting into two tool calls. Remapping is a no-op when no state exists
+// under oldID.
+func (m *Mapper) RemapToolCallID(oldID, newID string) {
+	if oldID == "" || newID == "" || oldID == newID {
+		return
+	}
+	if deltas, ok := m.pendingToolArgs[oldID]; ok {
+		m.pendingToolArgs[newID] = append(m.pendingToolArgs[newID], deltas...)
+		delete(m.pendingToolArgs, oldID)
+	}
+	if m.toolCallStarted[oldID] {
+		m.toolCallStarted[newID] = true
+		delete(m.toolCallStarted, oldID)
+	}
+	if m.toolCallArgDeltas[oldID] {
+		m.toolCallArgDeltas[newID] = true
+		delete(m.toolCallArgDeltas, oldID)
+	}
+	if m.toolCallFinished[oldID] {
+		m.toolCallFinished[newID] = true
+		delete(m.toolCallFinished, oldID)
+	}
+}
+
 // EmitOutputMediaFromResponse maps assistant message media in a completed Responses payload.
 func EmitOutputMediaFromResponse(m *Mapper, resp responses.Response) error {
 	for _, item := range resp.Output {