Skip to content

fix(agent): escape dangling backslash before a control char in tool-call JSON repair - #55620

Closed
briandevans wants to merge 1 commit into
NousResearch:mainfrom
briandevans:fix/agent-escape-dangling-backslash-control-char
Closed

fix(agent): escape dangling backslash before a control char in tool-call JSON repair#55620
briandevans wants to merge 1 commit into
NousResearch:mainfrom
briandevans:fix/agent-escape-dangling-backslash-control-char

Conversation

@briandevans

Copy link
Copy Markdown
Contributor

What does this PR do?

_escape_invalid_chars_in_json_strings (the control-char repair pass for malformed local-model tool-call JSON, in agent/message_sanitization.py) treats a backslash immediately followed by a literal control char (e.g. a real newline or tab) as a valid escape sequence and passes both bytes through unescaped. The control char then never reaches the ord(ch) < 0x20 escape branch, so the repaired string stays invalid JSON, _repair_tool_call_arguments exhausts its repair passes, and returns "{}" — silently discarding all tool-call arguments.

A model on a local/quirky backend (llama.cpp, GLM via Ollama — exactly what this repair pipeline exists for) that emits a backslash right before a line break therefore loses every argument, and the tool runs with none.

The fix: when a backslash precedes a literal control char, escape the backslash (\\) and let the existing control-char branch escape the following char on the next iteration. The repaired JSON is then valid and lossless — both the backslash and the control char are preserved.

Related Issue

N/A

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • agent/message_sanitization.py — in _escape_invalid_chars_in_json_strings, detect a backslash immediately before a literal control char (ord < 0x20); escape the backslash and advance by one so the control char is escaped by the existing branch on the next iteration.
  • tests/run_agent/test_repair_tool_call_arguments.py — add test_backslash_before_literal_control_char covering the lossless round-trip.

How to Test

Reproduction (before the fix):

from agent.message_sanitization import _repair_tool_call_arguments
_repair_tool_call_arguments('{"path": "C:\<backslash><newline>tmp"}', "write_file")
# -> "{}"   (all arguments silently dropped)

After the fix it repairs to valid JSON that parses (under strict=True) back to C: + backslash + newline + tmp.

  1. uv run --with pytest --with pytest-asyncio python3 -m pytest tests/run_agent/test_repair_tool_call_arguments.py -q
  2. The new test_backslash_before_literal_control_char fails on main (raises KeyError: 'path', args lost) and passes with this change.
  3. All 22 tests in the file pass; no existing control-char test regresses.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 15

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

…all JSON repair

_escape_invalid_chars_in_json_strings treats a backslash immediately
followed by a literal control char (e.g. a real newline or tab) as a
valid escape sequence and passes both bytes through unescaped. The
control char then never reaches the ord(ch) < 0x20 escape branch, so the
repaired string stays invalid JSON, _repair_tool_call_arguments exhausts
its passes and returns "{}" — silently discarding all tool-call
arguments.

When a backslash precedes a literal control char, escape the backslash
itself and let the existing control-char branch escape the following
char on the next iteration. The repaired JSON is then valid and lossless
(both the backslash and the control char are preserved).
Copilot AI review requested due to automatic review settings June 30, 2026 11:33

Copilot AI 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.

Pull request overview

This PR fixes a subtle but high-impact edge case in the tool-call argument JSON repair pipeline: a backslash immediately followed by a literal control character (e.g., an actual newline) inside a JSON string was previously treated as an “already-escaped” sequence, allowing the control character to remain unescaped and causing the repair pipeline to ultimately fall back to "{}" (dropping all tool-call args).

Changes:

  • Update _escape_invalid_chars_in_json_strings to treat \ + literal control char as not a valid JSON escape: it now escapes the backslash and lets the existing control-char branch escape the following character on the next iteration.
  • Add a regression test ensuring a backslash-before-literal-newline round-trips losslessly through _repair_tool_call_arguments.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
agent/message_sanitization.py Makes control-char escaping lossless when a dangling backslash precedes a literal control character inside a JSON string.
tests/run_agent/test_repair_tool_call_arguments.py Adds a regression test proving repaired arguments remain strict-JSON-parseable and preserve the original backslash + control char.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@alt-glitch alt-glitch added type/bug Something isn't working comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P2 Medium — degraded but workaround exists labels Jun 30, 2026

@tonydwb tonydwb 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.

Code Review Summary

Verdict: LGTM

Targeted fix for JSON repair: a backslash immediately before a literal control character (e.g., newline) was incorrectly treated as a valid JSON escape sequence, causing the repair to silently drop all tool-call arguments.

Looks Good

  • The fix is surgical: detect backslash + literal control char, escape the backslash, and let the existing control-char branch handle the following character on the next iteration
  • Lossless: both the backslash and the control char are preserved in the output
  • Regression test clearly demonstrates the issue and verifies the fix works under strict JSON parsing
  • Prior Copilot review confirmed no issues

Reviewed by Hermes Agent

swissly added a commit to swissly/hermes-agent that referenced this pull request Jul 11, 2026
Structured stats collection for the existing tool-call repair pipeline.
Records RepairEvent (pattern, tool, model, timestamp) at each repair
pass in message_sanitization.py and model_tools.py coerce_tool_args.

New module: agent/tool_repair_stats.py
- RepairPattern enum (20 known failure patterns)
- ToolRepairStats singleton: thread-safe, ring-buffer (10k events)
- record_repair() convenience function
- summary() for CLI display

Hooks added (1-2 lines each, zero-overhead when unused):
- message_sanitization.py: 6 hooks in _repair_tool_call_arguments
  (empty_args, none_literal, control_char_escape, trailing_comma,
   unrepairable)
- model_tools.py: 2 hooks in coerce_tool_args
  (bare_string_wrap, bare_object_wrap)

Design constraints:
- No new model tools (zero API cost impact)
- No prompt caching impact
- No new config keys
- Import failure → no-op (never breaks repair pipeline)
- Thread-safe with threading.Lock
- Bounded memory (ring buffer caps at 10k events)

Tests: 19 new tests (stats, thread-safety, ring-buffer, resilience)
Regression: 82 existing repair/coercion tests still pass

Complementary to existing repair PRs (NousResearch#62578, NousResearch#56399, NousResearch#61550, NousResearch#59267,
NousResearch#52747, NousResearch#55620, NousResearch#56557, NousResearch#21696) — adds observability, not repairs.
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused regression fix. Current main still unconditionally treats \\ followed by any next character as already escaped in agent/message_sanitization.py:164-169; a literal C0 character therefore skips the control-character escape at agent/message_sanitization.py:173-174. The proposed branch preserves the backslash and lets the existing branch escape the control character on the next iteration.

The same helper is used by API-message normalization (agent/conversation_loop.py:946-949) and streaming tool-call assembly (agent/chat_completion_helpers.py:2477-2494), so the helper-level regression test covers both active call paths. No substantive issues found.

Automated hermes-sweeper review.

@briandevans

Copy link
Copy Markdown
Contributor Author

Closing to keep my open queue focused and reviewable. This has sat ~2 weeks without any human review interest, and I would rather maintain a small set of actively-reviewed PRs than a large standing backlog. The analysis and branch remain available if anyone wants to pick this up — happy to reopen if it is useful.

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

Labels

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P2 Medium — degraded but workaround exists 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 type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants