Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 75 additions & 2 deletions libs/code/deepagents_code/offload.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,9 @@ def _filesystem_tool_path(path: PurePath) -> str:
_EPHEMERAL_OFFLOAD_STORAGE = False
"""Whether the most recent `_offload_fallback_root` fell back to temp storage."""

_UNIQUE_OFFLOAD_FALLBACK_ROOT: Path | None = None
"""Private random fallback root that cannot be reconstructed on a later call."""


def offload_storage_is_ephemeral() -> bool:
"""Return whether offload history is routed to non-persistent storage.
Expand Down Expand Up @@ -194,7 +197,13 @@ def _prepare_temp_dir(path: Path) -> Path:
_probe_writable(path)
return path

global _EPHEMERAL_OFFLOAD_STORAGE # noqa: PLW0603
global _EPHEMERAL_OFFLOAD_STORAGE, _UNIQUE_OFFLOAD_FALLBACK_ROOT # noqa: PLW0603
if _UNIQUE_OFFLOAD_FALLBACK_ROOT is not None:
# Unlike the persistent and predictable temp paths, a directory created
# by `mkdtemp` cannot be derived again. Keep returning the root already
# used by the archive backend so cleanup reaches the same files.
_EPHEMERAL_OFFLOAD_STORAGE = True
return _UNIQUE_OFFLOAD_FALLBACK_ROOT
Comment on lines +201 to +206

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.

🟡 Cached fallback skips directory hardening

When the unique temp fallback is cached, later calls return it without re-running _prepare_temp_dir. If that temp directory is removed during the process lifetime (for example by a temp cleaner or user cleanup), the next offload still builds a FilesystemBackend on this stale path; its write() recreates the missing root and conversation_history parents with default permissions (typically 0755) and the archive as 0644, instead of the intended private 0700 tree. Revalidate/harden the cached fallback root (and clear/recreate it if unusable) before returning it so conversation history cannot be recreated world-readable under /tmp.

(Refers to lines 201-206)


Your feedback helps Open SWE learn. React with 👍 or 👎 to tell us if this review comment was useful.

try:
root = _prepare_user_dir()
except (RuntimeError, OSError):
Expand Down Expand Up @@ -222,4 +231,68 @@ def _prepare_temp_dir(path: Path) -> Path:
exc_info=True,
)
unique = Path(tempfile.mkdtemp(prefix=f"deepagents-{suffix}-", dir=temp_root))
return _prepare_temp_dir(unique)
_UNIQUE_OFFLOAD_FALLBACK_ROOT = _prepare_temp_dir(unique)
return _UNIQUE_OFFLOAD_FALLBACK_ROOT


def delete_offloaded_history(thread_id: str) -> bool:
"""Remove a thread's offloaded conversation-history archive.

Deletes the per-thread markdown file written by the local-mode
`conversation_history` backend (`{root}/conversation_history/{thread_id}.md`),
resolving `root` with `_offload_fallback_root` so the persistent
`~/.deepagents` location and any temporary fallback are both covered.

Best-effort: filesystem failures are logged and swallowed rather than
raised, so a failed cleanup never blocks thread deletion. Resolving the
offload root is not side-effect-free -- it creates (and hardens) the
`conversation_history` directory and writes a short-lived probe file -- so a
call for a thread that has no archive still touches the filesystem before
returning `False`.

In server/sandbox mode the archive lives on the sandbox backend rather than
the local `~/.deepagents` directory, so there is no local archive to remove.

Args:
thread_id: Thread whose offloaded history should be removed.

