Skip to content

fix(@stdlib/blas/ext/sort): validate sortOrder broadcast shape - #14748

Closed
Planeshifter wants to merge 1 commit into
developfrom
philipp/ci-fix-sort-sorthp-broadcast-sortorder-2026-08-28
Closed

fix(@stdlib/blas/ext/sort): validate sortOrder broadcast shape#14748
Planeshifter wants to merge 1 commit into
developfrom
philipp/ci-fix-sort-sorthp-broadcast-sortorder-2026-08-28

Conversation

@Planeshifter

Copy link
Copy Markdown
Member

Description

What is the purpose of this pull request?

This pull request:

  • fixes @stdlib/blas/ext/sort and @stdlib/blas/ext/sorthp (structurally identical packages) silently accepting a non-zero-dimensional ndarray sortOrder argument instead of throwing, which has been failing the Node.js v16 job on the linux_test workflow.
  • when sortOrder is provided as an ndarray and no dims option is given, main.js in 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.
  • routes sortOrder through the already-imported maybeBroadcastArray helper (already used for the dims-provided branch) in both previously-unvalidated call paths, in both packages, so a sortOrder with a non-zero-dimensional shape now throws instead of being silently accepted.

Related Issues

Does this pull request have any 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

Any questions for reviewers of this pull request?

No.

Other

Any other information relevant to this pull request? This may include screenshots, references, and/or implementation notes.

Failing run: https://github.com/stdlib-js/stdlib/actions/runs/33173011694 (job Node.js v16, workflow linux_test).

Symptom: sort's and sorthp's own test suites each report 6 failing assertions: sort(x, sortOrder) / sort(x, sortOrder, {}) do not throw when sortOrder is an ndarray with shape [4], [2,2,2], or [0] against an x of shape [2,2], dtype generic.

Root cause: in each package's lib/main.js, when sortOrder is ndarray-like and no dims option is supplied, the function assumed (per an inline comment) that sortOrder was already zero-dimensional. In the 2-argument call form it was passed straight to base() unvalidated; in the 3-argument form with no dims property on opts, there was no else branch at all — sortOrder was left completely unhandled before being forwarded to base().

Fix: route sortOrder through maybeBroadcastArray( sortOrder, [] ) in both unvalidated branches of both files — the same helper already used one branch over for the dims-provided case, just against a fixed target shape of [] instead of the non-core shape. sort and sorthp share 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-dimensional sortOrder, maybeBroadcastArray returns the identical object reference, so the happy path is unchanged; for any higher-rank sortOrder, it throws.

Validation: local node_modules are not installed in this environment, so make test/make lint-pkg could not be run directly. Three independent adversarial reviews were performed against the diff and each package's own test suite, docs, and TypeScript declarations:

  • Correctness reviewer: approved. Empirically exercised both packages directly (no test runner available) against a 60-assertion harness derived from the packages' own test files: 60/60 pass post-fix versus 48/60 pre-fix, with exactly the 12 expected pre-fix failures (6 per package) matching CI. Confirmed the zero-dimensional case is a true no-op and no other test block in either file expects success with a non-zero-dimensional sortOrder and no dims.
  • Regression-scope reviewer: approved (after one revision — see below). Confirmed only sort and sorthp are 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-scalar sortOrder without dims; no public API change.
  • Style reviewer: approved. Matches existing formatting; both files carry a file-level 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 clean develop checkout and now contains exactly the two intended files. Non-blocking: the regression-scope reviewer also noted that @stdlib/blas/ext/to-sorted and @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

Please ensure the following tasks are completed before submitting this pull request.

AI Assistance

When authoring the changes proposed in this PR, did you use any kind of AI assistance?

  • Yes
  • No

If you answered "yes" above, how did you use AI assistance?

  • Code generation (e.g., when writing an implementation or fixing a bug)
  • Test/benchmark generation
  • Documentation (including examples)
  • Research and understanding

Disclosure

If you answered "yes" to using AI assistance, please provide a short disclosure indicating how you used AI assistance. This helps reviewers determine how much scrutiny to apply when reviewing your contribution. Example disclosures: "This PR was written primarily by Claude Code." or "I consulted ChatGPT to understand the codebase, but the proposed changes were fully authored manually by myself.".

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

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
@stdlib-bot stdlib-bot added the BLAS Issue or pull request related to Basic Linear Algebra Subprograms (BLAS). label Aug 28, 2026
@stdlib-bot

Copy link
Copy Markdown
Contributor

Coverage Report

Package Statements Branches Functions Lines
blas/ext/sort $\\color{green}381/381$
$\\color{green}+100.00\\%$
$\\color{green}44/44$
$\\color{green}+100.00\\%$
$\\color{green}5/5$
$\\color{green}+100.00\\%$
$\\color{green}381/381$
$\\color{green}+100.00\\%$
blas/ext/sorthp $\\color{green}381/381$
$\\color{green}+100.00\\%$
$\\color{green}44/44$
$\\color{green}+100.00\\%$
$\\color{green}5/5$
$\\color{green}+100.00\\%$
$\\color{green}381/381$
$\\color{green}+100.00\\%$

The above coverage report was generated for the changes in this PR.

@kgryte

kgryte commented Aug 28, 2026

Copy link
Copy Markdown
Member

Isn't this a duplicate?

Copy link
Copy Markdown
Member Author

Confirmed — #14228 already covers this exact fix for sort/sorthp (and additionally circshift, which shares the same defect). That PR has been open since 2026-08-13 and is still unmerged. Closing this in favor of #14228.


Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

BLAS Issue or pull request related to Basic Linear Algebra Subprograms (BLAS).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants