Skip to content

fix: serialize an error that is the whole record, and stop redaction throwing out of the log call - #148

Merged
cuzzlor merged 10 commits into
mainfrom
fix/redact-domexception
Aug 31, 2026
Merged

fix: serialize an error that is the whole record, and stop redaction throwing out of the log call#148
cuzzlor merged 10 commits into
mainfrom
fix/redact-domexception

Conversation

@cuzzlor

@cuzzlor cuzzlor commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

Three error-handling defects. A is silent data loss; B and C are the same symptom — redaction throwing out of the logger.error(...) call itself, so the caller loses the log line and everything it meant to do next. A catch that logs and then records the failure never reaches the recording.

Each was found by working on the one before it, which is why they are together.


A. A record that is itself an Error is never serialized

logger.error(err) takes a winston branch of its own — it assigns level and the routing symbols onto the error and writes the error itself as the record:

if (msg && typeof msg === 'object') {
  msg[LEVEL] = msg.level = level
  this._addDefaultMeta(msg)
  this.write(msg)
}

message, stack and name are not own enumerable properties of an Error, so every transport that spreads or enumerates the record loses them. Measured:

before after
{ ...info } for logger.error(err) code, level code, level, message, name, stack
{ ...info } for a DOMException level code, level, message, name, stack
this library's own CallbackTransport meta code code, name, stack

Not a corner case: CallbackTransport hands meta to its callback with no stack, and a spreading transport gets no message at all. Console output was never affected — format.json's replacer catches the record on its way out — which is why it went unnoticed.

Fix. serializeErrorFormat replaces such a record with its serialized form, re-applying level and every own symbol afterwards: those are winston's routing, not error data, and a custom serializer has no reason to return them. message is left to the serializer, so one that drops it produces a record without one, exactly as it already does for a nested error. Detection is instanceof Error, so a plain info object that happens to carry message and stack is untouched.

Only the second-argument shape was already fine — winston explicitly lifts message, stack and cause for logger.error('failed', err).


B. Logging a DOMException throws out of the log call

An AbortSignal carries a DOMException as its reason, so a DOMException is what a catch block receives whenever a fetch, a stream or a job is abandoned on a deadline.

TypeError: Cannot set property message of  which has only a getter

Only redactPaths triggers it. With every other createLogger option the same call logs fine:

baseline: ok    omitPaths: ok    flatten: ok    pretty: ok    mapAuditLevelForOtel: ok
redactPaths: THREW Cannot set property message of  which has only a getter

Why

  1. logger.error(msg, { error }) — winston stores the raw metadata twice: merged onto info, and under the SPLAT symbol.
  2. serializeErrorFormat walks Object.keys(record) only, deliberately, to protect winston's routing symbols. So info.error becomes a plain object but info[SPLAT][0].error stays a live DOMException.
  3. redactValues calls cloneDeep(info), and es-toolkit's clone does copy symbols. It reaches the DOMException and takes its instanceof Error branch — structuredClone(value), then result.message = value.message. structuredClone rebuilds a DOMException as a DOMException, whose message and name are getter-only prototype accessors. The assignment throws.

It is specific to DOMException. URL, Headers, AbortSignal, Request, Response, Error and AggregateError all clone without complaint — structuredClone preserving the DOMException type is what makes it the exception.

Fix

  • redact-values.ts — clone with a customizer that substitutes the object the configured serializer builds, deep-cloned so redaction cannot write back into a serializer's cached record or into the error's own properties, and with each DOMException mapped to its substitute first so a serializer returning the error it was handed resolves rather than recursing forever. redactValuesWith takes the serializer as an optional second argument (defaulting to serializeError), redactFormat forwards it, and createLogger passes the errorSerializer it was given, so a DOMException reaches the transports in the same shape as every other error.
  • serialize-error-format.ts — replace errors under SPLAT too, so no live Error survives anywhere in the record for a later format or transport to trip over. This also covers logger.error('failed: %j', { error }), where winston builds info from SPLAT alone.

