diff --git a/docs/api.md b/docs/api.md index b97229c..47967f3 100644 --- a/docs/api.md +++ b/docs/api.md @@ -844,7 +844,7 @@ |-----------|----------|-------------| | `type` | yes | `"message"` (a user turn) or `"compact"` (force context compression now — server streams `compression_started` → `context_compressed`, no `stream_start`; rejected if a run is active) | | `content` | yes | Message text (non-empty), required for `"message"` | -| `images` | no | Base64 image list. Max 8 images, 50 MB total payload. Both raw base64 and `data:image/...;base64,...` are accepted — server strips the prefix | +| `images` | no | Base64 image list. Max 8 images, 50 MB total payload. Both raw base64 and `data:image/...;base64,...` are accepted — server strips the prefix. Each image is written into the session directory as `image_N.` and the notice naming those paths goes into the hidden context copy; see [sessions.md](sessions.md#images-pasted-into-the-chat) | | `files` | no | Files uploaded via `POST /sessions/{id}/files`. Server appends their paths to the message content | --- diff --git a/docs/sessions.md b/docs/sessions.md index 1bb3b70..6367bac 100644 --- a/docs/sessions.md +++ b/docs/sessions.md @@ -174,6 +174,28 @@ This lets the agent use `filesystem` or `code_exec` to access the files. +### Images pasted into the chat + +Images sent in the `images` field of a chat message (a paste, or the client's image button) are written into the same session directory, under `image_N.`. The extension comes from the bytes themselves, never from a client-supplied name, and the bytes are stored exactly as they arrived — a PNG keeps its alpha channel, so re-encoding to JPEG never happens. + +The numbering runs across a session, not per format: the first paste is `image_1.png`, a later JPEG is `image_2.jpg`. Sending the same picture twice reuses the file already holding those bytes rather than writing a copy. + +A notice stating where they landed is appended to the hidden context copy of the message (the copy the LLM sees, not the one the user sees): +``` +[2 images attached inline — already in your context, no extra loading needed. +On disk: session_files/{id}/image_1.png, session_files/{id}/image_2.jpg] +``` + +Images past the context budget are not sent to the model at all, so the notice names them for the `image_view` tool instead: +``` +[3 images attached inline — 1 are in your context, the rest were not loaded; +open them with image_view: session_files/{id}/image_2.png, session_files/{id}/image_3.png] +``` + +Before this, a pasted image existed only in the transcript: an agent that had been told `session_files/{id}/` exists went looking there for "the attached images", found nothing, and spent turns on fruitless `find` calls. An image that cannot be stored (no writable directory) is logged as `agent.image_save_failed` and skipped — it never costs the turn. + +Hidden turns (`hidden=True`, e.g. form submissions) write nothing: they carry machine payloads, not pastes. + `workspace/` is separate from session files. Use `workspace/` for persistent private working files and `session_files/{session_id}/` for files that belong to the current chat. Two tools expose session files to the user: diff --git a/manuals/filesystem.md b/manuals/filesystem.md index c14649d..6b14702 100644 --- a/manuals/filesystem.md +++ b/manuals/filesystem.md @@ -80,6 +80,7 @@ ## Common mistakes +- Hunting for images the user pasted. They are already in this session's directory as `image_N.`, and the notice that arrived with the message names their exact paths. Read that line instead of searching for files that are in plain sight. - `edit` with an `old` string that appears twice — the call is refused, correctly. Include more surrounding context until it is unique. - `old` copied from a numbered `read` — the line-number column is not part of the file. Read with `numbered: false` when copying text into an edit. - `write` to change one line: it replaces the whole file, and anything you did not retype is gone. Use `edit`. diff --git a/navi/api/websocket.py b/navi/api/websocket.py index 5846d59..392c229 100644 --- a/navi/api/websocket.py +++ b/navi/api/websocket.py @@ -539,15 +539,10 @@ continue raw_images = cleaned - # Tell the model the inline images are already in its multimodal context, - # so it doesn't hallucinate a path/URL and call image_view to "load" them. - if raw_images: - n = len(raw_images) - noun = "image" if n == 1 else "images" - user_content = ( - user_content - + f"\n\n[{n} {noun} attached inline — already in your context, no extra loading needed.]" - ) + # The notice about the inline images is written by the agent, not here: it is + # the layer that puts them in the session directory, so only it knows the + # paths to name. A message queued behind a running turn gets its notice then, + # with the paths the files actually took. # Append uploaded file paths to user content so Navi knows about them uploaded_files: list[dict] = data.get("files") or [] diff --git a/navi/core/agent.py b/navi/core/agent.py index 992c47d..0477b55 100644 --- a/navi/core/agent.py +++ b/navi/core/agent.py @@ -16,15 +16,12 @@ """ import asyncio -import base64 -import io import time from datetime import datetime, timezone from pathlib import Path from typing import TYPE_CHECKING, AsyncGenerator import structlog -from PIL import Image from navi.config import settings from navi.exceptions import ( @@ -36,6 +33,7 @@ SessionNotFound, ) from navi.llm.base import LLMBackend, Message +from navi.session_files import save_images as save_session_images from navi.tools._internal.base import ( Tool, ToolContext, @@ -88,6 +86,36 @@ _TOOL_DONE = object() +def _inline_images_notice(total: int, in_context: int, paths: list[str]) -> str: + """The line the model reads next to the images it was sent. + + `paths` is index-aligned with the images and holds an empty string for one that + could not be stored. The notice always says the files exist, because a model told + the session directory exists and given no path will go looking for one — that hunt + is exactly what this line is here to prevent. + """ + noun = "image" if total == 1 else "images" + if in_context < total: + notice = ( + f"[{total} {noun} attached inline — {in_context} are in your context, " + "the rest were not loaded" + ) + dropped = [path for path in paths[in_context:] if path] + if dropped: + notice += f"; open them with image_view: {', '.join(dropped)}" + else: + notice += " and could not be saved to the session directory" + else: + notice = ( + f"[{total} {noun} attached inline — already in your context, " + "no extra loading needed" + ) + stored = [path for path in paths if path] + if stored: + notice += f". On disk: {', '.join(stored)}" + return notice + "]" + + class _TaggedSink: """Routes one tool's live events into the shared batch queue, tagged by index. @@ -415,9 +443,26 @@ files=files or None, created_at=datetime.now(timezone.utc), is_recall=is_recall, is_context=False) + # Inline chat images land in the session directory too, and the paths go into + # the context next to the images themselves. A pasted image used to exist in + # the transcript only, so an agent told the session directory exists went + # looking there for the "attached images", found nothing, and spent turns on + # fruitless `find` calls before giving up. + saved_paths: list[str] = [] + if images: + try: + names = save_session_images(session_id, images) + except OSError: + log.exception("agent.image_save_failed", session_id=session_id) + names = [] + saved_paths = [ + str(Path(settings.session_files_dir) / session_id / name) if name else "" + for name in names + ] + # Image token budgeting: fit as many images as possible into the LLM context. - # Overflow images are saved to the session directory so Navi can view them - # later via image_view if needed. + # Images that do not fit stay reachable through the session directory, so the + # notice has to name them. images_for_context = images context_content = user_message if images: @@ -426,29 +471,9 @@ max_images = max(0, available_tokens // 500) if max_images < len(images): images_for_context = images[:max_images] - overflow = images[max_images:] - saved_names = [] - try: - session_dir = Path(settings.session_files_dir) / session_id - session_dir.mkdir(parents=True, exist_ok=True) - for idx, b64 in enumerate(overflow): - raw = base64.b64decode(b64) - img = Image.open(io.BytesIO(raw)) - img = img.convert("RGB") - w, h = img.size - if w > 1024 or h > 1024: - ratio = 1024 / max(w, h) - img = img.resize((int(w * ratio), int(h * ratio)), Image.LANCZOS) - name = f"uploaded_{idx}.jpg" - path = session_dir / name - img.save(path, format="JPEG", quality=85, optimize=True) - saved_names.append(name) - except Exception: - pass - if saved_names: - context_content += ( - f"\n\n[Additional images saved to session directory: {', '.join(saved_names)}]" - ) + context_content += "\n\n" + _inline_images_notice( + len(images), len(images_for_context or []), saved_paths + ) user_msg_context = Message(role="user", content=context_content, images=images_for_context or None, files=files or None, created_at=datetime.now(timezone.utc), diff --git a/navi/session_files.py b/navi/session_files.py index 6fb9c1a..69db724 100644 --- a/navi/session_files.py +++ b/navi/session_files.py @@ -6,11 +6,16 @@ """ import asyncio +import base64 +import hashlib +import io +import re import shutil from datetime import datetime, timezone from pathlib import Path import structlog +from PIL import Image from navi.config import settings @@ -47,6 +52,100 @@ return name or "upload" +# The formats `image_view` can open. A pasted image is written to disk for exactly +# that tool to reach, so this set is the one worth naming files after. +_IMAGE_EXTENSIONS = { + "PNG": ".png", + "JPEG": ".jpg", + "WEBP": ".webp", + "GIF": ".gif", + "BMP": ".bmp", +} + + +def _decode_image(data: str) -> bytes: + """Decode one image from the chat, tolerating a `data:` prefix.""" + raw = data.split(",", 1)[1] if data.startswith("data:") and "," in data else data + return base64.b64decode(raw) + + +def _sniff_extension(raw: bytes) -> str | None: + """The extension for what the bytes actually are, or None if they are not an image.""" + with Image.open(io.BytesIO(raw)) as image: + return _IMAGE_EXTENSIONS.get((image.format or "").upper()) + + +_STORED_NAME = re.compile(r"image_\d+") + + +def _stored_images(directory: Path) -> list[Path]: + """The pasted images already in the directory, in the order they were numbered. + + Only names this module writes count: an upload called `image_notes.md` shares the + prefix but not the shape, and must not steal a number from the sequence. + """ + stored = [ + path + for path in directory.iterdir() + if path.is_file() + and _STORED_NAME.fullmatch(path.stem) + and path.suffix.lower() in _IMAGE_EXTENSIONS.values() + ] + return sorted(stored, key=lambda path: int(path.stem.split("_", 1)[1])) + + +def _store_image(directory: Path, raw: bytes, ext: str) -> str: + """Write `raw` under a free `image_N` name and return that name. + + Numbering runs across every pasted image, not per format, so the directory reads as + the order the user sent them in. Bytes already on disk are reused rather than written + again, so a message that was sent twice does not litter the directory with copies. + """ + digest = hashlib.sha256(raw).digest() + stored = _stored_images(directory) + for path in stored: + if path.stat().st_size == len(raw) and hashlib.sha256(path.read_bytes()).digest() == digest: + return path.name + taken = {int(path.stem.split("_", 1)[1]) for path in stored} + number = 1 + while number in taken: + number += 1 + name = f"image_{number}{ext}" + (directory / name).write_bytes(raw) + return name + + +def save_images(session_id: str, images: list[str]) -> list[str]: + """Write the images a user pasted into the chat into the session directory. + + Returns one name per input image, in order; an empty string marks one that could + not be stored. The bytes are written exactly as they arrived — re-encoding to JPEG + would drop the alpha channel of a pasted PNG, which is often the point of pasting it. + + Raising is left to the caller for the one failure it can act on: an `OSError` when + the session directory itself cannot be created. A single unreadable image is logged + and skipped so it cannot cost the rest of the message. + """ + directory = ensure_session_dir(session_id) + names: list[str] = [] + for index, data in enumerate(images, start=1): + try: + raw = _decode_image(data) + ext = _sniff_extension(raw) + if ext is None: + raise ValueError("unrecognised image format") + names.append(_store_image(directory, raw, ext)) + except (OSError, ValueError) as exc: + log.warning( + "session_files.image_not_stored", + session_id=session_id, + index=index, + error=str(exc), + ) + names.append("") + return names + + async def delete_session_dir(session_id: str) -> None: d = session_dir(session_id) if d.exists(): diff --git a/tests/unit/api/test_session_files.py b/tests/unit/api/test_session_files.py index 526859d..14a71bc 100644 --- a/tests/unit/api/test_session_files.py +++ b/tests/unit/api/test_session_files.py @@ -1,7 +1,12 @@ """Unit tests for session file API endpoint logic.""" +import base64 +import io +from unittest.mock import MagicMock + import pytest from fastapi import HTTPException +from PIL import Image import navi.api.routes.sessions as sessions_mod import navi.session_files as session_files_mod @@ -135,3 +140,92 @@ result = await sessions_mod.get_session_content(session_id, store) assert result == {"session_id": session_id, "content": [{"filename": "logo.svg"}]} + + +# ── save_images: the images a user pastes into the chat ────────────────────── + + +def _png_bytes(colour: tuple = (255, 0, 0, 128)) -> bytes: + """A tiny RGBA PNG — alpha present, so a JPEG round-trip would show up as damage.""" + buf = io.BytesIO() + Image.new("RGBA", (2, 2), colour).save(buf, format="PNG") + return buf.getvalue() + + +def _jpg_bytes() -> bytes: + buf = io.BytesIO() + Image.new("RGB", (2, 2), (0, 128, 255)).save(buf, format="JPEG") + return buf.getvalue() + + +def _b64(raw: bytes) -> str: + return base64.b64encode(raw).decode() + + +async def test_save_images_writes_each_one_under_its_own_format(store): + png, jpg = _png_bytes(), _jpg_bytes() + session_id = await _session_id(store) + + names = session_files_mod.save_images(session_id, [_b64(png), _b64(jpg)]) + + assert names == ["image_1.png", "image_2.jpg"] + directory = session_files_mod.session_dir(session_id) + assert (directory / "image_1.png").read_bytes() == png + assert (directory / "image_2.jpg").read_bytes() == jpg + # The bytes are stored as they arrived: an RGBA paste stays RGBA. + assert Image.open(directory / "image_1.png").mode == "RGBA" + + +async def test_save_images_tolerates_a_data_url_prefix(store): + session_id = await _session_id(store) + + names = session_files_mod.save_images( + session_id, [f"data:image/png;base64,{_b64(_png_bytes())}"] + ) + + assert names == ["image_1.png"] + + +async def test_save_images_reuses_a_file_that_already_holds_those_bytes(store): + raw = _png_bytes() + session_id = await _session_id(store) + + first = session_files_mod.save_images(session_id, [_b64(raw)]) + again = session_files_mod.save_images(session_id, [_b64(raw)]) + + assert first == again == ["image_1.png"] + assert len(list(session_files_mod.session_dir(session_id).glob("image_*"))) == 1 + + +async def test_save_images_gives_a_different_picture_the_next_number(store): + session_id = await _session_id(store) + session_files_mod.save_images(session_id, [_b64(_png_bytes())]) + + names = session_files_mod.save_images( + session_id, [_b64(_png_bytes(colour=(0, 255, 0, 255)))] + ) + + assert names == ["image_2.png"] + assert len(list(session_files_mod.session_dir(session_id).glob("image_*"))) == 2 + + +async def test_save_images_skips_what_it_cannot_read_and_says_so(store, monkeypatch): + log = MagicMock() + monkeypatch.setattr(session_files_mod, "log", log) + session_id = await _session_id(store) + + names = session_files_mod.save_images( + session_id, + ["abc", _b64(_png_bytes()), _b64(b"\x00\x01\x02\x03")], + ) + + # One name per input, in order — an unreadable one is an empty slot, not a gap. + assert names == ["", "image_1.png", ""] + assert [call.args[0] for call in log.warning.call_args_list] == [ + "session_files.image_not_stored", + "session_files.image_not_stored", + ] + # Nothing was written under a name that would claim it is a PNG. + assert [p.name for p in session_files_mod.session_dir(session_id).glob("image_*")] == [ + "image_1.png" + ] diff --git a/tests/unit/core/test_agent.py b/tests/unit/core/test_agent.py index 693f05a..8bcca30 100644 --- a/tests/unit/core/test_agent.py +++ b/tests/unit/core/test_agent.py @@ -7,6 +7,7 @@ import asyncio import copy from datetime import datetime, timezone +from unittest.mock import MagicMock import pytest import pytest_asyncio @@ -1020,3 +1021,175 @@ saved = await agent._sessions.get(sess.id) roles = [m.role for m in saved.messages] assert "tool" in roles, f"tool result missing after cancel: {roles}" + + +# ─── pasted images reach the session directory ─────────────────────────────── + + +def _png_b64(colour: tuple = (255, 0, 0, 128)) -> str: + """A pasted RGBA PNG, base64 — alpha present, so a re-encode would show up.""" + import base64 + import io + + from PIL import Image + + buf = io.BytesIO() + Image.new("RGBA", (2, 2), colour).save(buf, format="PNG") + return base64.b64encode(buf.getvalue()).decode() + + +def _inline_images_notice(*args, **kwargs): + from navi.core.agent import _inline_images_notice as notice + + return notice(*args, **kwargs) + + +@pytest.fixture +def patch_agent_settings(monkeypatch, tmp_path): + """Point every module that reads `session_files_dir` at a throwaway root. + + Returns a function so a test can also shrink the context window: the image budget + is computed from `settings.ollama_num_ctx` in the agent module, while the + compressor's own thresholds read their own binding and stay untouched. + """ + import navi.core.agent as agent_mod + import navi.session_files as session_files_mod + from navi.config import Settings + + def _apply(**overrides): + root = tmp_path / "sessions" + patched = Settings(_env_file=None, session_files_dir=str(root), **overrides) + monkeypatch.setattr(session_files_mod, "settings", patched) + monkeypatch.setattr(agent_mod, "settings", patched) + return root + + return _apply + + +class TestPastedImagesReachTheSessionDirectory: + @pytest.mark.asyncio + async def test_a_pasted_image_is_written_and_its_path_reaches_the_model( + self, agent, session, patch_agent_settings + ): + """The image lives in the transcript only; without a copy on disk an agent + told the session directory exists goes hunting there and finds nothing.""" + import base64 + + session_files_dir = patch_agent_settings() + backend = FakeLLMBackend(responses=["ok"]) + agent._backends.register("ollama", backend) + raw = base64.b64decode(_png_b64()) + + async for _ in agent.run_stream(session.id, "look at this", images=[_png_b64()]): + pass + + written = session_files_dir / session.id / "image_1.png" + assert written.read_bytes() == raw + # Stored as it arrived: an RGBA paste stays RGBA. + from PIL import Image + + assert Image.open(written).mode == "RGBA" + + saved = await agent._sessions.get(session.id) + display, context = saved.messages[0], saved.messages[1] + # The user sees their own words, not a notice about storage. + assert display.content == "look at this" + assert context.content.startswith("look at this") + assert "On disk: " in context.content + assert str(written) in context.content + + @pytest.mark.asyncio + async def test_an_image_that_does_not_fit_is_named_for_image_view( + self, agent, session, patch_agent_settings + ): + """Images over the budget stay out of the LLM context — so the notice has to + hand the model a path, or they are simply lost.""" + session_files_dir = patch_agent_settings(ollama_num_ctx=500) + backend = FakeLLMBackend(responses=["ok"]) + agent._backends.register("ollama", backend) + + async for _ in agent.run_stream(session.id, "look", images=[_png_b64()]): + pass + + saved = await agent._sessions.get(session.id) + context = saved.messages[1] + assert context.images is None + assert "1 image attached inline — 0 are in your context" in context.content + assert "open them with image_view: " in context.content + assert str(session_files_dir / session.id / "image_1.png") in context.content + + # One file, named for what it is — no `uploaded_*.jpg` re-encode. + assert sorted(p.name for p in (session_files_dir / session.id).iterdir()) == [ + "image_1.png" + ] + + @pytest.mark.asyncio + async def test_a_hidden_turn_writes_nothing(self, agent, session, patch_agent_settings): + """Hidden turns carry machine payloads, not pastes — nothing to store.""" + session_files_dir = patch_agent_settings() + backend = FakeLLMBackend(responses=["ok"]) + agent._backends.register("ollama", backend) + + async for _ in agent.run_stream( + session.id, '{"form": "submitted"}', images=[_png_b64()], hidden=True + ): + pass + + assert not (session_files_dir / session.id).exists() + + @pytest.mark.asyncio + async def test_a_failed_save_does_not_cost_the_turn( + self, agent, session, patch_agent_settings, monkeypatch + ): + """A directory that cannot be created must not take the message with it. The + model is not told either: the images are in its context already, so the copy + that failed is one it was never going to need. Operations hear it instead.""" + import navi.core.agent as agent_mod + + log = MagicMock() + monkeypatch.setattr(agent_mod, "log", log) + session_files_dir = patch_agent_settings() + backend = FakeLLMBackend(responses=["ok"]) + agent._backends.register("ollama", backend) + # The root exists as a plain file, so mkdir under it raises. + session_files_dir.write_text("not a directory") + + events = [ev async for ev in agent.run_stream(session.id, "look", images=[_png_b64()])] + + assert type(events[-1]).__name__ == "StreamEnd" + assert [call.args[0] for call in log.exception.call_args_list] == [ + "agent.image_save_failed" + ] + saved = await agent._sessions.get(session.id) + display, context = saved.messages[0], saved.messages[1] + assert display.content == "look" + assert context.content.startswith("look") + assert "On disk" not in context.content + assert context.images is not None + + +class TestInlineImagesNotice: + def test_all_images_in_context_names_the_files(self): + notice = _inline_images_notice(2, 2, ["sessions/s/image_1.png", "sessions/s/image_2.jpg"]) + + assert notice == ( + "[2 images attached inline — already in your context, no extra loading needed" + ". On disk: sessions/s/image_1.png, sessions/s/image_2.jpg]" + ) + + def test_a_single_image_reads_as_one_image(self): + assert _inline_images_notice(1, 1, []).startswith("[1 image attached inline") + + def test_a_dropped_image_is_named_for_image_view(self): + notice = _inline_images_notice(3, 1, ["a.png", "b.png", "c.png"]) + + assert "3 images attached inline — 1 are in your context" in notice + assert "open them with image_view: b.png, c.png" in notice + + def test_an_unstored_image_is_not_offered_as_a_path(self): + """An empty slot means the save failed; naming it would send the model + after a file that is not there.""" + notice = _inline_images_notice(2, 0, ["", ""]) + + assert "could not be saved to the session directory" in notice + assert "image_view" not in notice