The cherry-pick auto-resolution + AST sweep applied plain utf-8 at three
.env reader sites where the salvaged PRs (#62617, #62123) deliberately
use utf-8-sig — a Notepad BOM must not hide/duplicate the first key.
Restore the contract (tests pin it).
AST-driven pass over every Path.read_text()/write_text() without an
explicit encoding= across non-test code: 71 sites in 34 files
(skills_hub, hermes_cli/main+profiles+service_manager+container_boot,
mem0/hindsight/honcho plugins, achievements dashboard, release/CI
scripts, productivity+comfyui skill helpers, agent/*). Verified zero
positional-encoding collisions before insertion; per-file compile()
check after.
Adds a check-windows-footguns rule flagging bare single-line
read_text/write_text (multi-line forms stay covered by the AST guard
test from #38985). Together with the salvaged contributor commits this
retires the ~169-site bare file-I/O class (#37423's long tail).
Follow-up to review feedback:
- mem0 _prompt_api_key read .env with the locale default, so a Notepad
BOM hid the first key from the masked current-value lookup; read it
with utf-8-sig + errors=replace like the canonical readers in
hermes_cli/config.py.
- hindsight _load_simple_env used plain utf-8; it also parses the Hermes
.env during post_setup, where a BOM stuck to the first key. Switch to
utf-8-sig + errors=replace.
- Add hindsight regressions: BOM key matching in _load_simple_env and in
the cloud post_setup writer, plus non-ASCII round-trip preservation,
and a mem0 regression for the BOM'd masked-key lookup. The BOM tests
fail without the fix on any platform.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The mem0 and hindsight memory-provider setup routines round-trip the
user's ~/.hermes/.env: they read existing lines, update the keys they
manage, and rewrite the whole file preserving every other line verbatim.
Both used env_path.read_text() / write_text() with no encoding.
read_text()/write_text() with no encoding fall back to the system locale
(cp1252/GBK on Windows), so on a non-UTF-8 host the preserved lines get
mangled or the call crashes on any non-ASCII value, and — because the
reader never strips a BOM — a Notepad-edited .env makes the first key
fail the in-place match and get duplicated instead of updated.
Match the canonical .env readers in hermes_cli/config.py: read with
encoding='utf-8-sig' (BOM-tolerant) and write with encoding='utf-8'.
mem0/_setup.py already pins utf-8 for mem0.json, so this just aligns the
.env path in the same file. Fixes both memory plugins in one class fix.
Adds regression tests: a BOM'd .env updates the first key in place
(locale-independent, fails without the fix) and non-ASCII existing lines
survive the round-trip.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
On Windows with CJK locales (e.g. Chinese/GBK), pathlib.Path.read_text()
defaults to the system encoding instead of UTF-8, causing UnicodeDecodeError
when reading .env or .json config files that contain non-ASCII characters.
Explicitly pass encoding='utf-8' to all read_text() and write_text() calls
in the hindsight memory provider plugin.
Path.read_text() without an explicit encoding uses the platform's
default encoding. On Windows this is typically cp1252 or mbcs, which
causes UnicodeDecodeError or silent data corruption when reading
UTF-8 content (JSON files, user text, config with non-ASCII chars).
This is the read-side companion to the write_text() encoding fix.
Fixed the most critical locations that read JSON data, user content,
and config files across 14 files with 31 call sites.
Pattern: .read_text() → .read_text(encoding='utf-8')
json.loads(path.read_text()) → json.loads(path.read_text(encoding='utf-8'))
On Windows with Chinese locale (GBK), subprocess.run(text=True) without
explicit encoding causes UnicodeDecodeError crashes. This fix adds
encoding='utf-8', errors='replace' to all subprocess.run() and
subprocess.Popen() calls that use text=True across 76 non-test Python files.
Fixes#53428 (master tracker for Windows GBK locale crash).
Note: credential_pool.py and electron changes excluded per reviewer request —
those will be submitted as separate focused PRs.
RFC 8628 §3.2 makes the device-authorization `interval` optional with a
client-side default of 5 seconds. request_device_code required it, so a
compliant AS that omitted it hit the malformed-response path and the flow
could never complete. Fall back to 5s and cover it with a regression test.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adds a device authorization grant flow alongside the existing loopback
OAuth flow, so `hermes setup` can connect to Honcho cloud from SSH and
other no-browser environments.
- oauth.py: new HTTP seams — _http_post_form_status (non-raising, since
RFC 8628 polling reads the OAuth error off a 400) and _http_get_json
for the RFC 8414 metadata probe
- oauth_flow.py: DeviceCode, request_device_code, poll_for_token with
slow_down backoff (+5s, capped at 60s) bounded by expires_in, typed
errors (AccessDenied, DeviceCodeExpired, AuthorizationTimeout), and
supports_device_login (fail-closed metadata gate); device flow ends in
the same install_grant tail as loopback so refresh/status work
unchanged
- oauth_flow.py: loopback callback now serves a "sign-in was not
completed" page on consent cancel instead of the success page
- cli.py: cloud menu offers oauth / device / apikey; the device option
only appears when the host advertises the grant, and becomes the
default when no browser is detected
- 18 new tests covering the full flow against a local fake AS, backoff
schedule, error mapping, deadline bound, metadata gate, and wizard
branches
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Follow-up hardening on the salvaged _ensure_client() (#21130 fix):
- Failed-config cooldown: after a refresh attempt fails for a given
resolved config, skip re-probing for 30s. Previously every provider
access against a down endpoint paid a 3s health probe under
_client_refresh_lock and emitted a warning (2+ per turn, some on
user-facing threads: prefetch, tool calls, session end). Retries
still happen after the cooldown or immediately when config changes,
and the log message now says so instead of the false 'disabled until
config changes'.
- Atomic connection snapshot: _conn_snapshot (5-tuple, single
assignment) is published only after a health check passes.
_new_client() and on_memory_write's writer read it as one load, so
background writers can no longer observe a torn mix of old/new
identity fields mid-refresh or target an endpoint that never passed
health. Field writes in _ensure_client_locked keep tracking the
attempted config for the unchanged-config dedupe.
- _env_refresh_enabled moves to the top of initialize(): an exception
mid-initialize (swallowed by MemoryManager) can no longer leave the
provider silently stuck in never-refresh mode.
- _search_prefetch_context reuses _new_client() and degrades to ''
on construction failure instead of propagating.
Mutation-checked: neutering the cooldown or publishing the snapshot on
failed health makes the new regression tests fail.
Avoid spawning multiple local OpenViking server processes while a runtime autostart waiter is already active. Remote endpoints still retry on later accesses because they do not install a local waiter.
Route refreshed unreachable local OpenViking configs through the existing runtime recovery path so /reload can attach to a locally starting server instead of disabling memory until restart.
(cherry picked from commit 040e18ad90)
The _needs_trusted_identity_retry method was hard-coding specific
server-side error strings to detect when a request failed due to
missing X-OpenViking-Account / X-OpenViking-User headers. Each new
server-side error variant required another string added to the client.
Replace the string enumeration with a structural match: the error
message mentions one of the tenant headers AND the HTTP status is 400.
This covers all current error variants:
- "Trusted mode requests must include X-OpenViking-Account and User"
- "ROOT requests to tenant-scoped APIs must include X-OpenViking-Account"
- "Trusted mode requests must include X-OpenViking-Account."
- "Trusted mode requests must include X-OpenViking-User."
The 400 status guard avoids false-positives on 403 errors such as
"USER API keys cannot override X-OpenViking-User", which must not
trigger a retry.
All 176 existing tests pass.
(cherry picked from commit 5a24d6766c)
`OpenVikingMemoryProvider.shutdown()` joins in-flight writers, deferred-commit
threads, and prefetch threads, but not `_runtime_start_thread` — the tracked
`daemon=True` waiter that runs `_finish_runtime_openviking_start`, which blocks
on network health probes (`_wait_for_openviking_health` polling + a
`_VikingClient.health()` request).
If the local OpenViking runtime is slow or unreachable, that waiter can still
be blocked in network I/O at interpreter exit. CPython then forcibly kills it
during `Py_FinalizeEx` (`PyThread_exit_thread` -> `__pthread_unwind` ->
`abort()`), producing SIGABRT (exit 134) with no traceback — the same daemon-
thread-at-exit failure class fixed for the Honcho provider.
Fix:
- `shutdown()` now joins `_runtime_start_thread` (timeout-bounded) alongside the
other tracked threads.
- `_wait_for_openviking_health()` gains a `should_stop` callback; the waiter
passes `lambda: self._shutting_down` so the poll loop bails out promptly once
`shutdown()` flips the flag, instead of lingering up to the 60s autostart
timeout and timing out the join (which would leave the thread alive).
- Add tests/plugins/memory/test_openviking_shutdown.py covering the short-circuit
and the shutdown-joins-runtime-thread behaviour.
(cherry picked from commit 5471ec7021)
initialize() snapshots OPENVIKING_* into the provider once, so /reload
(which only updates os.environ) leaves viking_* tools running against
stale auth — users have to restart hermes to pick up keys added to
~/.hermes/.env after startup.
Add _ensure_client(), which re-resolves the connection settings via the
same _resolve_connection_settings/_load_hermes_openviking_config path
initialize() uses and rebuilds + health-checks the client only when an
OPENVIKING_* value actually changed; otherwise it reuses the cached
client so the hot path stays at one dict comparison with no network
calls. Every `if not self._client:` guard in system_prompt_block,
queue_prefetch, sync_turn, on_session_end, on_memory_write and
handle_tool_call now goes through it.
Refreshing is gated behind a flag set at the end of initialize() so the
baseline is established before any env re-resolution happens — callers
that wire up a client directly keep the existing client untouched.
Refs #21130
(cherry picked from commit b694d21b7c)
`_write_env_vars` in the OpenViking memory provider interpolates each
secret straight into a `KEY=VALUE` line, but the values only ever pass
through `_clean_config_value`, whose `value.strip()` trims surrounding
whitespace and leaves internal CR/LF intact. Because the file is strictly
line-oriented and is re-read via `read_text().splitlines()`, a value that
carries an embedded newline spills onto a second physical line, and the
tail is re-parsed as an independent `KEY=VALUE` entry on the next round
trip. A secret pasted with a trailing record (e.g. an `OPENVIKING_API_KEY`
copied with an extra line) therefore injects an arbitrary additional
variable into the persisted credentials file and silently corrupts it.
The fix neutralizes the line terminators at the single chokepoint where
values reach the file. A small `_env_line_safe` helper strips `\r`, `\n`,
and the NUL byte from each value, and both write sites in `_write_env_vars`
(the existing-key update branch and the appended-key branch) route through
it, so a value can only ever occupy the single line it is written on.
## What does this PR do?
Hardens the OpenViking memory provider's `.env` writer so a malformed or
pasted secret value can no longer break out of its `KEY=VALUE` line and
inject a rogue variable into the profile-scoped credentials file.
## Related Issue
N/A
## Type of Change
- [x] 🐛 Bug fix (non-breaking change that fixes an issue)
## Changes Made
- `plugins/memory/openviking/__init__.py`: add `_env_line_safe()` which
removes `\r`, `\n`, and `\x00` from a value, and apply it to both the
updated-key and appended-key write branches in `_write_env_vars()`.
- `tests/plugins/memory/test_openviking_provider.py`: add two regression
tests covering a fresh write and an in-place key update with embedded
CR/LF, asserting no injected line survives the read-back.
## How to Test
1. Run the targeted tests:
`pytest tests/plugins/memory/test_openviking_provider.py -k env_writer -q`
2. Reverting the `_env_line_safe` sanitization makes
`test_openviking_env_writer_strips_embedded_newlines_in_values` and
`test_openviking_env_writer_strips_newlines_when_updating_existing_key`
fail with a rogue `INJECTED_KEY=`/`ROGUE=1` line appearing in the file,
confirming the tests pin the bug.
3. `ruff check plugins/memory/openviking/__init__.py` and
`python scripts/check-windows-footguns.py plugins/memory/openviking/__init__.py`
both pass.
## Checklist
### Code
- [x] I've read the Contributing Guide
- [x] My commit messages follow Conventional Commits
- [x] I searched for existing PRs to make sure this isn't a duplicate
- [x] My PR contains only changes related to this fix
- [x] I've run the relevant tests and they pass
- [x] I've added tests for my changes (required for bug fixes)
- [x] I've tested on my platform: macOS 15 (Darwin 25.5)
### Documentation & Housekeeping
- [x] I've updated relevant documentation (docstrings) — or N/A
- [x] I've updated `cli-config.yaml.example` if I added/changed config keys — N/A
- [x] I've updated `CONTRIBUTING.md` or `AGENTS.md` if I changed architecture or workflows — N/A
- [x] I've considered cross-platform impact (strips CR as well as LF) — done
- [x] I've updated tool descriptions/schemas if I changed tool behavior — N/A
(cherry picked from commit f29dd2df84)
teknium1 review on #57690: harvesting logic was skipping the ENTIRE merged
row when a compaction summary was appended to the tail message, discarding
real prior user content that context_compressor retains before the
_MERGED_SUMMARY_DELIMITER. Extract and harvest that pre-delimiter segment
instead of dropping it wholesale.
Revert-to-fail: reverting plugins/memory/holographic/__init__.py alone
drops test_merged_into_tail_preserves_genuine_pre_delimiter_preference
(19 passed, 1 failed); restoring the fix returns 20/20 passed.
Two compounding defects in the holographic memory provider (#57682):
1. The on_session_end gate used plain truthiness on auto_extract, but the
plugin's own config schema declares it as a string enum with default
"false" — and not "false" is False, so extraction ran for users who
had it configured off. Coerce with the shared utils.is_truthy_value
(same fix class as the merged byterover no-op fix).
2. _auto_extract_facts scanned every role=user message. Context-compaction
handoff summaries can be inserted as role=user messages and their prose
reliably matches the decision patterns (we decided/agreed, the project
uses), so the compactor's own output was persisted as a durable project
fact on every rollover following a compaction — recreated even after
manual deletion.
Adds agent.context_compressor.is_compaction_summary_message(), a public
helper that prefers the in-process COMPRESSED_SUMMARY_METADATA_KEY marker
and falls back to _is_context_summary_content() (covers merged-into-tail
and historical prefixes), since the metadata key is stripped by wire
sanitizers and doesn't survive all session-store round-trips. The plugin
skips summary messages before pattern matching.
Fixes#57682
The _profile_prefetched_sessions set stores whichever session_id was
passed to prefetch(), which may differ from self._session_id. On
compression, only old_session_id (self._session_id) was discarded,
missing the case where the stored key was the prefetch session_id
parameter. Discard both old and new IDs to cover all cases.
- Remove dead current_sid parameter from _recover_pending_sessions
- Remove dead cleanup parameter from _release_owner_run_claim (always True)
- Set _run_lock_path after flock succeeds, not before
- Collapse redundant BlockingIOError branch (covered by OSError+errno check)
- Track _pending_marked_sids to skip re-writing marker file on every sync_turn
Preserve ordered structured turns across OpenViking's 100-message batch limit and resume retries from the first unconfirmed message.
Based on the OpenViking batching work from commit 1a567f7067 in #58981.
The supermemory SDK already honors SUPERMEMORY_BASE_URL, but the raw
urllib call used for session-end conversation ingest hardcoded
https://api.supermemory.ai/v4/conversations, so ingest always hit the
cloud even when pointing at a self-hosted server (e.g.
http://localhost:6767).
Resolve the base URL as config (supermemory.json base_url) >
SUPERMEMORY_BASE_URL env var > https://api.supermemory.ai, strip any
trailing slash, and use it for both the SDK client and the
/v4/conversations ingest endpoint.
The staleness check's bespoke mtime memo keyed only on the user
config.yaml, but load_config() merges the managed-scope config
(HERMES_MANAGED_DIR/config.yaml, /etc/hermes) whose leaf keys win. A
managed honcho.timeout with no user config.yaml made the memo cache
'no timeout' while _build resolved the managed value — the same
perpetual-rebuild mismatch this PR fixes for honcho.json. A managed
timeout edit was likewise invisible while the user file's mtime stayed
put.
load_config_readonly() is already cached on both files' signatures plus
the env-ref snapshot, so use it instead of duplicating that
invalidation logic; the defensive deepcopy the old memo existed to
avoid is skipped by the readonly variant. Drive the rebuild test
through a real config.yaml and add a HERMES_MANAGED_DIR regression
test covering stable reuse and managed-timeout edits.
The staleness check added in #66052 resolved the timeout from env,
config.yaml, and the default only, while the build path also reads the
honcho.json host block (timeout/requestTimeout). With a timeout
configured in honcho.json, the two permanently disagreed: every
no-config get_honcho_client() call — i.e. every HonchoSessionManager
.honcho property access — interpreted the mismatch as a config change
and tore down and rebuilt the client, defeating the singleton on the
hot path it was meant to protect.
Teach the check to read honcho.json through the same host-aware chain
as from_global_config, memoized on the file's mtime_ns so the per-call
cost stays one stat(). A genuine honcho.json timeout change is now also
detected, extending #57437 to that config surface.
Follow-ups for the consolidated salvage:
- Memoize the config.yaml-derived timeout on the file's mtime_ns so the
rebuild-on-timeout-change check from PR #57437 costs one stat() per
get_honcho_client() call instead of a full YAML load on the hot path.
- hermes honcho status now displays the host-block-resolved
dialecticCadence (remnant from PR #63776, whose runtime fix landed
in #62290).
- AUTHOR_MAP entries for the salvaged contributor emails.
The Honcho client singleton cached the HTTP timeout at first build.
In long-lived processes (gateway, dashboard), changing the timeout
via config.yaml or HONCHO_TIMEOUT had no effect until restart.
Track the resolved timeout alongside the cached client and compare
on each get_honcho_client() call. When the timeout differs, reset
the singleton so the next call rebuilds with the new value.
Fixes#57347
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.
FixesNousResearch/hermes-agent#61661
honcho_reasoning's minimal tier hard-caps Honcho's dialectic output at 250
tokens combined with the model's own hidden reasoning tokens. Confirmed via
direct honcho-api server logs that a multi-fact query ("summarize known
facts about this peer and communication preferences") run at
reasoning_level=minimal gets cut off mid chain-of-thought at exactly
output_tokens=250, before the model ever reaches a synthesized answer.
low/medium/high/max fall back to Honcho's much larger global dialectic
default and don't hit this cap. Since dialecticDynamic is on by default and
the calling model picks reasoning_level itself via this tool parameter, the
fix is to make the tradeoff explicit in the parameter description so the
model defaults to low unless the query is genuinely a single-fact lookup.
Schema-description-only change; the shared dialectic_max_chars truncation
cap in session.py's dialectic_query() is a separate fix (its own PR).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
_fetch_session_context was using user_peer_id directly as observer
when calling _fetch_peer_context, bypassing _resolve_observer_target.
This caused honcho_context to return empty data because the observer
perspective was wrong (user instead of hermes/assistant).
Fixed by resolving observer via _resolve_observer_target(session, 'user'),
consistent with all other call sites (get_peer_card, honcho_search, etc.).
queryRewrite (default off) gates the latest-message rewrite so the
extra auxiliary LLM call is opt-in. firstTurnBaseWait and
firstTurnDialecticWait expose the turn-1 bounded waits in seconds
(0 disables). All three resolve host-block-first like every other
field. Also pins per-host timeout resolution with tests.
Move query_rewrite from the honcho plugin to plugins/memory/ and
rename the auxiliary task key honcho_query_rewrite ->
memory_query_rewrite so any memory provider can use the same
rewrite path and model/timeout config block. No behavior change.
dialecticMaxChars (default 600) is documented as the budget for the dialectic
supplement auto-injected into the system prompt every turn — a small recurring
cost that is correct to bound tightly. But dialectic_query() applied that cap
unconditionally, so explicit honcho_reasoning tool calls — where the model
deliberately spends a turn asking for a synthesized answer — were silently
truncated mid-word to 600 chars with a trailing " …", no error surfaced. The
full answer is returned by Honcho server-side; the clip happens client-side.
The auto-injection path already has its own token-based budget (contextTokens,
enforced in prefetch() via _truncate_to_budget), so the char cap's real job is a
cheap always-on guardrail for that recurring injection. Explicit tool results
are already bounded server-side by Honcho's dialectic MAX_OUTPUT_TOKENS and don't
need the injection cap — sibling tools (honcho_search, honcho_context) don't
post-clip their results either.
Add apply_injection_cap (default True, preserving current behavior) to
dialectic_query(); the honcho_reasoning tool handler passes False so it returns
Honcho's full synthesized answer. Auto-injection is unchanged. Tests cover both
the capped injection path and the uncapped tool path.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