SPLAT gets its own walk rather than sharing the metadata one. The metadata walk rebuilds every object it visits as a plain record, which is right for something bound for a transport but wrong for SPLAT, which format.splat() interpolates into the message: a Date rebuilt as a plain record has no own enumerable keys, so %j would render {} in place of its value. The SPLAT walk therefore recurses through arrays and plain objects only, is gated per argument on whether an error is reachable at all, and returns the same reference for anything untouched. A container it must rebuild to replace an error inside it keeps its prototype and every own key the rebuild did not replace, descriptor intact — symbols, non-enumerable properties, an array's non-index properties.

All three call shapes are covered and each threw on main: logger.error(msg, { error }), logger.error('failed', error), and logger.error(error).


C. A cyclic record overflows the stack during redaction

Found while testing B, and pre-existing on main with no DOMException involved anywhere:

const cyclic = { authorization: 'secret' }
cyclic.self = cyclic
redactValues({ a: cyclic }, 'authorization')   // main: RangeError: Maximum call stack size exceeded

redactValues deep-clones its input, and cloneDeep faithfully reproduces the cycle — but the walk that applies the redaction paths to the clone has no cycle guard, so it never returns. logger.error('x', cyclic) with redactPaths configured has always been a RangeError out of the log call.

Fix. A WeakSet in the redaction walk. A node is visited once whether it was reached through a cycle or shared by two branches; redaction writes the same value either way, so visiting it again was never doing anything.

This is independent of A and B — it needs no DOMException and no whole-record error — so it is the one piece that could be split out if you would rather land it separately.

Included here rather than deferred because it is the same defect as B — redaction throwing out of the log call — and because leaving it would make this PR's own claim that redaction never throws on the record it is given untrue.


Verification

  • 105 tests pass (70 existing, 35 new), eslint --max-warnings 0 and tsc clean, full npm run build green including attw and the CJS smoke test.
  • Reverting each changed source file to main one at a time, to check the tests actually pin the behaviour rather than merely passing alongside it:
reverted result
src/redact-values.ts 10 failed
src/serialize-error-format.ts 9 failed
src/redact-format.ts 1 failed
src/index.ts all 105 pass

The last one is honest rather than covered. index.ts only passes errorSerializer through to redactFormat, and once A lands, serializeErrorFormat leaves no live error for redaction to substitute anywhere in the createLogger pipeline — so the line has no observable effect through the logger. It stays, commented as such, because it matters for a caller composing redactFormat themselves, and it keeps the shapes aligned if a future path ever does let one through. That standalone path is what the new redact-format.spec.ts covers.

  • End-to-end against the built dist, with a real AbortSignal.timeout() rejection and a downstream consumer's deployed logger config:
{"deliveryId":"abc","email":"<redacted>","error":"{\"name\":\"TimeoutError\",\"message\":\"The operation was aborted due to timeout\",...,\"code\":23}",...}
>>> reached the code after the log call

The record is written, redaction still applies, and execution continues past the log call.

Commits

965dbdd B: the DOMException fix
f1e8116 clears 25 pre-existing audit advisories that were failing CI before it reached lint, types or test — all dev-only, .nsprc stays empty. Kept separate so the 700-line lockfile diff does not bury the review
0df8681 review 1: the SPLAT walk made non-destructive, errorSerializer threaded into redaction, README wording on AbortSignal corrected
dfaea57 comments left stale by the review fixes
0ebee6b B: cover a DOMException passed as the second argument
0de2406 A: the whole-record error fix
84ee15e review 2: stop mutating the serializer's return value, cycle-aware SPLAT memoization, README example corrected
6675a20 review 3: deep-clone the serialized record, gate the SPLAT walk per argument, and C
4cfaccc cover redactFormat composed on its own, after checking which reverts actually fail
34b7a4c review 4: a rebuilt SPLAT argument keeps its prototype and every own key the rebuild did not replace

Notes

  • 2.1.0, not a patch. A record that is itself an Error now reaches transports as a plain object rather than an Error instance. A custom transport that tested info instanceof Error should read the serialized name / message / stack instead. redactValuesWith and redactFormat also gain optional additive arguments. Both are called out in the README under Error serialization.
  • README gains a DOMException subsection, an accurate account of the three ways an Error reaches a log call, and an upgrade note.
  • Consumers who hand-rolled a DOMExceptionError conversion before logging can drop it after upgrading.

🤖 Generated with Claude Code

`AbortSignal.timeout()` rejects with a `DOMException`, and logging one threw a
TypeError out of the `logger.error(...)` call itself, so the caller lost the log
line and everything it meant to do next.

