Remove mock LLM provider and improve error handling for missing API keys #42
Conversation
- Remove LlmProvider::Mock enum variant and all references - Update warning message when API keys are missing to indicate LLM extraction is disabled - Improve error handling to properly return errors when API keys are missing - Fix map_or usage in language parsers to use more idiomatic pattern matching - Fix unused indoc imports in test modules - Update tests to use real LLM providers instead of the removed Mock variant Part of issue #41.
|
I'm getting to work. See my steps and track my cost here.
|
![mentatbot[bot]](https://avatars.githubusercontent.com/in/837875?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review of PR #42: Remove mock LLM provider and improve error handling for missing API keys
This PR looks good! It successfully removes the mock LLM provider while improving error handling for missing API keys. I appreciate the focus on proper error reporting rather than silently falling back to mock data.
Strengths:
- ✅ Clean removal of all references to the Mock provider
- ✅ Improved error handling with clear error messages for missing API keys
- ✅ Code quality improvements with more idiomatic Rust patterns
- ✅ Addition of centralized file type checking with
is_supported_source_file - ✅ All tests pass, verifying that the changes don't break existing functionality
- ✅ Good documentation updates in CLAUDE.md
The PR implements the changes as described in the PR description and addresses the issue mentioned in #41.
Approve with minor suggestions below
|
|
||
| if needs_api_key && config.api_key.is_empty() { | ||
| return Err(anyhow::anyhow!( | ||
| "API key is required for provider: {:?}", |
There was a problem hiding this comment.
Consider making the error message more user-friendly by including a hint about setting the environment variable.
| "API key is required for provider: {:?}", | |
| "API key is required for {}. Set the appropriate environment variable for your provider.", |
| /// List of supported source file extensions. | ||
| pub const SUPPORTED_EXTENSIONS: &[&str] = &["rs", "py", "js", "java"]; |
There was a problem hiding this comment.
This is a good addition to centralize the supported file types. I notice this list doesn't include all the extensions mentioned in the old code (like "jsx", "ts", "tsx", "c", "cpp", etc.). Was this intentional to streamline the supported languages?
| // Mock provider has been removed | ||
| assert!(LlmProvider::from_str("mock").is_err()); |
There was a problem hiding this comment.
Good test update to reflect the removal of the Mock provider.
|
I also noticed that the tests in Overall, this PR is a good improvement to the codebase's error handling and code quality. The changes are well-focused and the tests pass, which gives confidence in the implementation. |
Summary
Test plan
running 103 tests
test graph::entity::tests::test_domain_concept_entity ... ok
test graph::entity::tests::test_base_entity ... ok
test graph::entity::tests::test_entity_id ... ok
test graph::entity::tests::test_entity_type_conversion ... ok
test graph::entity::tests::test_file_path_accessor ... ok
test graph::entity::tests::test_function_entity ... ok
test graph::entity::tests::test_module_entity ... ok
test graph::entity::tests::test_variable_entity ... ok
test graph::entity::tests::test_type_entity ... ok
test graph::knowledge_graph::tests::test_add_entity ... ok
test graph::knowledge_graph::tests::test_add_bidirectional_relationship ... ok
test graph::knowledge_graph::tests::test_add_entity_duplicate ... ok
test graph::knowledge_graph::tests::test_add_relationship ... ok
test graph::knowledge_graph::tests::test_add_relationship_with_nonexistent_entity ... ok
test graph::knowledge_graph::tests::test_domain_concept_relationships ... ok
test graph::knowledge_graph::tests::test_domain_concepts ... ok
test graph::knowledge_graph::tests::test_find_paths ... ok
test graph::knowledge_graph::tests::test_find_paths_complex ... ok
test graph::knowledge_graph::tests::test_get_entities_by_type ... ok
test graph::knowledge_graph::tests::test_get_entities_with_multiple_filters ... ok
test graph::knowledge_graph::tests::test_new_knowledge_graph ... ok
test graph::relationship::tests::test_relationship_creation ... ok
test graph::relationship::tests::test_relationship_generate_id ... ok
test graph::relationship::tests::test_relationship_id_creation ... ok
test graph::relationship::tests::test_relationship_metadata ... ok
test graph::relationship::tests::test_relationship_store ... ok
test graph::relationship::tests::test_relationship_store_empty ... ok
test graph::relationship::tests::test_relationship_store_multiple_relationships ... ok
test graph::relationship::tests::test_relationship_types ... ok
test graph::relationship::tests::test_relationship_weight ... ok
test mcp_server::router::tests::test_debug_graph_tool ... ok
test mcp_server::router::tests::test_explain_architecture_tool ... ok
test mcp_server::router::tests::test_explore_relationships_tool ... ok
test mcp_server::router::tests::test_find_relevant_files_tool ... ok
test mcp_server::router::tests::test_get_entity_tool ... ok
test mcp_server::router::tests::test_search_code_tool ... ok
test mcp_server::router::tests::test_tool_error_handling ... ok
test parser::language_support::java::tests::test_java_basic_parsing ... ok
test parser::language_support::java::tests::test_java_field_extraction ... ok
test parser::language_support::java::tests::test_java_generic_parameters ... ok
test parser::language_support::java::tests::test_java_parser_boundary_conditions ... ok
test parser::language_support::java::tests::test_java_parser_empty_content ... ok
test parser::language_support::java::tests::test_java_parser_invalid_content ... ok
test parser::language_support::javascript::tests::test_javascript_parser_boundary_conditions ... ok
test parser::language_support::javascript::tests::test_javascript_parser_empty_content ... ok
test parser::language_support::javascript::tests::test_javascript_parser_invalid_content ... ok
test parser::language_support::javascript::tests::test_parse_arrow_function ... ok
test parser::language_support::javascript::tests::test_parse_calls ... ok
test parser::language_support::javascript::tests::test_parse_class_method ... ok
test parser::language_support::javascript::tests::test_parse_function_declaration ... ok
test parser::language_support::javascript::tests::test_typescript_generic_parameters ... ok
test parser::language_support::python::tests::test_python_class_field_extraction ... ok
test parser::language_support::python::tests::test_python_function_parameter_extraction ... ok
test parser::language_support::python::tests::test_python_generic_parameters ... ok
test parser::language_support::python::tests::test_python_nested_entities ... ok
test parser::language_support::python::tests::test_python_parser_boundary_conditions ... ok
test parser::language_support::python::tests::test_python_parser_empty_content ... ok
test graph::knowledge_graph::tests::test_large_graph ... ok
test db::tests::test_database_initialization ... ok
test parser::language_support::rust::tests::test_impl_generic_parameters ... ignored
test parser::language_support::rust::tests::test_generic_parameters ... ok
test parser::language_support::python::tests::test_python_parser_invalid_content ... ok
test parser::language_support::rust::tests::test_parse_calls ... ok
test parser::language_support::rust::tests::test_parse_method ... ok
test parser::language_support::rust::tests::test_parse_simple_function ... ok
test parser::language_support::rust::tests::test_nested_entities ... ok
test parser::language_support::rust::tests::test_parse_struct_fields ... ok
test parser::language_support::rust::tests::test_rust_parser_empty_content ... ok
test prompt::domain_extraction::tests::test_build_domain_extraction_prompt ... ok
test prompt::domain_extraction::tests::test_chunk_file ... ok
test db::tests::test_multiple_entities_and_relationships ... ok
test parser::language_support::rust::tests::test_rust_parser_boundary_conditions ... ok
test db::tests::test_save_and_load_entity ... ok
test db::tests::test_transaction_integrity ... ok
test prompt::llm_integration::tests::test_get_endpoint_url ... ok
test prompt::llm_integration::tests::test_get_request_headers ... ok
test prompt::domain_extraction::tests::test_deduplicate_entities ... ok
test prompt::llm_integration::tests::test_llm_config_default ... ok
test prompt::llm_integration::tests::test_prepare_request_body ... ok
test prompt::llm_integration::tests::test_llm_provider_parsing ... ok
test query::executor::tests::test_execute_select_all_functions ... ok
test query::executor::tests::test_execute_traversal_query ... ok
test query::formatter::tests::test_empty_results ... ok
test query::executor::tests::test_execute_traversal_with_condition ... ok
test db::tests::test_save_and_load_relationship ... ok
test parser::language_support::rust::tests::test_rust_parser_invalid_content ... ok
test query::formatter::tests::test_format_text ... ok
test query::formatter::tests::test_format_csv ... ok
test query::formatter::tests::test_format_tree ... ok
test query::formatter::tests::test_format_json ... ok
test query::nl_translator::tests::test_build_translation_prompt ... ok
test query::nl_translator::tests::test_translate_simple_query ... ignored
test query::parser::tests::test_has_attribute_condition ... ok
test query::parser::tests::test_invalid_query ... ok
test query::parser::tests::test_complex_condition ... ok
test query::parser::tests::test_parse_select_query_simple ... ok
test query::nl_translator::tests::test_extract_query_and_confidence ... ok
test query::parser::tests::test_parse_traversal_query ... ok
test query::parser::tests::test_parse_traversal_query_with_condition ... ok
test query::parser::tests::test_parse_select_query_with_condition ... ok
test query::executor::tests::test_execute_select_with_condition ... ok
test query::executor::tests::test_execute_complex_condition ... ok
test db::tests::test_concurrent_saves ... ok
test result: ok. 101 passed; 0 failed; 2 ignored; 0 measured; 0 filtered out; finished in 0.13s
running 103 tests
test graph::entity::tests::test_domain_concept_entity ... ok
test graph::entity::tests::test_entity_id ... ok
test graph::entity::tests::test_base_entity ... ok
test graph::entity::tests::test_entity_type_conversion ... ok
test graph::entity::tests::test_file_path_accessor ... ok
test graph::entity::tests::test_function_entity ... ok
test graph::entity::tests::test_module_entity ... ok
test graph::entity::tests::test_type_entity ... ok
test graph::entity::tests::test_variable_entity ... ok
test graph::knowledge_graph::tests::test_add_bidirectional_relationship ... ok
test graph::knowledge_graph::tests::test_add_entity ... ok
test graph::knowledge_graph::tests::test_add_entity_duplicate ... ok
test graph::knowledge_graph::tests::test_add_relationship ... ok
test graph::knowledge_graph::tests::test_add_relationship_with_nonexistent_entity ... ok
test graph::knowledge_graph::tests::test_domain_concept_relationships ... ok
test graph::knowledge_graph::tests::test_domain_concepts ... ok
test graph::knowledge_graph::tests::test_find_paths ... ok
test graph::knowledge_graph::tests::test_get_entities_by_type ... ok
test graph::knowledge_graph::tests::test_find_paths_complex ... ok
test graph::knowledge_graph::tests::test_get_entities_with_multiple_filters ... ok
test graph::knowledge_graph::tests::test_new_knowledge_graph ... ok
test graph::relationship::tests::test_relationship_creation ... ok
test graph::relationship::tests::test_relationship_generate_id ... ok
test graph::relationship::tests::test_relationship_id_creation ... ok
test graph::relationship::tests::test_relationship_metadata ... ok
test graph::relationship::tests::test_relationship_store ... ok
test graph::relationship::tests::test_relationship_store_empty ... ok
test graph::relationship::tests::test_relationship_store_multiple_relationships ... ok
test graph::relationship::tests::test_relationship_types ... ok
test graph::relationship::tests::test_relationship_weight ... ok
test mcp_server::router::tests::test_debug_graph_tool ... ok
test mcp_server::router::tests::test_explain_architecture_tool ... ok
test mcp_server::router::tests::test_explore_relationships_tool ... ok
test mcp_server::router::tests::test_find_relevant_files_tool ... ok
test mcp_server::router::tests::test_get_entity_tool ... ok
test mcp_server::router::tests::test_search_code_tool ... ok
test mcp_server::router::tests::test_tool_error_handling ... ok
test parser::language_support::java::tests::test_java_basic_parsing ... ok
test parser::language_support::java::tests::test_java_field_extraction ... ok
test parser::language_support::java::tests::test_java_generic_parameters ... ok
test parser::language_support::java::tests::test_java_parser_boundary_conditions ... ok
test parser::language_support::java::tests::test_java_parser_empty_content ... ok
test parser::language_support::java::tests::test_java_parser_invalid_content ... ok
test db::tests::test_database_initialization ... ok
test parser::language_support::javascript::tests::test_javascript_parser_empty_content ... ok
test parser::language_support::javascript::tests::test_javascript_parser_boundary_conditions ... ok
test db::tests::test_save_and_load_entity ... ok
test db::tests::test_save_and_load_relationship ... ok
test parser::language_support::javascript::tests::test_parse_arrow_function ... ok
test parser::language_support::javascript::tests::test_javascript_parser_invalid_content ... ok
test parser::language_support::javascript::tests::test_parse_function_declaration ... ok
test db::tests::test_transaction_integrity ... ok
test parser::language_support::javascript::tests::test_parse_class_method ... ok
test parser::language_support::javascript::tests::test_typescript_generic_parameters ... ok
test parser::language_support::javascript::tests::test_parse_calls ... ok
test parser::language_support::python::tests::test_python_class_field_extraction ... ok
test parser::language_support::python::tests::test_python_parser_empty_content ... ok
test parser::language_support::python::tests::test_python_function_parameter_extraction ... ok
test parser::language_support::python::tests::test_python_generic_parameters ... ok
test parser::language_support::rust::tests::test_impl_generic_parameters ... ignored
test parser::language_support::python::tests::test_python_parser_boundary_conditions ... ok
test parser::language_support::python::tests::test_python_nested_entities ... ok
test db::tests::test_multiple_entities_and_relationships ... ok
test parser::language_support::rust::tests::test_parse_method ... ok
test parser::language_support::rust::tests::test_parse_calls ... ok
test parser::language_support::rust::tests::test_generic_parameters ... ok
test parser::language_support::rust::tests::test_parse_simple_function ... ok
test parser::language_support::rust::tests::test_nested_entities ... ok
test parser::language_support::python::tests::test_python_parser_invalid_content ... ok
test parser::language_support::rust::tests::test_rust_parser_empty_content ... ok
test prompt::domain_extraction::tests::test_chunk_file ... ok
test parser::language_support::rust::tests::test_parse_struct_fields ... ok
test parser::language_support::rust::tests::test_rust_parser_boundary_conditions ... ok
test prompt::domain_extraction::tests::test_deduplicate_entities ... ok
test prompt::llm_integration::tests::test_llm_config_default ... ok
test prompt::domain_extraction::tests::test_build_domain_extraction_prompt ... ok
test prompt::llm_integration::tests::test_get_endpoint_url ... ok
test prompt::llm_integration::tests::test_get_request_headers ... ok
test prompt::llm_integration::tests::test_prepare_request_body ... ok
test prompt::llm_integration::tests::test_llm_provider_parsing ... ok
test parser::language_support::rust::tests::test_rust_parser_invalid_content ... ok
test graph::knowledge_graph::tests::test_large_graph ... ok
test query::executor::tests::test_execute_select_all_functions ... ok
test query::formatter::tests::test_empty_results ... ok
test query::formatter::tests::test_format_csv ... ok
test query::executor::tests::test_execute_traversal_query ... ok
test query::executor::tests::test_execute_traversal_with_condition ... ok
test query::formatter::tests::test_format_json ... ok
test query::formatter::tests::test_format_text ... ok
test query::formatter::tests::test_format_tree ... ok
test query::nl_translator::tests::test_translate_simple_query ... ignored
test query::nl_translator::tests::test_build_translation_prompt ... ok
test query::parser::tests::test_has_attribute_condition ... ok
test query::parser::tests::test_complex_condition ... ok
test query::parser::tests::test_parse_select_query_simple ... ok
test query::parser::tests::test_invalid_query ... ok
test query::nl_translator::tests::test_extract_query_and_confidence ... ok
test query::parser::tests::test_parse_traversal_query ... ok
test query::parser::tests::test_parse_select_query_with_condition ... ok
test query::parser::tests::test_parse_traversal_query_with_condition ... ok
test query::executor::tests::test_execute_select_with_condition ... ok
test query::executor::tests::test_execute_complex_condition ... ok
test db::tests::test_concurrent_saves ... ok
test result: ok. 101 passed; 0 failed; 2 ignored; 0 measured; 0 filtered out; finished in 0.13s
running 0 tests
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s to ensure all tests pass
2. Test domain extraction with and without API keys to verify proper error handling
3. Verify natural language queries work with an API key
Fixes #41 (partially - addresses the error handling, full domain extraction improvements to be done separately)
🤖 Generated with Claude Code
Co-Authored-By: Claude [email protected]