avoid using unsuitable calculator #57

yannrichet-asnr · 2025-11-27T22:01:41Z

No description provided.

Copilot

Pull request overview

This PR introduces calculator discovery and model filtering functionality to avoid using unsuitable calculators. The implementation allows the system to automatically discover calculators from .fz/calculators/ directories and filter them based on model compatibility, replacing the previous behavior where calculators=None defaulted to a simple "sh://".

Key Changes

Adds wildcard calculator discovery from local and home .fz/calculators/ directories
Implements model-based filtering to only use calculators that support specific models
Changes default calculator behavior from "sh://" to automatic discovery when calculators=None

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.

File	Description
`fz/core.py`	Added three new functions (`_find_all_calculators`, `_calculator_supports_model`, `_filter_calculators_by_model`) to support discovery and filtering; modified `_resolve_calculators_arg` to handle wildcard discovery and `fzr` to pass model information for filtering
`tests/test_calculator_discovery.py`	New comprehensive test suite covering calculator discovery, model support checking, model filtering, and calculator argument resolution

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-27T22:07:19Z

tests/test_calculator_discovery.py

+        assert _calculator_supports_model(calc_data, "model1") == True
+        assert _calculator_supports_model(calc_data, "model2") == True


Using == True in assertions is not idiomatic Python and is unnecessary. The assert statement already evaluates the truthiness of the expression.

Consider simplifying to:

assert _calculator_supports_model(calc_data, "model1") assert _calculator_supports_model(calc_data, "model2")

Copilot · 2025-11-27T22:07:19Z

tests/test_calculator_discovery.py

+        assert _calculator_supports_model(calc_data, "model1") == True
+        assert _calculator_supports_model(calc_data, "model2") == True


Using == True in assertions is not idiomatic Python and is unnecessary. Consider simplifying to:

assert _calculator_supports_model(calc_data, "model1") assert _calculator_supports_model(calc_data, "model2")

Suggested change

assert _calculator_supports_model(calc_data, "model1") == True

assert _calculator_supports_model(calc_data, "model2") == True

assert _calculator_supports_model(calc_data, "model1")

assert _calculator_supports_model(calc_data, "model2")

Copilot · 2025-11-27T22:07:19Z

tests/test_calculator_discovery.py

+        assert isinstance(calculators, list)
+        assert len(calculators) >= 1


This assertion is too weak. It checks that the result is a list with at least 1 element, but doesn't verify that the explicit URI was actually preserved correctly.

Consider strengthening the assertion to:

assert calculators == ["sh://bash myscript.sh"]

This would ensure the URI is passed through unchanged, as stated in the test description.

Copilot · 2025-11-27T22:07:20Z

fz/core.py

+    search_dirs = [Path.cwd() / ".fz" / "calculators", Path.home() / ".fz" / "calculators"]
+    calculators = []
+
+    for calc_dir in search_dirs:
+        if not calc_dir.exists() or not calc_dir.is_dir():
+            continue
+
+        # Find all .json files in the calculator directory
+        for calc_file in calc_dir.glob("*.json"):
+            try:
+                with open(calc_file, 'r') as f:
+                    calc_data = json.load(f)
+
+                # Check if calculator supports the model (if model_name is provided)
+                if model_name and not _calculator_supports_model(calc_data, model_name):
+                    log_debug(f"Skipping calculator {calc_file.name}: does not support model '{model_name}'")
+                    continue
+
+                # Extract calculator specification
+                if "uri" in calc_data:
+                    # Calculator with URI specification
+                    uri = calc_data["uri"]
+
+                    # If models dict exists and model_name is provided, get model-specific command
+                    if "models" in calc_data and isinstance(calc_data["models"], dict) and model_name:
+                        if model_name in calc_data["models"]:
+                            # Use model-specific command from models dict
+                            model_command = calc_data["models"][model_name]
+                            # Append command to URI if it doesn't already contain it
+                            if not uri.endswith(model_command):
+                                uri = f"{uri.rstrip('/')}/{model_command}" if '://' in uri else model_command
+
+                    calculators.append(uri)
+                    log_debug(f"Found calculator: {calc_file.name} -> {uri}")
+                elif "command" in calc_data:
+                    # Simple calculator with command
+                    calculators.append(calc_data["command"])
+                    log_debug(f"Found calculator: {calc_file.name} -> {calc_data['command']}")
+                else:
+                    # Just add the whole dict as a calculator spec
+                    calculators.append(calc_data)
+                    log_debug(f"Found calculator: {calc_file.name} (dict spec)")
+
+            except (json.JSONDecodeError, IOError, KeyError) as e:
+                log_warning(f"Could not load calculator file {calc_file}: {e}")
+                continue
+
+    return calculators


Potential for duplicate calculators: the function searches both ./.fz/calculators/ and ~/.fz/calculators/ without deduplication. If the same calculator file exists in both locations (e.g., calc1.json in both directories), it will be added to the list twice, potentially leading to duplicate calculator executions.

Consider adding deduplication logic, either by:

Tracking seen calculator names and skipping duplicates

Using a set/dict to store unique calculators

Documenting that local calculators override home directory calculators

Example fix:

seen_names = set() for calc_dir in search_dirs: if not calc_dir.exists() or not calc_dir.is_dir(): continue for calc_file in calc_dir.glob("*.json"): if calc_file.stem in seen_names: continue # Skip duplicate seen_names.add(calc_file.stem) # ... rest of processing

Copilot · 2025-11-27T22:07:20Z

tests/test_calculator_discovery.py

+class TestCalculatorDiscovery:
+    """Tests for wildcard calculator discovery"""
+
+    def test_find_all_calculators_empty_directory(self, isolated_env, monkeypatch):
+        """Test finding calculators when .fz/calculators/ doesn't exist"""
+        with tempfile.TemporaryDirectory() as tmpdir:
+            monkeypatch.chdir(tmpdir)
+            calculators = _find_all_calculators()
+            assert calculators == []
+
+    def test_find_all_calculators_with_uri_spec(self, isolated_env, monkeypatch):
+        """Test finding calculators with URI specification"""
+        with tempfile.TemporaryDirectory() as tmpdir:
+            calc_dir = Path(tmpdir) / ".fz" / "calculators"
+            calc_dir.mkdir(parents=True)
+
+            # Create calculator with URI
+            calc_file = calc_dir / "local.json"
+            calc_data = {
+                "uri": "sh://bash calc.sh",
+                "description": "Local calculator"
+            }
+            with open(calc_file, 'w') as f:
+                json.dump(calc_data, f)
+
+            monkeypatch.chdir(tmpdir)
+            calculators = _find_all_calculators()
+
+            assert len(calculators) == 1
+            assert calculators[0] == "sh://bash calc.sh"
+
+    def test_find_all_calculators_with_command_spec(self, isolated_env, monkeypatch):
+        """Test finding calculators with command specification"""
+        with tempfile.TemporaryDirectory() as tmpdir:
+            calc_dir = Path(tmpdir) / ".fz" / "calculators"
+            calc_dir.mkdir(parents=True)
+
+            # Create calculator with command
+            calc_file = calc_dir / "simple.json"
+            calc_data = {
+                "command": "sh://bash run.sh",
+                "description": "Simple calculator"
+            }
+            with open(calc_file, 'w') as f:
+                json.dump(calc_data, f)
+
+            monkeypatch.chdir(tmpdir)
+            calculators = _find_all_calculators()
+
+            assert len(calculators) == 1
+            assert calculators[0] == "sh://bash run.sh"
+
+    def test_find_all_calculators_multiple_files(self, isolated_env, monkeypatch):
+        """Test finding multiple calculator files"""
+        with tempfile.TemporaryDirectory() as tmpdir:
+            calc_dir = Path(tmpdir) / ".fz" / "calculators"
+            calc_dir.mkdir(parents=True)
+
+            # Create multiple calculators
+            for i, name in enumerate(["calc1", "calc2", "calc3"]):
+                calc_file = calc_dir / f"{name}.json"
+                calc_data = {
+                    "uri": f"sh://bash {name}.sh",
+                    "description": f"Calculator {i+1}"
+                }
+                with open(calc_file, 'w') as f:
+                    json.dump(calc_data, f)
+
+            monkeypatch.chdir(tmpdir)
+            calculators = _find_all_calculators()
+
+            assert len(calculators) == 3
+            # Check all calculators are present (order not guaranteed)
+            assert "sh://bash calc1.sh" in calculators
+            assert "sh://bash calc2.sh" in calculators
+            assert "sh://bash calc3.sh" in calculators
+


