From bb59279705fc4f31c2d8b907f1f1b8498edf6228 Mon Sep 17 00:00:00 2001 From: can1357 Date: Fri, 23 Jan 2026 12:31:40 +0100 Subject: [PATCH] fix(coding-agent): fixed tool output bugs and documented patterns - Fixed file descriptor leak in OutputSink when write fails - Fixed isError flag not propagating to UI for grouped tool calls - Fixed Python streaming updates during cell execution (missing onChunk) - Fixed ls icon detection broken by age suffix in entry names - Added Tool Output Infrastructure section to DEVELOPMENT.md --- packages/coding-agent/DEVELOPMENT.md | 103 +++++++++++++++++- .../coding-agent/scripts/migrate-sessions.sh | 93 ---------------- .../src/session/streaming-output.ts | 3 + packages/coding-agent/src/tools/ls.ts | 12 +- .../coding-agent/src/tools/output-meta.ts | 7 +- packages/coding-agent/src/tools/python.ts | 9 +- 6 files changed, 122 insertions(+), 105 deletions(-) delete mode 100755 packages/coding-agent/scripts/migrate-sessions.sh diff --git a/packages/coding-agent/DEVELOPMENT.md b/packages/coding-agent/DEVELOPMENT.md index fa1ad708f..60e22d5e1 100644 --- a/packages/coding-agent/DEVELOPMENT.md +++ b/packages/coding-agent/DEVELOPMENT.md @@ -242,7 +242,7 @@ src/ │ ├── session-manager.ts # SessionManager class - JSONL persistence │ ├── session-storage.ts # Session storage utilities │ ├── storage-migration.ts # Storage migration -│ ├── streaming-output.ts # Streaming output handling +│ ├── streaming-output.ts # OutputSink with spill-to-disk for large outputs │ └── compaction/ # Context compaction system │ └── index.ts # Compaction logic, summary generation @@ -276,8 +276,8 @@ src/ │ ├── grep.ts # Content search (regex/literal) │ ├── ls.ts # Directory listing │ ├── notebook.ts # Jupyter notebook editing -│ ├── output-meta.ts # Output metadata -│ ├── output-utils.ts # Output utilities +│ ├── output-meta.ts # OutputMetaBuilder, wrapToolWithMetaNotice +│ ├── output-utils.ts # TailBuffer, allocateOutputArtifact helpers │ ├── path-utils.ts # Path resolution utilities │ ├── python.ts # Python tool (delegates to ipy/) │ ├── read.ts # File reading (text and images) @@ -287,7 +287,7 @@ src/ │ ├── ssh.ts # SSH tool (delegates to ssh/) │ ├── todo-write.ts # Todo management │ ├── tool-errors.ts # Tool error types -│ ├── tool-result.ts # Tool result utilities +│ ├── tool-result.ts # ToolResultBuilder fluent API │ ├── truncate.ts # Output truncation utilities │ └── write.ts # File writing @@ -442,6 +442,99 @@ Unified extension system that discovers and loads capabilities from multiple sou See [docs/extensions.md](docs/extensions.md) for full documentation. +### Tool Output Infrastructure (tools/) + +Standardized system for handling tool outputs with truncation, streaming, and artifact storage. + +#### Core Components + +**OutputSink** (`session/streaming-output.ts`): Line-buffered output collector with automatic spill-to-disk. + +- Tracks total lines/bytes as data flows through +- Spills to artifact file when memory threshold exceeded +- Produces `OutputSummary` with truncation metadata +- Used by executors (bash, python, ssh) for streaming command output + +**TailBuffer** (`tools/output-utils.ts`): Simple rolling buffer keeping the last N bytes. + +- Used for UI preview during streaming +- Handles UTF-8 boundaries correctly +- Lightweight alternative to OutputSink for non-spilling tools + +**ToolResultBuilder** (`tools/tool-result.ts`): Fluent builder for constructing `AgentToolResult`. + +- Chains `.text()`, `.truncation*()`, `.limits()`, `.sourcePath()` methods +- Automatically populates `details.meta` with structured metadata +- Produces well-formed result via `.done()` + +**OutputMetaBuilder** (`tools/output-meta.ts`): Fluent builder for `OutputMeta` structure. + +- Methods for truncation info, limits, source paths, diagnostics +- Accepts `TruncationResult` (from truncate.ts) or `OutputSummary` (from OutputSink) +- Returns `undefined` if no metadata to report (empty case) + +**wrapToolWithMetaNotice** (`tools/output-meta.ts`): Tool wrapper applied to all built-in tools. + +- Automatically appends truncation/limit notices from `details.meta` +- Catches exceptions and renders them via `ToolError.render()` + +#### Standard Pattern for Streaming Tools + +Tools that produce potentially large streaming output (bash, python, ssh): + +```typescript +async execute(...): Promise> { + // 1. Allocate artifact path for full output storage + const { artifactPath, artifactId } = await allocateOutputArtifact(this.session, "toolname"); + + // 2. Create tail buffer for UI preview + const tailBuffer = createTailBuffer(DEFAULT_MAX_BYTES); + + // 3. Execute with streaming callback + const result = await executeCommand({ + artifactPath, + artifactId, + onChunk: (chunk) => { + tailBuffer.append(chunk); + onUpdate?.({ + content: [{ type: "text", text: tailBuffer.text() }], + details: {}, + }); + }, + }); + + // 4. Build result with truncation metadata + return toolResult({}) + .text(result.output) + .truncationFromSummary(result, { direction: "tail" }) + .done(); +} +``` + +#### Standard Pattern for Non-Streaming Tools + +Tools that produce output in one shot (grep, find, ls): + +```typescript +async execute(...): Promise> { + const { items, limitReached } = await doWork(); + const output = formatItems(items); + + return toolResult({}) + .text(output) + .limits({ resultLimit: limitReached ? effectiveLimit : undefined }) + .done(); +} +``` + +#### Key Principles + +1. **Always allocate artifact before execution** — ensures path exists for spill +2. **Pass onChunk to OutputSink/executor** — required for live UI updates during streaming +3. **Use ToolResultBuilder for all results** — ensures consistent metadata structure +4. **Close file sinks on error** — prevent descriptor leaks in failure paths +5. **Truncation direction matters** — `"tail"` for command output (show recent), `"head"` for file reads (show beginning) + ## Development Workflow ### Running in Development @@ -514,6 +607,8 @@ Tools like `fd` and `rg` are auto-downloaded to `~/.omp/bin/` (migrated from `~/ 3. Add to `BUILTIN_TOOLS` map in `tools/index.ts` 4. Add tool prompt template to `prompts/tools/` if needed 5. Tool will automatically be included in system prompt +6. Use `ToolResultBuilder` for results and `OutputMeta` for truncation/limit metadata (see "Tool Output Infrastructure" section) +7. For streaming tools: use `allocateOutputArtifact()` + `createTailBuffer()` pattern with `onChunk` callback ### Adding a New Hook Event diff --git a/packages/coding-agent/scripts/migrate-sessions.sh b/packages/coding-agent/scripts/migrate-sessions.sh deleted file mode 100755 index 89f6ab493..000000000 --- a/packages/coding-agent/scripts/migrate-sessions.sh +++ /dev/null @@ -1,93 +0,0 @@ -#!/bin/bash -# -# Migrate sessions from ~/.omp/agent/*.jsonl to proper session directories. -# This fixes sessions created by the bug in v0.30.0 where sessions were -# saved to ~/.omp/agent/ instead of ~/.omp/agent/sessions//. -# -# Usage: ./migrate-sessions.sh [--dry-run] -# - -set -e - -AGENT_DIR="${OMP_AGENT_DIR:-$HOME/.omp/agent}" -DRY_RUN=false - -if [[ "$1" == "--dry-run" ]]; then - DRY_RUN=true - echo "Dry run mode - no files will be moved" - echo -fi - -# Find all .jsonl files directly in agent dir (not in subdirectories) -shopt -s nullglob -files=("$AGENT_DIR"/*.jsonl) -shopt -u nullglob - -if [[ ${#files[@]} -eq 0 ]]; then - echo "No session files found in $AGENT_DIR" - exit 0 -fi - -echo "Found ${#files[@]} session file(s) to migrate" -echo - -migrated=0 -failed=0 - -for file in "${files[@]}"; do - filename=$(basename "$file") - - # Read first line and extract cwd using jq - if ! first_line=$(head -1 "$file" 2>/dev/null); then - echo "SKIP: $filename - cannot read file" - ((failed++)) - continue - fi - - # Parse JSON and extract cwd - if ! cwd=$(echo "$first_line" | jq -r '.cwd // empty' 2>/dev/null); then - echo "SKIP: $filename - invalid JSON" - ((failed++)) - continue - fi - - if [[ -z "$cwd" ]]; then - echo "SKIP: $filename - no cwd in session header" - ((failed++)) - continue - fi - - # Encode cwd: remove leading slash, replace slashes with dashes, wrap with -- - encoded=$(echo "$cwd" | sed 's|^/||' | sed 's|[/:\\]|-|g') - encoded="--${encoded}--" - - target_dir="$AGENT_DIR/sessions/$encoded" - target_file="$target_dir/$filename" - - if [[ -e "$target_file" ]]; then - echo "SKIP: $filename - target already exists" - ((failed++)) - continue - fi - - echo "MIGRATE: $filename" - echo " cwd: $cwd" - echo " to: $target_dir/" - - if [[ "$DRY_RUN" == false ]]; then - mkdir -p "$target_dir" - mv "$file" "$target_file" - fi - - ((migrated++)) - echo -done - -echo "---" -echo "Migrated: $migrated" -echo "Skipped: $failed" - -if [[ "$DRY_RUN" == true && $migrated -gt 0 ]]; then - echo - echo "Run without --dry-run to perform the migration" -fi diff --git a/packages/coding-agent/src/session/streaming-output.ts b/packages/coding-agent/src/session/streaming-output.ts index 5034ebd9f..89abe0977 100644 --- a/packages/coding-agent/src/session/streaming-output.ts +++ b/packages/coding-agent/src/session/streaming-output.ts @@ -119,6 +119,9 @@ export class OutputSink { }; await this.#file.sink.write(this.#buffer); } catch { + try { + await this.#file?.sink?.end(); + } catch {} this.#file = undefined; return null; } diff --git a/packages/coding-agent/src/tools/ls.ts b/packages/coding-agent/src/tools/ls.ts index 0d6d00eb4..06e75596f 100644 --- a/packages/coding-agent/src/tools/ls.ts +++ b/packages/coding-agent/src/tools/ls.ts @@ -50,6 +50,8 @@ export interface LsToolOptions { export interface LsToolDetails { entries?: string[]; + /** Raw entry names (with / suffix for dirs) without age suffix, for icon detection */ + rawEntries?: string[]; dirCount?: number; fileCount?: number; truncation?: TruncationResult; @@ -125,6 +127,7 @@ export class LsTool implements AgentTool { // Format entries with directory indicators const results: string[] = []; + const rawResults: string[] = []; let dirCount = 0; let fileCount = 0; @@ -153,6 +156,7 @@ export class LsTool implements AgentTool { // Format: "name/ (2d ago)" or "name (just now)" const line = age ? `${entry}${suffix} (${age})` : entry + suffix; results.push(line); + rawResults.push(entry + suffix); } if (results.length === 0) { @@ -166,6 +170,7 @@ export class LsTool implements AgentTool { const output = truncation.content; const details: LsToolDetails = { entries: results, + rawEntries: rawResults, dirCount, fileCount, }; @@ -239,9 +244,11 @@ export const lsToolRenderer = { } let entries: string[] = details?.entries ? [...details.entries] : []; + let rawEntries: string[] | undefined = details?.rawEntries; if (entries.length === 0) { const rawLines = textContent.split("\n").filter((l: string) => l.trim()); entries = rawLines.filter((line) => !/^\[.*\]$/.test(line.trim())); + rawEntries = undefined; // Can't reliably extract raw paths from text } if (entries.length === 0) { @@ -293,10 +300,11 @@ export const lsToolRenderer = { for (let i = 0; i < maxEntries; i++) { const entry = entries[i]; + const rawEntry = rawEntries?.[i] ?? entry; const isLast = i === maxEntries - 1 && !hasMoreEntries && !hasTruncation; const branch = isLast ? uiTheme.tree.last : uiTheme.tree.branch; - const isDir = entry.endsWith("/"); - const entryPath = isDir ? entry.slice(0, -1) : entry; + const isDir = rawEntry.endsWith("/"); + const entryPath = isDir ? rawEntry.slice(0, -1) : rawEntry; const lang = isDir ? undefined : getLanguageFromPath(entryPath); const entryIcon = isDir ? uiTheme.fg("accent", uiTheme.icon.folder) diff --git a/packages/coding-agent/src/tools/output-meta.ts b/packages/coding-agent/src/tools/output-meta.ts index 031c72079..1b046c7fc 100644 --- a/packages/coding-agent/src/tools/output-meta.ts +++ b/packages/coding-agent/src/tools/output-meta.ts @@ -413,11 +413,8 @@ export function wrapToolWithMetaNotice>(tool: try { result = await originalExecute(toolCallId, params, signal, onUpdate, context); } catch (e) { - // Use ToolError.render() if available - return { - content: [{ type: "text", text: renderError(e) }], - details: {}, - }; + // Re-throw with formatted message so agent-loop sets isError flag + throw new Error(renderError(e)); } // Append notices from meta diff --git a/packages/coding-agent/src/tools/python.ts b/packages/coding-agent/src/tools/python.ts index 2bc798b6c..d0c334938 100644 --- a/packages/coding-agent/src/tools/python.ts +++ b/packages/coding-agent/src/tools/python.ts @@ -254,7 +254,14 @@ export class PythonTool implements AgentTool { const sessionFile = this.session.getSessionFile?.() ?? undefined; const artifactsDir = this.session.getArtifactsDir?.() ?? undefined; const { artifactPath, artifactId } = await allocateOutputArtifact(this.session, "python"); - outputSink = new OutputSink({ artifactPath, artifactId }); + outputSink = new OutputSink({ + artifactPath, + artifactId, + onChunk: (chunk) => { + appendTail(chunk); + pushUpdate(); + }, + }); const sessionId = sessionFile ? `session:${sessionFile}:cwd:${commandCwd}` : `cwd:${commandCwd}`; const baseExecutorOptions: Omit = { cwd: commandCwd,