fix(skill): align tool names + add safety gates to chat /skill new

C1: /skill new in chat had only name validation — no wiki-exists check,
no wiki-content check, no overwrite guard. The CLI had all three. Extract
the gates into _preflight_skill_new and call from both. Add explicit
'remove existing skill first' message in chat (no -y equivalent there).

C2: System prompt advertised tool names (list_wiki_dir, read_wiki_file,
write_skill_file) that didn't match what was registered with
@function_tool (list_wiki, read_wiki, write_skill). LLM saw the registered
names; prompt references would confuse it. Rename the wrappers to match.

I1: query_wiki was a sync @function_tool calling asyncio.run() on
run_query — works only because openai-agents SDK runs sync tools on
worker threads. Convert to async @function_tool so the runner awaits it
in the same loop, eliminating the nested-asyncio fragility.

Add 2 regression tests for the chat safety gates.

Author: KylinMountain

openkb/agent/chat.py

@@ -521,12 +521,22 @@ async def _handle_slash_skill(arg: str, kb_dir: Path, style: Style) -> None:
     name = parts[1]
     intent = " ".join(parts[2:])
 
-    from openkb.cli import _validate_skill_name
-    err = _validate_skill_name(name)
+    # Use the same safety gates as the CLI (name validation, wiki dir,
+    # wiki content). Chat doesn't have a -y flag, so existing skills
+    # block with a clear instruction to delete first.
+    from openkb.cli import _preflight_skill_new
+    err = _preflight_skill_new(kb_dir, name, yes_flag=False)
     if err:
         _fmt(style, ("class:error", f"[ERROR] {err}\n"))
         return
 
+    target = kb_dir / "output" / "skills" / name
+    if target.exists():
+        _fmt(style, ("class:error",
+            f"[ERROR] output/skills/{name}/ already exists. Remove it first "
+            f"with `rm -rf output/skills/{name}` and re-run.\n"))
+        return
+
     # Load model from KB config
     from openkb.config import load_config, DEFAULT_CONFIG
     config = load_config(kb_dir / ".openkb" / "config.yaml")

openkb/agent/skill_compiler.py