Missing test coverage for error handling in calculator discovery. The implementation handles various error cases (JSONDecodeError, IOError, KeyError at line 322 in fz/core.py), but there are no tests verifying this behavior.

Consider adding tests for:

Calculator JSON file with invalid JSON syntax

Calculator JSON file that can't be read (permission error)

Calculator JSON file with unexpected structure (e.g., models field is neither dict nor list)

Empty or malformed JSON files

This would ensure the error handling works correctly and doesn't crash the discovery process.

Copilot · 2025-11-27T22:07:20Z

fz/core.py

+                            model_command = calc_data["models"][model_name]
+                            # Append command to URI if it doesn't already contain it
+                            if not uri.endswith(model_command):
+                                uri = f"{uri.rstrip('/')}/{model_command}" if '://' in uri else model_command


The URI construction has a bug. When uri = "sh://" and we call uri.rstrip('/'), it removes BOTH trailing slashes, resulting in "sh:". Then appending "/{model_command}" produces "sh:/bash model1.sh" (single slash after colon), which is an invalid URI format.

The correct format should be "sh://bash model1.sh" (double slash). Consider using a different approach, such as:

if uri.endswith("://"): uri = f"{uri}{model_command}" else: uri = f"{uri}/{model_command}"

Suggested change

uri = f"{uri.rstrip('/')}/{model_command}" if '://' in uri else model_command

if uri.endswith("://"):

uri = f"{uri}{model_command}"

else:

uri = f"{uri}/{model_command}"

Copilot · 2025-11-27T22:07:20Z

tests/test_calculator_discovery.py

+            calculators_model1 = _find_all_calculators(model_name="model1")
+            assert len(calculators_model1) == 2  # calc1 and calc3
+            # Check that model-specific command is included for calc1
+            assert any("model1.sh" in str(c) or "universal.sh" in str(c) for c in calculators_model1)


This assertion is too weak to verify correct URI construction. It only checks if "model1.sh" OR "universal.sh" exists in ANY of the calculators, not that the URI is properly formatted.

Consider adding a more specific assertion to verify the exact URI format, such as:

assert "sh://bash model1.sh" in calculators_model1 or any("sh://" in c and "model1.sh" in c for c in calculators_model1)

This would catch URI construction bugs like producing "sh:/bash model1.sh" (single slash) instead of the correct "sh://bash model1.sh" (double slash).

Copilot · 2025-11-27T22:07:20Z

tests/test_calculator_discovery.py

+            "uri": "sh://",
+            "description": "Universal calculator"
+        }
+        assert _calculator_supports_model(calc_data, "anymodel") == True


Using == True in assertions is not idiomatic Python and is unnecessary. The assert statement already evaluates the truthiness of the expression.

Consider simplifying to:

assert _calculator_supports_model(calc_data, "anymodel")

This is the pattern used in other test files in this codebase (e.g., test_bash_availability.py).

Suggested change

assert _calculator_supports_model(calc_data, "anymodel") == True

