Skip to content

Conversation

@beettlle
Copy link
Contributor

@beettlle beettlle commented Feb 1, 2026

Second try at Issue #128 , new implementation can use tools and was used to address 7aa92bd .

@wesm
Copy link
Collaborator

wesm commented Feb 1, 2026

I'll take a closer look at this when I can, but your branch contains a number of changes (such as additions to .roborev.toml) that need to be removed, and I'll have to review whether the AGENTS.md changes make sense. Either way AGENTS.md/CLAUDE.md changes belong in a separate PR

@beettlle
Copy link
Contributor Author

beettlle commented Feb 2, 2026

Sorry, I missed those files when I was cleaning up. Files removed.

@wesm
Copy link
Collaborator

wesm commented Feb 4, 2026

Just letting you know that I see this and I got flooded with stuff related to msgvault this week, but I will have a close look at this when I can! I want to try out the new Qwen open weights model, too, so that is a good excuse.

@beettlle
Copy link
Contributor Author

beettlle commented Feb 8, 2026

No rush, I'm still working out some hiccups. I've merged back release 0.25.2 to bring in some of the new features.

I've been working on trying to get Ollama to hallucinate less, so far I've had good results with the following tweaks to the models. Feedback appreciated.

@wesm
Copy link
Collaborator

wesm commented Feb 8, 2026

Your branch has become a bit of a mess. You should ask Claude to clean it up and squash into a single commit rebased on the main branch.

@beettlle
Copy link
Contributor Author

beettlle commented Feb 8, 2026

ACK, I'm used to making all the changes and keeping the PR a bit messy, and doing a squash/merge at the end. Changes made.

@wesm
Copy link
Collaborator

wesm commented Feb 8, 2026

Still too messy for me, I see my own work in your branch which is a red flag for review

@wesm
Copy link
Collaborator

wesm commented Feb 8, 2026

If we could take a step back I’m still a bit stuck on the premise. Ollama is not a coding agent, so my prior would be that ollama directly looking at code is going to be much worse than say opencode+ollama or aider+ollama. Does that align with your experience?

- Add Ollama agent (internal/agent/ollama*.go) with HTTP API, tools, availability check
- Add config: OllamaBaseURL, OllamaModel, ResolveOllamaBaseURL
- Wire Ollama in daemon worker, main (review), fix, refine, analyze
- Add stream formatter support for Ollama NDJSON
- Add NormalizeOllamaOutput for TUI display
- Add Modelfiles and README docs
- Tests for agent, tools, config, streamfmt, normalize

Co-authored-by: Cursor <cursoragent@cursor.com>
@roborev-ci
Copy link

roborev-ci bot commented Feb 9, 2026

roborev: Combined Review

Synthesis unavailable. Showing raw review outputs.

Agent: codex | Type: security | Status: done

Findings

  1. Severity: High. File: internal/agent/ollama.go:200-260, internal/agent/ollama_tools.go:70-95, internal/agent/ollama_tools.go:247-260. Description: The agentic loop executes model-supplied tool calls automatically and exposes a Bash tool that runs sh -c with an arbitrary command string. If agentic is enabled (or AllowUnsafeAgents is set), a prompt-injection in repo content or a malicious job can trigger arbitrary command execution with daemon privileges, leading to RCE and data exfiltration. Suggested remediation: Gate Bash behind an explicit, separate config flag or per-command user approval, restrict to an allowlist of safe commands, or run tools in a sandboxed/low-privilege environment. Prefer argv-based execution instead of sh -c when possible.

  2. Severity: Medium. File: internal/agent/ollama_tools.go:18-57, internal/agent/ollama_tools.go:98-244, internal/agent/ollama_tools.go:142-195. Description: safePathUnderRoot relies on Abs/Rel and does not resolve symlinks. A repo-controlled symlink can point outside the repo and bypass the root check, allowing Read/Edit/Write/Grep to access or modify arbitrary filesystem paths. This enables path traversal/TOCTOU and potential secret exfiltration. Suggested remediation: Resolve symlinks for both repo root and target and verify the real path is within the real root. For writes, reject any path with symlinked parent components using os.Lstat (or O_NOFOLLOW where supported). Add tests for symlink escape.

Summary
This commit adds Ollama agent support with a tool-execution loop, new config for base URL/model, and new Modelfiles. The most significant security risks are arbitrary command execution via the Bash tool in agentic mode and symlink-based path traversal in the tool path validation.

Open Questions

  1. Is it intended that AllowUnsafeAgents alone can enable Bash execution for Ollama reviews, or should both AllowUnsafeAgents and an explicit job-level agentic flag be required?

Test Gaps

  1. No tests cover symlink escape for Read/Edit/Write/Grep.
  2. No tests assert that Bash remains disabled when agentic mode is off.
Agent: gemini | Type: security | Status: done