@@ -13,16 +13,15 @@
 """
 from __future__ import annotations
 
-import asyncio
 from pathlib import Path
 
 from agents import Agent, Runner, function_tool
 from agents.model_settings import ModelSettings
 
 from openkb.agent.skill_tools import (
-    list_wiki_dir,
-    read_wiki_file_for_skill,
-    write_skill_file,
+    list_wiki_dir as _list_wiki_dir_impl,
+    read_wiki_file_for_skill as _read_wiki_file_impl,
+    write_skill_file as _write_skill_file_impl,
 )
 from openkb.prompts import load_prompt
 from openkb.schema import get_agents_md
@@ -59,32 +58,31 @@ def build_skill_compile_agent(
     )
 
     @function_tool
-    def list_wiki(directory: str) -> str:
+    def list_wiki_dir(directory: str) -> str:
         """List .md files in a wiki subdirectory (e.g. 'concepts')."""
-        return list_wiki_dir(directory, wiki_root)
+        return _list_wiki_dir_impl(directory, wiki_root)
 
     @function_tool
-    def read_wiki(path: str) -> str:
+    def read_wiki_file(path: str) -> str:
         """Read a wiki markdown file by path relative to wiki/ (e.g. 'concepts/attention.md')."""
-        return read_wiki_file_for_skill(path, wiki_root)
+        return _read_wiki_file_impl(path, wiki_root)
 
     @function_tool
-    def query_wiki(question: str) -> str:
+    async def query_wiki(question: str) -> str:
         """Run a semantic query over the wiki and return the answer.
 
         Use sparingly — this is itself an LLM call. Prefer reading specific
         files when you already know which one you want.
         """
-        # Lazy import to avoid circular dependency at module import.
+        # Lazy import to avoid a circular dependency at module load time.
         from openkb.agent.query import run_query
         kb_dir = Path(wiki_root).parent
-        config_model = model
-        return asyncio.run(run_query(question, kb_dir, config_model, stream=False))
+        return await run_query(question, kb_dir, model, stream=False)
 
     @function_tool
-    def write_skill(path: str, content: str) -> str:
+    def write_skill_file(path: str, content: str) -> str:
         """Write a file under the skill directory."""
-        return write_skill_file(path, content, skill_root)
+        return _write_skill_file_impl(path, content, skill_root)
 
     @function_tool
     def done(summary: str) -> str:
@@ -94,7 +92,7 @@ def done(summary: str) -> str:
     return Agent(
         name="skill-compiler",
         instructions=instructions,
-        tools=[list_wiki, read_wiki, query_wiki, write_skill, done],
+        tools=[list_wiki_dir, read_wiki_file, query_wiki, write_skill_file, done],
         model=f"litellm/{model}",
         model_settings=ModelSettings(parallel_tool_calls=False),
     )

openkb/cli.py

@@ -165,6 +165,54 @@ def _validate_skill_name(name: str) -> str | None:
     return None
 
 
+def _preflight_skill_new(
+    kb_dir: Path, name: str, yes_flag: bool,
+) -> str | None:
+    """Run all the safety gates for ``openkb skill new`` / ``/skill new``.
+
+    Returns ``None`` if it's safe to proceed (caller will then either
+    proceed or, for the overwrite case, call ``_clear_existing_skill_dir``
+    after confirming with the user). Returns an error message string if
+    one of the gates trips.
+
+    NOTE: This intentionally does NOT handle the overwrite confirmation
+    itself — that's caller-specific (TTY-based ``click.confirm`` in CLI;
+    explicit ``--force``-style flag in chat). It only returns an error if
+    the target dir exists AND ``yes_flag`` is False, and the caller is
+    expected to detect that error message and prompt as appropriate.
+    """
+    err = _validate_skill_name(name)
+    if err:
+        return err
+
+    wiki = kb_dir / "wiki"
+    if not wiki.is_dir():
+        return (
+            "No wiki found in this KB. Run `openkb add <source>` to "
+            "ingest documents first."
+        )
+
+    has_content = any(
+        (wiki / sub).is_dir() and any((wiki / sub).iterdir())
+        for sub in ("concepts", "summaries")
+    )
+    if not has_content:
+        return (
+            "Wiki has no compiled content yet. Ingest at least one "
+            "document with `openkb add` first."
+        )
+
+    return None
+
+
+def _clear_existing_skill_dir(kb_dir: Path, name: str) -> None:
+    """Delete an existing ``<kb>/output/skills/<name>/`` directory."""
+    import shutil
+    target = kb_dir / "output" / "skills" / name
+    if target.exists():
+        shutil.rmtree(target)
+
+
 def add_single_file(file_path: Path, kb_dir: Path) -> Literal["added", "skipped", "failed"]:
     """Convert, index, and compile a single document into the knowledge base.
 
