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
34 changes: 29 additions & 5 deletions tests/server/openclaw/session_memory/test_claw_session_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

from __future__ import annotations

import hashlib
import shutil
from pathlib import Path
from unittest.mock import AsyncMock, MagicMock, patch
Expand Down Expand Up @@ -47,15 +48,26 @@ class TestGetSessionPath:

def test_constructs_correct_path(self, tmp_path):
svc = _make_service(tmp_path)
path = svc._get_session_path("app:user:session")
save_key = "app:user:session"
path = svc._get_session_path(save_key)
assert path.parent == svc.sessions_dir
assert path.suffix == ".jsonl"
assert "app_user_session" in path.stem or "app" in path.stem
assert path.stem == hashlib.sha256(save_key.encode("utf-8")).hexdigest()

def test_replaces_colons(self, tmp_path):
def test_filename_length_is_bounded(self, tmp_path):
svc = _make_service(tmp_path)
path = svc._get_session_path("a:b:c")
assert ":" not in path.name
short_path = svc._get_session_path("a:b:c")
long_path = svc._get_session_path("app:" + "user" * 1000 + ":session")

assert len(short_path.name) == len(long_path.name) == 70
assert short_path != long_path

def test_unhashed_path_preserves_previous_format(self, tmp_path):
svc = _make_service(tmp_path)
path = svc._get_unhashed_session_path("app:user:session")

assert path.parent == svc.sessions_dir
assert "app_user_session" in path.stem or "app" in path.stem


# ---------------------------------------------------------------------------
Expand Down Expand Up @@ -110,6 +122,18 @@ def test_migration_success(self, tmp_path):
assert target.read_text() == "legacy data"
assert not legacy.exists()

def test_migrates_unhashed_current_session(self, tmp_path):
svc = _make_service(tmp_path)
save_key = "app:user:session"
target = svc._get_session_path(save_key)
unhashed = svc._get_unhashed_session_path(save_key)
unhashed.write_text("current session data")

svc._maybe_migrate(save_key, target)

assert target.read_text() == "current session data"
assert not unhashed.exists()

@patch("trpc_agent_sdk.server.openclaw.session_memory._claw_session_service.shutil.move",
side_effect=OSError("permission denied"))
def test_migration_failure_logs_error(self, mock_move, tmp_path):
Expand Down
55 changes: 39 additions & 16 deletions tests/server/openclaw/storage/test_aiofile_storage.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
HISTORY_FILENAME,
MEMORY_FILENAME,
)
from trpc_agent_sdk.storage import DEFAULT_MAX_KEY_LENGTH


class TestAioFileStorageInit:
Expand All @@ -42,28 +41,31 @@ def test_max_key_length(self, tmp_path):
class TestValidateKey:
"""Tests for AioFileStorage._validate_key."""

def test_empty_key_raises(self):
def test_empty_key_raises(self, tmp_path):
storage = _make_storage(tmp_path)
with pytest.raises(ValueError, match="cannot be empty"):
AioFileStorage._validate_key("")
storage._validate_key("")

def test_too_long_key_raises(self):
long_key = "a" * (DEFAULT_MAX_KEY_LENGTH + 1)
with pytest.raises(ValueError, match="too long"):
AioFileStorage._validate_key(long_key)
def test_configured_max_key_length_is_enforced(self, tmp_path):
storage = AioFileStorage(FileStorageConfig(base_dir=str(tmp_path), max_key_length=140))

def test_forward_slash_raises(self):
with pytest.raises(ValueError, match="path separators"):
AioFileStorage._validate_key("a/b")
storage._validate_key("a" * 140)
with pytest.raises(ValueError, match="141 > 140"):
storage._validate_key("a" * 141)

def test_backslash_raises(self):
def test_forward_slash_raises(self, tmp_path):
storage = _make_storage(tmp_path)
with pytest.raises(ValueError, match="path separators"):
AioFileStorage._validate_key("a\\b")
storage._validate_key("a/b")