Returns:
`True` only if an archive file was removed. `False` in every other case:
an empty or rejected `thread_id`, an unresolvable offload root, a missing
archive, or an `unlink` failure.
"""
if not thread_id:
return False
try:
archive_dir = _offload_fallback_root() / "conversation_history"
except (OSError, RuntimeError):
logger.warning(
"Could not resolve offload root to clean history for thread %s",
thread_id,
exc_info=True,
)
return False
archive_path = archive_dir / f"{thread_id}.md"
# Guard against a crafted thread id escaping the archive directory. Thread
# ids are system-generated UUID7 strings, so a rejection here means either a
# crafted input or a bug emitting malformed ids -- both worth a trace, and
# both distinct from the benign "no archive exists" path below.
if archive_path.parent != archive_dir:
logger.warning(
"Refusing to delete offloaded history for suspicious thread id %r",
thread_id,
)
return False
try:
archive_path.unlink()
except FileNotFoundError:
return False
except OSError:
logger.warning(
"Failed to delete offloaded conversation history for thread %s",
thread_id,
exc_info=True,
)
return False
logger.debug("Deleted offloaded conversation history for thread %s", thread_id)
return True
45 changes: 28 additions & 17 deletions libs/code/deepagents_code/sessions.py
Original file line number Diff line number Diff line change
Expand Up @@ -1406,28 +1406,39 @@ async def find_similar_threads(thread_id: str, limit: int = 3) -> list[str]:


async def delete_thread(thread_id: str) -> bool:
"""Delete thread checkpoints.
"""Delete thread checkpoints and any offloaded conversation history.

Removes the thread's checkpoint/write rows, then makes a best-effort attempt
to remove the per-thread offloaded conversation-history archive under
`~/.deepagents` (local mode) so deletion does not leave orphaned history
behind. History cleanup failures are logged, not raised, and do not affect
the return value, which reflects only whether checkpoint rows were removed.

Returns:
True if thread was deleted, False if not found.
True if thread checkpoints were deleted, False if not found.
"""
deleted = False
async with _connect() as conn:
if not await _table_exists(conn, "checkpoints"):
return False
if await _table_exists(conn, "checkpoints"):
cursor = await conn.execute(
"DELETE FROM checkpoints WHERE thread_id = ?", (thread_id,)
)
deleted = cursor.rowcount > 0
if await _table_exists(conn, "writes"):
await conn.execute(
"DELETE FROM writes WHERE thread_id = ?", (thread_id,)
)
await conn.commit()
if deleted:
_message_count_cache.pop(thread_id, None)
for key, rows in list(_recent_threads_cache.items()):
filtered = [row for row in rows if row["thread_id"] != thread_id]
_recent_threads_cache[key] = filtered

cursor = await conn.execute(
"DELETE FROM checkpoints WHERE thread_id = ?", (thread_id,)
)
deleted = cursor.rowcount > 0
if await _table_exists(conn, "writes"):
await conn.execute("DELETE FROM writes WHERE thread_id = ?", (thread_id,))
await conn.commit()
if deleted:
_message_count_cache.pop(thread_id, None)
for key, rows in list(_recent_threads_cache.items()):
filtered = [row for row in rows if row["thread_id"] != thread_id]
_recent_threads_cache[key] = filtered
return deleted
from deepagents_code.offload import delete_offloaded_history

delete_offloaded_history(thread_id)
return deleted


@asynccontextmanager
Expand Down
122 changes: 122 additions & 0 deletions libs/code/tests/unit_tests/test_offload.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,15 @@
import pytest
from deepagents.backends.utils import validate_path

from deepagents_code import offload
from deepagents_code._session_stats import format_token_count
from deepagents_code.app import DeepAgentsApp
from deepagents_code.command_registry import get_slash_commands
from deepagents_code.offload import (
_artifacts_root,
_filesystem_tool_path,
_offload_fallback_root,
delete_offloaded_history,
)
from deepagents_code.tui.widgets.messages import AppMessage, ErrorMessage

