fix(plugins): fire on_session_delete on every session-deletion path

[Finn763]

Adds the on_session_delete post-commit observer requested in #124511 so plugins with per-session on-disk state stop inferring deletion by diffing the filesystem against core's private schema.

Mechanism: new late-bound seam emit_session_deleted() in hermes_state_common.py (function-local import of hermes_cli.lifecycle.notify_session_deleted, fail-open) fired after the write commit in delete_session, delete_session_if_empty, delete_sessions, delete_empty_sessions (reason empty_sweep), prune_sessions (reason prune), and prune_empty_ghost_sessions (reason ghost_prune). Gateway never-active prune is covered via delete_session. One invocation per operation; deleted_ids rides along only when the count differs from 1. Registered in VALID_HOOKS plus the hooks sample-payload map; unbounded caller-thread dispatch like on_session_finalize. EN docs tables plus section; zh-Hans table rows mirrored.

RED-to-GREEN: repro asserting delete_session fires the hook failed on origin/main (hook absent from VALID_HOOKS, no dispatch); passes after. tests/hermes_state/test_session_delete_hook.py: 7 passed. Neighbors: TestDeleteAndExport, test_session_boundary_hooks, test_lifecycle: 11 passed.

Risks: new observer adds one fan-out per delete op; failing callbacks are logged and skipped and can never break deletion. No signature changes; no veto semantics.

Relations: Related to #123272 (transcript scrub on dashboard/gateway deletes covers overlapping call sites). Closes #124511.

[Enough1122]

AI code review — automated follow-up for reference; not a maintainer.

Wiring on_session_delete into every session-row delete is the right shape. The post-commit, fail-open dispatch (hermes_cli/lifecycle.py:75, hermes_state_common.py:349) is correct: a raising observer cannot block a delete, and unknown ids are skipped. Six call sites in hermes_state_sessions.py / hermes_state_maintenance.py are wired, and the cascade helper _delete_delegate_children (hermes_state_sessions.py:167) is correctly folded into removed_ids, so the new deleted_ids doc claim holds. One session-delete entry point is still on the old behaviour.

1. delete_moved_session never fires the hook — hermes_state_profile_repair.py:243. This is a seventh delete path that removes a sessions row (line 253, in SessionProfileRepairMixin), and the file has zero emit_session_deleted references. Its production caller is hermes_cli/sessions_repair_profiles.py:424, the loop that drops the source-profile row after the target profile confirmed the move. A plugin holding per-session on-disk state for the source profile never learns the session died, and leaks it. Repro on head ba014e6, real SessionDB, real call path, one spy on lifecycle.notify_session_deleted:

call	row removed	hook fired
db.delete_session("s1")	yes	1
db.delete_moved_session("moved1")	yes	0

This is the "one branch changed, siblings silently keep the old behaviour" shape: delete_session, delete_sessions, delete_session_if_empty, delete_empty_sessions, prune_sessions and prune_empty_ghost_sessions all got the call; this one did not. Fix mirrors hermes_state_sessions.py:1595 — collect the removed id, then emit_session_deleted([session_id], reason="profile_repair") after _execute_write returns truthy.

Minor: gateway/platforms/api_server.py:3140 also runs DELETE FROM sessions, but it rolls back a row inserted in the same transaction (title conflict), so I do not think it should fire. A comment there would stop the next reader from adding the hook and double-counting.

Unverified / please confirm: the gap, both hook counts and the fix come from running the table on head ba014e6. sessions_repair_profiles.py:424 is the only non-test caller at head (grep) — please confirm nothing reaches it via a path I did not see. The api_server 3140 judgement is code reading only, not executed.

[Finn763]

Addressed the automated review finding in e2d59c4942f5 (amended head):

1. delete_moved_session now fires the hook — hermes_state_profile_repair.py:260. The delete path collected nothing and returned straight through, so the seventh delete path never notified anyone. It now mirrors hermes_state_sessions.py:1595: capture the write result, then emit_session_deleted([session_id], reason="profile_repair") only when the transaction committed, and still return the bool. emit_session_deleted is imported from hermes_state_common (the module already imported the placeholder/_id_chunks helpers from there, so no new seam).
2. gateway/platforms/api_server.py:3140 — agreed it must not fire; added the two-line comment there so the next reader does not wire it and double-count a row that never existed.

