Repository navigation
Conversation
1 task done
unsafe-libyaml is unmaintained (RUSTSEC-2024-0449) and reaches us only through serde_yaml, which is itself deprecated. serde-saphyr is a maintained serde deserializer written in safe Rust, and it is the crate the advisory names for serde users. All YAML parsing now goes through src/yaml.rs. Callers read the whole file and parse the bytes with options that keep serde_yaml's behavior where serde-saphyr allows it: `<<` stays an ordinary key, `!!binary` is neither decoded nor validated, `.inf`, `.nan` and `1e999` still parse in values we ignore, YAML 1.1 booleans such as `yes` are still rejected, and errors stay on one line. A repeated key takes its last value, as in Psych. serde-saphyr's default rejects one anywhere in a file, and serde_yaml rejected one only in a field we read. Error text also changes, and a few rare inputs are now rejected, accepted or read differently, as the pull request describes. Two deserialize_with helpers keep accepting an empty, `~` or `null` string field or list item, but read all three as "", where serde_yaml kept `~` and `null` as text. The list helper also reads an empty, `~` or `null` list as empty, where serde_yaml rejected the last two. package.yml is read as an Option, so an empty file still means no owner, and now so does a file that is only `~`, `null` or `...`. serde-saphyr 2.0 does both of these itself, but its unreleased 2.1, which our "2.0.0" requirement admits, stops (serde-saphyr#233). Errors from team files and package.yml are wrapped in a type with no source(), since error_stack would otherwise print an error inside an alias twice. With duplicate keys set to last-wins, serde-saphyr buffers each mapping it reads into one of our structs before serde sees it, and buffers a nested mapping again at each level where it follows an integer key, so under its default budget a crafted 480 KB package.yml made `generate` peak at about 2.7 GB, against 35 MB on main. Nesting is now capped at 16 levels and nodes at 20,000, and the caps on scalar bytes, parser events, recorded anchor events and recorded anchor bytes grow with the node limit, at 256, 50, 5 and 64 per node. The worst file found that fits, 68 KB, makes `generate` peak at about 290 MB on macOS, against 12 MB on main. Ordinary files are unaffected: the 1,015 team and package.yml files in two large private monorepos nest at most 5 deep with fewer than 2,200 nodes, and `generate` gives the same output for both as on main, with the same peak memory within about 2%. The node limit is the new `yaml_max_nodes` key in config/code_ownership.yml, so a repo whose team files or package.yml files outgrow it can raise it, and the caps that grow with it, without waiting for a release. Every command that reads team files or package.yml honors it, and the team cache and the memoized team map are keyed on it. A file over it, or over a cap that grows with it, fails like a malformed file, with a hint naming the key. The config file itself is always read with the default. Release archives now also hold a licenses/ directory next to the codeowners binary, which stays at the archive root, where dotslash-config.json looks for it. dev/license-notices copies into it the license files of two crates serde-saphyr brings in, whose licenses require binary redistributions to carry their notices: granit-parser, its parser, which asks for all inherited and new notices, and encoding_rs, whose WHATWG-derived data is under BSD-3-Clause. Today's macOS binaries don't link encoding_rs, since we parse only byte slices, so its notice is a precaution. The script takes versions from Cargo.lock and fails if a listed crate leaves the dependency graph or ships no license files. The jobs that build release archives run only on main after a version bump, so the Check job also runs the script on pull requests. Dependabot's cargo group now excludes serde-saphyr, so its bumps get their own pull request. New tests pin each option, budget and hint, both helpers and every deserialize_with field, empty package.yml files, and the node limit through the CLI. For the event cap they check only that it grows with the node limit. The config tests go through Config::load_from_path, as production does. Fixes #141
dduugg
force-pushed
the
replace-serde-yaml-with-serde-saphyr
branch
from
October 21, 2026 07:28
1721171 to
25be0f1
Compare
dduugg
added a commit
to rubyatscale/code_ownership
that referenced
this pull request
Oct 21, 2026
* Fix inverted stage_changes in CodeOwnership.validate! validate! is documented to stage the regenerated .github/CODEOWNERS by default and to leave it unstaged with stage_changes: false, which is what bin/codeownership validate --skip-stage passes. It did the opposite. validate! passed !stage_changes to the extension, whose generate_and_validate took it as skip_stage, as the RBI declared, but handed it unchanged to codeowners-rs's runner::generate_and_validate, whose third parameter is git_stage. That has been the case since the extension was added in #126: the codeowners-rs it pinned already took git_stage (rubyatscale/codeowners-rs#65). codeowners-rs ignores the result of its git add, so nothing ever failed. Pass stage_changes through, and rename the extension's parameter and the RBI's to git_stage to match codeowners-rs, so the flag means the same thing at every layer. The extension's behavior is unchanged, so the tracked bundle needs no rebuild: the fix works with it as committed, and the specs that call RustCodeOwners.generate_and_validate(nil, false) to write a CODEOWNERS file still don't stage it. Add specs that validate! stages CODEOWNERS by default in a git repository and leaves it unstaged with stage_changes: false. Both fail without the fix. They clear git's repo-local environment variables first, so a run from a hook or a linked worktree can't stage into the enclosing repository. The CLI specs only check the arguments that reach validate!, so nothing covered what it does to the index. Add a regression guard for an empty package.yml. rubyatscale/codeowners-rs#142 moves YAML parsing to serde-saphyr, whose unreleased 2.1.0 no longer reads a null document as an empty struct (the fix for bourumir-wyngs/serde-saphyr#233). That PR reads package.yml as an Option so an empty file still means no owner. The spec covers an empty file, a comment-only file and a bare "---". validate! raises if such a file fails to parse, while for_file only prints the error and falls back to the file's annotation, so the spec also checks that for_file prints nothing. It passes today. * Bump version to 2.2.1 * Clear git's repo-local env vars for the whole suite The fix makes every real validate! call with default arguments run git add, not only the new staging specs. Two cli_spec examples do that, and under a hook's GIT_DIR they staged .github/CODEOWNERS into the enclosing repository, since the scrub only wrapped the staging context. Move it to a before(:suite) hook so every example's git runs against its own temp project. Split the empty package.yml example in two, so validation and the for_file parse-error check pass or fail separately, without nesting one expect inside another.
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.
Fixes #141.
unsafe-libyamlis unmaintained (RUSTSEC-2024-0449) and reaches us only throughserde_yaml, which is itself deprecated. This replacesserde_yamlwith serde-saphyr 2.0.0, the crate the advisory names for serde users. serde-saphyr and its parser, granit-parser, are pure Rust with#![forbid(unsafe_code)], though granit-parser depends on arraydeque, which usesunsafe. serde-saphyr is actively maintained: 32 releases in the past year, outside issues closed in a median of under a day, and kube-rs among its 400+ dependents. Both crates have a single maintainer, though, and their security policy covers only the latest release. 2.0.0 shipped about ten weeks after 1.0.0, so we should expect to take major versions promptly, which is why Dependabot now opens serde-saphyr bumps on their own.Alternatives considered
unowned_globs:that four fixtures use. It is still 0.0.x, so every release is semver-incompatible with the last.unsafe-libyamlor a fork of it.What changed
All YAML parsing goes through the new
src/yaml.rs:fs::readand parse it withfrom_slice_with_options, which accepts the same encodings serde_yaml did (UTF-8, with or without a BOM).from_readerwould also accept UTF-16 and silently replace invalid UTF-8 after a BOM, and it parses about 2.3x slower than serde_yaml.duplicate_keys: LastWins. The default rejects a repeated key anywhere in a file, including keys we never read, and one suchpackage.ymlwould abortgenerateandvalidatefor the whole repo. These exist in the wild: this public packwerk app repeatsenforce_privacy. The other options keep serde_yaml's behavior for<<, for!!binary, for.infand.nanin values we ignore, for YAML 1.1 booleans (yesandnostay errors), and for single-line error messages.LastWinsbuffers each mapping read into one of our structs before serde sees it, and buffers a nested mapping again at each level where it follows an integer key, so under the defaults a crafted 480 KBpackage.ymlmadegeneratepeak at about 2.7 GB, against 35 MB onmain. Under these budgets that file fails with a budget error. The worst file found that fits, 68 KB of 14 nested mappings that each start with0: xaround 9,750[]: []entries, holds about 115 MB of heap at its peak and makesgeneratepeak at about 290 MB on macOS, against 12 MB onmain. With only string keys the same nesting holds about 12 MB of heap. The depth limit also keeps nested input from overflowing the stack. The 1,015 team andpackage.ymlfiles in two large private monorepos nest at most 5 deep, with fewer than 2,200 nodes and no anchors, so the node limit leaves 9x headroom. It is the newyaml_max_nodesconfig key, so a repo whose files outgrow it can raise it, and the caps that grow with it, without waiting for a release. A file over the node limit or any of those caps fails like a malformed file, with a hint naming the key. The worst case grows with the limit, by about 14 KB of resident memory per node, so a 50,000-node default would give 23x headroom but raise it to about 710 MB. The 290 MB and 710 MB figures are for a file crafted to waste memory. Ordinary files add almost nothing: on the larger monorepo,generatepeaks at the same memory asmain, within about 2%.~ornullvalue in aStringfield or list item, which serde_yaml accepted. Two smalldeserialize_withhelpers keep accepting them, reading all three as"", where serde_yaml kept~andnullas text. The list helper also reads an empty,~ornulllist as empty itself, rather than relying on serde-saphyr to, andpackage.ymlis read as an optional struct, so an empty file still means no owner. serde-saphyr's master branch (2.1.0, not yet released) stops reading null as an empty list (serde-saphyr#233) or as an empty struct, and our2.0.0requirement admits 2.1. The tests pass against that branch too.src/common_test.rsnow go throughConfig::load_from_path, so they exercise the production path.src/yaml.rs, next to the options they cover. Ten assert what serde_yaml already did (checked by running the same assertions onmain). The other 18 pin intended changes: a repeatedownertakes the last value; a BOM before several keys now parses; a~ornulllist reads as empty, also through the list helper on its own; an empty,~,nullor...package.ymlmeans no owner; 16 levels of nesting parse and 17 don't; and a list of 100,000 items is rejected unlessyaml_max_nodes, read from the config, raises the limit. That last part is checked throughgenerate,for-fileandfor-file --from-codeowners, and for the team cache and the team map that--from-codeownersmemoizes, which are both keyed on the limit. The rest check that each cap that should grow withyaml_max_nodesdoes, that a breach of the node limit or any of those caps, even through an alias, gets the hint while a depth breach doesn't, and that an error inside an alias is printed once.deserialize_withattributes and the 19 places that passyaml_max_nodeson, 89 fail at least one test. The 5 that pass change nothing serde-saphyr 2.0.0 lets a test observe: two other event-cap multipliers, since the event cap is reached first only at a limit of 0, where every multiplier gives 0;non_finite_float_policy: AsString, since we read no floats; unwrapping oneAliasErrorinstead of looping, since serde-saphyr nests at most one; and readingpackage.ymlas the struct instead of anOption, which the empty-file test catches under serde-saphyr 2.1.cargogroup now excludes serde-saphyr, so its bumps arrive as their own pull requests and never wait on unrelated breakage. granit-parser is a transitive dependency, which Dependabot version updates skip, so a granit-parser fix reachesCargo.lockwhen a serde-saphyr release requires it, through a RustSec advisory, or through a manualcargo update -p granit-parser.licenses/directory next to thecodeownersbinary, which stays at the archive root, wheredotslash-config.jsonlooks for it. The newdev/license-noticesscript copies into it, verbatim, the license files of two crates serde-saphyr brings in, whose licenses require redistributions of the binary to carry their notices: granit-parser, whoseLICENSEasks for all inherited and new notices, and encoding_rs, whose WHATWG-derived data is under BSD-3-Clause. The script takes their versions fromCargo.lockand fails if a listed crate leaves the dependency graph or ships no license files. The jobs that build release archives run only onmainafter a version bump, so a new step in the Check job runs the script on pull requests. Today's macOS binaries don't link encoding_rs, since we parse only byte slices, so its notice is a precaution for a later switch to reader input. The README's installation section mentions the new directory.Behavior changes
Verified unchanged:
package.ymlfiles, the empty-file cases in the last bullet, and all 13 fixtures (includingdocs/CODEOWNERSfrom the custom-path fixture).package.yml, team and config files from GitHub, and about 1,200 files from nine private repositories. The one exception is two public GitHub Actions workflows namedpackage.ymlthat start with a UTF-8 BOM, which serde_yaml rejected and serde-saphyr accepts. For the seven private repositories with a config,generategives byte-identical CODEOWNERS, stdout, stderr and exit codes.generateon two large private monorepos, which parses 1,015 team andpackage.ymlfiles, gives byte-identical CODEOWNERS, empty stdout and stderr and exit code 0, the same asmain, and peaks at the same memory asmain, within about 2%.validate,crosscheck-owners, andfor-filewith and without--from-codeownerson 1,719 sampled files also give the same exit codes and output lines asmain. None of a wider local corpus of 1,615package.yml, team and config files, or the 42 fixtures, hits a budget.----onlypackage.ymlstill means "no owner", also under serde-saphyr 2.1.Changed: error text on every failure path, and a handful of rare inputs that are now rejected, accepted, or read differently. Apart from the error text of files that fail in both, and the two BOM files above, none of these occur in the 2,356 files.
Every change, with examples
Error text changes on every failure path. Messages no longer start with the key path, type errors no longer say what was found, and locations read
line L, column C, including theline 1, column 1that serde_yaml left out:githubmissing field `github`missing field `github` at line 1, column 1owned_globs: 5owned_globs: invalid type: integer `5`, expected a sequence at line 1 column 14expected sequence start at line 1, column 14do_not_add_to_codeowners_file: yesgithub.do_not_add_to_codeowners_file: invalid type: string "yes", expected a boolean at line 4 column 34invalid boolean (strict mode expects true/false) at line 4, column 34missing field `name`unexpected end of input at line 1, column 1found unexpected end of stream at line 4 column 1, while scanning a quoted scalar at line 3 column 9invalid indentation in multiline quoted scalar at line 4, column 1deserializing from YAML containing more than one document is not supportedmultiple YAML documents detected; use from_str_multiple or from_str_multiple_with_options at line 3, column 1~: xat the top levelcannot deserialize null into string; use Option<String> at line 1, column 1The last two messages give advice meant for Rust callers of serde-saphyr. A budget breach reads like
budget breached: Depth { depth: 17 } at line 2, column 70, under the path of the file. A breach of the node limit or a cap that grows with it adds the hintraise `yaml_max_nodes` (now 20000) in config/code_ownership.yml to allow larger files. An error inside an alias also says where the anchor was defined and used.A missing field is also reported at a different line. serde_yaml gave the line where the mapping starts. serde-saphyr gives the line of the last key it read in that mapping, so a longer team file without
githubcan point at an unrelated key such asruby:.Newly rejected. serde_yaml accepts these, and so does Psych except where noted:
-, such as:[...]or{...}value that has a tab before it is indented past its key, such asdependencies: [followed by a tab-indented"packs/a",. libyaml treats that tab as whitespace. Spaces before the tab, or a whole-file JSON document indented with tabs, still parse.name: "Payroll"# note), a block scalar header (description: |# note), a comma in a flow list ([a,# note), or a directive (%YAML 1.1# note).~: xdirectly in a mapping we read: the top level,metadata,githuborruby. Nested deeper inside a value we ignore, null keys still parse.!!timestampwith any value (Psych'sload_filerejects it too),!!intwith a value outside 64 bits or in YAML 1.1 form such as017or1:20,!!float 1_000.5, or a value that does not fit, such as!!int abc. In a field we read, a non-string core tag such asname: !!int 123is rejected too, and so is!!str truefor a boolean. serde-saphyr also types some local tags:!int,!float,!bool,!str,!seq,!map,!timestampand!timeon a value that doesn't fit them fail too, as do!includeon a list or mapping and!degreesor!radianson any value. So do a key with a non-string core tag such as!!int 1:in a mapping we read, andmetadata: !ormetadata: !!mapwith no value.yaml_max_nodes), anywhere in the file. serde_yaml had no limits for values we ignore.*a: 1, and a recursive alias such as&a [*a], even under keys we ignore. That last one fails withrecursive references require weak recursion types, more advice meant for Rust callers. Psych 4'sload_filerejects aliases by default.---or after the first key: before an indented key or a-entry, on a blank or comment line, or as the file's last bytes. A BOM at the start of a value, too. libyaml skips a BOM at the start of a line, so serde_yaml and Psych accept these. A BOM directly before a key at column 0 already failed onmain.Newly accepted:
duplicate field `owner`.---or before the first of several keys. serde_yaml failed on these, usually with "deserializing from YAML containing more than one document is not supported".~for a list field, read as an empty list, and for a boolean a quoted or block-scalartrue, with or without surrounding spaces, or an odd-casetRuE. serde-saphyr 2.1 rejectstRuEagain.package.ymlthat is only~ornull, read as no owner, as in Psych, or only..., which Psych rejects.metadata:; anchor names containing/,.or non-ASCII characters; empty keys and:symbolitems in flow lists, in values we ignore; a...line before the content, which Psych rejects too; unknown%directives; several leading BOMs; and surrogate-pair escapes such as"😀".Different values:
~,nullin any capitalization such asnUlL, a!!null-tagged value, or a local-tagged one such as!foo ~or an empty!foo, in a string field or list item reads as""instead of the literal text. Inowner,~,null,Null,NULLand an empty value already meant no owner, and now the rest do too. In a glob list such asowned_globs,generatewrites that""as a/line in CODEOWNERS, wheremainwrote/~for a~item. An empty item already gave/onmain. Psych reads these as nil, except that it keeps the text of a!!null-tagged value that isn't null, such as!!null x. serde-saphyr 2.1 reads odd capitalizations such asnUlLas text again.|,|+,>or>+block scalar (not|-or>-) that ends the file without a trailing newline now ends in\n, soowner: |followed byTeamAat end of file reads as"TeamA\n".name: TeamAfollowed by U+0085 reads as"TeamA\u{85}". Ruby'sYAML.dumpwrites a U+2028 or U+2029 in a quoted string as a raw break followed by indentation, so such a dumped value now keeps the indentation spaces. After a comment, one of them no longer ends the comment, so a key after it on the same line is lost:# header, U+0085,owner: TeamAreads as no owner.:or?, such as&a:b TeamA, now includes them, so the value readsTeamArather than:b TeamA.In an
owneror a teamname, the block scalar and line break changes mean the name no longer matches its team, the way a typo would. In code_ownership, which compares the names Rust reads against the ones Psych reads,for_filethen returns nil for that team's files, or raises withallow_raise: true.Cost
strip -x, from 3.71 MB to 4.50 MB on aarch64-apple-darwin. Unstripped, each architecture's binary grows by 0.9 to 1.0 MB, and the universal mac binary the release job uploads by 1.9 MB. Most of it is serde-saphyr's deserializer, plus annotate-snippets, which stays linked even with snippets off.generateon a large monorepo is unchanged within noise, because walking the tree dominates.Verification
cargo audit: no warnings, down from RUSTSEC-2024-0449.cargo tree -i unsafe-libyamlandcargo tree -i serde_yamlmatch nothing.cargo fmt --all -- --check,RUSTFLAGS=-Dwarnings cargo clippy --all-targets --all-features, andcargo test: 206 passed, 0 failed, 3 ignored (178 before, plus the 28 new tests). The same 206 pass with serde-saphyr's unreleased 2.1.0 (its master at 2454b53) patched in.lipostep succeed. Itscrossbuild succeeds for x86_64-unknown-linux-gnu, and for aarch64-unknown-linux-gnu it builds and linkscodeowners.lipoandtarblocks ran as written, minusgh release upload, and the archive listscodeownersat its root pluslicenses/with 9 files, byte-identical to the crates' own. The extracted binary runs. The linux job's upload block, minus the uploads, ran on stand-in binaries under BSD and GNU userlands, and the script also ran with uutilscp,mkdiranddirname0.10.0 and 0.8.0, which ubuntu-26.04 uses by default, all giving identical trees. DotSlash 0.5.9 fetched a new-layout archive from localhost and rancodeownersfrom its root. The new Check step exits 1 when a listed crate is missing. zizmor reports no findings, the same asmain, and actionlint and shellcheck report nothing new.nmfinds granit_parser symbols but no encoding_rs symbols in either mac binary. No Linux binary was built for these checks, since Docker wasn't working locally.Cargo.lockis exactly whatcargo buildresolves frommain's lockfile, with nocargo update: 6 packages removed, 11 added.grep "\+version"check finds nothing in this diff with BSD grep or GNU grep, so merging does not cut a release.main, in both the dev profilebundle exec rakeuses and the release profile, on Ruby 4.0.7. The pending example runs only on Ruby before 3.4. They pass the same way with serde-saphyr's unreleased 2.1.0 (its master at 2454b53) patched in. They have no emptypackage.ymlcase, but through code_ownership's Ruby API,validate!accepts an empty, BOM-only, comment-only,---,~,nullor...package.ymlon both 2.0.0 and 2.1.0.IgnoredAnywith this PR's options, caused no panics.Notes
yaml_max_nodeskey is documented in the README's configuration section.README.md,src/config.rs,src/ownership/file_owner_resolver.rsandsrc/project_builder.rs. Resolving them means keeping both newConfigfields, in the struct and in the testConfigliterals, and taking this PR'sruby_package_owner. Add allow_unowned_globs to permit files with no owner #135 also adds a config test that callsserde_yaml::from_reader, which needs switching toConfig::load_from_path, and itsallow_unowned_globsfield needsdeserialize_with = "crate::yaml::strings_or_nulls"to match the other list fields. Without it, an emptyallow_unowned_globs:fails under serde-saphyr 2.1. Merging this first means Add allow_unowned_globs to permit files with no owner #135's 0.6.0 release ships the fix.v0.5.0. Once it moves to a release with this change, those gems redistribute granit-parser too and need its notices.