fix: say which package is missing when the native binding fails#168
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
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:
📝 WalkthroughWalkthroughThe native binding loader now distinguishes unsupported targets from build targets with missing optional packages, formats attempted-load errors, and has tests covering diagnostics and explicit library-path precedence. ChangesNative binding diagnostics
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (1)
packages/stdnum/src/__test__/native-node.test.ts (1)
24-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the preserved
Tried:diagnostics.This only verifies the package name, which is also in the high-level reason. Assert the
Tried:header and an indented attempted resolution so the stated diagnostic contract cannot regress.🤖 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 `@packages/stdnum/src/__test__/native-node.test.ts` around lines 24 - 34, Update the test around loadNativeStdnumBinding to assert the preserved “Tried:” diagnostics, including the header and an indented attempted resolution entry, while retaining the existing package-name assertion.
🤖 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 `@packages/stdnum/src/__test__/native-node.test.ts`:
- Around line 77-80: Document the unavoidable NativeStdnumBinding cast in the
Proxy setup by adding a // SAFETY: comment explaining that the Proxy returns
functions for every runtime contract check performed by asNativeBinding. Keep
the existing Proxy behavior and cast unchanged.
In `@packages/stdnum/src/native-node.ts`:
- Around line 86-92: Update the reason construction in the native module loading
path to describe a matched package as one that “could not be loaded,” rather
than asserting it was not installed. Keep the unsupported-platform message for
match === undefined, and leave the specific requireModule failure cause to the
existing “Tried:” details.
---
Nitpick comments:
In `@packages/stdnum/src/__test__/native-node.test.ts`:
- Around line 24-34: Update the test around loadNativeStdnumBinding to assert
the preserved “Tried:” diagnostics, including the header and an indented
attempted resolution entry, while retaining the existing package-name assertion.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 28f06f13-d063-40e4-9a1a-325efde0702f
📒 Files selected for processing (2)
packages/stdnum/src/__test__/native-node.test.tspackages/stdnum/src/native-node.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: Bindings and packages
- GitHub Check: Pack
- GitHub Check: Oracle
- GitHub Check: Rust
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{rs,ts,tsx,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Keep Node.js, WASM, and Python bindings as thin adapters over the single Rust implementation; do not hand-write validator logic in bindings.
**/*.{rs,ts,tsx,py}: Separate validation status from presentation: kernels return compact statuses and typed error codes; bindings construct user-facing strings, objects, exceptions, and diagnostics only when required.
Keep binding calls coarse; prefer indexed dispatch and batch operations to amortize Node and Python FFI crossings.
Treat performance as a tested product contract with deterministic allocation assertions and controlled normalized oracle benchmarks; do not use noisy wall-clock unit checks.
Files:
packages/stdnum/src/native-node.tspackages/stdnum/src/__test__/native-node.test.ts
**/*.{ts,tsx,py,json,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
After adding or changing a validator, use
bun run codegento generate npm entrypoints, TypeScript and Python registry types, package exports, and README tables; do not manually maintain generated outputs.
Files:
packages/stdnum/src/native-node.tspackages/stdnum/src/__test__/native-node.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Prefer explicit values and thread backend discriminators through URL parameters, component props, and the full TypeScript frontend stack instead of hardcoding defaults.
Prefer TypeScript types that make invalid states structurally impossible, including branded types, discriminated unions, and exhaustive checks.
Use named discriminators or domain types instead of boolean fields for states that may grow.
Fail fast: validate at boundaries and return or throw early; minimize brace nesting with inverted conditions and early returns.
Use named constants instead of string literals for domain values.
Do not assign directly todocument.cookie.
Avoid spread syntax in loop accumulators; use.push().
Do not use TypeScript enums; useas constobjects or union types.
Model mutually exclusive states as discriminated unions with a stable discriminator; avoid boolean flags combined with optional payload fields.
Avoidascasts; narrow with type guards,inchecks, or records. If unavoidable, add a// SAFETY:comment explaining why it is sound.
Trace type mismatches to their source rather than casting at the consumer, and verify whether the change introduced the mismatch.
Do not annotate values the compiler already infers or pass explicit type arguments to inference-driven hooks and API calls.
Validate large-union object literals withas const satisfies Trather than a: Tannotation.
Use.at(0)when an element may be absent; use[0]only when existence is established or documented with// SAFETY:.
Prefer arrow functions over function expressions.
Destructure function parameters when the intermediate object is not reused.
Prefer discriminated-union checks such asobj.type === "x"; useinonly without an available discriminator.
Use positional parameters for one or two clear arguments; use namedOptions,Args, orParamsobjects for three or more or interchangeable arguments. ReservePropsfor React props.
Reuse utility types from React,...
Files:
packages/stdnum/src/native-node.tspackages/stdnum/src/__test__/native-node.test.ts
**/*.{ts,tsx,rs}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,rs}: Use oxlint with the Ultracite preset and oxfmt; suppress a rule only with// eslint-disable-next-line rule-name.
Use property and mutation tests for checksum behavior when the input space is large.
Files:
packages/stdnum/src/native-node.tspackages/stdnum/src/__test__/native-node.test.ts
🔇 Additional comments (1)
packages/stdnum/src/native-node.ts (1)
72-78: LGTM!
| const binding = new Proxy( | ||
| {}, | ||
| { get: () => () => undefined }, | ||
| ) as NativeStdnumBinding; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document why this cast is sound.
Add a // SAFETY: comment explaining that the Proxy returns functions for every runtime contract check performed by asNativeBinding.
As per coding guidelines, “Avoid as casts; narrow with type guards… If unavoidable, add a // SAFETY: comment explaining why it is sound.”
🤖 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 `@packages/stdnum/src/__test__/native-node.test.ts` around lines 77 - 80,
Document the unavoidable NativeStdnumBinding cast in the Proxy setup by adding a
// SAFETY: comment explaining that the Proxy returns functions for every runtime
contract check performed by asNativeBinding. Keep the existing Proxy behavior
and cast unchanged.
Source: Coding guidelines
| const reason = | ||
| match === undefined | ||
| ? `This platform is not among the build targets (${supported}).` | ||
| : `This platform IS a build target, so ${match[3]} exists but was not ` + | ||
| `installed. It is an optionalDependency: check that optional ` + | ||
| `dependencies are enabled, that ${match[3]} is not excluded by an ` + | ||
| `install policy, and that it appears in your lockfile.`; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not equate a target match with an uninstalled package.
match proves only that this target has a package entry. requireModule(match[3]) can also fail when the package is installed but cannot initialise (for example, a broken native binary), or when it fails the binding contract. Describe it as “could not be loaded” and leave the specific cause to Tried:.
Proposed fix
- : `This platform IS a build target, so ${match[3]} exists but was not ` +
- `installed. It is an optionalDependency: check that optional ` +
+ : `This platform is a build target, but ${match[3]} could not be ` +
+ `loaded. It is an optionalDependency: check that optional ` +
`dependencies are enabled, that ${match[3]} is not excluded by an ` +
`install policy, and that it appears in your lockfile.`;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const reason = | |
| match === undefined | |
| ? `This platform is not among the build targets (${supported}).` | |
| : `This platform IS a build target, so ${match[3]} exists but was not ` + | |
| `installed. It is an optionalDependency: check that optional ` + | |
| `dependencies are enabled, that ${match[3]} is not excluded by an ` + | |
| `install policy, and that it appears in your lockfile.`; | |
| const reason = | |
| match === undefined | |
| ? `This platform is not among the build targets (${supported}).` | |
| : `This platform is a build target, but ${match[3]} could not be ` + | |
| `loaded. It is an optionalDependency: check that optional ` + | |
| `dependencies are enabled, that ${match[3]} is not excluded by an ` + | |
| `install policy, and that it appears in your lockfile.`; |
🤖 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 `@packages/stdnum/src/native-node.ts` around lines 86 - 92, Update the reason
construction in the native module loading path to describe a matched package as
one that “could not be loaded,” rather than asserting it was not installed. Keep
the unsupported-platform message for match === undefined, and leave the specific
requireModule failure cause to the existing “Tried:” details.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6e9f9157b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| : `This platform IS a build target, so ${match[3]} exists but was not ` + | ||
| `installed. It is an optionalDependency: check that optional ` + | ||
| `dependencies are enabled, that ${match[3]} is not excluded by an ` + | ||
| `install policy, and that it appears in your lockfile.`; |
There was a problem hiding this comment.
When the platform package is installed but fails to load, for example because its binary is incompatible, a shared library is unavailable, or its exports fail asNativeBinding, match is still defined and this message falsely states that the package was not installed. The collected errors already distinguish these cases, so the missing-package diagnosis should only be used after confirming that resolving the platform package itself failed; otherwise users are directed toward optional-dependency and lockfile fixes that cannot resolve the actual load failure.
Useful? React with 👍 / 👎.
The platform binaries are optionalDependencies, so any installer that declines them leaves this package importable and dead, throwing only on first use. The message it threw then listed every supported target, including the one it was running on, which reads as a portability problem that does not exist and sends the reader looking in the wrong place. It now separates the two cases: a platform that is not a build target, and a platform that is one whose package was not installed. The second names the package and the usual reasons an optionalDependency goes missing. The loader had no tests despite taking every input it consults as an injectable option, which is how this stayed invisible. Both branches are now covered.
db0757c to
046b7d0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.changeset/loud-pandas-repeat.md:
- Around line 5-9: Update the release-note wording in the changeset to replace
“missing install” with “missing installation” or “missing optional package,”
while preserving the explanation that the native binding fails because an
optional platform package was not installed.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: adfd9304-12df-4cb4-b6b0-c30a0ef836b8
📒 Files selected for processing (3)
.changeset/loud-pandas-repeat.mdpackages/stdnum/src/__test__/native-node.test.tspackages/stdnum/src/native-node.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/stdnum/src/test/native-node.test.ts
- packages/stdnum/src/native-node.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: Pack
- GitHub Check: Bindings and packages
- GitHub Check: Rust
- GitHub Check: Oracle
🧰 Additional context used
🪛 LanguageTool
.changeset/loud-pandas-repeat.md
[grammar] ~9-~9: The word ‘install’ is not a noun.
Context: ...rtability problem rather than a missing install.
(A_INSTALL)
| Say which package is missing when the native binding cannot load. The platform | ||
| binaries are optional dependencies, so an installer that declines them leaves | ||
| the package importable and dead, throwing only on first use. The previous error | ||
| listed every supported target, including the one it was running on, which reads | ||
| as a portability problem rather than a missing install. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use clearer release-note wording.
“Missing install” is awkward here; use “missing installation” or “missing optional package” to describe the failure precisely.
🧰 Tools
🪛 LanguageTool
[grammar] ~9-~9: The word ‘install’ is not a noun.
Context: ...rtability problem rather than a missing install.
(A_INSTALL)
🤖 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 @.changeset/loud-pandas-repeat.md around lines 5 - 9, Update the release-note
wording in the changeset to replace “missing install” with “missing
installation” or “missing optional package,” while preserving the explanation
that the native binding fails because an optional platform package was not
installed.
Source: Linters/SAST tools
1 participant
What changed
The native loader now distinguishes between:
For supported targets, the error names the expected package and preserves the detailed
Tried:diagnostics. The loader also keeps the explicit native-library override ahead of package resolution.Tests
Added coverage for supported and unsupported targets, missing optional packages, preserved resolution diagnostics, and explicit path precedence.
Release
Includes a patch changeset for
@stll/stdnum.