refactor: split repo context responsibilities

Author: avidspartan1

pr_agent/algo/repo_context.py

@@ -75,6 +75,107 @@ def _get_repo_context_process_cache_key(git_provider, context_files: list, max_l
     return type(git_provider).__name__, pr_url, _get_repo_context_cache_key(context_files, max_lines)
 
 
+def _get_repo_context_config() -> tuple[list, int] | None:
+    context_files = get_settings().config.get("repo_context_files", [])
+    if not context_files:
+        return None
+
+    if isinstance(context_files, str):
+        get_logger().warning(
+            "repo_context_files should be a list of file paths; treating string value as one file path",
+            artifact={"repo_context_files": context_files},
+        )
+        context_files = [context_files]
+    elif not isinstance(context_files, list):
+        get_logger().warning(
+            "repo_context_files should be a list of file paths; skipping repo context",
+            artifact={"repo_context_files": context_files},
+        )
+        return None
+
+    max_lines = get_settings().config.get("repo_context_max_lines", 500)
+    try:
+        max_lines = max(0, int(max_lines))
+    except (TypeError, ValueError):
+        max_lines = 500
+
+    return context_files, max_lines
+
+
+def _provider_supports_repo_context(git_provider) -> bool:
+    provider_class = type(git_provider)
+    if provider_class.get_repo_file_content is not GitProvider.get_repo_file_content:
+        return True
+
+    if provider_class not in _unsupported_repo_context_provider_classes:
+        _unsupported_repo_context_provider_classes.add(provider_class)
+        get_logger().warning(
+            f"repo_context_files is configured, but {provider_class.__name__} does not support repository "
+            "file fetching; skipping repo context"
+        )
+    return False
+
+
+def _get_provider_repo_context_cache(git_provider) -> _RepoContextCache:
+    repo_context_cache = getattr(git_provider, REPO_CONTEXT_CACHE_ATTRIBUTE, None)
+    if repo_context_cache is None or not isinstance(repo_context_cache, _RepoContextCache):
+        repo_context_cache = _RepoContextCache()
+        setattr(git_provider, REPO_CONTEXT_CACHE_ATTRIBUTE, repo_context_cache)
+    return repo_context_cache
+
+
+def _get_cached_repo_context(git_provider, context_files: list, max_lines: int):
+    process_cache_key = _get_repo_context_process_cache_key(git_provider, context_files, max_lines)
+    if process_cache_key is not None:
+        cached_repo_context = _repo_context_process_cache.get(process_cache_key, _REPO_CONTEXT_CACHE_MISS)
+        if cached_repo_context is not _REPO_CONTEXT_CACHE_MISS:
+            return cached_repo_context
+
+    cache_key = _get_repo_context_cache_key(context_files, max_lines)
+    cached_repo_context = _get_provider_repo_context_cache(git_provider).get(cache_key, _REPO_CONTEXT_CACHE_MISS)
+    if cached_repo_context is not _REPO_CONTEXT_CACHE_MISS:
+        return cached_repo_context
+
+    return _REPO_CONTEXT_CACHE_MISS
+
+
+def _store_repo_context(git_provider, context_files: list, max_lines: int, repo_context: str) -> None:
+    cache_key = _get_repo_context_cache_key(context_files, max_lines)
+    _get_provider_repo_context_cache(git_provider)[cache_key] = repo_context
+
+    process_cache_key = _get_repo_context_process_cache_key(git_provider, context_files, max_lines)
+    if process_cache_key:
+        _repo_context_process_cache[process_cache_key] = repo_context
+
+
+def _load_repo_context_files(git_provider, context_files: list) -> tuple[dict[str, str], bool]:
+    files = {}
+    had_fetch_error = False
+    for file_path in context_files:
+        if not isinstance(file_path, str) or not file_path.strip():
+            get_logger().warning("Skipping invalid repo context file path", artifact={"file_path": file_path})
+            continue
+
+        file_path = file_path.strip()
+        try:
+            content = git_provider.get_repo_file_content(file_path)
+        except Exception as e:
+            had_fetch_error = True
+            get_logger().warning(f"Failed to load repo context file: {file_path}", artifact={"error": str(e)})
+            continue
+
+        if not content:
+            get_logger().debug(f"Repo context file is empty or missing: {file_path}")
+            continue
+
+        if isinstance(content, bytes):
+            content = content.decode("utf-8", errors="replace")
+
+        files[file_path] = str(content).rstrip()
+
+    return files, had_fetch_error
+
+
 def render_instruction_files(files: dict[str, str]) -> str:
     parts = [
         INSTRUCTION_FILES_INTRO,
@@ -139,88 +240,26 @@ def render_instruction_files_with_line_budget(files: dict[str, str], max_lines:
 
 
 def build_repo_context(git_provider) -> str:
-    context_files = get_settings().config.get("repo_context_files", [])
-    if not context_files:
-        return ""
-    if isinstance(context_files, str):
-        get_logger().warning(
-            "repo_context_files should be a list of file paths; treating string value as one file path",
-            artifact={"repo_context_files": context_files},
-        )
-        context_files = [context_files]
-    elif not isinstance(context_files, list):
-        get_logger().warning(
-            "repo_context_files should be a list of file paths; skipping repo context",
-            artifact={"repo_context_files": context_files},
-        )
+    repo_context_config = _get_repo_context_config()
+    if repo_context_config is None:
         return ""
 
-    provider_class = type(git_provider)
-    if provider_class.get_repo_file_content is GitProvider.get_repo_file_content:
-        if provider_class not in _unsupported_repo_context_provider_classes:
-            _unsupported_repo_context_provider_classes.add(provider_class)
-            get_logger().warning(
-                f"repo_context_files is configured, but {provider_class.__name__} does not support repository "
-                "file fetching; skipping repo context"
-            )
+    context_files, max_lines = repo_context_config
+    if not _provider_supports_repo_context(git_provider):
         return ""
 
-    max_lines = get_settings().config.get("repo_context_max_lines", 500)
-    try:
-        max_lines = max(0, int(max_lines))
-    except (TypeError, ValueError):
-        max_lines = 500
-
-    cache_key = _get_repo_context_cache_key(context_files, max_lines)
-    process_cache_key = _get_repo_context_process_cache_key(git_provider, context_files, max_lines)
-    if process_cache_key is not None:
-        cached_repo_context = _repo_context_process_cache.get(process_cache_key, _REPO_CONTEXT_CACHE_MISS)
-        if cached_repo_context is not _REPO_CONTEXT_CACHE_MISS:
-            return cached_repo_context
-
-    repo_context_cache = getattr(git_provider, REPO_CONTEXT_CACHE_ATTRIBUTE, None)
-    if repo_context_cache is None or not isinstance(repo_context_cache, _RepoContextCache):
-        repo_context_cache = _RepoContextCache()
-        setattr(git_provider, REPO_CONTEXT_CACHE_ATTRIBUTE, repo_context_cache)
-    cached_repo_context = repo_context_cache.get(cache_key, _REPO_CONTEXT_CACHE_MISS)
+    cached_repo_context = _get_cached_repo_context(git_provider, context_files, max_lines)
     if cached_repo_context is not _REPO_CONTEXT_CACHE_MISS:
         return cached_repo_context
 
-    files = {}
-    had_fetch_error = False
-    for file_path in context_files:
-        if not isinstance(file_path, str) or not file_path.strip():
-            get_logger().warning("Skipping invalid repo context file path", artifact={"file_path": file_path})
-            continue
-
-        file_path = file_path.strip()
-        try:
-            content = git_provider.get_repo_file_content(file_path)
-        except Exception as e:
-            had_fetch_error = True
-            get_logger().warning(f"Failed to load repo context file: {file_path}", artifact={"error": str(e)})
-            continue
-
-        if not content:
-            get_logger().debug(f"Repo context file is empty or missing: {file_path}")
-            continue
-
-        if isinstance(content, bytes):
-            content = content.decode("utf-8", errors="replace")
-
-        files[file_path] = str(content).rstrip()
-
+    files, had_fetch_error = _load_repo_context_files(git_provider, context_files)
     if not files and had_fetch_error:
         return ""
 
     if not files:
-        repo_context_cache[cache_key] = ""
-        if process_cache_key:
-            _repo_context_process_cache[process_cache_key] = ""
+        _store_repo_context(git_provider, context_files, max_lines, "")
         return ""
 
     repo_context = render_instruction_files_with_line_budget(files, max_lines)
-    repo_context_cache[cache_key] = repo_context
-    if process_cache_key:
-        _repo_context_process_cache[process_cache_key] = repo_context
+    _store_repo_context(git_provider, context_files, max_lines, repo_context)
     return repo_context

tests/unittest/test_repo_context.py

@@ -152,6 +152,58 @@ def test_repo_context_cache_evicts_oldest_entry_when_full():
         assert cache.get("third", missing) == "three"
 
 
+def test_get_repo_context_config_normalizes_inputs(repo_context_settings):
+    repo_context_settings.set("CONFIG.REPO_CONTEXT_FILES", "AGENTS.md")
+    repo_context_settings.set("CONFIG.REPO_CONTEXT_MAX_LINES", "12")
+
+    assert repo_context._get_repo_context_config() == (["AGENTS.md"], 12)
+
+
+def test_get_repo_context_config_rejects_non_list_container(repo_context_settings):
+    repo_context_settings.set("CONFIG.REPO_CONTEXT_FILES", {"AGENTS.md": True})
+
+    assert repo_context._get_repo_context_config() is None
+
+
+def test_provider_supports_repo_context_warns_once_for_unsupported_provider(repo_context_settings):
+    provider = UnsupportedProvider()
+
+    with patch("pr_agent.algo.repo_context.get_logger") as mock_get_logger:
+        assert repo_context._provider_supports_repo_context(provider) is False
+        assert repo_context._provider_supports_repo_context(provider) is False
+
+    mock_get_logger.return_value.warning.assert_called_once_with(
+        "repo_context_files is configured, but UnsupportedProvider does not support repository file fetching; "
+        "skipping repo context"
+    )
+
+
+def test_load_repo_context_files_normalizes_fetch_results():
+    provider = FakeProvider({
+        "AGENTS.md": b"Repo purpose",
+        "EMPTY.md": "",
+        "MISSING.md": None,
+    })
+
+    files, had_fetch_error = repo_context._load_repo_context_files(
+        provider, ["AGENTS.md", "EMPTY.md", "MISSING.md", " "]
+    )
+
+    assert files == {"AGENTS.md": "Repo purpose"}
+    assert had_fetch_error is False
+    assert provider.requested_paths == ["AGENTS.md", "EMPTY.md", "MISSING.md"]
+
+
+def test_load_repo_context_files_reports_fetch_errors():
+    provider = FakeProvider({})
+    provider.get_repo_file_content = Mock(side_effect=Exception("temporary outage"))
+
+    files, had_fetch_error = repo_context._load_repo_context_files(provider, ["AGENTS.md"])
+
+    assert files == {}
+    assert had_fetch_error is True
+
+
 def test_build_repo_context_process_cache_invalidates_when_config_changes(repo_context_settings):
     repo_context_settings.set("CONFIG.REPO_CONTEXT_FILES", ["AGENTS.md"])
     repo_context_settings.set("CONFIG.REPO_CONTEXT_MAX_LINES", 500)