Skip to content

fix(honcho): honor host-specific baseUrl - #14489

Open
LeonSGP43 wants to merge 1 commit into
NousResearch:mainfrom
LeonSGP43:fix/honcho-host-base-url-4269
Open

fix(honcho): honor host-specific baseUrl#14489
LeonSGP43 wants to merge 1 commit into
NousResearch:mainfrom
LeonSGP43:fix/honcho-host-base-url-4269

Conversation

@LeonSGP43

Copy link
Copy Markdown
Contributor

Summary

  • honor hosts.<name>.baseUrl when resolving Honcho client config
  • keep root and env fallback precedence unchanged for deployments without host overrides
  • add a regression test covering host-block override over root/env values

Testing

  • python3 -m pytest -o addopts='' tests/honcho_plugin/test_client.py -q

Issue

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/plugins Plugin system and bundled plugins area/config Config system, migrations, profiles labels Apr 23, 2026
@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jul 12, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused regression test. Current main deliberately treats baseUrl as a shared root-level connection setting.

Problems

  • plugins/memory/honcho/client.py:454-459 resolves baseUrl from the config root or environment only, and tests/honcho_plugin/test_client.py:256-265 explicitly asserts that hosts.hermes.baseUrl must not override it.
  • The setup flow writes cfg["baseUrl"] at plugins/memory/honcho/cli.py:558-573, while the self-hosted config example keeps it at the root in website/docs/user-guide/features/memory-providers.md:128-139.
  • The linked [Bug]: Honcho session link for locally hosted honcho incorrectly links to public honcho server #4269 report was about a hard-coded public session hyperlink; its current closure records that the UI link was removed rather than redirected.

Suggested changes

  • Preserve the root-level endpoint contract unless maintainers elect to support per-host servers. If that direction is chosen, extend setup, documentation, and compatibility coverage as one config-model change.

Automated hermes-sweeper review.

@teknium1 teknium1 added the area/memory Memory subsystem: store, providers, sync, background reviews label Jul 19, 2026
@WaterR47

Copy link
Copy Markdown

Drive-by data point from someone who hit this independently and landed on the identical patch before finding this PR. Not asking for a merge — just adding four facts the review above didn't have, because I think one of them undercuts its central premise.

The review's reasoning is that main deliberately treats baseUrl as a root-level connection setting. Reading current main, that's harder to sustain than it was in July:

1. timeout is the counter-example. config_schema.py marks timeout with scope="root" — and the reader honours host_block.get("timeout") first anyway, in the block immediately below base_url. So scope="root" governs where the settings UI writes a value, not what the reader will accept — which is exactly what the field's own docstring says: "Host-block placement: host (per-profile) or root; flat-json ignores it." baseUrl carries the same scope="root" as timeout; the difference is that its reader also refuses the host block. On that reading this isn't a deliberate contract, it's baseUrl being the one setting that didn't get the treatment the others did.

2. The test that asserted the contract is gone. The review cites test_base_url_not_read_from_host_block as main explicitly asserting root-only. That test is no longer in tests/honcho_plugin/test_client.py — the file has been restructured since. So the strongest evidence for deliberate intent no longer exists in the tree a rebase would target. (This also means this PR's test hunk won't apply as written and needs re-deriving; the client.py hunk is still exactly right.)

3. That test's own rationale has expired. Its docstring justified root-only as "consistent with apiKey" — but the current reader resolves host_block.get("apiKey") or raw.get("apiKey"). apiKey became host-scoped, so the stated consistency argument now points the other way.

4. The plugin README states the opposite rule. plugins/memory/honcho/README.md: "For every key, resolution order is: host block > root > env var > default." baseUrl is currently an undocumented exception to a rule the docs state without qualification.

On the review's substantive point — that changing this properly means touching setup and docs too — I think that's right, and worth separating from the reader change. The setup flow does write cfg["baseUrl"] at the root and the self-hosted docs example does keep it there. But making the reader accept a host-block value doesn't break either: root stays the fallback, nothing writes a host-block baseUrl unless a human does it deliberately, and existing configs resolve exactly as they do today. A regression test pinning that — root still wins when the host block omits baseUrl — makes it explicit, and is worth adding here regardless of which way the larger question goes.

The practical case, for whatever it's worth: one machine, several profiles, a self-hosted Honcho on the LAN for one of them and the default for the rest. Every other setting needed for that already honours the host block. baseUrl is the only one that has to be set globally, which means it can't be per-profile at all.

Happy to leave this as evidence on your PR rather than open a competing one — and if a maintainer does elect the per-host direction, I'm glad to do the setup + schema + docs half as a separate change so this one can stay small.

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 P2 Medium — degraded but workaround exists sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Honcho session link for locally hosted honcho incorrectly links to public honcho server

4 participants