def test_valid_key(self):
AioFileStorage._validate_key("valid-key_123")
def test_backslash_raises(self, tmp_path):
storage = _make_storage(tmp_path)
with pytest.raises(ValueError, match="path separators"):
storage._validate_key("a\\b")

def test_max_length_key_ok(self):
AioFileStorage._validate_key("a" * DEFAULT_MAX_KEY_LENGTH)
def test_valid_key(self, tmp_path):
storage = _make_storage(tmp_path)
storage._validate_key("valid-key_123")


class TestKeyToPathAndPathToKey:
Expand Down Expand Up @@ -192,6 +194,15 @@ async def test_add_validates_key(self, tmp_path):
with pytest.raises(ValueError):
await storage.add(db, {"key": "", "value": "data"})

async def test_add_honors_configured_max_key_length(self, tmp_path):
storage = AioFileStorage(FileStorageConfig(base_dir=str(tmp_path), max_key_length=255))
db = FileSession(base_dir=tmp_path)
key = "k" * 150

Comment on lines +197 to +201

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.

问题: 新测试 test_add_honors_configured_max_key_length 仅用 150 字符 key 验证配置放宽路径,恰好避开 251-255 的崩溃边界,未覆盖“配置上限与文件系统 255 字节文件名限制冲突”的核心场景;test_get_validates_key/test_delete_validates_key 只测了超长抛错,未测 session: 前缀 key 与长 workspace 路径下 get 校验的回归;迁移测试未覆盖 unhashed 与 legacy 双候选并存的优先级及 move 失败后回退到第二候选。

触发条件: 无;为测试缺口。

实际影响: 上述 SEVERE 与 MODERATE 缺陷在测试全绿的情况下合入上线,回归无法被 CI 拦截。

修正方向: 补充 251-255 字符 ASCII key、CJK key、max_key_length 极值(1/200)的写-读往返用例;为 _maybe_migrate 增加双候选并存与“第一候选失败回退第二候选”的用例。

await storage.add(db, {"key": key, "value": "persisted"})

assert await storage.get(db, key) == "persisted"


class TestGet:
"""Tests for AioFileStorage.get."""
Expand All @@ -209,6 +220,12 @@ async def test_get_nonexistent(self, tmp_path):
result = await storage.get(db, "nope")
assert result is None

async def test_get_validates_key(self, tmp_path):
storage = AioFileStorage(FileStorageConfig(base_dir=str(tmp_path), max_key_length=3))
db = FileSession(base_dir=tmp_path)
with pytest.raises(ValueError, match="4 > 3"):
await storage.get(db, "long")

async def test_get_text_file(self, tmp_path):
storage = _make_storage(tmp_path)
db = FileSession(base_dir=tmp_path)
Expand Down Expand Up @@ -241,6 +258,12 @@ async def test_delete_nonexistent_no_error(self, tmp_path):
db = FileSession(base_dir=tmp_path)
await storage.delete(db, "nosuchkey")

async def test_delete_validates_key(self, tmp_path):
storage = AioFileStorage(FileStorageConfig(base_dir=str(tmp_path), max_key_length=3))
db = FileSession(base_dir=tmp_path)
with pytest.raises(ValueError, match="4 > 3"):
await storage.delete(db, "long")


class TestQuery:
"""Tests for AioFileStorage.query."""
Expand Down
2 changes: 1 addition & 1 deletion trpc_agent_sdk/server/openclaw/config/_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ class MemoryConfig(BaseModel):
class FileStorageConfig(BaseModel):
"""trpc_claw file storage config."""
base_dir: str = ""
max_key_length: int = 255
max_key_length: int = Field(default=255, ge=1)


class SqlStorageConfig(BaseModel):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
from __future__ import annotations

import copy
import hashlib
import shutil
import uuid
from pathlib import Path
Expand Down Expand Up @@ -191,6 +192,11 @@ async def update_session(self, session: Session) -> None:
# ------------------------------------------------------------------

def _get_session_path(self, save_key: str) -> Path:
digest = hashlib.sha256(save_key.encode("utf-8")).hexdigest()
return self.sessions_dir / f"{digest}.jsonl"