Confirmed your unverified point: grep -rn delete_moved_session gives three hits — the definition (hermes_state_profile_repair.py:243), the production caller (hermes_cli/sessions_repair_profiles.py:424, the drop-source-row loop), and one test reference (tests/hermes_cli/test_sessions_repair_profiles.py:294). No other non-test path reaches it.

Verification on the new head:

• uv run pytest tests/hermes_state/test_session_delete_hook.py tests/hermes_cli/test_sessions_repair_profiles.py -q → 15 passed.
• Two new cases in tests/hermes_state/test_session_delete_hook.py: test_delete_moved_session_fires_with_reason (one call, session_id == "moved1", reason == "profile_repair") and test_delete_moved_session_unknown_id_does_not_fire (returns False, no emit). Your repro table is now a test rather than a manual spy.
• Red check: reverting the emit back to return bool(self._execute_write(_do)) on the same tree → 1 failed, 8 passed, the failure being exactly test_delete_moved_session_fires_with_reason (assert 0 == 1). Restored and re-run green.

Whole-directory run uv run pytest tests/hermes_state -q: 1310 passed, 10 failed, 124 skipped. None of the failures touch this change — they are platform-specific process/lock tests, and 9 of the 10 reproduce identically with both touched files reverted to the pre-change commit (test_auto_vacuum_in_process_holder_gate retired-generation case, the three test_cross_vm_fs_wal_refusal /data, /mnt/host/db, /mnt/my share/db virtiofs/9p cases, two test_shared_session_db_registry teardown-barrier cases, test_state_db_repair_non_destructive::test_repair_outcome_is_recorded_while_cross_process_lock_is_held, test_state_db_second_process_maintenance::test_holder_scan_sees_through_a_symlinked_home, test_write_lock_owner_attribution::test_parse_proc_locks_...). The tenth (test_state_db_repair_non_destructive::test_interrupted_snapshot_rolls_back_destination) passes standalone on this head and on the pre-change tree — load-dependent, not path-dependent.

[Enough1122]

Confirming on head e2d59c4 — I re-read the post-image rather than taking the reply at face value, so this is a verification, not an acknowledgement.

1. delete_moved_session now emits the hook. Confirmed at post-image. hermes_state_profile_repair.py:260-262 is the fix: the _do(conn) transaction still returns False early when the row is absent, and emit_session_deleted([session_id], reason="profile_repair") runs only under if deleted:. So the no-row case is still a clean no-op and the fired case is the one that actually removed something — the same shape as delete_session, and passing the id list rather than a single id is right for cascade consumers.

What I checked beyond the line itself, since the contract is "every deletion path notifies": the gate is the transaction's boolean return, not a row count, and _delete_unreferenced_system_prompts(conn) at :258 runs inside that same transaction and can in principle remove prompt rows other sessions still reference by string id. If it ever returned cleanly while removing nothing, your flag would still be correct — I could not construct a case where the transaction returns True with zero session rows removed. So the gate is sound, and it is strictly tighter than the pre-image, which had no gate at all.

Worth stating for the other six paths: they key off the same "did this transaction actually remove the row" condition rather than a blanket unconditional fire, so an idempotent re-delete stays silent instead of emitting a phantom delete. That is the behaviour a plugin holding per-session on-disk state needs — a duplicate on_session_delete would make it drop state for a session id that another live session could later reuse. If that reasoning is not the intent, say so and I will re-check; but as written I believe it is.

That was the whole of my report on this PR. Nothing outstanding, no residual, and I have no objection to the other six paths as they stand.

[ljhade24]

Related integration use case for this generic hook: an opt-in external plugin pairs a messaging thread with Desktop session history and removes its own proven-owned temporary files when the user deletes that conversation. I am not proposing that this policy or a vendor-specific cleanup engine be added to core.

Reviewing head e2d59c4, the post-commit, fail-open notification and exact cascaded deleted IDs are useful. There are additional contracts needed before this observer can safely drive external deletion:

