Repository navigation
fix(sync): sync push no longer turns quoted strings into numbers - #452
Merged
Merged
Conversation
sync push serialized the config with PyYAML, which follows YAML 1.1 and leaves a string bare whenever YAML 1.1 would read it back as a string. ext-infrastructure parses that text with gopkg.in/yaml.v3 (YAML 1.2 rules), where some of those bare strings are numbers: '0308', '0129', zero-padded IDs containing an 8 or 9, '1e3' and '0o17' were stored in the hive as 308, 129, 1000 and 15, so D&R rules matching them silently stopped matching. Write every string as a double-quoted scalar, which is a string under any YAML version. Other types are emitted exactly as before. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
lcbill
approved these changes
Nov 7, 2026
Contributor
Author
|
/lc-review |
Contributor
Author
Self-review (adversarial)I checked these and found no open issues:
|
There was a problem hiding this comment.
AI Code Review Summary
No high or medium priority issues found.
General Feedback
test-time-budget: no check run was found for this commit.masterdoes not require the check in branch protection or rulesets, so it does not block this review.- I reviewed the code statically and did not run the tests in this environment.
Blast Radius
- Only
Configs.pushchanges, andConfigs.push_from_fileandlimacharlie sync pushuse it. The pushed YAML text changes for every config: every string, including keys likeversionandhives, is now double-quoted. Non-ASCII characters are still escaped becauseallow_unicodeis off, as before. Long strings are still folded at width 80, now as double-quoted line continuations. - This is a fix at the server level. Values that were stored as numbers by earlier pushes (for example
389) will show as changed on the next push, because they are now stored as the strings in the file. Users runningsync push --dry-runmay see unexpected diffs. fetch_to_fileand the other YAML dumpers in the CLI are unchanged.
This review was generated automatically by Claude Code.
LimaCharlie Cloud Security — code scanNo new code findings were introduced by this pull request. This check reports and never fails: no Scanned This comment is updated in place on every push to this pull request. |
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.
Problem
limacharlie sync pushstored some quoted strings as numbers. In a config file,value: '0389'was stored in the hive as389, and a D&R rule matching it silently stopped matching. The push reported success.Affected values are strings that YAML 1.2 reads as numbers but YAML 1.1 does not:
0308,0129, zero-padded IDs);1e3,2e5);0ooctals (0o17).Cause
Configs.pushserializes the config withyaml.safe_dump(config, version=(1, 1)). PyYAML follows YAML 1.1 and quotes a string only when YAML 1.1 would read the bare text as another type, so'0308'goes out as0308. ext-infrastructure parses the text withgopkg.in/yaml.v3, which applies YAML 1.2 rules (the%YAML 1.1header does not change that), and reads0308as the number 308.Fix
pushnow uses aSafeDumpersubclass that writes every string, keys included, as a double-quoted scalar. A double-quoted scalar is a string under every YAML version. Every other type (ints, floats, bools, null, dates, binary) goes through the same representers as before, so it is written exactly as it was.Sending JSON instead (JSON is valid YAML 1.2) was considered and rejected: yaml.v3 refuses implicit keys longer than 1024 characters, so a long lookup key such as a URL would fail to parse. It also rejects JSON's
😀surrogate-pair escapes for emoji. PyYAML handles both (?explicit keys,\Uescapes).fetch_to_fileis unchanged. Its output is read back by PyYAML on the next push, so it round-trips.Values already stored with the wrong type are corrected by pushing again with this version.
Testing
New unit tests in
tests/unit/test_sdk_configs.py::TestConfigsPushStringTyping. They decode the actual request the SDK sends and check that:yaml.safe_dump;push_from_fileon a rule file with quoted zero-padded values sends them quoted.13 of the 14 new tests fail without the fix. The 14th checks that non-string output is unchanged, so it passes either way.
Full suite as CI runs it:
pytest tests/unit/ tests/microbenchmarks/gives 5260 passed. The 5 skips are pre-existing and platform-only.Checked against the server's real parser. A small Go program unmarshals the SDK's output with ext-infrastructure's code (
gopkg.in/yaml.v3into go-limacharlie'sOrgConfig) and JSON-encodes the hive data the way it is stored:🤖 Generated with Claude Code