assert _calculator_supports_model(calc_data, "anymodel")

The calculator discovery tests were failing on Windows with PermissionError during cleanup because they used tempfile.TemporaryDirectory() with monkeypatch.chdir(), which locks the directory on Windows. Changes: - Replaced all tempfile.TemporaryDirectory() usage with isolated_env fixture - Each test now creates a subdirectory under isolated_env (tmp_path) - Removed import tempfile since it's no longer needed - All 16 calculator discovery tests now pass on both Linux and Windows The isolated_env fixture was already defined in the file and provides: - Mocked Path.home() to avoid finding real calculators - Proper cleanup without directory locking issues - Test isolation between test methods Tests affected: - test_find_all_calculators_with_uri_spec - test_find_all_calculators_with_command_spec - test_find_all_calculators_multiple_files - test_find_calculators_filtered_by_model - test_find_calculators_no_model_filter - test_resolve_calculators_none_defaults_to_wildcard - test_resolve_calculators_wildcard_explicit - test_resolve_calculators_wildcard_with_model_filter - test_resolve_calculators_fallback_to_default All 47 related tests pass: 16 calculator discovery + 28 shell path + 8 bash requirement 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>

Funz calculators use a special URI format where the model name is appended to the URI path: funz://:<udp_port>/<model> When using a calculator JSON file for Funz, the structure is: { "uri": "funz://:5555", "models": { "model_name": "model_name" } } The new test `test_funz_calculator_uri_construction` verifies: 1. Model appending: When model_name is specified, the URI is constructed as "funz://:5555/model_name" by appending the model to the base URI 2. Base URI return: When no model is specified, returns base URI "funz://:5555" without any model path appended 3. Model filtering: Only calculators that support the requested model are returned (based on the models dict keys) 4. Multiple calculators: Tests with two Funz servers on different ports to verify correct URI construction for each 5. Edge cases: - Models supported by both calculators - Models supported by only one calculator - Unsupported models (returns empty list) The test includes extensive comments explaining the Funz calculator pattern and the expected behavior for each test case. All 17 calculator discovery tests pass (16 existing + 1 new). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>

avoid using unsuitable calculator

d2ceb5f

Copilot AI review requested due to automatic review settings November 27, 2025 22:01

Copilot started reviewing on behalf of yannrichet-asnr November 27, 2025 22:02 View session

Copilot finished reviewing on behalf of yannrichet-asnr November 27, 2025 22:06

Copilot AI reviewed Nov 27, 2025

View reviewed changes

yannrichet and others added 3 commits November 28, 2025 00:06

release tests time limit

f998fda

yannrichet merged commit b908892 into main Nov 28, 2025
22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

avoid using unsuitable calculator #57

avoid using unsuitable calculator #57

Uh oh!

yannrichet-asnr commented Nov 27, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Nov 27, 2025

Uh oh!

Copilot AI Nov 27, 2025

Uh oh!

Copilot AI Nov 27, 2025

Uh oh!

Copilot AI Nov 27, 2025

Uh oh!

Copilot AI Nov 27, 2025

Uh oh!

Copilot AI Nov 27, 2025

Uh oh!

Copilot AI Nov 27, 2025

Uh oh!

Copilot AI Nov 27, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		assert _calculator_supports_model(calc_data, "model1") == True
		assert _calculator_supports_model(calc_data, "model2") == True

		assert isinstance(calculators, list)
		assert len(calculators) >= 1

-                                uri = f"{uri.rstrip('/')}/{model_command}" if '://' in uri else model_command
+                                if uri.endswith("://"):
+                                    uri = f"{uri}{model_command}"
+                                else:
+                                    uri = f"{uri}/{model_command}"

avoid using unsuitable calculator #57

avoid using unsuitable calculator #57

Uh oh!

Conversation

yannrichet-asnr commented Nov 27, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Key Changes

Reviewed changes

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants