fix: explicit undefined option no longer disables the recursion limits - #1
Merged
MichaelLeeHobbs merged 1 commit intoJul 17, 2026
Merged
Conversation
stderr() spread `...options` over the resolved defaults, so passing an
explicit `undefined` (e.g. `stderr(e, { maxDepth: config.maxDepth })` with
an absent config key) overwrote the default with `undefined` rather than
falling back to it.
validateOption() returns early on `undefined`, so the value passed
validation, and `checkDepthLimit` then evaluated `depth >= undefined`,
which is always false. That disabled the depth guard entirely and let
normalization recurse until the stack overflowed -- turning the library's
bounded-recursion guarantee (ADR-001, ADR-003) into a RangeError. The
`NormalizeOptionsInternal = Required<NormalizeOptions>` annotation could
not catch it, since the spread is type-compatible.
Resolve each field with `??` instead of spreading.
Note the blast radius was the normalize walker only: the display walker
reads maxDepth off the StdError instance, which independently defaults to
8, so toString() stayed bounded either way. The regression tests assert
the normalized structure accordingly.
There was a problem hiding this comment.
Pull request overview
This PR fixes a recursion-limit bypass in stderr() where explicitly passing undefined in options could overwrite default limits and disable depth guarding, leading to unbounded recursion and potential stack overflows.
Changes:
- Resolve
NormalizeOptionsInternalfields with per-option??fallback instead of spreadingoptionsover defaults. - Add regression tests to ensure explicitly
undefinedoptions fall back to defaults and that explicit values still take effect.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/stderr.ts |
Fixes option resolution so undefined no longer unsets recursion/normalization limits. |
test/stderr.test.ts |
Adds regression coverage for explicit-undefined options and default limit behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+589
to
+593
| const deepChain = () => { | ||
| let e = new Error('leaf'); | ||
| for (let i = 0; i < 5000; i++) e = new Error(`L${i}`, { cause: e }); | ||
| return e; | ||
| }; |
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.
The bug
stderr()spread...optionsover the resolved defaults:Passing an explicit
undefined— e.g.stderr(e, { maxDepth: config.maxDepth })where the config key is absent, the most natural caller pattern — overwrote the default withundefinedinstead of falling back to it.validateOption()returns early onundefined, so the value passed validation.checkDepthLimitthen evaluateddepth >= undefined, which is alwaysfalse, disabling the depth guard entirely. Normalization recursed until the stack overflowed, turning the bounded-recursion guarantee (ADR-001, ADR-003) into aRangeError.The
NormalizeOptionsInternal = Required<NormalizeOptions>annotation could not catch this — the spread is type-compatible.Reproduced against
dist/onmain:The fix
Resolve each field with
??rather than spreading. Four lines insrc/stderr.ts.Blast radius
Narrower than it first appears, and worth recording:
toString()reads maxDepth off the StdError instance (MAX_DEPTH_SYMBOL ?? StdError.defaultMaxDepth, which independently defaults to 8), not off the normalize options. Display therefore stayed bounded either way — only the normalize recursion ran unbounded.The regression tests assert the normalized structure rather than
toString()for exactly this reason; atoString()-based assertion passes even with the bug present.Tests
Four tests added under
Recursion & Depth Limiting. Two reproduce the bug (verified failing with the fix reverted, passing with it applied); two are guards that explicit values and the other two options still resolve correctly.Full suite: 313/313 passing, 100% coverage, typecheck clean.
Not included
No version bump, no release.
package.jsonuntouched at 2.2.0.