feat(settings): add connection-test + reachability reporting to taOSmd memory URL#1931
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Warning Review limit reached
Next review available in: 47 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe memory URL settings route now returns locality and reachability status, probes configured taOSmd endpoints, caches probe results, normalizes URLs, validates input, and synchronizes persisted configuration. Tests cover GET/PUT behavior, probing, locality, persistence, and validation. ChangesMemory URL reachability
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant SettingsRoutes
participant taOSmd
Client->>SettingsRoutes: PUT /api/settings/memory-url
SettingsRoutes->>SettingsRoutes: Validate and normalize URL
SettingsRoutes->>taOSmd: GET /health
taOSmd-->>SettingsRoutes: Reachability result
SettingsRoutes-->>Client: url, is_local, reachable
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| is_local = _is_local_url(url) | ||
| reachable = await _probe_taosmd(request, url) | ||
|
|
||
| request.app.state.taosmd_url = url |
There was a problem hiding this comment.
WARNING: Docstring claims the URL is validated "before accepting it", but taosmd_url is persisted unconditionally even when the probe fails (reachable == False).
Either the docstring is wrong, or the PUT should reject unreachable targets. As written, a bogus/unreachable URL is accepted and saved to app.state, and then GET /api/settings/memory-url will report it as the configured URL. Make the behavior explicit: either return 400/422 when not reachable, or change the docstring to "probes and persists regardless of reachability".
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| @router.put("/api/settings/memory-url") | ||
| async def set_memory_url(request: Request): | ||
| """Set the taOSmd memory URL, probing the new target before accepting it.""" | ||
| body = await request.json() |
There was a problem hiding this comment.
WARNING: await request.json() is not guarded. If the client sends a non-JSON or malformed body (e.g. empty body, wrong content-type), this raises and surfaces as a 500 rather than a clean 400.
Wrap it in a try/except (ValueError, TypeError) (or use a Pydantic request model like the rest of this file) and return a 400, consistent with the empty-URL and bad-scheme checks below.
| body = await request.json() | |
| try: | |
| body = await request.json() | |
| except Exception: | |
| return JSONResponse({"error": "Invalid JSON body"}, status_code=400) |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| async def set_memory_url(request: Request): | ||
| """Set the taOSmd memory URL, probing the new target before accepting it.""" | ||
| body = await request.json() | ||
| url = (body.get("url") or "").strip().rstrip("/") |
There was a problem hiding this comment.
SUGGESTION: rstrip("/") strips the set of characters, not just a trailing slash, so an input like "http://a//" collapses to "http://a" and any trailing run of / is removed. For a single trailing slash this is fine, but the intent reads better and is safer with a true suffix strip:
| url = (body.get("url") or "").strip().rstrip("/") | |
| url = (body.get("url") or "").strip() | |
| if url.endswith("/"): | |
| url = url[:-1] |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| """Return the configured taOSmd memory URL with local/reachable probes.""" | ||
| url = _taosmd_default_url(request) | ||
| is_local = _is_local_url(url) | ||
| reachable = await _probe_taosmd(request, url) |
There was a problem hiding this comment.
SUGGESTION: GET /api/settings/memory-url performs a live network probe (up to a 3s timeout) on every call. For a frequently-polled settings endpoint this adds latency and a hard dependency on taOSmd being up. Consider caching the probe result with a short TTL (or only probing on demand) so the endpoint stays fast and resilient when taOSmd is offline.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (1 file)
Incremental change verified
Previous Review Summaries (4 snapshots, latest commit abdabfa)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit abdabfa)Status: No Issues Found | Recommendation: Merge Files Reviewed (1 file)
Previous Findings — Resolved in this update
Previous review (commit 039adc8)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Previous Findings — Resolved in this update
Files Reviewed (2 files)
Previous review (commit a728593)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Previous Findings — Resolved in this updateThe 4 findings from the prior review at
Files Reviewed (2 files)
Fix these issues in Kilo Cloud Previous review (commit f9fe546)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (2 files)
Reviewed by hy3:free · Input: 68.5K · Output: 2.2K · Cached: 105.8K |
|
Thanks hognek. The reachability code is right (GET returns {url,is_local,reachable}, PUT probes {url}/health with an admin-gated 3s timeout, 10 tests pass), BUT do not merge or plain-rebase as-is: this branch predates #1904, which already shipped a GET+PUT /api/settings/memory-url pair on dev. Merging onto current dev registers the route TWICE (ruff F811 flags both get_memory_url/set_memory_url as redefinitions), and FastAPI serves your first-registered handlers, which makes #1904's config-backed pair dead code. That is a regression: your PUT only sets app.state.taosmd_url in memory and never calls save_config_locked, so the URL stops persisting across restart, and your GET ignores config.memory_url. The green tests miss it because #1904 shipped no route tests. Fix: rebase onto current dev (b0f3993) and FOLD the reachability logic INTO the existing #1904 endpoints, do not add a second pair. Add is_local/reachable to #1904's GET, add the probe to #1904's PUT that already persists via save_config_locked + the Pydantic MemoryUrlUpdate model. While there, fold the Kilo nits: the docstring says validate-before-accept but it persists even when reachable=False (decide: accept-anyway or 400), and use the Pydantic model instead of the unguarded await request.json() (avoids a 500 on a bad body). Keeping #1911 open until this lands. |
…— fold reachability logic into existing routes - Fold _is_local_url, _probe_taosmd, and _MEMORY_URL_PROBE_CACHE into jaylfc#1904's existing GET+PUT /api/settings/memory-url (no duplicate route registration). - GET returns {url, is_local, reachable} with 30s TTL probe cache. - PUT validates via Pydantic MemoryUrlUpdate model (no unguarded request.json()), persists via save_config_locked, and returns {url, is_local, reachable}. - Trailing-slash stripping uses explicit url.endswith('/') + url[:-1] instead of rstrip('/') character-set behavior. - Docstring explicitly states URL is always accepted and persisted; reachable field reports probe result but does not gate the save. - 13 tests: default URL, local/remote detection, PUT persistence, validation (empty URL, non-http scheme, missing hostname, malformed JSON), unreachable probe, trailing-slash strip, and _is_local_url unit coverage. - Closes jaylfc#1911. Fixes Kilo 2W+2S on PR jaylfc#1931.
f9fe546 to
a728593
Compare
|
Reconciled with #1904 — pushed commit jaylfc feedback (route duplication)
Kilo 2W+2S (all fixed)
Tests13 tests (10 original + 3 new: malformed JSON 422, Closes #1911. |
| # Memory URL (taOSmd) | ||
| # --------------------------------------------------------------------------- | ||
|
|
||
| _MEMORY_URL_PROBE_CACHE: dict = {} # {url: (timestamp, bool)} |
There was a problem hiding this comment.
SUGGESTION: _MEMORY_URL_PROBE_CACHE grows without bound across the process lifetime.
Every distinct URL probed (via GET or PUT) is appended to this module-level dict and never evicted. Over a long-running server, repeated distinct URLs (e.g. a misconfigured client cycling hostnames) will accumulate indefinitely and leak memory. Consider bounding it with a max size (e.g. collections.OrderedDict + LRU eviction) or a TTL-based cleanup so stale entries are removed.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/test_routes_settings.py (1)
381-382: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winTest durable and runtime persistence, not only the in-memory assignment.
This assertion would pass if both
save_config_lockedand theapp.state.taosmd_urlsynchronization were removed. Reload/readconfig_pathand assert the runtime override as well.🤖 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 `@tests/test_routes_settings.py` around lines 381 - 382, Update the test around the config persistence assertion to reload the configuration from config_path and verify the overridden memory URL was durably saved, then separately assert the runtime app.state.taosmd_url synchronization. Retain the existing app.state.config assertion only if it remains relevant.
🤖 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 `@tinyagentos/routes/settings.py`:
- Around line 1146-1150: Update the URL validation near parsed in the settings
route to require parsed.hostname, not merely parsed.netloc, so inputs such as
http://:7900 return the existing hostname error and are not persisted. Add a
regression test covering http://:7900 and verify it is rejected before probing
or persistence.
- Around line 1098-1108: Update _probe_taosmd to catch only the HTTPX request
and URL-related exceptions expected from the health check, rather than broad
Exception; leave unexpected failures such as a missing
request.app.state.http_client able to propagate while preserving False for
handled probe errors.
---
Nitpick comments:
In `@tests/test_routes_settings.py`:
- Around line 381-382: Update the test around the config persistence assertion
to reload the configuration from config_path and verify the overridden memory
URL was durably saved, then separately assert the runtime app.state.taosmd_url
synchronization. Retain the existing app.state.config assertion only if it
remains relevant.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e6619fd6-f8d3-4bee-83a9-012be45b8515
📒 Files selected for processing (2)
tests/test_routes_settings.pytinyagentos/routes/settings.py
|
The original hold is resolved, thanks: the reconcile folds into #1904's single GET+PUT pair (no route duplication), PUT now persists via
Nits (optional): |
…y-url hostname validation parsed.netloc returns ':7900' for http://:7900 (non-empty, bypasses validation). parsed.hostname correctly returns empty string, catching port-only URLs with no hostname. Added regression test for http://:7900 alongside existing http:///path case.
|
The netloc->hostname fix + the http://:7900 regression test look right, thanks. One nit from my earlier review is still open: |
…Dict FIFO eviction Use collections.OrderedDict with popitem(last=False) to evict oldest entry when cache exceeds 32 entries, preventing unbounded growth. Closes review nit on jaylfc#1931.
|
Close - most earlier findings were resolved in your rewrite. Two left: routes/settings.py ~1109 the _MEMORY_URL_PROBE_CACHE module dict grows unbounded (never evicted) on a frequently-polled endpoint - add an eviction/size cap. And confirm the old persist-on-probe-fail vs docstring mismatch survived the rewrite: the docstring says the URL is validated before accepting, but taosmd_url was persisted even when reachable==False - either gate the write on reachable or fix the docstring. Fold + green and I merge. |
Catch only (httpx.RequestError, httpx.InvalidURL) instead of the broad Exception, so unexpected failures (e.g. missing http_client) propagate rather than being silently swallowed as 'unreachable'. Closes CodeRabbit finding on PR jaylfc#1931.
|
All three findings are now addressed:
Validation tests pass (empty URL, non-http scheme, missing hostname). Ready for review. |
…o-remote control - Add _taosmd_tier helper reading taosmd_default.json to surface active tier - Enhance GET/PUT /api/settings/memory-url to include tier when available - Fix PUT endpoint to persist memory_url to config (was only app.state) - Remove old duplicate memory-url endpoints superseded by PR jaylfc#1931 - Add TaOSmdEndpointCard component: LOCAL/REMOTE badge, reachability, backend capabilities, tier badge, switch-to-remote URL input, connection test, revert-to-local button, ARIA labels - Add fetchMemoryEndpoint/updateMemoryEndpoint to memory.ts client - Closes jaylfc#1892
2 participants
Closes #1911.
Adds GET /api/settings/memory-url returning
{url, is_local, reachable}with a health-probe against the configured taOSmd. Adds PUT endpoint that validates the URL, probes the new target, and persists it toapp.state.taosmd_url.Changes:
tinyagentos/routes/settings.py: Add_is_local_url,_probe_taosmd,_taosmd_default_urlhelpers +GETandPUT /api/settings/memory-urlendpointsparsed.netloc→parsed.hostnamein hostname validation (line 1149) —http://:7900was bypassing validation because netloc returns:7900(non-empty) while hostname correctly returns empty stringtests/test_routes_settings.py: Addedhttp://:7900regression test totest_put_rejects_url_without_hostname_is_local_urlunit coverageTests: targeted MemoryUrl validation tests (3/3 pass). Full gate run timing out on pre-existing probe-hang issue — CI will run full matrix.