def _get_unhashed_session_path(self, save_key: str) -> Path:
"""Return the session path used before filenames were hashed."""
safe_key = safe_filename(save_key.replace(":", "_"))
return self.sessions_dir / f"{safe_key}.jsonl"

Expand All @@ -199,14 +205,19 @@ def _get_legacy_session_path(self, save_key: str) -> Path:
return self.legacy_sessions_dir / f"{safe_key}.jsonl"

def _maybe_migrate(self, save_key: str, target: Path) -> None:
"""Move a legacy session file to *target* if one exists and target is absent."""
"""Move an older session file to *target* when one exists."""
if target.exists():
return
legacy = self._get_legacy_session_path(save_key)
if not legacy.exists():
candidates = (
self._get_unhashed_session_path(save_key),
self._get_legacy_session_path(save_key),
)
for source in candidates:
if not source.exists():
continue
try:
shutil.move(str(source), str(target))
logger.info("Migrated session %s from %s", save_key, source)
except Exception as exc: # pylint: disable=broad-except
logger.error("Failed to migrate session %s from %s: %s", save_key, source, exc)
return
Comment on lines +215 to 223

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.

问题: _maybe_migrate 新循环中,第一个候选源(unhashed 文件)存在但 shutil.move 失败(权限、文件被占用等)时,return 位于 except 之后、循环体内,直接跳过对第二个候选(legacy 目录)的尝试;旧代码只有一个候选无此语义。且 return 在 try 外统一退出,两个候选共存时每次调用最多尝试一个。

触发条件: 磁盘同时存在 sessions/<safe_key>.jsonl(unhashed)与 legacy 目录同 key 文件,且 unhashed 候选迁移失败(如文件被其它进程占用、只读挂载)。

实际影响: legacy 目录中的历史会话数据被无限期搁置(下次调用会重试第一个候选,仍失败则始终不达 legacy),升级后这些会话恢复不到;无数据丢失但迁移停止,与注释宣称的“依次迁移两个来源”不符。

修正方向: 将循环体内的 return 改为 continue(源不存在或 move 失败时继续尝试下一候选;成功迁移后 break),并补充“unhashed 失败后仍尝试 legacy”的测试。

try:
shutil.move(str(legacy), str(target))
logger.info("Migrated session %s from legacy path", save_key)
except Exception as exc: # pylint: disable=broad-except
logger.error("Failed to migrate session %s: %s", save_key, exc)
10 changes: 5 additions & 5 deletions trpc_agent_sdk/server/openclaw/storage/_aiofile_storage.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
from aiofiles import ospath as aio_ospath
from nanobot.utils.helpers import safe_filename
from trpc_agent_sdk.storage import BaseStorage
from trpc_agent_sdk.storage import DEFAULT_MAX_KEY_LENGTH

from ..config import FileStorageConfig
from ._constants import HISTORY_FILENAME
Expand Down Expand Up @@ -113,6 +112,7 @@ async def add(self, db: FileSession, data: FileData | dict[str, Any]) -> None:

