refactor(tools): split tool_implementations.py into src/tools/ package (#4423)

* test(tools): add shim protection test for tool_implementations split

Covers all 48 top-level functions (33 do_* + 15 _helpers) extracted from
the original module. Guards the upcoming split: the shim must re-export
every symbol so existing 'from src.tool_implementations import X' imports
keep working. Passes on baseline (pre-split).

* refactor(tools): add src/tools/ package with shared _common

Slice 1 Task 2 (#4082/#4071). Adds the package skeleton and moves the
shared _parse_tool_args helper into src/tools/_common.py. Domain modules
will import from here. tool_implementations.py is untouched at this step.

* refactor(tools): extract system domain into src/tools/system.py

Slice 1 (#4082/#4071), Task 3: move the system-domain tool functions
(do_manage_skills/_skill_dump/do_manage_tasks/do_manage_endpoints/
do_manage_mcp/do_manage_webhooks/do_manage_tokens/do_manage_settings/
do_api_call/do_app_api) and the app_api blocklist constants out of
tool_implementations.py into a new src/tools/system.py module.

tool_implementations.py re-imports all of them so it stays a working
backward-compatible facade (shim test stays green).

- do_manage_mcp resolves get_mcp_manager via a function-local import
  from tool_implementations so the test that patches
  src.tool_implementations.get_mcp_manager still applies post-move.
- do_app_api imports _internal_headers and _INTERNAL_BASE (still in
  tool_implementations) function-locally to avoid a circular import.
- Repoint test_context_budget introspection assertion to the moved
  code's new home in src/tools/system.py.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* refactor(tools): extract cookbook domain into src/tools/cookbook.py

Moves the model-serving (cookbook) tool domain out of tool_implementations.py
into src/tools/cookbook.py as part of slice 1 (#4082/#4071):

- 13 do_* tools: download/serve/list/stop/tail/search/adopt/cached models,
  list downloads/cancel, list cookbook servers, serve presets
- 9 private helpers: _cookbook_servers, _resolve_cookbook_host,
  _cookbook_env_for_host, _infer_serve_{port,host}, _ensure_served_endpoint,
  _cookbook_register_task, _cookbook_apply_retry_suggestion,
  _scan_running_model_processes, _cookbook_kill_session
- _MODEL_PROCESS_PATTERNS constant (used only by _scan_running_model_processes)

tool_implementations.py stays a backward-compatible facade via a re-import
from src.tools.cookbook; src/tools/__init__ re-exports the same symbols.

_internal_headers and _INTERNAL_BASE stay in tool_implementations.py (shared
by system.py's do_app_api and many cookbook funcs). Each cookbook function
that needs them does a function-local import to avoid a top-level circular
dependency, matching the system-domain split.

Verified: compileall clean; shim test green; cookbook-touching suite
(652 passed, 1 skipped); full suite 3587 passed, 2 failed
(pre-existing test_api_chat_security, unrelated).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* refactor(tools): extract search domain into src/tools/search.py

* refactor(tools): extract notes domain into src/tools/notes.py

* refactor(tools): extract calendar domain into src/tools/calendar.py

Repoints tests/test_caldav_bidirectional_sync.py source-introspection
to src/tools/calendar.py (do_manage_calendar moved there).

* refactor(tools): extract image domain into src/tools/image.py

* refactor(tools): extract research domain into src/tools/research.py

* refactor(tools): extract contacts domain into src/tools/contacts.py

* refactor(tools): extract vault domain into src/tools/vault.py

Repoints tests/test_vault_password_not_in_argv.py source-introspection
to src/tools/vault.py (the vault do_* helpers moved there).

* refactor(tools): collapse tool_implementations to clean re-export shim

Move shared _INTERNAL_BASE/_internal_headers to src/tools/_common.py and
drop the duplicate _parse_tool_args (already in _common). tool_implementations.py
is now a pure re-export facade (+ 3 pre-existing email-context helpers, out of
scope). Domain files' function-local imports of these names still resolve via
the facade re-export.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix(tools): port upstream cookbook workflow changes to split module

Rebase onto dev dropped c504214 ("Cookbook model workflow fixes") edits
to do_serve_model / do_tail_serve_output: the extraction commit moved
the pre-edit bodies into src/tools/cookbook.py and git auto-accepted the
deletion from tool_implementations.py, losing dev's changes. Restore them
in their post-split home:

- do_serve_model: add where/log_path/next_tools and the expanded
  "Next required check" output message
- do_tail_serve_output: empty-output fallback message replacing
  "(empty pane)"

(do_manage_settings web_fetch alias edit was already applied to
src/tools/system.py during the system-extract conflict resolution.)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix(tools): break admin_tools circular import in split facade

After rebasing onto dev (#3629 moved the admin manage_* tools into
src/agent_tools/admin_tools), the facade re-exported them via a top-level
`from src.agent_tools.admin_tools import ...`. But src.agent_tools.__init__
imports this facade at top level, so the eager import re-entered the
partially-initialized agent_tools package and broke collection.

Re-export the admin symbols (do_manage_endpoints/mcp/webhooks/tokens/
settings, _MCP_DENIED_COMMANDS, _validate_mcp_command) lazily through
module __getattr__ instead, and drop them from src/tools/__init__ (they
no longer live in the src.tools package). system.py now holds only the
skills/tasks/api bridges; admin tools live solely in admin_tools.py,
matching upstream.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix(tools): re-export dropped helpers through the split shim

Address review finding from #4423: the compatibility facade claimed to
preserve every original top-level symbol but omitted three helpers the
old src.tool_implementations exposed. Re-export them and pin them in
the shim protection test:

- _string_arg, _validate_cookbook_ssh_target <- src/tools/cookbook.py
- _mcp_allowed_commands <- src/agent_tools/admin_tools.py (lazily via
  __getattr__, to keep the agent_tools.__init__ <-> facade import acyclic
  after the #3629 admin-tools migration)

All three added to tests/test_tool_implementations_shim.py _EXPECTED so
the test contract now matches its "every original top-level function"
comment.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* test(tools): self-verify shim re-exports every domain do_*

The hand-maintained _EXPECTED list in the shim protection test can drift
silently when a new tool is added to a domain module but not re-exported
by the facade — exactly the omission a reviewer flagged post-split.
Add an auto-discovering test that enumerates every do_* from the domain
modules (incl. admin_tools) and asserts reachability through the shim,
so a forgotten re-export fails the build automatically.

Uses hasattr (not dir(ti)) because the admin symbols are re-exported
lazily via module __getattr__ and don't appear in dir(ti).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* test(tools): self-verify every in-repo facade import resolves

RaresKeY's P3 on the shim test was a claim-vs-reality gap: the docstring
said it protected "every from src.tool_implementations import X" but the
hand-maintained _EXPECTED list omitted three underscore helpers, so the
claim wasn't enforced. Re-exporting the three (cf1f5e3) fixed the known
gap; this closes the structural one.

Add test_every_facade_import_in_repo_resolves: ast-enumerate every
`from src.tool_implementations import X` site in src/ and tests/ and
assert hasattr(ti, X) for each. A forgotten re-export that anything in
the repo imports now fails the build automatically — including underscore
helpers, which the do_* discovery test does not cover.

Together with test_shim_reexports_every_domain_do_function, the shim
contract is now self-verifying. Demote _EXPECTED in the docstring to the
curated historical/downstream surface (the three helpers have no in-repo
consumer, so they stay manual by necessity) instead of "ground truth".

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix(tools): dedupe _parse_tool_args + align shim guard with route consumers

Addresses two P3s from review (RaresKeY, 2026-06-26):

1. maintainability — _common carried a full copy of _parse_tool_args
   alongside the canonical src.tool_utils one; future parser fixes could
   diverge. The two bodies were byte-identical in logic, so _common now
   re-exports from tool_utils (a leaf module, no circular-import risk).
   The single-source test is extended to assert _common._parse_tool_args
   and tool_implementations._parse_tool_args are the same object as
   tool_utils._parse_tool_args.

2. test — the shim guard's import-site scan only walked src/ and tests/,
   missing routes/chat_routes.py's clear_active_email/set_active_email
   imports, and _EXPECTED omitted the active-email facade helpers. The
   scan now walks every first-party Python dir (pruning venvs/caches/data
   in-place), and set/get/clear_active_email are added to _EXPECTED
   (get_active_email has no in-repo importer, so the scan alone can't see
   it).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

---------

Co-authored-by: yuandonghao <yuandonghao@cohl.com>
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>

src/tools/contacts.py

@@ -0,0 +1,148 @@
+"""Contacts-domain tool implementations.
+
+Extracted from tool_implementations.py as part of slice 1 (#4082/#4071).
+Holds the resolve_contact and manage_contact (CardDAV CRUD) tools.
+``src.tool_implementations`` re-exports these for backward compatibility.
+``_INTERNAL_BASE`` still lives in tool_implementations.py and is pulled
+back function-locally where needed.
+"""
+from typing import Dict, Optional
+
+from src.tools._common import _parse_tool_args
+
+
+async def do_resolve_contact(content: str, owner: Optional[str] = None) -> Dict:
+    """Look up a contact by name. Searches: CardDAV -> email history -> memory."""
+    import httpx
+    from src.tool_implementations import _INTERNAL_BASE  # shared constant, still lives in the facade
+    try:
+        args = _parse_tool_args(content)
+    except ValueError:
+        return {"error": "Invalid JSON arguments", "exit_code": 1}
+    name = args.get("name", "")
+    if not name:
+        return {"error": "name is required", "exit_code": 1}
+
+    contacts = {}  # email_or_phone -> {name, source, phone?}
+
+    # 1. CardDAV (Radicale) — structured contacts. Call in-process: a
+    # server-side httpx GET to /api/contacts/search carries no session
+    # cookie and would 401 under require_user.
+    try:
+        import asyncio
+        from routes import contacts_routes as cc
+        all_contacts = await asyncio.to_thread(cc._fetch_contacts)
+        q = name.lower()
+        for c in (all_contacts or []):
+            hay_name = (c.get("name") or "").lower()
+            match = q in hay_name or any(q in (e or "").lower() for e in c.get("emails", []))
+            if not match:
+                continue
+            has_email = False
+            for email in (c.get("emails") or []):
+                email = (email or "").strip().lower()
+                if email and "@" in email:
+                    contacts[email] = {"name": c.get("name") or email, "source": "contacts"}
+                    has_email = True
+            # Fall back to phone numbers when the contact has no email address
+            if not has_email:
+                for phone in (c.get("phones") or []):
+                    phone = (phone or "").strip()
+                    if phone:
+                        contacts[phone] = {"name": c.get("name") or phone, "source": "contacts", "phone": phone}
+    except Exception:
+        pass
+
+    async with httpx.AsyncClient(timeout=30) as client:
+        # 2. Email history (sent/received)
+        try:
+            resp = await client.get(f"{_INTERNAL_BASE}/api/email/resolve-contact", params={"name": name})
+            if resp.status_code == 200:
+                for c in (resp.json().get("contacts") or []):
+                    email = (c.get("email") or "").strip().lower()
+                    if email and email not in contacts:
+                        contacts[email] = {"name": c.get("name") or email, "source": "email history"}
+        except Exception:
+            pass
+
+    if not contacts:
+        return {"output": f"No contacts found matching '{name}'.", "exit_code": 0}
+
+    lines = [f"Contacts matching '{name}':"]
+    for key, info in contacts.items():
+        if info.get("phone"):
+            lines.append(f"- {info['name']} — phone: {info['phone']} ({info['source']})")
+        else:
+            lines.append(f"- {info['name']} <{key}> ({info['source']})")
+    return {"output": "\n".join(lines), "exit_code": 0}
+
+
+async def do_manage_contact(content: str, owner: Optional[str] = None) -> Dict:
+    """Add / update / delete / list CardDAV contacts. Calls the contacts
+    helpers IN-PROCESS rather than over HTTP — a server-side httpx call to
+    /api/contacts/* carries no session cookie and would be rejected by
+    require_user (401), so the tool would see zero contacts even though
+    the browser-side UI works fine."""
+    try:
+        args = _parse_tool_args(content)
+    except ValueError:
+        return {"error": "Invalid JSON arguments", "exit_code": 1}
+    action = (args.get("action") or "").strip().lower()
+    try:
+        from routes import contacts_routes as cc
+    except Exception as e:
+        return {"error": f"Contacts module unavailable: {e}", "exit_code": 1}
+    # The contacts helpers are sync (httpx blocking calls to CardDAV) — run
+    # them in a thread so we don't block the event loop.
+    import asyncio
+    try:
+        if action == "list":
+            rows = await asyncio.to_thread(cc._fetch_contacts, True)
+            if not rows:
+                return {"output": "No contacts.", "exit_code": 0}
+            lines = [f"{len(rows)} contacts:"]
+            for c in rows:
+                em = ", ".join(c.get("emails") or [])
+                lines.append(f"- {c.get('name') or '(no name)'} <{em}>  [uid={c.get('uid','')}]")
+            return {"output": "\n".join(lines), "exit_code": 0}
+
+        if action == "add":
+            email = (args.get("email") or "").strip()
+            if not email:
+                return {"error": "email is required for add", "exit_code": 1}
+            name = (args.get("name") or "").strip() or email.split("@")[0]
+            # Dedupe by email (same as the /add route).
+            existing = await asyncio.to_thread(cc._fetch_contacts)
+            for c in existing:
+                if email.lower() in [e.lower() for e in c.get("emails", [])]:
+                    return {"output": f"{email} is already a contact ({c.get('name','')}).", "exit_code": 0}
+            ok = await asyncio.to_thread(cc._create_contact, name, email)
+            return {"output": f"{'Added' if ok else 'Failed to add'} {name} <{email}>.", "exit_code": 0 if ok else 1}
+
+        if action in ("update", "edit"):
+            uid = (args.get("uid") or "").strip()
+            if not uid:
+                return {"error": "uid is required for update (use action=list to find it)", "exit_code": 1}
+            name = (args.get("name") or "").strip()
+            emails = args.get("emails")
+            if emails is None and args.get("email"):
+                emails = [args["email"]]
+            emails = [e.strip() for e in (emails or []) if e and e.strip()]
+            phones = [p.strip() for p in (args.get("phones") or []) if p and p.strip()]
+            if not name and not emails:
+                return {"error": "Provide a name or emails to update", "exit_code": 1}
+            if not name and emails:
+                name = emails[0].split("@")[0]
+            ok = await asyncio.to_thread(cc._update_contact, uid, name, emails, phones)
+            return {"output": "Contact updated." if ok else "Update failed.", "exit_code": 0 if ok else 1}
+
+        if action == "delete":
+            uid = (args.get("uid") or "").strip()
+            if not uid:
+                return {"error": "uid is required for delete (use action=list to find it)", "exit_code": 1}
+            ok = await asyncio.to_thread(cc._delete_contact, uid)
+            return {"output": "Contact deleted." if ok else "Delete failed.", "exit_code": 0 if ok else 1}
+
+        return {"error": f"Unknown action '{action}'. Use list, add, update, or delete.", "exit_code": 1}
+    except Exception as e:
+        return {"error": f"Contact operation failed: {e}", "exit_code": 1}