diff --git a/core/config.py b/core/config.py index d4ecf49..e375a92 100644 --- a/core/config.py +++ b/core/config.py @@ -14,8 +14,17 @@ class AppConfig: providers: Dict[str, Any] default_provider: str +# The installed location of the harness (parent of core/), not the caller's +# CWD. `motion` can be pointed at any workspace directory, so config lookup +# must not depend on where it happens to be invoked from. +_REPO_DIR = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) + + class ConfigManager: - CONFIG_PATHS = ["config.yml", "config.example.yml"] + CONFIG_PATHS = [ + os.path.join(_REPO_DIR, "config.yml"), + os.path.join(_REPO_DIR, "config.example.yml"), + ] def __init__(self, config_path: str = ""): if config_path: diff --git a/core/learning.py b/core/learning.py index 377d75c..ecc5d20 100644 --- a/core/learning.py +++ b/core/learning.py @@ -1,10 +1,22 @@ +import hashlib import os import asyncio from typing import List, Dict, Any, Optional from dataclasses import dataclass from datetime import datetime from core.providers import ModelConfig, ProviderFactory -from memory.db import MemoryDB, MemoryChunk +from memory.db import MemoryDB, MemoryChunk, EMBEDDING_DIM + + +def _fallback_embedding(text: str, dim: int = EMBEDDING_DIM) -> List[float]: + """Deterministic, always-non-zero embedding for when no embedding + provider is available. Mirrors MotionAgent.get_embedding's fallback so + behavior is consistent across the codebase.""" + h = hashlib.sha256(text.encode()).digest() + raw = [float(b) / 255.0 for b in h] + vec = (raw * ((dim // len(raw)) + 1))[:dim] + norm = sum(v * v for v in vec) ** 0.5 or 1.0 + return [v / norm for v in vec] @dataclass class Trajectory: @@ -14,15 +26,26 @@ class Trajectory: final_result: str success: bool +# The installed location of the harness (parent of core/), not the caller's +# CWD - auto-synthesized skills accumulate here regardless of which project +# directory `motion` is currently pointed at. +_REPO_DIR = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) + + class SkillSynthesizer: """ The 'Crystallization' engine. Turns successful tool-call trajectories into reusable .md skills. """ - def __init__(self, model_config: ModelConfig, db: MemoryDB, skills_dir: str = "skills"): + def __init__(self, model_config: ModelConfig, db: MemoryDB, skills_dir: Optional[str] = None, embedding_provider=None): self.provider = ProviderFactory.get_provider(model_config) self.db = db - self.skills_dir = skills_dir + self.skills_dir = skills_dir or os.path.join(_REPO_DIR, "skills") + # Used to compute a real embedding for synthesized skills so they can + # actually participate in semantic recall. Expected to expose an + # async get_embedding(text) -> list[float] (e.g. a MotionAgent + # instance, which already has a safe hash-based fallback built in). + self.embedding_provider = embedding_provider if not os.path.exists(self.skills_dir): os.makedirs(self.skills_dir) @@ -60,10 +83,21 @@ async def synthesize(self, trajectory: Trajectory) -> Optional[str]: with open(file_path, "w", encoding="utf-8") as f: f.write(skill_content) - # Also index the skill in the MemoryDB for semantic recall + # Also index the skill in the MemoryDB for semantic recall. A + # zero-vector embedding is never valid here: cosine + # similarity/distance is undefined for a zero-norm vector, which + # crashes semantic search rather than just being unhelpful. + embedding = None + if self.embedding_provider is not None: + try: + embedding = await self.embedding_provider.get_embedding(skill_content) + except Exception: + embedding = None + if not embedding: + embedding = _fallback_embedding(skill_content) self.db.add_memory(MemoryChunk( content=skill_content, - embedding=[0.0] * 128, # Embedding would be generated by a real provider + embedding=embedding, metadata={"file": file_path, "type": "SKILL"}, mem_type="DOC" )) diff --git a/core/orchestrator.py b/core/orchestrator.py index 7b25744..c1226cb 100644 --- a/core/orchestrator.py +++ b/core/orchestrator.py @@ -10,7 +10,7 @@ import logging from core.providers import ModelConfig, ProviderFactory -from main import MotionAgent +from main import MotionAgent, REPO_DIR logger = logging.getLogger(__name__) @@ -189,7 +189,7 @@ async def _execute_task(self, request: TaskRequest, model_override: Optional[Mod try: config = model_override or self.default_config - agent = MotionAgent(config, memory_path=f"memory_{request.task_id}.db") + agent = MotionAgent(config, memory_path=os.path.join(REPO_DIR, f"memory_{request.task_id}.db")) # Record user turn task.conversation.append({"role": "user", "content": request.prompt}) @@ -208,6 +208,8 @@ async def on_stream_chunk(chunk: str) -> None: request.prompt, target="user", on_stream_chunk=on_stream_chunk, + workspace=self.workspace_path, + agent_mode="build", ) # Fallback for non-streaming providers @@ -233,7 +235,7 @@ async def on_stream_chunk(chunk: str) -> None: except Exception: pass try: - db_path = f"memory_{request.task_id}.db" + db_path = os.path.join(REPO_DIR, f"memory_{request.task_id}.db") if os.path.exists(db_path): os.remove(db_path) except Exception: diff --git a/core/workspace_tools.py b/core/workspace_tools.py new file mode 100644 index 0000000..bc1d18d --- /dev/null +++ b/core/workspace_tools.py @@ -0,0 +1,321 @@ +"""Workspace-scoped filesystem tools used by MotionAgent.""" + +from __future__ import annotations + +import json +import re +from pathlib import Path +from typing import Any + + +# `\s*` right after `<` (and after `<` before `/`) tolerates stray spaces +# models occasionally insert, e.g. "< motion_tool>" or "< /motion_tool>". +# Without this, such near-misses fall through every pattern below AND the +# malformed-tag leak guard in parse_tool_call(), so the raw tool-call JSON +# gets shown to the user as if it were the model's final answer. +TOOL_CALL_PATTERN = re.compile( + r"<\s*motion_tool\b[^>]*>\s*(\{.*?\})\s*<\s*/\s*motion_tool\b[^>]*>", + re.DOTALL, +) +MOTION_ENVELOPE_PATTERN = re.compile( + r"<\s*motion_[^>]*>\s*(?P.*?)\s*<\s*/\s*motion_tool\b[^>]*>", + re.DOTALL | re.IGNORECASE, +) +DIRECT_TOOL_PATTERN = re.compile( + r"<\s*(?Plist_files|read_file|write_file|replace_in_file)\b[^>]*>" + r"\s*(?P\{.*?\})\s*<\s*/\s*(?P=name)\b[^>]*>", + re.DOTALL, +) +# Some providers (observed on Ollama Cloud / deepseek-v4-flash) leak their own +# internal tool-channel markup verbatim instead of using , e.g. +# "<| DSML | tool:write_file>{...}". Recognize this shape +# directly so it executes instead of leaking as the final answer. +DSML_TOOL_PATTERN = re.compile( + r"<\s*\|\s*DSML\s*\|\s*tool\s*:\s*(?P[a-zA-Z_]+)\s*>" + r"\s*(?P\{.*?\})\s*<\s*/\s*\|\s*DSML\s*\|\s*tool\s*>", + re.DOTALL | re.IGNORECASE, +) +TOOL_MARKERS = ( + "motion_tool", + "list_files", + "read_file", + "write_file", + "replace_in_file", +) + + +class WorkspaceToolError(ValueError): + """Raised when a tool request is invalid or escapes the workspace.""" + + +class WorkspaceTools: + """Small, deterministic filesystem toolset restricted to one workspace.""" + + def __init__(self, workspace: str | Path, read_only: bool = False) -> None: + self.root = Path(workspace).expanduser().resolve() + self.read_only = read_only + + @property + def instructions(self) -> str: + mode = "READ-ONLY plan mode" if self.read_only else "BUILD mode with write access" + write_tools = "" if self.read_only else """ +- write_file: {"path": "relative/path", "content": "complete file contents"} +- replace_in_file: {"path": "relative/path", "old": "exact text", "new": "replacement text"}""" + if self.read_only: + # Plan mode must never be told to write files - write_file/replace_in_file + # are unavailable and calling them always fails. Instead of leaving the + # model to punt back to the user ("say exactly what to build"), instruct + # it to produce a concrete plan as its final text answer. + goal_block = """ +CRITICAL: You are in Plan mode. write_file and replace_in_file are DISABLED here - do +not attempt them. When the user describes something to build, do not ask them to repeat +or restate it. Instead, explore the workspace only as needed (list_files/read_file), +then respond with a concrete, structured PLAN as your final plain-text answer: a +proposed directory/file layout, the approach for each major piece, key libraries or +APIs to use, and any open questions. This plan is what the user will review and then +ask you to implement after switching you to Build mode. +""".strip() + else: + goal_block = """ +CRITICAL: When the user asks you to create or generate something (a project, a script, +a scraper, a component, etc.), you MUST actually write files to the workspace using +write_file. If an earlier message in this conversation already proposed a plan (from +Plan mode), follow it instead of re-deriving one. Listing files or describing the plan +again is not enough. Produce concrete, complete files with sensible relative paths and +then summarize what you created. +""".strip() + return f""" +You are an agent running on the user's machine in {mode}. +Workspace root: {self.root} + +You have real filesystem tools. When the user asks you to create, modify, or inspect +project files, use these tools directly. Never say you cannot access the filesystem, +and never give the user a shell script merely to create files you can create yourself. + +Available tools: +- list_files: {{"path": ".", "pattern": "*.py"}} +- read_file: {{"path": "relative/path"}}{write_tools} + +To call a tool, respond with exactly one call and no surrounding prose: +{{"name":"read_file","arguments":{{"path":"README.md"}}}} + +After each call you will receive a message. Continue calling tools +until the requested work is complete, then give a concise final summary. Use relative +paths. Do not invent tool results. Do not place tool calls in Markdown fences. + +{goal_block} +""".strip() + + def _resolve(self, raw_path: str) -> Path: + if not isinstance(raw_path, str) or not raw_path.strip(): + raise WorkspaceToolError("path must be a non-empty string") + candidate = (self.root / raw_path).resolve() + try: + candidate.relative_to(self.root) + except ValueError as exc: + raise WorkspaceToolError("path escapes the workspace") from exc + return candidate + + def execute(self, name: str, arguments: dict[str, Any]) -> dict[str, Any]: + if not isinstance(arguments, dict): + raise WorkspaceToolError("arguments must be an object") + if name == "list_files": + return self._list_files(arguments) + if name == "read_file": + return self._read_file(arguments) + if name == "write_file": + self._require_write_access() + return self._write_file(arguments) + if name == "replace_in_file": + self._require_write_access() + return self._replace_in_file(arguments) + raise WorkspaceToolError(f"unknown tool: {name}") + + def _require_write_access(self) -> None: + if self.read_only: + raise WorkspaceToolError("write tools are disabled in plan mode") + + def _list_files(self, arguments: dict[str, Any]) -> dict[str, Any]: + directory = self._resolve(arguments.get("path", ".")) + pattern = arguments.get("pattern", "*") + if not directory.exists(): + raise WorkspaceToolError(f"path does not exist: {arguments.get('path', '.')}") + if not directory.is_dir(): + raise WorkspaceToolError("list_files path must be a directory") + files = [ + str(path.relative_to(self.root)) + for path in directory.rglob(pattern) + if path.is_file() + ] + return {"files": files[:500], "truncated": len(files) > 500} + + def _read_file(self, arguments: dict[str, Any]) -> dict[str, Any]: + path = self._resolve(arguments.get("path", "")) + if not path.is_file(): + raise WorkspaceToolError(f"file does not exist: {arguments.get('path', '')}") + content = path.read_text(encoding="utf-8", errors="replace") + limit = 200_000 + return { + "path": str(path.relative_to(self.root)), + "content": content[:limit], + "truncated": len(content) > limit, + } + + def _write_file(self, arguments: dict[str, Any]) -> dict[str, Any]: + path = self._resolve(arguments.get("path", "")) + content = arguments.get("content") + if not isinstance(content, str): + raise WorkspaceToolError("content must be a string") + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(content, encoding="utf-8") + return { + "path": str(path.relative_to(self.root)), + "bytes_written": len(content.encode("utf-8")), + } + + def _replace_in_file(self, arguments: dict[str, Any]) -> dict[str, Any]: + path = self._resolve(arguments.get("path", "")) + old = arguments.get("old") + new = arguments.get("new") + if not isinstance(old, str) or not old: + raise WorkspaceToolError("old must be a non-empty string") + if not isinstance(new, str): + raise WorkspaceToolError("new must be a string") + if not path.is_file(): + raise WorkspaceToolError(f"file does not exist: {arguments.get('path', '')}") + content = path.read_text(encoding="utf-8") + occurrences = content.count(old) + if occurrences != 1: + raise WorkspaceToolError( + f"old text must occur exactly once; found {occurrences} occurrences" + ) + path.write_text(content.replace(old, new, 1), encoding="utf-8") + return {"path": str(path.relative_to(self.root)), "replacements": 1} + + +def parse_tool_call(text: str) -> tuple[str, dict[str, Any]] | None: + """Parse one Motion tool call from a model response.""" + raw = text or "" + match = TOOL_CALL_PATTERN.search(raw) + if not match: + envelope = MOTION_ENVELOPE_PATTERN.search(raw) + if envelope: + return _parse_motion_payload(envelope.group("payload")) + if not match: + direct_match = DIRECT_TOOL_PATTERN.search(raw) + if direct_match: + arguments = json.loads(direct_match.group("arguments")) + if not isinstance(arguments, dict): + raise WorkspaceToolError("tool arguments must be an object") + return direct_match.group("name"), arguments + if not match: + dsml_match = DSML_TOOL_PATTERN.search(raw) + if dsml_match: + arguments = json.loads(dsml_match.group("arguments")) + if not isinstance(arguments, dict): + raise WorkspaceToolError("tool arguments must be an object") + return dsml_match.group("name"), arguments + if not match: + # Never leak a malformed tool request into the user-facing response. + # Models occasionally produce variants such as or + # provider-style direct tags such as , or insert a stray + # space right after "<" (e.g. "< motion_tool>"). `\s*` after `<` + # catches that last case so it's treated as a recoverable parse + # error (which prompts the model to retry) instead of leaking the + # raw tag/JSON to the user as if it were the final answer. + lowered = raw.lower() + looks_like_leaked_tool_call = ( + re.search(r"<\s*motion_", lowered) + or any( + re.search(rf"<\s*{re.escape(marker)}\b", lowered) for marker in TOOL_MARKERS + ) + or "dsml" in lowered + # Generic last-resort net: whatever exotic envelope name a model + # invents next, a tag wrapping something shaped like our tool + # JSON (write_file's path+content, or a name/arguments envelope) + # is never a legitimate final answer. + or ( + "<" in raw + and ( + '"arguments"' in raw + or ('"path"' in raw and '"content"' in raw) + ) + ) + ) + if looks_like_leaked_tool_call: + raise WorkspaceToolError("malformed filesystem tool envelope") + return None + return _parse_motion_payload(match.group(1)) + + +def _parse_motion_payload(payload_text: str) -> tuple[str, dict[str, Any]]: + """Parse JSON, with a narrow recovery path for malformed write_file content. + + Some models emit a valid outer shape but fail to JSON-escape source-code + quotes inside the content value. For write_file only, recover the path and + treat everything after the content delimiter as raw source text. + """ + try: + payload = json.loads(payload_text) + except json.JSONDecodeError: + return _recover_write_file_payload(payload_text) + + name = payload.get("name") + arguments = payload.get("arguments", {}) + if not isinstance(name, str): + raise WorkspaceToolError("tool name must be a string") + if not isinstance(arguments, dict): + raise WorkspaceToolError("tool arguments must be an object") + return name, arguments + + +def _recover_write_file_payload(payload_text: str) -> tuple[str, dict[str, Any]]: + """Recover the specific malformed JSON shape produced for source files.""" + name_match = re.search(r'"name"\s*:\s*"([^"]+)"', payload_text) + if not name_match or name_match.group(1) != "write_file": + raise WorkspaceToolError("malformed motion tool JSON") + + path_match = re.search( + r'"path"\s*:\s*"((?:\\.|[^"\\])*)"', + payload_text, + re.DOTALL, + ) + content_match = re.search(r'"content"\s*:\s*', payload_text) + if not path_match or not content_match: + raise WorkspaceToolError("malformed write_file arguments") + + try: + path = json.loads(f'"{path_match.group(1)}"') + except json.JSONDecodeError as exc: + raise WorkspaceToolError("malformed write_file path") from exc + + content = payload_text[content_match.end():].strip() + # Remove outer object terminators, then the JSON delimiter quote. Internal + # source-code quotes are intentionally preserved as raw file content. + content = re.sub(r"\s*}\s*}\s*$", "", content, count=1).strip() + if content.startswith('"'): + content = content[1:] + if content.endswith('"'): + content = content[:-1] + content = ( + content.replace("\\r\\n", "\n") + .replace("\\n", "\n") + .replace("\\r", "\n") + .replace("\\t", "\t") + .replace('\\"', '"') + .replace("\\\\", "\\") + ) + return "write_file", {"path": path, "content": content} + + +def format_tool_result( + name: str, + result: dict[str, Any] | None = None, + error: str | None = None, +) -> str: + payload = {"name": name, "ok": error is None} + if error is None: + payload["result"] = result or {} + else: + payload["error"] = error + return f"{json.dumps(payload)}" diff --git a/docs/architecture.md b/docs/architecture.md index a464318..b5f6f2a 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -28,4 +28,6 @@ To combat token bloat in long agent-to-agent trajectories, the harness applies a - **Symmetry**: The compression is reversible, meaning the TUI can "decompress" the output back into natural language for the user, while the internal agents communicate in a dense, token-efficient format. ### 4. Parallel Orchestration -The orchestrator manages task concurrency using an `asyncio` semaphore gated by the system's CPU core count. This prevents system lockup during massive parallel research tasks while maximizing throughput. \ No newline at end of file +The orchestrator (`core/orchestrator.py`) manages task concurrency using an `asyncio` semaphore gated by the system's CPU core count. This prevents system lockup during massive parallel research tasks while maximizing throughput. + +> **Status:** the `TaskManager` is instantiated at startup but not yet exposed through the TUI — there is no UI to spawn parallel tasks yet. This is on the [Roadmap](roadmap.md). \ No newline at end of file diff --git a/docs/cli.md b/docs/cli.md new file mode 100644 index 0000000..94330ec --- /dev/null +++ b/docs/cli.md @@ -0,0 +1,71 @@ +# CLI & Auth + +Motion Harness ships a small CLI (`motion`) plus an opencode-style auth store +for managing provider API keys. + +## The `motion` command + +| Command | Action | +| :-- | :-- | +| `motion` | Launch the TUI (default) | +| `motion --chat` | Launch the legacy REPL chat | +| `motion --list` | List available providers/models | +| `motion --provider ` | Launch with a specific provider/model | +| `motion --test` | Run the Caveman compression test (no model needed) | +| `motion auth login ` | Store an API key (prompts, hidden input) | +| `motion auth logout ` | Remove a stored API key | +| `motion auth list` | List which providers have keys | + +Provider ids: `ollama-cloud`, `claude`, `openai`, `local-llama`, or any provider +you add to `config.yml`. Use `provider/model` to pick a specific model, e.g. +`motion --provider ollama-cloud/glm-5.2`. + +## Managing API keys + +Keys are stored in `~/.config/motion-harness/auth.json` with `0600` permissions +— never in `config.yml` or the shell environment. + +```bash +motion auth login ollama-cloud # prompts for your key, stored locally +motion auth login openai +motion auth login claude +motion auth list # see which providers have keys (masked) +motion auth logout ollama-cloud # remove a key +``` + +### Lookup order + +When resolving a key for a provider, the harness checks, in order: + +1. **Auth store** — `~/.config/motion-harness/auth.json` +2. **Environment variable** — e.g. `OLLAMA_API_KEY`, `OPENAI_API_KEY`, + `ANTHROPIC_API_KEY` +3. **`config.yml`** — the `api_key` field + +### In the TUI + +- Press `Ctrl+K` → **Manage API keys** to set a key from the command palette. +- Or type `/auth list`, `/auth login `, `/auth logout ` in + the chat composer. + +## Adding a provider + +Add a block under `providers:` in `config.yml`: + +```yaml +providers: + my-provider: + name: "My Provider" + endpoint: "https://api.myprovider.com/v1" + api_key: null # set via `motion auth login my-provider` + provider_type: "cloud" # "cloud" | "local" | "proxy" + default_model: "my-model-1" + models: + my-model-1: + temperature: 0.7 + max_tokens: 4096 +``` + +Then `motion auth login my-provider`. The built-in catalog (`core/catalog.py`) +is merged with your `config.yml`, so your provider and models are picked up +automatically. diff --git a/docs/roadmap.md b/docs/roadmap.md index 1b52f0b..eaa5687 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -29,6 +29,11 @@ operate. - [x] Remove dead code — unused Tasks/Skills/KB/Memory/Settings panes and the hardcoded dashboard admin key. +> **Note:** `TaskManager` (parallel orchestration) and `SkillSynthesizer` +> (auto-crystallization) exist in `core/` but are **not yet exposed in the TUI** — +> there's no UI to spawn parallel tasks or toggle auto-synthesis. Manual skills +> work via `/skill save `. Parallel orchestration is tracked in Phase 4. + ## Phase 2 — Document & File Ingestion (next) Adopts the opencode model of **base64 data-URL content parts** with **capability diff --git a/install.sh b/install.sh index 738fb9a..8a4bcae 100755 --- a/install.sh +++ b/install.sh @@ -55,10 +55,15 @@ mkdir -p "$REPO_DIR/bin" cat > "$WRAPPER" << WRAPPER_EOF #!/bin/bash # Motion Harness launcher — created by install.sh +# +# Intentionally does NOT cd into the repo: the harness's own state +# (config.yml, .env, memory DB, synthesized skills) is resolved relative to +# REPO_DIR by the Python code itself, while the *workspace* the agent reads +# and writes files in is whatever directory you run \`motion\` from. Point +# it at any project by cd-ing there first, just like any other CLI tool. REPO_DIR="$REPO_DIR" VENV_DIR="$REPO_DIR/.venv" export PYTHONPATH="\$REPO_DIR" -cd "\$REPO_DIR" exec "\$VENV_DIR/bin/python" "\$REPO_DIR/main.py" "\$@" WRAPPER_EOF diff --git a/main.py b/main.py index 882adee..ff89783 100644 --- a/main.py +++ b/main.py @@ -5,23 +5,46 @@ from core import auth from memory.db import MemoryDB, EMBEDDING_DIM from memory.retriever import HybridRetriever +from core.workspace_tools import ( + WorkspaceTools, + format_tool_result, + parse_tool_call, +) import asyncio import inspect import hashlib import logging import os +import re from typing import Optional logging.basicConfig(level=logging.INFO, format="%(asctime)s [%(levelname)s] %(name)s: %(message)s") logger = logging.getLogger(__name__) +# The installed location of the harness itself. Persistent harness state +# (memory DB, config.yml, .env, auto-synthesized skills) lives here so it +# stays put no matter which directory `motion` is invoked/pointed at - only +# the *workspace* (files the agent reads/writes) should follow the caller's +# current directory. See core/config.py and core/learning.py for the same +# pattern applied to config and skill-synthesis paths. +REPO_DIR = os.path.dirname(os.path.abspath(__file__)) + +# Per-turn cap on the tool-call agent loop (list/read/write/replace calls). +# This is a last-resort safety valve against a truly stuck/looping model, not +# a task-size limit - large multi-file builds are expected to run for many +# steps. Users can watch progress live (each tool op is streamed) and cancel +# at any time, or queue a follow-up message, so this is set high rather than +# tight. If it's ever hit, whatever progress was made is still reported (see +# the tool loop's `else` branch below) instead of being silently discarded. +MAX_TOOL_STEPS = 150 + class MotionAgent: - def __init__(self, model_config: ModelConfig, memory_path: str = "motion_memory.db", auto_skill_synthesis: bool = False): + def __init__(self, model_config: ModelConfig, memory_path: Optional[str] = None, auto_skill_synthesis: bool = False): self.provider = ProviderFactory.get_provider(model_config) - self.memory = MemoryDB(memory_path) + self.memory = MemoryDB(memory_path or os.path.join(REPO_DIR, "motion_memory.db")) self.retriever = HybridRetriever(self.memory, self) self.caveman = CavemanProtocol(enabled=True) - self.synthesizer = SkillSynthesizer(model_config, self.memory) + self.synthesizer = SkillSynthesizer(model_config, self.memory, embedding_provider=self) self.auto_skill_synthesis = auto_skill_synthesis async def get_embedding(self, text: str): @@ -43,7 +66,17 @@ async def get_embedding(self, text: str): norm = sum(v * v for v in vec) ** 0.5 or 1.0 return [v / norm for v in vec] - async def run(self, prompt: str, target: str = "user", on_stream_chunk=None, on_trace_event=None, history: Optional[list] = None, context_query: Optional[str] = None): + async def run( + self, + prompt: str, + target: str = "user", + on_stream_chunk=None, + on_trace_event=None, + history: Optional[list] = None, + context_query: Optional[str] = None, + workspace: Optional[str] = None, + agent_mode: str = "build", + ): async def emit_trace(stage: str, message: str, **extra): if not on_trace_event: return @@ -63,22 +96,233 @@ async def emit_trace(stage: str, message: str, **extra): # Augment recall with the session context query so we don't repeat ourselves. if context_query: context_chunks += await self.retriever.retrieve(context_query) - # De-duplicate by content, keep order. - seen = set() - deduped = [] - for c in context_chunks: - key = c["content"] - if key not in seen: - seen.add(key) - deduped.append(c) - context_chunks = deduped[:5] + # De-duplicate by content, keep order. Applied unconditionally (not just + # when context_query is set) since a single retrieve() call can itself + # surface duplicate content if the DB has duplicate rows. + seen = set() + deduped = [] + for c in context_chunks: + key = c["content"] + if key not in seen: + seen.add(key) + deduped.append(c) + context_chunks = deduped[:5] await emit_trace("memory_recall_done", "Memory recall complete", chunks=len(context_chunks)) context_text = "\n".join([c["content"] for c in context_chunks]) # 2. Construct System Prompt - system_prompt = f"You are Motion Agent. Memory Context:\n{context_text}" + tools = WorkspaceTools( + workspace or os.getcwd(), + read_only=agent_mode == "plan", + ) + system_prompt = ( + f"You are Motion Agent.\n\n{tools.instructions}\n\n" + f"Memory Context:\n{context_text}" + ) + + # Tool calls require a short agent loop. The XML envelope works across + # Ollama, OpenAI-compatible, Anthropic, and proxy providers without + # requiring each provider to implement a different native tool API. + tool_history = list(history or []) + tool_response = None + used_tool = False + empty_retries = 0 + inspection_only_loops = 0 + tool_operations: list[str] = [] + for tool_step in range(MAX_TOOL_STEPS): + step_prompt = prompt if tool_step == 0 else "Continue the task using the tool result above." + candidate = await self.provider.complete( + step_prompt, + system_prompt=system_prompt, + history=tool_history or None, + ) + + # Stream visible progress for the current step so the UI doesn't stay + # blank while tools are running. Strip any tool markup from previews. + visible = re.sub(r"<[^>]+>", "", candidate or "").strip() + if on_stream_chunk and visible: + maybe = on_stream_chunk(f"_step_ {visible[:500]}") + if inspect.isawaitable(maybe): + await maybe + + try: + tool_call = parse_tool_call(candidate) + except Exception as exc: + await emit_trace("tool_error", "Invalid tool call", error=str(exc)) + tool_response = ( + "Your tool call was invalid. Correct it and call one tool using " + "the exact JSON format." + ) + tool_history.extend([ + {"role": "assistant", "content": candidate}, + {"role": "user", "content": format_tool_result("invalid", error=str(exc))}, + ]) + continue + + if tool_call is None and not (candidate or "").strip() and used_tool: + empty_retries += 1 + if empty_retries <= 2: + tool_history.append({ + "role": "user", + "content": ( + "Your previous response was empty. Continue the task: use another " + "tool if work remains, otherwise provide a concise completion summary." + ), + }) + continue + break + + if tool_call is None: + tool_response = candidate + break + + name, arguments = tool_call + used_tool = True + + # Track loops that only inspect. If the model keeps listing/reading + # without writing on a build-mode task, nudge it to create files. + if name in {"list_files", "read_file"}: + inspection_only_loops += 1 + else: + inspection_only_loops = 0 + + await emit_trace( + "tool_start", + f"Running {name}", + tool=name, + path=str(arguments.get("path", "")), + ) + try: + result = tools.execute(name, arguments) + result_message = format_tool_result(name, result=result) + path = str(result.get("path") or arguments.get("path") or "").strip() + if name == "write_file": + operation = f"wrote `{path}`" + stream_text = f"wrote `{path}` ({result.get('bytes_written', 0)} bytes)" + elif name == "replace_in_file": + operation = f"updated `{path}`" + stream_text = operation + elif name == "read_file": + operation = f"read `{path}`" + stream_text = operation + elif name == "list_files": + operation = f"listed `{path or '.'}`" + stream_text = operation + else: + operation = f"ran `{name}`" + stream_text = operation + tool_operations.append(operation) + # Stream every tool op (not just writes) so the UI can show + # live step-by-step progress for the whole loop, however long + # it runs. + if on_stream_chunk: + maybe = on_stream_chunk(f"_tool_ {stream_text}") + if inspect.isawaitable(maybe): + await maybe + await emit_trace("tool_done", f"Completed {name}", tool=name) + except Exception as exc: + operation = f"`{name}` failed: {exc}" + result_message = format_tool_result(name, error=str(exc)) + tool_operations.append(operation) + if on_stream_chunk: + maybe = on_stream_chunk(f"_tool_ {operation}") + if inspect.isawaitable(maybe): + await maybe + await emit_trace("tool_error", f"{name} failed", tool=name, error=str(exc)) + tool_history.extend([ + {"role": "assistant", "content": candidate}, + {"role": "user", "content": result_message}, + ]) + + if agent_mode == "plan" and name in {"write_file", "replace_in_file"}: + # Plan mode always rejects writes (see WorkspaceTools._require_write_access). + # Steer the model away from retrying the same blocked call and toward + # actually answering with a concrete plan. + tool_history.append({ + "role": "user", + "content": ( + "You are in read-only Plan mode - file writes are disabled here. " + "Do not retry write_file/replace_in_file. Respond now with a concrete " + "written plan: proposed files/directories, the approach for each major " + "piece, and any libraries you'd use. The user will review this and switch " + "you to Build mode to implement it." + ), + }) + + if ( + agent_mode == "build" + and inspection_only_loops >= 2 + and not any(op.startswith(("wrote ", "updated ")) for op in tool_operations) + ): + tool_history.append({ + "role": "user", + "content": ( + "You have inspected the workspace enough. The user asked you to create " + "something. Now use write_file to create the requested files with concrete, " + "complete content. Do not ask for clarification and do not return a script." + ), + }) + else: + # Never discard real progress: if tools actually ran before the cap + # was hit, tell the user what was done and how to resume, instead + # of a bare "narrow the task" message that hides completed writes. + # Hitting this at all is unusual given how high the ceiling is - + # it almost always means the model is stuck looping rather than + # that the task was too big. + if tool_operations: + completed = "\n".join(f"- {operation}" for operation in tool_operations) + tool_response = ( + f"Hit the internal safety limit ({MAX_TOOL_STEPS} tool calls) before " + f"finishing - this usually means something got stuck. Progress so far:\n" + f"{completed}\n\nSay \"continue\" and I'll pick up from here." + ) + else: + tool_response = ( + f"Hit the internal safety limit ({MAX_TOOL_STEPS} tool calls) without " + "making any progress. Please narrow the task and try again." + ) + + # The final non-tool response is already complete. Tool markup is never + # streamed into the chat UI. + raw_response = (tool_response or "").strip() + if not raw_response and used_tool: + write_operations = [ + operation + for operation in tool_operations + if operation.startswith(("wrote ", "updated ")) + ] + if write_operations: + raw_response = "Completed filesystem changes:\n" + "\n".join( + f"- {operation}" for operation in write_operations + ) + elif agent_mode == "plan": + raw_response = ( + "I inspected the workspace but couldn't finish a plan in the space " + "available. Ask me again, or narrow the scope, and I'll lay out the " + "file/directory approach here in Plan mode before you switch to Build." + ) + else: + raw_response = ( + "I inspected the workspace but did not make any filesystem changes. " + "If you want me to create files, say exactly what to build and I will " + "use write_file to create it." + ) + + # Ensure the user always sees the final text, whether streaming or not. + if on_stream_chunk: + if raw_response: + maybe = on_stream_chunk(raw_response) + if inspect.isawaitable(maybe): + await maybe + await emit_trace( + "model_done", + "Agent tool loop finished" if used_tool else "Completion finished", + chars=len(raw_response), + ) # 3. Model Completion (streaming if callback is provided) + # If the model already returned a natural-language answer in the tool + # loop, use that. Otherwise fall back to a streaming/oneshot completion. stream_chunk_count = 0 provider_type = getattr(getattr(self.provider, "config", None), "provider_type", "unknown") await emit_trace( @@ -87,23 +331,24 @@ async def emit_trace(stage: str, message: str, **extra): mode="stream" if on_stream_chunk else "oneshot", provider=provider_type, ) - if on_stream_chunk: - raw_chunks = [] - async for chunk in self.provider.stream_complete(prompt, system_prompt=system_prompt, history=history): - stream_chunk_count += 1 - raw_chunks.append(chunk) - await emit_trace("stream_chunk", "Received stream chunk", chunk_index=stream_chunk_count, chars=len(chunk or "")) - try: - maybe = on_stream_chunk(chunk) - if inspect.isawaitable(maybe): - await maybe - except Exception: - pass - raw_response = "".join(raw_chunks) - await emit_trace("model_done", "Streaming completion finished", stream_chunks=stream_chunk_count, chars=len(raw_response)) - else: - raw_response = await self.provider.complete(prompt, system_prompt=system_prompt, history=history) - await emit_trace("model_done", "One-shot completion finished", chars=len(raw_response or "")) + if not raw_response: + if on_stream_chunk: + raw_chunks = [] + async for chunk in self.provider.stream_complete(prompt, system_prompt=system_prompt, history=history): + stream_chunk_count += 1 + raw_chunks.append(chunk) + await emit_trace("stream_chunk", "Received stream chunk", chunk_index=stream_chunk_count, chars=len(chunk or "")) + try: + maybe = on_stream_chunk(chunk) + if inspect.isawaitable(maybe): + await maybe + except Exception: + pass + raw_response = "".join(raw_chunks) + await emit_trace("model_done", "Streaming completion finished", stream_chunks=stream_chunk_count, chars=len(raw_response)) + else: + raw_response = await self.provider.complete(prompt, system_prompt=system_prompt, history=history) + await emit_trace("model_done", "One-shot completion finished", chars=len(raw_response or "")) # 4. Caveman Compression final_response = self.caveman.process_outgoing(raw_response, target=target) @@ -133,8 +378,9 @@ async def emit_trace(stage: str, message: str, **extra): def load_agent_from_config(config_path: str = "", provider_id: str | None = None) -> MotionAgent: """Load a MotionAgent using settings from config.yml (or config.example.yml) and .env.""" - # Load .env file if present - config_dir = os.path.dirname(os.path.abspath(config_path)) if config_path else "." + # Load .env file if present. Anchored to REPO_DIR (not CWD) so `motion` + # finds its own .env regardless of which directory it's invoked from. + config_dir = os.path.dirname(os.path.abspath(config_path)) if config_path else REPO_DIR env_path = os.path.join(config_dir, ".env") if os.path.exists(env_path): with open(env_path) as f: @@ -293,7 +539,7 @@ def cmd_auth(args) -> None: parser = argparse.ArgumentParser(description="Motion Agent") parser.add_argument("--test", action="store_true", help="Run Caveman compression test (no model needed)") parser.add_argument("--list", action="store_true", help="List available providers") - parser.add_argument("--provider", type=str, default=None, help="Provider to use (e.g. ollama-cloud, ollama-cloud/gemma4:31b, claude-3-5)") + parser.add_argument("--provider", type=str, default=None, help="Provider to use (e.g. ollama-cloud, ollama-cloud/gemma4:31b, claude, openai)") parser.add_argument("--chat", action="store_true", help="Launch in chat REPL mode instead of TUI") sub = parser.add_subparsers(dest="command") auth_parser = sub.add_parser("auth", help="Manage provider API keys") diff --git a/memory/db.py b/memory/db.py index 0c30bef..2af62d3 100644 --- a/memory/db.py +++ b/memory/db.py @@ -41,9 +41,28 @@ def _init_db(self): )""") if self._vec_available: dim = EMBEDDING_DIM + # Use cosine distance explicitly (vec0 defaults to L2, which is + # magnitude-sensitive and was previously being misread as a + # higher-is-better similarity score by HybridRetriever, silently + # ranking the LEAST similar memories first). Cosine distance + # matches the brute-force fallback's cosine similarity exactly. + existing_sql = self.conn.execute( + "SELECT sql FROM sqlite_master WHERE name = 'memories_vec'" + ).fetchone() + needs_migration = existing_sql is not None and "distance_metric=cosine" not in (existing_sql[0] or "") + if needs_migration: + self.conn.execute("DROP TABLE memories_vec") self.conn.execute( - f"CREATE VIRTUAL TABLE IF NOT EXISTS memories_vec USING vec0(embedding float[{dim}])" + f"CREATE VIRTUAL TABLE IF NOT EXISTS memories_vec USING vec0(embedding float[{dim}] distance_metric=cosine)" ) + if needs_migration: + # Re-populate the rebuilt index from the durable `memories` + # table so existing data survives the metric migration. + for mem_id, emb_blob in self.conn.execute("SELECT id, embedding FROM memories").fetchall(): + self.conn.execute( + "INSERT INTO memories_vec (rowid, embedding) VALUES (?, ?)", + (mem_id, emb_blob), + ) self.conn.commit() def _serialize_embedding(self, embedding: List[float]) -> bytes: @@ -88,7 +107,19 @@ def semantic_search(self, query_embedding: List[float], limit: int = 5) -> List[ ORDER BY v.distance""", (query_blob, limit), ).fetchall() - return [(row[1], row[0]) for row in rows] + # The vec0 table is created with distance_metric=cosine (see + # _init_db), so distance == 1 - cosine_similarity. Convert back + # to cosine similarity so callers (HybridRetriever) can treat + # "score" as higher-is-better identically across this path and + # the brute-force cosine-similarity fallback below. Without this, + # callers that sort descending by score would rank the LEAST + # similar memories first whenever this vector-index path is used. + # + # Cosine distance is undefined (NULL) for zero-norm embeddings + # (e.g. a stored placeholder embedding). Skip those rather than + # crashing on `1.0 - None` - they simply can't participate in + # semantic search, but remain findable via keyword search. + return [(1.0 - row[1], row[0]) for row in rows if row[1] is not None] # Fallback: brute-force cosine similarity using numpy cursor = self.conn.execute("SELECT content, embedding FROM memories") @@ -101,7 +132,13 @@ def semantic_search(self, query_embedding: List[float], limit: int = 5) -> List[ emb = np.frombuffer(emb_blob, dtype=" List[Dict[str, Any]]: # 1. Generate query embedding @@ -24,8 +38,24 @@ async def retrieve(self, query: str, top_k: int = 5) -> List[Dict[str, Any]]: for score, content in keyword_results: # FTS5 rank is lower = better, so we invert it for merging merged.append({"score": -score, "content": content, "type": "keyword"}) - + + # Drop weakly-related semantic matches (see DEFAULT_MIN_SEMANTIC_SCORE). + merged = [ + item for item in merged + if item["type"] == "keyword" or item["score"] >= self.min_semantic_score + ] + + # De-duplicate identical content - the same memory can surface from + # both search types, and duplicate rows can exist in the DB. + seen = set() + deduped = [] + for item in merged: + if item["content"] in seen: + continue + seen.add(item["content"]) + deduped.append(item) + # Sort by score descending - merged.sort(key=lambda x: x["score"], reverse=True) + deduped.sort(key=lambda x: x["score"], reverse=True) - return merged[:top_k] + return deduped[:top_k] diff --git a/tests/test_composer.py b/tests/test_composer.py index 9cd3546..e18c901 100644 --- a/tests/test_composer.py +++ b/tests/test_composer.py @@ -7,6 +7,7 @@ """ import pytest from rich.style import Style +from textual import events from textual.app import App, ComposeResult from textual.message import Message @@ -73,6 +74,20 @@ async def test_up_down_navigates_history(): await pilot.press("down") assert composer.value == "" +@pytest.mark.asyncio +async def test_paste_inserts_multiline_text_at_cursor(): + app = _ComposerHarness() + async with app.run_test() as pilot: + composer = app.query_one("#composer", ChatComposer) + composer.focus() + composer.value = "before after" + composer.cursor_position = len("before ") + composer.post_message(events.Paste("line 1\r\nline 2")) + await pilot.pause() + + assert composer.value == "before line 1\nline 2 after" + assert composer.cursor_position == len("before line 1\nline 2") + def test_prompt_history_records_and_navigates(): state = AppState() diff --git a/tests/test_integration.py b/tests/test_integration.py index 19a0765..8b4d6d5 100644 --- a/tests/test_integration.py +++ b/tests/test_integration.py @@ -38,25 +38,32 @@ async def test_golden_path(): User Prompt -> Hybrid Recall -> Model completion -> Caveman Compression """ model_config = ModelConfig(name="test-model", endpoint="http://localhost", provider_type="local") - agent = MotionAgent(model_config) + # IMPORTANT: use an isolated in-memory DB. Omitting memory_path defaults to + # the real "motion_memory.db" used by the TUI, which previously caused this + # test to inject a fake "null pointer" memory into production data on every + # test run (found repeated 134x, polluting unrelated conversations). + agent = MotionAgent(model_config, memory_path=":memory:") agent.provider = MockProvider() - # Add a memory to test recall - agent.memory.add_memory(MemoryChunk( - content="The bug in line 42 is caused by a null pointer in the handler.", - embedding=[0.1] * EMBEDDING_DIM, - metadata={"source": "test.md"}, - mem_type="DOC" - )) - - # 2. Execution - prompt = "Where is the bug?" - response = await agent.run(prompt, target="agent") - - # 3. Assertions - # Caveman should strip fluff from the MockProvider's response - assert "Certainly!" not in response - assert "bug is in line 42" in response + try: + # Add a memory to test recall + agent.memory.add_memory(MemoryChunk( + content="The bug in line 42 is caused by a null pointer in the handler.", + embedding=[0.1] * EMBEDDING_DIM, + metadata={"source": "test.md"}, + mem_type="DOC" + )) + + # 2. Execution + prompt = "Where is the bug?" + response = await agent.run(prompt, target="agent") + + # 3. Assertions + # Caveman should strip fluff from the MockProvider's response + assert "Certainly!" not in response + assert "bug is in line 42" in response + finally: + agent.memory.close() print("✅ Golden Path integration test passed!") diff --git a/tests/test_memory.py b/tests/test_memory.py index 02b6aa4..c5509f1 100644 --- a/tests/test_memory.py +++ b/tests/test_memory.py @@ -43,5 +43,123 @@ async def test_memory_pipeline(): print("✅ Memory pipeline tests passed!") +async def test_semantic_search_skips_zero_norm_embeddings(): + """Regression: cosine distance is undefined (NULL) for a zero-norm + embedding. semantic_search must skip such rows instead of crashing with + `unsupported operand type(s) for -: 'float' and 'NoneType'` - this + happened in practice because SkillSynthesizer used to hardcode + embedding=[0.0]*128 for every synthesized skill. + """ + from memory.db import MemoryDB, MemoryChunk, EMBEDDING_DIM + + class _FixedEmbeddingProvider: + async def get_embedding(self, text: str): + return [1.0] + [0.0] * (EMBEDDING_DIM - 1) + + db = MemoryDB(":memory:") + db.add_memory(MemoryChunk( + content="Zero vector placeholder memory", + embedding=[0.0] * EMBEDDING_DIM, + metadata={}, + mem_type="DOC", + )) + db.add_memory(MemoryChunk( + content="Real memory with a real embedding", + embedding=[1.0] + [0.0] * (EMBEDDING_DIM - 1), + metadata={}, + mem_type="DOC", + )) + + retriever = HybridRetriever(db, _FixedEmbeddingProvider()) + # Must not raise. + results = await retriever.retrieve("anything", top_k=5) + contents = [r["content"] for r in results] + assert "Zero vector placeholder memory" not in contents + assert "Real memory with a real embedding" in contents + + print("✅ Zero-norm embedding regression test passed!") + + +async def test_skill_synthesizer_never_stores_zero_embedding(): + """Regression: SkillSynthesizer used to hardcode embedding=[0.0]*128.""" + from unittest.mock import AsyncMock + from core.learning import SkillSynthesizer, Trajectory + from core.providers import ModelConfig + from memory.db import MemoryDB, EMBEDDING_DIM + import shutil + import struct + import tempfile + + model_config = ModelConfig(name="test", endpoint="http://localhost", provider_type="local") + db = MemoryDB(":memory:") + tmp_skills_dir = tempfile.mkdtemp() + try: + synthesizer = SkillSynthesizer(model_config, db, skills_dir=tmp_skills_dir, embedding_provider=None) + synthesizer.provider = AsyncMock() + synthesizer.provider.complete.return_value = "# Skill\n## Description\nTest\n## Procedure\n1. Do it." + + trajectory = Trajectory( + task_id="t1", prompt="test skill", steps=[], final_result="done", success=True, + ) + await synthesizer.synthesize(trajectory) + + row = db.conn.execute("SELECT embedding FROM memories ORDER BY id DESC LIMIT 1").fetchone() + assert row is not None + vals = struct.unpack(f"<{EMBEDDING_DIM}f", row[0]) + norm = sum(v * v for v in vals) ** 0.5 + assert norm > 0, "synthesized skill must not be stored with a zero-norm embedding" + finally: + shutil.rmtree(tmp_skills_dir, ignore_errors=True) + db.close() + + print("✅ Skill synthesizer non-zero embedding regression test passed!") + + +async def test_retriever_drops_unrelated_semantic_matches_and_dedupes(): + """Regression: a low-similarity semantic hit must not be blindly + injected as context, and duplicate content (e.g. from duplicate DB rows) + must be collapsed to one entry. This is what let an unrelated, heavily + duplicated memory hijack unrelated conversations. + """ + from memory.db import MemoryDB, MemoryChunk, EMBEDDING_DIM + + class _FixedEmbeddingProvider: + def __init__(self, vector): + self.vector = vector + + async def get_embedding(self, text: str): + return self.vector + + db = MemoryDB(":memory:") + query_vec = [1.0] + [0.0] * (EMBEDDING_DIM - 1) + # "Relevant" memory: identical direction to the query -> similarity ~1.0. + db.add_memory(MemoryChunk( + content="Relevant memory about the actual topic.", + embedding=query_vec, + metadata={}, + mem_type="DOC", + )) + # "Unrelated" memory: orthogonal-ish vector -> low similarity, should be + # filtered out by the relevance threshold instead of always appearing. + unrelated_vec = [0.0, 1.0] + [0.0] * (EMBEDDING_DIM - 2) + for _ in range(3): + db.add_memory(MemoryChunk( + content="The bug in line 42 is caused by a null pointer in the handler.", + embedding=unrelated_vec, + metadata={}, + mem_type="DOC", + )) + + retriever = HybridRetriever(db, _FixedEmbeddingProvider(query_vec)) + results = await retriever.retrieve("actual topic", top_k=5) + + contents = [r["content"] for r in results] + assert "Relevant memory about the actual topic." in contents + assert "The bug in line 42 is caused by a null pointer in the handler." not in contents + + print("✅ Retriever relevance/dedup regression test passed!") + + if __name__ == "__main__": asyncio.run(test_memory_pipeline()) + asyncio.run(test_retriever_drops_unrelated_semantic_matches_and_dedupes()) diff --git a/tests/test_session_context.py b/tests/test_session_context.py index b543de4..9faa186 100644 --- a/tests/test_session_context.py +++ b/tests/test_session_context.py @@ -205,15 +205,35 @@ def test_compact_preserves_context_in_build_prompt(): # ─── Agent mode (build/plan) ────────────────────────────────────────────────── -def test_agent_mode_defaults_to_build(): +def test_agent_mode_defaults_to_plan(): + # New asks start read-only/discuss-first; the user must explicitly + # confirm (Tab toggle or a build-trigger phrase) before file writes. state = AppState() - assert state.agent_mode == "build" + assert state.agent_mode == "plan" def test_agent_mode_toggle_cycles(): state = AppState() - assert state.agent_mode == "build" - state.agent_mode = "plan" if state.agent_mode == "build" else "build" assert state.agent_mode == "plan" state.agent_mode = "plan" if state.agent_mode == "build" else "build" assert state.agent_mode == "build" + state.agent_mode = "plan" if state.agent_mode == "build" else "build" + assert state.agent_mode == "plan" + + +def test_build_trigger_phrases_detected(): + from ui.tui import _is_build_trigger + + assert _is_build_trigger("Let's build it") + assert _is_build_trigger("ok go ahead") + assert _is_build_trigger("Great, build it.") + assert _is_build_trigger("ship it!") + + +def test_build_trigger_ignores_plain_descriptions(): + from ui.tui import _is_build_trigger + + assert not _is_build_trigger( + "We'll create a market scraper that grabs prices and descriptions." + ) + assert not _is_build_trigger("Can you explain how the scraper would work?") diff --git a/tests/test_workspace_tools.py b/tests/test_workspace_tools.py new file mode 100644 index 0000000..178fba1 --- /dev/null +++ b/tests/test_workspace_tools.py @@ -0,0 +1,333 @@ +from pathlib import Path + +import pytest + +from core.providers import ModelConfig +from core.workspace_tools import WorkspaceToolError, WorkspaceTools, parse_tool_call +from main import MotionAgent + + +def test_write_and_replace_file(tmp_path: Path) -> None: + tools = WorkspaceTools(tmp_path) + + result = tools.execute( + "write_file", + {"path": "src/app.py", "content": "print('old')\n"}, + ) + assert result["path"] == "src/app.py" + assert (tmp_path / "src/app.py").read_text() == "print('old')\n" + + tools.execute( + "replace_in_file", + { + "path": "src/app.py", + "old": "print('old')", + "new": "print('new')", + }, + ) + assert (tmp_path / "src/app.py").read_text() == "print('new')\n" + + +def test_tools_reject_paths_outside_workspace(tmp_path: Path) -> None: + tools = WorkspaceTools(tmp_path) + with pytest.raises(WorkspaceToolError, match="escapes"): + tools.execute("write_file", {"path": "../outside.txt", "content": "no"}) + + +def test_plan_mode_rejects_writes(tmp_path: Path) -> None: + tools = WorkspaceTools(tmp_path, read_only=True) + with pytest.raises(WorkspaceToolError, match="plan mode"): + tools.execute("write_file", {"path": "blocked.txt", "content": "no"}) + + +def test_plan_mode_instructions_do_not_demand_writes(tmp_path: Path) -> None: + # Regression: Plan-mode instructions must never tell the model it "MUST" + # write files, since write_file/replace_in_file always fail read-only. + # Otherwise the model just punts back to the user instead of planning. + instructions = WorkspaceTools(tmp_path, read_only=True).instructions + assert "MUST actually write files" not in instructions + assert "DISABLED" in instructions + assert "PLAN" in instructions + + +def test_build_mode_instructions_still_demand_writes(tmp_path: Path) -> None: + instructions = WorkspaceTools(tmp_path, read_only=False).instructions + assert "MUST actually write files" in instructions + + +def test_parse_tool_call() -> None: + call = parse_tool_call( + '{"name":"read_file","arguments":{"path":"README.md"}}' + ) + assert call == ("read_file", {"path": "README.md"}) + + +def test_parse_tool_call_tolerates_malformed_opening_tag() -> None: + call = parse_tool_call( + '{"name":"list_files","arguments":{"path":".","pattern":"*"}}' + ) + assert call == ("list_files", {"path": ".", "pattern": "*"}) +def test_parse_tool_call_accepts_direct_xml_tool_tag() -> None: + call = parse_tool_call( + 'I will inspect the project.\n{"path": ".", "pattern": "*"}' + ) + assert call == ("list_files", {"path": ".", "pattern": "*"}) + + +def test_parse_tool_call_accepts_direct_write_tag() -> None: + call = parse_tool_call( + '{"path":"app.py","content":"print(1)\\n"}' + ) + assert call == ("write_file", {"path": "app.py", "content": "print(1)\n"}) + + +def test_parse_recovers_misspelled_envelope_and_unescaped_source_quotes() -> None: + response = ( + '{"name":"write_file","arguments":' + '{"path":"models/product.py","content":""""Product data model."""\\n\\n' + 'from dataclasses import dataclass\\n\\n' + '@dataclass\\nclass Product:\\n name: str\\n' + ' def to_dict(self):\\n return {"name": self.name}\\n"}}' + '' + ) + + call = parse_tool_call(response) + + assert call is not None + name, arguments = call + assert name == "write_file" + assert arguments["path"] == "models/product.py" + assert arguments["content"].startswith('"""Product data model."""\n\n') + assert 'return {"name": self.name}' in arguments["content"] + + +def test_parse_tool_call_rejects_unparseable_markup() -> None: + with pytest.raises(WorkspaceToolError, match="malformed"): + parse_tool_call("not json") + + +def test_parse_tool_call_tolerates_space_after_opening_bracket() -> None: + # Regression: a model emitted "< motion_tool>" (stray space after "<"). + # This previously matched none of the tool patterns AND slipped past the + # malformed-tag leak guard (which checked for the literal substring + # "{"name":"write_file","arguments":' + '{"path":"requirements.txt","content":"fastapi\\nuvicorn\\n"}}' + ) + assert call == ("write_file", {"path": "requirements.txt", "content": "fastapi\nuvicorn\n"}) + + +def test_parse_tool_call_tolerates_space_before_closing_bracket() -> None: + call = parse_tool_call( + '{"name":"read_file","arguments":{"path":"a.txt"}}< /motion_tool>' + ) + assert call == ("read_file", {"path": "a.txt"}) + + +def test_parse_tool_call_raises_instead_of_leaking_when_truly_malformed() -> None: + # A stray-space opening tag with no valid closing tag at all must still + # be caught by the leak guard (raise) rather than silently returning + # None, which would let the raw markup leak to the user as final text. + with pytest.raises(WorkspaceToolError, match="malformed"): + parse_tool_call('< motion_tool>{"name":"write_file"') + + +def test_parse_tool_call_handles_dsml_channel_leak() -> None: + # Regression: deepseek-v4-flash leaked its own internal tool-channel + # markup verbatim - "<| DSML | tool:write_file>{...}" - + # which matched none of the motion_tool/direct-tag patterns nor the old + # leak guard, so the raw call (including a huge README) was shown as the + # final chat answer instead of being executed. + call = parse_tool_call( + '<| DSML | tool:write_file>{"path":"README.md","content":"# Title\\n"}' + '' + ) + assert call == ("write_file", {"path": "README.md", "content": "# Title\n"}) + + +def test_parse_tool_call_generic_guard_catches_unknown_envelope_names() -> None: + # Defense in depth: whatever exotic envelope name a model invents next, + # a tag wrapping something shaped like write_file's arguments (path + + # content) must never be shown as final text - it should raise instead + # of returning None, so main.py's retry loop kicks in. + with pytest.raises(WorkspaceToolError, match="malformed"): + parse_tool_call( + '{"path":"a.txt","content":"x"}' + ) + + +class _ToolCallingProvider: + class _Config: + provider_type = "local" + + def __init__(self) -> None: + self.config = self._Config() + self.calls = 0 + + async def complete(self, prompt, system_prompt="", history=None, **kwargs): + self.calls += 1 + if self.calls == 1: + assert "write_file" in system_prompt + return ( + '{"name":"write_file","arguments":' + '{"path":"generated.txt","content":"created by agent\\n"}}' + ) + assert history + assert "motion_tool_result" in history[-1]["content"] + return "Created `generated.txt`." + + async def close(self): + pass + + +class _EmptyRetriever: + async def retrieve(self, query, top_k=5): + return [] + + +async def test_agent_executes_write_tool(tmp_path: Path) -> None: + agent = MotionAgent( + ModelConfig(name="test", endpoint="http://localhost", provider_type="local"), + memory_path=":memory:", + ) + agent.provider = _ToolCallingProvider() + agent.retriever = _EmptyRetriever() + + response = await agent.run( + "Create generated.txt", + workspace=str(tmp_path), + agent_mode="build", + ) + + assert response == "Created `generated.txt`." + assert (tmp_path / "generated.txt").read_text() == "created by agent\n" + + +class _EmptyFinalProvider(_ToolCallingProvider): + async def complete(self, prompt, system_prompt="", history=None, **kwargs): + self.calls += 1 + if self.calls == 1: + return ( + '{"name":"write_file","arguments":' + '{"path":"fallback.txt","content":"done\\n"}}' + ) + return "" + + +async def test_agent_summarizes_tools_when_model_final_is_empty(tmp_path: Path) -> None: + agent = MotionAgent( + ModelConfig(name="test", endpoint="http://localhost", provider_type="local"), + memory_path=":memory:", + ) + agent.provider = _EmptyFinalProvider() + agent.retriever = _EmptyRetriever() + + response = await agent.run( + "Create fallback.txt", + workspace=str(tmp_path), + agent_mode="build", + ) + + assert response == "Completed filesystem changes:\n- wrote `fallback.txt`" + assert (tmp_path / "fallback.txt").read_text() == "done\n" + assert agent.provider.calls == 4 + + +class _PlanModeBlockedWriteProvider: + """Simulates a model that first tries to write, gets blocked, then plans.""" + + class _Config: + provider_type = "local" + + def __init__(self) -> None: + self.config = self._Config() + self.calls = 0 + self.seen_histories: list[list] = [] + + async def complete(self, prompt, system_prompt="", history=None, **kwargs): + self.calls += 1 + self.seen_histories.append(history or []) + if self.calls == 1: + assert "MUST actually write files" not in system_prompt + return ( + '{"name":"write_file","arguments":' + '{"path":"scraper.py","content":"pass\\n"}}' + ) + # Second turn: the loop must have nudged it away from retrying the + # write and toward answering with a real plan. + assert "read-only Plan mode" in history[-1]["content"] + return "Plan: create scraper.py, storage.py, and a CLI entrypoint." + + async def close(self): + pass + + +async def test_plan_mode_recovers_from_blocked_write_with_real_plan(tmp_path: Path) -> None: + agent = MotionAgent( + ModelConfig(name="test", endpoint="http://localhost", provider_type="local"), + memory_path=":memory:", + ) + agent.provider = _PlanModeBlockedWriteProvider() + agent.retriever = _EmptyRetriever() + + response = await agent.run( + "We'll create a market scraper", + workspace=str(tmp_path), + agent_mode="plan", + ) + + assert response == "Plan: create scraper.py, storage.py, and a CLI entrypoint." + assert not (tmp_path / "scraper.py").exists() + + +class _NeverFinishesProvider: + """Simulates a model that keeps writing files and never stops on its own, + forcing the tool loop to hit its MAX_TOOL_STEPS cap.""" + + class _Config: + provider_type = "local" + + def __init__(self) -> None: + self.config = self._Config() + self.calls = 0 + + async def complete(self, prompt, system_prompt="", history=None, **kwargs): + self.calls += 1 + return ( + '{"name":"write_file","arguments":' + f'{{"path":"file{self.calls}.txt","content":"content {self.calls}\\n"}}}}' + '' + ) + + async def close(self): + pass + + +async def test_agent_reports_progress_when_tool_call_cap_is_hit(tmp_path: Path) -> None: + # Regression: hitting the tool-call cap used to discard all completed + # work and reply with a bare "Stopped after 12 tool calls. Please narrow + # the task and try again." even when files had already been written. + from main import MAX_TOOL_STEPS + + agent = MotionAgent( + ModelConfig(name="test", endpoint="http://localhost", provider_type="local"), + memory_path=":memory:", + ) + agent.provider = _NeverFinishesProvider() + agent.retriever = _EmptyRetriever() + + response = await agent.run( + "Build a large multi-file project", + workspace=str(tmp_path), + agent_mode="build", + ) + + assert f"internal safety limit ({MAX_TOOL_STEPS} tool calls)" in response + assert "wrote `file1.txt`" in response + assert f"wrote `file{MAX_TOOL_STEPS}.txt`" in response + assert "continue" in response.lower() + # The files were actually written to disk despite the loop not finishing. + assert (tmp_path / "file1.txt").read_text() == "content 1\n" + assert (tmp_path / f"file{MAX_TOOL_STEPS}.txt").exists() diff --git a/ui/tui.py b/ui/tui.py index be94d82..c1fd26f 100644 --- a/ui/tui.py +++ b/ui/tui.py @@ -41,7 +41,7 @@ from rich.style import Style from rich.text import Text -from textual import work +from textual import events, work from textual.app import App, ComposeResult from textual.binding import Binding from textual.containers import Container, Horizontal, Vertical, VerticalScroll @@ -99,8 +99,20 @@ def __init__(self) -> None: self.ui_mode: str = "conservative" self.show_activity_rail: bool = True self.show_trace_panel: bool = False - self.agent_mode: str = "build" # "build" (full access) or "plan" (read-only) + # When enabled, the agent's intermediate tool-loop responses (its + # visible "thinking" between tool calls) are shown inline in the chat + # log, not just summarized in the trace panel. Off by default since + # it can be noisy; toggle with F7 or the command palette. + self.show_thinking: bool = False + # New asks start in "plan" (read-only, discuss-first). The agent only + # gains write access ("build") once the user explicitly confirms via + # a build-trigger phrase (see _is_build_trigger) or the Tab toggle. + self.agent_mode: str = "plan" self.busy: bool = False # True while an agent response is streaming + # Prompts submitted while busy are queued here instead of cancelling + # the in-flight agent worker (which @work(exclusive=True) would + # otherwise do). _run_agent drains this after each turn completes. + self.message_queue: list[str] = [] self.last_agent_response: str = "" self.prompt_history: list[str] = [] # submitted prompts for up/down recall self._history_index: int = -1 @@ -263,6 +275,35 @@ def _skills_dir() -> Path: return Path(WORKSPACE) / "skills" +# Phrases that count as an explicit go-ahead to start creating/editing files. +# Kept intentionally narrow (word-boundary anchored) to avoid false positives +# on messages that merely *describe* a build without asking for one yet. +_BUILD_TRIGGER_PATTERNS = [ + re.compile(pattern, re.IGNORECASE) + for pattern in ( + r"\blet'?s build\b", + r"\blet'?s do (it|this)\b", + r"\blet'?s ship (it|this)\b", + r"\bgo ahead\b", + r"\bbuild it\b", + r"\bbuild this\b", + r"\bbuild that\b", + r"\bship it\b", + r"\bimplement it\b", + r"\bimplement this\b", + r"\bstart building\b", + r"\byou can build\b", + r"\byes,? build\b", + r"\bmake it happen\b", + ) +] + + +def _is_build_trigger(text: str) -> bool: + """Return True if the message is an explicit confirmation to start building.""" + return any(pattern.search(text) for pattern in _BUILD_TRIGGER_PATTERNS) + + def _extract_reasoning_and_answer(text: str) -> tuple[str, str]: """Extract ... blocks if present; return (reasoning, answer).""" if "" not in text: @@ -307,6 +348,42 @@ class ReasoningMessage(Static): } """ +class ThinkingMessage(Static): + """Opt-in live view of the agent's intermediate tool-loop responses. + + Distinct from ReasoningMessage (which renders blocks from the + final answer): this shows the model's visible text between tool calls + as it works, when available and when the user has enabled it (F7 / + command palette "Toggle agent thinking"). + """ + DEFAULT_CSS = """ + ThinkingMessage { + background: $panel; + color: $text-muted; + border-left: solid $secondary; + padding: 0 2; + margin: 0 0 1 1; + } + """ + +class StepsMessage(Static): + """Always-visible live view of tool activity (list/read/write/replace). + + Unlike ThinkingMessage (the model's free-form intermediate text, opt-in + via show_thinking), this shows concrete tool operations - "wrote x.py", + "read y.py" - so long-running tasks always show visible progress instead + of a blank chat while many tool calls run in the background. + """ + DEFAULT_CSS = """ + StepsMessage { + background: $panel; + color: $text-muted; + border-left: solid $success; + padding: 0 2; + margin: 0 0 1 1; + } + """ + class AgentMessage(Static): """Agent reply — no box, clean text flow, spaced below the user prompt. @@ -457,9 +534,17 @@ def _invalidate_layout(self) -> None: def _insert(self, char: str) -> None: pos = self.cursor_position self.value = self.value[:pos] + char + self.value[pos:] - self.cursor_position = min(len(self.value), pos + 1) + self.cursor_position = min(len(self.value), pos + len(char)) self._invalidate_layout() + def on_paste(self, event: events.Paste) -> None: + """Insert terminal bracketed-paste text at the current cursor.""" + event.stop() + event.prevent_default() + if event.text: + # Preserve multiline prompts while normalizing terminal line endings. + self._insert(event.text.replace("\r\n", "\n").replace("\r", "\n")) + def _delete(self) -> None: pos = self.cursor_position if pos < len(self.value): @@ -882,6 +967,7 @@ def on_mount(self) -> None: ("Toggle theme", "theme"), ("Show shortcuts", "shortcuts"), ("Toggle trace panel", "trace"), + ("Toggle agent thinking", "thinking"), ("Copy last response", "copy"), ("Toggle context panel", "context"), ("Manage API keys (/auth)", "auth"), @@ -946,6 +1032,11 @@ def _run(self, action: str) -> None: self._main.query_one(ChatPane).action_toggle_trace_panel() except Exception: pass + elif action == "thinking": + try: + self._main.query_one(ChatPane).action_toggle_thinking() + except Exception: + pass elif action == "copy": try: self._main.query_one(ChatPane).action_copy_last_response() @@ -1335,6 +1426,7 @@ class ChatPane(Vertical): BINDINGS = [ Binding("ctrl+shift+t", "toggle_trace_panel", "Trace", priority=True), Binding("ctrl+shift+c", "copy_last_response", "Copy", priority=True), + Binding("f7", "toggle_thinking", "Thinking", priority=True), Binding("f8", "toggle_trace_panel", "Trace", priority=True), Binding("f9", "copy_last_response", "Copy", priority=True), Binding("tab", "toggle_agent_mode", "Agent", priority=True), @@ -1451,7 +1543,7 @@ def on_mount(self) -> None: name = self.state.agent.provider.config.name if self.state.agent else "?" log = self.query_one("#chat_log", VerticalScroll) log.mount(SystemMessage(f"⚡ Motion Harness — connected to {name}")) - log.mount(SystemMessage("Tip: Ctrl+K commands · Ctrl+O model · Tab agent · F8 trace · F9 copy · /skill save ")) + log.mount(SystemMessage("Tip: Ctrl+K commands · Ctrl+O model · Tab agent · F7 thinking · F8 trace · F9 copy · /skill save ")) self._append_trace("session_start", f"provider={name}") self._set_trace_panel_visible(self.state.show_trace_panel) self._refresh_meta() @@ -1485,6 +1577,17 @@ def _refresh_trace_chip(self) -> None: def action_toggle_trace_panel(self) -> None: self._set_trace_panel_visible(not self.state.show_trace_panel) + def action_toggle_thinking(self) -> None: + """Toggle inline display of the agent's intermediate tool-loop text. + + This is opt-in and separate from the trace panel: when enabled, the + model's visible responses between tool calls (if any - some + providers/tasks never produce intermediate text) are rendered as a + live "thinking" bubble in the chat log, not just summarized in trace. + """ + self.state.show_thinking = not self.state.show_thinking + self.notify(f"Agent thinking {'shown' if self.state.show_thinking else 'hidden'}") + def _toggle_agent_mode(self) -> None: """Switch between build (full access) and plan (read-only) agents.""" self.state.agent_mode = "plan" if self.state.agent_mode == "build" else "build" @@ -1686,10 +1789,26 @@ async def _submit_composer(self, text: str = "") -> None: await self._handle_auth_command(text, log) log.scroll_end(animate=False) return + if self.state.agent_mode == "plan" and _is_build_trigger(text): + self.state.agent_mode = "build" + self._refresh_meta() + log.mount(SystemMessage("🔧 Build confirmed — switching to Build mode, I'll create/edit files now.")) ts = datetime.now().strftime("%H:%M:%S") user_msg = UserMessage("") user_msg.update(self._render_user_markdown(ts, text)) log.mount(user_msg) + if self.state.busy: + # Don't call _run_agent here: it's @work(exclusive=True), so a + # second call would cancel the in-flight turn instead of running + # alongside it. Queue instead - the running worker drains this + # queue itself once its current turn finishes. + self.state.message_queue.append(text) + log.mount(SystemMessage( + f"📥 Queued (#{len(self.state.message_queue)}) — will run once the " + "current task finishes." + )) + log.scroll_end(animate=False) + return live_response = AgentMessage("") log.mount(live_response) log.scroll_end(animate=False) @@ -1728,18 +1847,89 @@ def _append_trace(self, event_type: str, detail: str = "") -> None: @work(exclusive=True, name="agent_chat") async def _run_agent(self, prompt: str, live_response: AgentMessage) -> None: + """Run one turn, then drain any prompts queued while it was busy. + + Looping here (rather than re-invoking this @work(exclusive=True) + method) means a queued follow-up runs in the SAME worker instead of + starting a second worker that would cancel this one. + """ log = self.query_one("#chat_log", VerticalScroll) self.state.busy = True self._refresh_status() + try: + while True: + cancelled = await self._run_agent_turn(prompt, live_response, log) + if cancelled: + if self.state.message_queue: + dropped = len(self.state.message_queue) + self.state.message_queue.clear() + log.mount(SystemMessage( + f"⏹ Discarded {dropped} queued message(s) due to cancellation." + )) + break + if not self.state.message_queue: + break + prompt = self.state.message_queue.pop(0) + live_response = AgentMessage("") + log.mount(live_response) + log.scroll_end(animate=False) + finally: + self.state.busy = False + self._refresh_status() + log.scroll_end(animate=False) + + async def _run_agent_turn( + self, prompt: str, live_response: AgentMessage, log: VerticalScroll + ) -> bool: + """Run a single agent turn. Returns True if it was cancelled.""" chunks: list[str] = [] header_ts = datetime.now().strftime("%H:%M:%S") reasoning_widget: Optional[ReasoningMessage] = None + thinking_widget: Optional[ThinkingMessage] = None + thinking_steps: list[str] = [] + steps_widget: Optional[StepsMessage] = None + step_lines: list[str] = [] current_raw = "" self._append_trace("interaction_start", prompt[:120]) async def on_stream_chunk(chunk: str) -> None: if not chunk: return + + # Internal progress markers come from the tool loop (e.g. "_step_", + # "_tool_"). "_tool_" (concrete tool operations like "wrote x.py") + # is always shown inline so long tasks show live progress instead + # of a blank chat. "_step_" (the model's free-form intermediate + # text) stays opt-in (F7 / "Toggle agent thinking") since it can + # be noisy/repetitive. + if chunk.startswith("_step_ "): + nonlocal thinking_widget + step_text = chunk[len("_step_ "):].strip() + self._append_trace("tool_progress", step_text[:220]) + if self.state.show_thinking and step_text: + thinking_steps.append(step_text) + if thinking_widget is None: + thinking_widget = ThinkingMessage("") + log.mount(thinking_widget, before=live_response) + preview = "\n\n".join(f"› {s}" for s in thinking_steps[-6:]) + thinking_widget.update(Text(preview[:3000], style="dim italic")) + log.scroll_end(animate=False) + return + if chunk.startswith("_tool_ "): + nonlocal steps_widget + tool_text = chunk[len("_tool_ "):].strip() + self._append_trace("tool_progress", tool_text[:220]) + self._refresh_status() + if tool_text: + step_lines.append(tool_text) + if steps_widget is None: + steps_widget = StepsMessage("") + log.mount(steps_widget, before=live_response) + preview = "\n".join(f"• {s}" for s in step_lines[-8:]) + steps_widget.update(Text(preview[:3000], style="dim")) + log.scroll_end(animate=False) + return + nonlocal reasoning_widget, current_raw chunks.append(chunk) current_raw = "".join(chunks) @@ -1785,6 +1975,8 @@ async def on_trace_event(*args) -> None: on_trace_event=on_trace_event, history=history or None, context_query=context_query or None, + workspace=WORKSPACE, + agent_mode=self.state.agent_mode, ) if not chunks: raw = response or "" @@ -1825,18 +2017,17 @@ async def on_trace_event(*args) -> None: if isinstance(main_screen, MainScreen): main_screen.refresh_session_footer() main_screen.refresh_context_panel() + return False except asyncio.CancelledError: live_response.remove() log.mount(SystemMessage("⏹ Cancelled.")) self._append_trace("interaction_cancelled") + return True except Exception as e: live_response.remove() log.mount(SystemMessage(f"❌ {e}")) self._append_trace("interaction_error", str(e)) - finally: - self.state.busy = False - self._refresh_status() - log.scroll_end(animate=False) + return False async def _handle_skill_command(self, text: str, log: VerticalScroll) -> None: parts = text.split(maxsplit=2)