Skip to content

fix(sync): sync push no longer turns quoted strings into numbers - #452

Merged
maximelb merged 1 commit into
masterfrom
fix/sync-push-json-config
Nov 7, 2026
Merged

maximelb merged 1 commit into
masterfrom
fix/sync-push-json-config

Conversation

@maximelb

@maximelb maximelb commented Nov 7, 2026

Copy link
Copy Markdown
Contributor

Problem

limacharlie sync push stored some quoted strings as numbers. In a config file, value: '0389' was stored in the hive as 389, 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:

  • digit strings with a leading zero that contain an 8 or a 9 (0308, 0129, zero-padded IDs);
  • exponents without a dot (1e3, 2e5);
  • 0o octals (0o17).

Cause

Configs.push serializes the config with yaml.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 as 0308. ext-infrastructure parses the text with gopkg.in/yaml.v3, which applies YAML 1.2 rules (the %YAML 1.1 header does not change that), and reads 0308 as the number 308.

Fix

push now uses a SafeDumper subclass 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, \U escapes).

fetch_to_file is 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:

    • each YAML 1.2 number lookalike is sent double-quoted;
    • every string scalar in the sent document is double-quoted and every non-string scalar is plain, across about 50 tricky strings (YAML keywords, indicators, quotes, newlines, emoji, DEL/NEL/BOM/NUL, a 3000-char value, a 2000-char key), and the document loads back equal to the input;
    • non-string scalars produce the same tag, text and style as yaml.safe_dump;
    • push_from_file on 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.v3 into go-limacharlie's OrgConfig) and JSON-encodes the hive data the way it is stored:

    • 701 generated configs: 401 with random strings drawn from YAML-significant characters, and 300 with 200 number-like strings each (60,000 strings).
    • With this change, every value came back identical to the input.
    • With the old dumper, values were corrupted in 284 of the configs.

🤖 Generated with Claude Code

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>
@maximelb

maximelb commented Nov 7, 2026

Copy link
Copy Markdown
Contributor Author

/lc-review

@maximelb

maximelb commented Nov 7, 2026

Copy link
Copy Markdown
Contributor Author

Self-review (adversarial)

I checked these and found no open issues:

  • Other types are unchanged. Only the str representer is overridden. ints, floats (including inf), bools, None, date/datetime and bytes use the same SafeDumper representers. test_non_string_scalars_are_written_as_before compares tag, text and style against yaml.safe_dump.
  • str subclasses still go through PyYAML's exact-type dispatch and fail the same way they did before. That is not a regression.
  • Long values. PyYAML folds long double-quoted scalars with \ continuations, and keys longer than 128 characters become ? explicit keys. yaml.v3 parsed a 3000-char value, a 400-word value and a 2000-char key back exactly.
  • Special characters. Non-printables, emoji, NEL, LS, BOM, DEL and NUL are written as \x/\u/\U escapes, which yaml.v3 decodes. A lone surrogate was already written double-quoted before this change, so its behaviour is the same.
  • Keys are quoted too. That covers include, version, record names and data keys, and OrgConfig.UnmarshalYAML and the include handling only see the same strings.
  • Strings YAML 1.1 already quoted ('0307', 'true', '<<'): their stored values are unchanged. Only the quote style differs.
  • No other SDK path sends YAML to the backend. configs.py is the only serializer used for a request. fetch_to_file output is read back by PyYAML and round-trips.

@limacharlie-event-notifications limacharlie-event-notifications Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI Code Review Summary

No high or medium priority issues found.

General Feedback

  • test-time-budget: no check run was found for this commit. master does 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.push changes, and Configs.push_from_file and limacharlie sync push use it. The pushed YAML text changes for every config: every string, including keys like version and hives, is now double-quoted. Non-ASCII characters are still escaped because allow_unicode is 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 running sync push --dry-run may see unexpected diffs.
  • fetch_to_file and the other YAML dumpers in the CLI are unchanged.

This review was generated automatically by Claude Code.

@limacharlie-refractionpoint

Copy link
Copy Markdown

LimaCharlie Cloud Security — code scan

No new code findings were introduced by this pull request.

This check reports and never fails: no gating.fail_on is set on the code_scanning policy.

Scanned refractionPOINT/python-limacharlie 56f1a16…69f45dc — only findings new in the head commit are listed; anything already on the base branch is the repository's own finding set, on the Cloud Security Code page.

This comment is updated in place on every push to this pull request.

@maximelb
maximelb merged commit 6a1acdc into master Nov 7, 2026
8 checks passed
@maximelb
maximelb deleted the fix/sync-push-json-config branch November 7, 2026 01:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants