Skip to content

Commit de1bee4

Browse files
sky-xoclaude
andcommitted
fix: address PR review feedback
- Add error logging for malformed JSON in Codex tool arguments unmarshal - Add test case for malformed JSON arguments in transcript parsing - Add test cases for nil/empty ToolInput in convertCodexEntries and convertGeminiEntries Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 parent 337ae54 commit de1bee4

3 files changed

Lines changed: 134 additions & 1 deletion

File tree

internal/codex/transcript.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,9 @@ func parseEntry(data []byte) TranscriptEntry {
9494
// Parse arguments JSON string into map
9595
var toolInput map[string]interface{}
9696
if argsStr, ok := payload["arguments"].(string); ok && argsStr != "" {
97-
_ = json.Unmarshal([]byte(argsStr), &toolInput)
97+
if err := json.Unmarshal([]byte(argsStr), &toolInput); err != nil {
98+
fmt.Fprintf(os.Stderr, "codex: failed to unmarshal tool arguments for %s: %v\n", name, err)
99+
}
98100
}
99101

100102
return TranscriptEntry{

internal/codex/transcript_test.go

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,29 @@ func TestParseEntryFunctionCallWithArguments(t *testing.T) {
5555
}
5656
}
5757

58+
func TestParseEntryFunctionCallMalformedArguments(t *testing.T) {
59+
// Malformed JSON in arguments field - should handle gracefully (not panic)
60+
data := []byte(`{"type":"response_item","payload":{"type":"function_call","name":"shell_command","arguments":"{invalid json here"}}`)
61+
62+
entry := parseEntry(data)
63+
64+
// Should still return a valid tool entry
65+
if entry.Type != "tool" {
66+
t.Errorf("Type = %q, want %q", entry.Type, "tool")
67+
}
68+
if entry.ToolName != "shell_command" {
69+
t.Errorf("ToolName = %q, want %q", entry.ToolName, "shell_command")
70+
}
71+
// ToolInput should be nil when JSON parsing fails
72+
if entry.ToolInput != nil {
73+
t.Errorf("ToolInput = %v, want nil for malformed JSON", entry.ToolInput)
74+
}
75+
// Content should still be populated
76+
if entry.Content != "[tool: shell_command]" {
77+
t.Errorf("Content = %q, want %q", entry.Content, "[tool: shell_command]")
78+
}
79+
}
80+
5881
func TestParseEntryFunctionCallOutput(t *testing.T) {
5982
// Actual Codex format: type is "response_item", payload.type is "function_call_output"
6083
data := []byte(`{"type":"response_item","payload":{"type":"function_call_output","output":"Exit code: 0\nOutput: hello"}}`)

internal/tui/commands_test.go

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -177,3 +177,111 @@ func TestConvertGeminiEntriesRichFormatting(t *testing.T) {
177177
t.Errorf("ToolName() = %q, want %q", entries[0].ToolName(), "Read")
178178
}
179179
}
180+
181+
func TestConvertCodexEntriesNilToolInput(t *testing.T) {
182+
// Verify that nil ToolInput doesn't cause panic
183+
codexEntries := []codex.TranscriptEntry{
184+
{
185+
Type: "tool",
186+
Content: "[tool: shell_command]",
187+
ToolName: "shell_command",
188+
ToolInput: nil, // nil input (can happen with malformed JSON)
189+
},
190+
}
191+
192+
// Should not panic
193+
entries := convertCodexEntries(codexEntries)
194+
195+
if len(entries) != 1 {
196+
t.Fatalf("len(entries) = %d, want 1", len(entries))
197+
}
198+
199+
// Should still normalize tool name
200+
if entries[0].ToolName() != "Bash" {
201+
t.Errorf("ToolName() = %q, want %q", entries[0].ToolName(), "Bash")
202+
}
203+
204+
// ToolInput should be empty map, not nil
205+
toolInput := entries[0].ToolInput()
206+
if toolInput == nil {
207+
t.Error("ToolInput() = nil, want empty map")
208+
}
209+
}
210+
211+
func TestConvertCodexEntriesEmptyToolInput(t *testing.T) {
212+
// Verify that empty ToolInput works correctly
213+
codexEntries := []codex.TranscriptEntry{
214+
{
215+
Type: "tool",
216+
Content: "[tool: read_file]",
217+
ToolName: "read_file",
218+
ToolInput: map[string]interface{}{}, // empty map
219+
},
220+
}
221+
222+
// Should not panic
223+
entries := convertCodexEntries(codexEntries)
224+
225+
if len(entries) != 1 {
226+
t.Fatalf("len(entries) = %d, want 1", len(entries))
227+
}
228+
229+
// Should still normalize tool name
230+
if entries[0].ToolName() != "Read" {
231+
t.Errorf("ToolName() = %q, want %q", entries[0].ToolName(), "Read")
232+
}
233+
}
234+
235+
func TestConvertGeminiEntriesNilToolInput(t *testing.T) {
236+
// Verify that nil ToolInput doesn't cause panic
237+
geminiEntries := []gemini.TranscriptEntry{
238+
{
239+
Type: "tool",
240+
Content: "[tool: shell]",
241+
ToolName: "shell",
242+
ToolInput: nil, // nil input (can happen with malformed JSON)
243+
},
244+
}
245+
246+
// Should not panic
247+
entries := convertGeminiEntries(geminiEntries)
248+
249+
if len(entries) != 1 {
250+
t.Fatalf("len(entries) = %d, want 1", len(entries))
251+
}
252+
253+
// Should still normalize tool name
254+
if entries[0].ToolName() != "Bash" {
255+
t.Errorf("ToolName() = %q, want %q", entries[0].ToolName(), "Bash")
256+
}
257+
258+
// ToolInput should be empty map, not nil
259+
toolInput := entries[0].ToolInput()
260+
if toolInput == nil {
261+
t.Error("ToolInput() = nil, want empty map")
262+
}
263+
}
264+
265+
func TestConvertGeminiEntriesEmptyToolInput(t *testing.T) {
266+
// Verify that empty ToolInput works correctly
267+
geminiEntries := []gemini.TranscriptEntry{
268+
{
269+
Type: "tool",
270+
Content: "[tool: read_file]",
271+
ToolName: "read_file",
272+
ToolInput: map[string]interface{}{}, // empty map
273+
},
274+
}
275+
276+
// Should not panic
277+
entries := convertGeminiEntries(geminiEntries)
278+
279+
if len(entries) != 1 {
280+
t.Fatalf("len(entries) = %d, want 1", len(entries))
281+
}
282+
283+
// Should still normalize tool name
284+
if entries[0].ToolName() != "Read" {
285+
t.Errorf("ToolName() = %q, want %q", entries[0].ToolName(), "Read")
286+
}
287+
}

0 commit comments

Comments
 (0)