@@ -1393,54 +1441,31 @@ def skill_new(ctx, name, intent, yes_flag):
       openkb skill new karpathy-thinking "Reason about transformers like Karpathy"
     """
     import asyncio
-    import shutil
     import sys
 
     kb_dir = _find_kb_dir(ctx.obj.get("kb_dir_override"))
     if kb_dir is None:
         click.echo("No knowledge base found. Run `openkb init` first.", err=True)
         ctx.exit(1)
 
-    # Validate name
-    err = _validate_skill_name(name)
+    err = _preflight_skill_new(kb_dir, name, yes_flag)
     if err:
         click.echo(f"[ERROR] {err}", err=True)
-        ctx.exit(2)
-
-    # Validate wiki exists and has content
-    wiki = kb_dir / "wiki"
-    if not wiki.is_dir():
-        click.echo(
-            "[ERROR] No wiki found in this KB. Run `openkb add <source>` "
-            "to ingest documents first.",
-            err=True,
-        )
-        ctx.exit(1)
-    has_content = any(
-        (wiki / sub).is_dir() and any((wiki / sub).iterdir())
-        for sub in ("concepts", "summaries")
-    )
-    if not has_content:
-        click.echo(
-            "[ERROR] Wiki has no compiled content yet. Ingest at least one "
-            "document with `openkb add` first.",
-            err=True,
-        )
         ctx.exit(1)
 
-    # Overwrite handling
+    # Overwrite handling (CLI-specific)
     target = kb_dir / "output" / "skills" / name
     if target.exists():
         if yes_flag:
-            shutil.rmtree(target)
+            _clear_existing_skill_dir(kb_dir, name)
         elif sys.stdin.isatty():
             if not click.confirm(
                 f"output/skills/{name}/ already exists. Overwrite?",
                 default=False,
             ):
                 click.echo("Aborted.")
                 ctx.exit(1)
-            shutil.rmtree(target)
+            _clear_existing_skill_dir(kb_dir, name)
         else:
             click.echo(
                 f"[ERROR] output/skills/{name}/ exists. Pass -y to overwrite "

tests/test_skill_chat_slash.py

@@ -17,6 +17,9 @@ def _make_kb(tmp_path):
     (tmp_path / "wiki" / "concepts").mkdir(parents=True)
     (tmp_path / "wiki" / "summaries").mkdir(parents=True)
     (tmp_path / "wiki" / "index.md").write_text("# index\n")
+    # Populate so wiki-content gate accepts
+    (tmp_path / "wiki" / "concepts" / "demo.md").write_text("# demo\n")
+    (tmp_path / "wiki" / "summaries" / "demo.md").write_text("# demo\n")
     return tmp_path
 
 
@@ -62,3 +65,41 @@ async def test_slash_skill_unknown_subcommand(tmp_path):
     style = Style.from_dict({})
     action = await _handle_slash('/skill list', kb, session, style)
     assert action is None
+
+
+@pytest.mark.asyncio
+async def test_slash_skill_new_rejects_empty_wiki(tmp_path):
+    """Chat / slash command must catch freshly-init'd KBs (no compiled content)."""
+    kb = tmp_path
+    (kb / ".openkb").mkdir()
+    (kb / ".openkb" / "config.yaml").write_text("model: gpt-4o-mini\n")
+    (kb / ".openkb" / "chats").mkdir()
+    # Empty wiki/ — exactly what `openkb init` creates
+    (kb / "wiki" / "concepts").mkdir(parents=True)
+    (kb / "wiki" / "summaries").mkdir(parents=True)
+    (kb / "wiki" / "index.md").write_text("# index\n")
+
+    session = ChatSession.new(kb, "gpt-4o-mini", "en")
+    style = Style.from_dict({})
+
+    action = await _handle_slash('/skill new demo "intent"', kb, session, style)
+    assert action is None
+    assert not (kb / "output").exists()
+
+
+@pytest.mark.asyncio
+async def test_slash_skill_new_rejects_when_target_exists(tmp_path):
+    """Chat / slash command must not silently overwrite an existing skill."""
+    kb = _make_kb(tmp_path)
+    (kb / "wiki" / "concepts" / "x.md").write_text("x")
+    (kb / "wiki" / "summaries" / "x.md").write_text("x")
+    (kb / "output" / "skills" / "demo").mkdir(parents=True)
+    (kb / "output" / "skills" / "demo" / "stale.txt").write_text("old")
+
+    session = ChatSession.new(kb, "gpt-4o-mini", "en")
+    style = Style.from_dict({})
+
+    action = await _handle_slash('/skill new demo "intent"', kb, session, style)
+    assert action is None
+    # stale.txt must still be there (we didn't overwrite)
+    assert (kb / "output" / "skills" / "demo" / "stale.txt").read_text() == "old"