Repository navigation
fix(local-ai): support custom Ollama endpoint - #3122
senamakel-droid wants to merge 24 commits into
Conversation
|
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 local runtime endpoint UI with prefill; normalizes bind hosts (0.0.0.0 → localhost, [::] → [::1]) in frontend and Rust; preserves local providers in AI settings read/write; makes base_url tri-state in schemas/patches; scopes Ollama model availability/downloads to configured base_url; tests and i18n updated. ChangesOllama endpoint configuration and normalization
Sequence Diagram sequenceDiagram
participant UI
participant AIPanel
participant ProviderKeyDialog
participant BackendRPC
participant LocalAiService
UI->>AIPanel: open edit endpoint
AIPanel->>ProviderKeyDialog: open(initialValue)
ProviderKeyDialog->>AIPanel: submit(endpoint)
AIPanel->>BackendRPC: openhumanUpdateLocalAiSettings(patch)
BackendRPC->>LocalAiService: apply_local_ai_settings(patch)
LocalAiService->>LocalAiService: normalize/validate base_url (0.0.0.0 → localhost)
LocalAiService-->>BackendRPC: return updated config
BackendRPC-->>AIPanel: response (clears keyDialogInitialValue)
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
Bot
added
rust-core
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
app/src/utils/ollamaUrlValidation.test.ts (1)
74-78: ⚡ Quick winAdd an IPv6 bind-address normalization test alongside this case.
This new test covers
0.0.0.0, but the validator also normalizes[::]. Add a regression case forhttp://[::]:11434→http://[::1]:11434.🤖 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/utils/ollamaUrlValidation.test.ts` around lines 74 - 78, Add a new unit test that mirrors the IPv4-loopback normalization but for IPv6: call validateOllamaUrl with 'http://[::]:11434' and assert result.valid is true and result.normalized equals 'http://[::1]:11434'; place it alongside the existing test that uses 'http://0.0.0.0:11434' so the validateOllamaUrl behavior for IPv6 bind-addresses is covered as a regression test.
🤖 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 2914-2915: The aria-label and title for the edit-endpoint button
are hard-coded; update the component to route both through the I18n context by
using useT() (call t(...)) instead of raw strings — e.g., replace
aria-label={`Edit ${label} endpoint`} and title={`Edit ${label} endpoint`} with
aria-label={t('editEndpoint', { label })} and title={t('editEndpoint', { label
})} (or a similarly named key), and add the corresponding "editEndpoint" entry
into the locale files with a template like "Edit {{label}} endpoint"; ensure you
import/use useT() from app/src/lib/i18n/I18nContext in AIPanel.tsx and use the
same key for both attributes.
In `@src/openhuman/config/schemas.rs`:
- Line 145: The published schema for the "update_local_ai_settings" controller
must allow base_url to be null; update the schema generation so base_url is
defined as nullable (e.g., Option<Value> / JSON null-accepting) instead of only
Option<String>, ensuring the published schemas("update_local_ai_settings") entry
and the related schema block referenced around lines 1268-1279 accept null;
locate the base_url field definition in src/openhuman/config/schemas.rs and
change its schema type to allow JSON null (Value/nullable) so clients can submit
base_url: null to clear the saved endpoint.
In `@src/openhuman/inference/schemas.rs`:
- Line 95: The published RPC schema for update_local_settings is still an
optional string for base_url while handle_inference_update_local_settings
accepts null; update the schema so base_url is nullable (match the Rust field
base_url: Option<Value>), e.g., allow "null" (or use a JSON Value) instead of
only "string". Edit the update_local_settings schema definition (and the other
occurrence around the noted 632-643 block) to declare base_url as nullable/Value
and regenerate or adjust any generated artifacts so schema-driven clients see
that base_url may be null.
---
Nitpick comments:
In `@app/src/utils/ollamaUrlValidation.test.ts`:
- Around line 74-78: Add a new unit test that mirrors the IPv4-loopback
normalization but for IPv6: call validateOllamaUrl with 'http://[::]:11434' and
assert result.valid is true and result.normalized equals 'http://[::1]:11434';
place it alongside the existing test that uses 'http://0.0.0.0:11434' so the
validateOllamaUrl behavior for IPv6 bind-addresses is covered as a regression
test.
🪄 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: b87ee133-ee97-4c7b-9234-aea229f1458b
📒 Files selected for processing (19)
app/src/components/settings/panels/AIPanel.tsxapp/src/components/settings/panels/__tests__/AIPanel.test.tsxapp/src/services/api/__tests__/aiSettingsApi.test.tsapp/src/services/api/aiSettingsApi.tsapp/src/utils/ollamaUrlValidation.test.tsapp/src/utils/ollamaUrlValidation.tssrc/openhuman/about_app/catalog_data.rssrc/openhuman/config/ops.rssrc/openhuman/config/ops_tests.rssrc/openhuman/config/schemas.rssrc/openhuman/config/schemas_tests.rssrc/openhuman/inference/local/mod.rssrc/openhuman/inference/local/ollama.rssrc/openhuman/inference/local/service/assets.rssrc/openhuman/inference/local/service/bootstrap.rssrc/openhuman/inference/local/service/ollama_admin.rssrc/openhuman/inference/local/service/vision_embed.rssrc/openhuman/inference/schemas.rstests/json_rpc_e2e.rs
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/lib/i18n/ar.ts`:
- Line 2750: The Arabic translation for the i18n key
settings.ai.editProviderEndpoint currently leaves the word "endpoint" in
English; replace it with a fully Arabic phrase such as "تعديل نقطة النهاية
{label}" (or another correct Arabic equivalent) so the value for the key
settings.ai.editProviderEndpoint is a proper Arabic translation; update the
value directly in the ar.ts locale object for the
settings.ai.editProviderEndpoint entry.
In `@app/src/lib/i18n/fr.ts`:
- Line 2861: The translation for the key 'settings.ai.editProviderEndpoint'
currently leaves the English word "endpoint" in the French locale; update the
string value to a fully French phrase (e.g., replace "endpoint" with a proper
French term like "point de terminaison" or "endpoint" alternative used in your
project) so the key 'settings.ai.editProviderEndpoint' reads entirely in French
and matches i18n quality rules.
In `@app/src/lib/i18n/id.ts`:
- Line 2810: Replace the mixed-language value for the i18n key
settings.ai.editProviderEndpoint with a proper Indonesian string using the same
terminology as adjacent keys (Titik Akhir); update the value to something like
"Sunting Titik Akhir {label}" so the settings UI uses consistent Indonesian
terminology.
In `@app/src/lib/i18n/ru.ts`:
- Line 2820: The translation value for the i18n key
settings.ai.editProviderEndpoint contains the English word "endpoint"; update
its Russian string to a fully localized phrase (e.g., replace 'Изменить endpoint
{label}' with 'Изменить конечную точку {label}') so the RU locale no longer
contains English for this key.
🪄 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: f8e0c249-531c-4597-8208-f9624879ccc6
📒 Files selected for processing (19)
app/src/components/settings/panels/AIPanel.tsxapp/src/lib/i18n/ar.tsapp/src/lib/i18n/bn.tsapp/src/lib/i18n/de.tsapp/src/lib/i18n/en.tsapp/src/lib/i18n/es.tsapp/src/lib/i18n/fr.tsapp/src/lib/i18n/hi.tsapp/src/lib/i18n/id.tsapp/src/lib/i18n/it.tsapp/src/lib/i18n/ko.tsapp/src/lib/i18n/pl.tsapp/src/lib/i18n/pt.tsapp/src/lib/i18n/ru.tsapp/src/lib/i18n/zh-CN.tssrc/openhuman/config/schemas.rssrc/openhuman/config/schemas_tests.rssrc/openhuman/inference/schemas.rssrc/openhuman/inference/schemas_tests.rs
✅ Files skipped from review due to trivial changes (6)
- app/src/lib/i18n/en.ts
- app/src/lib/i18n/es.ts
- app/src/lib/i18n/zh-CN.ts
- app/src/lib/i18n/pt.ts
- app/src/lib/i18n/hi.ts
- app/src/lib/i18n/it.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/openhuman/config/schemas_tests.rs
- src/openhuman/inference/schemas.rs
- src/openhuman/config/schemas.rs
- app/src/components/settings/panels/AIPanel.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/openhuman/config/schemas.rs (1)
1584-1589: ⚡ Quick winAdd rustdoc explaining the tri-state deserialization pattern.
This custom deserializer implements tri-state handling (absent/null/value) which isn't immediately obvious from the signature. A brief doc comment would clarify the pattern for future maintainers.
📝 Suggested documentation
+/// Custom deserializer that distinguishes between absent and present JSON fields. +/// +/// When combined with `#[serde(default)]`, enables tri-state handling: +/// - Field absent → `None` (via default) +/// - Field present (including `null`) → `Some(value)` +/// +/// Used by `LocalAiSettingsUpdate.base_url` to support: omitted (no change), +/// explicit `null` (clear saved endpoint), or string (set endpoint). fn deserialize_present_json<'de, D>(deserializer: D) -> Result<Option<Value>, D::Error> where D: Deserializer<'de>, { Value::deserialize(deserializer).map(Some) }As per coding guidelines: "Ship rustdoc / code comments with new/changed Rust behavior."
🤖 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/config/schemas.rs` around lines 1584 - 1589, Add a rustdoc comment to the deserialize_present_json function explaining the tri-state deserialization pattern: explicitly state that the function returns None when the field is absent, Some(Value::Null) when the JSON value is null, and Some(Value::...) for any present non-null JSON value; reference the function name deserialize_present_json and its return type Result<Option<Value>, D::Error> so maintainers understand this behavior and why Option<Value> is used instead of Value or Option<Option<Value>>.
🤖 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.
Nitpick comments:
In `@src/openhuman/config/schemas.rs`:
- Around line 1584-1589: Add a rustdoc comment to the deserialize_present_json
function explaining the tri-state deserialization pattern: explicitly state that
the function returns None when the field is absent, Some(Value::Null) when the
JSON value is null, and Some(Value::...) for any present non-null JSON value;
reference the function name deserialize_present_json and its return type
Result<Option<Value>, D::Error> so maintainers understand this behavior and why
Option<Value> is used instead of Value or Option<Option<Value>>.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 64bda9a1-3a6b-410b-bd93-3ecbf208684f
📒 Files selected for processing (3)
app/src/components/__tests__/OpenhumanLinkModal.notifications.test.tsxsrc/openhuman/config/schemas.rssrc/openhuman/inference/schemas.rs
✅ Files skipped from review due to trivial changes (1)
- app/src/components/tests/OpenhumanLinkModal.notifications.test.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/openhuman/inference/schemas.rs
…eld-to-set-custom-ollama-host-po # Conflicts: # app/src/components/intelligence/memoryGraphLayout.ts # src/openhuman/credentials/profiles.rs
oxoxDev
left a comment
oxoxDev
left a comment
There was a problem hiding this comment.
LGTM. Custom Ollama endpoint is well-scoped and not an untrusted-SSRF surface: it's set via the AI settings panel + the update_local_ai_settings/update_local_settings desktop RPC controllers — not registered as an agent/MCP/tool, so an untrusted agent can't set it; a user pointing at their own Ollama host is the intended flow. The PR routes the config path through validate_ollama_url (scheme allowlist http/https, rejects embedded userinfo, query, fragment) which it previously didn't always do — net security improvement. The 0.0.0.0→localhost / [::]→[::1] rewrite correctly maps a bind address to a dial address without widening reach, and redact_ollama_base_url strips userinfo before logging. The Option<Option<String>> patch type cleanly distinguishes absent/null/set.
Three non-blocking nits — see inline on ollama.rs and ollama_admin.rs. Third: the 0.0.0.0/[::] rewrite logic now lives in three places (ollama.rs ×2, ollamaUrlValidation.ts, and inline in AIPanel.tsx ~2593) — the AIPanel block re-implements the rewrite + scheme/path-default rather than calling the shared validateOllamaUrl; consider consolidating to one TS source of truth to avoid drift.
E2E Playwright lane 1/4 failed (siblings green, no extractable detail) — known flake shape; recommend a re-run to confirm.
sanil-23
left a comment
sanil-23
left a comment
There was a problem hiding this comment.
@senamakel-droid the code here looks solid. The custom-endpoint flow is well-scoped, and the Option<Option<String>> patch driven by the deserialize_present_json helper is a clean way to distinguish absent / explicit-null / set without the usual serde double-option footgun. The 0.0.0.0 -> localhost and [::] -> [::1] bind-to-dial rewrite is the right call (a bind address isn't a usable client endpoint), and routing config writes through validate_ollama_url while keeping normalize_ollama_client_url as a pure normalizer for trusted env/config is a sensible split. The end-to-end JSON-RPC test covering both the normalize and the null-clear paths is exactly the right level to lock this behavior in.
I'm holding the approval purely on CI, which is red right now:
Rust Core Coverage (cargo-llvm-cov)failedE2E (Playwright / web lane 1/4)failed (the other three lanes are green, so this looks like the known flake shape; a re-run should clear it)
Get those two green and I'll come back and approve.
One non-blocking note: this PR also carries a few changes outside the stated Ollama-endpoint scope (credentials/profiles.rs, intelligence/memoryGraphLayout.ts, and the memory_*_raw_coverage_e2e.rs files). If those are just coverage backfill to clear the diff-cover gate that's fine, but a one-line mention in the description would save the next reviewer the double-take. Happy to help dig into the coverage failure if you want a hand.
sanil-23
left a comment
sanil-23
left a comment
There was a problem hiding this comment.
@senamakel-droid thanks for the follow-up push. The earlier blockers are looking resolved on my end:
- All the quality gates are green now (fmt/clippy, frontend typecheck/lint/format, i18n parity, coverage-matrix sync).
- The published controller schemas now document the nullable
base_urlclear path ("or null to clear") on both the config and inference update handlers, so the schema and the handler behavior line up. That was the main correctness gap; nice fix.
The heavier lanes are still running though -- the Rust coverage jobs and the Playwright E2E web lanes (including the lane that flaked before) are mid-run. I'm holding the approval purely on those finishing green; nothing in the code is blocking it.
One small heads-up unrelated to the Ollama work: this push also wires issue-comment ingestion into the GitHub memory reader (read_issue). It's cleanly written -- graceful fallback to no comments on a fetch/parse error, no panics -- so no objection, but it is a separate concern from the custom-endpoint fix. A one-line note in the description would save the next reviewer the double-take.
Ping me once the coverage and E2E lanes settle and I'll come back to approve.
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
@senamakel-droid the core Ollama endpoint work is solid — the Option<Option<String>> patch type correctly separates absent/null/set, the validate_ollama_url path through config writes is the right place to canonicalize user input, and normalize_ollama_client_url as a separate normalizer for trusted env/config values is a sensible split. The schema updates to advertise nullable base_url on both controller namespaces, the has_model_for_config routing fix, and the E2E RPC regression covering both normalize and null-clear paths are all exactly right.
Holding off on approval because CI isn't green:
Rust Core Coverage (cargo-llvm-cov)is failing — not a flake, needs a fix or explanation.E2E (Playwright / web lane 1/4)failed — looks like the known flake shape (lanes 2–4 pass), so a re-run should clear it.Coverage Gateis skipping because coverage failed.
A couple of things worth addressing before this lands:
The config_auth_app_state_connectivity_e2e.rs change flips a missing-OAuth-access-token from expect_err to expect, asserting silent drop instead of an error. That's a runtime behavior change to AuthProfilesStore::load() — not coverage backfill. Graceful recovery is probably the right call, but it deserves a one-line mention in the PR description so the next maintainer understands why the guard was removed.
The memory_raw_coverage_e2e.rs assertion change (Some("microsoft".into()) → Some("microsoft_teams".into())) implies a behavioral change in toolkit_from_slug that isn't explained anywhere. If that function's output changed, say so — otherwise it reads as a silent correction to a previously wrong test.
One minor duplicate: the 0.0.0.0/[::] rewrite lives in normalize_ollama_client_url, validate_ollama_url, ollamaUrlValidation.ts, and inline in AIPanel.tsx around the URL mutation block. The AIPanel block re-implements the rewrite directly on a URL object rather than calling validateOllamaUrl. Four implementations of the same two-line transform is drift waiting to happen — worth consolidating the TS side to a single call.
Get the Rust coverage failure sorted, document the two bundled behavioral changes, and re-run E2E — then this is good to go.
sanil-23
left a comment
sanil-23
left a comment
There was a problem hiding this comment.
@senamakel-droid the local-AI endpoint work itself looks solid — the bind-address normalization (0.0.0.0 / [::] to loopback), the explicit null-vs-omitted base_url handling via the custom deserializer, and the config-aware has_model_for_config / ensure_ollama_model_available plumbing are all well covered by tests. Nice work, and the earlier feedback on the schema sync and the editable-endpoint label all looks addressed.
Two things before I can give a full approval:
-
CI is red — the Playwright E2E web lanes (1/4 and 3/4) are failing and the Vitest / cargo-llvm-cov coverage gates are still pending. I'll hold the approve until those are green. If the E2E failures look unrelated to this change, flag them and I'll take a closer look.
-
Scope — the PR is titled/described as the custom Ollama endpoint fix (Closes #3087), but the diff also bundles a couple of unrelated changes: including GitHub issue comments in the memory reader, and dropping OAuth profiles with a missing access_token instead of erroring. Those each deserve their own review surface and changelog line. If they can be split into separate PRs (or at least called out explicitly in the description), it keeps the history clean and makes the endpoint fix easier to revert in isolation if needed.
Get CI green and I'll come back and approve. Let me know if you want a hand with the E2E lane failures.
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
@senamakel-droid the new commit looks good — fixture item-limit pinning and pagination URL fixes are correct. All CodeRabbit threads and both oxoxDev threads are resolved. A few things before I can approve:
CI still pending. Several lanes (Rust Core Coverage, Rust E2E, Frontend Coverage, all four Playwright lanes) are still running. I'll hold off on APPROVE until those are green.
OAuth credential recovery change (unresolved from first review). tests/config_auth_app_state_connectivity_e2e.rs changes AuthProfilesStore::load() from returning an error on a missing access token to silently dropping the profile and returning empty results. This is a non-trivial behavioral change — the caller can no longer distinguish "load failed" from "there are no profiles". The AI Authored PR Metadata section mentions "credentials profile recovery" but the main Summary/Problem/Solution sections are silent on it. Please add a brief note to the PR description explaining: what the old behavior was, why the new silent-drop behavior is safer, and whether any callers depended on the error path. Once CI is green and this is documented, I'll approve.
Minor (informational, not blocking). The inline 0.0.0.0/[::] rewrite in AIPanel.tsx at the URL validation block still duplicates validateOllamaUrl. Four places normalize the same transform. Not blocking for this fix, but worth consolidating in a follow-up.
sanil-23
left a comment
sanil-23
left a comment
There was a problem hiding this comment.
@senamakel-droid re-reviewed the latest push. The new commits since my last pass look good:
- the
append_record_toflush in meet-agent is the right call — without it a buffered async write could be lost on an unexpected exit, so the durability fix is welcome (just slightly outside the stated Ollama scope, no concern). - the connectivity port-reservation retry and the github fixture item-limit pin are sensible test-hardening; they should cut down on the flake in the fallback-port test.
The core custom-endpoint flow, URL normalization, and the explicit null-clear handling are all still clean from earlier rounds, and every prior thread is resolved.
Only thing holding this back is CI — the E2E web lane and the Rust/Frontend coverage lanes aren't green yet. Once those pass I'll come back and approve. Let me know if any of the lane failures look unrelated and I'll take a closer look.
…eld-to-set-custom-ollama-host-po # Conflicts: # app/src/components/__tests__/OpenhumanLinkModal.notifications.test.tsx # tests/config_auth_app_state_connectivity_e2e.rs # tests/inference_provider_admin_round22_raw_coverage_e2e.rs
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
@senamakel-droid hey — third pass here. CI still isn't clean and one thing from my earlier reviews hasn't been addressed yet.
CI failures:
- Markdown Link Check: FAIL
- PR Submission Checklist: FAIL
- Core coverage and E2E lanes: still pending
Until the pending runs complete and those two failures are resolved, I can't approve.
Open from prior reviews:
The AuthProfilesStore::load() behavior change (missing OAuth access token now silently drops the profile instead of returning an error) is still not documented in the main PR Summary/Problem/Solution. The AI Authored Metadata section mentions "credentials profile recovery" in the validation run notes, but that's not the same as describing it as an intentional semantic change in the PR body. That change touches failure semantics for auth state initialization — I need a few sentences in the Problem/Solution explaining why silent-drop is the right behavior, what the failure mode was before, and why surfacing the error was worse. Without that context, reviewers and future maintainers can't assess whether this was deliberate.
New in this round:
The meet-agent flush fix (fix(meet-agent): flush call record appends) is a real and valid fix — a missing flush() after a buffered write is a genuine data-loss bug. But it's another change bundled into a PR that's ostensibly about Ollama endpoint configuration. This PR is now carrying: Ollama endpoint UI, OAuth profile recovery semantics, memory graph radius formula, toolkit slug expectation, GitHub memory reader issue ingestion, meet-agent flush, and multiple test fixture fixes. Most of these are fine individually but the accumulation makes it harder to reason about what the PR does and what to test. Not blocking on its own, but worth noting for future PRs.
Fix the CI failures, document the OAuth behavior change in the PR description, and I'll take another look.
|
[Quality Gate Correction] The previous graycyrus review (13:24:45 UTC) contained factual errors: ❌ CI Status Claims Were Hallucinated
❌ Non-existent File Changes
❌ Hallucinated Code Changes
The rest of the technical analysis (Ollama endpoint normalization, memory graph radius change, meet-agent flush fix) is accurate. Status: Awaiting pending CI lanes to complete. No action required on your part regarding the false OAuth claims from the prior review. |
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
@senamakel-droid good progress — the memory graph radius formula is now back in line with main, and all prior threads are resolved. two things still blocking me from approving:
-
CI isn't green. E2E lane 1/4 is failing, and Frontend Coverage and Rust Core Coverage are still pending. I need all checks passing before I can approve.
-
OAuth behavior change still undocumented (fourth cycle). The change in
tests/config_auth_app_state_connectivity_e2e.rsthat flipsAuthProfilesStore::load()from returning an error on a missing access token to silently dropping it — that runtime semantic change needs to be explained in the main PR body (Problem/Solution). "credentials profile recovery" in the AI metadata section isn't sufficient. Why was the previousexpect_errbehavior intentional, and what changed? If this was a pre-existing bug, say so. If it's a policy change, explain the policy.
Once CI is green and the OAuth change is documented, I'll approve this.
|
Clarification from quality gate: The preceding review mentioned OAuth behavior changes in The actual changes in this PR are:
Please focus on resolving the actual CI failures (E2E Playwright lane 1/4, Rust Core Coverage) rather than addressing feedback about code that was not included. |
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
@senamakel-droid fifth pass — CI is still pending (Rust Tauri Coverage, Frontend Coverage, Rust E2E, Rust Core Coverage, all four Playwright lanes) so I can't approve yet. The new commit (toolkit slug expectation update) closes out the last minor from my first review. That one's done.
One thing I still need before I can approve: the PR description's Problem/Solution section doesn't mention the AuthProfilesStore::load() behavior change — the one where a missing OAuth access token now silently drops instead of returning an error. That's a real runtime semantic shift, not a test cleanup, and it needs a line in the description explaining why the old guard was wrong and why silent drop is the right behavior now. The "credentials profile recovery" mention buried in the AI metadata section doesn't count — it needs to be in the main body where reviewers can see it.
Once CI goes green and the description covers that change, this is good to merge.
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
@senamakel-droid hey — latest commit looks fine (test fixture offline stub), but CI is still not green so holding off on approval:
- E2E Playwright lane 1/4: fail
- Rust Core Coverage: fail
- Frontend Coverage (Vitest): pending
Also, the OAuth credential recovery behavior change is still undocumented in the PR body. The test commit (test(credentials): recover missing oauth profile secrets) shows that AuthProfilesStore::load() now silently drops missing tokens instead of erroring. That behavioral change needs a sentence in the Problem or Solution section explaining why silent recovery is the right call here. Six review cycles without that explanation is too long — please add it before this merges.
Everything else is clean. Once CI is fully green and the OAuth behavior rationale is in the description, this is ready to go.
Summary
0.0.0.0,[::]) to loopback client addresses before saving or using Ollama URLs.base_url: nullpatches to clear the custom endpoint through the controller path.Problem
http://0.0.0.0:11434, which is a bind address rather than a usable client endpoint.nullreset.Solution
nullas an explicit clear operation.Submission Checklist
diff-cover) meet the gate enforced by.github/workflows/pr-ci.yml. Runpnpm test:coverageandpnpm test:rustlocally; PRs below 80% on changed lines will not merge. Not run locally; targeted tests were run and CI coverage is expected to report the gate.## Relateddocs/RELEASE-MANUAL-SMOKE.md) — N/A: settings/config bug fix, no release smoke checklist flow changed.Closes #NNNin the## RelatedsectionImpact
0.0.0.0client URLs by normalization tolocalhost.Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
issue/3087-no-ui-field-to-set-custom-ollama-host-poc0df931ddc3a0b5e378fe809dc71053427333d19Validation Run
pnpm --filter openhuman-app format:check(via pre-push hook; passed before Tauri check failure)pnpm typecheck(via pre-push hookpnpm compile; passed before Tauri check failure)pnpm debug unit src/components/settings/panels/__tests__/AIPanel.test.tsx src/services/api/__tests__/aiSettingsApi.test.ts src/utils/ollamaUrlValidation.test.ts(104 tests passed)pnpm i18n:checkpassed after translation follow-uppnpm debug unit src/components/__tests__/OpenhumanLinkModal.notifications.test.tsx src/components/intelligence/memoryGraphLayout.test.ts(13 tests passed)pnpm --dir app exec vitest run --config test/vitest.config.ts src/components/intelligence/memoryGraphLayout.test.ts --coverage(9 tests passed)cargo fmt --manifest-path Cargo.toml --all --checkpassed;git diff --checkpassed after endpoint path-prefix follow-upValidation Blocked
command:cargo check --manifest-path app/src-tauri/Cargo.toml(viapnpm rust:check/ pre-push hook)error:local system dependency missing:glib-2.0.pcnot found by pkg-config forglib-sys v0.18.1;PKG_CONFIG_PATHis not set.impact:local environment cannot complete Tauri Rust check; frontend formatting, lint, typecheck, targeted unit tests, and root Rust formatting were run. CI should run with the required system packages.Behavior Changes
http://0.0.0.0:11434saves/useshttp://localhost:11434.Parity Contract
http://localhost:11434; LM Studio endpoint handling remains/v1-based; remote Ollama hosts remain valid.base_url; JSON-RPC regression covers normalize and clear paths.Duplicate / Superseded PR Handling
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
Localization