Refactor: core.types and lazy import handling; ensure common usage of core types across codebase; house cleaning #203
Conversation
Reviewer's GuideRefactors core typing infrastructure (splitting models/dataclasses/env/utils), centralizes lazy import/getattr helpers, standardizes enum string usage via .variable, enhances schema metadata for NamedTuple/dataclass-based types, fixes and updates tests and build tooling, and removes misplaced/generated analysis data files. Sequence diagram for lazy getattr resolution using create_lazy_getattrsequenceDiagram
participant UserCode
participant ImportSystem
participant PackageCoreTypes as core.types.__init__
participant LazyGetter as create_lazy_getattr
participant DynamicGetattr as __getattr__
participant ImportBuiltin as __import__
participant Submodule as core.types.dataclasses
Note over PackageCoreTypes,LazyGetter: Package import time
UserCode->>ImportSystem: from codeweaver.core.types import DataclassSerializationMixin
ImportSystem->>PackageCoreTypes: evaluate module
PackageCoreTypes->>LazyGetter: create_lazy_getattr(_dynamic_imports, globals(), __name__)
LazyGetter-->>PackageCoreTypes: configured __getattr__
PackageCoreTypes->>PackageCoreTypes: assign __getattr__ = returned_function
ImportSystem-->>UserCode: module loaded
Note over UserCode,PackageCoreTypes: First attribute access (lazy)
UserCode->>PackageCoreTypes: access DataclassSerializationMixin
activate DynamicGetattr
PackageCoreTypes->>DynamicGetattr: __getattr__("DataclassSerializationMixin")
DynamicGetattr->>DynamicGetattr: name in _dynamic_imports?
DynamicGetattr->>ImportBuiltin: __import__("codeweaver.core.types.dataclasses", fromlist=[""])
ImportBuiltin-->>DynamicGetattr: module object
DynamicGetattr->>Submodule: getattr(module, "DataclassSerializationMixin")
Submodule-->>DynamicGetattr: class DataclassSerializationMixin
DynamicGetattr->>DynamicGetattr: detect LazyImport via _resolve? (not in this case)
DynamicGetattr->>PackageCoreTypes: cache in globals()["DataclassSerializationMixin"]
DynamicGetattr-->>UserCode: class DataclassSerializationMixin
deactivate DynamicGetattr
Note over UserCode,PackageCoreTypes: Subsequent access hits cache
UserCode->>PackageCoreTypes: access DataclassSerializationMixin
PackageCoreTypes-->>UserCode: cached class from globals()
Class diagram for refactored core.types modulesclassDiagram
direction LR
class core_types_utils {
+clean_sentinel_from_schema(schema: dict~str, Any~) void
+generate_title(model: type~Any~) str
+generate_field_title(name: str, info: FieldInfo) str
}
class core_types_dataclasses_DataclassSerializationMixin {
-_module: str
-_adapter: TypeAdapter~Self~
+_get_module() str
+dump_json(**kwargs) bytes
+dump_python(**kwargs) dict~str, Any~
+validate_json(data: bytes, **kwargs) Self
+validate_python(data: dict~str, Any~, **kwargs) Self
+serialize_for_cli() dict~str, Any~
+serialize_for_telemetry() dict~str, Any~
-_telemetry_keys() dict~FilteredKeyT, AnonymityConversion~ | None
-_telemetry_handler(_serialized_self: dict~str, Any~) dict~str, Any~
}
class core_types_dataclasses_BaseEnumData {
+aliases: tuple~str, ...~
-_description: str | None
+description: str | None
}
class core_types_dataclasses_DATACLASS_CONFIG {
<<ConfigDict>>
}
class core_types_models_BasedModel {
<<PydanticRootModel>>
+model_config: ConfigDict
}
class core_types_models_BASEDMODEL_CONFIG {
<<ConfigDict>>
}
class core_types_models_FROZEN_BASEDMODEL_CONFIG {
<<ConfigDict>>
}
class core_types_enum_BaseEnum {
<<Enum>>
+variable str
+as_title str
+from_string(value: str) BaseEnum
+filtered(values: Any) Any
}
class core_types_enum_BaseDataclassEnum {
<<Enum>>
}
class core_types_enum_AnonymityConversion {
<<Enum>>
+FORBIDDEN
+COUNT
+DISTRIBUTION
+filtered(values: Any) Any
}
class core_types_env_EnvFormat {
<<BaseEnum>>
+STRING
+NUMBER
+BOOLEAN
+FILEPATH
}
class core_types_env_EnvVarInfo {
<<NamedTuple>>
+env: str
+description: str
+is_required: bool
+is_secret: bool
+fmt: EnvFormat
+default: str | None
+choices: set~str~ | None
+variable_name: str | None
+as_mcp_info() dict~str, Any~
+as_kwarg() dict~str, str | None~
+as_docker_yaml() None
}
class core_types_sentinel_Sentinel {
<<BasedModel subclass>>
+name: str
+module_name: str | None
+value Any
}
class core_types_sentinel_Unset {
<<Sentinel subclass>>
}
class core_types_sentinel_UNSET {
<<instance Unset>>
}
%% Relationships within core.types
core_types_dataclasses_DataclassSerializationMixin ..> core_types_utils : uses
core_types_dataclasses_BaseEnumData --|> core_types_dataclasses_DataclassSerializationMixin
core_types_enum_BaseDataclassEnum <|-- BaseAgentTask_dataclasses : example
core_types_dataclasses_BaseEnumData <.. core_types_enum_BaseDataclassEnum : member_type
core_types_models_BASEDMODEL_CONFIG ..> core_types_utils : uses_generators
core_types_models_FROZEN_BASEDMODEL_CONFIG ..> core_types_models_BASEDMODEL_CONFIG
core_types_models_BasedModel ..> core_types_models_BASEDMODEL_CONFIG : configured_by
core_types_enum_BaseEnum <.. core_types_env_EnvFormat : inherits
core_types_env_EnvVarInfo ..> core_types_env_EnvFormat
core_types_sentinel_Sentinel ..|> core_types_models_BasedModel
core_types_sentinel_Unset --|> core_types_sentinel_Sentinel
core_types_sentinel_UNSET ..> core_types_sentinel_Unset
core_types_enum_AnonymityConversion ..> core_types_dataclasses_DataclassSerializationMixin : telemetry_filter
class BaseAgentTask_dataclasses {
<<example subclass of BaseEnumData>>
}
BaseAgentTask_dataclasses --|> core_types_dataclasses_BaseEnumData
%% Config relationships
core_types_dataclasses_DATACLASS_CONFIG ..> core_types_utils
core_types_dataclasses_DataclassSerializationMixin ..> core_types_dataclasses_DATACLASS_CONFIG : configured_by
Class diagram for lazy import and getattr infrastructureclassDiagram
direction LR
class common_utils_lazy_importer_LazyImport {
+module_name: str
+attr_name: str | None
+_resolved: object | None
+_resolve() object
+__call__(*args, **kwargs) Any
}
class common_utils_lazy_importer_module {
<<module>>
+lazy_import(module_name: str, attr_name: str) LazyImport
}
class common_utils_lazy_getter_module {
<<module>>
+create_lazy_getattr(dynamic_imports: MappingProxyType~str, tuple~str, str~~, module_globals: dict~str, object~, module_name: str) object
}
class common_package_export_common {
<<package __init__>>
+__getattr__(name: str) object
+_dynamic_imports: MappingProxyType~str, tuple~str, str~~
}
class core_package_export_core {
<<package __init__>>
+__getattr__(name: str) object
+_dynamic_imports: MappingProxyType~str, tuple~str, str~~
}
class core_types_package_export_core_types {
<<package __init__>>
+__getattr__(name: str) object
+_dynamic_imports: MappingProxyType~str, tuple~str, str~~
}
class engine_indexer_package_export {
<<package __init__>>
+__getattr__(name: str) object
}
class engine_search_package_export {
<<package __init__>>
+__getattr__(name: str) object
}
class engine_watcher_package_export {
<<package __init__>>
+__getattr__(name: str) object
}
class tokenizers_package_export {
<<package __init__>>
+__getattr__(name: str) object
}
class mcp_package_export {
<<package __init__>>
+__getattr__(name: str) object
}
class server_package_export {
<<package __init__>>
+__getattr__(name: str) object
}
class cli_package_export {
<<package __init__>>
+__getattr__(name: str) object
}
class cli_ui_package_export {
<<package __init__>>
+__getattr__(name: str) object
}
class agent_api_package_export {
<<package __init__>>
+__getattr__(name: str) object
}
class providers_agent_package_export {
<<package __init__>>
+__getattr__(name: str) object
}
class providers_embedding_capabilities_export {
<<package __init__>>
+__getattr__(name: str) object
}
class common_telemetry_package_export {
<<package __init__>>
+__getattr__(name: str) object
}
class config_package_export {
<<package __init__>>
+__getattr__(name: str) object
}
class engine_chunker_delimiters_export {
<<package __init__>>
+__getattr__(name: str) object
}
%% Relationships
common_utils_lazy_importer_module ..> common_utils_lazy_importer_LazyImport : returns
common_utils_lazy_getter_module ..> common_utils_lazy_importer_LazyImport : resolves_instances
common_package_export_common ..> common_utils_lazy_getter_module : uses_create_lazy_getattr
core_package_export_core ..> common_utils_lazy_getter_module : uses_create_lazy_getattr
core_types_package_export_core_types ..> common_utils_lazy_getter_module : uses_create_lazy_getattr
engine_indexer_package_export ..> common_utils_lazy_getter_module
engine_search_package_export ..> common_utils_lazy_getter_module
engine_watcher_package_export ..> common_utils_lazy_getter_module
tokenizers_package_export ..> common_utils_lazy_getter_module
mcp_package_export ..> common_utils_lazy_getter_module
server_package_export ..> common_utils_lazy_getter_module
cli_package_export ..> common_utils_lazy_getter_module
cli_ui_package_export ..> common_utils_lazy_getter_module
agent_api_package_export ..> common_utils_lazy_getter_module
providers_agent_package_export ..> common_utils_lazy_getter_module
providers_embedding_capabilities_export ..> common_utils_lazy_getter_module
common_telemetry_package_export ..> common_utils_lazy_getter_module
config_package_export ..> common_utils_lazy_getter_module
engine_chunker_delimiters_export ..> common_utils_lazy_getter_module
common_package_export_common ..> common_utils_lazy_importer_module : reexports_lazy_import
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
👋 Hey @bashandbone, Thanks for your contribution to codeweaver! 🧵You need to agree to the CLA first... 🖊️Before we can accept your contribution, you need to agree to our Contributor License Agreement (CLA). To agree to the CLA, please comment:
Those exact words are important1, so please don't change them. 😉 You can read the full CLA here: Contributor License Agreement ✅ @bashandbone has signed the CLA. You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. Footnotes
|
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| from pydantic import BaseModel, ConfigDict, PrivateAttr, RootModel, TypeAdapter | ||
| from pydantic.fields import ComputedFieldInfo, FieldInfo | ||
| from pydantic.main import IncEx | ||
| from pydantic import BaseModel, ConfigDict, RootModel |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
- In
tests/unit/engine/test_failover_tracker.py::TestRecordFileIndexed._validate_test_recorded_fileyou now call_mock_file_recordingbut that helper isn’t defined in the class/module (and the method previously recursed into itself), so this should be corrected to use an existing helper or renamed consistently to avoid a runtime error. - The signature change in
mcp/user_agent.find_code_toolfromcontext: Context | None = Nonetocontext: Contextis a breaking API change for any existing callers that omitcontext; consider keeping the default (or providing a compatibility wrapper) unless you intend to enforce this at all call sites. - The new
create_lazy_getattrincommon/utils/lazy_getter.pyduck-typesLazyImportby checking for a_resolveattribute; if any non-lazy object exposes_resolvethis will be incorrectly resolved—consider narrowing the check (e.g.,isinstanceagainstLazyImportor a marker protocol) to make this behavior safer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `tests/unit/engine/test_failover_tracker.py::TestRecordFileIndexed._validate_test_recorded_file` you now call `_mock_file_recording` but that helper isn’t defined in the class/module (and the method previously recursed into itself), so this should be corrected to use an existing helper or renamed consistently to avoid a runtime error.
- The signature change in `mcp/user_agent.find_code_tool` from `context: Context | None = None` to `context: Context` is a breaking API change for any existing callers that omit `context`; consider keeping the default (or providing a compatibility wrapper) unless you intend to enforce this at all call sites.
- The new `create_lazy_getattr` in `common/utils/lazy_getter.py` duck-types `LazyImport` by checking for a `_resolve` attribute; if any non-lazy object exposes `_resolve` this will be incorrectly resolved—consider narrowing the check (e.g., `isinstance` against `LazyImport` or a marker protocol) to make this behavior safer.
## Individual Comments
### Comment 1
<location> `src/codeweaver/providers/reranking/providers/base.py:325-328` </location>
<code_context>
-**File**: `src/codeweaver/providers/provider.py`
-
-```python
-@property
-def uses_httpx(self) -> bool:
- """Check if the provider uses httpx for HTTP connections.
</code_context>
<issue_to_address>
**issue (bug_risk):** circuit_breaker_state now accesses `.variable` on an Enum that only exposes `.value`, which will raise at runtime.
Here `CircuitBreakerState` is still a plain `Enum`, so members only expose `.value`. Using `.variable` on `self._circuit_state` will raise `AttributeError`. Either update `CircuitBreakerState` to inherit from `BaseEnum` (like other providers) or keep this property using `.value` for this enum.
</issue_to_address>
### Comment 2
<location> `src/codeweaver/semantic/classifier.py:162-156` </location>
<code_context>
),
]
classification_method: ClassificationMethod
- evidence: Annotated[
- tuple[EvidenceKind, ...],
- Field(description="Kinds of evidence used for classification", default_factory=tuple),
- ]
+ evidence: tuple[EvidenceKind, ...] = Field(
+ description="Kinds of evidence used for classification",
+ default_factory=tuple,
+ field_title_generator=generate_field_title,
+ )
adjustment: Annotated[
</code_context>
<issue_to_address>
**issue (bug_risk):** The `evidence` field on GrammarClassificationResult now defaults to a Field object rather than an empty tuple.
With the new declaration, `evidence: tuple[EvidenceKind, ...] = Field(...)` in a `NamedTuple` sets the class attribute default to the `Field` instance, so `GrammarClassificationResult()` will see `evidence` as a `Field` instead of a tuple. To preserve the empty-tuple default and correct typing, keep the `Annotated[tuple[EvidenceKind, ...], Field(..., default_factory=tuple)]` form and omit a `= ...` default on the class attribute.
</issue_to_address>
### Comment 3
<location> `src/codeweaver/server/health/health_service.py:252` </location>
<code_context>
def _extract_circuit_breaker_state(self, circuit_state_raw: Any) -> str:
"""Extract circuit breaker state string from raw value.
- Handles both string values, enum values, and mock objects with .value attribute.
+ Handles both string values, enum values, and mock objects with .variable attribute.
Args:
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Health service now prefers `.variable`, which may change reported state strings for non-BaseEnum types.
The helper now prefers `.variable` and otherwise uses `str(circuit_state_raw)`. For enums that are still plain `Enum` (e.g. circuit breaker enums not yet on `BaseEnum`), this will yield values like `'CircuitBreakerState.CLOSED'` instead of `'closed'`, which may break monitoring or health checks that expect the old lowercase strings. To maintain compatibility, consider falling back to `.value` when `.variable` is absent (e.g. `if hasattr(..., 'variable'): ... elif hasattr(..., 'value'): ...`).
</issue_to_address>
### Comment 4
<location> `tests/integration/chunker/test_e2e.py:100-105` </location>
<code_context>
- with pytest.raises(Exception): # Will fail until fallback implemented
- chunker = selector.select_for_file(file)
- chunker.chunk(content, file=file)
+ # Should gracefully degrade and still produce chunks via fallback
+ chunker = selector.select_for_file(file)
+ chunks = chunker.chunk(content, file=file)
+
+ # Should produce at least some chunks through degradation
+ assert len(chunks) > 0, "Degradation should produce chunks"
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding an explicit failure-path assertion to prove the semantic-to-delimiter fallback is used when semantic chunking fails.
This test only covers the case where `GracefulChunker` succeeds. To exercise the new behavior end-to-end, add or extend a test that forces `SemanticChunker` to fail (e.g., monkeypatch `SemanticChunker.chunk` or the underlying parser to raise `ParseError`/`NotImplementedError`), then assert that `GracefulChunker.chunk` still returns chunks from the delimiter-based strategy. That will better protect against regressions in the fallback wiring.
Suggested implementation:
```python
selector = ChunkerSelector(mock_governor)
file = mock_discovered_file(str(fixture_path))
# Should gracefully degrade and still produce chunks via fallback
chunker = selector.select_for_file(file)
chunks = chunker.chunk(content, file=file)
# Should produce at least some chunks through degradation
assert len(chunks) > 0, "Degradation should produce chunks"
def test_graceful_chunker_uses_delimiter_fallback_when_semantic_fails(
monkeypatch,
mock_governor,
mock_discovered_file,
fixture_path,
):
"""
Force SemanticChunker to fail and assert that GracefulChunker still
returns chunks via the delimiter-based fallback.
"""
from chunking import SemanticChunker # adjust import to match project structure
def _failing_semantic_chunk(*args, **kwargs):
raise NotImplementedError("forced failure for fallback test")
# Force semantic chunking to fail
monkeypatch.setattr(SemanticChunker, "chunk", _failing_semantic_chunk)
selector = ChunkerSelector(mock_governor)
file = mock_discovered_file(str(fixture_path))
with open(fixture_path, "r", encoding="utf-8") as f:
content = f.read()
chunker = selector.select_for_file(file)
# Even though semantic chunking fails, the GracefulChunker should still
# produce chunks using the delimiter-based strategy.
chunks = chunker.chunk(content, file=file)
assert chunks, "Delimiter-based fallback should still return chunks when semantic chunking fails"
for chunk in chunks:
assert getattr(chunk, "content", None), "Chunks produced by fallback should have content"
```
1. Adjust the import `from chunking import SemanticChunker` to match the actual module path where `SemanticChunker` is defined (for example, `from myapp.chunking.semantic import SemanticChunker`).
2. If your test module already imports `SemanticChunker` elsewhere, remove the local import inside the test and reuse the existing import instead.
3. Ensure `fixture_path` in this new test points to a real text file fixture in your test suite (you can mirror whatever fixture is used in the existing graceful degradation test).
</issue_to_address>
### Comment 5
<location> `tests/unit/engine/chunker/test_selector.py:65-68` </location>
<code_context>
- # Should gracefully degrade and still produce chunks
- # (implementation will add fallback logic)
- with pytest.raises(Exception): # Will fail until fallback implemented
- chunker = selector.select_for_file(file)
- chunker.chunk(content, file=file)
+ # Should gracefully degrade and still produce chunks via fallback
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen the selector unit test by asserting the fallback type and language configuration on GracefulChunker.
Since this now validates that `ChunkerSelector` returns a `GracefulChunker` with a `SemanticChunker` as `primary`, please also assert that:
- `chunker.fallback` is a `DelimiterChunker`, and
- the fallback’s language matches the expected semantic/config language for `.py` files.
This will ensure the test fails if future changes alter the fallback type or its language wiring while still returning a `GracefulChunker`.
</issue_to_address>
### Comment 6
<location> `tests/unit/engine/test_failover_tracker.py:98-101` </location>
<code_context>
def _validate_test_recorded_file(
self, tmp_path: Path, file_hash: str, tracker: FileChangeTracker
):
- self._validate_test_recorded_file(tmp_path, file_hash, tracker)
+ self._mock_file_recording(tmp_path, file_hash, tracker)
assert "src/test.py" in tracker.pending_changes
assert tracker.file_hashes["src/test.py"] == file_hash
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test case for the "no change" path when re-indexing a file with the same hash.
To better cover `record_file_indexed`, please add a test that calls it twice for the same path with the same hash and asserts that `pending_changes` is not updated (e.g., the set size stays the same). This verifies unchanged files are correctly ignored and avoids unnecessary re-indexing.
</issue_to_address>
### Comment 7
<location> `tests/integration/test_custom_config.py:62` </location>
<code_context>
base_config = {
"url": os.environ["CODEWEAVER_VECTOR_STORE_URL"],
"api_key": os.environ["QDRANT__SERVICE__API_KEY"],
- "prefer_grpc": True,
+ "prefer_grpc": False,
}
</code_context>
<issue_to_address>
**suggestion (testing):** Consider parametrizing this integration test over `prefer_grpc` to keep coverage for both HTTP and gRPC configurations.
Flipping `prefer_grpc` to `False` makes this test cover only the HTTP path. If we want continued confidence in both transports, please either parametrize the test over `prefer_grpc in (True, False)` or add a sibling test so both gRPC and HTTP configs are exercised end-to-end and regressions in the gRPC path are still caught.
Suggested implementation:
```python
# Base config shared across transports; individual tests/fixtures should
# inject "prefer_grpc" (True/False) so both HTTP and gRPC are exercised.
base_config = {
"url": os.environ["CODEWEAVER_VECTOR_STORE_URL"],
"api_key": os.environ["QDRANT__SERVICE__API_KEY"],
}
# Test custom collection name
```
To fully implement the parametrization you described, you’ll also need to:
1. Identify the test(s) in this file that currently use `base_config` directly (e.g. `config=base_config` or similar).
2. Parametrize those tests over `prefer_grpc`:
```python
import pytest
@pytest.mark.parametrize("prefer_grpc", [True, False])
def test_custom_config(..., prefer_grpc: bool):
config = {
**base_config,
"prefer_grpc": prefer_grpc,
}
# use `config` instead of `base_config` below
```
3. If multiple tests share this pattern, consider adding a parametrized fixture:
```python
@pytest.fixture(params=[True, False], ids=["http", "grpc"])
def transport_config(request):
return {
**base_config,
"prefer_grpc": request.param,
}
```
and then inject `transport_config` into the tests instead of `base_config`.
These additional changes ensure that every integration test that depends on this configuration will exercise both HTTP and gRPC paths end-to-end.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @property | ||
| def circuit_breaker_state(self) -> str: | ||
| """Get current circuit breaker state for health monitoring.""" | ||
| return self._circuit_state.value | ||
| return self._circuit_state.variable |
There was a problem hiding this comment.
issue (bug_risk): circuit_breaker_state now accesses .variable on an Enum that only exposes .value, which will raise at runtime.
Here CircuitBreakerState is still a plain Enum, so members only expose .value. Using .variable on self._circuit_state will raise AttributeError. Either update CircuitBreakerState to inherit from BaseEnum (like other providers) or keep this property using .value for this enum.
| ] | ||
| rank: Annotated[ | ||
| ImportanceRank, | ||
| Field( |
There was a problem hiding this comment.
issue (bug_risk): The evidence field on GrammarClassificationResult now defaults to a Field object rather than an empty tuple.
With the new declaration, evidence: tuple[EvidenceKind, ...] = Field(...) in a NamedTuple sets the class attribute default to the Field instance, so GrammarClassificationResult() will see evidence as a Field instead of a tuple. To preserve the empty-tuple default and correct typing, keep the Annotated[tuple[EvidenceKind, ...], Field(..., default_factory=tuple)] form and omit a = ... default on the class attribute.
| def _extract_circuit_breaker_state(self, circuit_state_raw: Any) -> str: | ||
| """Extract circuit breaker state string from raw value. | ||
| Handles both string values, enum values, and mock objects with .value attribute. |
There was a problem hiding this comment.
suggestion (bug_risk): Health service now prefers .variable, which may change reported state strings for non-BaseEnum types.
The helper now prefers .variable and otherwise uses str(circuit_state_raw). For enums that are still plain Enum (e.g. circuit breaker enums not yet on BaseEnum), this will yield values like 'CircuitBreakerState.CLOSED' instead of 'closed', which may break monitoring or health checks that expect the old lowercase strings. To maintain compatibility, consider falling back to .value when .variable is absent (e.g. if hasattr(..., 'variable'): ... elif hasattr(..., 'value'): ...).
| chunker = selector.select_for_file(file) | ||
| assert isinstance(chunker, SemanticChunker) | ||
| # Selector now wraps SemanticChunker in GracefulChunker for automatic fallback | ||
| assert isinstance(chunker, GracefulChunker) | ||
| assert isinstance(chunker.primary, SemanticChunker) |
There was a problem hiding this comment.
suggestion (testing): Strengthen the selector unit test by asserting the fallback type and language configuration on GracefulChunker.
Since this now validates that ChunkerSelector returns a GracefulChunker with a SemanticChunker as primary, please also assert that:
chunker.fallbackis aDelimiterChunker, and- the fallback’s language matches the expected semantic/config language for
.pyfiles.
This will ensure the test fails if future changes alter the fallback type or its language wiring while still returning aGracefulChunker.
| def _validate_test_recorded_file( | ||
| self, tmp_path: Path, file_hash: str, tracker: FileChangeTracker | ||
| ): | ||
| self._validate_test_recorded_file(tmp_path, file_hash, tracker) | ||
| self._mock_file_recording(tmp_path, file_hash, tracker) |
There was a problem hiding this comment.
suggestion (testing): Add a test case for the "no change" path when re-indexing a file with the same hash.
To better cover record_file_indexed, please add a test that calls it twice for the same path with the same hash and asserts that pending_changes is not updated (e.g., the set size stays the same). This verifies unchanged files are correctly ignored and avoids unnecessary re-indexing.
| base_config = { | ||
| "url": os.environ["CODEWEAVER_VECTOR_STORE_URL"], | ||
| "api_key": os.environ["QDRANT__SERVICE__API_KEY"], | ||
| "prefer_grpc": True, |
There was a problem hiding this comment.
suggestion (testing): Consider parametrizing this integration test over prefer_grpc to keep coverage for both HTTP and gRPC configurations.
Flipping prefer_grpc to False makes this test cover only the HTTP path. If we want continued confidence in both transports, please either parametrize the test over prefer_grpc in (True, False) or add a sibling test so both gRPC and HTTP configs are exercised end-to-end and regressions in the gRPC path are still caught.
Suggested implementation:
# Base config shared across transports; individual tests/fixtures should
# inject "prefer_grpc" (True/False) so both HTTP and gRPC are exercised.
base_config = {
"url": os.environ["CODEWEAVER_VECTOR_STORE_URL"],
"api_key": os.environ["QDRANT__SERVICE__API_KEY"],
}
# Test custom collection nameTo fully implement the parametrization you described, you’ll also need to:
-
Identify the test(s) in this file that currently use
base_configdirectly (e.g.config=base_configor similar). -
Parametrize those tests over
prefer_grpc:import pytest @pytest.mark.parametrize("prefer_grpc", [True, False]) def test_custom_config(..., prefer_grpc: bool): config = { **base_config, "prefer_grpc": prefer_grpc, } # use `config` instead of `base_config` below
-
If multiple tests share this pattern, consider adding a parametrized fixture:
@pytest.fixture(params=[True, False], ids=["http", "grpc"]) def transport_config(request): return { **base_config, "prefer_grpc": request.param, }
and then inject
transport_configinto the tests instead ofbase_config.
These additional changes ensure that every integration test that depends on this configuration will exercise both HTTP and gRPC paths end-to-end.
There was a problem hiding this comment.
Pull request overview
This PR refactors the core types system and lazy import handling in CodeWeaver to improve maintainability and reduce cross-dependencies. The changes include separating core types into focused modules (env.py, dataclasses.py, utils.py), refactoring lazy import utilities, fixing 13 failing tests, enhancing schema metadata for NamedTuples, and standardizing enum string conversion patterns from .value to .variable.
Key improvements:
- Modularized core.types package for better separation of concerns
- Separated lazy import infrastructure (create_lazy_getattr) from internal utilities
- Fixed test construction issues and updated references
- Enhanced IDE support through improved field definitions on NamedTuples
- Standardized enum string conversion across the codebase
Reviewed changes
Copilot reviewed 104 out of 110 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| uv.lock | Updated pydantic-ai dependencies and added new packages (ag-ui-protocol, argcomplete, logfire, nexus-rpc, temporalio, wrapt, opentelemetry packages) |
| tests/unit/engine/test_failover_tracker.py | Fixed test helper methods and simplified test setup |
| tests/unit/engine/chunker/test_selector.py | Updated tests to expect GracefulChunker wrapper around SemanticChunker |
| tests/integration/test_persistence.py | Added sourcery skip directive for test module import |
| tests/integration/test_custom_config.py | Changed prefer_grpc from True to False |
| tests/integration/chunker/test_e2e.py | Updated degradation test to expect successful fallback behavior |
| src/codeweaver/tokenizers/init.py | Updated import path for create_lazy_getattr |
| src/codeweaver/server/server.py | Updated import paths for dataclasses and registry modules |
| src/codeweaver/server/health/health_service.py | Changed .value to .variable for enum conversion and simplified exception handling |
| src/codeweaver/server/health/init.py | Updated import path for create_lazy_getattr |
| src/codeweaver/server/init.py | Updated import path for create_lazy_getattr |
| Multiple semantic files | Added generate_field_title to Field annotations and updated enum conversions to use .variable |
| Multiple provider files | Changed CircuitBreakerState from Enum to BaseEnum and updated enum conversions |
| src/codeweaver/mcp/user_agent.py | Changed context parameter from optional to required |
| src/codeweaver/mcp/types.py | Removed exclude_args from ToolRegistrationDict |
| src/codeweaver/mcp/tools.py | Removed exclude_args usage and updated tool title |
| Multiple init.py files | Updated import paths for create_lazy_getattr |
| src/codeweaver/engine files | Updated enum conversions to use .variable |
| src/codeweaver/data/node_types/hcl-node-types.json | Added new HCL language support file (1290 lines) |
| src/codeweaver/data/node_types/analysis/*.license | Removed license files for deleted analysis module |
| src/codeweaver/data/node_types/analysis/lang_to_nodes.json | Removed analysis data file (5970 lines) |
| src/codeweaver/data/init.py | Updated documentation to describe build-time generation |
| src/codeweaver/core/types/utils.py | New file containing utility functions moved from models.py |
| src/codeweaver/core/types/sentinel.py | Updated imports and all exports |
| src/codeweaver/core/types/models.py | Refactored to focus on Pydantic base implementations, moved utilities out |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| computed_field_names := type(self).__pydantic_decorators__.computed_fields.keys() # type: ignore | ||
| ): | ||
| # Add computed fields to the fields dict (keys only, values don't matter for iteration) | ||
| fields = {**fields, **dict.fromkeys(computed_field_names)} # type: ignore |
There was a problem hiding this comment.
Variable fields is not used.
| from pydantic import BaseModel, ConfigDict, PrivateAttr, RootModel, TypeAdapter | ||
| from pydantic.fields import ComputedFieldInfo, FieldInfo | ||
| from pydantic.main import IncEx | ||
| from pydantic import BaseModel, ConfigDict, RootModel |
There was a problem hiding this comment.
Import of 'RootModel' is not used.
… of NamedTuple types (being accessed as a BaseEnum member), updated string conversion patterns for consistency with BaseEnum classes across the codebase.
…latten imports - Removed obsolete license file for nodes_to_langs.json. - Updated imports in progress.py, state.py, tools.py, user_agent.py, and other files to use the correct modules for DataclassSerializationMixin and DATACLASS_CONFIG. - Removed exclude_args from ToolRegistrationDict and adjusted related code in tools.py. - Modified find_code_tool function to receive fastmcp context. - Enhanced embedding types with field title generation for better schema documentation. - Updated AgentTask classifications to include additional synonyms for tasks. - Improved code readability and maintainability by applying consistent formatting and annotations across various modules.
bashandbone
force-pushed
the
fix/tests-and-consistency
branch
from
1feedf3 to
e235e5d
Compare
…mendations - Introduced `test-infrastructure-summary.md` detailing current test coverage, risks, and a 4-tier CI strategy to enhance testing efficiency. - Added `test_skip_xfail_analysis.md` to analyze skipped and xfail tests, identifying gaps in coverage and proposing immediate actions for improvement. - Updated `dataclasses.py` to improve serialization logic by using fields directly instead of pydantic fields. - Enhanced `tools.py` documentation to reflect the increase in supported programming languages from 160 to 166. - Modified `user_agent.py` to allow optional context parameter for the `find_code_tool` function. - Fixed health monitoring tests to align with the BaseEnum interface by replacing `.value` with `.variable`. - Added tests in `test_selector.py` to verify fallback configurations for chunkers. - Implemented idempotency test in `test_failover_tracker.py` to ensure unchanged files do not affect pending changes during re-indexing.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 102 out of 107 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…s and initialization; add mock_only marker to config tests
…roved test execution and categorization
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 105 out of 111 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| manager._backup_sync_task.cancel() | ||
| try: | ||
| await manager._backup_sync_task | ||
| except asyncio.CancelledError: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
Pull Request Review - PR #203This is a comprehensive refactoring PR that makes significant architectural improvements to the codebase. I've reviewed the changes and have the following feedback: ✅ Strengths1. Excellent Type System RefactoringThe separation of core types into dedicated modules (
2. Smart Enum StandardizationThe move from
Example from @property
def variable(self) -> str:
return textcase.snake(self.value) if self.value_type is str else textcase.snake(self.name)3. Schema Metadata ImprovementsAdding field descriptions to NamedTuple types will significantly improve:
4. Repository CleanupRemoving obsolete files (analysis artifacts, unused docs) reduces clutter. The removal of
|
…──────────────────────�[0m
�[38;5;238m│ �[0m�[1mSTDIN�[0m
�[38;5;238m─────┼──────────────────────────────────────────────────────────────────────────�[0m
�[38;5;238m 1�[0m �[38;5;238m│�[0m �[38;5;231mrefactor(logging): Rename logging modules to _logging to fix namespace conflicts�[0m
�[38;5;238m 2�[0m �[38;5;238m│�[0m
�[38;5;238m 3�[0m �[38;5;238m│�[0m �[38;5;231mRenamed logging.py → _logging.py across multiple modules to resolve�[0m
�[38;5;238m 4�[0m �[38;5;238m│�[0m �[38;5;231m`import logging` namespace conflicts that were causing issues.�[0m
�[38;5;238m 5�[0m �[38;5;238m│�[0m
�[38;5;238m 6�[0m �[38;5;238m│�[0m �[38;5;231mChanges:�[0m
�[38;5;238m 7�[0m �[38;5;238m│�[0m �[38;5;231m- Renamed logging modules in common, config, chunker, watcher, server�[0m
�[38;5;238m 8�[0m �[38;5;238m│�[0m �[38;5;231m- Updated all imports to use _logging naming convention�[0m
�[38;5;238m 9�[0m �[38;5;238m│�[0m �[38;5;231m- Added test infrastructure for nightly and weekly test runs�[0m
�[38;5;238m 10�[0m �[38;5;238m│�[0m �[38;5;231m- Enhanced test documentation and coverage analysis�[0m
�[38;5;238m 11�[0m �[38;5;238m│�[0m �[38;5;231m- Updated lazy import validation�[0m
�[38;5;238m 12�[0m �[38;5;238m│�[0m �[38;5;231m- Cleaned up unused variables in test_publish_validation�[0m
�[38;5;238m 13�[0m �[38;5;238m│�[0m
�[38;5;238m 14�[0m �[38;5;238m│�[0m �[38;5;231mThis refactor maintains all existing functionality while fixing the�[0m
�[38;5;238m 15�[0m �[38;5;238m│�[0m �[38;5;231mnamespace collision issue that prevented proper access to Python's�[0m
�[38;5;238m 16�[0m �[38;5;238m│�[0m �[38;5;231mstandard logging module.�[0m
�[38;5;238m 17�[0m �[38;5;238m│�[0m
�[38;5;238m 18�[0m �[38;5;238m│�[0m �[38;5;231m🤖 Generated with [Claude Code](https://claude.com/claude-code)�[0m
�[38;5;238m 19�[0m �[38;5;238m│�[0m
�[38;5;238m 20�[0m �[38;5;238m│�[0m �[38;5;231mCo-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>�[0m
�[38;5;238m─────┴──────────────────────────────────────────────────────────────────────────�[0m
… enhanced -- identified unused/implemented structured logging system and implemented it
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 133 out of 142 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Updated package name references from \codeweaver\ to \code-weaver\ in integration and smoke tests. - Relaxed performance test thresholds for memory persistence to accommodate WSL I/O overhead. - Changed Qdrant Docker image version from \latest\ to \v1.16.1\ for consistency and reliability. - Enhanced documentation in unit tests for embedding reconciliation, clarifying test organization and rationale. - Removed outdated integration tests for reconciliation exception handling, consolidating testing strategy.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 150 out of 159 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| When add_missing_embeddings_to_existing_chunks raises ProviderError, | ||
| prime_index should log the error and continue successfully. | ||
| """ | ||
| import logging |
There was a problem hiding this comment.
This import of module logging is redundant, as it was previously imported on line 54.
| This test validates the error handling at indexer.py:1377-1388. | ||
| """ | ||
| import logging |
There was a problem hiding this comment.
This import of module logging is redundant, as it was previously imported on line 54.
| This test validates the error handling at indexer.py:1389-1396. | ||
| """ | ||
| import logging |
There was a problem hiding this comment.
This import of module logging is redundant, as it was previously imported on line 54.
|
Reviewing this PR now. Initial observations: This is a large refactoring with 159 files changed. I'm examining the core changes and will provide detailed feedback shortly. |
Code Review - PR #203OverviewThis is a substantial refactoring PR with 10,753 additions and 30,120 deletions across 107 files. The changes are well-organized and address several architectural improvements while fixing 13 failing tests. ✅ Positive Aspects1. Excellent Type System RefactoringThe core types module reorganization is well-executed:
2. Enhanced Schema MetadataAdding field descriptions to NamedTuple types improves:
3. Comprehensive CI ImprovementsThe new nightly and weekly test workflows are excellent additions:
4. Standardized Enum UsageAligning on 5. Logging Namespace FixRenaming 6. Test Infrastructure Improvements
|
Pull Request Review: Refactor core.types and lazy import handlingSummaryThis is a massive refactoring PR with 159 files changed (+10,753/-30,120 lines). While the goals are admirable—improving code organization, fixing tests, and enhancing schema metadata—the sheer scope makes this extremely difficult to review thoroughly and risky to merge. Current StatusCI Status: ❌ 3 test suites failing (Python 3.13t, 3.14, 3.14t experimental), plus CLA check failure Positive Aspects ✅1. Core Type Refactoring - Well ExecutedThe separation of
Code quality highlights:
2. Lazy Import Refactoring - Architecturally SoundThe split between
3. Schema Metadata ImprovementsAdding field descriptions to NamedTuple types improves:
4. Enum String Conversion StandardizationStandardizing on
Critical Concerns
|
|
|
||
|
|
||
| type AliasMap = dict[ | ||
| Annotated[Provider, BeforeValidator(lambda v: Provider.from_string(v))], |
| ModelMaker, | ||
| dict[ | ||
| ModelName, | ||
| tuple[Annotated[HFModelProviders, BeforeValidator(lambda v: Provider.from_string(v))], ...], |
| Convert an MTEB model metadata dictionary to a PartialCapabilities object. | ||
| """ | ||
| loader = getattr(model, "loader", {}) | ||
| loader = loader if isinstance(loader, dict) else {} |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review - PR #203
I've reviewed this large refactoring PR that reorganizes core types, lazy imports, and improves codebase consistency. Here's my detailed feedback:
✅ Strengths
1. Core Types Refactoring (core.types module split)
- Excellent separation of concerns: Breaking
core.typesintoenv.py,dataclasses.py,utils.pyis well-architected env.py: CleanEnvVarInfoNamedTuple with proper MCP integration methods (as_mcp_info(),as_kwarg())dataclasses.py: Well-designedDataclassSerializationMixinwith comprehensive Pydantic TypeAdapter integrationutils.py: Good utility separation withclean_sentinel_from_schema()for JSON schema cleanup- The lazy loading setup in
core.types/__init__.pyusingcreate_lazy_getattris clean and follows best practices
2. Lazy Import Infrastructure (lazy_getter.py extraction)
- Smart separation: Extracting
create_lazy_getattrinto its own module (lazy_getter.py) fromlazy_importer.pyis architecturally sound - The
LazyImportclass implementation is sophisticated with:- Thread-safe resolution using locks
- Proper parent chain resolution
- Good handling of introspection attributes for Pydantic compatibility
- Excellent documentation with clear examples
- This separation allows
create_lazy_getattrto be used at package level without circular dependency concerns
3. Schema Metadata Improvements
- Adding field descriptions to NamedTuple types improves JSON schema generation and IDE support
- The
DATACLASS_CONFIGwithfield_title_generatorandmodel_title_generatorprovides consistent schema metadata - Telemetry filtering with
AnonymityConversionenum andserialize_for_telemetry()is privacy-conscious
4. Enum Standardization (.variable pattern)
- Standardizing on
.variablefor enum-to-string conversion is good for consistency - The
BaseEnum.variableproperty usingtextcase.snake()provides predictable output - Keeping
.valueavailable for special cases maintains flexibility
⚠️ Issues to Address
1. CI Failures (BLOCKING)
All test jobs are failing in the "Setup Python environment with Mise" step. The mise tool is exiting with code 1 during environment setup. This needs investigation - possibly related to:
- Changes in
mise.toml(+162/-4 lines) - Changes in
hk.pklconfiguration - New validation tasks like
validate-lazy-imports.py
2. Unused Variable (from code quality bot - scripts/model_data/mteb_to_codeweaver.py:544-545)
loader = getattr(model, "loader", {})
loader = loader if isinstance(loader, dict) else {}The loader variable is assigned but never used. Should be removed as suggested by the bot.
3. Large Deletions (30,851 deletions)
- Deletion of
src/codeweaver/data/node_types/analysis/*.jsonfiles (17,049 lines of JSON removed) - PR description says "eliminates the analysis module in data/node_types, which is not used at runtime"
- ✅ This is appropriate IF confirmed these files aren't used at runtime and will be restored to
scripts/in a follow-up PR as stated
📋 Recommendations
1. Fix CI First (HIGH PRIORITY)
- Debug the mise setup failures
- Ensure all tests pass before merging
- The
validate-lazy-imports.pycheck (line 84 inhk.pkl) might be too strict for the PR changes
2. Code Quality
- Remove unused
loadervariable inmteb_to_codeweaver.py:544-545 - Verify the new
validate-lazy-importsmise task doesn't have false positives
3. Testing
- PR claims to fix 13 failing tests - good!
- Once CI is green, verify test coverage hasn't decreased
4. Documentation
- The PR adds excellent documentation files in
claudedocs/ - Consider adding migration notes if any public APIs changed in the core types refactoring
🎯 Overall Assessment
This is high-quality refactoring work with:
- Thoughtful architectural improvements
- Good separation of concerns
- Improved maintainability through reduced cross-dependencies
- Better IDE support through schema metadata
However, the PR cannot be merged until:
- ✅ CI passes (all Python version tests)
- ✅ Unused
loadervariable is removed - ✅ Any mise/hk configuration issues are resolved
The refactoring approach is sound and aligns with the repository's architecture. Once the CI and minor code quality issues are addressed, this will be a strong addition to the codebase.
Status: ❌ Request Changes - Fix CI failures and unused variable before merging.
Response to Code Quality Bot (Comment #2617214896)Unused Variable in The bot's assessment is correct. The loader = getattr(model, "loader", {})
loader = loader if isinstance(loader, dict) else {}Fix: Remove both lines 544-545. The variable is not referenced anywhere in the function body, so removal is safe and will have no impact on functionality. |
Comprehensive Pull Request Review - PR #203I've completed a thorough review of this PR. Below is my detailed assessment: Executive SummaryThis is a well-executed, comprehensive refactoring that significantly improves code organization, maintainability, and consistency across the codebase. The PR successfully addresses multiple technical debt items while maintaining backward compatibility. Key Changes Reviewed
Detailed Findings1. core.types Refactoring ⭐⭐⭐⭐⭐ (Excellent)New Structure:
Strengths:
Architecture Quality: This is a textbook example of layered architecture. Minor Issue:
2. Lazy Import Refactoring ⭐⭐⭐⭐½ (Excellent with bonus bug fix)Changes:
Improved Resolution Logic: # Before: isinstance() check
if isinstance(result, LazyImport):
result = result._resolve()
# After: Duck typing (better!)
if hasattr(result, "_resolve") and callable(result._resolve):
result = result._resolve()Benefits:
Recommendation:
3. Logging Namespace Fix ⭐⭐⭐⭐⭐ (Excellent)Solution: Renamed all Files Updated:
Consistency: All 16 import statements updated correctly ✓ Why this approach is ideal:
Defensive Programming: from logging import getLogger as getLogger # Explicit re-export4. Test Fixes ✅ (13 tests fixed)Pattern: Updated enum string conversion from Example from test_health_monitoring.py: # Before (lines 135, 153, etc.)
embedding_instance.circuit_breaker_state.value = "closed"
# After
embedding_instance.circuit_breaker_state.variable = "closed"Rationale (from PR description):
Files verified:
5. Code Quality AssessmentSecurity: ✅ No security concerns identified
Performance: ✅ No regressions, some improvements
Best Practices: ✅ Excellent adherence
Code Style: ✅ Consistent with existing codebase
6. CI Status
|
|
Closing this PR -- it has been superseded in the |
This pull request:
primarily refactors core types into several new modules in that package (
core.types) including:env.py,dataclasses.py,utils.py. This separation significantly reduced cross-dependencies within the package and improves maintainability.Refactors lazy imports in codeweaver.common.utils, separating
create_lazy_getattrfrom thelazy_importermodule into its own,lazy_getter.py. This keeps these foundational functions cleanly separated between package infrastructure for create_lazy_getattr and internal lazy imports for the lazy_importer module.Fixes 13 failing tests, which were failing for multiple reasons, mostly related to test construction and changes in references.
Improves schema metadata across the codebase, adding field definitions for most NamedTuple types to improve schema metadata and associated IDE support.
Removes several files that were either no longer current or not core to the development environment to reduce clutter in the repo.
eliminates the analysis module in data/node_types, which is not used at runtime. This was included in the package by mistake. A separate PR will restore it to its proper place in scripts/ and fix the build generation logic that put it there.
Aligns enum string conversion patterns with the rest of the codebase, using
.variableto convert BaseEnum classes to strings. In most cases .variable produces the same results as '.value' but we've settled on standardizing on .variable for consistency and to allow us to use .value when needed for special cases (whereas .variable should always produce the same results across all class/subclass members).Summary by Sourcery
Refactor core typing and lazy import infrastructure, expand schema metadata for NamedTuples and enums, and clean up unused data and configuration while fixing related tests.
Enhancements:
Build:
Documentation:
Tests:
Chores: