diff --git a/.jules/bolt.md b/.jules/bolt.md index 5047f271..20bdfb8a 100644 --- a/.jules/bolt.md +++ b/.jules/bolt.md @@ -34,3 +34,6 @@ ## 2024-07-11 - Iterative JSON Extraction vs Recursion **Learning:** In `scripts/ci/opencode_review_normalize_output.py`, deeply recursive nested data processing for extracting JSON dictionaries (such as with LLM outputs where structure is arbitrary) using a recursive `extract_dicts` function causes Python to hit max recursion depth (`RecursionError`) on sufficiently deep structures, and increases runtime overhead due to call stack memory allocations. **Action:** When extracting data from arbitrary deeply nested dictionaries and lists in Python, use an iterative stack-based approach with `stack.extend()` or `stack.extend(reversed(...))` instead of recursive functions to eliminate recursion depth limitations and reduce overhead. +## 2024-07-25 - Pre-calculate and cache environmental file reads +**Learning:** The `current_changed_files` function in `scripts/ci/opencode_review_normalize_output.py` was being called multiple times per item during JSON structure normalization, leading to redundant I/O reads of `OPENCODE_CHANGED_FILES_FILE`. +**Action:** Use `@functools.lru_cache(maxsize=1)` and return an immutable `frozenset` when repeatedly reading static contextual files within a script's execution lifecycle. diff --git a/scripts/ci/opencode_review_normalize_output.py b/scripts/ci/opencode_review_normalize_output.py index cd46d266..91e99ed7 100755 --- a/scripts/ci/opencode_review_normalize_output.py +++ b/scripts/ci/opencode_review_normalize_output.py @@ -7,6 +7,7 @@ import os import re import sys +from functools import lru_cache from pathlib import Path from typing import Any @@ -307,21 +308,22 @@ def mentions_changed_file_evidence(reason: str, summary: str) -> bool: return bool(CHANGED_FILE_EVIDENCE_PATTERN.search(f"{reason}\n{summary}")) -def current_changed_files() -> set[str]: +@lru_cache(maxsize=1) +def current_changed_files() -> frozenset[str]: """Return the exact current-head changed files when the workflow provides them.""" changed_files_path = os.environ.get("OPENCODE_CHANGED_FILES_FILE") if not changed_files_path: - return set() + return frozenset() try: - return { + return frozenset( line.strip() for line in Path(changed_files_path) .read_text(encoding="utf-8") .splitlines() if line.strip() - } + ) except OSError: - return set() + return frozenset() def changed_file_is_source_like(path: str) -> bool: diff --git a/tests/test_opencode_review_normalize_output.py b/tests/test_opencode_review_normalize_output.py index fc317101..bc77c3a4 100644 --- a/tests/test_opencode_review_normalize_output.py +++ b/tests/test_opencode_review_normalize_output.py @@ -1,8 +1,15 @@ import json +import pytest + from scripts.ci import opencode_review_normalize_output as norm +@pytest.fixture(autouse=True) +def clear_caches(): + norm.current_changed_files.cache_clear() + + FULL_SUMMARY = """\ Approval sufficiency: affirmative evidence supported approval beyond the absence of blockers. Verification posture: CodeGraph inspected scripts/ci/example.py on the current head. @@ -92,7 +99,8 @@ def test_changed_file_and_verification_posture_detection(): def test_actual_changed_file_detection_prefers_current_head_file_list(tmp_path, monkeypatch): monkeypatch.delenv("OPENCODE_CHANGED_FILES_FILE", raising=False) - assert norm.current_changed_files() == set() + norm.current_changed_files.cache_clear() + assert norm.current_changed_files() == frozenset() assert norm.mentions_actual_changed_file("scripts/ci/example.py", "") changed_files = tmp_path / "changed-files.txt" @@ -107,8 +115,10 @@ def test_actual_changed_file_detection_prefers_current_head_file_list(tmp_path, encoding="utf-8", ) monkeypatch.setenv("OPENCODE_CHANGED_FILES_FILE", str(changed_files)) + norm.current_changed_files.cache_clear() monkeypatch.delenv("OPENCODE_CHANGED_FILES_FILE", raising=False) + norm.current_changed_files.cache_clear() assert norm.mentions_actual_changed_file("No executable changes here", "no changed files") assert norm.mentions_verification_posture("No executable changes here", "no changed files") assert norm.mentions_full_coverage("No executable changes here", "no changed files") @@ -119,12 +129,13 @@ def test_actual_changed_file_detection_prefers_current_head_file_list(tmp_path, assert norm.mentions_verification_posture("No UI codebase changes", "No UI codebase changes") assert norm.mentions_full_coverage("No UI codebase changes", "No UI codebase changes") monkeypatch.setenv("OPENCODE_CHANGED_FILES_FILE", str(changed_files)) + norm.current_changed_files.cache_clear() - assert norm.current_changed_files() == { + assert norm.current_changed_files() == frozenset({ ".github/workflows/opencode-review.yml", "scripts/ci/opencode_review_normalize_output.py", - } + }) assert norm.mentions_actual_changed_file( "Reviewed .github/workflows/opencode-review.yml.", "", @@ -139,7 +150,8 @@ def test_actual_changed_file_detection_prefers_current_head_file_list(tmp_path, ) monkeypatch.setenv("OPENCODE_CHANGED_FILES_FILE", str(tmp_path / "missing.txt")) - assert norm.current_changed_files() == set() + norm.current_changed_files.cache_clear() + assert norm.current_changed_files() == frozenset() assert norm.mentions_actual_changed_file("scripts/ci/example.py", "") @@ -149,6 +161,7 @@ def test_preferred_review_language_handles_unreadable_and_unknown_evidence(tmp_p "## Review language evidence\nPreferred review language: `Spanish`\n", encoding="utf-8", ) + norm.current_changed_files.cache_clear() monkeypatch.setenv("OPENCODE_APPROVAL_REPAIR_EVIDENCE_FILE", str(evidence)) assert norm.preferred_review_language() is None @@ -169,6 +182,7 @@ def test_changed_file_kind_contradictions_are_rejected(tmp_path, monkeypatch): encoding="utf-8", ) monkeypatch.setenv("OPENCODE_CHANGED_FILES_FILE", str(changed_files)) + norm.current_changed_files.cache_clear() false_summary = ( FULL_SUMMARY.replace("scripts/ci/example.py", ".github/workflows/opencode-review.yml") @@ -227,6 +241,7 @@ def test_changed_file_kind_contradictions_are_rejected(tmp_path, monkeypatch): ) monkeypatch.delenv("OPENCODE_CHANGED_FILES_FILE") + norm.current_changed_files.cache_clear() assert not norm.contradicts_changed_file_kinds(approval["reason"], approval["summary"]) @@ -243,6 +258,7 @@ def test_material_changed_file_scope_rejects_trivial_string_approval(tmp_path, m encoding="utf-8", ) monkeypatch.setenv("OPENCODE_CHANGED_FILES_FILE", str(changed_files)) + norm.current_changed_files.cache_clear() summary = ( "Approval sufficiency: The change is a simple typo fix in a string with no functional impact. " @@ -276,6 +292,7 @@ def test_material_changed_file_scope_rejects_trivial_string_approval(tmp_path, m assert norm.check_structural_approval(path) == 4 changed_files.write_text("README.md\n", encoding="utf-8") + norm.current_changed_files.cache_clear() assert not norm.contradicts_material_changed_file_scope( approval["reason"], approval["summary"], @@ -295,6 +312,7 @@ def test_material_changed_file_scope_rejects_false_documentation_typo_reason(tmp encoding="utf-8", ) monkeypatch.setenv("OPENCODE_CHANGED_FILES_FILE", str(changed_files)) + norm.current_changed_files.cache_clear() approval = control( reason="Typo fix in documentation string", @@ -400,10 +418,12 @@ def test_check_structural_approval_rejects_invalid_or_unsafe_approvals(tmp_path, changed_files = tmp_path / "changed-files.txt" changed_files.write_text("tests/actual_changed_file.py\n", encoding="utf-8") monkeypatch.setenv("OPENCODE_CHANGED_FILES_FILE", str(changed_files)) + norm.current_changed_files.cache_clear() wrong_file = tmp_path / "wrong-file.json" wrong_file.write_text(json.dumps(control()), encoding="utf-8") assert norm.check_structural_approval(wrong_file) == 4 monkeypatch.delenv("OPENCODE_CHANGED_FILES_FILE") + norm.current_changed_files.cache_clear() request_changes = tmp_path / "request.json" request_changes.write_text(json.dumps(control(result="REQUEST_CHANGES")), encoding="utf-8") @@ -526,6 +546,7 @@ def test_valid_control_repairs_approval_summary_from_bounded_evidence(tmp_path, """, encoding="utf-8", ) + norm.current_changed_files.cache_clear() monkeypatch.setenv("OPENCODE_APPROVAL_REPAIR_EVIDENCE_FILE", str(evidence)) repaired = norm.valid_control( @@ -557,6 +578,7 @@ def test_valid_control_repairs_summary_from_invalid_utf8_evidence(tmp_path, monk b"## Changed files\n\n" b"M\tscripts/ci/opencode_review_normalize_output.py\n" ) + norm.current_changed_files.cache_clear() monkeypatch.setenv("OPENCODE_APPROVAL_REPAIR_EVIDENCE_FILE", str(evidence)) repaired = norm.valid_control( @@ -604,11 +626,13 @@ def test_valid_control_repairs_fragile_approval_reason_from_bounded_evidence(tmp """, encoding="utf-8", ) + norm.current_changed_files.cache_clear() monkeypatch.setenv("OPENCODE_EVIDENCE_FILE", str(evidence)) monkeypatch.delenv("OPENCODE_APPROVAL_REPAIR_EVIDENCE_FILE", raising=False) changed_files = tmp_path / "changed-files.txt" changed_files.write_text(".github/workflows/r.yml\n", encoding="utf-8") monkeypatch.setenv("OPENCODE_CHANGED_FILES_FILE", str(changed_files)) + norm.current_changed_files.cache_clear() repaired = norm.valid_control( control( @@ -665,6 +689,7 @@ def test_valid_control_repair_overrides_earlier_invalid_coverage_labels(tmp_path """, encoding="utf-8", ) + norm.current_changed_files.cache_clear() monkeypatch.setenv("OPENCODE_APPROVAL_REPAIR_EVIDENCE_FILE", str(evidence)) repaired = norm.valid_control( @@ -731,8 +756,10 @@ def test_valid_control_repair_drops_contradictory_changed_file_kind_claims(tmp_p "apps/desktop/src/App.tsx\napps/desktop/src/App.test.tsx\n", encoding="utf-8", ) + norm.current_changed_files.cache_clear() monkeypatch.setenv("OPENCODE_APPROVAL_REPAIR_EVIDENCE_FILE", str(evidence)) monkeypatch.setenv("OPENCODE_CHANGED_FILES_FILE", str(changed_files)) + norm.current_changed_files.cache_clear() repaired = norm.valid_control( control( @@ -799,8 +826,10 @@ def test_valid_control_repair_drops_material_trivialization(tmp_path, monkeypatc ".github/workflows/strix.yml\nscripts/ci/test_strix_quick_gate.sh\n", encoding="utf-8", ) + norm.current_changed_files.cache_clear() monkeypatch.setenv("OPENCODE_APPROVAL_REPAIR_EVIDENCE_FILE", str(evidence)) monkeypatch.setenv("OPENCODE_CHANGED_FILES_FILE", str(changed_files)) + norm.current_changed_files.cache_clear() repaired = norm.valid_control( control( @@ -845,6 +874,7 @@ def test_valid_control_does_not_repair_unsafe_or_unproven_approval(tmp_path, mon """, encoding="utf-8", ) + norm.current_changed_files.cache_clear() monkeypatch.setenv("OPENCODE_APPROVAL_REPAIR_EVIDENCE_FILE", str(evidence)) kwargs = { "expected_head_sha": "head", @@ -949,6 +979,7 @@ def test_approval_repair_evidence_helpers_cover_edge_cases(tmp_path, monkeypatch evidence = tmp_path / "bounded-review-evidence.md" evidence.write_text("placeholder", encoding="utf-8") + norm.current_changed_files.cache_clear() monkeypatch.setenv("OPENCODE_APPROVAL_REPAIR_EVIDENCE_FILE", str(evidence)) original_read_text = norm.Path.read_text @@ -978,6 +1009,7 @@ def test_approval_language_contract_runs_after_evidence_repair(tmp_path, monkeyp """, encoding="utf-8", ) + norm.current_changed_files.cache_clear() monkeypatch.setenv("OPENCODE_APPROVAL_REPAIR_EVIDENCE_FILE", str(evidence)) reviewed = norm.valid_control( @@ -1017,6 +1049,7 @@ def test_request_changes_still_enforces_korean_language_contract(tmp_path, monke """, encoding="utf-8", ) + norm.current_changed_files.cache_clear() monkeypatch.setenv("OPENCODE_APPROVAL_REPAIR_EVIDENCE_FILE", str(evidence)) assert ( @@ -1151,6 +1184,7 @@ def test_review_language_contract_rejects_english_only_korean_pr(tmp_path, monke "## Review language evidence\n\n- Preferred review language: `Korean`\n", encoding="utf-8", ) + norm.current_changed_files.cache_clear() monkeypatch.setenv("OPENCODE_EVIDENCE_FILE", str(evidence)) assert norm.valid_control(