feat(chat,agent): improve usability with summarization, new tools, and readline#128
feat(chat,agent): improve usability with summarization, new tools, and readline#128toasterbook88 wants to merge 4 commits into
Conversation
- Bump version to 0.10.3 - Refresh docs/current-state.md for v0.10.3 - Update summary golden files for version bump - Add docs/roadmap-status.md with final v9 status - Add CHANGELOG entry for v0.10.3 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The agent relies on tool calling, but choosePreferredModel() fell back to alphabetically first installed model which was often gemma3n:e2b. That model does not support /api/chat tools, producing a 400 Bad Request. - Expand recommendedLocalModels to include tool-capable variants. - Add toolCapablePrefixes list of known tool-capable families. - Add nonToolFamilies exclusion list to skip embedding/vision variants. - Update choosePreferredModel to prefer tool-capable families over all others when falling back to any installed model. - Improve 400 error message to suggest tool-capable alternatives. - Add tests for all new paths. All existing tests continue to pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…d readline This commit addresses critical usability issues discovered during evaluation of axis chat and axis agent commands: - Tool outputs are now human-readable summaries instead of raw JSON, preventing local LLM hangs on large cluster snapshots. - Added 4 new tools: axis_summary, axis_reservations, read_file, list_directory. - Added tool execution transparency: prints tool calls and one-line result summaries to stderr. - Added conversation persistence with --resume flag for both chat and agent. - Replaced primitive bufio.Scanner REPLs with readline-based editors supporting arrow keys, history, and ctrl+c graceful exit. - Added path validation to file tools to prevent directory traversal. - Updated isReadOnlyTool to include new read-only tools. - Added tests for new tools and persistence. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates AXIS to v0.10.3, introducing conversation persistence with a new --resume flag and an enhanced REPL experience using the readline library. The agent's capabilities are expanded with new tools for file and directory access, and existing tools are refactored to return human-readable summaries instead of raw JSON to optimize LLM context usage. Furthermore, the model auto-selection logic now prioritizes tool-capable families to avoid 400 errors. Review feedback highlights several areas for improvement, including the need for LimitReader to prevent OOM when reading large files, resolving symbolic links during path validation to prevent directory traversal, ensuring UTF-8 safety during string truncation, and hardening file permissions for stored history.
| if err != nil { | ||
| return "", err | ||
| } | ||
| data, err := os.ReadFile(clean) |
There was a problem hiding this comment.
Using os.ReadFile on a path provided by the user (via the LLM) poses a risk of Out-Of-Memory (OOM) crashes if the file is extremely large. Since the output is truncated to 8000 characters anyway, it is safer to use a LimitReader to avoid reading the entire file into memory.
| func validateToolPath(p string) (string, error) { | ||
| cwd, err := os.Getwd() | ||
| if err != nil { | ||
| return "", fmt.Errorf("cannot determine working directory: %w", err) | ||
| } | ||
| clean := filepath.Clean(p) | ||
| if !filepath.IsAbs(clean) { | ||
| clean = filepath.Join(cwd, clean) | ||
| } | ||
| // Ensure the resolved path is within cwd. | ||
| rel, err := filepath.Rel(cwd, clean) | ||
| if err != nil { | ||
| return "", fmt.Errorf("invalid path %q: %w", p, err) | ||
| } | ||
| if strings.HasPrefix(rel, "..") { | ||
| return "", fmt.Errorf("path %q escapes working directory", p) | ||
| } | ||
| return clean, nil | ||
| } |
There was a problem hiding this comment.
The current path validation is lexical and does not account for symbolic links. An attacker could potentially use a symlink within the working directory that points to a sensitive file outside of it. To properly prevent directory traversal, you should resolve symlinks using filepath.EvalSymlinks before performing the bounds check.
| content := string(data) | ||
| const maxFileSize = 8000 | ||
| if len(content) > maxFileSize { | ||
| content = content[:maxFileSize] + "\n... [truncated to 8000 chars]" |
There was a problem hiding this comment.
Slicing a string by byte index (e.g., content[:maxFileSize]) is unsafe for UTF-8 encoded strings as it can split a multi-byte character. Consider using the truncate helper function (once updated to be UTF-8 safe) to ensure valid boundaries.
| for _, e := range entries { | ||
| name := e.Name() | ||
| if e.IsDir() { | ||
| name += "/" | ||
| } | ||
| b.WriteString(name + "\n") | ||
| } |
There was a problem hiding this comment.
The directory listing is not truncated. If a directory contains a very large number of entries, this tool will return an excessively long string, which could exceed the LLM's context window. Per the general rules, long lists should be truncated with an indication of the remaining count.
References
- When displaying a list of items in a user-facing message (e.g., warnings, logs), truncate the list if it can become excessively long and indicate that it has been truncated, for example by showing a count of the remaining items.
| func truncate(s string, maxLen int) string { | ||
| if len(s) <= maxLen { | ||
| return s | ||
| } | ||
| if maxLen <= 3 { | ||
| return s[:maxLen] | ||
| } | ||
| return s[:maxLen-3] + "..." | ||
| } |
There was a problem hiding this comment.
Slicing a string by byte index is unsafe for UTF-8 encoded strings as it can split a multi-byte character. It is better to convert the string to a slice of runes to find a safe truncation point.
|
|
||
| // PersistPath returns the default path for conversation history files. | ||
| func PersistPath(name string) string { | ||
| home, _ := os.UserHomeDir() |
There was a problem hiding this comment.
The error from os.UserHomeDir() is ignored. If the home directory cannot be determined, the history will be saved in a .axis directory relative to the current working directory, which is likely unintended.
| if err := os.MkdirAll(filepath.Dir(path), 0755); err != nil { | ||
| return fmt.Errorf("create history directory: %w", err) | ||
| } | ||
| if err := os.WriteFile(path, data, 0644); err != nil { |
There was a problem hiding this comment.
The conversation history files and their parent directory are created with broad permissions (0755 and 0644). Since these files may contain sensitive information, it is recommended to restrict access to the current user only using 0700 for directories and 0600 for files.
- read_file: use io.LimitReader instead of os.ReadFile to prevent OOM - validateToolPath: resolve symlinks with filepath.EvalSymlinks before directory traversal bounds check - read_file: UTF-8 safe truncation using rune-based truncateRune helper - list_directory: truncate output to max 100 entries with remaining count - summarize.go/truncate: operate on runes instead of bytes for UTF-8 safety - persist.go: handle os.UserHomeDir() error; restrict permissions to 0700/0600 - Guard all PersistPath usages against empty path (fallback to no persistence) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 participant
This PR addresses critical usability issues discovered during evaluation of axis chat and axis agent commands:
Changes
Tool Output Summarization
New Tools (4)
Execution Transparency
Conversation Persistence
Readline REPL
Safety
Testing
Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com