From 8f7640b6229b1b3a889b228b55cdd20b703aaea0 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Mon, 7 Sep 2026 02:33:59 -0700 Subject: [PATCH] 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. --- acp_adapter/session.py | 20 +--------- .../5318600+Willhong@users.noreply.github.com | 2 + evals/acp_empty_session_wire.py | 4 +- tests/acp/test_empty_session_persistence.py | 10 ++--- tests/acp/test_session.py | 40 ------------------- website/docs/user-guide/features/acp.md | 11 ++++- 6 files changed, 20 insertions(+), 67 deletions(-) create mode 100644 contributors/emails/5318600+Willhong@users.noreply.github.com diff --git a/acp_adapter/session.py b/acp_adapter/session.py index 9d292ac9aa..293a441740 100644 --- a/acp_adapter/session.py +++ b/acp_adapter/session.py @@ -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}) diff --git a/contributors/emails/5318600+Willhong@users.noreply.github.com b/contributors/emails/5318600+Willhong@users.noreply.github.com new file mode 100644 index 0000000000..f2375d916c --- /dev/null +++ b/contributors/emails/5318600+Willhong@users.noreply.github.com @@ -0,0 +1,2 @@ +Willhong +# ACP empty-session prevention salvaged from #104726 diff --git a/evals/acp_empty_session_wire.py b/evals/acp_empty_session_wire.py index 23cffcb6e1..c33d30a3fa 100644 --- a/evals/acp_empty_session_wire.py +++ b/evals/acp_empty_session_wire.py @@ -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)) diff --git a/tests/acp/test_empty_session_persistence.py b/tests/acp/test_empty_session_persistence.py index 7bbca46d24..95846eb574 100644 --- a/tests/acp/test_empty_session_persistence.py +++ b/tests/acp/test_empty_session_persistence.py @@ -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) diff --git a/tests/acp/test_session.py b/tests/acp/test_session.py index 1f1fafffc7..dc6180fbea 100644 --- a/tests/acp/test_session.py +++ b/tests/acp/test_session.py @@ -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 diff --git a/website/docs/user-guide/features/acp.md b/website/docs/user-guide/features/acp.md index 1424c427f0..aff86e2a32 100644 --- a/website/docs/user-guide/features/acp.md +++ b/website/docs/user-guide/features/acp.md @@ -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 ` and remove only +confirmed unwanted sessions with `hermes sessions delete `. ## Working directory behavior