@override
async def delete(self, db: FileSession, key: str, conditions: Optional[FileCondition] = None) -> None:
self._validate_key(key)
file_path = await self._resolve_key_path(db.base_dir, key)
if not await aio_ospath.exists(file_path):
return
Expand Down Expand Up @@ -140,6 +140,7 @@ async def query(self,

@override
async def get(self, db: FileSession, key: str) -> Any:
self._validate_key(key)
Comment on lines 142 to +143

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.

问题: get/delete 新增 self._validate_key(key) 破坏了“缺失即返回 None / 幂等无操作”的既有契约:超长 key 之前静默返回 None/无操作,现在直接抛 ValueError。而 StorageManager.load_session 中 _get_value(内部调用 storage.get)位于 try/except 之外(_manager.py:160),异常直接冒泡,会话恢复从“文件缺失返回 None → 创建新会话”变为硬崩溃。session:{quote(str(path))} 类 key 的长度由 workspace 路径决定(实测 200 字符 workspace 路径时 key 达 311 字符),用户显式配置较小 max_key_length(如沿用旧文档的 128)时同样命中。

触发条件: max_key_length 配置值小于会话/记忆 key 的实际长度:包括 workspace 位于长路径(CI 沙箱、容器挂载路径等,>/120 字符)或用户在 config.yaml 中显式配置 <255 的情形。

实际影响: 升级后首次 get_session/create_session 恢复会话直接抛 ValueError: AioFileStorage key too long,无法恢复历史会话;delete 对历史遗留超长 key 的幂等清理也变为抛异常。

修正方向: 对 session:/memory:/history: 前缀 key(内部构造、不受外部输入长度影响)在 get/delete 中跳过或放宽长度校验,只保留普通 key 的长度检查;或将 max_key_length 的默认上限与 session key 最长可能长度解耦(如对内部前缀 key 不做长度限制)。

file_path = await self._resolve_key_path(db.base_dir, key)
if not await aio_ospath.exists(file_path):
return None
Expand Down Expand Up @@ -177,12 +178,11 @@ def _key_to_path(base_dir: Path, key: str) -> Path:
def _path_to_key(path: Path) -> str:
return unquote(path.stem)

@staticmethod
def _validate_key(key: str) -> None:
def _validate_key(self, key: str) -> None:
if not key:
raise ValueError("AioFileStorage key cannot be empty")
if len(key) > DEFAULT_MAX_KEY_LENGTH:
raise ValueError(f"AioFileStorage key too long: {len(key)} > {DEFAULT_MAX_KEY_LENGTH}")
if len(key) > self._max_key_length:
raise ValueError(f"AioFileStorage key too long: {len(key)} > {self._max_key_length}")
if "/" in key or "\\" in key:
# Key is logical identifier, not filesystem path.
raise ValueError("AioFileStorage key must not contain path separators")
Comment on lines +181 to 188

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.

问题: _validate_key 改用 self._max_key_length 后按字符数(len(key))校验,而 _key_to_path 生成的文件名是 quote(key, safe='') + ".json"(按字节计),校验上限 255 与文件系统 NAME_MAX=255 字节直接冲突:默认配置下 251-255 个 ASCII 字符(或约 28 个 CJK 字符,经 quote 后每个占 9 字节)的 key 能通过校验,但落盘文件名超过 255 字节,add() 在 aiofiles.open 处抛未捕获的 OSError(36, ENAMETOOLONG)。旧代码固定使用 DEFAULT_MAX_KEY_LENGTH=128,ASCII key 文件名最多 133 字节,任何 ASCII key 都不会触发此崩溃;本次将配置激活为默认 255 后,校验放行的区间内存在必然崩溃的合法 key,且 ge=1 无上限允许用户配置更大值进一步放大受损区间。

触发条件: 存储类型为 file + 默认或更大的 max_key_length + 写入无前缀普通 key(≤255 字符但编码后文件名 >255 字节)。实测:key=250 字符(文件名 255 字节)成功,key=251 字符(256 字节)抛 OSError: [Errno 36] File name too long;约 28 个中文字符即触发。

实际影响: 写入持久化永久失败且每次重试必崩;StorageManager._set_value 与 ClawSessionService.update_session 均无 try/except,异常直接冒泡到请求层导致会话保存数据丢失、请求失败;get/delete 路径同样受影响(aio_ospath.exists 内部 stat 抛同名异常)。修复目标(放开 key 长度)与底层文件系统限制自相矛盾。

修正方向: 校验应以 _key_to_path 的最终字节数为准:在 _validate_key(或 _key_to_path)中对 quote(key, safe='') + ".json" 的 UTF-8 字节长度做上限检查(如 ≤240 预留后缀余量),并在 FileStorageConfig.max_key_length 默认值改为与文件系统兼容值(如 200)或增加 le=200 约束;测试需补 251-255 字符与 CJK key 的写入用例。

Expand Down
Loading