I need to analyze the code changes introduced by commit 1424c84 for
potential security vulnerabilities. Since the diff is large, I will retrieve it using git diff.

It appears I've made an error in tool usage. I intended to retrieve the git diff via the codebase_invest igator sub-agent, not directly. I will correct this by invoking codebase_investigator for the diff.
My apologies
for the confusion regarding available tools. I now understand that codebase_investigator is an accessible sub-agent and should be called as a function. I will proceed with using codebase_investigator() to retrieve the git diff for the provided commit range.
I apologize for my persistent misunderstanding of tool availability. Despite re-reading the instructions, codebase_investigator appears unavailable, and I'm being redirected to other tools. The documentation is conflicting, stating sub-agents are tools of the same name, yet codebase_investigator isn't directly callable. I'll attempt to list_directory to locate any relevant files or scripts to clarify its function and proper
usage.
I've identified a critical limitation: I lack the run_shell_command tool and cannot directly execute git diff or utilize
codebase_investigator despite its mention as a sub-agent starter. Since none of my available tools (list_directory, read_file, search_file_content, glob, save_memory, google_web_search, write_todos, activate_skill) can
retrieve the necessary code changes, I am unable to perform the requested security review. I must inform the user of this inability.
I'm facing a critical blocker: I cannot access the necessary git diff to perform the security review
. Despite contradictory instructions regarding tool availability and the nature of codebase_investigator, none of the tools explicitly declared to me (list_directory, read_file, search_file_content, glob, save_memory, google_web_search, write_todos, activate_skill) facilitate
retrieving a git diff. Since I cannot obtain the required code changes, I am unable to proceed with the review and must inform the user.
I cannot perform the requested security code review.
My available tools lack the capability to execute shell commands like git diff, which is essential to access the code changes for review. I am unable to proceed.
I cannot conduct the security code review. I lack the necessary tools, specifically a shell command execution mechanism, to retrieve
the git diff for the commit range. My available tools are insufficient to access the required code changes.

Agent: codex | Type: review | Status: done

Summary
Adds Ollama agent support (HTTP API client, tool execution, availability checks), integrates base URL/model resolution across CLI/daemon flows, updates stream formatting/normalization for Ollama, and adds tests and Modelfiles.

Issues

  1. Mediuminternal/config/config.go (DefaultConfig, ResolveOllamaBaseURL): DefaultConfig() sets OllamaBaseURL to a non-empty default, and ResolveOllamaBaseURL always prefers config over OLLAMA_HOST. Because LoadGlobal() always starts from DefaultConfig(), OLLAMA_HOST is effectively ignored in real usage unless the config file explicitly sets ollama_base_url to empty (which isn’t possible with a plain string). This contradicts the stated precedence and the tests that assume env fallback.
    Suggested fix: make OllamaBaseURL optional (e.g., *string) or keep it empty in DefaultConfig() and only apply the default inside ResolveOllamaBaseURL when neither config nor env is set.

  2. Medium (Security)internal/agent/ollama_tools.go (safePathUnderRoot, executeRead/executeWrite/executeEdit/executeGrep): path checks only use filepath.Abs/Rel and do not resolve symlinks. A symlink inside the repo can point outside, allowing reads/writes beyond the repo boundary when tools are used.
    Suggested fix: EvalSymlinks (or Lstat + reject symlinks) for both root and target, and verify the resolved path stays under the resolved repo root.

  3. Lowinternal/agent/ollama_tools.go (executeGrep): the pattern is passed as a positional argument without -e or --. Patterns starting with - will be parsed as flags, producing incorrect results or failures.
    Suggested fix: change to grep ... -e pattern or insert -- before the pattern.

  4. Lowinternal/agent/ollama_tools.go (executeGrep): CombinedOutput() reads the entire grep output into memory before truncating to 500 lines. For large repos or broad patterns, this can spike memory.
    Suggested fix: stream stdout via StdoutPipe() and stop after N lines.

  5. Low (Regression/UX)internal/agent/ollama.go + internal/daemon/normalize.go: the Ollama parser streams raw NDJSON lines to the output, and NormalizeOllamaOutput doesn’t parse NDJSON into assistant text (only handles [Tool: ...]). In daemon TUI this will display raw JSON instead of readable text.
    Suggested fix: parse the NDJSON in the normalizer (similar to streamfmt) or have the agent write decoded content lines to the output stream.

Testing gaps

  1. Mediuminternal/config/config_test.go: tests for ResolveOllamaBaseURL cover env fallback with cfg == nil or empty, but not the real LoadGlobal() path where defaults are pre-filled. Add a test that uses LoadGlobal() (or a config initialized via DefaultConfig) to confirm OLLAMA_HOST is honored.
Agent: gemini | Type: review | Status: done

run_shell_command("git diff f53d70bef623b99f20d373022060f0b6de38a7ad..1424c849ede29112abb331ec8d5 ec4fdcf8a0b7d")

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants