diff --git a/openkb/agent/chat.py b/openkb/agent/chat.py index 8e0d603f..2d1a584c 100644 --- a/openkb/agent/chat.py +++ b/openkb/agent/chat.py @@ -60,6 +60,8 @@ " /lint Lint the knowledge base\n" " /add Add a document or directory to the knowledge base\n" ' /skill new "" Compile a skill from the wiki\n' + ' /deck new [--critique] [--skill ] "" Generate an HTML deck from the wiki\n' + " /critique Run html-critic skill on a file (CSS bugs, layout, self-containment)\n" " /help Show this" ) @@ -216,6 +218,8 @@ def _bottom_toolbar(session: ChatSession) -> FormattedText: ("/lint", "Lint the knowledge base"), ("/add", "Add a document or directory"), ("/skill", "Compile a skill (try `/skill new \"intent\"`)"), + ("/deck", "Generate a deck (try `/deck new \"intent\"`)"), + ("/critique", "Run html-critic skill on a file (e.g. `/critique output/decks/foo/index.html`)"), ] @@ -558,7 +562,9 @@ async def _handle_slash_skill(arg: str, kb_dir: Path, style: Style) -> None: _fmt(style, ("class:error", f"[ERROR] {exc}\n")) return - # Surface validation issues from Generator.run (same gate as CLI). + # Surface validation issues from Generator.run. Unlike the CLI + # (which exits 1 on validation errors), chat is interactive — print + # issues inline and continue so the user can inspect and iterate. result = gen.validation if result is not None and (result.errors or result.warnings): _fmt(style, ("class:error", "[WARN] Validation found issues:\n")) @@ -576,6 +582,120 @@ async def _handle_slash_skill(arg: str, kb_dir: Path, style: Style) -> None: f"edit files under output/skills/{name}/ directly.\n")) +async def _handle_slash_deck(arg: str, kb_dir: Path, style: Style) -> None: + """Dispatch ``/deck new [--critique] ""``. + + Mirrors :func:`_handle_slash_skill`: validates the name, runs the + same wiki preflight gate, refuses to overwrite an existing deck + (chat has no ``-y`` flag), then invokes ``Generator(target_type="deck")``. + """ + import shlex + + try: + parts = shlex.split(arg) if arg else [] + except ValueError as exc: + _fmt(style, ("class:error", f"[ERROR] Could not parse: {exc}\n")) + return + if not parts: + _fmt(style, ("class:error", + "Usage: /deck new [--critique] \"\"\n")) + return + + sub = parts[0].lower() + if sub != "new": + _fmt(style, ("class:error", f"Unknown deck subcommand: {sub}. Try /deck new.\n")) + return + + # Parse optional --critique flag and --skill option. Both can + # appear anywhere among the remaining tokens. + rest = parts[1:] + critique = False + skill_name: str | None = None + filtered: list[str] = [] + i = 0 + while i < len(rest): + tok = rest[i] + if tok == "--critique": + critique = True + elif tok == "--skill" and i + 1 < len(rest): + skill_name = rest[i + 1] + i += 1 + elif tok.startswith("--skill="): + skill_name = tok.split("=", 1)[1] + else: + filtered.append(tok) + i += 1 + + if len(filtered) < 2: + _fmt(style, ("class:error", + "Usage: /deck new [--critique] [--skill ] \"\"\n")) + return + + name = filtered[0] + intent = " ".join(filtered[1:]) + + # Reuse the shared safety gates from the CLI (name validation, + # wiki dir, wiki content). Chat has no -y flag, so existing decks + # block with a clear instruction to delete first. + from openkb.cli import _preflight_skill_new + err = _preflight_skill_new(kb_dir, name) + if err: + # Reword "Skill name" → "Deck name" so error matches the command. + err = err.replace("Skill name", "Deck name") + _fmt(style, ("class:error", f"[ERROR] {err}\n")) + return + + from openkb.deck import deck_dir + target = deck_dir(kb_dir, name) + if target.exists(): + _fmt(style, ("class:error", + f"[ERROR] output/decks/{name}/ already exists. Remove it first " + f"with `rm -rf output/decks/{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") + model = config.get("model", DEFAULT_CONFIG["model"]) + + from openkb.skill.generator import Generator + skill_label = skill_name if skill_name else "openkb-deck-editorial (default)" + _fmt( + style, + ("class:slash.help", f"Generating deck '{name}' via skill {skill_label}...\n"), + ) + gen = Generator( + target_type="deck", + name=name, + intent=intent, + kb_dir=kb_dir, + model=model, + critique=critique, + skill_name=skill_name, + ) + try: + await gen.run() + except RuntimeError as exc: + _fmt(style, ("class:error", f"[ERROR] {exc}\n")) + return + + # Surface validation issues from Generator.run. Unlike the CLI + # (which exits 1 on validation errors), chat is interactive — print + # issues inline and continue so the user can inspect and iterate. + result = gen.validation + if result is not None and (result.errors or result.warnings): + _fmt(style, ("class:error", "[WARN] Validation found issues:\n")) + for err in result.errors: + _fmt(style, ("class:error", f" ERROR: {err}\n")) + for warn in result.warnings: + _fmt(style, ("class:error", f" WARN: {warn}\n")) + + _fmt(style, ("class:slash.ok", f"Saved: output/decks/{name}/index.html\n")) + _fmt(style, ("class:slash.help", + f"Iterate: ask follow-up questions in this chat and the agent can " + f"edit files under output/decks/{name}/ directly.\n")) + + async def _handle_slash( cmd: str, kb_dir: Path, @@ -643,6 +763,14 @@ async def _handle_slash( await _handle_slash_skill(arg, kb_dir, style) return None + if head == "/deck": + await _handle_slash_deck(arg, kb_dir, style) + return None + + if head == "/critique": + await _handle_slash_critique(arg, kb_dir, style) + return None + _fmt( style, ("class:error", f"Unknown command: {head}. Try /help.\n"), @@ -650,6 +778,68 @@ async def _handle_slash( return None +async def _handle_slash_critique(arg: str, kb_dir: Path, style: Style) -> None: + """``/critique `` — run the openkb-html-critic skill on a file. + + The skill reads the HTML, fixes CSS specificity bugs / missing nav / + self-containment violations, and writes the corrected file back. It + will not touch slide content (numbers, names, quotes). + """ + path = arg.strip() + if not path: + _fmt( + style, + ("class:slash.help", "Usage: /critique \n"), + ) + return + + target = (kb_dir / path).resolve() if not Path(path).is_absolute() else Path(path) + if not target.is_file(): + _fmt(style, ("class:error", f"[ERROR] File not found: {path}\n")) + return + + from openkb.agent.skill_runner import ( + MAX_TURNS_WITH_CRITIQUE, + SkillNotFoundError, + run_skill, + ) + from openkb.config import DEFAULT_CONFIG, load_config + + config = load_config(kb_dir / ".openkb" / "config.yaml") + model = config.get("model", DEFAULT_CONFIG["model"]) + + # Path passed to the skill is relative to kb_dir (the agent's cwd + # conceptually). The skill's read_file/write_file tools operate + # under wiki/ and output/ scopes — give it the relative form so + # write_kb_file resolves correctly. + try: + rel = target.relative_to(kb_dir) + rel_str = str(rel) + except ValueError: + # Outside KB — pass absolute, write tool will reject, but read + # may still work for a critique-only diagnostic. + rel_str = str(target) + + _fmt(style, ("class:slash.ok", f"Critiquing {rel_str}...\n")) + + try: + await run_skill( + skill_name="openkb-html-critic", + intent=f"Critique and patch the HTML file at: {rel_str}", + kb_dir=kb_dir, + model=model, + max_turns=40, + ) + except SkillNotFoundError as exc: + _fmt(style, ("class:error", f"[ERROR] {exc}\n")) + return + except RuntimeError as exc: + _fmt(style, ("class:error", f"[ERROR] {exc}\n")) + return + + _fmt(style, ("class:slash.ok", f"Critique pass complete: {rel_str}\n")) + + async def run_chat( kb_dir: Path, session: ChatSession, diff --git a/openkb/agent/query.py b/openkb/agent/query.py index 790a186c..dd737858 100644 --- a/openkb/agent/query.py +++ b/openkb/agent/query.py @@ -102,12 +102,19 @@ def build_chat_agent( language: str = "en", ) -> Agent: """Build the chat agent: query agent + a write tool restricted to - ``/wiki/explorations/**`` and ``/output/**``. + ``/wiki/explorations/**`` and ``/output/**`` + a ``ShellTool`` + advertising locally-installed Anthropic-style skills. This is the variant used by the interactive ``openkb chat`` REPL so users can iterate on generated artifacts (e.g. ``output/skills//``) via natural-language follow-ups without giving the agent unrestricted write access to the wiki. + + Skill discovery: ``openkb/agent/skills.scan_local_skills`` looks in + ``/skills/``, ``~/.openkb/skills/``, ``~/.claude/skills/`` for + ``SKILL.md`` files. Any found skill is exposed to the agent via + ``ShellTool.environment.skills`` so the model can ``cat`` the skill body + and follow its instructions when the user's request matches. """ wiki_root = str(kb_dir / "wiki") kb_root = str(kb_dir) @@ -130,7 +137,107 @@ def write_file(path: str, content: str) -> str: """ return write_kb_file(path, content, kb_root) - return base.clone(tools=[*base.tools, write_file]) + extra_tools: list = [write_file] + skill_instructions_addendum = "" + + # Skill discovery via function tools. The agents SDK has a richer + # ``ShellTool``+``ShellToolLocalSkill`` mechanism for this, but those + # are OpenAI Responses-API hosted tools; LiteLLM routes through + # ChatCompletions which rejects hosted tools. So we use plain + # ``function_tool`` primitives that work with any LiteLLM-routed model. + from openkb.agent.skills import scan_local_skills + + skills = scan_local_skills(kb_dir) + skill_index = {s["name"]: s for s in skills} + + if skill_index: + skill_list_text = _format_skill_list(skills) + + @function_tool + def list_skills() -> str: + """List skills available in this environment. + + Returns a text catalog of installed Anthropic-style skills. + Each entry has a name and a one-line description; use the + description to decide whether the skill matches the user's + request, then call ``read_skill(name)`` to load its body. + """ + return skill_list_text + + @function_tool + def read_skill(name: str) -> str: + """Read a skill's ``SKILL.md`` body. + + Call this once you've decided a skill matches the user's + request. The returned text is the full skill instructions + (frontmatter stripped). Follow it as your working method + and write outputs via the ``write_file`` tool. + + Args: + name: skill name as listed by ``list_skills``. + """ + entry = skill_index.get(name) + if entry is None: + return ( + f"Unknown skill: {name!r}. Call list_skills() to see " + f"available skills." + ) + md_path = Path(entry["path"]) / "SKILL.md" + try: + text = md_path.read_text(encoding="utf-8") + except OSError as exc: + return f"Could not read {md_path}: {exc}" + # Strip frontmatter, return body only. + from openkb.agent.skills import _parse_frontmatter + _, body = _parse_frontmatter(text) + return body + + extra_tools.extend([list_skills, read_skill]) + + # Build the prompt addendum listing skill names + descriptions + # right inside the system prompt so the model sees them up front + # and knows what to look for, even before deciding to call + # list_skills(). This is the difference between "agent + # eventually discovers skills" and "agent treats skill use as + # the default for matching requests". + skill_lines = [] + for s in skills: + desc_one_line = " ".join(s["description"].split()) + skill_lines.append(f"- **{s['name']}** — {desc_one_line}") + skill_instructions_addendum = ( + "\n\n## Available skills\n\n" + "The following Anthropic-style skill packages are installed in " + "this environment. **When a user request matches a skill's " + "description (e.g. 'make a deck', 'generate slides', 'draft a " + "report'), you MUST call `read_skill(name)` to load that " + "skill's full instructions and follow them strictly** — do not " + "freestyle the output format if a skill covers it.\n\n" + + "\n".join(skill_lines) + + "\n\nIf no listed skill matches the request, proceed with " + "your default tools." + ) + + new_instructions = (base.instructions or "") + skill_instructions_addendum + return base.clone( + tools=[*base.tools, *extra_tools], + instructions=new_instructions, + ) + + +def _format_skill_list(skills: list[dict[str, str]]) -> str: + """Render the skill catalog as a compact text block for the agent.""" + if not skills: + return "No skills installed." + lines = [f"{len(skills)} skill(s) available:\n"] + for s in skills: + lines.append(f"- {s['name']}") + # Indent description; keep it one paragraph so the agent reads it fast. + desc = " ".join(s["description"].split()) + lines.append(f" {desc}") + lines.append( + "\nTo use a skill, call read_skill(name) and follow its instructions." + ) + return "\n".join(lines) async def run_query( diff --git a/openkb/agent/skill_runner.py b/openkb/agent/skill_runner.py new file mode 100644 index 00000000..a5825314 --- /dev/null +++ b/openkb/agent/skill_runner.py @@ -0,0 +1,217 @@ +"""Generic skill runner — the shared core between CLI and chat surfaces. + +A skill (Anthropic-style ``SKILL.md`` with YAML frontmatter and a body of +agent instructions) is loaded by ``run_skill``, which builds an Agent +whose ``instructions`` are that body. The agent gets the standard wiki +read-tool set plus a constrained ``write_file`` tool scoped to +``wiki/explorations/**`` and ``output/**``, and ``read_output_or_skill_file`` +for inspecting prior artifacts. + +This decouples generators from hard-coded prompts. ``openkb deck new`` / +``openkb skill new`` / any future ``openkb new`` command becomes a +two-line wrapper around ``run_skill(skill_name=..., intent=...)``. + +Skill frontmatter that ``run_skill`` honours (all optional; under the +top-level ``od:`` key): + +* ``mode`` — the artifact type. ``"deck"`` triggers deck-specific + post-run handling (output-path templating + validation). Unknown modes + are accepted; they just don't trigger extra hooks. +* ``output_path_template`` — a path string with ``{slug}`` placeholder, + relative to KB root. When set, ``run_skill`` injects the resolved path + into the agent's intent and verifies the file exists post-run. +* ``deck_grammar`` — passed to :func:`openkb.deck.validator.validate_deck` + when ``mode == "deck"``. See that module for the contract. +""" +from __future__ import annotations + +from dataclasses import dataclass, field +from pathlib import Path +from typing import Any, Optional + +from agents import Runner, function_tool + +from openkb.agent.query import build_query_agent +from openkb.agent.skills import _parse_frontmatter, scan_local_skills +from openkb.agent.tools import read_kb_file, write_kb_file + + +MAX_TURNS = 80 +MAX_TURNS_WITH_CRITIQUE = 120 + + +class SkillNotFoundError(RuntimeError): + """Raised when the requested skill can't be located in any skill root.""" + + +@dataclass +class SkillRunResult: + """Outcome of a :func:`run_skill` call. + + ``validation`` is populated only when the skill declared a + deck-mode artifact and the post-run validator was therefore invoked; + otherwise ``None``. ``output_path`` is the KB-relative path the + runner enforced via ``output_path_template``, or ``None`` if the + skill left output placement to itself. + """ + + skill_name: str + output_path: Optional[Path] = None + validation: Optional[Any] = None # openkb.deck.validator.ValidationResult + metadata: dict = field(default_factory=dict) # skill's ``od:`` block + + +async def run_skill( + *, + skill_name: str, + intent: str, + kb_dir: Path, + model: str, + language: str = "en", + max_turns: int = MAX_TURNS, + seed: Optional[str] = None, + slug: Optional[str] = None, + extra_skill_roots: tuple[str | Path, ...] = (), +) -> SkillRunResult: + """Load ``skill_name`` and run it as an agent with ``intent``. + + When the skill's frontmatter declares ``od.mode == "deck"`` and + ``od.output_path_template`` is set, ``run_skill``: + + 1. Templates the path with the provided ``slug``. + 2. Adds a "Write to: " line to the agent's intent so the + skill knows the expected destination. + 3. After the agent run, checks the file exists and (if the skill + declared ``od.deck_grammar``) runs ``validate_deck`` against it. + + Args: + skill_name: Name (frontmatter ``name:``) of the skill to invoke. + intent: Natural-language brief for what to produce. + kb_dir: KB root. Used both for skill discovery and for the + agent's wiki read-tools / write-file scoping. + model: LiteLLM-formatted model string from KB config. + language: Passed through to the underlying query agent for + answer-language consistency. + max_turns: Hard cap on agent loop iterations. + seed: Optional kick-off user message. Defaults to a short nudge + that points the agent at its own instructions. + slug: Required when the skill declares + ``od.output_path_template`` (it substitutes ``{slug}``). + Otherwise ignored. + extra_skill_roots: Additional directories to scan beyond the + built-in ``/skills``, ``~/.openkb/skills``, + ``~/.claude/skills``. + + Returns: + A :class:`SkillRunResult` carrying the resolved output path (if + any) and the validation result (for deck-mode skills). + + Raises: + SkillNotFoundError: if no skill with ``skill_name`` is found. + RuntimeError: on turn-cap, model error, or missing + output file after a templated-path run. + """ + skills = scan_local_skills(kb_dir, extra_roots=extra_skill_roots) + match = next((s for s in skills if s["name"] == skill_name), None) + if match is None: + available = ", ".join(sorted(s["name"] for s in skills)) or "(none)" + raise SkillNotFoundError( + f"Skill {skill_name!r} not found. Available: {available}. " + f"Drop a SKILL.md into ~/.openkb/skills// or " + f"/skills// and re-run." + ) + + skill_md = Path(match["path"]) / "SKILL.md" + meta, body = _parse_frontmatter(skill_md.read_text(encoding="utf-8")) + od_meta: dict = (meta.get("od") or {}) if isinstance(meta, dict) else {} + + # Resolve output path if the skill templated one. + output_path: Optional[Path] = None + template = od_meta.get("output_path_template") + if template and slug: + rel = template.format(slug=slug) + output_path = (kb_dir / rel).resolve() + intent = ( + f"Output file (write the artifact here, full file in one " + f"write_file call): {rel}\n\n{intent}" + ) + + wiki_root = str(kb_dir / "wiki") + kb_root = str(kb_dir) + base = build_query_agent(wiki_root, model, language=language) + + @function_tool + def write_file(path: str, content: str) -> str: + """Write a text file under the KB. + + Allowed paths (relative to KB root): + * ``wiki/explorations/**`` — chat-derived notes. + * ``output/**`` — generator artifacts (skills, decks, etc.). + + Any other path is rejected. Parent directories are created. + """ + return write_kb_file(path, content, kb_root) + + @function_tool + def read_output_or_skill_file(path: str) -> str: + """Read any text file under the KB's ``output/`` or ``skills/``. + + Use this when the skill needs to inspect a previously-generated + artifact (e.g. critique an existing deck) or another skill's + body. For wiki content, prefer the dedicated wiki read tools. + + Args: + path: File path relative to the KB root, e.g. + ``"output/decks/foo/index.html"``. + """ + return read_kb_file(path, kb_root) + + agent = base.clone( + name=f"skill::{skill_name}", + instructions=(base.instructions or "") + + "\n\n# Skill instructions (you are this skill)\n\n" + + body + + "\n\n## User intent\n\n" + + intent, + tools=[*base.tools, write_file, read_output_or_skill_file], + ) + + user_seed = seed or ( + f"Follow the skill instructions above. Begin work now. " + f"User intent: {intent}" + ) + + from agents.exceptions import MaxTurnsExceeded + + try: + await Runner.run(agent, user_seed, max_turns=max_turns) + except MaxTurnsExceeded as exc: + raise RuntimeError( + f"Skill {skill_name!r} hit the {max_turns}-step cap before " + f"finishing. The intent may be too broad or the wiki too large; " + f"try a tighter intent or split into smaller skills." + ) from exc + + result = SkillRunResult( + skill_name=skill_name, + output_path=output_path, + metadata=od_meta if isinstance(od_meta, dict) else {}, + ) + + # Post-run hooks driven by skill frontmatter. + if output_path is not None and not output_path.is_file(): + raise RuntimeError( + f"Skill {skill_name!r} finished but did not write the expected " + f"output file at {output_path}. The skill is either misconfigured " + f"or the wiki lacks content matching the intent." + ) + + if od_meta.get("mode") == "deck" and output_path is not None: + # Lazy import — skill_runner shouldn't pull in deck-specific code + # at import time, only when actually running a deck skill. + from openkb.deck.validator import DeckGrammar, validate_deck + + grammar: Optional[DeckGrammar] = od_meta.get("deck_grammar") + result.validation = validate_deck(output_path.parent, grammar=grammar) + + return result diff --git a/openkb/agent/skills.py b/openkb/agent/skills.py new file mode 100644 index 00000000..59023ceb --- /dev/null +++ b/openkb/agent/skills.py @@ -0,0 +1,109 @@ +"""Skill discovery for openkb chat — Anthropic-style SKILL.md format. + +Scans skill directories and returns the metadata list the OpenAI Agents +SDK expects for ``ShellTool.environment.skills``: a list of +``{name, description, path}`` dicts. + +Skill search roots (first hit wins on name collision): + + 1. ``/skills/`` — project-local skills shipped with the KB + 2. ``~/.openkb/skills/`` — user-global skills + 3. ``~/.claude/skills/`` — Claude Code's skill dir (interop bonus) + +Skill file layout:: + + // + SKILL.md # frontmatter + body + references/... # optional supporting files (the skill body can + # cite them; agent reads via shell) + +Frontmatter:: + + --- + name: my-skill + description: One-line trigger description the agent sees up front. + --- + +""" +from __future__ import annotations + +from pathlib import Path +from typing import Iterable, Tuple + +import yaml + + +DEFAULT_SKILL_ROOTS: Tuple[str, ...] = ( + "skills", # relative to kb_dir + "~/.openkb/skills", + "~/.claude/skills", +) + + +def _parse_frontmatter(text: str) -> Tuple[dict, str]: + """Return ``(metadata_dict, body)`` from a markdown file with YAML + frontmatter delimited by ``---`` lines. Files without frontmatter + return ``({}, full_text)``. + """ + lines = text.splitlines() + if not lines or lines[0].strip() != "---": + return {}, text + try: + end = lines.index("---", 1) + except ValueError: + return {}, text + try: + meta = yaml.safe_load("\n".join(lines[1:end])) or {} + except yaml.YAMLError: + meta = {} + body = "\n".join(lines[end + 1:]) + return meta if isinstance(meta, dict) else {}, body + + +def scan_local_skills( + kb_dir: Path, + extra_roots: Iterable[str | Path] = (), +) -> list[dict[str, str]]: + """Scan known skill directories. Return SDK-shape skill list. + + Each entry is ``{"name": str, "description": str, "path": str}`` — + the exact shape :class:`agents.ShellToolLocalSkill` expects. + + Args: + kb_dir: KB root. Used to resolve the relative ``skills/`` root. + extra_roots: Additional roots to scan, appended after defaults. + + Returns: + List of skill metadata dicts. Empty if no skills found. + """ + seen: dict[str, dict[str, str]] = {} + roots = list(DEFAULT_SKILL_ROOTS) + [str(r) for r in extra_roots] + for root_spec in roots: + root = Path(root_spec).expanduser() + if not root.is_absolute(): + root = kb_dir / root + if not root.is_dir(): + continue + for skill_dir in sorted(root.iterdir()): + if not skill_dir.is_dir(): + continue + md = skill_dir / "SKILL.md" + if not md.is_file(): + continue + try: + text = md.read_text(encoding="utf-8") + except OSError: + continue + meta, _body = _parse_frontmatter(text) + name = str(meta.get("name") or skill_dir.name).strip() + desc = str(meta.get("description") or "").strip() + if not name or not desc: + continue # SDK requires both + if name in seen: + continue # first-hit-wins; earlier roots take precedence + seen[name] = { + "name": name, + "description": desc[:1024], + "path": str(skill_dir.resolve()), + } + return list(seen.values()) diff --git a/openkb/agent/tools.py b/openkb/agent/tools.py index 76520785..f954623f 100644 --- a/openkb/agent/tools.py +++ b/openkb/agent/tools.py @@ -167,6 +167,40 @@ def read_wiki_image(path: str, wiki_root: str) -> dict: return {"type": "image", "image_url": f"data:{mime};base64,{b64}"} +def read_kb_file(path: str, kb_root: str) -> str: + """Read a text file from the KB, restricted to safe read zones. + + Allowed prefixes (relative to *kb_root*): + * ``wiki/**`` — compiled wiki content (full read access). + * ``output/**`` — generated artifacts (decks, skills, etc.) for + critic / iteration workflows. + * ``skills/**`` — locally installed skills' bodies. + + Args: + path: File path relative to *kb_root*. + kb_root: Absolute path to the KB root directory. + + Returns: + File content on success, or an access-denied / not-found message. + """ + if not path: + return "Access denied: empty path." + root = Path(kb_root).resolve() + full_path = (root / path).resolve() + if not full_path.is_relative_to(root): + return "Access denied: path escapes KB root." + rel = full_path.relative_to(root) + if not rel.parts: + return "Access denied: KB root itself is not readable." + if rel.parts[0] not in ("wiki", "output", "skills"): + return ( + "Access denied: path must be under wiki/, output/, or skills/." + ) + if not full_path.is_file(): + return f"File not found: {path}" + return full_path.read_text(encoding="utf-8", errors="replace") + + def write_kb_file(path: str, content: str, kb_root: str) -> str: """Write a text file under the KB, restricted to safe write zones. diff --git a/openkb/cli.py b/openkb/cli.py index d5e6ff1a..04fe3b02 100644 --- a/openkb/cli.py +++ b/openkb/cli.py @@ -1909,3 +1909,176 @@ def skill_eval(ctx, name, save_flag, eval_set_path, count): if save_flag and eval_set is None: path = save_eval_set(kb_dir, name, result.prompts) click.echo(f"\nEval set persisted to {path}") + + +# --------------------------------------------------------------------------- +# `openkb deck ...` — deck factory (v0.2) +# --------------------------------------------------------------------------- + + +@cli.group() +def deck(): + """Generate a polished single-file HTML slide deck from the wiki.""" + + +@deck.command("new") +@click.argument("name") +@click.argument("intent") +@click.option( + "-y", "--yes", "yes_flag", + is_flag=True, default=False, + help="Overwrite existing output/decks// without prompting.", +) +@click.option( + "--critique", "critique_flag", + is_flag=True, default=False, + help="Opt-in second-pass review via a critic agent (slower, higher quality).", +) +@click.option( + "--skill", "skill_name", + metavar="SKILL_NAME", + default=None, + help=( + "Which deck skill to use. Defaults to 'openkb-deck-editorial' " + "(the built-in). Pass e.g. 'deck-guizang-editorial' to route to " + "a third-party skill installed under ~/.openkb/skills/." + ), +) +@click.pass_context +def deck_new(ctx, name, intent, yes_flag, critique_flag, skill_name): + """Generate a new HTML deck from this KB's wiki. + + NAME is a kebab-case slug used for the output directory. + INTENT is a natural-language description of what the deck is about. + + Example: + + openkb deck new transformers-pitch "Explain attention to engineers" + openkb deck new transformers-pitch "Explain attention to engineers" --critique + openkb deck new transformers-pitch "..." --skill deck-guizang-editorial + """ + 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) + + # Reuse the shared safety gates: name validation + wiki content check. + # Matches chat's `/deck new` so users see the same errors in both UIs. + err = _preflight_skill_new(kb_dir, name) + if err: + # _preflight_skill_new returns messages like "Skill name must not be empty." + # and "Wiki at ... is empty — add documents with `openkb add` first." + err = err.replace("Skill name", "Deck name") + # Only append the kebab-case hint when the failure is actually about + # the slug, not the wiki-content gate. + if "kebab" not in err.lower() and "Wiki" not in err and "wiki" not in err: + err = err + " Use a kebab-case slug like 'my-deck'." + click.echo(f"[ERROR] {err}", err=True) + ctx.exit(1) + + # Verify LLM key + load config BEFORE touching existing output. Any + # failure here (missing API key, malformed config) must leave the old + # deck directory intact — we can't replace it if we can't proceed. + try: + _setup_llm_key(kb_dir) + except RuntimeError as exc: + click.echo(f"[ERROR] {exc}", err=True) + ctx.exit(1) + config = load_config(kb_dir / ".openkb" / "config.yaml") + model = config.get("model", DEFAULT_CONFIG["model"]) + + # Overwrite handling — inline because openkb.skill.workspace.save_iteration + # is hard-wired to skill paths (uses skill_dir / skill_workspace_dir from + # openkb.skill). Mirror its iteration-N copy-then-rmtree behavior here + # using deck_workspace_dir so users keep rollback safety without coupling + # deck CLI to skill internals. + from openkb.deck import deck_dir as _deck_dir, deck_workspace_dir as _deck_workspace_dir + + target = _deck_dir(kb_dir, name) + if target.exists(): + if yes_flag: + _save_deck_iteration(kb_dir, name) + shutil.rmtree(target) + elif sys.stdin.isatty(): + if not click.confirm( + f"output/decks/{name}/ already exists. Overwrite?", + default=False, + ): + click.echo("Aborted.") + ctx.exit(1) + _save_deck_iteration(kb_dir, name) + shutil.rmtree(target) + else: + click.echo( + f"[ERROR] output/decks/{name}/ exists. Pass -y to overwrite " + f"in non-interactive contexts.", + err=True, + ) + ctx.exit(1) + + # Run the generator. + from openkb.skill.generator import Generator + skill_label = skill_name if skill_name else "openkb-deck-editorial (default)" + click.echo(f"Generating deck '{name}' via skill {skill_label}...") + gen = Generator( + target_type="deck", + name=name, + intent=intent, + kb_dir=kb_dir, + model=model, + critique=critique_flag, + skill_name=skill_name, + ) + try: + asyncio.run(gen.run()) + except RuntimeError as exc: + click.echo(f"[ERROR] {exc}", err=True) + ctx.exit(1) + + # Surface validation result. + if gen.validation: + for w in gen.validation.warnings: + click.echo(f"[WARN] {w}", err=True) + for e in gen.validation.errors: + click.echo(f"[ERROR] {e}", err=True) + if gen.validation.errors: + click.echo( + f"Deck written to {gen.output_dir / 'index.html'} but failed validation. " + f"Inspect and re-run.", + err=True, + ) + ctx.exit(1) + + click.echo(f"Deck written to {gen.output_dir / 'index.html'}") + + +def _save_deck_iteration(kb_dir: Path, deck_name: str) -> Path | None: + """Copy ``/output/decks//`` to the next iteration slot. + + Mirrors ``openkb.skill.workspace.save_iteration`` but uses + ``deck_workspace_dir`` so deck rollback history stays separate from + skill history. Returns the saved iteration path, or ``None`` if there's + no current deck to save. + """ + import re + from openkb.deck import deck_dir as _deck_dir, deck_workspace_dir as _deck_workspace_dir + + src = _deck_dir(kb_dir, deck_name) + if not src.is_dir(): + return None + + ws = _deck_workspace_dir(kb_dir, deck_name) + ws.mkdir(parents=True, exist_ok=True) + + iter_re = re.compile(r"^iteration-(\d+)$") + existing_ns: list[int] = [] + for child in ws.iterdir(): + if child.is_dir(): + m = iter_re.match(child.name) + if m: + existing_ns.append(int(m.group(1))) + next_n = (max(existing_ns) if existing_ns else 0) + 1 + + dest = ws / f"iteration-{next_n}" + shutil.copytree(src, dest) + return dest diff --git a/openkb/deck/__init__.py b/openkb/deck/__init__.py new file mode 100644 index 00000000..79fa62c1 --- /dev/null +++ b/openkb/deck/__init__.py @@ -0,0 +1,33 @@ +"""Deck target — HTML slide-deck generator. Second Generator target after skill. + +This package mirrors openkb/skill/ structurally. Today it owns: +* path construction — ``deck_dir`` / ``decks_root`` / ``deck_workspace_dir`` + +A deck is a single self-contained ``index.html`` file at +``/output/decks//index.html``. Workspace iteration history lives +at ``/output/decks/-workspace/iteration-N/``. +""" +from __future__ import annotations + +from pathlib import Path + +__all__ = [ + "decks_root", + "deck_dir", + "deck_workspace_dir", +] + + +def decks_root(kb_dir: Path) -> Path: + """``/output/decks`` — the directory holding every compiled deck.""" + return kb_dir / "output" / "decks" + + +def deck_dir(kb_dir: Path, deck_name: str) -> Path: + """``/output/decks/`` — one compiled deck's home.""" + return decks_root(kb_dir) / deck_name + + +def deck_workspace_dir(kb_dir: Path, deck_name: str) -> Path: + """``/output/decks/-workspace`` — iteration history for a deck.""" + return decks_root(kb_dir) / f"{deck_name}-workspace" diff --git a/openkb/deck/creator.py b/openkb/deck/creator.py new file mode 100644 index 00000000..0c7d26e5 --- /dev/null +++ b/openkb/deck/creator.py @@ -0,0 +1,122 @@ +"""Thin CLI/Generator wrapper around any deck-producing skill. + +The actual deck generation lives in skill packages — by default +``skills/openkb-deck-editorial/SKILL.md``, but the caller may pass +``skill_name`` to route to a different one (e.g. +``deck-guizang-editorial`` from open-design). Skill execution runs +through :func:`openkb.agent.skill_runner.run_skill`, which also handles +output-path templating and post-run validation based on the skill's +``od:`` frontmatter. + +This module exists only to: + +* expose the deck CLI's ``critique`` flag as a second skill call + (``openkb-html-critic``) chained after the producer skill, +* surface a clean ``Path``-returning interface for callers that don't + care about skill plumbing. +""" +from __future__ import annotations + +from pathlib import Path + +from openkb.agent.skill_runner import ( + MAX_TURNS, + MAX_TURNS_WITH_CRITIQUE, + SkillNotFoundError, + SkillRunResult, + run_skill, +) +from openkb.deck import deck_dir + + +DEFAULT_DECK_SKILL = "openkb-deck-editorial" +"""Skill name routed to when the CLI / chat doesn't pass ``--skill``.""" + +CRITIC_SKILL = "openkb-html-critic" +"""Skill chained after the producer when ``--critique`` is set.""" + +CRITIC_MAX_TURNS = 40 +"""Critic is read-and-patch, not authoring; converges fast.""" + + +async def run_deck_create( + *, + kb_dir: Path, + deck_name: str, + intent: str, + model: str, + critique: bool, + skill_name: str = DEFAULT_DECK_SKILL, +) -> SkillRunResult: + """Compile a single deck from the KB's wiki via the chosen skill. + + Args: + skill_name: Which deck skill to run. Defaults to the built-in + ``openkb-deck-editorial``. Pass ``"deck-guizang-editorial"`` + etc. to route to a third-party skill installed under + ``~/.openkb/skills/``. + + Returns the :class:`SkillRunResult` from the producer skill (carries + ``output_path`` and ``validation`` populated by ``run_skill`` per + the skill's frontmatter contract). + + Raises ``RuntimeError`` if the skill is missing, hits the turn cap, + or doesn't write its declared output path. + + When ``critique=True`` the html-critic skill runs as a second pass + on the produced file. Missing critic skill is a soft failure (the + deck still ships, just unpatched). + """ + # Ensure the conventional deck dir exists. Skills that use + # output_path_template = "output/decks/{slug}/index.html" need this; + # skills that pick their own location won't be hindered. + deck_root = deck_dir(kb_dir, deck_name) + deck_root.mkdir(parents=True, exist_ok=True) + + try: + result = await run_skill( + skill_name=skill_name, + intent=intent, + kb_dir=kb_dir, + model=model, + slug=deck_name, + max_turns=MAX_TURNS_WITH_CRITIQUE if critique else MAX_TURNS, + ) + except SkillNotFoundError as exc: + raise RuntimeError( + f"Deck skill {skill_name!r} is not installed. " + f"Drop a SKILL.md into ~/.openkb/skills/{skill_name}/ (or " + f"/skills/{skill_name}/) and re-run." + ) from exc + + if critique: + # The producer's output_path tells the critic which file to patch. + # If the producer didn't template a path, fall back to the + # conventional location. + # + # ``result.output_path`` is already ``.resolve()``-d by run_skill; + # ``kb_dir`` may still hold an un-resolved form (e.g. ``/tmp/...`` + # on macOS where ``/tmp`` symlinks to ``/private/tmp``). Resolve + # the KB root too so ``relative_to`` doesn't trip on the symlink. + target_path = ( + result.output_path.relative_to(kb_dir.resolve()) + if result.output_path is not None + else Path(f"output/decks/{deck_name}/index.html") + ) + critic_intent = ( + f"Critique and patch the HTML deck at: {target_path}\n" + f"Original user brief (for context, do NOT change content):\n{intent}" + ) + try: + await run_skill( + skill_name=CRITIC_SKILL, + intent=critic_intent, + kb_dir=kb_dir, + model=model, + max_turns=CRITIC_MAX_TURNS, + ) + except SkillNotFoundError: + # Critic missing is non-fatal — the unpatched deck still ships. + pass + + return result diff --git a/openkb/deck/validator.py b/openkb/deck/validator.py new file mode 100644 index 00000000..39cfd1f8 --- /dev/null +++ b/openkb/deck/validator.py @@ -0,0 +1,244 @@ +"""Structural validation for a generated deck. + +Default mode (``grammar=None``) only enforces SKILL-AGNOSTIC invariants: +file exists, parses as HTML, ≥ 5 ``
`` blocks, +self-contained (no external link/script/img references), reasonable size. + +A skill that wants stricter checks can declare its slide grammar in the +SKILL.md frontmatter under ``od.deck_grammar`` and pass it as the +``grammar`` argument. The Editorial Monocle skill does this — its +grammar names the 7 ``data-type`` values plus the cover/closing +requirement; guizang / swiss / any community skill simply omit it and +get the generic validation. + +Mirrors ``openkb/skill/validator.py``'s ``ValidationResult`` shape so +callers can format issues identically regardless of artifact type. +""" +from __future__ import annotations + +from dataclasses import dataclass, field +from html.parser import HTMLParser +from pathlib import Path +from typing import Optional, TypedDict + +__all__ = [ + "ALLOWED_DATA_TYPES", + "DeckGrammar", + "EDITORIAL_MONOCLE_GRAMMAR", + "ValidationResult", + "validate_deck", +] + + +class DeckGrammar(TypedDict, total=False): + """Skill-declared rules for slide classification. + + All keys are optional. If a key is missing, the corresponding check + is skipped. This is what a skill writer puts under + ``frontmatter.od.deck_grammar`` to opt into structural validation. + + Example (Editorial Monocle skill):: + + od: + mode: deck + deck_grammar: + kind_attr: data-type + required: [cover, closing] + allowed: [cover, chapter, thesis, quote, compare, data, closing] + min_distinct: 4 + max_consecutive_same: 2 + """ + kind_attr: str # attribute name carrying the slide kind (e.g. "data-type") + required: list[str] # kinds that MUST appear at least once + allowed: list[str] # whitelist; anything else is rejected + min_distinct: int # warn if fewer distinct kinds present + max_consecutive_same: int # warn if run-length exceeds this + + +# Editorial Monocle's published grammar. Kept here for the openkb-deck-editorial +# skill to import (and for tests / docs to reference); third-party skills may +# define their own or omit grammar entirely. +EDITORIAL_MONOCLE_GRAMMAR: DeckGrammar = { + "kind_attr": "data-type", + "required": ["cover", "closing"], + "allowed": ["cover", "chapter", "thesis", "quote", "compare", "data", "closing"], + "min_distinct": 4, + "max_consecutive_same": 2, +} + +# Legacy alias used by tests that pinned the old name. New code should +# read this from EDITORIAL_MONOCLE_GRAMMAR["allowed"]. +ALLOWED_DATA_TYPES: frozenset[str] = frozenset(EDITORIAL_MONOCLE_GRAMMAR["allowed"]) + +MAX_FILE_BYTES = 2 * 1024 * 1024 # 2 MB +MIN_SLIDES_HARD = 5 # error threshold (skill-agnostic) +MIN_SLIDES_SOFT = 8 # warning threshold (count outside [8,15]) +MAX_SLIDES_SOFT = 15 + + +@dataclass +class ValidationResult: + errors: list[str] = field(default_factory=list) + warnings: list[str] = field(default_factory=list) + + @property + def ok(self) -> bool: + return not self.errors + + +class _DeckParser(HTMLParser): + """Collects ``
`` blocks and any external refs. + + The slide kind (e.g. ``data-type="cover"`` for Editorial Monocle) is + extracted lazily — ``slide_kinds`` is keyed by the configured + ``kind_attr``; an empty string means the slide didn't declare a kind + under that attr. + """ + + def __init__(self, kind_attr: Optional[str] = None) -> None: + super().__init__() + self.kind_attr = kind_attr + self.slide_kinds: list[str] = [] + self.external_links: list[str] = [] + + def handle_starttag(self, tag: str, attrs: list[tuple[str, str | None]]) -> None: + a = dict(attrs) + if tag == "section" and "slide" in (a.get("class") or "").split(): + if self.kind_attr is None: + # Skill-agnostic: just count slides; kind is irrelevant. + self.slide_kinds.append("") + else: + self.slide_kinds.append((a.get(self.kind_attr) or "").strip()) + elif tag == "link": + href = (a.get("href") or "").strip() + if href.startswith(("http://", "https://", "//")): + self.external_links.append(f"") + elif tag == "script": + src = (a.get("src") or "").strip() + if src.startswith(("http://", "https://", "//")): + self.external_links.append(f"', + ) + result = validate_deck(_write(tmp_path, html)) + assert any("not self-contained" in e for e in result.errors) + + +def test_external_img_blocks(tmp_path: Path): + html = GOOD_DECK.replace( + '
"…"
', + '
', + ) + result = validate_deck(_write(tmp_path, html)) + assert any("not self-contained" in e for e in result.errors) + + +def test_few_slides_warning(tmp_path: Path): + # 6 slides — passes hard floor (5), but warns (< 8). + html = ( + "" + '
' + '
' + '
' + '
' + '
' + '
' + "" + ) + result = validate_deck(_write(tmp_path, html)) + assert result.errors == [] + assert any("slide count 6" in w for w in result.warnings) + + +def test_run_of_3_same_type_warning(tmp_path: Path): + html = ( + "" + '
' + '
' + '
' + '
' + '
' + '
' + '
' + '
' + "" + ) + result = validate_deck(_write(tmp_path, html), grammar=EDITORIAL_MONOCLE_GRAMMAR) + assert result.errors == [] + assert any("consecutive" in w for w in result.warnings) + + +def test_low_distinct_types_warning(tmp_path: Path): + # 8 slides but only 3 distinct types (cover, thesis, closing). + html = ( + "" + '
' + + '
' * 6 + + '
' + "" + ) + result = validate_deck(_write(tmp_path, html), grammar=EDITORIAL_MONOCLE_GRAMMAR) + # Errors fine; this run-length and distinct-count will both warn. + assert any("distinct data-type" in w for w in result.warnings) + + +def test_no_slides_no_distinct_warning(tmp_path: Path): + # A deck with zero slides already produces hard errors; the distinct-type + # warning ("only 0 distinct…") is noise on an empty deck and is suppressed. + html = "" + result = validate_deck(_write(tmp_path, html), grammar=EDITORIAL_MONOCLE_GRAMMAR) + assert not any("distinct data-type" in w for w in result.warnings) + + +def test_oversize_file_warning(tmp_path: Path, monkeypatch): + # Lower the size threshold so we don't actually allocate 2MB to + # trigger the warning branch. + monkeypatch.setattr( + "openkb.deck.validator.MAX_FILE_BYTES", + 100, # threshold 100 bytes for the test + ) + result = validate_deck(_write(tmp_path, GOOD_DECK)) + assert any("MB" in w for w in result.warnings) diff --git a/tests/test_generator.py b/tests/test_generator.py index 37d574f3..4ce9f145 100644 --- a/tests/test_generator.py +++ b/tests/test_generator.py @@ -58,3 +58,84 @@ async def test_generator_run_delegates_to_skill_creator(tmp_path): await g.run() runner.assert_awaited_once() regen.assert_called_once_with(kb) + + +# --- target_type="deck" dispatch ------------------------------------------- + +from openkb.deck.validator import ValidationResult as DeckValidationResult + + +@pytest.mark.asyncio +async def test_generator_deck_dispatches_to_deck_creator(tmp_path): + kb_dir = tmp_path + (kb_dir / "wiki").mkdir() + (kb_dir / "wiki" / "AGENTS.md").write_text("schema", encoding="utf-8") + + gen = Generator( + target_type="deck", + name="my-deck", + intent="…", + kb_dir=kb_dir, + model="openai/gpt-4o", + critique=False, + ) + + # Post-refactor: validate_deck moved into run_skill (called inside + # run_deck_create). Generator just propagates the SkillRunResult's + # validation up to self.validation. + from openkb.agent.skill_runner import SkillRunResult + fake_run_result = SkillRunResult( + skill_name="openkb-deck-editorial", + output_path=gen.output_dir / "index.html", + validation=DeckValidationResult(), + metadata={"mode": "deck"}, + ) + + with patch("openkb.skill.generator.run_deck_create", new_callable=AsyncMock) as run_dc, \ + patch("openkb.skill.generator.regenerate_marketplace") as regen: + run_dc.return_value = fake_run_result + result = await gen.run() + + run_dc.assert_awaited_once_with( + kb_dir=kb_dir, + deck_name="my-deck", + intent="…", + model="openai/gpt-4o", + critique=False, + skill_name="openkb-deck-editorial", + ) + regen.assert_not_called() # marketplace is skill-only + assert result == gen.output_dir + assert gen.validation is fake_run_result.validation + + +@pytest.mark.asyncio +async def test_generator_deck_output_dir_is_decks(tmp_path): + gen = Generator( + target_type="deck", + name="my-deck", + intent="…", + kb_dir=tmp_path, + model="openai/gpt-4o", + critique=False, + ) + assert gen.output_dir == tmp_path / "output" / "decks" / "my-deck" + + +def test_generator_rejects_podcast_target_type(tmp_path): + with pytest.raises(ValueError, match="Unknown target_type"): + Generator( + target_type="podcast", # type: ignore[arg-type] + name="x", + intent="…", + kb_dir=tmp_path, + model="openai/gpt-4o", + critique=False, + ) + + +# Pre-skill-system snapshot/restore Generator tests removed: the +# html-critic skill patches HTML in place, so there is no longer a +# pre-critique snapshot to clean up or restore. See +# tests/test_deck_creator.py::test_run_deck_create_chains_critic_when_critique_true +# for the equivalent guarantee at the new layer. diff --git a/tests/test_read_kb_file.py b/tests/test_read_kb_file.py new file mode 100644 index 00000000..1d67f545 --- /dev/null +++ b/tests/test_read_kb_file.py @@ -0,0 +1,132 @@ +"""Regression tests for ``openkb.agent.tools.read_kb_file``. + +Symmetric to :mod:`tests.test_write_kb_file`. ``read_kb_file`` is the +read-side allow-list exposed to skill agents via the +``read_output_or_skill_file`` function tool in +``openkb.agent.skill_runner``. It controls what files the agent can +inspect — particularly that it **cannot** see ``.openkb/config.yaml`` +(which contains the LLM API key path), ``.env``, or anything outside +``wiki/``, ``output/``, ``skills/``. +""" +from __future__ import annotations + +from pathlib import Path + +import pytest + +from openkb.agent.tools import read_kb_file + + +@pytest.fixture +def kb_root(tmp_path: Path) -> str: + return str(tmp_path) + + +def test_rejects_empty_path(kb_root: str) -> None: + result = read_kb_file("", kb_root) + assert result.startswith("Access denied") + + +def test_rejects_path_escape_via_dotdot(kb_root: str, tmp_path: Path) -> None: + """Even if the relative path resolves outside kb_root via ``..``, + the guard must reject it before reading anything.""" + # Create a secret file outside the KB + secret = tmp_path.parent / "secret.txt" + secret.write_text("API_KEY=xxx") + try: + result = read_kb_file("../secret.txt", kb_root) + assert result.startswith("Access denied") + finally: + secret.unlink(missing_ok=True) + + +def test_rejects_absolute_path_outside_kb(kb_root: str) -> None: + result = read_kb_file("/etc/passwd", kb_root) + assert result.startswith("Access denied") + + +def test_rejects_dotopenkb_config(kb_root: str, tmp_path: Path) -> None: + """The .openkb/ directory holds the user's model config and is the + canonical place an attacker would aim for. Must be inaccessible.""" + (tmp_path / ".openkb").mkdir() + (tmp_path / ".openkb" / "config.yaml").write_text("model: gpt-5.4\n") + result = read_kb_file(".openkb/config.yaml", kb_root) + assert result.startswith("Access denied") + + +def test_rejects_env_file(kb_root: str, tmp_path: Path) -> None: + """``.env`` holds API keys; must not be readable by skill agents.""" + (tmp_path / ".env").write_text("OPENAI_API_KEY=sk-xxx") + result = read_kb_file(".env", kb_root) + assert result.startswith("Access denied") + + +def test_rejects_raw_dir(kb_root: str, tmp_path: Path) -> None: + """The ``raw/`` directory holds pre-ingest source documents — not + in the read allow-list.""" + (tmp_path / "raw").mkdir() + (tmp_path / "raw" / "doc.pdf").write_bytes(b"PDF bytes") + result = read_kb_file("raw/doc.pdf", kb_root) + assert result.startswith("Access denied") + + +def test_rejects_bare_kb_root(kb_root: str) -> None: + """A bare empty/root path must be rejected.""" + result = read_kb_file(".", kb_root) + # Either rejected as escape or as empty-after-relative — either is + # a refusal, which is what we want. + assert result.startswith("Access denied") or "not a file" in result.lower() + + +def test_returns_not_found_for_missing_allowed_path(kb_root: str, tmp_path: Path) -> None: + """Inside the allow-list but file missing — clear error, no leak.""" + (tmp_path / "output").mkdir() + result = read_kb_file("output/decks/missing/index.html", kb_root) + assert "not found" in result.lower() + + +def test_reads_wiki_file(kb_root: str, tmp_path: Path) -> None: + (tmp_path / "wiki").mkdir() + target = tmp_path / "wiki" / "index.md" + target.write_text("# Wiki content", encoding="utf-8") + result = read_kb_file("wiki/index.md", kb_root) + assert result == "# Wiki content" + + +def test_reads_output_file(kb_root: str, tmp_path: Path) -> None: + """The whole point of ``read_kb_file`` (vs the existing wiki-only + reader): skill agents need to inspect their own prior outputs to + critique / iterate. Confirm ``output/`` is in the read scope.""" + out = tmp_path / "output" / "decks" / "foo" + out.mkdir(parents=True) + (out / "index.html").write_text("deck content", encoding="utf-8") + result = read_kb_file("output/decks/foo/index.html", kb_root) + assert "deck content" in result + + +def test_reads_skill_file(kb_root: str, tmp_path: Path) -> None: + """Skill bodies must be readable so a meta-skill can introspect + another skill (rare but legitimate use).""" + sk = tmp_path / "skills" / "my-skill" + sk.mkdir(parents=True) + (sk / "SKILL.md").write_text("---\nname: my-skill\n---\nbody", encoding="utf-8") + result = read_kb_file("skills/my-skill/SKILL.md", kb_root) + assert "name: my-skill" in result + + +def test_rejects_relative_traversal_inside_allow_list(kb_root: str, tmp_path: Path) -> None: + """`output/../.env` must NOT be allowed even though it starts with + `output/`. The resolve-then-check pattern handles this; pin it.""" + (tmp_path / ".env").write_text("OPENAI_API_KEY=sk-xxx") + (tmp_path / "output").mkdir() + result = read_kb_file("output/../.env", kb_root) + assert result.startswith("Access denied") + + +def test_directory_target_returns_not_found(kb_root: str, tmp_path: Path) -> None: + """If the path resolves to a directory inside the allow-list, the + reader treats it as not-a-file rather than crashing.""" + (tmp_path / "output" / "decks").mkdir(parents=True) + result = read_kb_file("output/decks", kb_root) + # Directory exists but is not a file — should return a clear message. + assert "not found" in result.lower() or "not a file" in result.lower() diff --git a/tests/test_skill_runner.py b/tests/test_skill_runner.py new file mode 100644 index 00000000..34670130 --- /dev/null +++ b/tests/test_skill_runner.py @@ -0,0 +1,307 @@ +"""Direct unit tests for :mod:`openkb.agent.skill_runner`. + +Pre-existing test files mocked ``run_skill`` at every caller (deck +creator, generator, chat slash). That covered the dispatch boundary but +left the function itself untested. The tests below construct synthetic +``SKILL.md`` files under ``tmp_path`` and patch ``agents.Runner.run`` so +we can verify the assembly path end-to-end without spending tokens: + +* skill lookup — ``SkillNotFoundError`` with a helpful "available" list +* prompt assembly — body lands in ``agent.instructions`` followed by + the "## User intent" section +* tools wired — ``write_file`` + ``read_output_or_skill_file`` present + alongside the inherited wiki-read tools +* output-path templating — ``{slug}`` substituted, injected into + intent, file existence enforced post-run +* deck-mode validator hook — when frontmatter ``od.mode == "deck"``, + ``validate_deck`` runs with the skill's grammar after the agent finishes +* MaxTurnsExceeded → RuntimeError translation with a useful message +""" +from __future__ import annotations + +from pathlib import Path +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +from openkb.agent.skill_runner import ( + MAX_TURNS, + SkillNotFoundError, + SkillRunResult, + run_skill, +) + + +def _make_kb(tmp_path: Path) -> Path: + """Minimal KB layout so build_query_agent inside run_skill can boot.""" + (tmp_path / "wiki").mkdir() + (tmp_path / "wiki" / "AGENTS.md").write_text("schema", encoding="utf-8") + return tmp_path + + +def _install_skill( + kb_dir: Path, + name: str, + *, + body: str = "Do the thing.", + od: dict | None = None, + description: str = "A test skill.", +) -> Path: + """Drop a SKILL.md under ``/skills//`` and return its path.""" + sk_dir = kb_dir / "skills" / name + sk_dir.mkdir(parents=True) + frontmatter = {"name": name, "description": description} + if od is not None: + frontmatter["od"] = od + # Write YAML by hand to avoid a yaml.dump dependency on test side + fm_lines = ["---"] + for k, v in frontmatter.items(): + if isinstance(v, dict): + fm_lines.append(f"{k}:") + for kk, vv in v.items(): + if isinstance(vv, dict): + fm_lines.append(f" {kk}:") + for kkk, vvv in vv.items(): + fm_lines.append(f" {kkk}: {vvv!r}") + else: + fm_lines.append(f" {kk}: {vv!r}") + else: + fm_lines.append(f"{k}: {v!r}") + fm_lines.append("---") + (sk_dir / "SKILL.md").write_text("\n".join(fm_lines) + "\n" + body, encoding="utf-8") + return sk_dir + + +@pytest.mark.asyncio +async def test_run_skill_raises_skill_not_found_with_available_list(tmp_path: Path): + kb_dir = _make_kb(tmp_path) + _install_skill(kb_dir, "alpha") + _install_skill(kb_dir, "beta") + + with pytest.raises(SkillNotFoundError) as exc_info: + await run_skill( + skill_name="nonexistent", + intent="anything", + kb_dir=kb_dir, + model="openai/gpt-4o", + ) + msg = str(exc_info.value) + # Helpful: lists what IS available so the user can see the typo + assert "alpha" in msg + assert "beta" in msg + assert "nonexistent" in msg + + +@pytest.mark.asyncio +async def test_run_skill_loads_body_into_instructions(tmp_path: Path): + kb_dir = _make_kb(tmp_path) + _install_skill(kb_dir, "marker-skill", body="UNIQUE-BODY-MARKER") + + captured: dict = {} + + async def fake_runner_run(agent, seed, **kw): + captured["instructions"] = agent.instructions + captured["tools"] = [getattr(t, "name", "?") for t in agent.tools] + return MagicMock() + + with patch("openkb.agent.skill_runner.Runner.run", new=fake_runner_run): + result = await run_skill( + skill_name="marker-skill", + intent="DO-THE-INTENT", + kb_dir=kb_dir, + model="openai/gpt-4o", + ) + + # Body and intent are both present in the constructed agent's instructions. + assert "UNIQUE-BODY-MARKER" in captured["instructions"] + assert "DO-THE-INTENT" in captured["instructions"] + assert "## User intent" in captured["instructions"] + # The skill-runner's two distinguishing tools are wired in. + assert "write_file" in captured["tools"] + assert "read_output_or_skill_file" in captured["tools"] + # Return shape + assert isinstance(result, SkillRunResult) + assert result.skill_name == "marker-skill" + + +@pytest.mark.asyncio +async def test_run_skill_templates_output_path_and_enforces_existence(tmp_path: Path): + kb_dir = _make_kb(tmp_path) + _install_skill( + kb_dir, + "templated", + od={ + "mode": "deck", + "output_path_template": "output/decks/{slug}/index.html", + }, + ) + + captured: dict = {} + + async def fake_runner_run(agent, seed, **kw): + captured["instructions"] = agent.instructions + # Simulate the skill writing the expected file + out = kb_dir / "output" / "decks" / "my-slug" / "index.html" + out.parent.mkdir(parents=True, exist_ok=True) + out.write_text( + '
' * 8, + encoding="utf-8", + ) + return MagicMock() + + with patch("openkb.agent.skill_runner.Runner.run", new=fake_runner_run): + result = await run_skill( + skill_name="templated", + intent="brief", + kb_dir=kb_dir, + model="openai/gpt-4o", + slug="my-slug", + ) + + # Path was injected into agent's intent so the skill knows where to write + assert "output/decks/my-slug/index.html" in captured["instructions"] + # output_path resolved against kb_dir + assert result.output_path is not None + assert result.output_path.name == "index.html" + assert "my-slug" in str(result.output_path) + + +@pytest.mark.asyncio +async def test_run_skill_raises_if_templated_output_missing_post_run(tmp_path: Path): + """If the skill declared output_path_template but didn't write the + file, that's a hard error — the skill is misconfigured or the wiki + lacks content.""" + kb_dir = _make_kb(tmp_path) + _install_skill( + kb_dir, + "lazy-skill", + od={ + "mode": "deck", + "output_path_template": "output/decks/{slug}/index.html", + }, + ) + + async def fake_runner_run(agent, seed, **kw): + return MagicMock() # agent does nothing + + with patch("openkb.agent.skill_runner.Runner.run", new=fake_runner_run): + with pytest.raises(RuntimeError, match="did not write the expected"): + await run_skill( + skill_name="lazy-skill", + intent="x", + kb_dir=kb_dir, + model="openai/gpt-4o", + slug="ghost", + ) + + +@pytest.mark.asyncio +async def test_run_skill_runs_deck_validator_when_mode_is_deck(tmp_path: Path): + """When ``od.mode == "deck"`` and ``output_path_template`` is set, + ``run_skill`` calls ``validate_deck`` with the skill's grammar after + the run completes.""" + kb_dir = _make_kb(tmp_path) + _install_skill( + kb_dir, + "deck-with-grammar", + od={ + "mode": "deck", + "output_path_template": "output/decks/{slug}/index.html", + "deck_grammar": { + "kind_attr": "data-type", + "required": ["cover", "closing"], + }, + }, + ) + + async def fake_runner_run(agent, seed, **kw): + out = kb_dir / "output" / "decks" / "demo" / "index.html" + out.parent.mkdir(parents=True, exist_ok=True) + # 5 slides, NO closing — grammar should reject + out.write_text( + '
' + + '
' * 4, + encoding="utf-8", + ) + return MagicMock() + + with patch("openkb.agent.skill_runner.Runner.run", new=fake_runner_run): + result = await run_skill( + skill_name="deck-with-grammar", + intent="x", + kb_dir=kb_dir, + model="openai/gpt-4o", + slug="demo", + ) + + # Validation was applied with the skill's grammar; missing-closing fired + assert result.validation is not None + assert any("closing" in e for e in result.validation.errors) + + +@pytest.mark.asyncio +async def test_run_skill_no_validator_when_mode_not_deck(tmp_path: Path): + """Skills with ``od.mode`` other than ``"deck"`` (or no mode at all) + skip the validator — they're not deck-shaped artifacts.""" + kb_dir = _make_kb(tmp_path) + _install_skill(kb_dir, "no-mode-skill") # no od block at all + + async def fake_runner_run(agent, seed, **kw): + return MagicMock() + + with patch("openkb.agent.skill_runner.Runner.run", new=fake_runner_run): + result = await run_skill( + skill_name="no-mode-skill", + intent="x", + kb_dir=kb_dir, + model="openai/gpt-4o", + ) + + assert result.validation is None + + +@pytest.mark.asyncio +async def test_run_skill_translates_max_turns_exceeded(tmp_path: Path): + kb_dir = _make_kb(tmp_path) + _install_skill(kb_dir, "loopy") + from agents.exceptions import MaxTurnsExceeded + + async def fake_runner_run(agent, seed, **kw): + raise MaxTurnsExceeded("model spun forever") + + with patch("openkb.agent.skill_runner.Runner.run", new=fake_runner_run): + with pytest.raises(RuntimeError) as exc_info: + await run_skill( + skill_name="loopy", + intent="x", + kb_dir=kb_dir, + model="openai/gpt-4o", + max_turns=10, + ) + msg = str(exc_info.value) + assert "10" in msg + assert "step cap" in msg or "step" in msg.lower() + assert "loopy" in msg + + +@pytest.mark.asyncio +async def test_run_skill_default_max_turns(tmp_path: Path): + """Default ``max_turns`` is the module-level ``MAX_TURNS`` constant.""" + kb_dir = _make_kb(tmp_path) + _install_skill(kb_dir, "default-budget") + + captured: dict = {} + + async def fake_runner_run(agent, seed, **kw): + captured["max_turns"] = kw.get("max_turns") + return MagicMock() + + with patch("openkb.agent.skill_runner.Runner.run", new=fake_runner_run): + await run_skill( + skill_name="default-budget", + intent="x", + kb_dir=kb_dir, + model="openai/gpt-4o", + ) + + assert captured["max_turns"] == MAX_TURNS diff --git a/tests/test_skills.py b/tests/test_skills.py new file mode 100644 index 00000000..70dbabf3 --- /dev/null +++ b/tests/test_skills.py @@ -0,0 +1,216 @@ +"""Direct unit tests for :mod:`openkb.agent.skills`. + +Tests the scanner that ``run_skill`` / ``build_chat_agent`` depend on +to discover ``SKILL.md`` packages across multiple root directories. +Pre-existing tests only exercised the scanner indirectly (via a +test_deck_prompt assertion that the built-in deck skill loads), so +edge-case behavior (precedence, malformed frontmatter, missing fields, +description truncation) was silently uncovered. +""" +from __future__ import annotations + +from pathlib import Path + +import pytest + +from openkb.agent.skills import ( + DEFAULT_SKILL_ROOTS, + _parse_frontmatter, + scan_local_skills, +) + + +@pytest.fixture(autouse=True) +def _isolate_home(monkeypatch, tmp_path): + """Point ``$HOME`` at the test's tmp_path so the scanner's default + ``~/.openkb/skills`` and ``~/.claude/skills`` roots resolve to empty + locations under the test sandbox — otherwise the user's real + installed skills leak into every test.""" + fake_home = tmp_path / "isolated-home" + fake_home.mkdir() + monkeypatch.setenv("HOME", str(fake_home)) + + +def _write_skill( + root: Path, + name: str, + *, + description: str = "A test skill.", + body: str = "instructions", +) -> Path: + """Drop a minimally well-formed SKILL.md and return its directory.""" + sk_dir = root / name + sk_dir.mkdir(parents=True, exist_ok=True) + (sk_dir / "SKILL.md").write_text( + f"---\nname: {name}\ndescription: {description}\n---\n{body}", + encoding="utf-8", + ) + return sk_dir + + +# ─── scan_local_skills ────────────────────────────────────────────────── + + +def test_scan_returns_empty_when_no_skill_roots_exist(tmp_path: Path): + assert scan_local_skills(tmp_path) == [] + + +def test_scan_finds_kb_local_skills(tmp_path: Path): + _write_skill(tmp_path / "skills", "alpha") + _write_skill(tmp_path / "skills", "beta") + skills = scan_local_skills(tmp_path) + names = {s["name"] for s in skills} + assert names == {"alpha", "beta"} + + +def test_scan_returns_sdk_shape(tmp_path: Path): + """SDK contract: each entry has 'name', 'description', 'path' keys + of type str. ``path`` is absolute (resolved).""" + sk_dir = _write_skill(tmp_path / "skills", "shape-check") + skills = scan_local_skills(tmp_path) + assert len(skills) == 1 + s = skills[0] + assert set(s.keys()) == {"name", "description", "path"} + assert all(isinstance(s[k], str) for k in s) + assert Path(s["path"]).is_absolute() + assert Path(s["path"]) == sk_dir.resolve() + + +def test_scan_skips_dirs_without_skill_md(tmp_path: Path): + """A subdirectory under skills/ that doesn't have SKILL.md is ignored, + not an error.""" + (tmp_path / "skills" / "no-skill-here").mkdir(parents=True) + (tmp_path / "skills" / "no-skill-here" / "README.md").write_text("nope") + _write_skill(tmp_path / "skills", "real-skill") + skills = scan_local_skills(tmp_path) + assert [s["name"] for s in skills] == ["real-skill"] + + +def test_scan_skips_skill_without_description(tmp_path: Path): + """SDK requires both name and description; missing description -> + silently skipped (don't crash, don't include).""" + sk_dir = tmp_path / "skills" / "no-desc" + sk_dir.mkdir(parents=True) + (sk_dir / "SKILL.md").write_text("---\nname: no-desc\n---\nbody") + # And one well-formed one for control + _write_skill(tmp_path / "skills", "well-formed") + skills = scan_local_skills(tmp_path) + assert [s["name"] for s in skills] == ["well-formed"] + + +def test_scan_falls_back_to_dir_name_when_frontmatter_omits_name(tmp_path: Path): + """If frontmatter has ``description`` but no ``name``, the scanner + uses the directory name as a sane default. This lets a user drop a + skill into ``~/.openkb/skills/my-deck/`` without writing the name + twice.""" + sk_dir = tmp_path / "skills" / "dir-name-only" + sk_dir.mkdir(parents=True) + (sk_dir / "SKILL.md").write_text("---\ndescription: x\n---\nbody") + skills = scan_local_skills(tmp_path) + assert [s["name"] for s in skills] == ["dir-name-only"] + + +def test_scan_truncates_long_description_to_1024(tmp_path: Path): + """The SDK ``ShellToolLocalSkill`` description field has a length + cap. Ours conservatively truncates at 1024 chars so any agent UI + that surfaces this string can't be overrun.""" + long_desc = "a" * 5000 + _write_skill(tmp_path / "skills", "verbose", description=long_desc) + skills = scan_local_skills(tmp_path) + assert len(skills) == 1 + assert len(skills[0]["description"]) == 1024 + + +def test_scan_first_hit_wins_across_roots(tmp_path: Path, monkeypatch): + """When the same skill name lives in multiple roots, the EARLIER + root wins. This is what lets a user override a built-in skill by + dropping a same-named SKILL.md into ``/skills/``.""" + # Point the home-global root at a tmp location (so the test doesn't + # rely on the real ~/.openkb/skills state) + home_root = tmp_path / "home-openkb-skills" + home_root.mkdir() + _write_skill(home_root, "dup", description="HOME version") + + kb_local = tmp_path / "skills" + _write_skill(kb_local, "dup", description="KB-LOCAL version") + + # extra_roots is APPENDED, so home_root (passed explicitly here) + # comes after the default kb-local "skills/" — kb-local wins. + skills = scan_local_skills(tmp_path, extra_roots=(str(home_root),)) + dup = next(s for s in skills if s["name"] == "dup") + assert dup["description"] == "KB-LOCAL version" + + +def test_scan_extra_roots_appended_after_defaults(tmp_path: Path): + """``extra_roots`` come AFTER the built-in defaults — they're a + courtesy for callers that want to scan additional locations, + not a way to override default order.""" + extra = tmp_path / "extra" + _write_skill(extra, "extra-only") + skills = scan_local_skills(tmp_path, extra_roots=(str(extra),)) + assert "extra-only" in {s["name"] for s in skills} + + +def test_scan_default_roots_listed(tmp_path: Path): + """The module's published constant matches expectations — pin it so + a refactor doesn't silently change which dirs are searched.""" + assert DEFAULT_SKILL_ROOTS == ( + "skills", + "~/.openkb/skills", + "~/.claude/skills", + ) + + +# ─── _parse_frontmatter ────────────────────────────────────────────────── + + +def test_parse_frontmatter_happy_path(): + text = "---\nname: foo\ndescription: bar\n---\nbody text" + meta, body = _parse_frontmatter(text) + assert meta == {"name": "foo", "description": "bar"} + assert body == "body text" + + +def test_parse_frontmatter_returns_empty_when_no_delim(): + """No leading ``---`` means no frontmatter; return ({}, full_text).""" + text = "just a body, no frontmatter" + meta, body = _parse_frontmatter(text) + assert meta == {} + assert body == text + + +def test_parse_frontmatter_returns_empty_when_unclosed(): + """Frontmatter that opens with ``---`` but never closes is malformed; + treat as no frontmatter rather than raising.""" + text = "---\nname: foo\nbut no closing" + meta, body = _parse_frontmatter(text) + assert meta == {} + assert body == text # everything passed through as body + + +def test_parse_frontmatter_handles_malformed_yaml(): + """Malformed YAML inside the delimiters shouldn't crash; return + ({}, body). Caller is responsible for asserting required keys.""" + text = "---\n: : :\nname: foo\n---\nbody" + meta, body = _parse_frontmatter(text) + assert meta == {} + assert body == "body" + + +def test_parse_frontmatter_non_dict_yaml_returns_empty(): + """If the frontmatter parses but isn't a dict (e.g. a YAML list), + treat as empty metadata rather than trying to use it.""" + text = "---\n- item-1\n- item-2\n---\nbody" + meta, body = _parse_frontmatter(text) + assert meta == {} + # body is correctly extracted even though metadata was discarded + assert body == "body" + + +def test_parse_frontmatter_preserves_body_with_dashes(): + """Body containing standalone ``---`` (a Markdown horizontal rule) is + preserved intact — only the FIRST closing ``---`` ends frontmatter.""" + text = "---\nname: foo\ndescription: bar\n---\nIntro\n\n---\n\nMore body" + meta, body = _parse_frontmatter(text) + assert meta == {"name": "foo", "description": "bar"} + assert body == "Intro\n\n---\n\nMore body"