1. Caller provenance: explicit user deletion must be distinguishable from retention, empty sweeps, compensation and profile moves. The state helper currently receives reason/sessions_dir, but a stable surface/profile + explicit-user reason should be supplied by the actual REST/RPC callers. An unknown reason must never authorize deleting an external resource.
2. Pre-deletion identity: after commit the removed rows are unavailable. A plugin needs a supported way to preserve minimal profile/platform/thread mapping before deletion, without exposing message content or querying private tables in a race. This might be a separate narrow preparation hook/context API; it should not introduce veto semantics into this post-commit observer.
3. Process ownership: Desktop/backend and the messaging gateway can be different processes. The process-local observer is valuable, but should not be documented as durable cross-process delivery. External cleanup needs a supported handoff or a documented plugin-owned mechanism, with no guarantee of exactly-once external side effects.

Would you prefer these as a follow-up generic extension rather than expanding this PR? Keeping the current notification PR focused seems reasonable. Happy to contribute a focused follow-up with rollback/no-op, profile isolation, prune-vs-user-delete and cross-process tests. A separate Desktop deletion-visibility fix is being prepared independently; no competing on_session_delete PR is intended.

[Finn763]

Thanks for the careful pass. Agree with keeping this PR focused -- the observer contract stands on its own, so I'd rather not fold the external-consumer contracts into it.

On (3): pushed a docs clarification in cace3e6631c2 -- the hook section now states delivery is process-local (runs only in the process that committed the delete), that it is not a durable cross-process delivery channel, and that external side effects get no exactly-once guarantee.

On (1) and (2): both read as the right follow-up scope -- authoring the provenance distinction (explicit user deletion vs retention/sweep/compensation) and a pre-deletion identity/preparation surface. If you take that follow-up (rollback/no-op, profile isolation, prune-vs-user-delete, cross-process tests), happy to review it; I won't open anything competing here. Noted re the separate Desktop deletion-visibility fix -- no overlap from my side.

[ljhade24]

Thanks — I am preparing an independent, draft follow-up against current main (no duplicate on_session_delete implementation). Proposed narrow contract: actual REST/RPC delete callers explicitly mark user intent; within the guarded delete transaction core snapshots only exact session/routing identifiers, and an optional preparation hook can stage plugin-owned pending work. A small generic committed deletion receipt, read through a supported SessionDB API, would let a different gateway process confirm that pending work committed, including after a backend crash; plugins retain ownership checks, retry/cursor state and external actions. No message content, vendor policy, veto on the existing post-commit observer, or exactly-once external guarantees. Unknown/maintenance/compensation/profile-move paths would not authorize external cleanup. Maintainer design question: is a generic transactional receipt plus preparation hook acceptable, or would you prefer plugin-owned durable receipts with an explicitly constrained transactional writer surface? I will keep this draft pending that decision and include rollback/no-op, guarded/cascaded IDs, profile isolation and real cross-process tests.

[ljhade24]

Published the focused independent draft follow-up: #135513 (head 4808cab, base main). @Finn763, your offered review would be welcome. The draft implements caller-authored REST/RPC user intent plus transactionally prepared minimal routing identities and public committed-receipt reads for a plugin-owned durable cross-process ledger. I intentionally did not execute an arbitrary preparation callback inside the SQLite write transaction: re-entrant DB access and external actions before rollback would be unsafe without a constrained contract. The maintainers’ concrete decision is whether this small generic receipt store/read surface is preferable to a constrained transactional writer callback, and what receipt-retention/consumer coordination policy is required. Focused checks: 93 tests passed, 8 RPC deletion neighbors passed, and all 11 blocking lint checks passed. Broad state-suite verification is incomplete and had failures (representative failures reproduced on unchanged base); draft limitations are explicit. No duplication or veto changes to your post-commit observer, no vendor policy, runtime deployment or exactly-once external guarantee.

[teknium1]

@Finn763 your two commits landed verbatim in #140995 (merged at 627ce03) with authorship kept: ee3ce9a (state-layer hook on every delete path) and 9949552 (process-local delivery docs).

On top of them, #140995 added:

• Desktop/dashboard single delete and archive coverage. These go through the canonical runtime receipt (mutate_runtime_session), not SessionDB.delete_session.
• Deliberate archives, as reason="archive".
• reason and profile on every event.
• Dispatch under the store's own profile home.
• Firing before media cleanup.

Thanks for the groundwork.