Skip to content

fix: fire notification hooks for approvals #2284

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
he-yufeng wants to merge 1 commit into
base: main
Choose a base branch
from
+140 −1
Open

fix: fire notification hooks for approvals #2284

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
47 changes: 47 additions & 0 deletions src/kimi_cli/soul/kimisoul.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
from tenacity import RetryCallState, retry_if_exception, stop_after_attempt, wait_exponential_jitter

from kimi_cli.approval_runtime import (
ApprovalRuntimeEvent,
ApprovalSource,
get_current_approval_source_or_none,
reset_current_approval_source,
Expand Down Expand Up @@ -209,8 +210,13 @@ def __init__(
]
self._hook_engine: HookEngine = HookEngine()
self._stop_hook_active: bool = False
self._approval_hook_subscription: str | None = None
if self.is_root:
self._runtime.notifications.ack_ids("llm", extract_notification_ids(context.history))
if self._runtime.approval_runtime is not None:
self._approval_hook_subscription = self._runtime.approval_runtime.subscribe(
self._on_approval_runtime_event
)
Comment on lines +217 to +219
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot May 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Unsubscribe approval-event listener when creating souls

Avoid registering a permanent approval-runtime subscriber in KimiSoul.__init__ without removing it later. ApprovalRuntime.subscribe() stores the bound callback strongly, and this class currently has no corresponding unsubscribe(), so temporary root souls (for example /init, which creates KimiSoul(soul.agent, ...) in src/kimi_cli/soul/slash.py) accumulate listeners. After each /init, every new approval request will fire the Notification hook multiple times and keep old soul instances alive in memory.

Useful? React with 👍 / 👎.

Comment on lines +213 to +219
Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot May 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Approval runtime subscription is never unsubscribed, causing resource leak

The subscription token is stored in _approval_hook_subscription but unsubscribe() is never called anywhere in KimiSoul's lifecycle. The class has no cleanup/close method. This leaks the subscription callback (a bound method holding a strong reference to self) for the lifetime of the ApprovalRuntime.

The concrete impact is in src/kimi_cli/soul/slash.py:44 where the /init command creates a temporary KimiSoul sharing the same root runtime. That temporary soul subscribes at line 217, but when it goes out of scope, the subscription keeps it alive. Every subsequent create_request() call invokes the leaked callback, which calls fire_and_forget_trigger on a stale, empty HookEngine. The established pattern in this codebase (see src/kimi_cli/background/agent_runner.py:128) is to always pair subscribe() with unsubscribe() in a cleanup block.

Prompt for agents 
The _approval_hook_subscription token is stored but never used to unsubscribe from the ApprovalRuntime. KimiSoul has no cleanup/close method.

The most straightforward fix is to add a cleanup method (e.g. close() or dispose()) to KimiSoul that calls self._runtime.approval_runtime.unsubscribe(self._approval_hook_subscription) when the token is not None. Then ensure this cleanup is called wherever KimiSoul instances are discarded — notably in src/kimi_cli/soul/slash.py:44-45 (the /init command creates a temporary KimiSoul that goes out of scope after the with block).

Alternatively, the /init path could be fixed by having the temporary soul not subscribe (e.g. by passing a flag), or by wrapping the temporary soul usage in a try/finally that calls unsubscribe.

See src/kimi_cli/background/agent_runner.py:128 for the established pattern of pairing subscribe() with unsubscribe() in a finally block.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.


# Bind plan mode state to tools that support it
self._bind_plan_mode_tools()
Expand Down Expand Up @@ -276,6 +282,40 @@ def set_hook_engine(self, engine: HookEngine) -> None:
if isinstance(self._agent.toolset, KimiToolset):
self._agent.toolset.set_hook_engine(engine)

def _on_approval_runtime_event(self, event: ApprovalRuntimeEvent) -> None:
if event.kind != "request_created":
return

from kimi_cli.hooks import events

request = event.request
input_data = {
**events.notification(
session_id=self._runtime.session.id,
cwd=str(Path.cwd()),
sink="user",
notification_type="permission_prompt",
title=f"Approval required: {request.sender}",
body=request.description,
severity="warning",
),
"request_id": request.id,
"tool_call_id": request.tool_call_id,
"sender": request.sender,
"action": request.action,
"description": request.description,
"source_kind": request.source.kind,
"source_id": request.source.id,
}
try:
self._hook_engine.fire_and_forget_trigger(
"Notification",
matcher_value="permission_prompt",
input_data=input_data,
)
except Exception:
logger.exception("Failed to trigger approval notification hook")

def add_injection_provider(self, provider: DynamicInjectionProvider) -> None:
"""Register an additional dynamic injection provider."""
self._injection_providers.append(provider)
Expand Down Expand Up @@ -507,6 +547,13 @@ def runtime(self) -> Runtime:
def context(self) -> Context:
return self._context

def close(self) -> None:
if self._approval_hook_subscription is None:
return
if self._runtime.approval_runtime is not None:
self._runtime.approval_runtime.unsubscribe(self._approval_hook_subscription)
self._approval_hook_subscription = None

@property
def _context_usage(self) -> float:
if self._runtime.llm is not None:
Expand Down
5 changes: 4 additions & 1 deletion src/kimi_cli/soul/slash.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,10 @@ async def init(soul: KimiSoul, args: str):
with tempfile.TemporaryDirectory() as temp_dir:
tmp_context = Context(file_backend=Path(temp_dir) / "context.jsonl")
tmp_soul = KimiSoul(soul.agent, context=tmp_context)
await tmp_soul.run(prompts.INIT)
try:
await tmp_soul.run(prompts.INIT)
finally:
tmp_soul.close()

agents_md = await load_agents_md(soul.runtime.builtin_args.KIMI_WORK_DIR)
system_message = system(
Expand Down
89 changes: 89 additions & 0 deletions tests/core/test_approval_runtime.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,95 @@ async def _drain_ui_messages(wire: Wire) -> None:
return


@pytest.mark.asyncio
async def test_kimisoul_triggers_notification_hook_for_approval_requests(
runtime, tmp_path, monkeypatch: pytest.MonkeyPatch
) -> None:
assert runtime.approval_runtime is not None

soul = KimiSoul(
SoulAgent(
name="test",
system_prompt="test prompt",
toolset=EmptyToolset(),
runtime=runtime,
),
context=Context(file_backend=tmp_path / "history.jsonl"),
)

calls: list[tuple[str, str, dict]] = []

async def _done() -> list:
return []

def fake_fire_and_forget(event: str, *, matcher_value: str, input_data: dict):
calls.append((event, matcher_value, input_data))
return asyncio.create_task(_done())

monkeypatch.setattr(soul.hook_engine, "fire_and_forget_trigger", fake_fire_and_forget)

runtime.approval_runtime.create_request(
request_id="req-notification-hook",
tool_call_id="call-notification-hook",
sender="WriteFile",
action="edit file",
description="Write file /tmp/test.txt",
display=[],
source=ApprovalSource(kind="foreground_turn", id="turn-notification-hook"),
)

assert len(calls) == 1
event, matcher_value, payload = calls[0]
assert event == "Notification"
assert matcher_value == "permission_prompt"
assert payload["notification_type"] == "permission_prompt"
assert payload["sender"] == "WriteFile"
assert payload["description"] == "Write file /tmp/test.txt"


@pytest.mark.asyncio
async def test_kimisoul_close_unsubscribes_approval_hook(
runtime, tmp_path, monkeypatch: pytest.MonkeyPatch
) -> None:
assert runtime.approval_runtime is not None

soul = KimiSoul(
SoulAgent(
name="test",
system_prompt="test prompt",
toolset=EmptyToolset(),
runtime=runtime,
),
context=Context(file_backend=tmp_path / "history.jsonl"),
)

calls: list[tuple[str, str, dict]] = []

async def _done() -> list:
return []

def fake_fire_and_forget(event: str, *, matcher_value: str, input_data: dict):
calls.append((event, matcher_value, input_data))
return asyncio.create_task(_done())

monkeypatch.setattr(soul.hook_engine, "fire_and_forget_trigger", fake_fire_and_forget)

soul.close()
soul.close()

runtime.approval_runtime.create_request(
request_id="req-closed-notification-hook",
tool_call_id="call-closed-notification-hook",
sender="WriteFile",
action="edit file",
description="Write file /tmp/test.txt",
display=[],
source=ApprovalSource(kind="foreground_turn", id="turn-closed-notification-hook"),
)

assert calls == []


@pytest.mark.asyncio
async def test_kimisoul_run_preserves_existing_approval_source(
runtime, tmp_path, monkeypatch
Expand Down