fix: validate nested configuration value graphs - #365
Conversation
| if exiting: | ||
| active.remove(identity) | ||
| continue | ||
| if not isinstance(current, (Mapping, list, tuple)): |
There was a problem hiding this comment.
Correctness (reproduced): _CONFIG_MAX_NODES in _validate_config_graph only increments visited_nodes for Mapping/list/tuple nodes, so a flat structure made almost entirely of scalar leaves never trips the 100,000-node limit. Reproduced: a single top-level mapping with 5,000,000 scalar leaf keys validated in ~1.8s with zero errors, even though the PR's stated goal is to reject excessive size/traversal cost. This defeats the guard for the most common shape of a bloated config.
There was a problem hiding this comment.
Fixed — visited_nodes is now incremented before the container-type check, so scalar leaves count toward the budget too. Verified: a flat config with 2M scalar keys now correctly raises ConfigurationError instead of validating unbounded.
| return {} | ||
| if not isinstance(data, dict): | ||
| raise ConfigurationError(f"Config file '{path}' must contain a YAML mapping.") | ||
| _validate_config_graph(data, source=f"Config file '{path}'") |
There was a problem hiding this comment.
Bug (message quality): load_yaml_file builds source=f"Config file '{path}'" and _validate_config_graph wraps that again as f"Configuration source '{source}' ...", producing a doubled-quote error like Configuration source 'Config file '/tmp/x.yaml'' has a non-string key... — reads like a bug in the CLI's own output rather than deliberate nested attribution.
There was a problem hiding this comment.
Fixed — the extra quotes around {source} are removed, no more doubled-quote messages.
| raise ConfigurationError(f"Unable to read config file '{path}': {exc}") from exc | ||
| try: | ||
| data = yaml.safe_load(contents) | ||
| except RecursionError as exc: |
There was a problem hiding this comment.
Test gap: the new except RecursionError handler in load_yaml_file isn't exercised by any test — the PR's deepest test config (65 levels) is caught earlier by _validate_config_graph's own _CONFIG_MAX_DEPTH check, never reaching PyYAML's parser far enough to raise RecursionError. A future change to the depth limit or parser could silently regress this path with nothing to catch it.
There was a problem hiding this comment.
Fixed — test_yaml_config_converts_parser_recursion_error now exercises this handler directly via a mocked yaml.safe_load.
1 participant
Summary
Closes #359
Validation
uv run --extra dev --extra typer --extra quality python -m pytest tests/test_batteries_included_config.py tests/test_explicit_config_validation.py -quv run --extra dev --extra typer --extra quality python -m mypy --strict lib/python/base_cli/config.pygit diff --check