`redactValues` deep-clones the record before it redacts it. es-toolkit clones an
`Error` with `structuredClone` and then re-assigns `message` and `name`, but
`structuredClone` rebuilds a `DOMException` as a `DOMException`, whose `message`
and `name` are getter-only prototype accessors. The assignment throws. It is
specific to `DOMException`: `URL`, `Headers`, `AbortSignal`, `Request`,
`Response`, `Error` and `AggregateError` all clone without complaint.

Substitute the plain object `serializeError` already builds before cloning, and
carry own symbols across so a `DOMException` given to the logger as the whole
record keeps winston's `LEVEL` and `SPLAT` routing symbols.

`serializeErrorFormat` now walks `SPLAT` as well. Winston keeps the raw metadata
argument there in addition to merging its properties onto `info`, so an `Error`
logged as `logger.error(msg, { error })` is reachable twice. Serializing only the
string keys left the live `Error` under `SPLAT` for every later format to trip
over, which is how the `DOMException` reached the clone. It also covers
`logger.error('failed: %j', { error })`, where winston builds `info` from `SPLAT`
alone.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@cuzzlor

cuzzlor commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator Author

CI is red on a pre-existing audit failure, not on this change

node-ci fails at the npm run audit step, which runs before test and build — so nothing in this PR was actually exercised by CI.

  • package-lock.json is byte-identical to main (git diff origin/main -- package-lock.json is empty), so npm run audit fails the same way on main.
  • All 25 advisories are dev-only — vite, postcss, nanoid, shell-quote, esbuild, js-yaml, brace-expansion, fast-uri, from the vitest/eslint/rollup toolchain. npm audit --omit=dev reports 0 vulnerabilities.
  • .nsprc is {}, and the last CI run on this repo was 2026-05-07, so this is ~4 months of accumulated advisory drift.

Locally, on this branch: 78 tests pass (70 existing, 8 new), eslint --max-warnings 0 clean, tsc clean, and the full npm run build is green including attw and the CJS smoke test.

Worth noting that publish.yml runs the same node-ci with needs: ci, so 2.0.3 will not publish until the audit is dealt with — either by bumping the dev toolchain or by adding exceptions to .nsprc. Both feel like a separate PR.

`npm run audit` reported 25 advisories across 8 packages, all of them dev-only
(`npm audit --omit=dev` was already clean) and all reachable within the existing
semver ranges. `npm audit fix` resolved seven:

  brace-expansion  5.0.5, 1.1.14 -> 5.0.9, 1.1.18
  fast-uri         3.1.0         -> 3.1.6
  js-yaml          4.1.1         -> 4.3.2
  nanoid           3.3.11        -> 3.3.18
  postcss          8.5.10        -> 8.5.26
  shell-quote      1.8.3         -> 1.10.0
  vite             8.0.8         -> 8.2.2

The eighth, esbuild, needed a direct bump: the advisory is fixed in 0.28.1, and
`vite@8.2.2` already allows `^0.27.0 || ^0.28.0`, but `tsx@4.21.0` pins
`~0.27.0`. `tsx@4.23.13` moves to `~0.28.0`, so esbuild resolves to 0.28.2.

The `postcss` override floor moves from `^8.5.10` to `^8.5.23`, the first
release outside the advisory range, so a resolution from scratch cannot land
back below the fix.

`.nsprc` stays empty — no advisory needed an exception.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@cuzzlor

cuzzlor commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator Author

CI is green

f1e8116 clears the audit. All 25 advisories were dev-only (npm audit --omit=dev was already clean) and all reachable within the existing semver ranges — .nsprc stays empty, so nothing is suppressed.

npm audit fix resolved seven inside their current ranges:

package before after
brace-expansion 5.0.5, 1.1.14 5.0.9, 1.1.18
fast-uri 3.1.0 3.1.6
js-yaml 4.1.1 4.3.2
nanoid 3.3.11 3.3.18
postcss 8.5.10 8.5.26
shell-quote 1.8.3 1.10.0
vite 8.0.8 8.2.2

The eighth, esbuild, needed a direct bump. The advisory is fixed in 0.28.1 and vite@8.2.2 already allows ^0.27.0 || ^0.28.0, but tsx@4.21.0 pins ~0.27.0. tsx@4.23.13 moves to ~0.28.0, so esbuild resolves to 0.28.2.

The postcss override floor also moves from ^8.5.10 to ^8.5.23, the first release outside the advisory range, so a resolution from scratch cannot land back below the fix.

CI now passes audit, lint, check-types and test (78 passed). pr.yml runs with run-build: false, so the build only runs on publish — verified locally: npm run build green including attw and the CJS smoke test, and npm ci confirms the lockfile is in sync with package.json.

@cuzzlor
cuzzlor requested review from mderriey and robdmoore and a balanced review from Copilot August 31, 2026 02:22

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Fixes logging failures when redaction encounters DOMException values.

Changes:

  • Serializes errors stored under Winston’s SPLAT symbol.
  • Safely clones and redacts DOMException values with regression tests.
  • Updates documentation, package version, and tooling dependencies.

Reviewed changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/serialize-error-format.ts Adds SPLAT traversal.
src/serialize-error-format.spec.ts Tests SPLAT serialization.
src/redact-values.ts Adds safe DOMException cloning.
src/redact-values.spec.ts Tests cloning, redaction, and symbols.
src/index.spec.ts Adds logger-level regression tests.
README.md Documents DOMException logging.
package.json Bumps release and tooling versions.
package-lock.json Refreshes resolved dependencies.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/serialize-error-format.ts Outdated
Comment thread src/redact-values.ts Outdated
Comment thread README.md Outdated
cuzzlor and others added 4 commits August 31, 2026 10:35
Three findings from review, all confirmed against the code.

The `SPLAT` walk was destructive. The main walk rebuilds every object it visits
as a plain record, which is right for metadata bound for a transport but wrong
for `SPLAT`: `format.splat()` interpolates those values into the message, and a
`Date` rebuilt as a plain record holds no own enumerable keys, so
`logger.info('%j', date)` rendered `{}` in place of the ISO value. Replace
errors under `SPLAT` with a separate walk that recurses through arrays and plain
objects only, rebuilds a container only when it really holds an error, and
returns the same reference for anything untouched.

Redaction ignored a configured `errorSerializer`. The substitution for a
`DOMException` hard-coded `serializeError`, so with both `redactPaths` and an
`errorSerializer` a `DOMException` reached the transports in the default shape
while every other error used the consumer's. `redactValuesWith` now takes the
serializer as an optional second argument, `redactFormat` forwards it, and
`createLogger` passes the one it was given.

The README misstated the platform API: `AbortSignal.timeout()` returns a signal,
it does not reject. An operation cancelled through the signal rejects with the
signal's `reason`, which is the `DOMException`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Moving `replaceErrors` above `serializeErrorFormat` left the format's doc block
attached to `isPlainObject`. Put it back on the export it describes.

`redact-values.ts` still named `serializeError` as what it substitutes, which
stopped being true once the configured serializer was threaded through, and
`errorSerializer` in `CreateLoggerOptions` did not mention that `redactFormat`
now uses it.

Two comments repeated the wording review corrected in the README:
`AbortSignal.timeout()` returns a signal, it does not reject.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`logger.error('failed', error)` takes a different winston path from both shapes
already covered: with a message and a non-plain second argument, winston lifts
`message`, `stack` and `cause` onto a fresh info object and keeps the raw error
under `SPLAT` — which is where the clone used to find it and throw. It fails on
`main` with the same TypeError, and no log line is written.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`logger.error(err)` takes a winston branch of its own: it assigns `level` and the
routing symbols onto the error and writes the error as the record. `message`,
`stack` and `name` are not own enumerable properties of an `Error`, so every
transport that spreads or enumerates the record loses them. It is not a corner
case — this library's own `CallbackTransport` hands `meta` to its callback
without a stack, and `{ ...info }` yields no message at all.

`serializeErrorFormat` now replaces such a record with its serialized form,
re-applying `level` and every own symbol afterwards: those are winston's
routing, not error data, and a custom serializer has no reason to return them.
`message` is left to the serializer, so one that drops it produces a record
without one, exactly as it already does for a nested error. Detection is
`instanceof Error`, so a plain info object that happens to carry `message` and
`stack` is untouched.

Minor rather than patch: a custom transport that received an `Error` instance
for this call shape now receives a plain object.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@cuzzlor cuzzlor changed the title fix: log a DOMException instead of throwing from redaction fix: serialize an error that is the whole record, and stop a DOMException throwing out of the log call Aug 31, 2026
@cuzzlor
cuzzlor requested a balanced review from Copilot August 31, 2026 02:58
@cuzzlor

cuzzlor commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator Author

Scope expanded to two fixes

Retitled, and the description is rewritten around A) a record that is itself an Error is never serialized and B) logging a DOMException throws out of the log call.

A came out of the review thread on redact-values.ts, where I had argued the root gap deserved its own PR. Measuring it changed the answer — CallbackTransport has been handing meta to its callback with no stack, and { ...info } yields no message at all, so it is a live defect rather than a tidiness point. 0de2406 fixes it in serializeErrorFormat.

Version moves to 2.1.0: a record that is itself an Error now reaches transports as a plain object rather than an Error instance, so a custom transport testing info instanceof Error needs the serialized name / message / stack instead. README has an upgrade note.

Two earlier comments on this PR are superseded and left as they stand for the record: the first said the audit belonged in a separate PR (it is in f1e8116 here), and the redact-values.ts reply declined the root fix (taken, in 0de2406, with a follow-up on that thread).

89 tests, CI green.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

README.md:309

  • logger.log()'s one-argument overload expects a complete log entry with its own level; passing a bare Error does not select the info level or exercise the whole-record path covered by this PR. Use a level helper such as logger.error(error) so this example is executable and accurately demonstrates the changed behavior.
logger.log(new Error('cause')) // { ...info } is { level: 'info' } — no message, no stack

Comment thread src/serialize-error-format.ts Outdated
Comment thread src/redact-values.ts Outdated
Comment thread src/serialize-error-format.ts Outdated
Three findings, all reproduced before fixing.

Both places that stamp winston's routing back onto a serialized error mutated
the serializer's return value. A serializer is free to return a frozen record,
or a cached one shared between calls, and stamping either throws out of the log
call — the very failure this PR exists to fix:

  logger.error(new TypeError('boom'))
  TypeError: Cannot add property level, object is not extensible

Copy first, in `serializeRecord` and in `plainDomException`.

The `SPLAT` walk returned the source container on a back-edge, so a rebuilt
cyclic branch linked back to the unprocessed original and a live error stayed
reachable through the cycle. Replace the `WeakSet` with a `Map` from source to
replacement, which resolves a back-edge to the replacement and, as a bonus,
keeps a container reached twice without a cycle shared.

The README example used `logger.log(err)`, whose one-argument overload wants a
complete entry carrying its own `level`. A bare error there is dropped without
being logged at all, so it demonstrated nothing. `logger.error(err)` is the
shape the section is about.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@cuzzlor

cuzzlor commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator Author

Second review round addressed in 84ee15e

All three inline comments were valid and are fixed, plus the suppressed README one. Each was reproduced before being fixed.

  • Mutating the serializer's return value, in both serializeRecord and plainDomException. A frozen result made logger.error(new TypeError('boom')) throw Cannot add property level, object is not extensible — the exact failure this PR exists to remove. A cached record shared between calls would have been polluted with another log's routing, too. Both copy first now.
  • Back-edges in the SPLAT walk left a live error reachable through a cycle. WeakSet replaced with a Map from source to replacement, so a rebuilt branch closes on itself. Shared references now stay shared as well.
  • The README example used logger.log(err), whose one-argument overload wants a complete entry carrying its own level. Measured: a bare error there is dropped without being logged at all, so the example demonstrated nothing. Now logger.error(err).

93 tests (70 existing, 23 new). I confirmed the 4 new ones fail against the unfixed code, and npm run build is green including attw and the CJS smoke test.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.

Comment thread src/redact-values.ts Outdated
Comment thread src/serialize-error-format.ts Outdated
Redaction wrote into the values a serializer returned. `plainDomException`
spread the serialized record shallowly, so its nested values stayed shared with
whatever the serializer handed back — a cached record, or a reference to the
error's own properties. Redaction then wrote through them:

  serializer returns a cached { context: { authorization: 'secret' } }
  → cached.context.authorization becomes '<redacted>'

