From 35d10ba88a36934c274590d7ae8b386dbab3dc29 Mon Sep 17 00:00:00 2001 From: ParthSareen Date: Mon, 5 Jan 2026 22:02:52 -0800 Subject: [PATCH] address comments --- cmd/cmd.go | 9 +- cmd/interactive.go | 22 --- x/APPROVAL_UX.md | 231 ----------------------------- x/DOUBLE_PRESS_FIX.md | 12 -- x/README.md | 331 ------------------------------------------ 5 files changed, 3 insertions(+), 602 deletions(-) delete mode 100644 x/APPROVAL_UX.md delete mode 100644 x/DOUBLE_PRESS_FIX.md delete mode 100644 x/README.md diff --git a/cmd/cmd.go b/cmd/cmd.go index 7b460c941..0f4431c08 100644 --- a/cmd/cmd.go +++ b/cmd/cmd.go @@ -518,10 +518,8 @@ func RunHandler(cmd *cobra.Command, args []string) error { return generateEmbedding(cmd, name, opts.Prompt, opts.KeepAlive, truncate, dimensions) } - // Check for experimental/beta flag - experimental, _ := cmd.Flags().GetBool("experimental") - beta, _ := cmd.Flags().GetBool("beta") - isExperimental := experimental || beta + // Check for experimental flag + isExperimental, _ := cmd.Flags().GetBool("experimental") if interactive { if err := loadOrUnloadModel(cmd, &opts); err != nil { @@ -1765,8 +1763,7 @@ func NewCLI() *cobra.Command { runCmd.Flags().Bool("hidethinking", false, "Hide thinking output (if provided)") runCmd.Flags().Bool("truncate", false, "For embedding models: truncate inputs exceeding context length (default: true). Set --truncate=false to error instead") runCmd.Flags().Int("dimensions", 0, "Truncate output embeddings to specified dimension (embedding models only)") - runCmd.Flags().Bool("experimental", false, "Enable experimental features (MCP, agent loop)") - runCmd.Flags().Bool("beta", false, "Alias for --experimental") + runCmd.Flags().Bool("experimental", false, "Enable experimental agent loop with tools") stopCmd := &cobra.Command{ Use: "stop MODEL", diff --git a/cmd/interactive.go b/cmd/interactive.go index a58b528bc..9c4e32a2e 100644 --- a/cmd/interactive.go +++ b/cmd/interactive.go @@ -41,15 +41,6 @@ func generateInteractive(cmd *cobra.Command, opts runOptions) error { fmt.Fprintln(os.Stderr, " /?, /help Help for a command") fmt.Fprintln(os.Stderr, " /? shortcuts Help for keyboard shortcuts") - // Show experimental commands if enabled - experimental, _ := cmd.Flags().GetBool("experimental") - beta, _ := cmd.Flags().GetBool("beta") - if experimental || beta { - fmt.Fprintln(os.Stderr, "") - fmt.Fprintln(os.Stderr, "Experimental Commands:") - fmt.Fprintln(os.Stderr, " /tools Show available tools") - } - fmt.Fprintln(os.Stderr, "") fmt.Fprintln(os.Stderr, "Use \"\"\" to begin a multi-line message.") @@ -469,19 +460,6 @@ func generateInteractive(cmd *cobra.Command, opts runOptions) error { } case strings.HasPrefix(line, "/exit"), strings.HasPrefix(line, "/bye"): return nil - case strings.HasPrefix(line, "/tools"): - // Check if experimental mode is enabled - experimental, _ := cmd.Flags().GetBool("experimental") - beta, _ := cmd.Flags().GetBool("beta") - if !experimental && !beta { - fmt.Println("/tools requires --experimental or --beta flag") - continue - } - fmt.Println("Available tools:") - fmt.Println(" web_search - Search the web for current information") - fmt.Println(" bash - Execute bash commands") - fmt.Println() - continue case strings.HasPrefix(line, "/"): args := strings.Fields(line) isFile := false diff --git a/x/APPROVAL_UX.md b/x/APPROVAL_UX.md deleted file mode 100644 index 1fa7cc907..000000000 --- a/x/APPROVAL_UX.md +++ /dev/null @@ -1,231 +0,0 @@ -# Tool Approval UX - -This document describes the interactive tool approval system for the agent loop. - -## Overview - -When the agent requests to execute a tool (bash command, web search, etc.), the user is presented with an interactive approval dialog. This provides a secure, user-friendly way to control tool execution. - -## Features - -### Interactive Selector Box - -``` -┌──────────────────────────────────────────────────────────┐ -│ Tool: bash │ -│ Command: cat src/main.go | head -50 │ -├──────────────────────────────────────────────────────────┤ -│ > 1. Execute once │ -│ 2. Always allow │ -│ 3. Deny: │ -└──────────────────────────────────────────────────────────┘ -↑/↓ navigate, Enter confirm, 1-3 quick, Ctrl+C cancel -``` - -### Input Methods - -| Input | Action | -|-------|--------| -| `↑`/`↓` | Navigate between options | -| `Enter` | Confirm current selection | -| `1`, `2`, `3` | Quick select option | -| `Ctrl+C` | Cancel (deny with "cancelled" reason) | -| Any letter | Type deny reason (auto-selects Deny option) | -| `Backspace` | Delete last character from reason | -| `Esc` | Clear deny reason | - -### Inline Deny Reason - -The deny reason input is displayed inline with the Deny option: - -``` -│ 3. Deny: too dangerous │ -``` - -When you start typing, the Deny option is automatically highlighted: - -``` -Before typing: -│ > 1. Execute once │ -│ 2. Always allow │ -│ 3. Deny: │ - -After typing: -│ 1. Execute once │ -│ 2. Always allow │ -│ > 3. Deny: risky command │ -``` - -### Prefix-Based Allowlist - -For bash commands, the "Always allow" option saves command prefixes rather than exact commands. This enables allowing categories of safe operations: - -| Command | Saved Prefix | -|---------|--------------| -| `cat src/main.go` | `cat:src/` | -| `ls -la tools/` | `ls:tools/` | -| `head -n 100 README.md` | `head:./` | -| `grep -r "pattern" api/` | `grep:api/` | - -Safe commands eligible for prefix matching: -- `cat`, `ls`, `head`, `tail`, `less`, `more` -- `file`, `wc`, `grep`, `find`, `tree`, `stat` - -Non-safe commands (like `rm`, `mv`, `curl`) require exact match approval. - -### Responsive Layout - -The box adapts to terminal width: -- Width: 90% of terminal, clamped between 24-60 characters -- Long text wraps instead of truncating -- Hint text wraps on narrow terminals - -## Files - -### `x/agent/approval.go` - -Main implementation containing: - -- `ApprovalManager` - Manages session allowlist and prefix matching -- `ApprovalResult` - Contains decision and optional deny reason -- `runSelector()` - Interactive terminal UI -- `extractBashPrefix()` - Extracts safe command prefixes -- Rendering functions for the selector box - -### `x/cmd/run.go` - -Agent loop integration: - -```go -if !approval.IsAllowed(toolName, args) { - result, err = approval.RequestApproval(toolName, args) - switch result.Decision { - case agent.ApprovalDeny: - // Return denial message to model - case agent.ApprovalAlways: - approval.AddToAllowlist(toolName, args) - } -} -``` - -## Implementation Details - -### Terminal Handling - -- Uses `golang.org/x/term` for raw mode input -- ANSI escape codes for cursor movement and colors -- Proper `\r\n` line endings in raw mode - -### Stdin Flushing - -To prevent buffered input from causing double-press issues: - -```go -func flushStdin(fd int) { - syscall.SetNonblock(fd, true) - defer syscall.SetNonblock(fd, false) - time.Sleep(5 * time.Millisecond) - // Drain buffered input - buf := make([]byte, 256) - for { - n, _ := syscall.Read(fd, buf) - if n <= 0 { break } - } -} -``` - -See also: `x/DOUBLE_PRESS_FIX.md` for the readline synchronization fix. - -### Warning Box for Outside-Project Commands - -When a bash command targets paths outside the current working directory, the box is rendered in red with a warning indicator: - -``` -┌────────────────────────────────────────┐ -│ !! OUTSIDE PROJECT !! │ -│ Tool: bash │ -│ Command: cat /etc/passwd │ -├────────────────────────────────────────┤ -│ > 1. Execute once │ -│ 2. Always allow │ -│ 3. Deny: │ -└────────────────────────────────────────┘ -``` - -Detection includes: -- Absolute paths outside cwd (e.g., `/etc/passwd`, `/home/user/file`) -- Parent directory traversal (e.g., `../../../etc/passwd`) -- Home directory expansion (e.g., `~/.bashrc`) - -### Color Scheme - -| Element | Color Code | Description | -|---------|------------|-------------| -| Box borders (normal) | `\033[36m` | Cyan | -| Box borders (warning) | `\033[91m` | Bright red | -| Selected option | `\033[1;32m` | Bold green | -| Unselected Deny | `\033[90m` | Gray | -| Hint text | `\033[90m` | Gray | - -## Testing - -Test program available at `/tmp/selector_test/`: - -```bash -cd /tmp/selector_test -go build -o selector_test . -./selector_test # Normal cyan box -./selector_test -w # Warning red box (outside project) -``` - -Expect scripts for automated testing: - -```bash -/tmp/test_exhaustive.exp # Multiple timing scenarios -/tmp/test_autohighlight.exp # Auto-highlight on typing -``` - -## API - -### ApprovalManager - -```go -// Create new manager -approval := agent.NewApprovalManager() - -// Check if tool/command is allowed -allowed := approval.IsAllowed("bash", map[string]any{"command": "ls -la"}) - -// Request user approval -result, err := approval.RequestApproval("bash", args) - -// Add to allowlist (uses prefix for safe bash commands) -approval.AddToAllowlist("bash", args) - -// Get list of allowed tools/prefixes -tools := approval.AllowedTools() - -// Reset session allowlist -approval.Reset() -``` - -### ApprovalResult - -```go -type ApprovalResult struct { - Decision ApprovalDecision // ApprovalOnce, ApprovalAlways, ApprovalDeny - DenyReason string // Optional reason when denied -} -``` - -### Helper Functions - -```go -// Format denial message for tool result -msg := agent.FormatDenyResult("bash", "too risky") -// -> "User denied execution of bash. Reason: too risky" - -// Format approval result for display -display := agent.FormatApprovalResult("bash", args, result) -// -> "▶ bash: ls -la [Approved] ✓" -``` diff --git a/x/DOUBLE_PRESS_FIX.md b/x/DOUBLE_PRESS_FIX.md deleted file mode 100644 index 90361c659..000000000 --- a/x/DOUBLE_PRESS_FIX.md +++ /dev/null @@ -1,12 +0,0 @@ -# Tool approval double-press fix - -## What happened -The approval popup reads directly from `os.Stdin` in raw mode. Meanwhile the interactive prompt uses the `readline` package, which spun up a background goroutine (`Terminal.ioloop`) that continuously read from stdin and queued runes on a channel. - -When the popup appeared, the first keypress was often consumed by the background goroutine and held in its channel, so the popup never saw it. That forced a second keypress, and the first key would later show up at the next prompt (e.g., a stray `1`). - -## Fix -`readline` now reads from stdin synchronously instead of via a background goroutine. The terminal is not left in raw mode while idle, and `Terminal.Read()` pulls directly from a buffered reader when `Readline()` is active. - -## Files changed -- `readline/readline.go` diff --git a/x/README.md b/x/README.md deleted file mode 100644 index 3c4c6c7a4..000000000 --- a/x/README.md +++ /dev/null @@ -1,331 +0,0 @@ -# Experimental Agent Loop (`x/`) - -This directory contains the experimental agent loop implementation for Ollama. It enables LLMs to use tools (bash commands, web search) in an interactive loop with user approval. - -## Quick Start - -```bash -# Run with experimental agent loop (use a model that supports tools) -ollama run qwen2.5 --experimental - -# Or use --beta alias -ollama run qwen2.5 --beta -``` - -For web search, set your API key: -```bash -export OLLAMA_API_KEY=your_key_here -ollama run qwen2.5 --experimental -``` - -**Note:** The model must support tool calling. If it doesn't, the agent loop will run in chat-only mode: -``` -Note: Model does not support tools - running in chat-only mode -``` - -Models with tool support include: `qwen2.5`, `llama3.1`, `mistral`, `command-r`, etc. - -## Features - -### Built-in Tools - -| Tool | Description | -|------|-------------| -| `bash` | Execute shell commands with 60s timeout | -| `web_search` | Search the web via Ollama's hosted API | - -### Interactive Approval - -Every tool execution requires user approval: - -``` -┌──────────────────────────────────────────────────────────┐ -│ Tool: bash │ -│ Command: ls -la src/ │ -├──────────────────────────────────────────────────────────┤ -│ > 1. Execute once │ -│ 2. Always allow │ -│ 3. Deny: │ -└──────────────────────────────────────────────────────────┘ -↑/↓ navigate, Enter confirm, 1-3 quick, Ctrl+C cancel -``` - -**Input options:** -- `↑`/`↓` - Navigate between options -- `Enter` - Confirm selection -- `1`, `2`, `3` - Quick select -- `Ctrl+C` - Cancel (deny) -- Type any text - Enters deny reason (auto-selects Deny option) -- `Backspace` - Delete from reason -- `Esc` - Clear reason - -### Warning for Commands Outside Project - -Commands that target paths outside the current working directory show a **red warning box**: - -``` -┌────────────────────────────────────────┐ -│ !! OUTSIDE PROJECT !! │ -│ Tool: bash │ -│ Command: cat /etc/passwd │ -├────────────────────────────────────────┤ -│ > 1. Execute once │ -│ 2. Always allow │ -│ 3. Deny: │ -└────────────────────────────────────────┘ -``` - -This alerts you when the model tries to: -- Access absolute paths outside cwd (e.g., `/etc/passwd`) -- Navigate to parent directories (e.g., `../../../etc/passwd`) -- Access home directory paths (e.g., `~/.bashrc`) - -### Prefix-Based Allowlist - -When you select "Always allow" for safe commands (`cat`, `ls`, `grep`, etc.), the system saves the directory prefix rather than the exact command: - -| Command Approved | Saved as Prefix | -|------------------|-----------------| -| `cat src/main.go` | `cat:src/` | -| `ls -la tools/` | `ls:tools/` | -| `grep -r "pattern" api/` | `grep:api/` | - -This means future commands to the same directory are auto-approved. - -**Safe commands** (eligible for prefix matching): -- `cat`, `ls`, `head`, `tail`, `less`, `more` -- `file`, `wc`, `grep`, `find`, `tree`, `stat`, `sed` - -### Auto-Allowed Commands - -These commands run without prompting (zero risk): - -| Category | Commands | -|----------|----------| -| **System info** | `pwd`, `echo`, `date`, `whoami`, `hostname`, `uname` | -| **Git (read-only)** | `git status`, `git log`, `git diff`, `git branch`, `git show` | -| **Package managers** | `npm run`, `npm test`, `bun run`, `uv run`, `yarn run`, `pnpm run` | -| **Build/test** | `go build`, `go test`, `make`, `cargo build`, `cargo test` | - -### Blocked Commands (Denylist) - -Dangerous commands are automatically blocked and return an error to the model: - -| Category | Patterns | -|----------|----------| -| **Destructive** | `rm -rf`, `rm -r`, `mkfs`, `dd`, `shred` | -| **Privilege escalation** | `sudo`, `su`, `chmod 777`, `chown` | -| **Credential access** | `.ssh/id_*`, `.aws/credentials`, `.env`, `*secret*`, `*password*` | -| **Network exfil** | `curl -d`, `curl --data`, `scp`, `rsync`, `nc` | - -When blocked, the model receives: -``` -Command blocked: this command matches a dangerous pattern (rm -rf) and cannot be executed. -If this command is necessary, please ask the user to run it manually. -``` - -### Session Commands - -| Command | Description | -|---------|-------------| -| `/tools` | Show available tools and current approvals | -| `/clear` | Clear conversation history and approvals | -| `/bye` | Exit | -| `/?` | Help | - -## Architecture - -``` -x/ -├── README.md # This file -├── APPROVAL_UX.md # Detailed approval UX documentation -├── DOUBLE_PRESS_FIX.md # Fix for stdin race condition -├── agent/ -│ ├── approval.go # Interactive approval system -│ └── approval_test.go # Tests -├── tools/ -│ ├── registry.go # Tool interface and registry -│ ├── registry_test.go # Tests -│ ├── bash.go # Bash command execution -│ └── websearch.go # Web search via Ollama API -└── cmd/ - └── run.go # Agent loop and interactive session -``` - -## How It Works - -### Agent Loop - -1. User sends a message -2. LLM responds, optionally requesting tool calls -3. For each tool call: - - Check if already approved (exact match or prefix) - - If not, show interactive approval dialog - - Execute tool if approved - - Return result to LLM -4. LLM continues with tool results -5. Repeat until LLM responds without tool calls - -### Tool Execution Flow - -``` -User Message - ↓ -LLM Response (with tool calls) - ↓ -┌─────────────────────────────┐ -│ For each tool call: │ -│ ├─ Check allowlist │ -│ ├─ Request approval (UI) │ -│ ├─ Execute tool │ -│ └─ Collect result │ -└─────────────────────────────┘ - ↓ -Tool Results → LLM - ↓ -LLM Final Response -``` - -## Files Changed (from main) - -### New Files (`x/`) - -| File | Purpose | -|------|---------| -| `x/cmd/run.go` | Agent loop, interactive session, tool execution | -| `x/tools/registry.go` | Tool interface and registry | -| `x/tools/bash.go` | Bash command execution with timeout | -| `x/tools/websearch.go` | Web search using Ollama API | -| `x/agent/approval.go` | Interactive approval UI | -| `x/agent/approval_test.go` | Approval tests | -| `x/tools/registry_test.go` | Registry tests | - -### Modified Files - -| File | Changes | -|------|---------| -| `cmd/cmd.go` | Added `--experimental`/`--beta` flags, routes to `x/cmd` | -| `cmd/interactive.go` | Added `/tools` command info | -| `readline/readline.go` | Fixed stdin race condition (see DOUBLE_PRESS_FIX.md) | - -## API Reference - -### Tool Interface - -```go -type Tool interface { - Name() string - Description() string - Schema() api.ToolFunction - Execute(args map[string]any) (string, error) -} -``` - -### Registry - -```go -// Create registry with built-in tools -registry := tools.DefaultRegistry() - -// Or create custom registry -registry := tools.NewRegistry() -registry.Register(&MyCustomTool{}) - -// Get tool definitions for LLM -apiTools := registry.Tools() - -// Execute a tool call -result, err := registry.Execute(toolCall) -``` - -### Approval Manager - -```go -approval := agent.NewApprovalManager() - -// Check if allowed -if !approval.IsAllowed("bash", args) { - result, err := approval.RequestApproval("bash", args) - if result.Decision == agent.ApprovalAlways { - approval.AddToAllowlist("bash", args) - } -} - -// Get allowed tools/prefixes -allowed := approval.AllowedTools() - -// Reset session -approval.Reset() -``` - -## Configuration - -### Environment Variables - -| Variable | Description | -|----------|-------------| -| `OLLAMA_API_KEY` | Required for `web_search` tool | -| `OLLAMA_HOST` | Ollama server URL (default: http://localhost:11434) | - -### Bash Tool Limits - -| Setting | Value | -|---------|-------| -| Timeout | 60 seconds | -| Max output | 50,000 bytes | - -### Web Search Limits - -| Setting | Value | -|---------|-------| -| Max results | 5 | -| Content truncation | 300 chars per result | - -## Development - -### Running Tests - -```bash -# Run all x/ tests -go test ./x/... - -# Run with verbose output -go test -v ./x/agent/... -go test -v ./x/tools/... -``` - -### Test Program - -An isolated test program for the approval UI is available: - -```bash -cd /tmp/selector_test -go build -o selector_test . -./selector_test -``` - -### Expect Tests - -Automated UI tests using expect: - -```bash -/tmp/test_exhaustive.exp # Multiple input scenarios -/tmp/test_autohighlight.exp # Auto-highlight on typing -``` - -## Known Issues - -### Double-Press Fix - -The approval UI initially had an issue where keystrokes required double-pressing. This was caused by the readline library's background goroutine consuming stdin input. See `DOUBLE_PRESS_FIX.md` for details. - -**Fix:** readline now reads stdin synchronously instead of via a background goroutine. - -## Future Enhancements - -- [ ] More built-in tools (file read/write, HTTP requests) -- [ ] Tool result caching -- [ ] Persistent allowlist across sessions -- [ ] Custom tool loading from config -- [ ] Streaming tool output -- [ ] Tool execution sandboxing