fix(acp): retain only ephemeral-new-session persistence guard
Keep two invariants covering empty create/cwd/save/fork, genuine content and existing-row metadata. Preserve original authorship and avoid source-only legacy pruning: an empty ACP row does not prove its owner is dead. Native ACP wire plus a local streaming model fixture verifies the first turn and nonempty fork remain durable.
This commit is contained in:
+1
-19
@@ -294,25 +294,7 @@ class SessionManager:
|
||||
try:
|
||||
if db.get_session(state.session_id) is None:
|
||||
if not state.history:
|
||||
# Defer row creation until the session has actually exchanged
|
||||
# something. ACP clients open a session BEFORE knowing whether a
|
||||
# prompt is coming, and some open one purely to interrogate the
|
||||
# agent: bb's model discovery spawns a throwaway `hermes acp`,
|
||||
# sends initialize + session/new, reads the model catalog off the
|
||||
# response, and kills the process without ever prompting. Its
|
||||
# cache TTL is 60s, so an open editor re-probes on a ~10-15 minute
|
||||
# cadence and every probe used to leave a message_count=0 shell in
|
||||
# state.db, indistinguishable from a real chat in the session list
|
||||
# and unreapable by `hermes sessions prune` (these rows never end,
|
||||
# so ended_at stays NULL and prune skips them).
|
||||
#
|
||||
# Nothing is lost for a genuine conversation: AIAgent's
|
||||
# _ensure_db_session() creates the row on the first turn and the
|
||||
# post-prompt save_session() lands the ACP metadata on top.
|
||||
#
|
||||
# Gate on state.history rather than message_count so fork_session,
|
||||
# which deep-copies a non-empty history into a fresh id, still
|
||||
# persists immediately.
|
||||
# Empty editor probes stay ephemeral; copied fork history persists.
|
||||
return
|
||||
db.create_session(session_id=state.session_id, source="acp", model=model_str,
|
||||
model_config={"cwd": state.cwd})
|
||||
|
||||
@@ -0,0 +1,2 @@
|
||||
Willhong
|
||||
# ACP empty-session prevention salvaged from #104726
|
||||
@@ -75,7 +75,7 @@ def main():
|
||||
env.update(HOME=str(home), HERMES_HOME=str(hermes), PYTHONPATH=os.pathsep.join([str(repo), os.environ.get('PYTHONPATH', '')]),
|
||||
HERMES_ACP_SKIP_CONFIGURED_MCP='1', OPENAI_API_KEY='local-fixture-key',
|
||||
OPENAI_BASE_URL=f'http://127.0.0.1:{server.server_port}/v1')
|
||||
stderr = open(str(args.output) + '.stderr', 'w')
|
||||
stderr = open(str(args.output) + '.stderr', 'w', encoding='utf-8')
|
||||
proc = subprocess.Popen([sys.executable, '-m', 'acp_adapter'], cwd=repo, env=env,
|
||||
stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=stderr, text=True)
|
||||
received = queue.Queue()
|
||||
@@ -133,7 +133,7 @@ def main():
|
||||
stderr.close()
|
||||
server.shutdown()
|
||||
result.update(wire=wire, model_requests=requests)
|
||||
Path(args.output).write_text(json.dumps(result, indent=2))
|
||||
Path(args.output).write_text(json.dumps(result, indent=2), encoding='utf-8')
|
||||
print(json.dumps({k: v for k, v in result.items() if k not in ('wire', 'model_requests', 'messages')}, indent=2))
|
||||
|
||||
|
||||
|
||||
@@ -28,11 +28,11 @@ def test_new_session_persists_only_when_content_exists(tmp_path):
|
||||
def test_existing_empty_history_still_updates_metadata(tmp_path):
|
||||
db = SessionDB(tmp_path / "state.db")
|
||||
manager = SessionManager(db=db, agent_factory=lambda: SimpleNamespace(model="fixture"))
|
||||
state = manager.create_session(cwd=str(tmp_path))
|
||||
# A genuine agent creates the row before its first model reply. An old,
|
||||
# unprompted ACP client may also still own its row: source is not liveness.
|
||||
if db.get_session(state.session_id) is None:
|
||||
db.create_session(session_id=state.session_id, source="acp", model="original")
|
||||
# An old, unprompted ACP client may still own its row: source is not liveness.
|
||||
db.create_session(session_id="existing", source="acp", model="original")
|
||||
state = manager.get_session("existing")
|
||||
assert state is not None
|
||||
assert not state.history
|
||||
state.model = "selected-model"
|
||||
manager.update_cwd(state.session_id, str(tmp_path / "selected"))
|
||||
row = db.get_session(state.session_id)
|
||||
|
||||
@@ -249,47 +249,7 @@ class TestListAndCleanup:
|
||||
class TestPersistence:
|
||||
"""Verify that sessions are persisted to SessionDB and can be restored."""
|
||||
|
||||
def test_create_session_does_not_mint_an_empty_db_row(self, manager):
|
||||
"""A session with no history must NOT create a state.db row.
|
||||
|
||||
ACP clients open a session before knowing whether a prompt is coming,
|
||||
and some open one purely to interrogate the agent (a model-catalog
|
||||
probe that sends initialize + session/new, reads the response, and
|
||||
exits). Persisting on create left a message_count=0 shell per probe,
|
||||
indistinguishable from a real chat in the session list and unreapable
|
||||
by `hermes sessions prune` (the rows never end, so ended_at stays NULL).
|
||||
"""
|
||||
db = manager._get_db()
|
||||
before = len(db.search_sessions(source="acp", limit=10000))
|
||||
|
||||
state = manager.create_session(cwd="/tmp/probe")
|
||||
|
||||
assert db.get_session(state.session_id) is None
|
||||
assert len(db.search_sessions(source="acp", limit=10000)) == before
|
||||
|
||||
def test_session_row_is_created_once_history_exists(self, manager):
|
||||
"""The deferral must not lose a real conversation."""
|
||||
state = manager.create_session(cwd="/tmp/real")
|
||||
assert manager._get_db().get_session(state.session_id) is None
|
||||
|
||||
state.history.append({"role": "user", "content": "hello"})
|
||||
manager.save_session(state.session_id)
|
||||
|
||||
row = manager._get_db().get_session(state.session_id)
|
||||
assert row is not None
|
||||
assert row["source"] == "acp"
|
||||
|
||||
def test_fork_persists_immediately_because_history_is_copied(self, manager):
|
||||
"""fork_session copies a non-empty history, so its row lands at once."""
|
||||
parent = manager.create_session(cwd="/tmp/fork-src")
|
||||
parent.history.append({"role": "user", "content": "forked content"})
|
||||
manager.save_session(parent.session_id)
|
||||
|
||||
child = manager.fork_session(parent.session_id, cwd="/tmp/fork-dst")
|
||||
|
||||
assert child is not None
|
||||
assert child.session_id != parent.session_id
|
||||
assert manager._get_db().get_session(child.session_id) is not None
|
||||
|
||||
|
||||
|
||||
|
||||
@@ -323,7 +323,16 @@ Each session stores:
|
||||
- current conversation history
|
||||
- cancel event
|
||||
|
||||
The underlying `AIAgent` still uses Hermes' normal persistence/logging paths, but ACP `list/load/resume/fork` are scoped to the currently running ACP server process.
|
||||
Conversations are persisted to Hermes' session database and can be listed, loaded,
|
||||
resumed, or forked after the ACP server restarts. Opening a new session without a
|
||||
prompt keeps it in memory only: model-discovery probes do not create empty history
|
||||
rows. A nonempty fork is persisted immediately, and existing session metadata can
|
||||
still be updated even when its current history is empty.
|
||||
|
||||
Existing empty rows from older versions are not automatically deleted. An open ACP
|
||||
row does not prove its client has disconnected. After closing the relevant editor
|
||||
sessions, inspect unwanted rows with `hermes sessions show <id>` and remove only
|
||||
confirmed unwanted sessions with `hermes sessions delete <id>`.
|
||||
|
||||
## Working directory behavior
|
||||
|
||||
|
||||
Reference in New Issue
Block a user