Merge pull request #1245 from unoplat/fix-read-tool-call-truncation-issue

fix(read-tool-call-pagination-issue): align updater read clamp and no…

Author: JayGhiya

unoplat-code-confluence-query-engine/src/unoplat_code_confluence_query_engine/services/temporal/temporal_agents.py

@@ -177,6 +177,12 @@ def build_section_updater_prompt(
         f"Codebase path: {codebase_path}\n"
         f"Programming language metadata: {programming_language_metadata}\n"
         f"Section data:\n{section_data}\n\n"
+        "Deterministic update workflow:\n"
+        "- Read the target section first (and only within scope).\n"
+        "- Compute expected section content from section_data.\n"
+        "- Compare current vs expected content before writing.\n"
+        "- If equivalent, do NOT call write tools and return status='no_changes'.\n"
+        "- Use updater_apply_patch/updater_edit_file only when a real diff exists.\n\n"
         "Return summary metadata only (no raw file content)."
     )
 
@@ -317,6 +323,36 @@ def validate_agents_md_updater_output(
         raise ModelRetry("touched_file_paths cannot be empty")
     if not output.file_changes:
         raise ModelRetry("file_changes cannot be empty")
+
+    if output.status == "no_changes":
+        for file_change in output.file_changes:
+            if file_change.changed:
+                raise ModelRetry(
+                    "status no_changes requires file_changes[].changed to be false"
+                )
+            if file_change.change_type != "unchanged":
+                raise ModelRetry(
+                    "status no_changes requires file_changes[].change_type to be 'unchanged'"
+                )
+
+    if output.status == "updated":
+        has_changed_file = any(
+            file_change.changed for file_change in output.file_changes
+        )
+        if not has_changed_file:
+            raise ModelRetry(
+                "status updated requires at least one file_changes[].changed to be true"
+            )
+
+        has_mutation_change_type = any(
+            file_change.change_type in {"created", "updated"}
+            for file_change in output.file_changes
+        )
+        if not has_mutation_change_type:
+            raise ModelRetry(
+                "status updated requires at least one file_changes[].change_type in {'created','updated'}"
+            )
+
     return output
 
 
@@ -509,6 +545,9 @@ def create_agents_md_updater_agent(
 - Always pass absolute file paths.
 - Operate only within the current codebase root.
 - Prefer updater_apply_patch for creates and structured updates.
+- Read the target section first, then compute the expected section body from section_data.
+- Compare before write: if content is already equivalent, do not call write tools and return status="no_changes".
+- Call updater_apply_patch/updater_edit_file only when an actual diff exists.
 
 Output constraints:
 - Return summary metadata only.

unoplat-code-confluence-query-engine/src/unoplat_code_confluence_query_engine/tools/agents_md_updater_tools.py

@@ -66,13 +66,12 @@ def _normalize_scoped_path(
     return normalized
 
 
-def _validate_read_bounds(offset: int, limit: int) -> None:
+def _validate_read_bounds(offset: int, limit: int) -> int:
     if offset < 0:
         raise ModelRetry("invalid_patch: offset must be >= 0")
     if limit < 1:
         raise ModelRetry("invalid_patch: limit must be >= 1")
-    if limit > READ_MAX_LIMIT:
-        raise ModelRetry(f"invalid_patch: limit must be <= {READ_MAX_LIMIT}")
+    return min(limit, READ_MAX_LIMIT)
 
 
 def _format_numbered_line(line_number: int, content: str) -> str:
@@ -229,7 +228,7 @@ async def updater_read_file(
     limit: int = READ_MAX_LIMIT,
 ) -> str:
     """Read file content with absolute path guard and truncation."""
-    _validate_read_bounds(offset, limit)
+    effective_limit = _validate_read_bounds(offset, limit)
     target = _normalize_scoped_path(ctx, file_path=file_path, allow_missing=False)
     if target.is_dir():
         raise ModelRetry(f"not_found: path is a directory '{target.as_posix()}'")
@@ -241,7 +240,7 @@ async def updater_read_file(
             total_lines += 1
             if total_lines <= offset:
                 continue
-            if len(output_lines) >= limit:
+            if len(output_lines) >= effective_limit:
                 break
             output_lines.append(
                 _format_numbered_line(total_lines, raw_line.rstrip("\r\n"))
@@ -294,15 +293,17 @@ async def updater_edit_file(
 
     replace_count = expected_replacements if expected_replacements is not None else 1
     updated_content = existing_content.replace(old_string, new_string, replace_count)
+    changed = updated_content != existing_content
 
-    async with aiofiles.open(target, mode="w", encoding="utf-8") as handle:
-        await handle.write(updated_content)
+    if changed:
+        async with aiofiles.open(target, mode="w", encoding="utf-8") as handle:
+            await handle.write(updated_content)
 
     return {
         "file_path": target.as_posix(),
-        "changed": updated_content != existing_content,
+        "changed": changed,
         "replacements": replace_count,
-        "status": "updated",
+        "status": "updated" if changed else "unchanged",
     }
 
 
@@ -314,6 +315,7 @@ async def updater_apply_patch(
     operations = _parse_patch_operations(patch_text)
 
     planned_writes: dict[Path, str | None] = {}
+    skipped_noop_files = 0
     for operation in operations:
         scoped = _normalize_scoped_path(
             ctx,
@@ -330,6 +332,16 @@ async def updater_apply_patch(
             continue
 
         if operation.operation_type == "add":
+            if scoped.exists() and scoped.is_dir():
+                raise ModelRetry(
+                    f"not_found: add target is a directory '{scoped.as_posix()}'"
+                )
+            if scoped.exists():
+                async with aiofiles.open(scoped, mode="r", encoding="utf-8") as handle:
+                    existing_content = await handle.read()
+                if existing_content == operation.add_content:
+                    skipped_noop_files += 1
+                    continue
             planned_writes[scoped] = operation.add_content
             continue
 
@@ -344,6 +356,9 @@ async def updater_apply_patch(
                 existing_content,
                 chunks=operation.chunks,
             )
+            if updated_content == existing_content:
+                skipped_noop_files += 1
+                continue
             planned_writes[scoped] = updated_content
             continue
 
@@ -364,4 +379,5 @@ async def updater_apply_patch(
         "status": "applied",
         "operations": len(operations),
         "changed_files": changed_files,
+        "skipped_noop_files": skipped_noop_files,
     }

unoplat-code-confluence-query-engine/tests/tools/test_agents_md_updater_output_validator.py

@@ -0,0 +1,102 @@
+"""Unit tests for AGENTS.md updater output validation semantics."""
+
+from __future__ import annotations
+
+from pydantic_ai import ModelRetry
+import pytest
+
+from unoplat_code_confluence_query_engine.models.output.agents_md_updater_output import (
+    AgentsMdUpdaterOutput,
+    UpdaterFileChange,
+)
+from unoplat_code_confluence_query_engine.services.temporal.temporal_agents import (
+    validate_agents_md_updater_output,
+)
+
+
+def test_no_changes_with_changed_file_is_rejected() -> None:
+    """status=no_changes cannot include changed=true entries."""
+    output = AgentsMdUpdaterOutput(
+        status="no_changes",
+        touched_file_paths=["/repo/AGENTS.md"],
+        file_changes=[
+            UpdaterFileChange(
+                path="/repo/AGENTS.md",
+                changed=True,
+                change_type="updated",
+                change_summary="unexpected mutation",
+            )
+        ],
+    )
+
+    with pytest.raises(
+        ModelRetry,
+        match=r"status no_changes requires file_changes\[\]\.changed to be false",
+    ):
+        validate_agents_md_updater_output(output)
+
+
+def test_updated_with_no_changed_files_is_rejected() -> None:
+    """status=updated requires at least one changed file."""
+    output = AgentsMdUpdaterOutput(
+        status="updated",
+        touched_file_paths=["/repo/AGENTS.md"],
+        file_changes=[
+            UpdaterFileChange(
+                path="/repo/AGENTS.md",
+                changed=False,
+                change_type="unchanged",
+                change_summary="already up to date",
+            )
+        ],
+    )
+
+    with pytest.raises(
+        ModelRetry,
+        match=r"status updated requires at least one file_changes\[\]\.changed to be true",
+    ):
+        validate_agents_md_updater_output(output)
+
+
+def test_valid_no_changes_payload_passes() -> None:
+    """Consistent no_changes payload should pass validation."""
+    output = AgentsMdUpdaterOutput(
+        status="no_changes",
+        touched_file_paths=["/repo/AGENTS.md"],
+        file_changes=[
+            UpdaterFileChange(
+                path="/repo/AGENTS.md",
+                changed=False,
+                change_type="unchanged",
+                change_summary="section already equivalent",
+            )
+        ],
+    )
+
+    validated = validate_agents_md_updater_output(output)
+    assert validated is output
+
+
+def test_valid_updated_payload_passes() -> None:
+    """Consistent updated payload should pass validation."""
+    output = AgentsMdUpdaterOutput(
+        status="updated",
+        touched_file_paths=["/repo/AGENTS.md", "/repo/business_logic_references.md"],
+        file_changes=[
+            UpdaterFileChange(
+                path="/repo/AGENTS.md",
+                changed=True,
+                change_type="updated",
+                change_summary="refreshed section content",
+            ),
+            UpdaterFileChange(
+                path="/repo/business_logic_references.md",
+                changed=False,
+                change_type="unchanged",
+                change_summary="already current",
+            ),
+        ],
+    )
+
+    validated = validate_agents_md_updater_output(output)
+    assert validated is output

unoplat-code-confluence-query-engine/tests/tools/test_agents_md_updater_tools.py

@@ -22,6 +22,7 @@
     AgentDependencies,
 )
 from unoplat_code_confluence_query_engine.tools.agents_md_updater_tools import (
+    READ_MAX_LIMIT,
     _apply_replacement_chunks,
     _parse_patch_operations,
     updater_apply_patch,
@@ -487,7 +488,9 @@ class TestRootScopeEnforcement:
     """Regression tests for codebase-root-only file operation enforcement."""
 
     @pytest.mark.asyncio
-    async def test_relative_path_rejected_with_out_of_scope(self, tmp_path: Path) -> None:
+    async def test_relative_path_rejected_with_out_of_scope(
+        self, tmp_path: Path
+    ) -> None:
         """Relative paths must raise ModelRetry with 'out_of_scope' prefix."""
         ctx = _make_run_context(str(tmp_path))
         patch = _build_patch(
@@ -578,3 +581,94 @@ async def test_valid_path_within_root_succeeds(self, tmp_path: Path) -> None:
         result = await updater_apply_patch(ctx, patch)
         assert result["status"] == "applied"
         assert agents_md.read_text() == "# Title\nnew line\n"
+
+
+class TestUpdaterReadFileLimits:
+    """Validate read bound handling for updater_read_file."""
+
+    @pytest.mark.asyncio
+    async def test_limit_above_max_is_clamped(self, tmp_path: Path) -> None:
+        """Oversized limit must clamp to READ_MAX_LIMIT instead of failing."""
+        target = tmp_path / "AGENTS.md"
+        target.write_text("\n".join(f"line-{index}" for index in range(1, 2105)) + "\n")
+        ctx = _make_run_context(str(tmp_path))
+
+        result = await updater_read_file(ctx, str(target), offset=0, limit=4000)
+        numbered_lines = [
+            line for line in result.splitlines() if len(line) > 7 and line[5:7] == "| "
+        ]
+
+        assert len(numbered_lines) == READ_MAX_LIMIT
+        assert "[truncated]" in result
+
+    @pytest.mark.asyncio
+    async def test_limit_below_one_still_rejected(self, tmp_path: Path) -> None:
+        """Limit less than one must still fail with invalid_patch."""
+        target = tmp_path / "AGENTS.md"
+        target.write_text("content\n")
+        ctx = _make_run_context(str(tmp_path))
+
+        with pytest.raises(ModelRetry, match="limit must be >= 1"):
+            await updater_read_file(ctx, str(target), limit=0)
+
+
+class TestUpdaterApplyPatchNoopSemantics:
+    """Ensure no-op updates do not write or count as changed files."""
+
+    @pytest.mark.asyncio
+    async def test_update_noop_does_not_count_as_change(self, tmp_path: Path) -> None:
+        """Update patch yielding identical content should be treated as no-op."""
+        target = tmp_path / "AGENTS.md"
+        target.write_text("line one\nline two\n")
+        ctx = _make_run_context(str(tmp_path))
+
+        patch = _build_patch(
+            f"*** Update File: {target}",
+            "@@",
+            "-line two",
+            "+line two",
+        )
+
+        result = await updater_apply_patch(ctx, patch)
+
+        assert result["operations"] == 1
+        assert result["changed_files"] == 0
+        assert result["skipped_noop_files"] == 1
+        assert target.read_text() == "line one\nline two\n"
+
+    @pytest.mark.asyncio
+    async def test_mixed_operations_track_requested_vs_actual_changes(
+        self, tmp_path: Path
+    ) -> None:
+        """Operations count should include no-op intents; changed_files should not."""
+        update_target = tmp_path / "AGENTS.md"
+        update_target.write_text("alpha\nbeta\n")
+
+        noop_target = tmp_path / "README.md"
+        noop_target.write_text("same\n")
+
+        add_target = tmp_path / "business_logic_references.md"
+        ctx = _make_run_context(str(tmp_path))
+
+        patch = _build_patch(
+            f"*** Update File: {update_target}",
+            "@@",
+            "-beta",
+            "+BETA",
+            f"*** Update File: {noop_target}",
+            "@@",
+            "-same",
+            "+same",
+            f"*** Add File: {add_target}",
+            "+# Business Logic",
+            "+artifact content",
+        )
+
+        result = await updater_apply_patch(ctx, patch)
+
+        assert result["operations"] == 3
+        assert result["changed_files"] == 2
+        assert result["skipped_noop_files"] == 1
+        assert update_target.read_text() == "alpha\nBETA\n"
+        assert noop_target.read_text() == "same\n"
+        assert add_target.read_text() == "# Business Logic\nartifact content"