fix(ollama): URL builder + local-AI noise suppression bundle#1553
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
🚧 Files skipped from review as they are similar to previous changes (8)
📝 WalkthroughWalkthroughThis PR gates frontend local-AI actions when the runtime is disabled, classifies expected errors to avoid Sentry noise, enables optional/no-auth provider configurations, and validates Ollama embedding configuration at construction time. ChangesLocal AI Runtime Gating and Error Classification
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
app/src/components/settings/panels/local-model/ModelDownloadSection.test.tsx (1)
52-61: ⚡ Quick winExpand runtime-disabled assertions beyond the summary action.
This PR gated multiple controls; adding at least one more assertion (e.g., asset download or prompt test button) would reduce regression risk for the broader gate change.
As per coding guidelines, “Achieve ≥ 80% coverage on changed lines; PRs must pass
diff-covercheck via Vitest andcargo-llvm-cov.”🤖 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 `@app/src/components/settings/panels/local-model/ModelDownloadSection.test.tsx` around lines 52 - 61, The test in ModelDownloadSection currently only asserts the summary test button is disabled and its handler (props.onRunSummaryTest) isn't called; extend it to cover another gated control (e.g., the asset download or prompt test) by retrieving that button (for example by role/name like 'Download Model Assets' or 'Run Prompt Test'), asserting it is disabled, firing a click, and verifying the corresponding prop handler (e.g., props.onDownloadAssets or props.onRunPromptTest) was not called; update the test that uses makeProps and ModelDownloadSection to include this additional assertion so the runtime-disabled gate is validated for multiple actions.app/src/components/settings/panels/__tests__/LocalModelPanel.test.tsx (1)
163-172: ⚡ Quick winAdd a runtime-disabled assertion for the “Advanced settings” action too.
This file now covers download gating, but the same runtime gate was added to navigation; adding that assertion will fully cover the new UI guard behavior.
As per coding guidelines, “Achieve ≥ 80% coverage on changed lines; PRs must pass
diff-covercheck via Vitest andcargo-llvm-cov.”🤖 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 `@app/src/components/settings/panels/__tests__/LocalModelPanel.test.tsx` around lines 163 - 172, Extend the existing test in LocalModelPanel.test.tsx to also assert that the "Advanced settings" action is gated when the runtime is disabled: locate the Advanced settings control (e.g., using screen.findByRole('button', { name: 'Advanced settings' }) or the same query style used for Download Models), add expect(advancedButton).toBeDisabled(), fireEvent.click(advancedButton), and assert that whatever mock/handler would navigate/open advanced settings is not called (follow the same pattern used for openhumanLocalAiDownload and openhumanLocalAiDownloadAllAssets); keep the assertion inside the same it(...) block for 'does not invoke model downloads while runtime is disabled'.app/src/components/settings/panels/local-model/ModelStatusSection.test.tsx (1)
27-27: ⚡ Quick winPlease add a runtime-disabled button-state test in this suite.
Setting
runtimeEnabled: truefixes setup compatibility, but it doesn’t verify the new gating behavior (Bootstrap / ResumeandForce Re-bootstrapdisabled when runtime is off).As per coding guidelines, “Achieve ≥ 80% coverage on changed lines; PRs must pass
diff-covercheck via Vitest andcargo-llvm-cov.”🤖 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 `@app/src/components/settings/panels/local-model/ModelStatusSection.test.tsx` at line 27, Add a new test in ModelStatusSection.test.tsx that mounts the ModelStatusSection component with runtimeEnabled: false (instead of true) and asserts that the "Bootstrap / Resume" and "Force Re-bootstrap" controls are rendered in a disabled state; locate the setup helper used in existing tests (e.g., the render or setup function that supplies props like runtimeEnabled) and reuse it with runtimeEnabled: false, then query the buttons by their accessible text or test-id and expect them to have the disabled attribute or aria-disabled=true to validate the gating behavior when runtime is off.
🤖 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 `@app/src/components/settings/panels/AIPanel.tsx`:
- Around line 132-136: The retry-download onClick handler may throw from
openhumanLocalAiDownload(true) and needs a try/catch like other handlers; wrap
the await openhumanLocalAiDownload(true) call inside try { ... } catch (err) {
setError(err); } and ensure loadLocalAiStatus() still runs (either after
successful download or in a finally block) so the component state is refreshed;
reference the localAiRuntimeEnabled guard, openhumanLocalAiDownload,
loadLocalAiStatus, and setError when making this change.
In `@app/src/components/settings/panels/LocalModelDebugPanel.tsx`:
- Line 108: The current runtimeEnabled check treats status === null as disabled
causing a permanent no-op; change the logic in the LocalModelDebugPanel where
runtimeEnabled is computed (the runtimeEnabled constant that uses status and
status.state) so that only an explicit state of "disabled" blocks actions—e.g.
compute runtimeEnabled by negating status?.state === 'disabled' or by treating
null/undefined status as enabled rather than disabled; update any dependent
checks that rely on runtimeEnabled accordingly.
In `@src/openhuman/agent/triage/routing.rs`:
- Around line 107-116: Normalize the local API key before deriving auth_style:
replace the current is_some() check on local_cfg.api_key with a
trimmed/empty-aware test (e.g., obtain an Option<&str> that trims whitespace and
becomes None for empty strings) and use that normalized option to set auth_style
(AuthStyle::Bearer vs AuthStyle::None) and to pass into
OpenAiCompatibleProvider::new (instead of raw local_cfg.api_key.as_deref()).
Ensure you reference local_cfg.api_key, auth_style, AuthStyle::Bearer, and
OpenAiCompatibleProvider::new when making the change.
In `@src/openhuman/embeddings/ollama.rs`:
- Around line 82-121: The normalize_base_url function currently accepts
credentialed URLs (e.g., http://user:pass@host) which can leak secrets when
stored or logged; update normalize_base_url to detect and reject any Url that
has non-empty username() or password() (use reqwest::Url::username()/password()
or the Url methods available), returning an anyhow::bail! with a clear message
instructing to provide the server root without credentials; keep existing
validations (scheme, query/fragment, path segments) and still trim trailing
slashes before returning.
In `@src/openhuman/providers/compatible.rs`:
- Around line 327-338: The credential_for_request method currently treats
Some("") or whitespace-only credentials as present; update it so when auth is
required (AuthStyle != AuthStyle::None) you treat empty or whitespace-only
strings as missing: check self.credential.as_deref().map(str::trim).filter(|s|
!s.is_empty()) and return Ok(Some(trimmed)) when non-empty, otherwise return the
same anyhow::anyhow!("{} API key not set...") error using self.name; keep the
early return for AuthStyle::None but ensure blank/whitespace credentials do not
produce an Ok(Some("")) value.
In `@src/openhuman/routing/factory.rs`:
- Around line 77-87: The auth-mode selection incorrectly treats Some("") as a
present key and forces AuthStyle::Bearer; update the check around
local_auth_style to consider only non-empty strings (e.g. use
local_ai_config.api_key.as_deref().filter(|s| !s.is_empty()).is_some() or
equivalent) so empty API keys map to AuthStyle::None; adjust the creation of the
OpenAiCompatibleProvider (OpenAiCompatibleProvider::new with provider_label,
&local_base, local_ai_config.api_key.as_deref(), local_auth_style) to use the
corrected local_auth_style logic.
---
Nitpick comments:
In `@app/src/components/settings/panels/__tests__/LocalModelPanel.test.tsx`:
- Around line 163-172: Extend the existing test in LocalModelPanel.test.tsx to
also assert that the "Advanced settings" action is gated when the runtime is
disabled: locate the Advanced settings control (e.g., using
screen.findByRole('button', { name: 'Advanced settings' }) or the same query
style used for Download Models), add expect(advancedButton).toBeDisabled(),
fireEvent.click(advancedButton), and assert that whatever mock/handler would
navigate/open advanced settings is not called (follow the same pattern used for
openhumanLocalAiDownload and openhumanLocalAiDownloadAllAssets); keep the
assertion inside the same it(...) block for 'does not invoke model downloads
while runtime is disabled'.
In
`@app/src/components/settings/panels/local-model/ModelDownloadSection.test.tsx`:
- Around line 52-61: The test in ModelDownloadSection currently only asserts the
summary test button is disabled and its handler (props.onRunSummaryTest) isn't
called; extend it to cover another gated control (e.g., the asset download or
prompt test) by retrieving that button (for example by role/name like 'Download
Model Assets' or 'Run Prompt Test'), asserting it is disabled, firing a click,
and verifying the corresponding prop handler (e.g., props.onDownloadAssets or
props.onRunPromptTest) was not called; update the test that uses makeProps and
ModelDownloadSection to include this additional assertion so the
runtime-disabled gate is validated for multiple actions.
In `@app/src/components/settings/panels/local-model/ModelStatusSection.test.tsx`:
- Line 27: Add a new test in ModelStatusSection.test.tsx that mounts the
ModelStatusSection component with runtimeEnabled: false (instead of true) and
asserts that the "Bootstrap / Resume" and "Force Re-bootstrap" controls are
rendered in a disabled state; locate the setup helper used in existing tests
(e.g., the render or setup function that supplies props like runtimeEnabled) and
reuse it with runtimeEnabled: false, then query the buttons by their accessible
text or test-id and expect them to have the disabled attribute or
aria-disabled=true to validate the gating behavior when runtime is off.
🪄 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: 38b56b96-6223-4294-876e-b0067d1d0dab
📒 Files selected for processing (21)
app/src/components/settings/panels/AIPanel.tsxapp/src/components/settings/panels/LocalModelDebugPanel.tsxapp/src/components/settings/panels/LocalModelPanel.tsxapp/src/components/settings/panels/__tests__/AIPanel.test.tsxapp/src/components/settings/panels/__tests__/LocalModelPanel.test.tsxapp/src/components/settings/panels/local-model/ModelDownloadSection.test.tsxapp/src/components/settings/panels/local-model/ModelDownloadSection.tsxapp/src/components/settings/panels/local-model/ModelStatusSection.test.tsxapp/src/components/settings/panels/local-model/ModelStatusSection.tsxsrc/core/jsonrpc.rssrc/core/observability.rssrc/openhuman/agent/harness/tool_loop.rssrc/openhuman/agent/triage/routing.rssrc/openhuman/embeddings/factory.rssrc/openhuman/embeddings/ollama.rssrc/openhuman/embeddings/ollama_tests.rssrc/openhuman/providers/compatible.rssrc/openhuman/providers/compatible_tests.rssrc/openhuman/providers/reliable.rssrc/openhuman/routing/factory.rssrc/openhuman/tree_summarizer/ops.rs
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
/api,/v1, and/chat/completionsendpoints fail early with clear errors.local-*virtual model IDs for native Ollama embeddings, while preservinglocal-v1for OpenAI-compatible chat routing.Problem
/api/embed.config.local_ai.runtime_enabledwas off.Solution
OllamaEmbedding::try_newwith URL/model validation, kept legacynewfor internal callers, and validated the configured Ollama base URL in the embedding factory.AuthStyle::Nonefor local OpenAI-compatible Ollama routes and updated local provider construction to skip auth when no API key is configured.report_error_or_expectedto classifylocal ai is disabledas info-level and API-key-missing config errors as warning-level instead of sending them through the Sentry error path.Submission Checklist
## Related— N/A: Sentry/meta-tracker links listed below.Closes #NNNin the## RelatedsectionImpact
local-v1chat routing remains intact.Related
-D warningsgates can run cleanly.AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/ollama-bundle4dde3994acdff0b8cadea2a7cf95aa44c5449a51Validation Run
pnpm --filter openhuman-app format:check(via pre-push)pnpm typecheckcargo test --lib openhuman::embeddings;cargo test --lib openhuman::local_ai;cargo test --lib openhuman::providers::compatible;cargo test --lib core::observability;pnpm --filter openhuman-app test:unit src/components/settings/panels/__tests__/LocalModelPanel.test.tsx src/components/settings/panels/__tests__/AIPanel.test.tsx src/components/settings/panels/local-model/ModelDownloadSection.test.tsxcargo fmt --check;cargo check --manifest-path Cargo.tomlcargo fmt --manifest-path app/src-tauri/Cargo.toml --all --check;cargo check --manifest-path app/src-tauri/Cargo.toml(via pre-push)Validation Blocked
command:cargo clippy --workspace -- -D warningserror:Existing workspace warnings outside this change fail the global-D warningsgate; filtered clippy check found no hits in touched Rust files after fixes.impact:This PR's touched-file clippy issues were resolved; unrelated warning backlog remains.Behavior Changes
Parity Contract
local-v1remains valid for OpenAI-compatible chat routes; status/progress views still load while runtime is disabled.Duplicate / Superseded PR Handling
Summary by CodeRabbit
New Features
Bug Fixes
Tests