From 9f8aa54e2bbfb273557bea7f28f652fcd2b1345d Mon Sep 17 00:00:00 2001 From: gilesb Date: Mon, 12 Jan 2026 12:14:59 +0000 Subject: [PATCH] Fix duplicate get_by_cid method shadowing recipe lookup Bug: Two get_by_cid methods existed in L1CacheManager. The second definition shadowed the first, breaking recipe lookup because the comprehensive method (using find_by_cid) was hidden. - Remove duplicate get_by_cid method (lines 470-494) - Add regression test to ensure only one get_by_cid exists - Add tests for template variables and recipe visibility Co-Authored-By: Claude Opus 4.5 --- cache_manager.py | 26 -------- tests/test_recipe_visibility.py | 112 ++++++++++++++++++++++++++++++++ tests/test_run_artifacts.py | 29 ++++++++- 3 files changed, 140 insertions(+), 27 deletions(-) create mode 100644 tests/test_recipe_visibility.py diff --git a/cache_manager.py b/cache_manager.py index f42d039..2ec95ef 100644 --- a/cache_manager.py +++ b/cache_manager.py @@ -467,32 +467,6 @@ class L1CacheManager: return None - def get_by_cid(self, ipfs_cid: str) -> Optional[Path]: - """Get cached file path by IPFS CID. Fetches from IPFS if not in local cache.""" - - # Check if we have this CID cached locally (indexed by CID) - cached_path = self.legacy_dir / ipfs_cid - if cached_path.exists() and cached_path.is_file(): - return cached_path - - # Check cache directory structure - cid_cache_dir = self.cache_dir / ipfs_cid - if cid_cache_dir.exists() and cid_cache_dir.is_dir(): - # Look for output file - for f in cid_cache_dir.iterdir(): - if f.is_file() and not f.name.endswith('.json'): - return f - - # Fetch from IPFS - logger.info(f"Fetching from IPFS: {ipfs_cid[:16]}...") - recovery_path = self.legacy_dir / ipfs_cid - recovery_path.parent.mkdir(parents=True, exist_ok=True) - if ipfs_client.get_file(ipfs_cid, recovery_path): - logger.info(f"Fetched from IPFS: {recovery_path}") - return recovery_path - - return None - def has_content(self, cid: str) -> bool: """Check if content exists in cache.""" return self.get_by_cid(cid) is not None diff --git a/tests/test_recipe_visibility.py b/tests/test_recipe_visibility.py new file mode 100644 index 0000000..7fea339 --- /dev/null +++ b/tests/test_recipe_visibility.py @@ -0,0 +1,112 @@ +""" +Tests for recipe visibility in web UI. + +Bug found 2026-01-12: Recipes not showing in list even after upload. +""" + +import pytest +from pathlib import Path + + +class TestRecipeListingFlow: + """Tests for recipe listing data flow.""" + + def test_cache_manager_has_list_by_type(self) -> None: + """L1CacheManager should have list_by_type method.""" + # Read cache_manager.py and verify list_by_type exists + path = Path('/home/giles/art/art-celery/cache_manager.py') + content = path.read_text() + + assert 'def list_by_type' in content, \ + "L1CacheManager should have list_by_type method" + + def test_list_by_type_returns_cid(self) -> None: + """list_by_type should return entry.cid values.""" + path = Path('/home/giles/art/art-celery/cache_manager.py') + content = path.read_text() + + # Find list_by_type function and verify it appends entry.cid + assert 'hashes.append(entry.cid)' in content, \ + "list_by_type should append entry.cid to results" + + def test_recipe_service_uses_cache_list_by_type(self) -> None: + """Recipe service should use cache.list_by_type('recipe').""" + path = Path('/home/giles/art/art-celery/app/services/recipe_service.py') + content = path.read_text() + + assert "list_by_type('recipe')" in content, \ + "Recipe service should call list_by_type with 'recipe'" + + def test_recipe_upload_uses_recipe_node_type(self) -> None: + """Recipe upload should store with node_type='recipe'.""" + path = Path('/home/giles/art/art-celery/app/services/recipe_service.py') + content = path.read_text() + + assert 'node_type="recipe"' in content, \ + "Recipe upload should use node_type='recipe'" + + def test_get_by_cid_uses_find_by_cid(self) -> None: + """get_by_cid should use cache.find_by_cid to locate entries.""" + path = Path('/home/giles/art/art-celery/cache_manager.py') + content = path.read_text() + + # Verify get_by_cid uses find_by_cid + assert 'find_by_cid(cid)' in content, \ + "get_by_cid should use find_by_cid to locate entries" + + def test_no_duplicate_get_by_cid_methods(self) -> None: + """ + Regression test: There should only be ONE get_by_cid method. + + Bug: Two get_by_cid methods existed, the second shadowed the first, + breaking recipe lookup because the comprehensive method was hidden. + """ + path = Path('/home/giles/art/art-celery/cache_manager.py') + content = path.read_text() + + # Count occurrences of 'def get_by_cid' + count = content.count('def get_by_cid') + assert count == 1, \ + f"Should have exactly 1 get_by_cid method, found {count}" + + +class TestRecipeFilterLogic: + """Tests for recipe filtering logic.""" + + def test_filter_allows_none_owner(self) -> None: + """Recipes with owner=None should be visible.""" + path = Path('/home/giles/art/art-celery/app/services/recipe_service.py') + content = path.read_text() + + assert 'owner is None' in content, \ + "Recipe filter should allow owner=None recipes" + + def test_filter_allows_matching_owner(self) -> None: + """Recipes with matching owner should be visible.""" + path = Path('/home/giles/art/art-celery/app/services/recipe_service.py') + content = path.read_text() + + assert 'owner == actor_id' in content, \ + "Recipe filter should allow recipes where owner matches actor_id" + + +class TestCacheEntryHasCid: + """Tests for cache entry cid field.""" + + def test_artdag_cache_entry_has_cid(self) -> None: + """artdag CacheEntry should have cid field.""" + from artdag import CacheEntry + import dataclasses + + fields = {f.name for f in dataclasses.fields(CacheEntry)} + assert 'cid' in fields, \ + "CacheEntry should have cid field" + + def test_artdag_cache_put_sets_cid(self) -> None: + """artdag Cache.put should set cid on entry.""" + from artdag import Cache + import inspect + + source = inspect.getsource(Cache.put) + assert 'cid=' in source, \ + "Cache.put should set cid on entry" diff --git a/tests/test_run_artifacts.py b/tests/test_run_artifacts.py index f028385..4591d95 100644 --- a/tests/test_run_artifacts.py +++ b/tests/test_run_artifacts.py @@ -1,5 +1,5 @@ """ -Tests for run artifacts data structure. +Tests for run artifacts data structure and template variables. Bug found 2026-01-12: runs/detail.html template expects artifact.cid but the route provides artifact.hash, causing UndefinedError. @@ -9,6 +9,33 @@ import pytest from pathlib import Path +class TestCacheNotFoundTemplate: + """Tests for cache/not_found.html template.""" + + def test_template_uses_cid_not_content_hash(self) -> None: + """ + Regression test: not_found.html must use 'cid' variable. + + Bug: Template used 'content_hash' but route passes 'cid'. + Fix: Changed template to use 'cid'. + """ + template_path = Path('/home/giles/art/art-celery/app/templates/cache/not_found.html') + content = template_path.read_text() + + assert 'cid' in content, "Template should use 'cid' variable" + assert 'content_hash' not in content, \ + "Template should not use 'content_hash' (route passes 'cid')" + + def test_route_passes_cid_to_template(self) -> None: + """Verify route passes 'cid' variable to not_found template.""" + router_path = Path('/home/giles/art/art-celery/app/routers/cache.py') + content = router_path.read_text() + + # Find the render call for not_found.html + assert 'cid=cid' in content, \ + "Route should pass cid=cid to not_found.html template" + + class TestArtifactDataStructure: """Tests for artifact dict keys matching template expectations."""