Repository navigation
perf(lockfile): byte-cursor subset parser for pnpm-lock.yaml - #912
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughA new byte-cursor "subset" YAML parser ( Changespnpm Lockfile Subset Parser and Benchmarks
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/aube-lockfile/src/pnpm/subset.rs (1)
686-699: ⚡ Quick winAdd regression tests for
overrides/timeinline-map decline behavior.Given the fallback contract, it’s worth adding explicit tests that
try_parsereturnsNoneforoverrides: {k: v}andtime: {k: v}so this doesn’t regress silently.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aube-lockfile/src/pnpm/subset.rs` around lines 686 - 699, Add two new regression test functions following the pattern of the existing declines_unknown_top_level_key test. Create a test that verifies try_parse returns None when given a lockfile string containing overrides with an inline-map format (e.g., "overrides: {k: v}"), and another test that verifies try_parse returns None for time with an inline-map format (e.g., "time: {k: v}"). These tests ensure the fallback contract is maintained and prevents silent regressions where these inline-map formats could be incorrectly accepted or silently dropped.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/aube-lockfile/src/pnpm/subset.rs`:
- Around line 155-157: The parse_string_map method is being called
unconditionally for both the overrides field (around line 155) and the time
field (around line 179) without checking whether the input is actually in
string-map form or in inline/null form. This causes inline maps like `overrides:
{foo: bar}` to be silently misparsed as empty maps. You need to add a check
before calling parse_string_map to detect if the value is in inline or null
form, and if so, skip the custom parsing to allow serde to handle it with its
default fallback behavior instead of forcing the parse_string_map call.
---
Nitpick comments:
In `@crates/aube-lockfile/src/pnpm/subset.rs`:
- Around line 686-699: Add two new regression test functions following the
pattern of the existing declines_unknown_top_level_key test. Create a test that
verifies try_parse returns None when given a lockfile string containing
overrides with an inline-map format (e.g., "overrides: {k: v}"), and another
test that verifies try_parse returns None for time with an inline-map format
(e.g., "time: {k: v}"). These tests ensure the fallback contract is maintained
and prevents silent regressions where these inline-map formats could be
incorrectly accepted or silently dropped.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: ee3f9645-bbf7-4fe0-98a8-66d777898f65
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
crates/aube-lockfile/Cargo.tomlcrates/aube-lockfile/benches/pnpm_lock_parse.rscrates/aube-lockfile/src/pnpm/mod.rscrates/aube-lockfile/src/pnpm/raw.rscrates/aube-lockfile/src/pnpm/subset.rs
Greptile SummaryAdds a byte-cursor subset parser for
Confidence Score: 5/5Safe to merge. The fast path is purely additive — any input it doesn't recognize falls through to the unchanged serde path — and the differential All previously-raised concerns have been resolved in this iteration: the empty-map guard in No files require special attention. Important Files Changed
Reviews (13): Last reviewed commit: "fix(lockfile): strip YAML inline comment..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/aube-lockfile/tests/fixtures/pnpm-kitchen-sink.yaml`:
- Around line 102-107: The bundled-pkg@1.0.0 package definition (lines 102-107)
is not referenced anywhere in the importers section or snapshot dependency
trees, making it an orphaned package entry. Either add a dependency reference to
bundled-pkg in one of the importer sections to make it part of the actual
dependency graph, or remove the entire bundled-pkg@1.0.0 package definition and
its snapshot entry if it is not needed for testing parser functionality. Ensure
that if this package is intentionally included for comprehensive parser testing,
add a comment explaining this in the fixture file.
- Around line 104-106: The bundledDependencies list in the package definition
and the snapshot section of the pnpm-kitchen-sink.yaml fixture file are
inconsistent. In the package definition around lines 104-106, the
bundledDependencies includes both '`@napi-rs/wasm-runtime`' and '`@emnapi/core`',
but the snapshot section around lines 136-137 only includes
'`@napi-rs/wasm-runtime`'. Update one of these lists to match the other so that
the bundledDependencies declaration is consistent between the package metadata
and its resolved snapshot state. Choose whichever list correctly represents the
intended fixture state and update the mismatched section accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 612dd5ef-aae6-4c9e-a516-497bc41385e9
📒 Files selected for processing (5)
crates/aube-lockfile/src/pnpm/raw.rscrates/aube-lockfile/src/pnpm/subset.rscrates/aube-lockfile/tests/fixtures/pnpm-git-resolution.yamlcrates/aube-lockfile/tests/fixtures/pnpm-kitchen-sink.yamlcrates/aube-lockfile/tests/fixtures/pnpm-v6.yaml
✅ Files skipped from review due to trivial changes (1)
- crates/aube-lockfile/tests/fixtures/pnpm-v6.yaml
|
Addressed the review feedback. Summary: Finding 1 (CodeRabbit, silent wrong-parse) — fixed. Confirmed real via a differential test: Finding 2 (Greptile) — fixed. Removed the unused Finding 3 (Greptile) — fixed. Broadened Finding 4 (Greptile) — fixed. Finding 5 (Greptile) — fixed. Finding 6 (CodeRabbit) — fixed. Added a comment to the orphaned Attribution. Credited jzon-rs (Maciej Hirsz's Validation: |
|
This PR currently has failing checks. If this continues for 7 days, it will be closed automatically. This is warning day 1 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
|
This PR currently has failing checks. If this continues for 7 days, it will be closed automatically. This is warning day 1 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
|
This PR currently has failing checks. If this continues for 7 days, it will be closed automatically. This is warning day 2 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
|
This PR currently has failing checks. If this continues for 7 days, it will be closed automatically. This is warning day 3 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
Add an additive fast-path parser for the constrained YAML dialect pnpm emits (2-space block indent, no anchors/aliases/multiline scalars, flow style only for small inline maps/seqs). It walks the bytes directly with memchr for line boundaries and the structural ':' separator, producing the same RawPnpmLockfile the serde/yaml_serde path produces. The parser is a strict subset parser: on any construct outside the recognized shape (multi-document streams, unmodeled top-level keys, unexpected indent, quoting it cannot normalize) it returns None and parse_raw_lockfile transparently falls back to the existing yaml_serde parser. Fiddly inline flow values (resolution/variants/string_or_seq) and package bodies are handed to yaml_serde so those shapes stay in lockstep with serde. Standalone behavior is preserved by construction. Adds a criterion bench (benches/pnpm_lock_parse.rs) comparing both paths; set AUBE_BENCH_LOCKFILE to point at a real lockfile. On a generated 1.1MB / 4000-package lockfile the subset path parses in ~28ms vs ~45ms for the serde path. Existing aube-lockfile tests (345 unit + 5 proptest, including the pnpm roundtrip proptest) stay green.
…npm subset parser
Add a differential test harness for the pnpm-lock.yaml subset parser:
parse a fixture both ways (subset fast path and the yaml_serde path)
and assert the resulting RawPnpmLockfile is structurally identical, or
that the subset path declined (an acceptable fallback). Equality is
compared via the {:#?} rendering since the raw types are
Deserialize-only. Covers committed fixtures and an opt-in sweep over an
external corpus directory (AUBE_PNPM_CORPUS), which fails on any
divergence.
Add fixtures exercising catalog:/catalogs:, overrides,
patchedDependencies, npm-alias deps, scoped packages,
peerDependencies(+Meta), os/cpu/libc, deprecated, bundledDependencies,
remote-tarball resolutions, empty importer {}, optional snapshots,
transitivePeerDependencies, the v6 layout, and git/ssh resolutions with
peer-paren and subpath keys. Add byte-scanner edge-case unit tests:
CRLF, tab indentation (declines), comments/blank lines, colon-bearing
values, quote styles and escapes, trailing whitespace.
Fix: the subset parser declined any importer written as an inline `{}`
(a workspace package with no dependencies), falling the whole lockfile
back to serde. Accept `{}` as an empty importer body, mirroring the
existing package/snapshot handling, so the fast path fires on
multi-package workspaces. Verified default-preserving against a 38-file
real-world corpus (pnpm lockfileVersion 9): 36 match, 2 decline
(multi-document streams), 0 divergences; the fix raised matches from 13
to 36 with no silent wrong parse.
Add real pnpm-lock.yaml fixtures harvested from pinned pnpm releases (pnpm 6 -> lv5.3, 7 -> lv5.4, 8 -> lv6.0, 9/10 -> lv9.0), covering plain, workspace, patched/overrides, and git/tarball/npm-alias deps. Differential coverage added: - cross-version match-or-decline check over the committed fixtures - pre-v9 top-level-dependency formats decline rather than misparse - v9 git/tarball/alias package keys (embedded #, @https://) fire and match serde - adversarial unit tests: snapshot dep values with peer parens, scoped peer keys, numeric/bool-looking string values, quoted bool for optional (declines), multiline block scalars (decline) Corpus sweep (differential_real_corpus) print per-file outcome. No divergences found across any pnpm/lockfile version.
…pnpm subset parser
The subset parser called the block-map/serde-delegate helpers
unconditionally for map-typed fields, ignoring an inline `{...}` value on
the header line. For `overrides`/`time`/`settings`/`catalogs`/
`patchedDependencies`/`importers`/`packages`/`snapshots` written in inline
flow form, it scanned an empty block and produced `Some({})` while serde
parsed the real entries — a silent divergence. An empty block map (`key:`
with a null body, or an empty snapshot `dependencies:`) also produced
`Some({})` where serde reads `None`.
parse_string_map now takes the inline value and declines on an inline map
or an empty result; the block-bodied top-level arms decline on any inline
value. Adds differential tests covering the inline-flow and empty-block
forms for every field touched.
Also:
- remove the unused `header` parameter from parse_seq_value
- broaden has_document_separator to catch `---\r`/`---\t`/`---#`; note it
is a fast-path hint with the structural-colon decline as the guarantee
- gate the `__bench_parse_*`/`__bench_counts` shims behind a `bench`
feature so they leave the crate's default public surface; the
pnpm_lock_parse bench requires it
- replace default_package_info's per-call `from_str("{}").expect(...)`
with `RawPackageInfo::default()` (derive Default), pinned equal to
serde's `{}` parse by a test
- comment the intentionally-orphaned bundled-pkg kitchen-sink fixture
entry
- credit jzon-rs for the byte-cursor design in the module header
…it-rate guards for pnpm subset parser Hardens the c766fc4 decline fix against two regressions: - inline_vs_block_never_diverges: a proptest (256 cases, deterministic ChaCha seed) generates a logical lockfile over the map-typed fields (overrides/time) as Null/Empty/Populated, renders it both block and inline-flow style, and asserts diff_subset_vs_serde returns Match or Declined for BOTH renderings. Fuzzes the inline-vs-block boundary so a re-introduced silent inline misparse is caught. - populated_block_forms_still_fire_per_field: pins the legitimate populated BLOCK form of overrides/time/settings/patchedDependencies/ catalogs/importers/snapshot.optionalDependencies to the fast path (fires + matches), evidencing the inline-decline fix did not regress the common-case hit rate. - map_field_null_empty_populated_track_serde: asserts the null/empty/populated decision matches serde's None/empty/populated reading (populated block fires; null body and inline {} decline; no divergence either way).
…een package and snapshot
…ique reuse The pnpm subset parser is a plain byte-cursor line scanner: it finds line boundaries with memchr and matches the single structural colon with a short scalar pass. It does not use jzon-rs's distinctive SWAR/SIMD structural-scan kernels. Reword the module doc to credit jzon-rs for the byte-cursor + decline-on-doubt shape that inspired the design, rather than claiming its techniques were adopted.
A bare `dependencies:` (no children) under an importer caused
`parse_dep_specs` to build an empty map and store `Some({})`, while the
serde path reads the `Option<BTreeMap<…>>` field as `None`. That is a
silent subset-vs-serde divergence, which the subset parser's contract
forbids (it must match serde or decline to the fallback, never produce a
different result).
Add an empty-map guard before the final `Some(map)` so the empty case
declines and serde decides, mirroring the existing guard in the sibling
`parse_string_map`. Adds a regression test with an importer carrying an
empty `dependencies:` plus a populated `devDependencies:`.
4929114 to
851988a
Compare
…et-parser # Conflicts: # Cargo.lock
Summary
Adds an additive, byte-cursor subset parser for
pnpm-lock.yamlas a fast path in front of the existingyaml_serdeparser.pnpm emits a tightly constrained dialect of YAML: 2-space block indentation, no anchors/aliases, no multi-line scalars, and flow style only for small inline maps (
resolution: {...},engines: {...}) and seqs (os: [linux, darwin]). That regularity lets a purpose-built parser walk the bytes directly — usingmemchrfor line boundaries and the structural:separator — instead of running a general YAML event stream through serde. The bulk of the work in a large lockfile is thousands of trivialkey: valuesnapshot/dependency lines, which is exactly what the byte cursor handles cheaply.Behavior preservation
The new parser is a strict subset parser. It produces the same
RawPnpmLockfilethe serde path produces, and the instant it meets anything outside the recognized shape — a multi-document stream (pnpm v11 bootstrap + project doc), an unmodeled top-level key, an unexpected indent, a quoting style it can't normalize — it returnsNoneandparse_raw_lockfiletransparently falls back to the existingyaml_serdeparser.Fiddly inline flow values (
resolution/variants/string_or_seq) and full package bodies are handed toyaml_serdefor a small reconstructed fragment, so those shapes stay in lockstep with serde rather than being re-implemented. Net result: the fast path only ever fires when it can produce a byte-identical result; everything else degrades to the original parser.Benchmark
Added
crates/aube-lockfile/benches/pnpm_lock_parse.rs(criterion) comparing both paths. SetAUBE_BENCH_LOCKFILE=/path/to/pnpm-lock.yamlto point it at a real lockfile.On a generated 1.1 MB / 4000-package lockfile (200 deps + 200 devDeps in the importer, 4000
packages:entries, 4000snapshots:):yaml_serde(existing)The bench asserts both paths agree on the parsed structure before measuring.
Tests
Existing
aube-lockfilesuite stays green: 345 unit tests + 5 proptests, including the pnpm roundtrip proptest (which now also exercises the subset parser on randomly generated lockfiles). Added four targeted unit tests for the subset module (fast-path fires on the native fixture; declines multi-doc streams; declines unknown top-level keys; structural-colon splitting respects quoting).Notes
memchrandrustc-hashadded as deps ofaube-lockfile;criterionadded as a dev-dep with the[[bench]]entry.Draft: opening for design feedback on whether the subset-parser-with-fallback approach is a direction you'd want upstream, and on the bench numbers above. Happy to adjust the subset coverage (e.g. hand-parse package bodies too) if the tradeoff is interesting.
The byte-cursor + decline-on-doubt shape (a raw
&[u8]cursor that scans for structural bytes and declines to a full parser the moment input leaves the recognized subset) was inspired by jzon-rs: https://github.com/Rajaniraiyn/jzon-rs. Its specific SWAR/SIMD structural-scan kernels are not used here — this is a plain byte-cursor line scanner that finds line boundaries withmemchrand matches the single structural:with a short scalar pass; the constrained pnpm dialect doesn't justify vectorized kernels.Summary by CodeRabbit
pnpm-lock.yamlparsing with a new fast-path for common pnpm/YAML patterns and automatic fallback to the existing parser when unsupported.