diff --git a/.jules/bolt.md b/.jules/bolt.md index a86b7aafd..144b0543d 100644 --- a/.jules/bolt.md +++ b/.jules/bolt.md @@ -43,3 +43,6 @@ ## 2026-07-09 - Avoid N+1 API blocking in SBOM aggregator **Learning:** The `collect_inventories` function in `scripts/ci/sbom_inventory_aggregator.py` was fetching SBOMs from the GitHub dependency graph synchronously for every repository in the organization. For large organizations (up to 500 repos), this N+1 network/CLI bottleneck significantly stalled the aggregation workflow. **Action:** Use `concurrent.futures.ThreadPoolExecutor` to fetch SBOMs concurrently when multiple repositories are provided, bounded by a `max_workers` limit (e.g., 10) to avoid overwhelming the CLI/API, while preserving the fast serial path for single-item inputs. +## 2026-07-25 - Avoid N+1 API blocking in Noema review context fetching +**Learning:** In `scripts/ci/noema_review_gate.py`, the `changed_file_context` function was fetching file contents from the GitHub API sequentially, causing an N+1 API bottleneck. +**Action:** Use `concurrent.futures.ThreadPoolExecutor` to fetch bounded file contents concurrently, bounded by an explicit conservative `max_workers` limit (no more than 6), while preserving the fast serial path for single-item inputs and ensuring deterministic output order. diff --git a/scripts/ci/noema_review_gate.py b/scripts/ci/noema_review_gate.py index 9317860e4..d37022b8e 100644 --- a/scripts/ci/noema_review_gate.py +++ b/scripts/ci/noema_review_gate.py @@ -5,6 +5,7 @@ import argparse import base64 +import concurrent.futures import ipaddress import json import os @@ -41,6 +42,7 @@ MAX_FILE_CONTEXT_CHARS = 4000 MAX_REVIEW_CONTEXT_CHARS = 24000 MAX_THREAD_BODY_CHARS = 1200 +MAX_CONTEXT_WORKERS = 6 # ⚡ Bolt: Pre-compiled regex patterns to avoid recompilation on every scrub_sensitive_data call. # Impact: Improves string processing performance in error reporting. @@ -342,17 +344,26 @@ def changed_file_context(repo: str, number: int, head_sha: str) -> str: if not paths: return "Changed file context unavailable: PR reported no changed files." sections: list[str] = [] - for path in paths[:MAX_CONTEXT_FILES]: + target_paths = paths[:MAX_CONTEXT_FILES] + + def process_path(path: str) -> str: + """Fetch and truncate one changed file for the bounded review context.""" try: content = fetch_head_file_content(repo, path, head_sha) except RuntimeError as exc: reason = scrub_sensitive_data(str(exc)) or "unknown error" - sections.append(f"### {path}\nUnavailable from head content API: {reason}") - continue + return f"### {path}\nUnavailable from head content API: {reason}" if not content: - sections.append(f"### {path}\nNo UTF-8 text content available from head content API.") - continue - sections.append(f"### {path}\n{truncate_text(content, MAX_FILE_CONTEXT_CHARS)}") + return f"### {path}\nNo UTF-8 text content available from head content API." + return f"### {path}\n{truncate_text(content, MAX_FILE_CONTEXT_CHARS)}" + + if len(target_paths) <= 1: + sections.extend(process_path(path) for path in target_paths) + else: + max_workers = min(MAX_CONTEXT_WORKERS, len(target_paths)) + with concurrent.futures.ThreadPoolExecutor(max_workers=max_workers) as executor: + sections.extend(executor.map(process_path, target_paths)) + if len(paths) > MAX_CONTEXT_FILES: sections.append(f"[{len(paths) - MAX_CONTEXT_FILES} changed files omitted from context budget]") return "\n\n".join(sections) diff --git a/tests/test_noema_review_gate.py b/tests/test_noema_review_gate.py index 408bb95b9..eb5e2d6be 100644 --- a/tests/test_noema_review_gate.py +++ b/tests/test_noema_review_gate.py @@ -309,6 +309,52 @@ def test_review_context_reports_omitted_files_and_missing_codegraph(monkeypatch, assert "1 changed files omitted from context budget" in context + paths = ["src/file_only.py"] + monkeypatch.setattr(noema, "fetch_changed_file_paths", lambda repo, number: paths) + context = noema.changed_file_context("owner/repo", 7, "head") + assert "src/file_only.py" in context + + +def test_changed_file_context_concurrency_and_ordering(monkeypatch): + import time + + paths = ["src/a.py", "src/b.py", "src/c.py", "src/empty.txt"] + monkeypatch.setattr(noema, "fetch_changed_file_paths", lambda repo, number: paths) + + completion_order = [] + + def fake_fetch_head_file_content(repo, path, head_sha): + if path == "src/a.py": + completion_order.append(path) + raise RuntimeError("API error") + elif path == "src/b.py": + time.sleep(0.1) + completion_order.append(path) + return "b content" + elif path == "src/c.py": + completion_order.append(path) + return "c content" + elif path == "src/empty.txt": + completion_order.append(path) + return "" + return "content" + + monkeypatch.setattr(noema, "fetch_head_file_content", fake_fetch_head_file_content) + + context = noema.changed_file_context("owner/repo", 7, "head") + + assert "### src/a.py\nUnavailable from head content API: API error" in context + assert "### src/b.py\nb content" in context + assert "### src/c.py\nc content" in context + assert "### src/empty.txt\nNo UTF-8 text content available" in context + + pos_a = context.find("### src/a.py") + pos_b = context.find("### src/b.py") + pos_c = context.find("### src/c.py") + + assert pos_a < pos_b < pos_c, "Output order does not match input order" + + class FakeResponse: """Small context-manager response for urllib monkeypatches.""" diff --git a/tests/test_noema_review_gate_concurrency_contract.py b/tests/test_noema_review_gate_concurrency_contract.py new file mode 100644 index 000000000..11712d7b3 --- /dev/null +++ b/tests/test_noema_review_gate_concurrency_contract.py @@ -0,0 +1,101 @@ +"""Focused contracts for changed-file review context concurrency.""" + +from collections.abc import Callable, Iterable, Iterator +from typing import Any + +from scripts.ci import noema_review_gate as noema + + +class RecordingExecutor: + """Synchronous executor double that records worker and map contracts.""" + + instances: list["RecordingExecutor"] = [] + + def __init__(self, *, max_workers: int) -> None: + """Record the configured worker limit.""" + self.max_workers = max_workers + self.map_inputs: list[tuple[str, ...]] = [] + self.instances.append(self) + + def __enter__(self) -> "RecordingExecutor": + """Return the executor double for context-manager use.""" + return self + + def __exit__(self, *args: object) -> bool: + """Propagate exceptions raised by the code under test.""" + return False + + def map( + self, + function: Callable[[str], str], + values: Iterable[str], + ) -> Iterator[str]: + """Record ordered inputs and evaluate them synchronously.""" + ordered_values = tuple(values) + self.map_inputs.append(ordered_values) + return map(function, ordered_values) + + +def install_recording_executor(monkeypatch: Any) -> None: + """Install a fresh recording executor double.""" + RecordingExecutor.instances.clear() + monkeypatch.setattr(noema.concurrent.futures, "ThreadPoolExecutor", RecordingExecutor) + + +def test_single_file_context_does_not_create_executor(monkeypatch: Any) -> None: + """Keep the one-file fast path strictly serial.""" + install_recording_executor(monkeypatch) + monkeypatch.setattr(noema, "fetch_changed_file_paths", lambda repo, number: ["src/only.py"]) + monkeypatch.setattr(noema, "fetch_head_file_content", lambda repo, path, head_sha: "only content") + + context = noema.changed_file_context("owner/repo", 7, "head") + + assert RecordingExecutor.instances == [] + assert context == "### src/only.py\nonly content" + + +def test_parallel_file_context_uses_bounded_map_and_scrubs_errors(monkeypatch: Any) -> None: + """Verify bounded parallel mapping, stable order, and error redaction.""" + install_recording_executor(monkeypatch) + paths = [f"src/file_{index}.py" for index in range(noema.MAX_CONTEXT_WORKERS + 2)] + sensitive_value = "".join(("github_", "pat_", "123456789")) + monkeypatch.setattr(noema, "fetch_changed_file_paths", lambda repo, number: paths) + + def fake_fetch_head_file_content(repo: str, path: str, head_sha: str) -> str: + """Return deterministic content while exercising error and empty paths.""" + if path == paths[0]: + raise RuntimeError(f"API error token {sensitive_value}") + if path == paths[-1]: + return "" + return f"content for {path}" + + monkeypatch.setattr(noema, "fetch_head_file_content", fake_fetch_head_file_content) + + context = noema.changed_file_context("owner/repo", 7, "head") + + assert len(RecordingExecutor.instances) == 1 + executor = RecordingExecutor.instances[0] + assert executor.max_workers == min(noema.MAX_CONTEXT_WORKERS, len(paths)) + assert executor.max_workers <= noema.MAX_CONTEXT_WORKERS + assert executor.max_workers <= len(paths) + assert executor.map_inputs == [tuple(paths)] + assert "Unavailable from head content API: API error token ***" in context + assert sensitive_value not in context + assert "No UTF-8 text content available from head content API." in context + + section_positions = [context.index(f"### {path}") for path in paths] + assert section_positions == sorted(section_positions) + + +def test_parallel_file_context_worker_count_tracks_small_batches(monkeypatch: Any) -> None: + """Limit worker creation to the number of files in a small batch.""" + install_recording_executor(monkeypatch) + paths = ["src/first.py", "src/second.py"] + monkeypatch.setattr(noema, "fetch_changed_file_paths", lambda repo, number: paths) + monkeypatch.setattr(noema, "fetch_head_file_content", lambda repo, path, head_sha: path) + + noema.changed_file_context("owner/repo", 7, "head") + + executor = RecordingExecutor.instances[0] + assert executor.max_workers == len(paths) + assert executor.map_inputs == [tuple(paths)]