fix(tools): dedup early-return bypasses loop detection in read_file (#15759) by briandevans · Pull Request #15762 · NousResearch/hermes-agent

briandevans · 2026-04-25T19:19:50Z

Summary

The read_file dedup path at tools/file_tools.py:435 returned immediately for unchanged-file hits without updating the consecutive re-read counter.
The hard-block guard (count >= 4) and the warning guard (count >= 3) at line 538 were unreachable for dedup hits.
Models that repeatedly call read_file on an unchanged file received an infinite stream of dedup stubs with no escalation.

The bug

_read_tracker[task_id]["consecutive"] is only incremented in the full-read path (line 504). The dedup path returned before that code ran:

read_file → content      (mtime cached, consecutive=1)
read_file → dedup stub   (returns early — consecutive stays at 1 forever)
read_file → dedup stub   ← infinite loop

Issue reported with Qwen3.6 local models that became stuck re-reading files mid-task after a hermes update.

The fix

Before returning the dedup stub, acquire _read_tracker_lock and update last_key / consecutive using the same logic as the full-read path. If count >= 4, return the BLOCKED error instead of the stub. If count >= 3, annotate the stub with _warning.

notify_other_tool_call() already zeroes consecutive, so interleaved tool calls continue to work correctly.

Test plan

Before: test_dedup_fourth_read_is_hard_blocked and test_dedup_third_read_returns_stub_with_warning fail (stub returned with no warning or block)
After: all 30 loop-detection tests pass including 5 new dedup-specific cases
Regression guard: reverted fix → observed dedup returning plain stub on 4th call; restored → 0 failures
CI baseline: 8 pre-existing failures (7 dingtalk + 1 matrix) on origin/main — unchanged

…ousResearch#15759) The dedup path at tools/file_tools.py:435 returned immediately on unchanged-file hits without updating the consecutive counter. The hard-block guard at line 538 (count >= 4) was therefore unreachable for models that repeatedly call read_file on a file that hasn't changed, producing an infinite loop of dedup stubs. Fix: before returning the dedup stub, acquire the tracker lock and increment consecutive exactly as the full-read path does. If the counter reaches the block threshold, return the BLOCKED error instead of the stub. If the counter reaches the warning threshold, annotate the stub with _warning to give the model a softer nudge first. notify_other_tool_call already resets consecutive=0, so interleaved tool calls continue to work correctly. Five new regression tests cover: dedup-hit warning, dedup-hit hard block, notify resets counter through the dedup path, and task isolation. Regression guard: removed fix → test_dedup_third_read_returns_stub_with_warning and test_dedup_fourth_read_is_hard_blocked fail; restored → 30/30 pass. CI baseline (8 pre-existing failures on origin/main) unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Fixes an infinite-loop scenario where read_file_tool would return a dedup stub for unchanged files without incrementing the consecutive re-read counter, preventing warning/block escalation.

Changes:

Update the dedup-return path in read_file_tool to increment last_key/consecutive and emit warning/block responses at the same thresholds as the full-read path.
Add dedup-specific loop-detection tests covering stub, warning, hard-block, counter reset, and task isolation cases.

Reviewed changes

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

File	Description
tools/file_tools.py	Ensures dedup hits participate in consecutive-loop detection and can trigger warning/block responses.
tests/tools/test_read_loop_detection.py	Adds regression tests validating warning/block behavior for consecutive dedup hits.

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

Copilot · 2026-04-25T19:23:16Z

+                    read_key = ("read", path, offset, limit)
+                    with _read_tracker_lock:
+                        if task_data["last_key"] == read_key:
+                            task_data["consecutive"] += 1
+                        else:
+                            task_data["last_key"] = read_key
+                            task_data["consecutive"] = 1


Loop detection can still be bypassed by calling read_file_tool with different string forms that resolve to the same file (e.g., ./foo.py vs foo.py). Dedup uses resolved_str for dedup_key, but read_key uses the raw path, so last_key changes and consecutive resets, preventing the >=3/4 warning/block from ever triggering. Consider building read_key from the resolved path (and updating the full-read path key similarly) so consecutive tracking matches the dedup identity.

Copilot · 2026-04-25T19:23:16Z

+    Without this fix, the dedup path returned early and the loop-detection guard
+    at line 538 was never reached — the agent could loop forever on a dedup stub.


This docstring hard-codes an internal line number ("at line 538"), which will become stale as tools/file_tools.py changes. Prefer referencing the guard semantically (e.g., "the loop-detection guard in read_file_tool") or linking to the relevant condition (e.g., count >= 4) instead of a specific line number.

Suggested change

Without this fix, the dedup path returned early and the loop-detection guard

at line 538 was never reached — the agent could loop forever on a dedup stub.

Without this fix, the dedup path returned early and bypassed the

loop-detection guard in read_file_tool, so the agent could loop forever

on a dedup stub.

Copilot · 2026-04-25T19:23:17Z

+    def test_dedup_second_read_returns_stub_no_warning(self, _mock_ops, _mock_mtime):
+        """2nd read (dedup hit) returns the stub but no warning yet."""
+        read_file_tool("/tmp/test.py", task_id="td1")
+        result = json.loads(read_file_tool("/tmp/test.py", task_id="td1"))
+        assert result.get("dedup") is True
+        assert "_warning" not in result
+        assert "error" not in result
+


These new tests are inside a unittest.TestCase but use bare assert statements, while the rest of this file uses self.assert* methods. Mixing styles makes failures less consistent and can reduce diagnostics when running under unittest directly. Consider converting these assertions to self.assertIn/self.assertNotIn/self.assertTrue, etc., for consistency with the surrounding tests.

…_file (NousResearch#15759) Address three Copilot review findings on PR NousResearch#15762: 1. Loop-detection bypass via alternate path forms: both read_key constructions now use resolved_str instead of the raw path argument, so ./foo.py and /abs/foo.py share the consecutive counter and cannot reset it by switching string forms. 2. Stale line-number reference in the TestDedupLoopDetection docstring ("at line 538") removed; guard referenced semantically instead. 3. Bare assert statements in TestDedupLoopDetection converted to self.assert* methods for consistent unittest failure reporting. Added test_dedup_path_normalization_shares_counter to prove the bypass is closed: reverted fix → test fails with dedup stub instead of BLOCKED. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

briandevans · 2026-04-25T20:12:01Z

Thanks @copilot — all three findings are real. Addressed in `3dfec2cd`.

1. Loop-detection bypass via alternate path forms (line 448)
Real bug. Both `read_key` constructions (the dedup-hit path at the old line 442 and the full-read path at the old line 528) now use `resolved_str` instead of the raw `path` argument. Added `test_dedup_path_normalization_shares_counter` which patches `_resolve_path_for_task` to a fixed return value, then alternates four different raw strings that all resolve there — the 4th is now BLOCKED. Regressed the fix: test fails with a dedup stub instead of BLOCKED without the change.

2. Hard-coded line number in docstring (test line 198)
Removed "at line 538" — the guard is now referenced semantically: "the loop-detection guard was never reached".

3. Bare `assert` in `TestDedupLoopDetection` (test line 216)
Converted all bare `assert` / `assert … not in …` / `assert … in …` / `assert … is True` statements to `self.assertTrue` / `self.assertNotIn` / `self.assertIn` throughout the class — consistent with the rest of the file.

briandevans · 2026-04-26T16:36:15Z

@copilot Both issues are already resolved: the docstring no longer references any hardcoded line number, and all assertions in TestDedupLoopDetection use self.assert* methods consistently. Thanks for the review.

briandevans · 2026-04-28T10:12:05Z

Closing — superseded by @teknium1's #16382, which closed #15759 and is the better fix here.

Both PRs target the read_file dedup-stub vs loop-detector race. Coverage comparison:

Root cause: Mine routed the dedup early-return through the loop-detector counter so dedup stubs would still increment the loop count. fix(file-tools): escalate to BLOCKED on repeated read_file dedup stubs #16382 takes a stronger approach — it escalates to a BLOCKED status on repeated dedup stubs, which surfaces the runaway read loop more visibly than my counter-only fix.
Adjacent hardening: Subsequent commits on main (a32b325 invalidate-on-write, 977d5f5 keep dedup-status out of content, ced8f44 broaden dedup-status write guard) further harden the boundary, all of which post-date my PR.

No remaining gap. Thanks @teknium1.

Copilot AI review requested due to automatic review settings April 25, 2026 19:19

Copilot started reviewing on behalf of briandevans April 25, 2026 19:20 View session

Copilot AI reviewed Apr 25, 2026

View reviewed changes

alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/tools Tool registry, model_tools, toolsets tool/file File tools (read, write, patch, search) labels Apr 25, 2026

alt-glitch mentioned this pull request Apr 27, 2026

fix(file-tools): escalate to BLOCKED on repeated read_file dedup stubs #16382

Merged

briandevans closed this Apr 28, 2026

github-actions Bot mentioned this pull request May 1, 2026

chore: bump NousResearch/hermes-agent version from v2026.4.23 to v2026.4.30 Docker-Hub-sirmark/docker-hermes-agent#4

Merged

Navigation Menu

Uh oh!

fix(tools): dedup early-return bypasses loop detection in read_file (#15759) - #15762

fix(tools): dedup early-return bypasses loop detection in read_file (#15759)#15762
briandevans wants to merge 2 commits into
NousResearch:mainfrom
briandevans:fix/read-file-dedup-bypasses-loop-detection-15759

briandevans commented Apr 25, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 25, 2026

Uh oh!

Copilot AI Apr 25, 2026

Uh oh!

Copilot AI Apr 25, 2026

Uh oh!

briandevans commented Apr 25, 2026

Uh oh!

briandevans commented Apr 26, 2026

Uh oh!

briandevans commented Apr 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		Without this fix, the dedup path returned early and the loop-detection guard
		at line 538 was never reached — the agent could loop forever on a dedup stub.

Uh oh!

Conversation

briandevans commented Apr 25, 2026

Summary

The bug

The fix

Test plan

Related

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

Uh oh!

briandevans commented Apr 25, 2026

Uh oh!

briandevans commented Apr 26, 2026

Uh oh!

briandevans commented Apr 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants