Skip to content

Python: Decode Ollama inline image data before serialization - #9459

Open
Jose Alvarez (jpalvarezl) wants to merge 2 commits into
mainfrom
jpalvarezl-ollama-file-input-review
Open

Jose Alvarez (jpalvarezl) wants to merge 2 commits into
mainfrom
jpalvarezl-ollama-file-input-review

Conversation

@jpalvarezl

Copy link
Copy Markdown
Member

Motivation & Context

Ollama inline image content is represented as a data URI, but the connector currently forwards the URI payload as a string. Because the Ollama image type supports multiple string semantics, the connector should decode inline content at its boundary so the image representation remains unambiguous and malformed inputs fail consistently.

Description & Review Guide

  • What are the major changes? Require Ollama image content to use a valid data:image/...;base64 URI, strictly decode the payload, and pass an Ollama image backed by bytes. Add regression coverage for valid images, malformed POSIX/Windows/UNC/relative path-shaped payloads, and base64-shaped payloads, including assertions that path helpers are not invoked.
  • What is the impact of these changes? Valid inline image data remains compatible. Missing base64 markers, invalid payloads, empty payloads, and mismatched data-URI media types now raise ChatClientInvalidRequestException before the Ollama request is sent.
  • What do you want reviewers to focus on? The validation boundary and compatibility for existing valid inline image inputs.

Related Issue

N/A - trivial change. This is a narrow correction that enforces the documented inline-image contract without changing valid input behavior.

AI Assistance

  • No material AI assistance was used.
  • This is an AI-assisted contribution. I reviewed, understood, and verified all submitted content and accept responsibility for it.

AI assistance details: Copilot assisted with the implementation, regression tests, and pull request description.

Contribution Checklist

  • The code builds clean without any errors or warnings
  • All unit tests pass, and I have added new tests where possible
  • The PR follows the Contribution Guidelines
  • This PR links to an agreed issue with no competing open PR, or the Related Issue section documents a trivial-change or repository-automation exception.
  • This is not a breaking change. If it is a breaking change, add the breaking change label (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The focused validation change and regression coverage have no identified blocking issues; project tests were not run during this review.

0 open findings

What changed in this PR

This PR makes Ollama inline-image serialization unambiguous by decoding validated data URIs into bytes before passing them to Ollama.

Changes:

  • Validate image data-URI structure and strictly decode base64 payloads.
  • Add regression tests for valid images, malformed inputs, and path-shaped payloads without filesystem access.
File Description
python/​packages/​ollama/​tests/​test_ollama_chat_client.py Adds validation and byte-backed serialization regression coverage.
python/​packages/​ollama/​agent_framework_ollama/​_chat_client.py Validates inline image URIs and constructs byte-backed Ollama images.

🧠 Review effort: Balanced


💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MAF Automated Review — Iteration 1

Result: Findings reported
Scope: full PR (1 commit(s)): 1c7254981bb8
Model: gpt-5.6-sol

Overview

The change moves inline Ollama images to a strict, byte-backed data-URI conversion boundary, preventing the SDK from interpreting image strings as filesystem paths. The added tests cover canonical data URIs, malformed and path-shaped payloads, pre-dispatch rejection, and the absence of filesystem access. However, decoding every supplied image without count or size limits adds a process-memory exhaustion path before network dispatch.

Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
1 verified finding remained after source verification (1 medium) across 1 file. Details are attached to the affected lines below.

Affected areas: python/packages/ollama/agent_framework_ollama/_chat_client.py

Comment thread python/packages/ollama/agent_framework_ollama/_chat_client.py
@github-code-quality

Copy link
Copy Markdown
Contributor

Code coverage overview

Languages: Python

Python / code-coverage/python

main 9127b89 jpalvarezl-ollama-fi... 221b92b +/-
92% 92% 0%

ℹ️ Calculations represent line coverage. Results may change as calculations are updated.

This branch was successfully deployed

1 active deployment
github-app-auth — 221b92be Deployed Oct 16, 2026 by jpalvarezl via add_label #25618
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

python Usage: [Issues, PRs], Target: Python

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants