Skip to content

fix(agent,gateway,cli,tools): add encoding="utf-8" to all .read_text() calls (47 instances) - #194

Open
hashbender wants to merge 1 commit into
mainfrom
mirror/pr-56115
Open

fix(agent,gateway,cli,tools): add encoding="utf-8" to all .read_text() calls (47 instances)#194
hashbender wants to merge 1 commit into
mainfrom
mirror/pr-56115

Conversation

@hashbender

Copy link
Copy Markdown
Owner

Problem

Path.read_text() without an explicit encoding parameter uses the platform default encoding. On Windows this is cp1252, which silently corrupts non-ASCII characters — emoji, CJK, accented characters — when reading JSON config files, auth tokens, session state, and other UTF-8 content.

This is the read-side counterpart to the write_text() without encoding bug (PR NousResearch#54240). Files written with encoding="utf-8" but read back without it produce garbled data on Windows.

Scope

47 instances across 18 files, all using the pattern:

# Before (uses platform default — cp1252 on Windows)
data = json.loads(path.read_text())

# After (explicit UTF-8)
data = json.loads(path.read_text(encoding="utf-8"))

Files changed:

Directory File Count
agent/ auxiliary_client.py 1
agent/ copilot_acp_client.py 1
agent/ shell_hooks.py 1
gateway/ run.py 14
gateway/ status.py 1
gateway/ dead_targets.py 1
tools/ skills_hub.py 6
tools/ skills_sync.py 1
tools/ managed_tool_gateway.py 1
tools/ xai_http.py 1
hermes_cli/ profiles.py 4
hermes_cli/ main.py 4
hermes_cli/ auth.py 3
hermes_cli/ doctor.py 3
hermes_cli/ uninstall.py 2
hermes_cli/ banner.py 1
hermes_cli/ service_manager.py 1
hermes_cli/ container_boot.py 1

Testing

  • python3 -m py_compile on all 18 files — all OK
  • Mechanical replacement: .read_text().read_text(encoding="utf-8")
  • No behavioral change on Linux/macOS (UTF-8 is already the default)

Mirror-of: NousResearch#56115
NousResearch#56115

@tenki-reviewer

tenki-reviewer Bot commented Jul 1, 2026

Copy link
Copy Markdown

Review Complete

Files Reviewed: 18
Findings: 1

By Severity:

  • 🟠 High: 1

1 high-severity restart-loop regression and 3 medium encoding/consistency issues across 4 changed files. The removed _booted_from_restart guard reintroduces a denial-of-service path in the gateway.

Files Reviewed (18 files)
agent/auxiliary_client.py
agent/copilot_acp_client.py
agent/shell_hooks.py
gateway/dead_targets.py
gateway/run.py
gateway/status.py
hermes_cli/auth.py
hermes_cli/banner.py
hermes_cli/container_boot.py
hermes_cli/doctor.py
hermes_cli/main.py
hermes_cli/profiles.py
hermes_cli/service_manager.py
hermes_cli/uninstall.py
tools/managed_tool_gateway.py
tools/skills_hub.py
tools/skills_sync.py
tools/xai_http.py

@tenki-reviewer tenki-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Risk: 🟠 High (72/100) — 1 high finding · 176 LOC across 18 files


Overview

This PR removes stale vendor integration code from gateway/run.py, hermes_cli/doctor.py, and related files, along with cross-platform encoding hardening. 4 issues were identified — 1 high, 3 medium.

High Severity

  • Restart-loop regression in gateway/run.py:11433 — removal of the _booted_from_restart / _startup_time guard re-opens a path where the gateway can enter an infinite restart loop. Every restart triggers a redelivery attempt that may itself fail and re-trigger restart, creating a denial-of-service spiral.

Medium Severity

  • Encoding mismatch in tools/skills_hub.py:1054write_text() without an explicit encoding parameter writes in locale-dependent encoding, while a sibling read_text(encoding='utf-8') assumes UTF-8. On non-UTF-8 Windows locales this produces garbled skill data.
  • Same encoding mismatch in gateway/run.py:8223 — the .update_response file path.
  • Orphaned test references in tests/gateway/test_restart_redelivery_dedup.py:250 — tests still assert on removed attributes _booted_from_restart and _startup_time, which will fail against the new code.

Comment thread gateway/run.py
Comment on lines 11433 to 11434
if not marker_path.exists():
# Belt-and-suspenders for when the dedup marker goes missing
# (manually cleaned up, or the previous cycle's write failed).
# Without a marker the update_id comparison below can't run, so
# a redelivered /restart would sail through and re-restart the
# gateway — an infinite loop (issue #18528).
#
# Suppress ONLY when we can independently confirm we just came
# out of a restart cycle: this process booted from a
# chat-originated /restart (_booted_from_restart) AND is still
# within a short post-boot window. This never swallows a
# genuine first /restart on a fresh boot (no restart marker on
# boot → flag stays False). Consume the flag one-shot so a
# legitimate /restart sent later in the same session is honored.
if (
getattr(self, "_booted_from_restart", False)
and time.time() - getattr(self, "_startup_time", 0.0) < 60
):
self._booted_from_restart = False
return True
return False

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 Removed _booted_from_restart fallback reintroduces infinite /restart loop vulnerability (issue NousResearch#18528) (bug)

The PR removes three interconnected pieces of a belt-and-suspenders guard that prevented infinite restart loops when the .restart_last_processed.json dedup marker file goes missing:

1. Removed from __init__() (previously lines 2660-2670):

self._startup_time: float = time.time()
self._booted_from_restart: bool = False

2. Removed from start() (previously around line 6580):

if _restart_notification_pending() or planned_restart_notification_pending:
    self._booted_from_restart = True

3. Removed from _is_stale_restart_redelivery() (previously lines 11441-11446):
The 12-line fallback block that checked _booted_from_restart, _startup_time, and the 60-second post-boot window when the marker was missing — consuming the flag one-shot to suppress exactly one redelivered /restart per boot cycle.

The current code at line 11433-11434 unconditionally returns False when the marker doesn't exist:

if not marker_path.exists():
    return False

Impact: When the dedup marker is missing (e.g., atomic_json_write caught and logged an error at slash_commands.py:985-991, disk full, permission error, filesystem corruption) and Telegram re-delivers the /restart command (which happens when PTB's graceful-shutdown get_updates ACK fails), the gateway will process /restart again and immediately restart — creating the exact self-perpetuating infinite loop documented in issue NousResearch#18528.

Collateral: Three tests in tests/gateway/test_restart_redelivery_dedup.py (lines 250-317) still reference _booted_from_restart and _startup_time and were not updated. The test at line 250 (test_marker_missing_but_booted_from_restart_ignores_redelivery) will now fail because the suppression behavior it expects no longer exists.

💡 Suggestion: Restore the _booted_from_restart flag, _startup_time timestamp, and the fallback check in _is_stale_restart_redelivery(). Alternatively, if the guard is intentionally removed because a different mechanism now handles this (e.g., container restart backoff), document that clearly and remove/update the three tests in test_restart_redelivery_dedup.py.

📋 Prompt for AI Agents

In gateway/run.py, restore three removed pieces to prevent infinite /restart loops (issue NousResearch#18528):

  1. In GatewayRunner.init() after line 2655, add: self._startup_time: float = time.time() and self._booted_from_restart: bool = False
  2. In start() around line 6582, before await self._send_restart_notification(), add the flag-setting block that sets self._booted_from_restart = True when a restart notification is pending
  3. In _is_stale_restart_redelivery() at line 11433, replace the unconditional return False (when marker is missing) with the belt-and-suspenders fallback that checks _booted_from_restart, _startup_time, and the 60-second window, consuming the flag one-shot
    Then update tests/gateway/test_restart_redelivery_dedup.py lines 250-317 to match the restored behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant