test: add failing tests for circular include detection in non-recursive mode (#553) (#554)

* test: add failing tests for circular include detection in non-recursive mode (#553)

Add 6 unit tests and 6 E2E tests that detect the bug where circular
<include> tags cause an infinite loop in the default (non-recursive)
preprocess path. The _seen set is only populated inside `if recursive:`
blocks, making cycle detection dead code in non-recursive mode.

Unit tests (test_circular_includes.py):
- 4 circular tests that fail with timeout (infinite loop)
- 2 regression guards that pass (non-circular includes still work)

E2E tests (test_e2e_issue_553_circular_includes_non_recursive.py):
- 4 subprocess-based CLI tests that fail with timeout
- 2 regression guards that pass

All circular tests are verified to fail on current code and will pass
once the fix is implemented.

Refs #553

* fix: add circular include cycle detection in non-recursive preprocess path (#553)

Populate _seen tracking in the non-recursive convergence loops of
process_include_tags and process_backtick_includes. Previously _seen was
only updated inside `if recursive:` blocks, making the cycle check dead
code in non-recursive (default) mode and causing infinite loops.

Also clean up unused _timeout/signal/contextlib imports in tests and
make E2E output file assertion unconditional.

Fixes #553

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: address PR review — add PYTHONPATH to E2E subprocess tests and replace pytest-timeout markers with signal-based timeouts

- E2E tests now set PYTHONPATH to project root, matching other E2E tests in the repo (prevents ModuleNotFoundError in CI)
- Replace @pytest.mark.timeout(10) with @_with_timeout(10) using SIGALRM since pytest-timeout is not a declared dependency

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

Author: Serhan-Asad

pdd/preprocess.py

@@ -197,11 +197,30 @@ def replace_include(match):
             console.print(f"[bold red]Error processing include:[/bold red] {str(e)}")
             _dbg(f"Error processing backtick include {file_path}: {e}")
             return f"```[Error processing include: {file_path}]```"
+    _expanded_prior = set()
+    _expanded_current = set()
+
+    def replace_include_tracked(match):
+        file_path = match.group(1).strip()
+        try:
+            full_path = get_file_path(file_path)
+            resolved = os.path.realpath(full_path)
+            if resolved in _expanded_prior:
+                raise ValueError(f"Circular include detected: {file_path} is already in the include chain")
+            _expanded_current.add(resolved)
+        except (FileNotFoundError, ValueError):
+            raise
+        except Exception:
+            pass
+        return replace_include(match)
+
     prev_text = ""
     current_text = text
     while prev_text != current_text:
         prev_text = current_text
-        current_text = re.sub(pattern, replace_include, current_text, flags=re.DOTALL)
+        _expanded_current = set()
+        current_text = re.sub(pattern, replace_include_tracked, current_text, flags=re.DOTALL)
+        _expanded_prior.update(_expanded_current)
     return current_text
 
 def process_xml_tags(text: str, recursive: bool, _seen: Optional[set] = None) -> str:
@@ -290,16 +309,35 @@ def replace_include(match):
             console.print(f"[bold red]Error processing include:[/bold red] {str(e)}")
             _dbg(f"Error processing XML include {file_path}: {e}")
             return f"[Error processing include: {file_path}]"
+    _expanded_prior = set()
+    _expanded_current = set()
+
+    def replace_include_tracked(match):
+        file_path = match.group(1).strip()
+        try:
+            full_path = get_file_path(file_path)
+            resolved = os.path.realpath(full_path)
+            if resolved in _expanded_prior:
+                raise ValueError(f"Circular include detected: {file_path} is already in the include chain")
+            _expanded_current.add(resolved)
+        except (FileNotFoundError, ValueError):
+            raise
+        except Exception:
+            pass
+        return replace_include(match)
+
     prev_text = ""
     current_text = text
     while prev_text != current_text:
         prev_text = current_text
+        _expanded_current = set()
         code_spans = _extract_code_spans(current_text)
         def replace_include_with_spans(match):
             if _intersects_any_span(match.start(), match.end(), code_spans):
                 return match.group(0)
-            return replace_include(match)
+            return replace_include_tracked(match)
         current_text = re.sub(pattern, replace_include_with_spans, current_text, flags=re.DOTALL)
+        _expanded_prior.update(_expanded_current)
     return current_text
 
 def process_pdd_tags(text: str) -> str:

tests/test_circular_includes.py

@@ -1,19 +1,42 @@
 """
-Tests for circular <include> detection in pdd/preprocess.py (Issue #521).
+Tests for circular <include> detection in pdd/preprocess.py (Issues #521, #553).
 
-The preprocessor has no cycle detection — circular includes recurse ~82 times
-until Python's recursion limit, then the broad `except Exception` swallows
-the RecursionError and returns corrupted output with exit code 0.
+Issue #521: Circular includes in the recursive path must be detected.
+Issue #553: Circular includes in the non-recursive (default) path must also
+be detected. The _seen set is only populated inside `if recursive:` blocks,
+so the non-recursive convergence loop has no cycle guard and loops forever.
 
 These tests verify that circular includes produce an error (exception or
 error marker in output), NOT silently corrupted content.
 """
 
 import os
+import signal
 import pytest
 from unittest.mock import patch, mock_open, MagicMock
 from pdd.preprocess import preprocess
 
+
+def _timeout_handler(signum, frame):
+    raise TimeoutError("Test timed out — possible infinite loop")
+
+
+def _with_timeout(seconds=10):
+    """Decorator that enforces a real timeout using SIGALRM (Unix only)."""
+    def decorator(func):
+        def wrapper(*args, **kwargs):
+            old_handler = signal.signal(signal.SIGALRM, _timeout_handler)
+            signal.alarm(seconds)
+            try:
+                return func(*args, **kwargs)
+            finally:
+                signal.alarm(0)
+                signal.signal(signal.SIGALRM, old_handler)
+        wrapper.__name__ = func.__name__
+        wrapper.__doc__ = func.__doc__
+        return wrapper
+    return decorator
+
 # Store original so we can restore
 _original_pdd_path = os.environ.get('PDD_PATH')
 
@@ -194,3 +217,135 @@ def test_diamond_includes_not_falsely_flagged(self):
             assert 'C' in result
             # D is included twice (via B and via C) — that's fine, not circular
             assert result.count('Shared') == 2
+
+
+class TestCircularIncludesNonRecursive:
+    """Issue #553: Circular includes must be detected in the non-recursive (default) path.
+
+    PR #528 added cycle detection for recursive=True, but the non-recursive path
+    (which uses a while-loop for convergence) never populates the _seen set,
+    so circular includes cause an infinite loop instead of raising ValueError.
+    """
+
+    def setup_method(self):
+        set_pdd_path('/mock/path')
+
+    def teardown_method(self):
+        if _original_pdd_path is not None:
+            os.environ['PDD_PATH'] = _original_pdd_path
+        elif 'PDD_PATH' in os.environ:
+            del os.environ['PDD_PATH']
+
+    @_with_timeout(10)
+    def test_circular_xml_includes_non_recursive_must_error(self):
+        """A→B→A circular XML include with recursive=False must raise ValueError, not loop forever."""
+        file_map = {
+            'a.txt': 'Hello\n<include>b.txt</include>',
+            'b.txt': 'World\n<include>a.txt</include>',
+        }
+        with patch('builtins.open', mock_open()) as m:
+            m.side_effect = _make_mock_open(file_map)
+
+            # Buggy code: infinite loop in the while-loop → timeout → test fails
+            # Fixed code: ValueError("Circular include detected: ...") → test passes
+            with pytest.raises(ValueError, match="Circular include detected"):
+                preprocess(
+                    '<include>a.txt</include>',
+                    recursive=False,
+                    double_curly_brackets=False,
+                )
+
+    @_with_timeout(10)
+    def test_circular_backtick_includes_non_recursive_must_error(self):
+        """A→B→A circular backtick include with recursive=False must raise ValueError."""
+        file_map = {
+            'x.txt': 'Foo\n```<y.txt>```',
+            'y.txt': 'Bar\n```<x.txt>```',
+        }
+        with patch('builtins.open', mock_open()) as m:
+            m.side_effect = _make_mock_open(file_map)
+
+            # Buggy code: infinite loop in process_backtick_includes while-loop
+            # Fixed code: ValueError raised
+            with pytest.raises(ValueError, match="Circular include detected"):
+                preprocess(
+                    '```<x.txt>```',
+                    recursive=False,
+                    double_curly_brackets=False,
+                )
+
+    @_with_timeout(10)
+    def test_self_referencing_include_non_recursive_must_error(self):
+        """A file that includes itself must be detected in non-recursive mode."""
+        file_map = {
+            'self.txt': 'Content\n<include>self.txt</include>',
+        }
+        with patch('builtins.open', mock_open()) as m:
+            m.side_effect = _make_mock_open(file_map)
+
+            with pytest.raises(ValueError, match="Circular include detected"):
+                preprocess(
+                    '<include>self.txt</include>',
+                    recursive=False,
+                    double_curly_brackets=False,
+                )
+
+    @_with_timeout(10)
+    def test_three_file_cycle_non_recursive_must_error(self):
+        """A→B→C→A three-file cycle must be detected in non-recursive mode."""
+        file_map = {
+            'a.txt': 'AAA\n<include>b.txt</include>',
+            'b.txt': 'BBB\n<include>c.txt</include>',
+            'c.txt': 'CCC\n<include>a.txt</include>',
+        }
+        with patch('builtins.open', mock_open()) as m:
+            m.side_effect = _make_mock_open(file_map)
+
+            with pytest.raises(ValueError, match="Circular include detected"):
+                preprocess(
+                    '<include>a.txt</include>',
+                    recursive=False,
+                    double_curly_brackets=False,
+                )
+
+    def test_non_circular_includes_non_recursive_still_work(self):
+        """Non-circular includes (A→B→C, no cycle) must still work in non-recursive mode."""
+        file_map = {
+            'top.txt': 'Top\n<include>mid.txt</include>',
+            'mid.txt': 'Mid\n<include>leaf.txt</include>',
+            'leaf.txt': 'Leaf',
+        }
+        with patch('builtins.open', mock_open()) as m:
+            m.side_effect = _make_mock_open(file_map)
+
+            result = preprocess(
+                '<include>top.txt</include>',
+                recursive=False,
+                double_curly_brackets=False,
+            )
+
+            assert 'Top' in result
+            assert 'Mid' in result
+            assert 'Leaf' in result
+
+    def test_diamond_includes_non_recursive_not_falsely_flagged(self):
+        """Diamond pattern (A→B, A→C, B→D, C→D) is NOT circular and must work."""
+        file_map = {
+            'a.txt': '<include>b.txt</include>\n<include>c.txt</include>',
+            'b.txt': 'B\n<include>d.txt</include>',
+            'c.txt': 'C\n<include>d.txt</include>',
+            'd.txt': 'Shared',
+        }
+        with patch('builtins.open', mock_open()) as m:
+            m.side_effect = _make_mock_open(file_map)
+
+            result = preprocess(
+                '<include>a.txt</include>',
+                recursive=False,
+                double_curly_brackets=False,
+            )
+
+            assert 'B' in result
+            assert 'C' in result
+            # D is included twice (via B and via C) — that's fine, not circular
+            assert result.count('Shared') == 2