fix: centralized pipeline config type coercion to prevent string-type crashes#2031
fix: centralized pipeline config type coercion to prevent string-type crashes#2031RockChinQ merged 4 commits intolangbot-app:masterfrom
Conversation
…ions Pipeline configs stored in SQLAlchemy JSON columns can have values turned into strings after UI edits (e.g. "120" instead of 120), causing runtime arithmetic/logic errors. Add centralized type coercion in load_pipeline() that leverages existing metadata YAML type definitions (integer, number, float, boolean) to convert values before they reach downstream stages. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
dosubot
Bot
added
size:L
There was a problem hiding this comment.
Pull request overview
This PR adds a centralized type coercion layer for pipeline configuration values. Since pipeline configs are stored in a SQLAlchemy JSON column, numeric and boolean values can become strings after users modify them via the admin UI and reload. The new config_coercion.py module uses the existing metadata YAML type definitions to cast values back to their declared types (integer, number/float, boolean) during pipeline loading.
Changes:
- New
config_coercion.pymodule with type coercion logic that walks metadata YAML definitions and converts config values in-place - Integration point in
load_pipeline()to callcoerce_pipeline_configafter entity construction, before stage initialization
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/langbot/pkg/pipeline/config_coercion.py |
New module implementing _coerce_value and coerce_pipeline_config functions for type-safe config loading |
src/langbot/pkg/pipeline/pipelinemgr.py |
Imports and calls coerce_pipeline_config in load_pipeline() with the four metadata dicts |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| coerce_pipeline_config( | ||
| pipeline_entity.config, | ||
| self.ap.pipeline_config_meta_trigger, | ||
| self.ap.pipeline_config_meta_safety, | ||
| self.ap.pipeline_config_meta_ai, | ||
| self.ap.pipeline_config_meta_output, |
There was a problem hiding this comment.
This change will break at least 3 existing unit tests (test_load_pipeline, test_get_pipeline_by_uuid, test_remove_pipeline in tests/unit_tests/pipeline/test_pipelinemgr.py) because MockApplication in tests/unit_tests/pipeline/conftest.py does not define pipeline_config_meta_trigger, pipeline_config_meta_safety, pipeline_config_meta_ai, or pipeline_config_meta_output. Since MockApplication is a plain class (not a Mock), accessing these attributes will raise AttributeError.
You need to add these attributes to MockApplication.__init__, for example as empty dicts {'name': 'trigger', 'stages': []}, etc., or update the test fixture to include them.
| coerce_pipeline_config( | |
| pipeline_entity.config, | |
| self.ap.pipeline_config_meta_trigger, | |
| self.ap.pipeline_config_meta_safety, | |
| self.ap.pipeline_config_meta_ai, | |
| self.ap.pipeline_config_meta_output, | |
| trigger_meta = getattr(self.ap, "pipeline_config_meta_trigger", {"name": "trigger", "stages": []}) | |
| safety_meta = getattr(self.ap, "pipeline_config_meta_safety", {"name": "safety", "stages": []}) | |
| ai_meta = getattr(self.ap, "pipeline_config_meta_ai", {"name": "ai", "stages": []}) | |
| output_meta = getattr(self.ap, "pipeline_config_meta_output", {"name": "output", "stages": []}) | |
| coerce_pipeline_config( | |
| pipeline_entity.config, | |
| trigger_meta, | |
| safety_meta, | |
| ai_meta, | |
| output_meta, |
There was a problem hiding this comment.
Fixed by using getattr with default values. See commit c918e84
| def _coerce_value(value, expected_type: str): | ||
| """Convert a single value to the expected type. | ||
|
|
||
| Returns the converted value, or the original value if no conversion needed. | ||
| """ | ||
| if value is None: | ||
| return value | ||
|
|
||
| if expected_type == 'boolean': | ||
| if isinstance(value, bool): | ||
| return value | ||
| return _coerce_bool(value) | ||
|
|
||
| coerce_fn = _COERCE_MAP.get(expected_type) | ||
| if coerce_fn is None: | ||
| return value | ||
|
|
||
| # Already the correct type | ||
| if expected_type == 'integer' and isinstance(value, int) and not isinstance(value, bool): | ||
| return value | ||
| if expected_type in ('number', 'float') and isinstance(value, (int, float)) and not isinstance(value, bool): | ||
| return float(value) | ||
|
|
||
| return coerce_fn(value) |
There was a problem hiding this comment.
The new config_coercion.py module has no unit tests, but the tests/unit_tests/pipeline/ directory contains tests for other pipeline modules (e.g., test_pipelinemgr.py, test_bansess.py, test_ratelimit.py, test_resprule.py). Since this module contains non-trivial coercion logic with multiple code paths (integer, float, number, boolean, None handling, error cases), it should have dedicated unit tests covering at least: string-to-int, string-to-float, string-to-bool (true/false/invalid), None passthrough, already-correct-type passthrough, and the error/warning path for invalid conversions.
There was a problem hiding this comment.
Added 18 unit tests in test_config_coercion.py, covering string-to-int/float/bool, None passthrough, type passthrough, invalid conversion warning, missing section/field skip, and multiple metadata handling.
…oercion - Use getattr with defaults for pipeline_config_meta_* attributes to avoid AttributeError when MockApplication lacks these fields - Add 18 unit tests for config_coercion module covering all code paths Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Summary
"120"而非120），导致下游 15+ 处代码出现算术崩溃、逻辑错误或 API 调用失败config_coercion.py模块，利用已有的 metadata YAML 类型定义（integer/number/float/boolean），在load_pipeline()中集中做类型转换，一处调用覆盖所有 pipeline 加载场景None值跳过，已正确类型跳过，转换失败仅 log warning 不崩溃Changes
src/langbot/pkg/pipeline/config_coercion.pysrc/langbot/pkg/pipeline/pipelinemgr.pyload_pipeline()中调用转换函数