Skip to content

fix(honcho): read base_url and defaultHost from honcho.json host blocks - #61675

Closed
Elektrofussel wants to merge 2 commits into
NousResearch:mainfrom
Elektrofussel:fix-honcho-client-config-resolution
Closed

fix(honcho): read base_url and defaultHost from honcho.json host blocks#61675
Elektrofussel wants to merge 2 commits into
NousResearch:mainfrom
Elektrofussel:fix-honcho-client-config-resolution

Conversation

@Elektrofussel

Copy link
Copy Markdown

Fixes Honcho client initialization for setup-generated configs that store connection details in a named host block (e.g. "local"). Previously base_url was only read from the flat config root and resolve_active_host() ignored defaultHost, causing the SDK to send an empty API key and fail to reach self-hosted Honcho instances.\n\nFixes #61661

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/plugins Plugin system and bundled plugins tool/memory Memory tool and memory providers area/config Config system, migrations, profiles labels Jul 9, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tracing the Honcho configuration path and linking #61661.

Problems

  • plugins/memory/honcho/client.py:72 returns defaultHost before the active-profile lookup. That makes every profile without HERMES_HONCHO_HOST use one configured host, conflicting with the current profile-derived host contract in plugins/memory/honcho/client.py:54-72 and plugins/memory/honcho/README.md:126.
  • The host-block baseUrl precedence at plugins/memory/honcho/client.py:468 reverses an explicit existing contract: tests/honcho_plugin/test_client.py:256-265 requires root baseUrl to win. Current setup also writes baseUrl at the root (plugins/memory/honcho/cli.py:568-573) and the local JWT under the host block (plugins/memory/honcho/cli.py:599-604).

Suggested changes

  • Reproduce the empty-key case with the setup-generated root-URL/host-key shape and fix that demonstrated path without changing profile host selection or URL precedence.
  • Add regression coverage for the actual reproducer and preserve the existing conflicting-root/host behavior.

Automated hermes-sweeper review.

Comment thread plugins/memory/honcho/client.py Outdated
try:
path = resolve_config_path()
if path.exists():
raw = json.loads(path.read_text(encoding="utf-8"))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

defaultHost is evaluated before the active-profile lookup, so a coder profile without HERMES_HONCHO_HOST would silently use this shared host. Current main derives host keys from the active profile; please preserve that isolation unless a new documented profile-routing design is intended.

Comment thread plugins/memory/honcho/client.py Outdated
@@ -452,7 +465,9 @@ def from_global_config(
)

base_url = (
raw.get("baseUrl")
host_block.get("baseUrl")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current main explicitly tests that baseUrl is root-level and must win over a host-block URL (tests/honcho_plugin/test_client.py:256-265); this precedence change would fail that contract. The setup wizard also writes its self-hosted URL at the root.

@Elektrofussel

Copy link
Copy Markdown
Author

Addressed the review feedback in eb12310b0.

  • Removed the global pre-profile defaultHost selection. defaultHost is now only considered after resolving the active Hermes profile, so named profiles keep their derived host keys.
  • Restored root-level baseUrl precedence; host-block baseUrl no longer wins over the root setting, preserving the existing test contract.
  • Fixed the empty-key local-IP path by treating loopback/LAN/VPN IP baseUrl values as local for SDK placeholder-key handling, while preserving explicit host-block local JWTs.
  • Added regression coverage for the setup-style root baseUrl + defaultHost + host-block shape, including LAN IP no-auth and explicit local auth cases.

Tested with:

python3 -m pytest tests/honcho_plugin/test_client.py -v
# 84 passed, 17 skipped

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 11, 2026
@Elektrofussel

Copy link
Copy Markdown
Author

Hi maintainers — just checking in on this PR. Is there a planned merge timeline, or is there an alternative solution being considered? We are running this patch locally and would love to get it into main so we can safely update Hermes without losing the fix. Happy to rebase or address any remaining feedback if needed.

@Elektrofussel

Copy link
Copy Markdown
Author

Hi maintainers,

this PR has been open for a while and is currently showing as mergeable but blocked. Is there anything still needed from our side, or is a merge planned in the near future? We would love to be able to update hermes-agent without breaking our local Honcho setup.

Thanks for your time!

Hermes Pi added 2 commits July 15, 2026 15:15
Fixes Honcho client initialization for setup-generated configs that store
connection details in a named host block (e.g. "local"). Previously:
- base_url was only read from flat config root, not from host_block.
- resolve_active_host() ignored defaultHost and always used the Hermes
  profile key ("hermes"), so the host block lookup returned {} and the
  api_key was also lost.

Fixes NousResearch#61661
@Elektrofussel
Elektrofussel force-pushed the fix-honcho-client-config-resolution branch from eb12310 to 1409bed Compare July 15, 2026 13:16
@Elektrofussel

Copy link
Copy Markdown
Author

Friendly bump after rebasing both PRs onto current origin/main.

Just rebased this branch and re-ran the test suite:

python3 -m pytest tests/honcho_plugin/test_client.py -v
# 11 passed
python3 -m pytest tests/honcho_plugin/test_honcho.py -v
# 23 passed

The branch is now conflict-free and mergeable. If there is anything else blocking review/merge, please let me know and I will address it promptly. Otherwise, would appreciate a review when you have a moment.

@teknium1

Copy link
Copy Markdown
Contributor

Merged via #66052 (rebase-merge) — both your commits landed on main. Note: they were authored under a local identity (pi@hermes.local), so GitHub can't link them to your profile, but the AUTHOR_MAP credits them to you. Thanks @Elektrofussel!

@teknium1 teknium1 closed this Jul 17, 2026
@teknium1 teknium1 added the area/memory Memory subsystem: store, providers, sync, background reviews label Jul 19, 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 area/memory Memory subsystem: store, providers, sync, background reviews comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state 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.

3 participants