The substitution is now built by deep-cloning the serialized record through the
same customizer, with each `DOMException` mapped to the object standing in for
it, so a serializer that puts the error back into its own output resolves to
that substitute instead of recursing forever.

The `SPLAT` walk rebuilt a cyclic argument even when it held no error: a
back-edge always resolves to something that is not the source, which counts as a
change. That cost an unrelated argument its identity and every symbol and
non-enumerable property on it. Gate the walk per argument on whether an error is
reachable at all.

Found while testing the above, and fixed here because this PR claims redaction
never throws on the record it is given: the redaction walk had no cycle guard,
so any cyclic record overflowed the stack — on `main` too, with no
`DOMException` involved. `logger.error('x', cyclic)` with `redactPaths` was a
RangeError out of the log call.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@cuzzlor

cuzzlor commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator Author

Third review round addressed in 6675a20, plus a pre-existing stack overflow it uncovered

Both inline comments were valid and reproduced before fixing.

  • Redaction wrote into the serializer's nested values. The shallow spread left them shared, so a cached { context: { authorization: 'secret' } } came back '<redacted>' — the caller's object corrupted, and this API's contract of returning a new object broken. Now deep-cloned through the same customizer, with each DOMException mapped to its substitute first so a serializer returning the error it was handed resolves rather than recursing forever.
  • The SPLAT walk rebuilt cyclic arguments holding no error, costing them their identity, symbols and non-enumerable properties. Gated per argument on a cycle-aware holdsError pass.

And one that was not asked about. Testing the first fix showed the redaction walk has no cycle guard at all, so any cyclic record overflows the stack — on main too, with no DOMException anywhere near it:

redactValues({ a: cyclic }, 'authorization')   // main: RangeError

logger.error('x', cyclic) with redactPaths has always been a RangeError out of the log call. That is the same defect this PR exists to remove, and leaving it would have made the PR's own claim that redaction never throws on the record it is given untrue, so it is fixed here rather than deferred. A shared node is visited once whether reached through a cycle or by two branches — redaction writes the same value either way.

98 tests (70 existing, 28 new). The 5 newest fail against the unfixed code, npm run build green including attw and the CJS smoke test.

@cuzzlor cuzzlor changed the title fix: serialize an error that is the whole record, and stop a DOMException throwing out of the log call fix: serialize an error that is the whole record, and stop redaction throwing out of the log call Aug 31, 2026
Checking the claim that reverting any one piece of these fixes breaks a test
showed it was not true of `redact-format.ts` or `index.ts`. Both carry the
`errorSerializer` through to redaction, and once a record that is itself an
error is serialized, `serializeErrorFormat` leaves nothing for redaction to
substitute in the `createLogger` pipeline at all.

`redactFormat` is a public export, though, and composed on its own it is the
only thing between a `DOMException` and a deep clone that cannot rebuild one.
That path had no test; it has one now, along with the rest of the format's
behaviour.

The pass-through in `createLogger` stays, with a comment saying it is for
consistency rather than effect. It matters for a caller composing `redactFormat`
themselves, and it keeps the shapes aligned if a future path ever does let a
live error reach redaction.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 10 out of 11 changed files in this pull request and generated 2 comments.

Comment thread src/serialize-error-format.ts Outdated
Comment thread src/serialize-error-format.ts
Rebuilding a container to replace an error inside it dropped everything
`Object.keys` does not report. Measured before the fix:

  object:    symbol=undefined  nonEnumerable=undefined
  nullProto: proto=Object.prototype
  array:     extra=undefined   symbol=undefined

The doc comment claims the walk substitutes errors and changes nothing else, so
that was an overclaim: a custom format reading `SPLAT` saw more than the error
substitution.

A rebuilt object is now created on the source's own prototype, and both branches
copy across every own key the walk did not visit, descriptor intact — symbols,
non-enumerable properties, and an array's non-index properties. `length` is
skipped, since an array manages its own.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@cuzzlor

cuzzlor commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator Author

Fourth review round addressed in 34b7a4c

Both comments were valid, and reproduced before fixing:

object:    symbol=undefined  nonEnumerable=undefined
nullProto: prototype was Object.prototype
array:     extra=undefined   symbol=undefined

A rebuilt object is now created on the source's own prototype, and both the object and array branches copy across every own key the walk did not visit, descriptor intact — so a non-enumerable property comes back still non-enumerable. length is skipped, since an array manages its own.

The doc comment was as much the problem as the code: it claimed the walk substitutes errors and changes nothing else. It now says what the code actually guarantees — an untouched argument comes back as the same reference, and a container that must be rebuilt keeps its prototype and every own key the rebuild did not replace.

105 tests (70 existing, 35 new); the 3 newest fail against the unfixed code.

@cuzzlor
cuzzlor merged commit 1b7c2ad into main Aug 31, 2026
1 check passed
cuzzlor added a commit that referenced this pull request Sep 1, 2026
* fix: hoist an error winston nested under `message`

`logger.error(err)` on an error whose `message` is empty loses the error's name,
stack and cause in the pretty console, which prints `[object Object]`.

Winston's single-argument hot path (`winston/lib/winston/create-logger.js:78`)
reads `msg && msg.message && msg || { message: msg }`, so the branch is decided
purely by whether the message is truthy: a truthy one makes the error the
record — the case fix A in #148 handled — and an empty one nests it as
`{ message: err }`. A bare `new Error('')` does it, and so does an
`AggregateError` whose detail is all in `errors`.

In the nested case the record is not an `Error`, so the root-error branch never
fires. The walk serialises the nested error correctly but leaves it under
`message`, where `prettyConsoleFormat` interpolates an object, and `format.json`
keeps every field but emits `message` as an object where a log query expects a
string.

`hoistWrappedError` serialises it and spreads the result onto the record before
the walk runs, so `message` is a string and `name`/`stack` are siblings whichever
branch winston took. Only `message` comes from the serialiser; every other key
the caller already set wins, so `{ message: err, requestId }` keeps its
`requestId`. Not `serializeRecord`: that lifts `level` and the routing symbols
off the error, and here they are on the record already.

The same shape can be passed deliberately rather than built by winston, and
there is no way to tell the two apart, so both are hoisted — a caller who puts
an `Error` in `message` wants it logged as an error either way.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* feat: nest serialised error detail under `error`

`logger.error(err)` put the error's fields at the top of the record. That puts
the error in the same namespace as `level`, `defaultMeta` and the caller's own
metadata, and the two overlap on `name`, `message`, `stack`, `code`, `cause` and
`errors`. Either precedence loses something real. With
`defaultMeta: { name: 'my-service' }`:

  logger.error(new Error('boom'))
  { name: 'my-service', message: 'boom', stack: 'my-service: boom\n    at …' }

The error's name is gone, and the stack header reads `my-service: boom` — V8
formats `stack` lazily on first access, and winston assigns `defaultMeta` onto
the record before any format runs, which in this branch is the error itself.

Nesting removes the overlap instead of arbitrating it. Both winston branches now
produce the shape a nested error already had, so error detail is always at
`error` and `message` is always a string:

  logger.error(new Error('boom'))         { message: 'boom', error: { name, message, stack } }
  logger.error(new Error(''))             { message: '',     error: { name, message, stack } }
  logger.error('failed', { error: err })   { message: 'failed', error: { name, message, stack } }

That last one is unchanged, and is how most callers already log an error, so
`error.stack` is now the single path to error detail however it was logged.

Two branches, one shape. `serializeRecord` carries the error's own enumerable
keys to record level — an `Error`'s intrinsic fields are non-enumerable, so its
own enumerable keys are the record side of the merge winston made — and nests
the serialised error. `hoistWrappedError` covers the other side of
`winston/lib/winston/create-logger.js:78`, where an empty message makes the
record `{ message: err }`, and nests it in the same place.

`message` is still set from the serialiser, because it is winston's slot rather
than error data: `format.printf` and `prettyConsoleFormat` interpolate it, and an
object there is the `[object Object]` this started as. A serialiser that drops
`message` yields `''`.

Minor rather than patch: 2.1.0's top-level `name`/`message`/`stack` shape is one
release old, and this is the cheapest moment to correct it. README documents the
migration.

Known and unchanged: in the whole-record branch, `defaultMeta` keys are
indistinguishable from properties the thrower attached, so they are carried to
record level *and* seen by the serialiser, and a colliding key still poisons the
nested error's `name` and stack header. Not fixable from inside a format.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* feat: normalise an `Error` argument before winston branches on it

