FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix(@stdlib/blas/ext/circshift): validate `k` broadcast shape by Planeshifter · Pull Request #14747 · stdlib-js/stdlib · GitHub

fix(@stdlib/blas/ext/circshift): validate k broadcast shape - #14747

Draft
Planeshifter wants to merge 1 commit into
developfrom
philipp/ci-fix-circshift-broadcast-k-2026-08-28
Draft

fix(@stdlib/blas/ext/circshift): validate k broadcast shape#14747
Planeshifter wants to merge 1 commit into
developfrom
philipp/ci-fix-circshift-broadcast-k-2026-08-28

Conversation

Copy link
Copy Markdown
Member

Description

What is the purpose of this pull request?

This pull request:

  • fixes @stdlib/blas/ext/circshift silently accepting a non-zero-dimensional ndarray k argument instead of throwing, which has been failing the Node.js v16 job on the macos_test workflow deterministically on develop for weeks.
  • when k is provided as an ndarray and no dims option is given, main.js passed k straight through to the base implementation without validating that it is zero-dimensional, contradicting the package's own documented contract ("an ndarray for k must be a zero-dimensional ndarray") and its own test suite.
  • routes k through the already-imported maybeBroadcastArray helper (already used for the dims-provided branch) in both previously-unvalidated call paths, so a k 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/33173054715 (job Node.js v16, workflow macos_test; the same failure reproduces on every scheduled run of this workflow going back weeks — not a flake).

Symptom: circshift's own test suite reports 6 failing assertions: circshift(x, k) and circshift(x, k, {}) do not throw when k is an ndarray with shape [4], [2,2,2], or [0] against an x of shape [2,2].

Root cause: in lib/main.js, when k is ndarray-like and no dims option is supplied, the function assumed (per an inline comment) that k was already zero-dimensional and passed it straight to base() with no validation — in both the 2-argument call form and the 3-argument form when opts lacks a dims property.

Fix: route k through maybeBroadcastArray( k, [] ) in both unvalidated branches — the same helper already used one branch over for the dims-provided case, just against a fixed target shape of [] (zero dimensions) instead of the non-core shape. For an already-valid zero-dimensional k, maybeBroadcastArray returns the identical object reference, so the happy path is unchanged; for any higher-rank k, 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 the package's own test suite, docs, and TypeScript declarations:

  • Correctness reviewer: approved. Traced the fix through maybe-broadcast-array and broadcast-array by hand and by direct execution (no test runner available, so the module was exercised directly), confirming all 6 previously-failing assertions now throw and no currently-passing assertion in the package's test suite regresses.
  • Regression-scope reviewer: approved. Confirmed the diff touches only this one file and these two branches; no other caller in the repo passes a non-scalar k without dims; README and docs/repl.txt already document the enforced invariant; no public API change.
  • Style reviewer: approved. Matches existing formatting, delegates error construction to the existing helper (no new hand-written throw / no template-literal error messages), copyright header untouched.

Reviewer notes: non-blocking — the broadcast helper raises a generic Error rather than a more specific TypeError, and the top-of-file JSDoc @throws list doesn't enumerate the broadcast-incompatibility error (a pre-existing gap on the neighboring dims branch, not introduced by this change). Left as-is for consistency with the existing dims-provided branch.

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 `macos_test` has failed
deterministically on develop for weeks, with 6 assertion failures in
`circshift`'s own test suite. Root cause: when `k` is provided as an
ndarray and no `dims` option is given, `main.js` passed `k` straight
through to the base implementation without validating that it is
zero-dimensional, so a `k` with shape `[4]`, `[2,2,2]`, or `[0]` was
silently accepted instead of raising an error. This commit routes
`k` through the already-imported `maybeBroadcastArray` helper
(already used for the `dims`-provided branch) so that a
non-zero-dimensional `k` throws, matching the function's documented
"`k` is assumed to be a zero-dimensional ndarray" semantics.

Ref: https://github.com/stdlib-js/stdlib/actions/runs/33173054715
stdlib-bot added the BLAS Issue or pull request related to Basic Linear Algebra Subprograms (BLAS). label Aug 28, 2026

Copy link
Copy Markdown
Contributor

Coverage Report

Package Statements Branches Functions Lines
blas/ext/circshift $\\color{green}263/263$
$\\color{green}+100.00\\%$
$\\color{green}20/20$
$\\color{green}+100.00\\%$
$\\color{green}1/1$
$\\color{green}+100.00\\%$
$\\color{green}263/263$
$\\color{green}+100.00\\%$

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

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

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.

3 participants


Back | FazBrowse Home | New Git URL