fix(@stdlib/blas/ext/sort): validate sortOrder broadcast shape - #14748
Closed
Planeshifter wants to merge 1 commit into
Closed
fix(@stdlib/blas/ext/sort): validate sortOrder broadcast shape#14748Planeshifter wants to merge 1 commit into
sortOrder broadcast shape#14748Planeshifter wants to merge 1 commit into
Conversation
The job `Node.js v16` on workflow `linux_test` has failed deterministically on develop with 6 assertion failures each in `sort`'s and `sorthp`'s own test suites. Root cause: when `sortOrder` is provided as an ndarray and no `dims` option is given, `main.js` passed it straight through to the base implementation without validating that it is zero-dimensional, so a `sortOrder` with shape `[4]`, `[2,2,2]`, or `[0]` was silently accepted instead of raising an error. This commit routes `sortOrder` through the already-imported `maybeBroadcastArray` helper (already used for the `dims`-provided branch) in both `sort` and `sorthp`, which share byte-for-byte identical logic here, so that a non-zero-dimensional `sortOrder` throws, matching the function's documented "`sortOrder` must be a zero-dimensional ndarray" semantics. Ref: https://github.com/stdlib-js/stdlib/actions/runs/33173011694
Contributor
Coverage Report
The above coverage report was generated for the changes in this PR. |
Member
|
Isn't this a duplicate? |
Member
Author
|
Confirmed — #14228 already covers this exact fix for Generated by Claude Code |
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.
Description
This pull request:
@stdlib/blas/ext/sortand@stdlib/blas/ext/sorthp(structurally identical packages) silently accepting a non-zero-dimensional ndarraysortOrderargument instead of throwing, which has been failing theNode.js v16job on thelinux_testworkflow.sortOrderis provided as an ndarray and nodimsoption is given,main.jsin both packages passed it straight through to the base implementation without validating that it is zero-dimensional, contradicting each package's own documented contract ("an ndarray sort order must be a zero-dimensional ndarray") and its own test suite.sortOrderthrough the already-importedmaybeBroadcastArrayhelper (already used for thedims-provided branch) in both previously-unvalidated call paths, in both packages, so asortOrderwith a non-zero-dimensional shape now throws instead of being silently accepted.Related Issues
This pull request has the following related issues: none. Discovered via automated monitoring of scheduled CI failures on
develop, not a filed issue.Questions
No.
Other
Failing run: https://github.com/stdlib-js/stdlib/actions/runs/33173011694 (job
Node.js v16, workflowlinux_test).Symptom:
sort's andsorthp's own test suites each report 6 failing assertions:sort(x, sortOrder)/sort(x, sortOrder, {})do not throw whensortOrderis an ndarray with shape[4],[2,2,2], or[0]against anxof shape[2,2], dtypegeneric.Root cause: in each package's
lib/main.js, whensortOrderis ndarray-like and nodimsoption is supplied, the function assumed (per an inline comment) thatsortOrderwas already zero-dimensional. In the 2-argument call form it was passed straight tobase()unvalidated; in the 3-argument form with nodimsproperty onopts, there was noelsebranch at all —sortOrderwas left completely unhandled before being forwarded tobase().Fix: route
sortOrderthroughmaybeBroadcastArray( sortOrder, [] )in both unvalidated branches of both files — the same helper already used one branch over for thedims-provided case, just against a fixed target shape of[]instead of the non-core shape.sortandsorthpshare byte-for-byte identical logic here (only the function name and a doc line differ), so both are fixed in lockstep to avoid leaving one of the two broken. For an already-valid zero-dimensionalsortOrder,maybeBroadcastArrayreturns the identical object reference, so the happy path is unchanged; for any higher-ranksortOrder, it throws.Validation: local
node_modulesare not installed in this environment, somake test/make lint-pkgcould not be run directly. Three independent adversarial reviews were performed against the diff and each package's own test suite, docs, and TypeScript declarations:sortOrderand nodims.sortandsorthpare touched; the only in-repo callers (to-sorted,to-sortedhp) always pass an options object, hitting the fixed branch, and neither their docs, README, nor TypeScript types document or test a non-scalarsortOrderwithoutdims; no public API change.eslint-disable max-len, so no new per-line disable comments are needed; no hand-written error construction was added; the two files' diffs are structurally identical apart from naming.Reviewer notes: the regression-scope reviewer's first pass flagged that the working tree, at the time of that review, incorrectly also showed an unrelated modification to
@stdlib/blas/ext/circshift(a sibling fix being developed in parallel on a different branch in the same session). That was a local working-tree/session artifact, not a defect in this diff; the branch was rebuilt from a cleandevelopcheckout and now contains exactly the two intended files. Non-blocking: the regression-scope reviewer also noted that@stdlib/blas/ext/to-sortedand@stdlib/blas/ext/to-sortedhp(downstream wrappers) have their own currently-failing broadcast-compatibility tests for the same underlying reason but weren't part of any CI failure signature observed in this run's 24-hour window, so they're intentionally left out of scope here — flagged for maintainer follow-up.Checklist
AI Assistance
If you answered "yes" above, how did you use AI assistance?
Disclosure
This PR was found, diagnosed, fixed, and validated autonomously by Claude Code as part of a scheduled CI-failure-monitoring routine, including an independent three-reviewer adversarial validation pass. A human should still review before merging.
@stdlib-js/reviewers
Generated by Claude Code