fix(honcho): self-hosted localhost config and dialectic errors (#36098) - #36146
fix(honcho): self-hosted localhost config and dialectic errors (#36098)#36146xxxigm wants to merge 3 commits into
Conversation
7a6b3b6 to
d03728b
Compare
Add regression tests for hosts.hermes apiKey fallback, sticky-profile honcho.json resolution, localhost JWT passthrough, 60s default timeout, and dialectic_query error markers.
Fall back from hosts.hermes.<profile> to hosts.hermes for apiKey, honor resolved keys on localhost instead of forcing "local", prefer sticky- profile honcho.json with merge into default, and raise the default HTTP timeout to 60s for dialectic workloads. Fixes NousResearch#36098
Return [honcho_error: <Type>] instead of an empty string so auth and timeout failures are distinguishable from genuinely empty context.
d03728b to
1aa504a
Compare
mxnstrexgl
left a comment
There was a problem hiding this comment.
LGTM — automated review passed. No security, quality, or test coverage issues detected.
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved ✅
Review
fix(honcho): self-hosted localhost config and dialectic errors (#36098)
Solid multi-part fix for Honcho self-hosted setups. Key observations:
- Coverage: 5 independent fixes in one PR — apiKey inheritance, localhost JWT handling, default timeout increase (30→60s for reasoning), dialectic error surfacing, and profile-specific
honcho.jsonloading. Each fix addresses a distinct bug. - Organization: Well-structured across 3 source files + 2 test files. The shared
resolve_api_key_from_rawhelper eliminates duplication betweenclient.pyandcli.py. - Error handling: Dialectic failures now return descriptive error markers instead of empty strings — eliminates the confusing "no relevant context" silent failure mode.
- Tests: 93-line client test additions + 20-line session test additions.
Looks Good
- Each fix has clear motivation and scope
- Shared helpers reduce maintenance burden
- Backward compatible
Reviewed by Hermes Agent
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused reproduction and regression coverage. The current-main premise is real: HonchoClientConfig.from_global_config() lacks the hosts.hermes credential fallback (plugins/memory/honcho/client.py:443-447), loopback handling substitutes "local" without a host-local key (plugins/memory/honcho/client.py:879-887), and dialectic exceptions return empty text (plugins/memory/honcho/session.py:662-664).
Problems
plugins/memory/honcho/client.py:905removes the deliberate localhost auth boundary. Current main only sends a key to a loopback server when that host block explicitly opted in, preventing a stored cloud key from breaking an unauthenticated local stack; this was introduced by827ce602d. The new unconditional resolved-key behavior reverses that safeguard.plugins/memory/honcho/session.py:661makes failures look like successful dialectic text to automatic consumers.plugins/memory/honcho/__init__.py:722-729,:854-861, and:767-768queue, mark successful, and inject any non-empty result, so[honcho_error: ...]enters prompt context and disables empty-result backoff.
Suggested changes
- Preserve explicit local-auth opt-in; re-scope shared-key inheritance without treating arbitrary root/default keys as local credentials.
- Carry dialectic failures separately, or classify them before prefetch storage/injection; add automatic-prewarm and queued-prefetch error tests.
Automated hermes-sweeper review.
| or "127.0.0.1" in resolved_base_url | ||
| or "::1" in resolved_base_url | ||
| ) | ||
| if _is_local: |
There was a problem hiding this comment.
This removes the explicit local-auth opt-in. Current main intentionally substitutes "local" unless this host block has apiKey, so a stored cloud/default key cannot break an unauthenticated loopback stack (827ce602d; current client.py:879-887). Please preserve that boundary and add an explicit shared-local-JWT mechanism if inheritance is required.
| except Exception as e: | ||
| logger.warning("Honcho dialectic query failed: %s", e) | ||
| return "" | ||
| return f"[honcho_error: {type(e).__name__}]" |
There was a problem hiding this comment.
This non-empty marker is treated as a successful dialectic result by automatic prefetch: it is stored, resets _dialectic_empty_streak, and is appended into injected context (plugins/memory/honcho/__init__.py:722-729, 854-861, 767-768). Classify failures separately or prevent markers from entering those paths.
|
Closing after re-verifying each part against current main (post #62290 / #66052) — this PR predates two rewrites of the config-resolution code it touches, and most of it has since been addressed by other routes:
Thanks @xxxigm — the #36098 report was accurate on all three fronts when filed; two got fixed by other salvages and the third is now tracked on its own. |
What does this PR do?
Fixes five Honcho self-hosted issues that together made multi-profile localhost setups fail silently (#36098):
hosts.hermes.<profile>now falls back tohosts.hermes.apiKey, then root, then env.config.api_key(including top-level) is passed to the SDK on localhost instead of being replaced with"local"unless empty.reasoning_level≥medium.[honcho_error: TimeoutError](etc.) instead of""(which looked like “no relevant context”).active_profile+~/.hermes/profiles/<name>/honcho.jsonis loaded (merged over default) whenHERMES_HOMEis unset.Related Issue
Fixes #36098
Type of Change
Changes Made
plugins/memory/honcho/client.py—resolve_api_key_from_raw,load_honcho_config_raw, profile config path,_is_localkey handling,_DEFAULT_HTTP_TIMEOUT = 60.plugins/memory/honcho/cli.py— CLI_resolve_api_keyuses shared resolver.plugins/memory/honcho/session.py—dialectic_queryerror marker.tests/honcho_plugin/test_client.py,tests/honcho_plugin/test_session.py— regression tests.How to Test
Manual:
honcho.jsonwithapiKeyonly underhosts.hermes,baseUrl: http://localhost:8000, and ahermes.<profile>host block withoutapiKey—hermes honcho status/ reasoning should authenticate instead of 401 + silent empty context.