`logger.error(err)` and `logger.error('failed', err)` now produce the same
record. Winston decides what to do with an `Error` argument in three separate
places and the results have nothing in common:

  logger.error(err)             the record IS the error
  logger.error(new Error(''))   { message: err } — the branch turns on a truthy message
  logger.error('failed', err)   { message: 'failed ' + err.message, stack }, error under SPLAT
  logger.error('failed', { err }) { message: 'failed', error: err }

A format only sees what winston has already built. By then the first shape has
had `level` and `defaultMeta` assigned onto the error instance, and the third has
had the error's message concatenated onto the caller's — recoverable, if at all,
only by guessing. So `createLogger` now rewrites the arguments instead, and
winston only ever sees the last shape, which needs no repair.

`normalizeErrorArgs` moves an `Error` from the message or metadata position into
`{ error }`, keeping the caller's message (or, for an error passed alone, its own)
and merging with metadata already there. Nothing is dropped: a non-plain-object
in the metadata position has `{ error }` inserted before it. Interpolation is
untouched, and an `Error` past the metadata position is a splat value, left to
`serializeErrorFormat`.

`withNormalizedErrorArgs` installs it on the level methods and `log` as own
properties, shadowing the prototype methods they call, so the rest of the winston
API is untouched. `child` needs no wrapping: winston builds a child with
`Object.create(logger, { write })`, so these own properties are already on its
prototype chain, and `this` is forwarded so a wrapped method called on a child
still writes through the child.

This closes the caveat from the previous commit. The error is never written as
the record, so winston never assigns `defaultMeta` onto it, so a colliding
`defaultMeta.name` can no longer take the error's name or — V8 formats `stack`
lazily on first access — rewrite its stack header to `my-service: boom`.

`serializeErrorFormat` keeps handling both record shapes. It is exported for use
outside `createLogger`, and `logger.write`, winston's exception handlers and the
object form of `log` all reach it without passing through a level method.

Behaviour change beyond the record shape: `logger.error('msg', err)` no longer
has the error's message concatenated onto yours and no longer copies `stack` to
the top of the record.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix: keep an error in the splat position when the message holds a token

Winston reads the arguments after the message as interpolation values rather than
metadata when the message holds a `util.format` token, and merges nothing onto the
record for such a call. Normalising `logger.error('failed: %s', err)` to
`('failed: %s', { error: err })` therefore produced no top-level `error` at all,
and put a wrapper object where `format.splat()` expects the error.

Those calls are left alone now, so they keep winston's splat semantics: the error
stays in the position the caller passed it, serialized under `SPLAT` where the
format finds it. Reported by Copilot on #149.

An error in the *message* position gets the same guard for a different reason: its
own message can hold a token by accident — `new Error('bad format: %s')` — which
would have winston read the `{ error }` just added as an interpolation value and
drop it. Those go over as `{ message: err }` instead, the shape
`serializeErrorFormat` already nests, and one winston cannot resolve back to a
record that is the error.

Also from the same review: the README's `logger.error('failed', { error })`
example referenced an undeclared binding, so it could not be copied or
type-checked. Inlined `new Error('cause')` like the lines around it, and corrected
"three results" to four.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* docs: correct the claims the interpolation carve-out invalidated

The carve-out in the previous commit left two statements overclaiming. The
README said an `Error` "always" reaches winston as `{ error }` metadata and
"never" as the record or the message, which a tokened message now contradicts on
both counts. And `normalizeErrorArgs`'s own doc introduced a four-row table as
"three different places" — the same slip Copilot caught in the README, where the
first two rows are in fact one expression resolving two ways.

Also: two `{@link serializeErrorFormat}` references in a module that imports
nothing, so neither could resolve; plain code spans instead.

`log` is skipped in the level loop. Winston declines to define a level named
`log`, so `target.log` is `Logger.prototype.log` and the block below already
wraps it with the message at index 1; the level loop would wrap it again with the
message at index 0. Both passes together happen to be idempotent for every shape
that reaches them, so this is a second pointless pass rather than a bug — but
reading the same method twice invites one. The accompanying test pins the message
position, which is the part that would actually break.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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