pr-141-review.md

Code Review: PR #141 — fix: adjust content parsing

Resolves: #137 — Content parsing issue with OpenCode / Ollama

Overview

This PR fixes a json.Unmarshal failure when Ollama's content field is an array of content blocks ([{"type": "text", "text": "..."}]) instead of a plain string. The approach:

1. Changes ollamaMessage.Content to json:"-" and adds ContentRaw any tagged as json:"content" to capture either format
2. Adds convertRawContent() to normalize both forms into a string
3. Extracts extractTextFromContent from pkg/backfill into pkg/utils for reuse
4. Adds good test coverage for ExtractTextFromContent

What's Good

• Clean separation — The ContentRaw any / Content string split is a reasonable way to handle polymorphic JSON without a custom UnmarshalJSON
• Reuse — Extracting ExtractTextFromContent to pkg/utils and sharing it with backfill is the right call
• Test coverage — Tests cover empty, irrelevant, matching, and mixed content blocks
• Minimal blast radius — Changes are scoped to Ollama provider + the shared utility

Issues

Bug: convertRawContent won't match []map[string]any after json.Unmarshal

This is the critical issue. When json.Unmarshal decodes into any, arrays of objects become []any (where each element is map[string]any), not []map[string]any. The type assertion at ollama.go:203 will never succeed for real deserialized payloads:

// This won't match JSON-unmarshalled data:
if slice, ok := contentRaw.([]map[string]any); ok {

It needs to handle []any and assert each element individually, or adjust ExtractTextFromContent to accept []any. This should be verified by adding an integration-style test that unmarshals actual JSON with array content and runs it through ParseRequest.

Minor: ollamaToolCall.ID changed to omitempty

The json:"id,omitempty" change on ollamaToolCall (types.go:29) is unrelated to the content parsing fix. It should either be called out in the PR description or split into a separate commit. It's fine on its own — Ollama tool calls may not always have IDs — but it's a silent behavioral change.

Minor: No test for convertRawContent or ParseRequest with array content

The tests cover ExtractTextFromContent well, but there are no tests for convertRawContent itself or for ParseRequest/ParseResponse with the array content format. Given the type assertion bug above, a round-trip test through ParseRequest with real JSON would have caught this.

Suggestion

The fix for the critical issue would look something like:

func convertRawContent(contentRaw any) string {
    if s, ok := contentRaw.(string); ok {
        return s
    }
    if slice, ok := contentRaw.([]any); ok {
        blocks := make([]map[string]any, 0, len(slice))
        for _, item := range slice {
            if m, ok := item.(map[string]any); ok {
                blocks = append(blocks, m)
            }
        }
        return utils.ExtractTextFromContent(blocks)
    }
    return ""
}

Summary

The approach and structure are solid, but the []map[string]any type assertion is a bug that means array-format content will silently be dropped (returning "") on real JSON payloads. This needs to be fixed and covered by a test that unmarshals actual JSON before merging.