fix: preserve CLI session tool metadata
This commit is contained in:
Michael Lam
test
parent
3bc8bc8bdd
commit
0bd65ef0bf
4 changed files with 273 additions and 6 deletions
|
|
@ -1618,10 +1618,24 @@ def get_cli_sessions() -> list:
|
|||
return cli_sessions
|
||||
|
||||
|
||||
def _json_loads_if_string(value):
|
||||
if not isinstance(value, str):
|
||||
return value
|
||||
text = value.strip()
|
||||
if not text:
|
||||
return None
|
||||
try:
|
||||
return json.loads(text)
|
||||
except Exception:
|
||||
return value
|
||||
|
||||
|
||||
def get_cli_session_messages(sid) -> list:
|
||||
"""Read messages for a single CLI/external-agent session.
|
||||
Returns a list of {role, content, timestamp} dicts.
|
||||
Returns empty list on any error.
|
||||
|
||||
Preserve tool-call/result and reasoning metadata from the agent state.db so
|
||||
CLI-origin transcripts render with the same tool cards as WebUI-native
|
||||
sessions. Returns empty list on any error.
|
||||
"""
|
||||
import os
|
||||
if str(sid or '').startswith(f'{CLAUDE_CODE_SOURCE}_'):
|
||||
|
|
@ -1644,19 +1658,47 @@ def get_cli_session_messages(sid) -> list:
|
|||
with closing(sqlite3.connect(str(db_path))) as conn:
|
||||
conn.row_factory = sqlite3.Row
|
||||
cur = conn.cursor()
|
||||
cur.execute("""
|
||||
SELECT role, content, timestamp
|
||||
cur.execute("PRAGMA table_info(messages)")
|
||||
available = {str(row['name']) for row in cur.fetchall()}
|
||||
required = {'role', 'content', 'timestamp'}
|
||||
if not required.issubset(available):
|
||||
return []
|
||||
optional = [
|
||||
'tool_call_id',
|
||||
'tool_calls',
|
||||
'tool_name',
|
||||
'reasoning',
|
||||
'reasoning_details',
|
||||
'codex_reasoning_items',
|
||||
'reasoning_content',
|
||||
'codex_message_items',
|
||||
]
|
||||
selected = ['role', 'content', 'timestamp'] + [c for c in optional if c in available]
|
||||
cur.execute(f"""
|
||||
SELECT {', '.join(selected)}
|
||||
FROM messages
|
||||
WHERE session_id = ?
|
||||
ORDER BY timestamp ASC
|
||||
""", (sid,))
|
||||
msgs = []
|
||||
for row in cur.fetchall():
|
||||
msgs.append({
|
||||
msg = {
|
||||
'role': row['role'],
|
||||
'content': row['content'],
|
||||
'timestamp': row['timestamp'],
|
||||
})
|
||||
}
|
||||
for col in optional:
|
||||
if col not in row.keys():
|
||||
continue
|
||||
value = row[col]
|
||||
if value in (None, ''):
|
||||
continue
|
||||
if col in {'tool_calls', 'reasoning_details', 'codex_reasoning_items', 'codex_message_items'}:
|
||||
value = _json_loads_if_string(value)
|
||||
msg[col] = value
|
||||
if msg.get('role') == 'tool' and msg.get('tool_name') and not msg.get('name'):
|
||||
msg['name'] = msg['tool_name']
|
||||
msgs.append(msg)
|
||||
except Exception:
|
||||
return []
|
||||
return msgs
|
||||
|
|
|
|||
|
|
@ -7861,6 +7861,49 @@ def _normalize_message_for_import_refresh(message: object) -> object:
|
|||
return normalized
|
||||
|
||||
|
||||
def _message_has_cli_tool_metadata(message: object) -> bool:
|
||||
if not isinstance(message, dict):
|
||||
return False
|
||||
if message.get("role") == "assistant" and message.get("tool_calls"):
|
||||
return True
|
||||
if message.get("role") == "tool" and (message.get("tool_call_id") or message.get("tool_name") or message.get("name")):
|
||||
return True
|
||||
return False
|
||||
|
||||
|
||||
def _strip_cli_tool_metadata_for_refresh(message: object) -> object:
|
||||
if not isinstance(message, dict):
|
||||
return _normalize_message_for_import_refresh(message)
|
||||
normalized = _normalize_message_for_import_refresh(message)
|
||||
if not isinstance(normalized, dict):
|
||||
return normalized
|
||||
for key in ("tool_calls", "tool_call_id", "tool_name", "name"):
|
||||
normalized.pop(key, None)
|
||||
return normalized
|
||||
|
||||
|
||||
def _is_cli_tool_metadata_enrichment(existing_messages: list, fresh_messages: list) -> bool:
|
||||
"""Return True when fresh messages only add CLI tool metadata.
|
||||
|
||||
Older imports from get_cli_session_messages() persisted assistant/tool rows
|
||||
without tool_calls, tool_call_id, or tool_name. After #1772 the refreshed
|
||||
transcript can have the same length but richer metadata, so re-imports must
|
||||
rebuild the stored sidecar even without a new row.
|
||||
"""
|
||||
if not isinstance(existing_messages, list) or not isinstance(fresh_messages, list):
|
||||
return False
|
||||
if len(existing_messages) != len(fresh_messages):
|
||||
return False
|
||||
if any(_message_has_cli_tool_metadata(m) for m in existing_messages):
|
||||
return False
|
||||
if not any(_message_has_cli_tool_metadata(m) for m in fresh_messages):
|
||||
return False
|
||||
for idx, existing_message in enumerate(existing_messages):
|
||||
if _strip_cli_tool_metadata_for_refresh(existing_message) != _strip_cli_tool_metadata_for_refresh(fresh_messages[idx]):
|
||||
return False
|
||||
return True
|
||||
|
||||
|
||||
def _is_messages_refresh_prefix_match(existing_messages: list, fresh_messages: list) -> bool:
|
||||
"""Return True when existing_messages is a prefix of fresh_messages by value.
|
||||
|
||||
|
|
@ -7905,6 +7948,11 @@ def _handle_session_import_cli(handler, body):
|
|||
if _is_messages_refresh_prefix_match(existing.messages, fresh_msgs):
|
||||
existing.messages = fresh_msgs
|
||||
changed = True
|
||||
elif fresh_msgs and _is_cli_tool_metadata_enrichment(existing.messages, fresh_msgs):
|
||||
# Same row count, richer payload: rebuild sidecars imported before
|
||||
# CLI tool metadata was preserved (#1772).
|
||||
existing.messages = fresh_msgs
|
||||
changed = True
|
||||
if cli_meta:
|
||||
updates = {
|
||||
"is_cli_session": True,
|
||||
|
|
|
|||
25
docs/pr-media/1772/cli-tool-metadata-api-evidence.json
Normal file
25
docs/pr-media/1772/cli-tool-metadata-api-evidence.json
Normal file
|
|
@ -0,0 +1,25 @@
|
|||
{
|
||||
"issue": 1772,
|
||||
"check": "api.models.get_cli_session_messages preserves CLI tool metadata for WebUI rendering",
|
||||
"session_id": "cli_issue_1772_demo",
|
||||
"message_count": 2,
|
||||
"assistant_tool_calls": [
|
||||
{
|
||||
"id": "call_1772_demo",
|
||||
"type": "function",
|
||||
"function": {
|
||||
"name": "terminal",
|
||||
"arguments": "{\"command\": \"printf ok\"}"
|
||||
}
|
||||
}
|
||||
],
|
||||
"tool_result": {
|
||||
"role": "tool",
|
||||
"tool_call_id": "call_1772_demo",
|
||||
"tool_name": "terminal",
|
||||
"name": "terminal",
|
||||
"content": {
|
||||
"output": "ok"
|
||||
}
|
||||
}
|
||||
}
|
||||
152
tests/test_cli_session_tool_metadata.py
Normal file
152
tests/test_cli_session_tool_metadata.py
Normal file
|
|
@ -0,0 +1,152 @@
|
|||
"""Regression coverage for CLI session tool-call metadata import (#1772)."""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import json
|
||||
import sqlite3
|
||||
|
||||
import api.models as models
|
||||
|
||||
|
||||
def _patch_active_home(monkeypatch, home):
|
||||
import api.profiles as profiles
|
||||
|
||||
monkeypatch.setattr(profiles, "get_active_hermes_home", lambda: home)
|
||||
monkeypatch.setattr(profiles, "get_active_profile_name", lambda: None)
|
||||
|
||||
|
||||
def _create_state_db_with_tool_turn(path, session_id="cli_tool_session_001"):
|
||||
conn = sqlite3.connect(str(path))
|
||||
conn.execute(
|
||||
"""
|
||||
CREATE TABLE messages (
|
||||
id INTEGER PRIMARY KEY AUTOINCREMENT,
|
||||
session_id TEXT NOT NULL,
|
||||
role TEXT NOT NULL,
|
||||
content TEXT,
|
||||
tool_call_id TEXT,
|
||||
tool_calls TEXT,
|
||||
tool_name TEXT,
|
||||
timestamp REAL NOT NULL,
|
||||
token_count INTEGER,
|
||||
finish_reason TEXT,
|
||||
reasoning TEXT,
|
||||
reasoning_details TEXT,
|
||||
codex_reasoning_items TEXT,
|
||||
reasoning_content TEXT,
|
||||
codex_message_items TEXT
|
||||
)
|
||||
"""
|
||||
)
|
||||
tool_calls = [
|
||||
{
|
||||
"id": "call_123",
|
||||
"type": "function",
|
||||
"function": {
|
||||
"name": "terminal",
|
||||
"arguments": json.dumps({"command": "printf ok"}),
|
||||
},
|
||||
}
|
||||
]
|
||||
conn.execute(
|
||||
"""
|
||||
INSERT INTO messages (
|
||||
session_id, role, content, tool_calls, timestamp, reasoning, reasoning_content
|
||||
) VALUES (?, 'assistant', '', ?, 1.0, 'Need a shell check', 'Need a shell check')
|
||||
""",
|
||||
(session_id, json.dumps(tool_calls)),
|
||||
)
|
||||
conn.execute(
|
||||
"""
|
||||
INSERT INTO messages (
|
||||
session_id, role, content, tool_call_id, tool_name, timestamp
|
||||
) VALUES (?, 'tool', ?, 'call_123', 'terminal', 2.0)
|
||||
""",
|
||||
(session_id, json.dumps({"output": "ok"})),
|
||||
)
|
||||
conn.commit()
|
||||
conn.close()
|
||||
return tool_calls
|
||||
|
||||
|
||||
def test_get_cli_session_messages_preserves_tool_call_metadata(tmp_path, monkeypatch):
|
||||
hermes_home = tmp_path / "hermes"
|
||||
hermes_home.mkdir()
|
||||
_patch_active_home(monkeypatch, hermes_home)
|
||||
expected_tool_calls = _create_state_db_with_tool_turn(hermes_home / "state.db")
|
||||
|
||||
messages = models.get_cli_session_messages("cli_tool_session_001")
|
||||
|
||||
assert messages[0]["role"] == "assistant"
|
||||
assert messages[0]["content"] == ""
|
||||
assert messages[0]["tool_calls"] == expected_tool_calls
|
||||
assert messages[0]["reasoning"] == "Need a shell check"
|
||||
assert messages[0]["reasoning_content"] == "Need a shell check"
|
||||
assert messages[1]["role"] == "tool"
|
||||
assert messages[1]["tool_call_id"] == "call_123"
|
||||
assert messages[1]["tool_name"] == "terminal"
|
||||
assert messages[1]["name"] == "terminal"
|
||||
assert json.loads(messages[1]["content"])["output"] == "ok"
|
||||
|
||||
|
||||
def test_existing_cli_import_refreshes_same_length_tool_metadata(monkeypatch):
|
||||
"""Previously imported CLI sessions with stripped metadata must be rebuilt.
|
||||
|
||||
The broken importer saved the same assistant/tool rows without tool_calls,
|
||||
tool_call_id, or tool_name. A later import after the loader fix has the same
|
||||
message count, so the refresh path must still replace the stripped messages.
|
||||
"""
|
||||
import api.routes as routes
|
||||
|
||||
session_id = "existing_cli_tool_session_001"
|
||||
stripped = [
|
||||
{"role": "assistant", "content": "", "timestamp": 1.0},
|
||||
{"role": "tool", "content": json.dumps({"output": "ok"}), "timestamp": 2.0},
|
||||
]
|
||||
enriched = [
|
||||
{
|
||||
"role": "assistant",
|
||||
"content": "",
|
||||
"timestamp": 1.0,
|
||||
"tool_calls": [{"id": "call_123", "function": {"name": "terminal", "arguments": "{}"}}],
|
||||
},
|
||||
{
|
||||
"role": "tool",
|
||||
"content": json.dumps({"output": "ok"}),
|
||||
"timestamp": 2.0,
|
||||
"tool_call_id": "call_123",
|
||||
"tool_name": "terminal",
|
||||
"name": "terminal",
|
||||
},
|
||||
]
|
||||
|
||||
class FakeSession:
|
||||
def __init__(self):
|
||||
self.messages = list(stripped)
|
||||
self.source_tag = "cli"
|
||||
self.raw_source = "cli"
|
||||
self.session_source = "cli"
|
||||
self.source_label = "CLI"
|
||||
self.parent_session_id = None
|
||||
self.is_cli_session = True
|
||||
|
||||
def compact(self):
|
||||
return {"session_id": session_id, "title": "Imported CLI"}
|
||||
|
||||
def save(self, touch_updated_at=False):
|
||||
save_calls.append(touch_updated_at)
|
||||
|
||||
save_calls = []
|
||||
existing = FakeSession()
|
||||
monkeypatch.setattr(routes.Session, "load", classmethod(lambda _cls, sid: existing if sid == session_id else None))
|
||||
monkeypatch.setattr(routes, "require", lambda body, *keys: None)
|
||||
monkeypatch.setattr(routes, "j", lambda _handler, payload, status=200, extra_headers=None: payload)
|
||||
monkeypatch.setattr(routes, "get_cli_session_messages", lambda sid: enriched if sid == session_id else [])
|
||||
monkeypatch.setattr(routes, "get_cli_sessions", lambda: [{"session_id": session_id, "source_tag": "cli", "raw_source": "cli", "session_source": "cli", "source_label": "CLI"}])
|
||||
|
||||
response = routes._handle_session_import_cli(object(), {"session_id": session_id})
|
||||
|
||||
assert response["imported"] is False
|
||||
assert existing.messages == enriched
|
||||
assert response["session"]["messages"] == enriched
|
||||
assert save_calls == [False]
|
||||
Loading…
Reference in a new issue