address comments

This commit is contained in:
ParthSareen 2026-01-05 22:02:52 -08:00
parent dd74829c27
commit 35d10ba88a
5 changed files with 3 additions and 602 deletions

View File

@ -518,10 +518,8 @@ func RunHandler(cmd *cobra.Command, args []string) error {
return generateEmbedding(cmd, name, opts.Prompt, opts.KeepAlive, truncate, dimensions) return generateEmbedding(cmd, name, opts.Prompt, opts.KeepAlive, truncate, dimensions)
} }
// Check for experimental/beta flag // Check for experimental flag
experimental, _ := cmd.Flags().GetBool("experimental") isExperimental, _ := cmd.Flags().GetBool("experimental")
beta, _ := cmd.Flags().GetBool("beta")
isExperimental := experimental || beta
if interactive { if interactive {
if err := loadOrUnloadModel(cmd, &opts); err != nil { 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("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().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().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("experimental", false, "Enable experimental agent loop with tools")
runCmd.Flags().Bool("beta", false, "Alias for --experimental")
stopCmd := &cobra.Command{ stopCmd := &cobra.Command{
Use: "stop MODEL", Use: "stop MODEL",

View File

@ -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, " /?, /help Help for a command")
fmt.Fprintln(os.Stderr, " /? shortcuts Help for keyboard shortcuts") 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, "")
fmt.Fprintln(os.Stderr, "Use \"\"\" to begin a multi-line message.") 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"): case strings.HasPrefix(line, "/exit"), strings.HasPrefix(line, "/bye"):
return nil 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, "/"): case strings.HasPrefix(line, "/"):
args := strings.Fields(line) args := strings.Fields(line)
isFile := false isFile := false

View File

@ -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] ✓"
```

View File

@ -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`

View File

@ -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