feat(local-ai): add editable Ollama server URL with connection test#2210
Merged
graycyrus merged 12 commits intoMay 20, 2026
Merged
Conversation
Merges the split is_path_allowed/is_resolved_path_allowed API into a single validate_path() that resolves symlinks before checking workspace boundaries and forbidden paths. Adds validate_parent_path() for write operations where the target file may not yet exist. Two callers (image_info.rs, cron/scheduler.rs) were missing the resolved check entirely — image_info.rs could be used to exfiltrate files via a symlink inside the workspace. Closes tinyhumansai#1927
…te before create_dir_all - In validate_path/validate_parent_path, switch from string starts_with to path-component-aware comparison for forbidden_paths. - Resolve relative forbidden entries against the workspace root so entries like "secrets" correctly block workspace/secrets/ even after canonicalization. - Skip absolute forbidden entries that are ancestors of the workspace root (e.g. /tmp when workspace is /tmp/…) — the workspace containment check already guarantees the resolved path is safe. - validate_parent_path now walks up to the deepest existing ancestor before canonicalizing, so it works without requiring the parent directory to exist. - file_write and csv_export now call validate_parent_path BEFORE create_dir_all, then create directories at the validated canonical location. This prevents a symlinked path component from causing directory creation outside the workspace before the security check fires. Fixes 25 failing filesystem tests (false-positive forbidden-path rejections when workspace is under /tmp) and closes the pre-create-dir_all attack surface.
…forbidden-entry symlink regression test - image_info.rs: remove redundant "Path not allowed: " prefix — validate_path already returns a complete user-facing error string. - policy_tests.rs: add validate_path_blocks_symlink_to_relative_forbidden_entry to lock in the be29669 fix where relative forbidden entries (e.g. "secrets") were not resolved against the workspace root and could be bypassed via a symlink pointing into the forbidden directory.
Lines 888-896 of policy.rs were uncovered — the forbidden_paths loop inside validate_parent_path had no test. Add validate_parent_path_blocks_forbidden_path to assert that writing a new file into a relative-forbidden directory is rejected.
Wires config.local_ai.base_url into the UI for the first time: previously
the field existed in core but was never written from the frontend and was
ignored by ollama_base_url() which only read env vars.
- Add ollama_base_url_from_config(config) with priority chain:
config.local_ai.base_url > env vars > default; update bootstrap/health
check/infer/vision-embed callsites so an external server is used when
configured without requiring a local binary
- Add validate_ollama_url() (Rust + TS mirrors) for scheme/host/creds
validation with path normalisation
- Register openhuman.local_ai_test_connection RPC that probes /api/tags
with a 3-second timeout and returns {reachable, error, models_count}
- Add Ollama Server URL section to LocalModelDebugPanel/ModelStatusSection:
editable input, inline validation, Test Connection button, Save/Reset,
seeds from persisted config on mount via openhumanGetConfig()
- Add i18n keys (localModel.ollamaServer.*) across all locale chunks
- 4 new Rust tests for test_ollama_connection; 8 new Vitest tests for
URL input UX; 12 tests for ollamaUrlValidation util
Closes tinyhumansai#2159
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds editable Ollama Server URL end-to-end: client validation, UI Test/Save/Reset, RPC + backend connectivity probe and normalization, i18n and tests. Simultaneously replaces string-only path checks with canonicalization-based async validators and updates filesystem tools and tests to use them. ChangesExternal Ollama Server Configuration
Security Policy Path Validation Refactor
Sequence Diagram(s)sequenceDiagram
participant User
participant ModelStatusSection
participant LocalModelDebugPanel
participant RPC as openhumanLocalAiTestConnection
participant Core as Rust Backend
User->>ModelStatusSection: Type Ollama URL
ModelStatusSection->>ModelStatusSection: validateOllamaUrl()
ModelStatusSection->>User: Show validation error or enable Test/Save
User->>ModelStatusSection: Click "Test Connection"
ModelStatusSection->>LocalModelDebugPanel: onTestConnection()
LocalModelDebugPanel->>RPC: openhumanLocalAiTestConnection(url)
RPC->>Core: call openhuman.local_ai_test_connection
Core->>Core: test_ollama_connection() validates & probes /api/tags
Core-->>RPC: JSON {reachable, models_count, error}
RPC-->>LocalModelDebugPanel: OllamaConnectionTestResult
LocalModelDebugPanel->>ModelStatusSection: connectionTestResult prop
ModelStatusSection->>User: Display "Reachable (N models)" or error
User->>ModelStatusSection: Click "Save"
ModelStatusSection->>LocalModelDebugPanel: onSaveOllamaBaseUrl()
LocalModelDebugPanel->>RPC: openhumanUpdateLocalAiSettings({base_url})
RPC->>Core: update config.local_ai.base_url
Core->>Core: persist to config.toml
Core-->>RPC: success
RPC-->>LocalModelDebugPanel: update savedOllamaBaseUrl state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
coderabbitai
Bot
added
the
feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/openhuman/inference/local/service/vision_embed.rs (1)
71-76:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid logging raw configured Ollama URLs.
Lines 71-76/83-87/93-96/103-107/159-160 emit
base/urldirectly. With external URLs, userinfo credentials can be exposed in logs.Use the same redaction approach used in
public_infer.rsbefore logging URL fields.As per coding guidelines: "Never log secrets, raw JWTs, API keys, or full PII — redact or omit sensitive fields."
Also applies to: 83-87, 93-96, 103-107, 159-160
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/inference/local/service/vision_embed.rs` around lines 71 - 76, The tracing::debug calls in src/openhuman/inference/local/service/vision_embed.rs log raw URL variables (base and url) which may contain sensitive userinfo; update those debug statements (the tracing::debug invocations referencing %base and %url and using body.model/body.prompt) to redact or omit credentials before logging by reusing the same URL-redaction approach implemented in public_infer.rs (call the existing redaction helper used there or parse the URL and strip username/password/authority info), then log the sanitized URL string instead of the raw base/url.src/openhuman/security/policy.rs (1)
722-755:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBlock Windows
%5ctraversal and~\home paths here too.Line 723 only expands
~/, and Line 748 only rejects..%2f/%2f... On Windows, inputs like~\.ssh\id_rsaand..%5c..%5csecret.txtstill passis_path_string_allowed(), so the new string-only policy can be bypassed there.Suggested hardening
fn expand_tilde(&self, path: &str) -> String { - if let Some(rest) = path.strip_prefix("~/") { + if let Some(rest) = path + .strip_prefix("~/") + .or_else(|| path.strip_prefix("~\\")) + { if let Some(home) = dirs::home_dir() { - return format!("{}/{rest}", home.display()); + return home.join(rest).to_string_lossy().into_owned(); } } path.to_string() } // Block URL-encoded traversal attempts (e.g. ..%2f) let lower = path.to_lowercase(); - if lower.contains("..%2f") || lower.contains("%2f..") { + if lower.contains("..%2f") + || lower.contains("%2f..") + || lower.contains("..%5c") + || lower.contains("%5c..") + { return false; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/security/policy.rs` around lines 722 - 755, The tilde-expansion and traversal checks in expand_tilde and is_path_string_allowed miss Windows-style backslashes and URL-encoded backslashes; update expand_tilde to also match and expand "~\\" and "~\" variants (so "~\\..." and "~\..." resolve to the home dir) and update is_path_string_allowed to reject URL-encoded backslashes ("%5c" / "%5C") and encoded traversal patterns such as "..%5c", "%5c.." (in addition to "..%2f" / "%2f.."), and to normalize/check the lowercased path for "%5c" occurrences; reference the expand_tilde method for tilde handling and is_path_string_allowed for adding the extra encoded-backslash/traversal checks.src/openhuman/inference/local/service/ollama_admin.rs (1)
1163-1171:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winChange
/api/tagsprobe from POST to GET.Line 1166 uses
POST {base_url}/api/tags, but Ollama's/api/tagsendpoint supports onlyGET(andHEAD). Using POST will fail even when the Ollama service is healthy, incorrectly blocking startup.Fix
let resp = self .http - .post(format!("{base_url}/api/tags")) + .get(format!("{base_url}/api/tags")) .timeout(std::time::Duration::from_secs(3)) .send() .await;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/inference/local/service/ollama_admin.rs` around lines 1163 - 1171, In ollama_runner_ok_at change the probe request from a POST to a GET (or HEAD) against the /api/tags endpoint so the health check uses an HTTP method the Ollama server supports; update the call in ollama_runner_ok_at (currently using self.http.post(format!("{base_url}/api/tags"))) to use self.http.get(...) and keep the timeout, send().await, and the same success/status check logic intact.src/openhuman/tools/impl/filesystem/edit_file.rs (1)
100-117:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReinstate a raw path gate before
symlink_metadata().After removing the old early path check,
workspace_dir.join(path)now lets absolute inputs pass straight intosymlink_metadata(). A request like/etc/localtimecan return a symlink-specific error before sandbox validation runs, which leaks forbidden-path metadata. Reject disallowed path strings before any filesystem access here.Suggested fix
+ if !self.security.is_path_string_allowed(path) { + return Ok(ToolResult::error(format!("path not allowed: {path}"))); + } + let full = self.security.workspace_dir.join(path); // Symlink check must happen on the *unresolved* path — // `canonicalize` resolves symlinks, so checking after that point // would always see the link's final target.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/tools/impl/filesystem/edit_file.rs` around lines 100 - 117, Before calling tokio::fs::symlink_metadata(&full).await, add a raw-path string gate that rejects disallowed path strings without touching the filesystem: call a new or existing string-only validator (e.g. self.security.validate_path_string(path) or implement that helper) to reject absolute paths, path traversal, or other disallowed patterns and return ToolResult::error on failure; keep the existing symlink_metadata check and the later self.security.validate_path(path).await (which resolves symlinks) unchanged so no filesystem access occurs for rejected raw paths.
🧹 Nitpick comments (1)
src/openhuman/security/policy.rs (1)
777-905: ⚡ Quick winAdd debug/trace logs on the reject branches in these validators.
These methods only log the success path today. The early
Errbranches are the ones operators will need when canonicalization, workspace-containment, or forbidden-path checks fail.As per coding guidelines, "Use log / tracing at
debugortracelevel on RPC entry and exit, error paths, state transitions, and any branch that is hard to infer from tests alone."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/security/policy.rs` around lines 777 - 905, Add debug/trace logging on all early-return error branches in validate_path and validate_parent_path so failures are visible; for each Err return (e.g., when is_path_string_allowed(path) fails, canonicalize/map_err failures, is_resolved_path_allowed checks, parent/file_name missing, and forbidden_paths rejections) emit a log::debug or log::trace including the function name (validate_path or validate_parent_path), the input path, any resolved/canonical value (resolved, canonical_ancestor, resolved_parent, result), and the error or reason string before returning the Err; place these logs immediately before each return Err to show context for workspace_root, forbidden_paths and canonicalization failures so operators can see why the validator rejected the path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/memory.md:
- Line 154: The notes conflict about test commands: Line 154 suggests using
`pnpm debug unit` for local iteration, while Line 213 mandates `pnpm
test:coverage` before commit; resolve by clarifying scope and guidance—update
the memory.md entries so the `pnpm debug unit` recommendation is explicitly
marked as a local/faster iteration workflow and `pnpm test:coverage` is
described as the CI/pre-commit validation step (or remove the pre-commit
requirement if you intend only local use). Mention both commands by name (`pnpm
debug unit`, `pnpm test:coverage`), why/when to use each, and make the
pre-commit policy consistent with the repository’s gating rules.
In `@app/src/components/settings/panels/local-model/ModelStatusSection.tsx`:
- Line 114: The "Test Connection" button enables RPC calls even for invalid URLs
because canTest only checks ollamaBaseUrlInput and isTestingConnection; update
the canTest computation to also require urlValidation.valid so it becomes true
only when ollamaBaseUrlInput.trim().length > 0, isTestingConnection is false,
and urlValidation.valid is true (referencing the canTest constant and
urlValidation variable in ModelStatusSection.tsx).
- Around line 203-204: The model-count suffix is hardcoded in English; update
the status string assembly where connectionTestResult is used (the ternary
producing `${...} (${connectionTestResult.models_count} models)`) to use the
i18n translator `t` with a pluralized/localized key (e.g. call
`t('localModel.modelsCount', { count: connectionTestResult.models_count })` or
similar) so the count and "models" suffix are localized; keep the existing keys
`localModel.ollamaServer.reachable`/`unreachable` and include the translated
count fragment conditionally when connectionTestResult.models_count is a number.
In `@app/src/components/settings/panels/LocalModelDebugPanel.tsx`:
- Line 351: The current log call writes the raw ollamaBaseUrlInput (in
LocalModelDebugPanel.tsx) which can include userinfo and leak credentials;
update the save/log path that calls log(...) so it redacts or omits sensitive
userinfo before logging (e.g., parse ollamaBaseUrlInput, strip username/password
or replace them with "[REDACTED]" and log only the scheme+host+path or a masked
URL), referencing the ollamaBaseUrlInput variable and the log(...) invocation in
the save handler to ensure no raw secrets are emitted to frontend logs.
- Around line 318-321: The code unconditionally sets both setOllamaBaseUrlInput
and setSavedOllamaBaseUrl from result.ollama_base_url, which overwrites any
in-progress user edits; change it so only the saved state is always updated
(call setSavedOllamaBaseUrl(result.ollama_base_url)) but only update the input
field via setOllamaBaseUrlInput when the current ollamaBaseUrlInput is empty or
matches the previous saved value (i.e., no unsaved edits). Use the existing
state variables ollamaBaseUrlInput and savedOllamaBaseUrl to decide whether to
call setOllamaBaseUrlInput.
In `@app/src/lib/i18n/chunks/ar-2.ts`:
- Around line 242-250: The Arabic locale file contains untranslated English
strings for the ollamaServer keys; translate the entries
'localModel.ollamaServer.helperText', 'localModel.ollamaServer.label',
'localModel.ollamaServer.placeholder', 'localModel.ollamaServer.reachable',
'localModel.ollamaServer.resetButton', 'localModel.ollamaServer.saveButton',
'localModel.ollamaServer.testButton', 'localModel.ollamaServer.unreachable', and
'localModel.ollamaServer.validationError' into Arabic, replacing the English
values with proper Arabic text while keeping the keys unchanged (e.g., provide
Arabic equivalents for "Example: http://192.168.1.5:11434", "Ollama Server URL",
"http://localhost:11434", "Reachable", "Reset to default", "Save", "Test
Connection", "Unreachable", and "Must be a valid http:// or https:// URL").
In `@app/src/lib/i18n/chunks/bn-2.ts`:
- Around line 251-259: The new Bengali locale file contains untranslated English
strings for the Ollama keys; update all keys under localModel.ollamaServer
(localModel.ollamaServer.helperText, .label, .placeholder, .reachable,
.resetButton, .saveButton, .testButton, .unreachable, .validationError) with
proper Bengali translations so the UI remains consistent for bn locale—replace
each English value with the correct Bengali phrase, keeping placeholders and URL
examples intact (e.g., keep http:// or https:// in validationError and example
URLs in helperText/placeholder) and ensure punctuation/formatting matches other
bn entries.
In `@app/src/lib/i18n/chunks/es-2.ts`:
- Around line 254-262: The Spanish locale entries for the
localModel.ollamaServer keys are still in English; update the values for
'localModel.ollamaServer.helperText', 'localModel.ollamaServer.label',
'localModel.ollamaServer.placeholder', 'localModel.ollamaServer.reachable',
'localModel.ollamaServer.resetButton', 'localModel.ollamaServer.saveButton',
'localModel.ollamaServer.testButton', 'localModel.ollamaServer.unreachable', and
'localModel.ollamaServer.validationError' to correct Spanish translations (keep
the example URL format in helperText/placeholder but translate surrounding text
and UI button/state labels and the validation message).
In `@app/src/lib/i18n/chunks/fr-2.ts`:
- Around line 256-264: Replace the English strings for the i18n keys
'localModel.ollamaServer.helperText', 'localModel.ollamaServer.label',
'localModel.ollamaServer.placeholder', 'localModel.ollamaServer.reachable',
'localModel.ollamaServer.resetButton', 'localModel.ollamaServer.saveButton',
'localModel.ollamaServer.testButton', 'localModel.ollamaServer.unreachable', and
'localModel.ollamaServer.validationError' with proper French translations (e.g.,
helperText -> "Exemple : http://192.168.1.5:11434", label -> "URL du serveur
Ollama", placeholder -> "http://localhost:11434", reachable -> "Accessible",
resetButton -> "Réinitialiser par défaut", saveButton -> "Enregistrer",
testButton -> "Tester la connexion", unreachable -> "Inaccessible",
validationError -> "Doit être une URL valide commençant par http:// ou
https://"), ensuring the keys remain unchanged.
In `@app/src/lib/i18n/chunks/hi-2.ts`:
- Around line 249-257: The Hindi i18n chunk contains English strings for the new
keys (localModel.ollamaServer.helperText, localModel.ollamaServer.label,
localModel.ollamaServer.placeholder, localModel.ollamaServer.reachable,
localModel.ollamaServer.resetButton, localModel.ollamaServer.saveButton,
localModel.ollamaServer.testButton, localModel.ollamaServer.unreachable,
localModel.ollamaServer.validationError); replace each English value with an
appropriate Hindi translation preserving meaning and any example URL formatting
(e.g., keep "http://..." examples unchanged) so all localModel.ollamaServer.*
entries are fully localized for Hindi users.
In `@app/src/lib/i18n/chunks/id-2.ts`:
- Around line 250-258: The Indonesian locale contains untranslated English
strings for the localModel.ollamaServer keys; replace the values for
'localModel.ollamaServer.helperText', 'localModel.ollamaServer.label',
'localModel.ollamaServer.placeholder', 'localModel.ollamaServer.reachable',
'localModel.ollamaServer.resetButton', 'localModel.ollamaServer.saveButton',
'localModel.ollamaServer.testButton', 'localModel.ollamaServer.unreachable', and
'localModel.ollamaServer.validationError' with proper Indonesian translations
(preserve placeholders like example URLs), ensuring wording matches existing
locale style and pluralization/format conventions.
In `@app/src/lib/i18n/chunks/it-2.ts`:
- Around line 251-259: The new localization entries for the Ollama server are
still in English; update the values for the keys
'localModel.ollamaServer.helperText', 'localModel.ollamaServer.label',
'localModel.ollamaServer.placeholder', 'localModel.ollamaServer.reachable',
'localModel.ollamaServer.resetButton', 'localModel.ollamaServer.saveButton',
'localModel.ollamaServer.testButton', 'localModel.ollamaServer.unreachable', and
'localModel.ollamaServer.validationError' with their Italian equivalents
(translate the helper/example URL text, label, placeholder, status strings,
button labels and validation message) so the it-2.ts chunk is fully localized.
In `@app/src/lib/i18n/chunks/pt-2.ts`:
- Around line 254-262: The Portuguese locale file contains untranslated strings
for the Ollama server UI keys (localModel.ollamaServer.helperText, .label,
.placeholder, .reachable, .resetButton, .saveButton, .testButton, .unreachable,
.validationError); replace each English value with its proper Portuguese
translation (e.g., helperText/example, label, placeholder URL text,
reachable/unreachable statuses, button labels, and the validation message) so
the UI is fully localized and consistent with other pt translations.
In `@app/src/lib/i18n/chunks/ru-2.ts`:
- Around line 249-257: Translate the English strings for the Ollama Server UI
entries in the Russian locale chunk: replace values for keys
'localModel.ollamaServer.helperText', 'localModel.ollamaServer.label',
'localModel.ollamaServer.placeholder', 'localModel.ollamaServer.reachable',
'localModel.ollamaServer.resetButton', 'localModel.ollamaServer.saveButton',
'localModel.ollamaServer.testButton', 'localModel.ollamaServer.unreachable', and
'localModel.ollamaServer.validationError' with proper Russian translations
(e.g., helperText -> "Например: http://192.168.1.5:11434", label -> "URL сервера
Ollama", placeholder -> "http://localhost:11434", reachable -> "Доступен",
resetButton -> "Сбросить по умолчанию", saveButton -> "Сохранить", testButton ->
"Проверить подключение", unreachable -> "Недоступен", validationError -> "Должен
быть корректный URL с http:// или https://"), keeping placeholders and example
URLs intact.
In `@app/src/lib/i18n/chunks/zh-CN-2.ts`:
- Around line 233-241: Translate the English values for the keys
'localModel.ollamaServer.helperText', 'localModel.ollamaServer.label',
'localModel.ollamaServer.placeholder', 'localModel.ollamaServer.reachable',
'localModel.ollamaServer.resetButton', 'localModel.ollamaServer.saveButton',
'localModel.ollamaServer.testButton', 'localModel.ollamaServer.unreachable', and
'localModel.ollamaServer.validationError' into Simplified Chinese in the zh-CN
chunk so the UI is consistent (keep the keys unchanged, only replace the string
values with appropriate Chinese translations for helper text, label,
placeholder, status texts, button labels, and validation message).
In `@src/openhuman/inference/local/service/ollama_admin.rs`:
- Around line 1252-1256: has_model() currently calls ollama_base_url() and
ignores the configured external URL (config.local_ai.base_url), causing
inconsistent model checks; update has_model() and its callers to accept a
resolved base_url (or a &Config reference) and call
has_model_at(&resolved_base_url, model). Thread the resolved base URL from the
higher-level model ensure/pull functions (where config is available) into the
has_model() call sites so has_model_at uses the same base URL used for
pull/ensure operations; adjust signatures for has_model() and any callsites
accordingly and remove direct use of ollama_base_url() inside has_model().
In `@src/openhuman/inference/local/service/public_infer.rs`:
- Around line 567-569: The debug log in inference_with_temperature_internal
prints base_url verbatim which can leak credentials in URL userinfo; update the
logging to redact or omit sensitive userinfo by parsing base_url (e.g., with
Url::parse) and replacing username/password/userinfo with a fixed placeholder
(or log only the scheme+host+path) before passing it to log::debug; ensure you
reference the base_url variable and keep the rest of the log message unchanged.
In `@src/openhuman/tools/impl/filesystem/csv_export.rs`:
- Around line 195-197: Move the SecurityPolicy::record_action() call to occur
before any filesystem probing or creation so the action budget is charged prior
to canonicalization, symlink checks, or tokio::fs::create_dir_all; specifically,
call record_action() before you canonicalize/resolve the target path and before
the block that checks resolved_target.parent() and calls
tokio::fs::create_dir_all(), and ensure the same change is applied to the
surrounding range (lines ~200-214) where symlink existence/canonicalization and
file-existence checks occur.
In `@src/openhuman/tools/impl/filesystem/file_write.rs`:
- Around line 74-76: The code currently creates parent directories and probes
the target before consuming quota; move the SecurityPolicy::record_action() call
to run immediately after validating the target path (before any filesystem
mutations or probing such as tokio::fs::create_dir_all,
resolved_target.parent(), or symlink checks). Specifically, update the
file_write implementation so that is_rate_limited() still guards entry but
record_action() is invoked prior to any use of resolved_target (including the
create_dir_all call and the logic around resolved_target.exists()/symlink
probing in the block covering lines ~74–93), ensuring budget is consumed before
any directory creation or metadata probing.
---
Outside diff comments:
In `@src/openhuman/inference/local/service/ollama_admin.rs`:
- Around line 1163-1171: In ollama_runner_ok_at change the probe request from a
POST to a GET (or HEAD) against the /api/tags endpoint so the health check uses
an HTTP method the Ollama server supports; update the call in
ollama_runner_ok_at (currently using
self.http.post(format!("{base_url}/api/tags"))) to use self.http.get(...) and
keep the timeout, send().await, and the same success/status check logic intact.
In `@src/openhuman/inference/local/service/vision_embed.rs`:
- Around line 71-76: The tracing::debug calls in
src/openhuman/inference/local/service/vision_embed.rs log raw URL variables
(base and url) which may contain sensitive userinfo; update those debug
statements (the tracing::debug invocations referencing %base and %url and using
body.model/body.prompt) to redact or omit credentials before logging by reusing
the same URL-redaction approach implemented in public_infer.rs (call the
existing redaction helper used there or parse the URL and strip
username/password/authority info), then log the sanitized URL string instead of
the raw base/url.
In `@src/openhuman/security/policy.rs`:
- Around line 722-755: The tilde-expansion and traversal checks in expand_tilde
and is_path_string_allowed miss Windows-style backslashes and URL-encoded
backslashes; update expand_tilde to also match and expand "~\\" and "~\"
variants (so "~\\..." and "~\..." resolve to the home dir) and update
is_path_string_allowed to reject URL-encoded backslashes ("%5c" / "%5C") and
encoded traversal patterns such as "..%5c", "%5c.." (in addition to "..%2f" /
"%2f.."), and to normalize/check the lowercased path for "%5c" occurrences;
reference the expand_tilde method for tilde handling and is_path_string_allowed
for adding the extra encoded-backslash/traversal checks.
In `@src/openhuman/tools/impl/filesystem/edit_file.rs`:
- Around line 100-117: Before calling tokio::fs::symlink_metadata(&full).await,
add a raw-path string gate that rejects disallowed path strings without touching
the filesystem: call a new or existing string-only validator (e.g.
self.security.validate_path_string(path) or implement that helper) to reject
absolute paths, path traversal, or other disallowed patterns and return
ToolResult::error on failure; keep the existing symlink_metadata check and the
later self.security.validate_path(path).await (which resolves symlinks)
unchanged so no filesystem access occurs for rejected raw paths.
---
Nitpick comments:
In `@src/openhuman/security/policy.rs`:
- Around line 777-905: Add debug/trace logging on all early-return error
branches in validate_path and validate_parent_path so failures are visible; for
each Err return (e.g., when is_path_string_allowed(path) fails,
canonicalize/map_err failures, is_resolved_path_allowed checks, parent/file_name
missing, and forbidden_paths rejections) emit a log::debug or log::trace
including the function name (validate_path or validate_parent_path), the input
path, any resolved/canonical value (resolved, canonical_ancestor,
resolved_parent, result), and the error or reason string before returning the
Err; place these logs immediately before each return Err to show context for
workspace_root, forbidden_paths and canonicalization failures so operators can
see why the validator rejected the path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 81a5b35e-b1e1-4f6b-9283-2475ffdb640e
📒 Files selected for processing (38)
.claude/memory.mdapp/src/components/settings/panels/LocalModelDebugPanel.tsxapp/src/components/settings/panels/local-model/ModelStatusSection.test.tsxapp/src/components/settings/panels/local-model/ModelStatusSection.tsxapp/src/lib/i18n/chunks/ar-2.tsapp/src/lib/i18n/chunks/bn-2.tsapp/src/lib/i18n/chunks/en-2.tsapp/src/lib/i18n/chunks/es-2.tsapp/src/lib/i18n/chunks/fr-2.tsapp/src/lib/i18n/chunks/hi-2.tsapp/src/lib/i18n/chunks/id-2.tsapp/src/lib/i18n/chunks/it-2.tsapp/src/lib/i18n/chunks/pt-2.tsapp/src/lib/i18n/chunks/ru-2.tsapp/src/lib/i18n/chunks/zh-CN-2.tsapp/src/lib/i18n/en.tsapp/src/utils/ollamaUrlValidation.test.tsapp/src/utils/ollamaUrlValidation.tsapp/src/utils/tauriCommands/localAi.tssrc/openhuman/cron/scheduler.rssrc/openhuman/inference/local/ollama.rssrc/openhuman/inference/local/schemas.rssrc/openhuman/inference/local/schemas_tests.rssrc/openhuman/inference/local/service/mod.rssrc/openhuman/inference/local/service/ollama_admin.rssrc/openhuman/inference/local/service/ollama_admin_tests.rssrc/openhuman/inference/local/service/public_infer.rssrc/openhuman/inference/local/service/vision_embed.rssrc/openhuman/security/policy.rssrc/openhuman/security/policy_tests.rssrc/openhuman/tools/impl/browser/image_info.rssrc/openhuman/tools/impl/filesystem/apply_patch.rssrc/openhuman/tools/impl/filesystem/csv_export.rssrc/openhuman/tools/impl/filesystem/edit_file.rssrc/openhuman/tools/impl/filesystem/file_read.rssrc/openhuman/tools/impl/filesystem/file_write.rssrc/openhuman/tools/impl/filesystem/grep.rssrc/openhuman/tools/impl/filesystem/list_files.rs
- Gate Test button on URL validation rather than non-empty input - Localize model count in connection result display - Prevent diagnostics poll from overwriting unsaved URL edits - Redact credentials from Ollama URL in debug log output - Replace English placeholder translations in all 10 locale chunks (ar, bn, es, fr, hi, id, it, pt, ru, zh-CN) including new modelCount key
…nd validation Covers diff-cover missing lines: - LocalModelDebugPanel: config seed on mount, handleTestConnection (success + throw paths), handleSaveOllamaBaseUrl, handleResetOllamaBaseUrl - ModelStatusSection: onChange input, spinner when testing, reachable/unreachable display, validation error rendering - ollamaUrlValidation: unparseable URL format error (catch branch) - tauriCommands/localAi: openhumanLocalAiTestConnection RPC dispatch
graycyrus
requested changes
May 19, 2026
Contributor
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Walkthrough
This PR adds a user-configurable Ollama server URL with inline validation and connection testing in the Local Model settings panel, backed by a new ollama_base_url_from_config priority chain and openhuman.local_ai_test_connection RPC. It also bundles an unrelated security policy refactor that consolidates path validation into validate_path / validate_parent_path across all filesystem tool implementations. Overall solid work — the Ollama URL feature is well-structured with good test coverage, and the security consolidation is a real improvement. Two credential-leak risks in new log lines need fixing before merge.
Change Summary
| File | Change type | Description |
|---|---|---|
ollama.rs |
Feature | ollama_base_url_from_config, validate_ollama_url + tests |
ollama_admin.rs |
Feature | Config-aware URL resolution across bootstrap/health/diagnostics, test_ollama_connection standalone fn |
public_infer.rs |
Feature | Config-aware URL + redact_ollama_base_url applied (good) |
vision_embed.rs |
Feature | Config-aware URL, but new log line leaks raw URL |
schemas.rs |
Feature | local_ai_test_connection RPC registration |
LocalModelDebugPanel.tsx |
Feature | URL state, config seeding on mount, test/save/reset handlers |
ModelStatusSection.tsx |
Feature | Editable URL input, validation UI, connection test display |
ollamaUrlValidation.ts |
New | TS-side URL validation (mirrors Rust) |
localAi.ts |
Feature | openhumanLocalAiTestConnection RPC wrapper |
| 12 i18n chunks | i18n | localModel.ollamaServer.* keys across all locales |
policy.rs |
Refactor | validate_path/validate_parent_path, expand_tilde extraction, is_path_allowed → is_path_string_allowed |
policy_tests.rs |
Tests | Renamed assertions + 7 new async validation tests |
| 7 filesystem tools | Refactor | Migrated from manual is_path_allowed+canonicalize to validate_path/validate_parent_path |
.claude/memory.md |
Meta | Internal AI agent notes |
| 4 new test files | Tests | TS validation, RPC wrapper, panel integration, status section |
Per-file Analysis
Ollama URL Feature (commits 5-7)
Rust core — ollama_base_url_from_config correctly implements config → env → default priority. validate_ollama_url is thorough (scheme, host, creds, query, fragment checks). test_ollama_connection properly validates before probing and uses GET (correct for /api/tags). All callsites in ollama_admin.rs and public_infer.rs are updated to use config-aware resolution.
Frontend — Clean lifted-state pattern with state in LocalModelDebugPanel and props to ModelStatusSection. Config seeded on mount via useEffect. The conditional diagnostics update (prev === savedOllamaBaseUrl) correctly avoids overwriting unsaved edits. URL validation runs inline with clear error display.
i18n — All 12 locales updated with proper translations (not English placeholders).
Tests — Good coverage: Rust has 8 validation + 2 config priority + 4 connection tests. TS has 12 validation + 2 RPC + 8 panel + 12 status section tests. Edge cases covered well.
Security Refactor (commits 1-4)
validate_path and validate_parent_path are well-designed — they combine string checks, canonicalization, workspace containment, and forbidden-path checking in one call. The validate_parent_path ancestor-walk pattern is smart for write operations where the target doesn't exist yet. All 7 filesystem tools are cleanly migrated. The expand_tilde extraction using dirs::home_dir() is a cross-platform improvement over the old std::env::var("HOME").
Red Flags
- Multiple unrelated changes bundled: Security refactor (commits 1-4) and Ollama URL feature (commits 5-7) are independent concerns. Consider splitting in future PRs for cleaner review and bisect history.
- Large diff (1577 lines): Justified by i18n boilerplate + security migration across 7 tools + test coverage.
…gging Move redact_ollama_base_url to ollama.rs (pub(crate)) so it can be shared by vision_embed.rs and public_infer.rs. Apply it at the two new log sites flagged by graycyrus: - ollama_base_url_from_config: log redacted form of config base_url - vision_embed embed(): redact embed_base before debug print Tests for the redaction helper move to ollama.rs where the function now lives.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/openhuman/inference/local/ollama.rs`:
- Around line 99-104: The normalization drops IPv6 brackets because
parsed.host_str() returns raw addresses (e.g., "::1"); update the normalization
in validate_ollama_url (or the code building `normalized` in ollama.rs) to
detect hosts containing ':' and wrap them in brackets if not already bracketed
before appending the optional port (i.e., produce "[::1]" + ":11434"). Keep
using `parsed.scheme()` and `parsed.port()` as before, ensure you only add
brackets for IPv6 literals, and add the suggested test
`validate_ollama_url_handles_ipv6` to verify the normalized output equals
"http://[::1]:11434".
- Around line 57-61: The fallback debug log prints the resolved base URL without
redaction; update the log in the ollama_base_url_from_config path to redact
sensitive parts of the resolved value before logging (e.g., call the existing
redact function used on line 52 — e.g., redact_url or the same redaction helper
— and pass redact_url(&resolved) into log::debug) so both the config URL and the
resolved OLLAMA_HOST are logged consistently and never reveal credentials.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 98863518-b94c-417e-9095-f02c64522370
📒 Files selected for processing (3)
src/openhuman/inference/local/ollama.rssrc/openhuman/inference/local/service/public_infer.rssrc/openhuman/inference/local/service/vision_embed.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/openhuman/inference/local/service/vision_embed.rs
- src/openhuman/inference/local/service/public_infer.rs
…tion - Apply redact_ollama_base_url to the fallback resolved-URL log in ollama_base_url_from_config for consistent credential safety - Fix validate_ollama_url to re-add IPv6 brackets stripped by host_str() so http://[::1]:11434 normalizes correctly instead of producing invalid http://::1:11434 syntax - Add validate_ollama_url_handles_ipv6 test
…ction + ollama_admin) Resolved two conflicts from 4 upstream commits: - ModelStatusSection.tsx: keep both OllamaConnectionTestResult (this branch) and ModelContextEligibility (upstream, from tinyhumansai#2122 memory-layer gate) - ollama_admin.rs: merge both import sets — our validate_ollama_url + ollama_base_url_from_config with upstream OllamaShowRequest + OllamaShowResponse
…uble-bracketing host_str() behaviour differs across url-crate versions: some return "::1", others return "[::1]". Using parsed.host() with the Host enum variant match is version-agnostic: Ipv6(addr) always yields the raw address so we wrap it in brackets exactly once.
graycyrus
reviewed
May 19, 2026
Contributor
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Re-review — previous changes addressed
All three findings from my prior review have been addressed:
| Prior finding | Status |
|---|---|
[major] vision_embed.rs — raw URL in debug log |
Fixed — now uses redact_ollama_base_url() |
[major] ollama.rs — ollama_base_url_from_config logs raw config URL |
Fixed — redaction applied to both config and fallback log lines |
[minor] ollama_admin.rs — throwaway reqwest::Client in test_ollama_connection |
Acknowledged — standalone utility function, acceptable trade-off |
Additional fixes since last review:
- IPv6 normalization corrected (commit
24469d6f) — usesurl::Hostenum to avoid double-bracketing - Fallback URL log in
ollama_base_url_from_confignow redacted for consistency - Merge conflict with upstream/main resolved cleanly
The redact_ollama_base_url helper was consolidated into ollama.rs (previously duplicated in public_infer.rs) — good cleanup.
No new issues found. Ready for maintainer approval.
…l-server-url # Conflicts: # src/openhuman/security/policy.rs # src/openhuman/security/policy_tests.rs
graycyrus
approved these changes
May 20, 2026
Contributor
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Looks good, nice work!
mtkik
pushed a commit
to mtkik/openhuman-meet
that referenced
this pull request
May 21, 2026
CodeGhost21
pushed a commit
to CodeGhost21/openhuman
that referenced
this pull request
May 22, 2026
AusAgentSmith
pushed a commit
to AusAgentSmith/openhuman
that referenced
this pull request
May 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ollama_base_url()previously read only env vars and ignoredconfig.local_ai.base_url; addsollama_base_url_from_config(config)with correct priority chain (config → env → default) and updates bootstrap/health-check/infer/vision-embed callsitesopenhuman.local_ai_test_connectionthat probes/api/tagswith a 3-second timeout and returns{reachable, error, models_count}LocalModelDebugPanel/ModelStatusSection: text input, inline validation, Test Connection button, Save/Reset, seeded from persisted config on mountvalidate_ollama_urlin both Rust and TypeScript (scheme + host + creds + path normalization)localModel.ollamaServer.*) across all 12 locale chunksProblem
config.tomlollama_base_url()ignoredconfig.local_ai.base_urlentirely, so the config field was effectively a no-opollama_healthy()uses the config-aware URL this path is skipped naturallySolution
ollama_base_url_from_config(config)replaces the env-var-only lookup in all bootstrap-critical callsites;ollama_base_url()zero-arg wrapper preserved for non-critical legacy callsitesensure_ollama_serveris now purely a connectivity check — if the configured endpoint responds, bootstrap proceeds without any binary discovery or install attemptopenhumanGetConfig()on mount so the saved value is visible immediately, not only after running diagnosticsSubmission Checklist
test_ollama_connection, 8 Vitest tests forModelStatusSectionURL UX, 12 tests forollamaUrlValidation, 8 Rust tests forvalidate_ollama_url+ollama_base_url_from_configtest_ollama_connectionuses the existingreqwestclient already in scope; mock Axum server used in Rust testsCloses #2159in Related sectionImpact
ollama_base_url()zero-arg form unchanged — non-critical callsites (embeddings, memory, triage) are unaffectedRelated
Closes #2159
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
Validation Run
pnpm --filter openhuman-app format:checkpnpm typecheckpnpm debug unit— 2627 passed, 0 failed;cargo test -p openhuman test_ollama_connection— 4 passedcargo check --manifest-path Cargo.tomlcleancargo check --manifest-path app/src-tauri/Cargo.tomlcleanValidation Blocked
Behavior Changes
Parity Contract
ollama_base_url()zero-arg unchanged; env varOPENHUMAN_OLLAMA_BASE_URLstill overrides default when config field is unsetconfig.local_ai.base_urlisNoneDuplicate / Superseded PR Handling
Summary by CodeRabbit