Accept duplicate package manifest fields - #718
Conversation
There was a problem hiding this comment.
Copilot wasn't able to review any files in this pull request.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ The fallback itself is sound. One scope gap and a hot-path nit worth a look.
Reviewed changes — the single-commit diff against main, plus the surrounding aube-manifest parse machinery and every other site that deserializes a package.json.
- Duplicate-key fallback in
PackageJson::parse— on anyparse_jsonfailure, re-parse throughserde_json::Valueandfrom_value, returning the original error if either step fails. - Test for last-value semantics — duplicate
name,scripts, anddependencieskeys, asserting the whole nested object is replaced rather than merged.
Last-wins is correct here for a non-obvious reason worth recording: the aube workspace enables serde_json's preserve_order (vendor/aube/Cargo.toml:62), so Map is IndexMap-backed and insert replaces the value while keeping the original key position. Nested Value subtrees whose order is load-bearing (exports conditions, most obviously) round-trip unchanged, so there is no ordering regression on the fallback path. The widened acceptance is also narrow in the right way: JSONC comments, trailing content, NaN/Infinity, and bad escapes all fail from_str::<serde_json::Value> too, so the original diagnostic — including the special "cannot contain JSON comments" message — still surfaces.
ℹ️ Duplicate keys still hard-fail for git:, file:, and tarball-URL dependencies
PackageJson::parse is not the only door into PackageJson. Four sites in aube-resolver deserialize manifest bytes straight into the struct with sonic_rs::from_slice/serde_json::from_slice, so a dependency resolved from a git clone, a file:/link:/portal: path, a yarn exec: manifest, or a bare tarball URL still fails with the same duplicate-field error this PR fixes for registry packages. Placing the fallback inside the generic parse_json instead of PackageJson::parse would close the class rather than the instance.
Technical details
# Duplicate-key tolerance is applied at one call site, not to the parse layer
## Affected sites
- `vendor/aube/crates/aube-manifest/src/lib.rs:390` — the fallback lives in `PackageJson::parse`, so only
callers of `parse` / `from_path` / `from_path_cached` benefit.
- `vendor/aube/crates/aube-resolver/src/local_source.rs:169` — `read_local_manifest`, `file:`/`link:`/`portal:`
dependencies: `sonic_rs::from_slice(&content).or_else(|_| serde_json::from_slice(&content))`.
- `vendor/aube/crates/aube-resolver/src/local_source.rs:243` — `resolve_exec_manifest`, yarn `exec:` manifests.
- `vendor/aube/crates/aube-resolver/src/local_source.rs:387` — `read_git_package_manifest`, git dependencies.
- `vendor/aube/crates/aube-resolver/src/local_source.rs:676` — `resolve_remote_tarball`, bare `https://….tgz`
dependencies; this one has no `sonic_rs` → `serde_json` retry at all.
## Required outcome
- A `package.json` with duplicate top-level keys parses identically regardless of which source kind it came
from, so `git+https://…/lzma-native` behaves the same as `lzma-native` from the registry.
## Suggested approach (optional)
- Move the `serde_json::Value` retry into `parse_json` (it is already generic over
`T: DeserializeOwned`, so the retry is generic too), leave `PackageJson::parse` as a thin delegate, and
route the four `local_source.rs` sites through `parse_json` so they also pick up the BOM strip and the
JSONC diagnostic they currently lack.
## Open questions for the human
- Is closing the whole class in scope for this PR, or is the registry path the only one you want moved now?ℹ️ Nitpicks
- The BOM strip at line 398 duplicates the one inside
parse_json(line 1376) and is the only thing making a BOM-prefixed, duplicate-keyed manifest work — no fixture covers that combination. - Nothing pins the "preserve the original diagnostic" claim from the PR description. A manifest with a
//comment asserting thecannot contain JSON commentsmessage still comes back would guard against a future fallback that swallows it.
Claude Opus ｜ 𝕏
| /// `fancy` handler renders a pointer at the offending byte. | ||
| pub fn parse(path: &Path, content: String) -> Result<Self, Error> { | ||
| parse_json(path, content) | ||
| match parse_json(path, content.clone()) { |
There was a problem hiding this comment.
This clones the entire manifest string on every parse, including the overwhelmingly common success path, to serve a fallback that essentially never fires. parse_json can only return Error::Parse(Box<ParseError>), and ParseError.src is a miette::NamedSource<String> that already owns the content — BOM-stripped, which would also make the second strip on line 398 unnecessary.
Technical details
# Avoid the per-manifest copy on the success path
## Affected sites
- `vendor/aube/crates/aube-manifest/src/lib.rs:391` — `content.clone()` runs for every manifest
`PackageJson::parse` sees, which during an install is once per dependency with lifecycle work plus every
workspace member.
- `vendor/aube/crates/aube-manifest/src/lib.rs:398` — the BOM strip becomes redundant if the retry reads the
content back out of the error, since `parse_json` strips the BOM before constructing it (line 1376).
## Required outcome
- No allocation or copy of `content` on the path where `parse_json` succeeds.
## Suggested approach (optional)
`miette::NamedSource::inner()` exists in miette 7.6 and returns `&String`, so the retry can borrow rather than
copy. Sketch — not compiled by me, so treat it as a shape rather than a patch:
```rust
let original = match parse_json(path, content) {
Ok(manifest) => return Ok(manifest),
Err(original) => original,
};
// ...existing comment...
if let Error::Parse(pe) = &original
&& let Ok(value) = serde_json::from_str::<serde_json::Value>(pe.src.inner())
&& let Ok(manifest) = serde_json::from_value(value)
{
return Ok(manifest);
}
Err(original)
```
Summary
package.jsonfiles with last-value semantics.Verification
XDG_CONFIG_HOME=<empty> make verifycargo test -p aube-manifestcargo clippy -p aube-manifest --all-targets -- -D warningslzma-native@0.0.5now reaches itsnode-gypinstall step instead of failing withERR_NUB_MANIFEST_PARSE.