fix(configtree): upgrade YAML parser to 1.2 to prevent implicit type coercions - #549
Open
smrutisenapati wants to merge 4 commits into
Open
fix(configtree): upgrade YAML parser to 1.2 to prevent implicit type coercions#549smrutisenapati wants to merge 4 commits into
smrutisenapati wants to merge 4 commits into
Conversation
…coercions Replace PyYAML (YAML 1.1) with ruamel.yaml (YAML 1.2) in the configtree import/export pipeline. YAML 1.1 silently coerces yes/no/on/off → bool and 0777 → octal 511 before values reach the DB or etcd; YAML 1.2 preserves these as strings/decimal integers. Fixes: - import_keys.py: _load_yaml_file() uses YAML(typ="safe") instead of benedict(f, format="yaml") for YAML inputs (JSON path unchanged) - util.py combine_metadata(): _yaml_reader.load() replaces yaml.safe_load() so stored values like "yes" export as "yes", not "true" - util.py export_to_files(): ruamel.yaml writer replaces benedict.to_yaml() for YAML output; _to_plain() normalises benedict/CommentedMap subclasses before dumping since ruamel's representer requires exact dict/list types Closes rapyuta-robotics/rapyuta_io#2045 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
🤖 Pull Request Artifacts (#27817697081) 🎉 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR upgrades the configtree YAML import/export path from YAML 1.1 parsing behavior to YAML 1.2 by replacing PyYAML/benedict YAML parsing with ruamel.yaml, preventing implicit coercions like yes/no/on/off → bool and 0777 → octal.
Changes:
- Added
ruamel-yamlas a dependency (and locked it inuv.lock). - Updated configtree YAML read/write paths to use
ruamel.yamlfor parsing/dumping while keeping JSON behavior unchanged. - Added unit tests asserting YAML 1.1 boolean-like scalars remain strings and
0777parses as decimal777.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
pyproject.toml |
Adds ruamel-yaml>=0.17.0 runtime dependency. |
uv.lock |
Locks ruamel-yaml (0.19.1) into the resolved environment. |
riocli/configtree/import_keys.py |
Switches YAML file loading to ruamel.yaml safe loader for import + overrides. |
riocli/configtree/util.py |
Switches YAML parsing/dumping in combine/export helpers to ruamel.yaml. |
tests/unit/configtree/test_util.py |
Adds regression tests for YAML 1.1 implicit coercion differences. |
|
|
||
| import yaml | ||
| from benedict import benedict | ||
| from ruamel.yaml import YAML |
Comment on lines
+327
to
+331
| if file_format == "json": | ||
| loaded = benedict(f, format="json") | ||
| else: | ||
| loaded = benedict(_load_yaml_file(f)) | ||
| override.merge(loaded.unflatten(separator="/")) |
Add parse_configtree_value() to util.py and use it in put_key_in_revision
so that the put path matches the import path's type detection:
- '{a: 1}' → {"a": 1} (normalised to valid JSON, not raw YAML shorthand)
- '[1,2]' → [1, 2] (spacing normalised via json.dumps)
- 'true' → bool True → "true" (same as import)
- 'yes' → str "yes" (YAML 1.2: not coerced to bool)
- '300' → int 300 → "300" (same as import)
For simple scalars (int/float/bool/null/plain string) the stored bytes are
identical to before. The visible fix is for YAML shorthand dicts/lists and
spacing normalisation, where raw string passthrough produced non-JSON output.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Sort third-party imports in util.py and import_keys.py (ruff I001) - Import YAMLError explicitly; narrow except clauses from Exception to YAMLError in combine_metadata() and parse_configtree_value() - Fix --override Click option default from None to () so iterating over it never raises TypeError when the flag is omitted Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ruamel.yaml(YAML 1.2) across the entire configtree import/export pipelineyes/on/no/off→booland0777→ octal 511 before values ever reached the DB or etcd; YAML 1.2 preserves these as strings/decimal integersconfig_ui.pyalready used ruamel.yaml — this aligns the CLI with that decisionChanges
pyproject.toml/uv.lockruamel-yaml>=0.17.0dependencyriocli/configtree/import_keys.py_load_yaml_file()helper usesYAML(typ="safe")instead ofbenedict(f, format="yaml")for YAML inputs; JSON path unchangedriocli/configtree/util.pycombine_metadata()_yaml_reader.load()replacesyaml.safe_load()— stored value"yes"now exports as"yes"instead of"true"riocli/configtree/util.pyexport_to_files()benedict.to_yaml();_to_plain()helper normalisesbenedict/CommentedMapsubclasses before dumping (ruamel's representer requires exactdict/listtypes)tests/unit/configtree/test_util.pytest_yaml11_booleans_are_preserved_as_stringsandtest_octal_0777_is_decimal_777— assertions built from observed ruamel.yaml output, not the issue's tableTest plan
uv run pytest tests/unit/ -v)yes/no/on/off→strand0777→int 777test_non_yaml_value_is_kept_as_raw_stringconfirms logging format strings still survive parse errors gracefullyCloses rapyuta-robotics/rapyuta_io#2045
🤖 Generated with Claude Code