Skip to content

fix(hindsight): validate and normalize api_url, fail fast on bad config - #78968

Closed
Realmagnum wants to merge 1 commit into
NousResearch:mainfrom
Realmagnum:fix/hindsight-api-url-validation
Closed

Realmagnum wants to merge 1 commit into
NousResearch:mainfrom
Realmagnum:fix/hindsight-api-url-validation

Conversation

@Realmagnum

Copy link
Copy Markdown

Summary

Fixes #78967 — a schemeless api_url (e.g. "host:8888") made every hindsight_* tool fail with a cryptic Failed to search memory: host:8888/v1/... error, because the hindsight client concatenates base_url + path into an invalid URL and the plugin never validated the config.

This PR makes the provider validate/normalize api_url and fail fast with actionable messages:

  • _normalize_api_url() — auto-adds a missing scheme (http:// for local modes, https:// for cloud) with a warning log, and rejects invalid schemes (ftp://, etc.) with an error naming the exact config file to fix.
  • is_available() — validates the effective api_url up front; an invalid scheme returns False and logs the reason, so the provider is never registered with broken tools (same philosophy as fix(hindsight): log why HindsightMemoryProvider fails to register instead of failing silently #35332).
  • initialize() — disables the provider on an invalid scheme (belt-and-braces, mirroring the existing local-runtime-unavailable path).
  • handle_tool_call() — returns a clear "provider is disabled, fix api_url in config.json" error instead of a cryptic URL failure.
  • _diagnose_scheme_mismatch() — for local_external + https://, probes /version once per session; if the configured scheme fails but the opposite one answers, logs the exact corrected URL (catches the common https://-to-plain-HTTP-daemon mixup that passes validation).
  • Tool errors now include the exception type (Failed to search memory: ValueError: ...).

Tests

New file tests/plugins/memory/test_hindsight_api_url_normalization.py (27 tests): normalization by mode, whitespace/empty handling, invalid-scheme rejection, provider disable, is_available() behavior, disabled-tool error, scheme-mismatch diagnosis (configured works / https-to-http / http-to-https / both down / non-http ignored), and a guard that plain-http:// configs never touch the network during init.

All existing hindsight tests pass (70 tests in test_hindsight_provider.py, test_hindsight_config_schema.py, test_hindsight_env_perms.py, test_hindsight_health_grace_timeout.py, test_hindsight_root_guard.py).

Compatibility

Test plan

  • pytest tests/plugins/memory/test_hindsight_api_url_normalization.py -o 'addopts=' -q → 27 passed
  • Full hindsight suite → 97 passed

Fixes #78967

@alt-glitch alt-glitch added type/bug Something isn't working comp/plugins Plugin system and bundled plugins tool/memory Memory tool and memory providers area/config Config system, migrations, profiles P3 Low — cosmetic, nice to have sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Aug 5, 2026
@Realmagnum
Realmagnum force-pushed the fix/hindsight-api-url-validation branch from ed3b349 to fc0b2b6 Compare August 15, 2026 11:06
@Enough1122

Copy link
Copy Markdown

AI code review — automated review for reference, author can ignore or act on any point.

PR #78968: fix(hindsight): validate and normalize api_url, fail fast on bad config

  • is_available and initialize disagree on the value they check: is_available computes has_url = bool(api_url) from the raw config value, while initialize() uses the normalized one. A whitespace-only api_url (" ") is truthy, so is_available returns True, but initialize() normalizes it to "" — the provider registers as available yet is effectively broken. Normalize in one place (or compare the normalized result in has_url) so both paths agree.
  • Scheme-mismatch probe sends the API key to the swapped-scheme host: _diagnose_scheme_mismatch calls _fetch_hindsight_api_version(swapped, api_key, ...), so for an https:// → http:// diagnosis the key would travel over plaintext HTTP to the swapped target. Probe without credentials first (or restrict the probe to localhost/private hosts) so the diagnostic can't leak the key.
  • Per-session network probe: for local_external + https, initialize() performs a probe (2s timeout) on every session start — adds latency and a network dependency at init. Consider caching the diagnosis per process or gating it to config changes.
  • Minor: _normalize_api_url returns "" for whitespace-only input instead of falling back — combined with the first point, callers end up holding "" rather than their default URL. Returning None after strip would let callers use their defaults consistently.
  • Good fail-fast disabled guard in handle_tool_call, and the exception-type suffix on tool errors is a nice debugging improvement.

A schemeless api_url (e.g. "host:8888") made every hindsight_* tool fail
with a cryptic "Failed to search memory: host:8888/v1/..." error because
the client concatenates base_url + path into an invalid URL.

- _normalize_api_url(): auto-add missing scheme (http for local modes,
  https for cloud) with a warning; reject invalid schemes (ftp://, ...)
  with an actionable error naming the config file
- is_available(): return False and log the reason when api_url is invalid,
  so the provider is not registered at all
- initialize(): disable the provider on invalid scheme (belt-and-braces)
- handle_tool_call(): clear message when the provider is disabled
- _diagnose_scheme_mismatch(): detect https-to-http local daemon mixups
  via a one-shot /version probe (local_external + https only)
- tool errors now include the exception type

Adds tests/plugins/memory/test_hindsight_api_url_normalization.py (27 tests).
@teknium1

Copy link
Copy Markdown
Collaborator

Closing: the bundled Hindsight provider this PR patches has moved out of this repo.

Thanks @Realmagnum for this contribution. In #119888 (merge 9d799e0531c; removal commit 4cbf862abe4) the in-tree plugins/memory/hindsight/ provider was removed — Hindsight now installs from the plugin catalog and its code lives in vectorize-io/hindsight hindsight-integrations/hermes (maintained by @nicoloboschi). There is no longer any code in this repo for the Hindsight half of this PR to patch, so we are closing every open PR against the bundled provider rather than leaving them stranded.

Triage notes:

  • Only touches the removed bundled provider. At the pin api_url is still taken verbatim (hindsight-integrations/hermes/init.py:999, _apply_connection_settings) with no scheme normalization or fail-fast, so the schemeless-URL misconfiguration still produces cryptic per-call errors. The scheme-mismatch network probe / per-process diagnosis cache is an add-on feature worth proposing separately upstream.
  • Still relevant at the catalog pin (dc750388)? Yes — the same code is at hindsight-integrations/hermes/__init__.py:999 in the upstream tree. It is listed with your credit in Fixes from Hermes-side PRs worth carrying into hindsight-integrations/hermes vectorize-io/hindsight#4662 so it is not lost; if you want to carry the fix yourself, please open it against vectorize-io/hindsight — it would be welcome there.

If you believe this was closed in error, comment and we will reopen.

(Bulk-closed in the hindsight-move close pass.)

@teknium1 teknium1 closed this Sep 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/config Config system, migrations, profiles comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades tool/memory Memory tool and memory providers type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Hindsight api_url without a scheme makes every hindsight_* tool fail with a cryptic error

4 participants