Skip to content

build: guard postinstall; fix: snapshot values in doctest rule#13437

Draft
Planeshifter wants to merge 4 commits into
developfrom
claude/elegant-feynman-nh0uui
Draft

build: guard postinstall; fix: snapshot values in doctest rule#13437
Planeshifter wants to merge 4 commits into
developfrom
claude/elegant-feynman-nh0uui

Conversation

@Planeshifter

Copy link
Copy Markdown
Member

Description

What is the purpose of this pull request?

This pull request fixes two persistent CI failures found on develop in a
routine sweep of the last 24h of GitHub Actions runs. Both landed on one
branch/PR because this session was constrained to a single designated
branch; they are otherwise unrelated fixes and are kept as separate commits.

  • postinstall fails on every real npm install. The weekly
    test_published_package workflow has failed 6 consecutive runs (every
    run since 2026-06-07) with sh: 1: tools/scripts/apply_patches: not found (exit 127). package.json's postinstall hook runs
    tools/scripts/apply_patches to patch dev dependencies from
    etc/patches/, but .npmignore has excluded /tools/ (and /etc/)
    from the published npm package since 2021 — well before the
    postinstall hook was added in December 2025. Any npm install of
    @stdlib/stdlib from the registry (not a git checkout) fails outright,
    since the referenced script never exists in the installed tree. Fixed
    by guarding the hook so it no-ops when the script is absent, while
    still running (and propagating the real exit code of) apply_patches
    for contributors who install from a git checkout.

  • Spurious doctest lint failures on docs/repl.txt. lint_changed_files
    failed on a new package, stats/incr/nanmeanvar, reporting 4 "doctest"
    mismatches that all claimed the expected value was the final example's
    result, even for lines documenting earlier intermediate values. The
    documented values are mathematically correct (verified independently via
    Welford's algorithm, and corroborated by an unrelated open PR, docs: clean-up #13435,
    whose reviewers found no fault with nanmeanvar's docs). Root cause: the
    doctest lint rule (@stdlib/_tools/repl-txt/rules/doctest, used for
    every package's docs/repl.txt in CI) captured each REPL example's
    return value as a live reference (scope[varName]) rather than a
    snapshot. Accumulator-style packages such as stats/incr/meanvar
    intentionally mutate and return the same output array on every call,
    so by the time the rule compared captured values against the documented
    text, every captured reference pointed at the same, already-mutated
    final state — producing false positives for any chained-call
    accumulator example. Fixed by snapshotting each captured value with
    @stdlib/utils/copy at capture time.

Related Issues

Does this pull request have any related issues?

No.

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 runs:

Validation: No local node_modules were available in this environment
to run make lint-pkg-json or the full lint tool end-to-end (it
transitively requires third-party packages, e.g. debug, via
@stdlib/namespace). Instead:

  • postinstall: verified package.json remains valid JSON; directly
    exercised the new shell guard under sh -c in three scenarios — script
    present, script absent, and script present-but-failing — confirming exit
    0 (no-op) only when absent, and the real exit code otherwise.
  • doctest rule: verified @stdlib/utils/copy loads standalone and
    correctly deep-clones arrays/primitives/undefined/null without
    mutation leakage; reproduced the exact reference-aliasing failure mode
    (and confirmed the fix) in an isolated vm-based script mirroring the
    rule's example-execution loop; manually recomputed nanmeanvar's
    documented values via Welford's algorithm to confirm they were correct
    all along.

Three independent reviewers (correctness, regression-scope, style — two
Opus, one Sonnet) evaluated both fixes against the original job logs, root
cause, and diff. All three approved both fixes with no blocking findings.

Reviewer notes:

  • (addressed) postinstall's original A && B || true form silently
    swallowed a genuine apply_patches failure, not just its absence;
    switched to if/fi so the real exit code still propagates when the
    script is present.
  • (addressed) the doctest rule's explanatory comment was reflowed from a
    single ~180-char line to stdlib's usual ~80-column wrapping.
  • (open, non-blocking) copy() in the doctest rule is applied
    unconditionally to every captured value. For the common cases —
    primitives, plain arrays/objects, standard typed arrays, Date,
    RegExp, Error, complex scalars — this is verified safe. For a few
    exotic container types not in @stdlib/utils/copy's typed-array hash
    (e.g. Complex64Array/Complex128Array, BooleanArray, ndarray),
    cloning falls back to a documented-as-"fragile" generic instance clone.
    This could theoretically produce a false doctest failure if some other
    package's docs/repl.txt documents a chained accumulator returning one
    of those types. Low probability, and not exercisable end-to-end in this
    environment (no node_modules) — worth a maintainer spot-check against
    a docs/repl.txt using one of those types before merge.

Two further CI failures from the same 24h window were investigated and
found to be intermittent (network/timing flakes), not fixed here per the
routine's classification criteria; see the local report for detail.

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 authored by Claude Code as part of a scheduled routine that
reviews the last 24h of GitHub Actions failures on develop, classifies
them as intermittent or persistent, and proposes fixes for persistent
failures. Root cause for each fix was traced by reading source, git
history, and CI logs, and independently validated with isolated
reproduction scripts (not the full test suite, which could not be run in
this environment). Three independent Claude reviewer passes (correctness,
regression-scope, style) evaluated both fixes before this PR was opened.


@stdlib-js/reviewers


Generated by Claude Code

claude added 4 commits July 12, 2026 14:18
The weekly `test_published_package` workflow has failed every run
since 2026-06-07 with `sh: 1: tools/scripts/apply_patches: not
found` (exit 127). Root cause: commit fdbe637 added a
`postinstall` hook running `tools/scripts/apply_patches` to patch
dev dependencies, but `.npmignore` has excluded `/tools/` from the
published npm package since 2021. Any real `npm install` of
`@stdlib/stdlib` therefore fails outright, since the referenced
script never exists in the installed tree. This commit guards the
hook so it silently no-ops when the script is absent, while
preserving the original patch-application behavior for contributors
who install from a git checkout.

Ref: https://github.com/stdlib-js/stdlib/actions/runs/29173854732
The job `Lint Changed Files` on workflow `lint_changed_files` failed
on develop with spurious `doctest` errors against
`stats/incr/nanmeanvar`'s `docs/repl.txt`, reporting the final
example's return value as the "expected" value for every prior
step in the chain. Root cause: accumulator-style packages (e.g.
`stats/incr/meanvar`) intentionally mutate and return the same
output array on every call; the rule captured `actual` values as
live references into that shared array, so once the full example
had executed, all captured references pointed to the same,
already-mutated final state. This commit snapshots each captured
value with `@stdlib/utils/copy` at capture time, so earlier steps
in a REPL example chain are compared against the value they held
when captured rather than the final accumulator state.

Ref: https://github.com/stdlib-js/stdlib/actions/runs/29180627893
Follow-up to ad4bf4d per reviewer feedback: `A && B || true`
silently swallowed a genuine failure of `apply_patches` itself, not
just its absence, hiding real patch-application errors for
contributors installing from a git checkout. Switch to a plain
`if`/`fi` guard so a present-and-failing script still propagates its
exit code, while an absent script (published npm install) still
no-ops.
Follow-up to ddb4ab3 per reviewer feedback: wrap the explanatory
comment added for the `copy()` snapshot fix to stdlib's usual ~80
column width instead of a single long line.
Comment thread package.json
"check-deps": "make check-deps",
"check-licenses": "make check-licenses",
"postinstall": "tools/scripts/apply_patches"
"postinstall": "if test -f tools/scripts/apply_patches; then tools/scripts/apply_patches; fi"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@Planeshifter What is the plan now that the most recent npm version disables running postinstall scripts by default? I suppose we could run the script during make init.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Hmm, given it's a postinstall script in the project for which one would run npm install, I don't think it will be skipped. The npm change targets scripts from dependencies (packages in node_modules), not the top-level project's own scripts. And we really only need this script for local development.

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.

3 participants