Skip to content

fix(honcho): drop non-printable base_url values before client init (salvage of #2757 by @teyrebaz33) - #62757

Closed
Bartok9 wants to merge 2 commits into
NousResearch:mainfrom
Bartok9:salvage/2757-honcho-sanitize-url
Closed

fix(honcho): drop non-printable base_url values before client init (salvage of #2757 by @teyrebaz33)#62757
Bartok9 wants to merge 2 commits into
NousResearch:mainfrom
Bartok9:salvage/2757-honcho-sanitize-url

Conversation

@Bartok9

@Bartok9 Bartok9 commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Summary

Salvages the core URL-safety portion of #2757 by @teyrebaz33 onto current main.

What the original PR fixed

A terminal escape sequence accidentally pasted into honcho.base_url / HONCHO_BASE_URL caused Honcho SDK construction to raise Invalid non-printable ASCII character and poisoned startup.

Why it needed salvage

  • Original PR is dirty against current main
  • Honcho integration moved under plugins/memory/honcho/
  • Original also added a full honcho disable CLI surface; this salvage keeps the high-value fail-open sanitize only (small, mergeable)

Changes from original

  • Ported _sanitize_url into plugins/memory/honcho/client.py
  • Applied on env, config, and YAML override paths
  • Unit tests for clean / dirty config + env URLs

Testing

python3 -m pytest tests/test_honcho_client_config.py::TestHonchoBaseUrlSanitize -q

Full credit to @teyrebaz33 for root cause and original patch.

@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 preserving the focused URL-safety fix and crediting the original report. The current main path is still vulnerable: plugins/memory/honcho/client.py:391, :454-459, and :850 accept base URLs without printable-character validation before Honcho(**kwargs) receives the resolved value at :913.

Problems

  • plugins/memory/honcho/client.py:870 in this PR calls sanitize_url(...), but the helper added by the PR is named _sanitize_url(...). The surrounding config-load block catches Exception (plugins/memory/honcho/client.py:843-857 on current main), so the resulting NameError silently drops the entire config.yaml override rather than applying the intended fail-open behavior.
  • The added tests in tests/test_honcho_client_config.py:128-156 cover JSON config and environment resolution, but not the changed config.yaml override path.

Suggested changes

  • Use _sanitize_url(...) at the config.yaml override call site.
  • Add a test that reaches the get_honcho_client() config.yaml override path with a control character, so this spelling and integration path are covered.

Automated hermes-sweeper review.

Comment thread plugins/memory/honcho/client.py Outdated
@@ -847,7 +867,7 @@ def _build() -> "Honcho":
honcho_cfg = hermes_cfg.get("honcho", {})
if isinstance(honcho_cfg, dict):
if not resolved_base_url:
resolved_base_url = honcho_cfg.get("base_url", "").strip() or None
resolved_base_url = sanitize_url(honcho_cfg.get("base_url", "").strip() or None)

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.

_sanitize_url is the helper introduced above; sanitize_url is undefined. Because the surrounding config-loading block catches Exception, this NameError is swallowed and the config.yaml override is silently ignored. Please call _sanitize_url(...) here.

@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 11, 2026
@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jul 11, 2026
@Bartok9

Bartok9 commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

Addressed review: `sanitize_url(...)` → `_sanitize_url(...)` at the config.yaml override path so NameError is not swallowed.

Verification: `pytest tests/test_honcho_client_config.py tests/test_honcho_client_concurrency.py` → 14 passed.

@Bartok9

Bartok9 commented Jul 17, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto current main (resolved tests conflict with new TestLatencyFlagResolution suite — both kept).

Review (keep_open): addressed earlier and re-verified post-rebase:

  • config.yaml override call site uses _sanitize_url(...) (no sanitize_url NameError swallow)
  • Env + JSON baseUrl non-printables still dropped
  • Added test_config_yaml_override_uses_sanitize_helper covering the override path

Local: pytest tests/test_honcho_client_config.py → 16 passed.

Ready for re-review. Not merging from author side.

@Bartok9
Bartok9 force-pushed the salvage/2757-honcho-sanitize-url branch from 2afcda5 to 22843e7 Compare July 17, 2026 17:08
@teknium1 teknium1 added the area/memory Memory subsystem: store, providers, sync, background reviews label Jul 19, 2026
…alvage of NousResearch#2757 by @teyrebaz33)

Rebuilt on latest main (Bartok9 hygiene 2026-08-01).
Original: NousResearch#62757
@Bartok9

Bartok9 commented Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

Rebuilt onto latest main via patch re-apply (force-push). Please re-run CI.

— Bartok9 public PR hygiene 2026-08-01

…tok9

Per-PR attribution so check-attribution passes on this branch (Teknium).
@Bartok9

Bartok9 commented Aug 16, 2026

Copy link
Copy Markdown
Contributor Author

Closing as superseded — the fix is already on main.

Evidence (traced, not assumed):

  • origin/main plugins/memory/honcho/client.py already defines _sanitize_url() and wires it at from_env, from_global_config, and the _build config.yaml override path.
  • Landed as 41d77caf11 (fix(honcho): drop non-printable base_url values before client init, 2026-08-13) — same helper + call-site shape as this salvage of fix(honcho): graceful degradation for bad URL and add disable command #2757.
  • This PR is CONFLICTING/DIRTY against current main only because the change is already present; keeping it open adds noise.

Original upstream intent still tracked via #2757 if that PR has remaining scope beyond base_url sanitization.

@Bartok9

Bartok9 commented Aug 16, 2026

Copy link
Copy Markdown
Contributor Author

Superseded by main commit 41d77ca (same _sanitize_url fix already on origin/main).

@Bartok9 Bartok9 closed this Aug 16, 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-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 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