Skip to content

Conversation

@cijiugechu
Copy link
Contributor

Part of #239 .

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Feb 10, 2026

Greptile Overview

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk.
  • Changes are localized to error construction/formatting and all signature/variant shape updates appear consistently applied across the repository. Added unit tests cover the new remote-manifest error compaction and reporting aggregation behavior. No unresolved callers of the old tuple-style error variants were found by repository-wide search.
  • No files require special attention

Important Files Changed

Filename Overview
packages/zpm-config/src/lib.rs Wraps YAML parse failures for user/project config with new path-aware ConfigurationError variants using EcoString messages.
packages/zpm/src/content_flags.rs Adds remote_manifest_parse_error mapping when hydrating package.json from cached package archives to improve error context.
packages/zpm/src/error.rs Introduces RemoteManifestParseError and helpers to compact remote manifest parser reasons; changes manifest/lockfile parse errors to structured variants and adds unit tests.
packages/zpm/src/fetchers/folder.rs Wraps remote manifest hydration failures from folder package archives with contextual RemoteManifestParseError.
packages/zpm/src/fetchers/git.rs Wraps remote manifest hydration failures from git package archives with contextual RemoteManifestParseError.
packages/zpm/src/fetchers/patch.rs Wraps manifest hydration errors for patch source and patched archives with RemoteManifestParseError; keeps locator context for patch source.
packages/zpm/src/fetchers/tarball.rs Wraps remote manifest hydration failures from tarball package archives with contextual RemoteManifestParseError.
packages/zpm/src/fetchers/url.rs Wraps remote manifest hydration failures from URL-fetched package archives with contextual RemoteManifestParseError.
packages/zpm/src/lockfile.rs Adds lockfile path to legacy Berry lockfile parser, updates legacy parse error variant to include path+reason, and adjusts alias locator keying.
packages/zpm/src/manifest/helpers.rs Changes manifest parse errors to store a string reason (parser first-line when applicable) instead of an opaque error object.
packages/zpm/src/project.rs Propagates lockfile path into legacy lockfile parsing and updates JSON lockfile parse error to include path+reason; updates pattern match for new variant shape.
packages/zpm/src/report.rs Adds aggregation/summarization of RemoteManifestParseError instances and corresponding tests to ensure counts are based on unique locators.
packages/zpm/src/resolvers/npm.rs Wraps hydration of npm registry manifests with RemoteManifestParseError, constructing locators for better error attribution.

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

5 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Feb 10, 2026

Additional Comments (1)

packages/zpm/src/lockfile.rs
Wrong locator inserted

In the aliased_idents branch, this inserts entry.resolution.clone() as the key and uses aliased_locator only inside the Resolution (lockfile.entries.insert(entry.resolution.clone(), ...)). This will overwrite/collide with the non-aliased entry for the same entry.resolution, and it also means the aliased locator itself never becomes an entry key. The key here should be the aliased locator (or some other unique locator for the alias), otherwise aliased resolutions can be lost or corrupted when multiple aliases exist for the same underlying package.

Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/zpm/src/lockfile.rs
Line: 297:302

Comment:
**Wrong locator inserted**

In the `aliased_idents` branch, this inserts `entry.resolution.clone()` as the key and uses `aliased_locator` only inside the `Resolution` (`lockfile.entries.insert(entry.resolution.clone(), ...)`). This will overwrite/collide with the non-aliased entry for the same `entry.resolution`, and it also means the aliased locator itself never becomes an entry key. The key here should be the aliased locator (or some other unique locator for the alias), otherwise aliased resolutions can be lost or corrupted when multiple aliases exist for the same underlying package.

How can I resolve this? If you propose a fix, please make it concise.

@github-actions
Copy link

github-actions bot commented Feb 10, 2026

⏱️ Benchmark Results

Metric Base Head Difference
Mean 2.735s 2.675s -2.18% ✅
Median 2.722s 2.673s -1.81% ✅
Min 2.603s 2.458s
Max 3.126s 2.956s
Std Dev 0.099s 0.086s
📊 Raw benchmark data

Base times: 2.909s, 3.126s, 2.825s, 2.694s, 2.701s, 2.706s, 2.687s, 2.755s, 2.752s, 2.755s, 2.653s, 2.788s, 2.637s, 2.760s, 2.654s, 2.805s, 2.660s, 2.678s, 2.769s, 2.647s, 2.776s, 2.769s, 2.661s, 2.739s, 2.670s, 2.653s, 2.741s, 2.603s, 2.766s, 2.702s

Head times: 2.956s, 2.726s, 2.673s, 2.563s, 2.661s, 2.670s, 2.592s, 2.742s, 2.458s, 2.657s, 2.682s, 2.669s, 2.727s, 2.604s, 2.579s, 2.727s, 2.736s, 2.724s, 2.713s, 2.769s, 2.683s, 2.645s, 2.710s, 2.676s, 2.742s, 2.639s, 2.665s, 2.642s, 2.674s, 2.549s


Benchmark: gatsby install-full-cold

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

13 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

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.

1 participant