This does look interesting and useful - I see you've made a lot of extra commits - would you say its ready for review now?

This does look interesting and useful - I see you've made a lot of extra commits - would you say its ready for review now?

Yes, it's ready for review! The relevant commits are the last 3:

feat(tools): add notification_tool with desktop/system notifications
feat(tools): register notification toolset
feat(tools): discover notification_tool on startup

The earlier commits in the branch are unrelated work from my fork. The actual changes are clean — one new file (tools/notification_tool.py) and two small modifications (toolsets.py, model_tools.py). Happy to clean up the branch history if needed!

Hey @ygd58 — this is Hermes-Agent (Teknium's AI assistant) doing code review on this PR. The notification concept is great — alerting users when long tasks finish is a real gap, especially for CLI users. However, there are several blocking issues that mean this needs a ground-up rebuild rather than fixes on top. I'll break them down:

1. Stale Branch (blocker)

The PR touches 22 files but only 3 are relevant to notifications (notification_tool.py, toolsets.py, model_tools.py). The rest are unrelated additions (SELF_TOOL_BUILDER.md, ADVENTURE.md, code_sandbox.py, github_intel.py, hf_intel.py, notion_pro.py, pomodoro_tool.py, weather.py, etc.). This can't be merged without pulling in a large amount of unreviewed code.

2. Wrong Registration Pattern (blocker — tool won't load)

The PR uses a def register(registry) wrapper function, but the codebase uses module-level registration at import time:

# What the PR does:
def register(registry):
    registry.register(name=..., description=..., parameters=..., handler=...)

# What the codebase actually expects (see tools/tts_tool.py, tools/terminal_tool.py, etc.):
from tools.registry import registry

registry.register(
    name="tool_name",
    toolset="toolset_name",    # required, missing from PR
    schema={...},              # full OpenAI-format schema dict, not separate params
    handler=lambda args, **kw: ...,
    check_fn=check_function,
)

The tool would fail to load entirely because _discover_tools() does importlib.import_module() and expects module-level registry.register() calls.

3. Handler Signature Mismatch (blocker)

PR handlers: handle_notify(title, message, urgency="normal")
Registry dispatches: entry.handler(args, **kwargs) where args is a dict

Even if registration worked, every call would crash with a TypeError. Existing tools use lambda wrappers:

handler=lambda args, **kw: handle_notify(
    title=args.get("title", ""),
    message=args.get("message", ""),
    urgency=args.get("urgency", "normal"),
)

4. Remote Machine / SSH (design issue)

This is the big conceptual problem. Hermes often runs on remote machines via SSH — which is exactly the scenario where "long-running task finished" notifications are most wanted. But:

• notify-send requires D-Bus + display server (X11/Wayland)
• osascript requires a macOS GUI session
• afplay/paplay require audio output

Over SSH, none of these work. The check_fn would return False (good — graceful degradation), but the tool becomes unavailable in its primary use case. A rebuilt version should consider alternatives for remote scenarios (e.g., terminal bell \a, or integration with the existing send_message gateway tool).

5. Command Injection Vulnerability (security)

_esc() does basic quote replacement:

def _esc(s: str) -> str:
    return s.replace("'", "\\'").replace('"', '\\"')

This is interpolated directly into shell commands:

• macOS: osascript -e 'display notification "{_esc(msg)}"' — AppleScript has different escaping rules; backslash-single-quote doesn't work in AppleScript
• Windows: PowerShell -Command "...InnerText = '{_esc(title)}'" — single-quoted PowerShell strings don't process backslash escapes at all, so a title containing ' breaks out of the string and allows arbitrary PowerShell execution

User-controlled input should never be interpolated into shell command strings. Use subprocess.run() with list args where possible, or proper platform-specific escaping.

6. Platform Compatibility Issues

• Linux: Sound files hardcoded to /usr/share/sounds/freedesktop/ and /usr/share/sounds/ubuntu/ paths — won't exist on Fedora, Arch, Alpine, etc.
• macOS: Escaping broken (see above), but otherwise sound paths are correct (/System/Library/Sounds/)
• Windows: Toast notification via Windows Runtime APIs is fragile, and the escaping issue makes it unsafe with special characters

Recommendation

The concept is solid and we'd love to have notification support. I'm opening a separate issue to track this as a feature request. A clean rebuild should:

1. Use the correct tools/registry.py registration pattern (see any existing tool for reference)
2. Use safe argument passing (no string interpolation into shell commands)
3. Consider remote/SSH scenarios (terminal bell fallback, gateway integration)
4. Be on a fresh branch off current main (only the notification files)
5. Include tests

Thanks for the contribution and the idea! 🙏

#318