Expand Down Expand Up @@ -1077,6 +1079,7 @@ def test_fallback_root_avoids_file_at_predictable_per_user_path(
monkeypatch.setattr(Path, "home", lambda: tmp_path / "home")
monkeypatch.setattr(tempfile, "gettempdir", lambda: str(temp_dir))
monkeypatch.setattr(tempfile, "NamedTemporaryFile", probe)
monkeypatch.setattr(offload, "_UNIQUE_OFFLOAD_FALLBACK_ROOT", None)

root = _offload_fallback_root()

Expand Down Expand Up @@ -1123,6 +1126,7 @@ def fake_lstat(self: Path) -> Any: # noqa: ANN401
monkeypatch.setattr(tempfile, "gettempdir", lambda: str(temp_dir))
monkeypatch.setattr(tempfile, "NamedTemporaryFile", probe)
monkeypatch.setattr(Path, "lstat", fake_lstat)
monkeypatch.setattr(offload, "_UNIQUE_OFFLOAD_FALLBACK_ROOT", None)

root = _offload_fallback_root()

Expand Down Expand Up @@ -1189,6 +1193,124 @@ def test_fallback_root_tightens_preexisting_loose_archive_subdir(
assert offload_storage_is_ephemeral() is False


class TestDeleteOffloadedHistory:
"""Cover cleanup of a thread's offloaded conversation-history archive."""

def test_removes_persistent_archive(
self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch
) -> None:
"""The per-thread archive under `~/.deepagents` is removed."""
monkeypatch.setattr(Path, "home", lambda: tmp_path)
archive_dir = tmp_path / ".deepagents" / "conversation_history"
archive_dir.mkdir(parents=True)
archive = archive_dir / "thread-1.md"
archive.write_text("history")
keep = archive_dir / "thread-2.md"
keep.write_text("other")

assert delete_offloaded_history("thread-1") is True
assert not archive.exists()
# Unrelated threads' archives are left untouched.
assert keep.exists()

def test_removes_archive_from_reused_unique_fallback(
self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch
) -> None:
"""Cleanup reuses the random root selected when the archive was written."""
home_root = tmp_path / "home" / ".deepagents"
home_root.mkdir(parents=True)
temp_dir = tmp_path / "tmp"
temp_dir.mkdir()
getuid = getattr(os, "getuid", None)
uid = getuid() if getuid is not None else os.getpid()
(temp_dir / f"deepagents-{uid}").write_text("not a directory")
probe = MagicMock(
side_effect=[PermissionError("read-only home"), nullcontext()]
)

monkeypatch.setattr(Path, "home", lambda: tmp_path / "home")
monkeypatch.setattr(tempfile, "gettempdir", lambda: str(temp_dir))
monkeypatch.setattr(tempfile, "NamedTemporaryFile", probe)
monkeypatch.setattr(offload, "_UNIQUE_OFFLOAD_FALLBACK_ROOT", None)

root = _offload_fallback_root()
archive = root / "conversation_history" / "thread-1.md"
archive.parent.mkdir(parents=True)
archive.write_text("history")

assert delete_offloaded_history("thread-1") is True
assert not archive.exists()
assert probe.call_count == 2

def test_missing_archive_reports_nothing_removed(
self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch
) -> None:
"""Deleting a thread with no archive reports nothing removed."""
monkeypatch.setattr(Path, "home", lambda: tmp_path)

assert delete_offloaded_history("thread-1") is False

def test_empty_thread_id_is_noop(
self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch
) -> None:
"""An empty thread id never touches the filesystem."""
monkeypatch.setattr(Path, "home", lambda: tmp_path)

assert delete_offloaded_history("") is False

def test_unlink_failure_is_swallowed(
self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch
) -> None:
"""A failing `unlink` is logged and reported as nothing removed."""
monkeypatch.setattr(Path, "home", lambda: tmp_path)
monkeypatch.setattr(offload, "_UNIQUE_OFFLOAD_FALLBACK_ROOT", None)
archive_dir = tmp_path / ".deepagents" / "conversation_history"
archive_dir.mkdir(parents=True)
archive = archive_dir / "thread-1.md"
archive.write_text("history")
monkeypatch.setattr(
Path, "unlink", MagicMock(side_effect=PermissionError("read-only mount"))
)

assert delete_offloaded_history("thread-1") is False
# The archive survives the failed deletion rather than being lost.
assert archive.exists()

def test_unresolvable_root_returns_false(
self, monkeypatch: pytest.MonkeyPatch
) -> None:
"""An unresolvable offload root is swallowed, not raised."""
monkeypatch.setattr(
offload,
"_offload_fallback_root",
MagicMock(side_effect=OSError("no writable location")),
)

assert delete_offloaded_history("thread-1") is False

def test_rejects_thread_id_path_traversal(
self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch
) -> None:
"""A crafted thread id cannot escape the archive directory."""
monkeypatch.setattr(Path, "home", lambda: tmp_path)
(tmp_path / ".deepagents" / "conversation_history").mkdir(parents=True)
# A relative escape resolves to `.deepagents/config.md`, so a decoy there
# is load-bearing: were the guard removed, `unlink` would delete it.
relative_decoy = tmp_path / ".deepagents" / "config.md"
relative_decoy.write_text("secret")
# An absolute thread id resets the join, escaping the archive tree
# entirely; place its decoy where that reset lands.
outside = tmp_path / "outside.md"
outside.write_text("secret")

assert delete_offloaded_history("../config") is False
assert delete_offloaded_history(str(tmp_path / "outside")) is False
# An embedded separator lands in a subdirectory, not `archive_dir`.
assert delete_offloaded_history("sub/thread") is False
assert relative_decoy.exists()
assert outside.exists()


class TestArtifactsRoot:
"""Cover the real-filesystem artifacts root for offloaded tool results."""

Expand Down
54 changes: 51 additions & 3 deletions libs/code/tests/unit_tests/test_sessions.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
from datetime import UTC, datetime, timedelta
from pathlib import Path
from typing import TYPE_CHECKING, ClassVar, cast
from unittest.mock import AsyncMock, patch
from unittest.mock import AsyncMock, MagicMock, patch

import pytest
from langgraph.checkpoint.serde.jsonplus import JsonPlusSerializer
Expand Down Expand Up @@ -351,19 +351,67 @@ def test_get_thread_cwd_ignores_empty_string(self, temp_db):
cwd = asyncio.run(sessions.get_thread_cwd("thread-empty"))
assert cwd is None

def test_delete_thread(self, temp_db):
def test_delete_thread(self, temp_db, monkeypatch, tmp_path):
"""Delete thread removes thread."""
monkeypatch.setattr(Path, "home", lambda: tmp_path)
with patch.object(sessions, "get_db_path", return_value=temp_db):
result = asyncio.run(sessions.delete_thread("thread1"))
assert result is True
assert asyncio.run(sessions.thread_exists("thread1")) is False

def test_delete_thread_not_found(self, temp_db):
def test_delete_thread_not_found(self, temp_db, monkeypatch, tmp_path):
"""Delete thread returns False for non-existing thread."""
monkeypatch.setattr(Path, "home", lambda: tmp_path)
with patch.object(sessions, "get_db_path", return_value=temp_db):
result = asyncio.run(sessions.delete_thread("nonexistent"))
assert result is False

def test_delete_thread_cleans_offloaded_history(
self, temp_db, monkeypatch, tmp_path
):
"""Deleting a thread removes its offloaded conversation history."""
monkeypatch.setattr(Path, "home", lambda: tmp_path)
archive_dir = tmp_path / ".deepagents" / "conversation_history"
archive_dir.mkdir(parents=True)
archive = archive_dir / "thread1.md"
archive.write_text("history")
with patch.object(sessions, "get_db_path", return_value=temp_db):
result = asyncio.run(sessions.delete_thread("thread1"))
assert result is True
assert not archive.exists()

def test_delete_thread_succeeds_when_history_cleanup_fails(
self, temp_db, monkeypatch, tmp_path
):
"""Checkpoint deletion drives the result; history cleanup is best-effort."""
monkeypatch.setattr(Path, "home", lambda: tmp_path)
# `delete_thread` imports the helper from `offload` at call time, so
# patching it there simulates a failed (but swallowed) cleanup.
from deepagents_code import offload

monkeypatch.setattr(
offload, "delete_offloaded_history", MagicMock(return_value=False)
)
with patch.object(sessions, "get_db_path", return_value=temp_db):
result = asyncio.run(sessions.delete_thread("thread1"))
assert result is True

def test_delete_thread_removes_orphan_archive_without_checkpoints(
self, temp_db, monkeypatch, tmp_path
):
"""A thread with no checkpoints still has its orphaned archive cleaned."""
monkeypatch.setattr(Path, "home", lambda: tmp_path)
archive_dir = tmp_path / ".deepagents" / "conversation_history"
archive_dir.mkdir(parents=True)
archive = archive_dir / "orphan-thread.md"
archive.write_text("history")
with patch.object(sessions, "get_db_path", return_value=temp_db):
result = asyncio.run(sessions.delete_thread("orphan-thread"))
# No checkpoint rows existed, so the thread is reported "not found"...
assert result is False
# ...but its stranded archive is removed regardless.
assert not archive.exists()


class TestGetCheckpointer:
"""Tests for get_checkpointer async context manager."""
Expand Down