fix(providers): use non-descriptive placeholder when stripping images - #4401
Conversation
The image-strip fallback (triggered when a model errors on image input) replaced image_url blocks with [image: <path>] or [image omitted]. Both read like a live, available image to the LLM, causing it to: 1. hallucinate about image contents it never received 2. attempt read_file on the leaked server path 3. expose internal file paths to the model Replace with an explicit '[Image not delivered to model — do not describe or reference it]' placeholder that tells the LLM the image was stripped. Fixes HKUDS#4345
…ve text The _strip_image_content methods now use a fixed non-descriptive placeholder instead of path-derived text. Update the 3 existing test_provider_retry assertions to match the new placeholder format.
chengyongru
left a comment
There was a problem hiding this comment.
Automated PR review by nanobot. This is not a human maintainer review or approval.
Summary
This PR replaces the file-path-leaking placeholder [image: <path>] / [image omitted] with a static, non-descriptive message [Image not delivered to model — do not describe or reference it] in both _strip_image_content and _strip_image_content_inplace.
What is done well
- Correctly scoped: only touches the fallback path in
base.py. Theimage_placeholder_texthelper remains in use elsewhere (loop.py,session/manager.py) where path info is intentionally preserved. - Removes the now-unused import from
base.py— no dead references. - Test coverage is solid: 8 new tests covering both methods, multi-image handling, no-image return, missing
_meta, and in-place mutation semantics. - Existing
test_provider_retry.pyassertions updated to match the new placeholder text.
Observations
- This is a genuine security improvement: the old placeholder leaked server-side file paths into LLM context, which could cause the model to hallucinate image contents or attempt to
read_fileon those paths. - No blocking concerns. The change is small, focused, and well-tested.
| { | ||
| "role": "user", | ||
| "content": [ | ||
| {"type": "text", "text": "Look at these:"}, |
There was a problem hiding this comment.
This assertion [image not in placeholder.lower() or not delivered in placeholder.lower()is always True for the new placeholder, since the first clause is always satisfied. Consider simplifying to justassert "not delivered" in placeholder.lower()` for clarity. Non-blocking.
There was a problem hiding this comment.
Addressed in 3d2f422. I tightened the placeholder assertions to require not delivered directly, including the nearby equivalent checks, so the tests now state the intended contract without the always-satisfied alternative.
There was a problem hiding this comment.
LGTM. This fixes the image-strip fallback behavior in the right place: when image inputs are removed before retrying, the replacement text no longer leaks local paths or implies the model actually saw the image. The regression coverage is nicely focused around both strip paths.
I pushed two tiny follow-up commits: one to fix the ruff import-format issue in the new test file, and one to address chengyongru’s inline comment by tightening the placeholder assertions to require not delivered directly.
Validated locally after those fixes:
- python -m pytest tests/providers/test_provider_retry.py tests/providers/test_strip_image_content.py -q
- python -m ruff check nanobot/providers/base.py tests/providers/test_provider_retry.py tests/providers/test_strip_image_content.py
GitHub Actions is running again for the latest test-only follow-up commit; please wait for the matrix before merging.
Summary
When a model returns a non-transient error on
image_urlinput, the provider base strips image blocks and retries as text. The current fallback placeholder[image: <path>](or[image omitted]) reads like a live, available image to the LLM, causing it to hallucinate about contents it never received, attemptread_fileon the leaked server path, and expose internal file paths.This PR replaces the placeholder with an explicit
[Image not delivered to model — do not describe or reference it]message in both_strip_image_contentand_strip_image_content_inplace.Linked Issue
Fixes #4345
Type of Change
Changes
nanobot/providers/base.py: Replaceimage_placeholder_text(path, empty="[image omitted]")with a fixed[Image not delivered to model — do not describe or reference it]placeholder in both_strip_image_contentand_strip_image_content_inplaceimage_placeholder_textimport frombase.pytests/providers/test_strip_image_content.py: 8 new tests covering both methods — verifies no path leakage, correct placeholder text, multi-image handling, no-image return, and in-place mutationHow to Test
All 8 tests verify that:
_metaare handledChecklist