Conversation
- Implemented history management in `internal/history` with session recording, loading, and clearing functionalities. - Created a command execution runner in `internal/runner` that handles instruction execution, retries on failure, and integrates with the history management. - Added tests for both history and runner functionalities to ensure reliability and correctness.
- Implemented a new tools package providing functionalities such as listing directories, reading files, checking command availability, and retrieving system resources. - Added a registry of available tools with descriptions and expected arguments. - Created additional tests for tool execution, safety checks, and environment variable handling. - Included tests for command help retrieval using tldr and man pages. - Ensured proper error handling and output truncation for large results.
There was a problem hiding this comment.
Pull request overview
Adds persistent session history to AI CLI and enables automatic/manual retries of failed command runs by replaying recent execution context back to the model. This introduces a new runner abstraction to centralize instruction execution, history recording, and retry behavior across both single-shot and interactive modes.
Changes:
- Add
internal/historysessions persisted to~/.ai-cli/history.logwith rotation, plus retry-context message building. - Introduce
internal/runnerto run instructions, record history, and retry the last failed session (interactiveretry+ auto-retry-on-failure). - Extend LLM client surface with
ChatWithTraceand persist optional trace/debug metadata; addai historyCLI + new history config keys/flags/docs.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/tools/safety.go | Refactors path containment logic to support symlink-resolved cwd bases. |
| internal/tools/runner.go | Uses a named const for the environment tool name in safety messaging. |
| internal/tools/runner_test.go | Updates mock LLM client to implement the new ChatWithTrace API. |
| internal/shell/detect_test.go | Adds branch coverage for Windows shell detection + scripted version probing tests. |
| internal/runner/runner.go | New runner that executes instructions, records history, and supports retries/auto-retries. |
| internal/runner/runner_test.go | New tests covering retries, history persistence, config actions, and helper behavior. |
| internal/llm/types.go | Adds Response.Explanation plus ChatTrace / ChatResult types for trace capture. |
| internal/llm/prompt.go | Updates system prompt contract to include optional explanation and history config actions. |
| internal/llm/client.go | Adds ChatWithTrace to clients; captures request/response trace metadata. |
| internal/interactive/repl.go | Routes REPL input through the runner; adds history + retry built-ins. |
| internal/interactive/repl_test.go | Simplifies tests to focus on retry-depth parsing (moved logic to runner). |
| internal/interactive/repl_run_test.go | Updates REPL tests to use a runner interface; adds history/retry dispatch coverage. |
| internal/history/history.go | New history log implementation (save/list/load/remove/clear) and retry message builder. |
| internal/history/history_test.go | Comprehensive tests for history persistence, rotation, dedupe, and retry message output. |
| internal/executor/executor.go | Adds captured stdout/stderr + timing to execution results for history context. |
| internal/executor/executor_test.go | Updates executor tests for the new execution result structure. |
| internal/config/config.go | Adds HistoryConfig to config + defaults for history and retry behavior. |
| internal/config/config_test.go | Extends config tests to validate new history defaults and persistence. |
| internal/config/apply.go | Adds set_history action to support model-driven history config updates. |
| internal/config/apply_test.go | Adds coverage for set_history keys and validation. |
| cmd/root.go | Adds retry flags, integrates runner for single-shot + interactive execution, wires REPL history built-in. |
| cmd/root_test.go | Updates interactive stubs to accept a runner interface. |
| cmd/history.go | New ai history command to list/show/remove/clear stored sessions. |
| cmd/config.go | Adds CLI config keys for history settings and their setters/getters. |
| cmd/cmd_test.go | Adds tests for ai history output/flags and new history config keys. |
| README.md | Documents history storage, retry behavior, new flags, and new config keys. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| func applyConfig(resp *llm.Response, cfg *config.Config) error { | ||
| fmt.Printf("Config change: %s %s = %s\n", resp.Action, resp.Key, resp.Value) |
There was a problem hiding this comment.
applyConfig prints resp.Value directly. For actions like set_key / llm_key this will echo secrets to the terminal. Use config.DisplayValue(resp.Action, resp.Key, resp.Value) (as cmd/root.go does) when rendering the value.
| fmt.Printf("Config change: %s %s = %s\n", resp.Action, resp.Key, resp.Value) | |
| displayValue := config.DisplayValue(resp.Action, resp.Key, resp.Value) | |
| fmt.Printf("Config change: %s %s = %s\n", resp.Action, resp.Key, displayValue) |
| case "config": | ||
| fmt.Printf(" config: %s %s = %s\n", exchange.Response.Action, exchange.Response.Key, exchange.Response.Value) | ||
| } |
There was a problem hiding this comment.
showHistory prints config responses using exchange.Response.Value without redaction. If history contains a set_key/llm_key action, ai history show will leak the raw secret. Consider importing internal/config and using config.DisplayValue(action, key, value) (and/or avoid printing values for set_key entirely).
| } | ||
|
|
||
| func (r *Runner) RunInstruction(instruction string) error { | ||
| cwd, _ := os.Getwd() |
There was a problem hiding this comment.
RunInstruction ignores the error from os.Getwd(). If the current directory has been deleted or is otherwise inaccessible, this will silently use an empty cwd in the system prompt and history session. Please handle the error (return it, or fall back to a safe default while surfacing the failure).
| cwd, _ := os.Getwd() | |
| cwd, err := os.Getwd() | |
| if err != nil { | |
| fmt.Fprintf(os.Stderr, "warning: failed to get current working directory: %v\n", err) | |
| cwd = "" | |
| } |
| } | ||
|
|
||
| func (r *Runner) saveSession(session *history.Session) { | ||
| _ = history.Save(session) |
There was a problem hiding this comment.
saveSession discards errors from history.Save, which can make history/retry appear to work while silently failing to persist sessions (e.g., permission or disk-full issues). Consider returning the error to callers or at least logging a warning to stderr so the user knows history wasn't saved.
| _ = history.Save(session) | |
| if err := history.Save(session); err != nil { | |
| fmt.Fprintf(os.Stderr, "Warning: failed to save session history: %v\n", err) | |
| } |
| rootCmd.Flags().BoolVar(&retryOnErrorFlag, "retry-on-error", false, "Automatically send failed commands back to the AI for one-shot retries") | ||
| rootCmd.Flags().IntVar(&retryDepthFlag, "retry-depth", 0, "Override how many recent command results are included in AI retry context") |
There was a problem hiding this comment.
The flag help text says "one-shot retries", but enabling --retry-on-error only sets history.auto_check_on_error=true; the number of retries is still controlled by history.retry_max_attempts (which the user may have set >1). Either update the description to match behavior or force RetryMaxAttempts=1 when this flag is used.
| } | ||
|
|
||
| func withinPath(path, base string) bool { | ||
| return path == base || strings.HasPrefix(path, base+string(os.PathSeparator)) |
There was a problem hiding this comment.
withinPath builds the prefix check as base + string(os.PathSeparator). If base already ends with a separator (e.g., cwdClean=="/" on Unix or "C:\" on Windows), this becomes "//" or "C:\\" and valid paths under the root won't match. Consider normalizing base to not have a trailing separator (except maybe the root itself) or using filepath.Rel-based containment to avoid this edge case.
| return path == base || strings.HasPrefix(path, base+string(os.PathSeparator)) | |
| // Normalize the base path to avoid issues with trailing separators (except for | |
| // canonical roots like "/" or "C:\"). Then ensure either exact equality or that | |
| // path has base as a prefix followed by a path separator. | |
| baseClean := filepath.Clean(base) | |
| if path == baseClean { | |
| return true | |
| } | |
| if len(path) <= len(baseClean) { | |
| return false | |
| } | |
| if !strings.HasPrefix(path, baseClean) { | |
| return false | |
| } | |
| // Ensure we are matching a full path segment: the next character after the | |
| // base prefix must be a path separator. | |
| next := path[len(baseClean)] | |
| return os.IsPathSeparator(next) |
| - set_key: key="llm_key" (sets key on current provider), value="the-key" | ||
| - set_safety: key="always_confirm" or "min_certainty", value="true"/"false" or number` | ||
| - set_safety: key="always_confirm" or "min_certainty", value="true"/"false" or number | ||
| - set_history: key is one of "include_llm_output", "include_debug", "ask_on_error", "auto_check_on_error", "retry_max_attempts", "retry_context_depth"` |
There was a problem hiding this comment.
The set_history line in the prompt ends with an unmatched backtick. That extra character can confuse the model and is inconsistent with the other bullets.
| - set_history: key is one of "include_llm_output", "include_debug", "ask_on_error", "auto_check_on_error", "retry_max_attempts", "retry_context_depth"` | |
| - set_history: key is one of "include_llm_output", "include_debug", "ask_on_error", "auto_check_on_error", "retry_max_attempts", "retry_context_depth" | |
| ` |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 423a8fda2f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| func applyConfig(resp *llm.Response, cfg *config.Config) error { | ||
| fmt.Printf("Config change: %s %s = %s\n", resp.Action, resp.Key, resp.Value) |
There was a problem hiding this comment.
applyConfig prints resp.Value verbatim, so when the model returns a config action like set_key the raw API key is echoed to stdout before confirmation. That leaks credentials into terminal scrollback and any captured logs, and it regresses the prior behavior that used redaction-aware display helpers for sensitive config values.
Useful? React with 👍 / 👎.
| } | ||
| if result != nil { | ||
| if cfg.IncludeLLMOutput { | ||
| exchange.Response = result.Response |
There was a problem hiding this comment.
When history.include_llm_output is enabled (default), this stores the full structured LLM response, including config response value fields. If a user asks the AI to set an API key, the plaintext secret is persisted to ~/.ai-cli/history.log and later retrievable via history commands, which is a credential exposure risk in normal usage.
Useful? React with 👍 / 👎.
|
|
||
| func (r *Runner) shouldRetry(session *history.Session, autoTriggered bool) bool { | ||
| if r.cfg.History.AutoCheckOnError { | ||
| if r.cfg.History.RetryMaxAttempts <= 0 || session.RetryCount < r.cfg.History.RetryMaxAttempts { |
There was a problem hiding this comment.
This treats retry_max_attempts <= 0 as “always retry,” but config setters explicitly allow retry_max_attempts=0; with auto_check_on_error=true, any repeatedly failing command path recurses through runSession -> retrySession without a stop condition. In failure-heavy scenarios this can spin indefinitely and eventually exhaust stack/memory.
Useful? React with 👍 / 👎.
|
@copilot open a new pull request to apply changes based on the comments in this thread |
|
@kriserickson I've opened a new pull request, #47, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@copilot open a pull request to handle this issue: `internal/runner/runner.go Stop unbounded auto-retry when max attempts is zeroThis treats retry_max_attempts <= 0 as “always retry,” but config setters explicitly allow retry_max_attempts=0; with auto_check_on_error=true, any repeatedly failing command path recurses through runSession -> retrySession without a stop condition. In failure-heavy scenarios this can spin indefinitely and eventually exhaust stack/memory.` |
|
@kriserickson I've opened a new pull request, #48, to work on those changes. Once the pull request is ready, I'll request review from you. |
Adds history, automatically retries failed command or allows retrying of commands.