🔊 fix: Preserve Log Metadata on Console for Warn/Error Levels#12737
🔊 fix: Preserve Log Metadata on Console for Warn/Error Levels#12737danny-avila merged 14 commits intodevfrom
Conversation
The default console formatter discarded every structured metadata key on the winston info object — only `CONSOLE_JSON=true` preserved it. That meant failures emitted by the agents SDK (e.g. "Summarization LLM call failed") reached stdout without the provider, model, or underlying error attached, leaving users unable to diagnose the root cause. - Add `formatConsoleMeta` helper to serialize non-reserved metadata as a compact JSON trailer, with per-value string truncation and safe handling of circular references. - Append the metadata trailer to warn/error console lines; info/debug behavior is unchanged. - Relax `debugTraverse`'s debug-only gate so warn/error messages routed through the debug formatter also surface their metadata. - Add a `formatConsoleMeta` stub to the shared logger mock so existing tests keep working.
|
@codex review |
There was a problem hiding this comment.
Pull request overview
This PR improves diagnosability of console logs by preserving structured Winston info metadata for warn/error output in the default (non-JSON) console formatter, aligning console behavior more closely with the existing JSON console mode.
Changes:
- Add
formatConsoleMeta()inapi/config/parsers.jsto serialize non-reserved log metadata into a compact JSON trailer with truncation and safe failure behavior. - Update
api/config/winston.jsconsole formatter to append serialized metadata forwarn/errorlines (while keepinginfo/debugoutput unchanged and preserving error redaction). - Add Jest coverage and mocks to support and validate the new metadata formatting behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| api/config/parsers.js | Adds metadata extraction/serialization helper and adjusts debugTraverse gating logic. |
| api/config/winston.js | Appends formatted metadata to default console output for warn/error. |
| api/test/mocks/logger.js | Extends parsers mock to include formatConsoleMeta. |
| api/config/tests/parsers.spec.js | Adds unit tests for formatConsoleMeta. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (level !== 'debug') { | ||
| const levelStr = typeof level === 'string' ? level : String(level); | ||
| const isErrorOrWarn = levelStr.includes('error') || levelStr.includes('warn'); | ||
| if (level !== 'debug' && !isErrorOrWarn) { |
There was a problem hiding this comment.
In debugTraverse, the gate uses level !== 'debug' even though you normalize levelStr right above. If level is non-string (or gets decorated in the future), this check can incorrectly treat debug logs as non-debug and skip metadata printing. Use the normalized levelStr for the comparison (or normalize once and only use that variable) to keep the gating consistent with the includes() checks.
| if (level !== 'debug' && !isErrorOrWarn) { | |
| if (levelStr !== 'debug' && !isErrorOrWarn) { |
|
Codex Review: Didn't find any major issues. You're on a roll. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
The warn-level console output now includes a metadata trailer that may contain provider-returned error strings with embedded tokens or keys (e.g. `Bearer ...`, `sk-...`). Apply `redactMessage` to warn lines in addition to error, matching the new surface area.
GitNexus: 🚀 deployedThe |
GitNexus: 🚀 deployedThe |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 68c31b6980
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const metaTrailer = formatConsoleMeta(info); | ||
| if (metaTrailer) { | ||
| line = `${base} ${metaTrailer}`; |
There was a problem hiding this comment.
Appending formatConsoleMeta(info) directly to warn/error lines can leak secrets from structured metadata (for example {"apiKey":"sk-..."}), because the subsequent redactMessage pass does not reliably catch plain sk- tokens inside JSON payloads (its OpenAI-key regex is anchored to the start of the line in api/config/parsers.js). This commit newly exposes metadata on console warn/error paths, so sensitive fields that were previously dropped can now be emitted unredacted.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in e288f7f. Two fixes: (1) dropped the ^ anchor on the sk- regex (it was dead code since every console line starts with a timestamp) and added /g so multi-key JSON payloads get fully scrubbed; (2) moved redaction into formatConsoleMeta so every string value is run through redactMessage before JSON serialization, for defense in depth. Added regression tests for both cases.
Two gaps in the existing console redaction that became user-visible once
warn/error lines started emitting structured metadata:
1. The OpenAI-key regex (`/^(sk-)[^\s]+/`) was anchored to start-of-line,
so keys embedded inside JSON payloads (e.g. `{"apiKey":"sk-..."}`)
were never redacted. Every console line begins with a timestamp, so
the anchor effectively made this pattern dead code.
2. `formatConsoleMeta` stringified metadata values verbatim; a sensitive
string value was only redacted by the whole-line regex pass, which
missed the anchored `sk-` case above.
Fix:
- Drop the `^` anchor; add `/g` so every occurrence is redacted, not just
the first.
- Also exclude `"` and `'` from the token body so JSON-embedded values
terminate at the closing quote rather than chewing into the next field.
- Simplify `redactMessage` to apply patterns directly (dropping the
`getMatchingSensitivePatterns` filter) — the filter used `.test()`
which has stateful behavior on `/g` regexes and is no longer needed.
- `formatConsoleMeta` now runs `redactMessage` over every string value
before JSON serialization, so the metadata trailer is safe even on the
warn path.
- Add regression tests covering both fixes.
Reviewed-by: Codex (P1 finding on PR #12737, commit 68c31b6).
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e288f7fda7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const isErrorOrWarn = levelStr.includes('error') || levelStr.includes('warn'); | ||
| if (level !== 'debug' && !isErrorOrWarn) { |
There was a problem hiding this comment.
This new gate now sends warn/error records through debugTraverse, but that path appends leaf string metadata with truncateLongStrings(value) and never calls redactMessage, so sensitive values in metadata are written verbatim. With DEBUG_LOGGING defaulting to true, the debug file transport uses this formatter, which means payloads like { auth: 'Bearer ...' } or { openaiKey: 'sk-...' } are now persisted unredacted in debug logs; please redact metadata values in this branch before formatting them.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in c09d293. Apply redactMessage to the final debugTraverse output when level is warn/error. Debug-level behavior is preserved (matches prior art, which did not redact). Added regression tests covering all three cases.
GitNexus: 🚀 deployedThe |
Relaxing the debug-only gate in debugTraverse (in commit 59371be) routed warn/error records through the traversal path, which emits leaf string values verbatim (via truncateLongStrings only). Because DEBUG_LOGGING defaults to true, those records are also written to the rotating debug log file — which means payloads like `{ auth: 'Bearer ...' }` or `{ openaiKey: 'sk-...' }` were persisted unredacted once my earlier change took effect. Apply redactMessage to the final formatted string when the level is warn or error. Debug-level behavior is unchanged (matching prior art). Includes regression tests covering error/warn redaction and debug-level preservation. Reviewed-by: Codex (P1 finding on PR #12737, commit e288f7f).
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c09d293d47
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /(Bearer )[^\s]+/, // Header: Bearer token pattern | ||
| /(api-key:? )[^\s]+/, // Header: API key pattern | ||
| /(key=)[^\s]+/, // URL query param: sensitive key pattern (Google) | ||
| /(sk-)[^\s"']+/g, // OpenAI API key pattern (also catches keys embedded in JSON/quoted strings) |
There was a problem hiding this comment.
The new pattern /(sk-)[^\s"']+/g now matches any sk- substring inside ordinary text, so non-secret messages like task-runner failed or mask-token are rewritten to task-[REDACTED].... Because redactMessage is used by both file/error redaction and warn/error console formatting, this introduces broad false-positive redaction that can hide real log content and make troubleshooting harder. Restrict the match to actual token boundaries (or known key formats) instead of any in-word sk- occurrence.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in fdb6b36. Anchored every sensitive pattern with \b, constrained the OpenAI-key charset to the documented alphabet, and added regression tests for both false positives (task-runner, monkey=10) and still-firing true positives.
…tion Removing the `^` anchor in commit e288f7f let the OpenAI-key regex match anywhere in the line — including inside ordinary words like `task-runner` or `mask-value`, where `sk-` appears mid-word. Non-secret text was being rewritten to `task-[REDACTED]`, hiding real log content from operators. - Anchor every sensitive-key pattern with `\b` so matches only fire at word boundaries. - Constrain the OpenAI-key body to the documented charset (`[a-zA-Z0-9_-]+`) instead of the broader "not whitespace or quote" character class. - Add `&` to the `key=` exclusion so a query-string value stops at the next parameter separator. - Regression tests covering both the over-redaction cases (`task-runner`, `monkey=10`) and the intended redactions still firing. Reviewed-by: Codex (P2 finding on PR #12737, commit c09d293).
|
@codex review |
GitNexus: 🚀 deployedThe |
GitNexus: 🚀 deployedThe |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fdb6b36179
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /\b(Bearer )[^\s"']+/g, // Header: Bearer token pattern | ||
| /\b(api-key:? )[^\s"']+/g, // Header: API key pattern | ||
| /\b(key=)[^\s"'&]+/g, // URL query param: sensitive key pattern (Google) |
There was a problem hiding this comment.
The new \b anchors in the sensitive-token regexes can miss secrets once log text is colorized, because consoleFormat applies colorize({ all: true }) before calling redactMessage, and ANSI color prefixes end with m (a word character). In that flow, values that begin a colored segment (for example Bearer ... at the start of a warn message) may no longer match and can be printed unredacted.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 1e43d63. Extended redactFormat to cover warn in addition to error, operating on the raw pre-colorize info.message so the regex runs on plain text instead of contending with ANSI escapes. The in-printf redactMessage(line) stays as a backstop. Added regression tests for the new warn path and the info/debug no-op path.
The console pipeline runs `redactFormat → colorize({ all: true }) → printf`.
With `all: true`, winston wraps `info.message` in ANSI escapes whose
trailing `m` is a word character. That means `\b(Bearer )…` placed at the
start of a colorized segment can fall on a (word,word) boundary and miss —
the earlier line-wise `redactMessage(line)` pass in printf suffers the
same issue because it runs after colorize.
Extend `redactFormat` to run for `warn` in addition to `error`, operating
on the raw pre-colorize `info.message` + `Symbol.for('message')` strings.
The later in-printf `redactMessage(line)` stays as a backstop, but the
primary redaction now happens where the regex can actually see the text.
Metadata redaction already operates on the raw info object via
`formatConsoleMeta`, so it was never affected by ANSI — no change there.
Includes regression tests for the new warn-level behavior and for the
info/debug no-op path.
Reviewed-by: Codex (P2 finding on PR #12737, commit fdb6b36).
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e43d63687
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| const debugValue = metadata[SPLAT_SYMBOL]?.[0]; | ||
| const debugValue = metadata[SPLAT_SYMBOL]?.[0] ?? extractMetaObject(metadata); |
There was a problem hiding this comment.
When warn/error levels now enter debugTraverse, this line prefers metadata[Symbol.for('splat')][0] even if it is just a consumed printf argument string (for example logger.warn('failed for %s', tenant, { provider }) after format.splat()). In that case the formatter appends the tenant a second time (failed for tenant tenant) and never surfaces the structured metadata object, which regresses debug-file/DEBUG_CONSOLE log quality in the winston.format.combine(fileFormat, debugTraverse) path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in bccbf11. Swapped the preference order in debugTraverse: it now tries the structured metadata object (extractMetaObject) first and only falls back to SPLAT[0] if nothing else is available. That way consumed printf args don't get appended a second time, and the real metadata object still surfaces. Added a regression test for the logger.warn('failed for %s', tenant, { provider }) shape.
GitNexus: 🚀 deployedThe |
`debugTraverse` previously read `metadata[Symbol.for('splat')][0]` first
and only fell back to the structured metadata object. When a caller uses
printf interpolation alongside a metadata object — for example
`logger.warn('failed for %s', tenant, { provider })` — winston leaves the
*consumed* positional arg (`tenant`) in `SPLAT[0]` after interpolation.
The formatter would then append the tenant a second time and skip the
real metadata, regressing debug-file and `DEBUG_CONSOLE` output quality
now that warn/error share this path.
Prefer the structured metadata object (via `extractMetaObject`) and only
fall back to `SPLAT[0]` when there's nothing else, so the surviving log
line surfaces the actual key/value pairs regardless of call shape.
Reviewed-by: Codex (P2 finding on PR #12737, commit 1e43d63).
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bccbf117d6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| */ | ||
| const extracted = extractMetaObject(metadata); | ||
| const splatFirst = metadata[SPLAT_SYMBOL]?.[0]; | ||
| const debugValue = extracted ?? splatFirst; |
There was a problem hiding this comment.
When debugTraverse now processes warn/error, it falls back to metadata[SPLAT_SYMBOL][0] whenever no structured metadata is extracted, but winston leaves already-consumed %s arguments in SPLAT. That causes duplicated output for common calls like logger.warn('failed for %s', tenantId) (e.g. failed for tenant-7 tenant-7) in debug file/console formats, which this commit newly affects outside debug level. Reproduced with format.combine(timestamp(), splat(), debugTraverse) after this change.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in b34628d. Only fall back to SPLAT[0] when it's a plain object or array — structural data worth surfacing. Primitives there are almost certainly consumed %s/%d args and get skipped now. Regression tests cover both the single-%s case (failed for %s, tenantId) and arrays-as-metadata (still surfaced).
GitNexus: 🚀 deployedThe |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b34628de85
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (const key of Object.keys(source)) { | ||
| if (RESERVED_LOG_KEYS.has(key) || key.startsWith('_')) { | ||
| continue; |
There was a problem hiding this comment.
extractMetaObject currently accepts every enumerable key except a small reserved set, so when winston/logform has processed a warn/error call with a primitive extra arg (e.g. logger.warn('Unhandled step creation type:', step.type)), format.splat() can leave character-index keys ("0", "1", …) on info. Because this commit now emits warn/error metadata, those synthetic keys get logged as bogus metadata, producing noisy and misleading output in debug/file logs instead of useful context. Please exclude numeric/index-like keys (or otherwise guard against primitive splat artifacts) when collecting metadata.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 6bf9548. Added a numeric-key filter to extractMetaObject so synthetic splat artifacts (like the '0','1','2' keys that appear when a primitive is passed as the second log arg) don't get emitted as bogus metadata. Regression test covers the case.
When a caller passes a primitive as the second argument — e.g.
`logger.warn('Unhandled step creation type:', step.type)` — winston /
`format.splat()` can leave character-index keys (`"0"`, `"1"`, …) on the
`info` object. With the warn/error metadata trailer in play, those
synthetic artifacts were being surfaced as bogus metadata, producing
noisy console and debug-file output.
Filter out numeric-string keys in `extractMetaObject` so only real
metadata fields reach the trailer. Added a regression test.
Reviewed-by: Codex (P2 finding on PR #12737, commit b34628d).
|
@codex review |
GitNexus: 🚀 deployedThe |
GitNexus: 🚀 deployedThe |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6bf9548ff2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const splatUsable = | ||
| Array.isArray(splatFirst) || (splatFirst != null && typeof splatFirst === 'object'); | ||
| const debugValue = extracted ?? (splatUsable ? splatFirst : undefined); |
There was a problem hiding this comment.
Restricting SPLAT[0] to arrays/objects causes primitive trailing arguments to be dropped entirely, so calls like logger.debug('prefix:', detail) now log only prefix: (early-returning with no metadata) even when no %s/%d placeholder consumed detail. Before this change, those primitive args were surfaced in debugTraverse, so this is a regression that removes useful context from debug console/debug file diagnostics for existing two-argument debug logs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in c29c18e. Refined the heuristic: a primitive splat value is now skipped only when it already appears in the post-interpolation info.message (the %s case). Unconsumed primitives like logger.debug('prefix:', detail) are still surfaced. Regression test covers the restored path.
The previous round dropped every primitive SPLAT[0] value to avoid
duplicating consumed %s args, but that removed useful context from
calls like \`logger.debug('prefix:', detail)\` where the primitive was
never interpolated — users lost the \`detail\` value.
Refine the heuristic: skip a primitive splat value only when it already
appears inside the (post-interpolation) \`info.message\`; otherwise
surface it. Arrays and objects continue to surface unconditionally.
Regression test covers the 'prefix:', detail case.
Reviewed-by: Codex (P2 finding on PR #12737, commit 6bf9548).
|
@codex review |
GitNexus: 🚀 deployedThe |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c29c18e8a1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| return false; | ||
| })(); | ||
| const debugValue = extracted ?? (splatUsable ? splatFirst : undefined); |
There was a problem hiding this comment.
debugTraverse now computes extracted = extractMetaObject(metadata) to remove numeric splat artifacts, but the object-rendering path still traverses the raw metadata object later. In cases where format.splat() leaves character-index keys alongside real metadata (for example, warn/error calls that mix an unconsumed primitive argument with a metadata object), the output will include noisy "0", "1", ... entries and bury the useful fields, which defeats the filtering this patch introduced.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 9ea1152. Swap the traversal source to clone debugValue (the already-filtered object) instead of the raw metadata — that way the reserved-key + numeric-artifact filters in extractMetaObject actually take effect in the rendered multi-line output. Regression test covers the numeric-artifact case.
`debugTraverse` computes `extracted = extractMetaObject(metadata)` to strip reserved keys, underscore-prefixed internals, and numeric splat artifacts — but the later \`klona(metadata)\` + \`traverse\` path still read the raw object, putting all the filtered junk back into the rendered multi-line output. Clone and traverse \`debugValue\` (the already-filtered object) instead. Regression test exercises the case where numeric splat artifacts sit alongside a real metadata field. Reviewed-by: Codex (P2 finding on PR #12737, commit c29c18e).
|
@codex review |
GitNexus: 🚀 deployedThe |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ea11529f0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (RESERVED_LOG_KEYS.has(key) || key.startsWith('_')) { | ||
| continue; |
There was a problem hiding this comment.
extractMetaObject now skips every key that starts with _, and debugTraverse routes debug/warn/error metadata through this filter. That drops legitimate diagnostic fields like Mongo _id (for example logger.debug('doc', {_id, ...})), which is a regression from the previous formatter behavior and makes log correlation harder during debugging incidents.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in d63742a. Dropped the underscore-prefix filter so legitimate fields like MongoDB _id pass through. Only reserved winston keys and numeric splat artifacts remain filtered.
| const splatStr = String(splatFirst); | ||
| return splatStr !== '' && !message.includes(splatStr); | ||
| } |
There was a problem hiding this comment.
The splatUsable guard treats a primitive SPLAT value as already consumed whenever its text appears anywhere in message (!message.includes(splatStr)). This drops real unconsumed args in cases like logger.debug('HTTP 404 from upstream', 404), where the second argument should still be surfaced; prior behavior preserved that value.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in d63742a by a broader refactor. Warn/error no longer go through the splat/traverse code at all — they now emit a simple compact JSON trailer via formatConsoleMeta (same shape as the console formatter). Debug level reverts to its original behavior. This removes the over-aggressive primitive heuristic entirely and eliminates the class of splat-interaction edge cases we kept hitting.
Retrofitting `debugTraverse`'s multi-line object walker to cover warn/error created a minefield of splat-interaction edge cases (numeric artifacts, consumed %s args, bogus `_`-prefix filtering, over-eager suppression of unconsumed primitives). Each fix kept introducing new corner cases. Split the two concerns instead: - Warn/error now emit a compact single-line JSON metadata trailer via `formatConsoleMeta`, then pass the full line through `redactMessage`. This mirrors what the console formatter already does, so behavior between the console and debug-file outputs stays consistent for warn/error — and none of the splat/traversal edge cases apply. - Debug level keeps its original code path verbatim (including the raw `metadata` traversal and SPLAT\[0\] fallback). No regressions from my earlier iterations. - `extractMetaObject` no longer filters underscore-prefixed keys, so legitimate fields like MongoDB `_id` still appear. Reserved winston keys and numeric splat artifacts remain filtered. Updated tests reflect the simpler contract (underscore preservation, single-line trailer expectations already covered). Reviewed-by: Codex (two P2 findings on PR #12737, commit 9ea1152: `_id` regression and over-eager primitive suppression).
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d63742a5cb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } catch { | ||
| return ''; |
There was a problem hiding this comment.
If any warn/error metadata value is circular (for example, an attached request/response object), JSON.stringify throws and the catch returns an empty string, which drops all metadata including useful scalar fields like provider, model, or status. This undermines the goal of making failures diagnosable, because one problematic field suppresses the entire trailer instead of just being omitted/safely serialized.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 58c2dc8. Added a WeakSet-based circular replacer (replaces repeated visits with [Circular]) and a per-field fallback if the whole-object serialization still fails — scalar fields like provider/model/status always land, and only the offending value becomes [Unserializable]. Regression test covers a shape with both scalars and a circular object.
`formatConsoleMeta` previously wrapped a single `JSON.stringify` in try/catch — any circular reference inside any field (e.g. an attached request/response object) caused the entire trailer to be dropped. That defeats the goal of making failures diagnosable: one malformed field would mask the provider/model/status we wanted to surface. Use a `WeakSet`-based replacer that emits `[Circular]` for repeated object visits. On the whole-object serialization failing, fall back to per-field serialization so scalar keys always land and only the offending field is replaced with \`"[Unserializable]"\`. Reviewed-by: Codex (P2 finding on PR #12737, commit d63742a).
GitNexus: 🚀 deployedThe |
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
GitNexus: 🚀 deployedThe |
Audit review identified several MINOR/NIT items on top of the codex rounds. This commit closes the actionable ones: - **JSDoc (#1, #2)**: `extractMetaObject` no longer claims to filter underscore-prefixed keys (that filter was removed intentionally for MongoDB `_id`). `debugTraverse`'s docblock now describes the three code paths (warn/error compact trailer, debug multi-line traversal, other levels). - **Case-insensitive api-key regex (#6)**: `/gi` so the Azure style `Api-Key:` / `API-KEY:` also gets redacted. Pre-existing behavior was lowercase-only. - **Consolidated redundant branch (#5)**: `consoleFormat` printf was checking `isError || isWarn` twice; merged into one block. - **Pre-compiled regex (#9)**: `NUMERIC_KEY_RE` moved to module scope. - **Test coverage (#3, #4)**: Added regression tests for - per-field serialization fallback when a value's `toJSON` throws, - sensitive strings nested inside metadata objects, - the Azure-style `Api-Key:` header.
GitNexus: 🚀 deployedThe |
…avila#12737) * 🔊 fix: Preserve Log Metadata on Console for Warn/Error Levels The default console formatter discarded every structured metadata key on the winston info object — only `CONSOLE_JSON=true` preserved it. That meant failures emitted by the agents SDK (e.g. "Summarization LLM call failed") reached stdout without the provider, model, or underlying error attached, leaving users unable to diagnose the root cause. - Add `formatConsoleMeta` helper to serialize non-reserved metadata as a compact JSON trailer, with per-value string truncation and safe handling of circular references. - Append the metadata trailer to warn/error console lines; info/debug behavior is unchanged. - Relax `debugTraverse`'s debug-only gate so warn/error messages routed through the debug formatter also surface their metadata. - Add a `formatConsoleMeta` stub to the shared logger mock so existing tests keep working. * 🔐 fix: Also Redact Sensitive Patterns on Warn Console Lines The warn-level console output now includes a metadata trailer that may contain provider-returned error strings with embedded tokens or keys (e.g. `Bearer ...`, `sk-...`). Apply `redactMessage` to warn lines in addition to error, matching the new surface area. * 🔐 fix: Redact Sensitive Tokens Embedded in JSON Metadata Two gaps in the existing console redaction that became user-visible once warn/error lines started emitting structured metadata: 1. The OpenAI-key regex (`/^(sk-)[^\s]+/`) was anchored to start-of-line, so keys embedded inside JSON payloads (e.g. `{"apiKey":"sk-..."}`) were never redacted. Every console line begins with a timestamp, so the anchor effectively made this pattern dead code. 2. `formatConsoleMeta` stringified metadata values verbatim; a sensitive string value was only redacted by the whole-line regex pass, which missed the anchored `sk-` case above. Fix: - Drop the `^` anchor; add `/g` so every occurrence is redacted, not just the first. - Also exclude `"` and `'` from the token body so JSON-embedded values terminate at the closing quote rather than chewing into the next field. - Simplify `redactMessage` to apply patterns directly (dropping the `getMatchingSensitivePatterns` filter) — the filter used `.test()` which has stateful behavior on `/g` regexes and is no longer needed. - `formatConsoleMeta` now runs `redactMessage` over every string value before JSON serialization, so the metadata trailer is safe even on the warn path. - Add regression tests covering both fixes. Reviewed-by: Codex (P1 finding on PR danny-avila#12737, commit 68c31b6). * 🔐 fix: Redact Metadata in debugTraverse for Warn and Error Relaxing the debug-only gate in debugTraverse (in commit 59371be) routed warn/error records through the traversal path, which emits leaf string values verbatim (via truncateLongStrings only). Because DEBUG_LOGGING defaults to true, those records are also written to the rotating debug log file — which means payloads like `{ auth: 'Bearer ...' }` or `{ openaiKey: 'sk-...' }` were persisted unredacted once my earlier change took effect. Apply redactMessage to the final formatted string when the level is warn or error. Debug-level behavior is unchanged (matching prior art). Includes regression tests covering error/warn redaction and debug-level preservation. Reviewed-by: Codex (P1 finding on PR danny-avila#12737, commit e288f7f). * 🔐 fix: Anchor Secret Regexes at Word Boundaries to Prevent Over-Redaction Removing the `^` anchor in commit e288f7f let the OpenAI-key regex match anywhere in the line — including inside ordinary words like `task-runner` or `mask-value`, where `sk-` appears mid-word. Non-secret text was being rewritten to `task-[REDACTED]`, hiding real log content from operators. - Anchor every sensitive-key pattern with `\b` so matches only fire at word boundaries. - Constrain the OpenAI-key body to the documented charset (`[a-zA-Z0-9_-]+`) instead of the broader "not whitespace or quote" character class. - Add `&` to the `key=` exclusion so a query-string value stops at the next parameter separator. - Regression tests covering both the over-redaction cases (`task-runner`, `monkey=10`) and the intended redactions still firing. Reviewed-by: Codex (P2 finding on PR danny-avila#12737, commit c09d293). * 🔐 fix: Redact Before Colorize To Survive ANSI Word-Boundary Interference The console pipeline runs `redactFormat → colorize({ all: true }) → printf`. With `all: true`, winston wraps `info.message` in ANSI escapes whose trailing `m` is a word character. That means `\b(Bearer )…` placed at the start of a colorized segment can fall on a (word,word) boundary and miss — the earlier line-wise `redactMessage(line)` pass in printf suffers the same issue because it runs after colorize. Extend `redactFormat` to run for `warn` in addition to `error`, operating on the raw pre-colorize `info.message` + `Symbol.for('message')` strings. The later in-printf `redactMessage(line)` stays as a backstop, but the primary redaction now happens where the regex can actually see the text. Metadata redaction already operates on the raw info object via `formatConsoleMeta`, so it was never affected by ANSI — no change there. Includes regression tests for the new warn-level behavior and for the info/debug no-op path. Reviewed-by: Codex (P2 finding on PR danny-avila#12737, commit fdb6b36). * 🧹 fix: Prefer Structured Metadata Over Consumed Splat Args in Traversal `debugTraverse` previously read `metadata[Symbol.for('splat')][0]` first and only fell back to the structured metadata object. When a caller uses printf interpolation alongside a metadata object — for example `logger.warn('failed for %s', tenant, { provider })` — winston leaves the *consumed* positional arg (`tenant`) in `SPLAT[0]` after interpolation. The formatter would then append the tenant a second time and skip the real metadata, regressing debug-file and `DEBUG_CONSOLE` output quality now that warn/error share this path. Prefer the structured metadata object (via `extractMetaObject`) and only fall back to `SPLAT[0]` when there's nothing else, so the surviving log line surfaces the actual key/value pairs regardless of call shape. Reviewed-by: Codex (P2 finding on PR danny-avila#12737, commit 1e43d63). * 🧹 fix: Skip Consumed Splat Primitives in Warn/Error Debug Traversal When no structured metadata is attached, winston still leaves consumed `%s` / `%d` arguments in `Symbol.for('splat')`. Previous fix preferred the structured object but still fell back to whatever sat at `SPLAT[0]` — so `logger.warn('failed for %s', tenantId)` emitted `failed for tenant-7 tenant-7` in the traversal path (debug file and `DEBUG_CONSOLE`), now regressed outside of `debug` level because warn/error share the path. Only accept the splat fallback when the value is a plain object or an array (structural data worth surfacing). Primitives there are almost certainly consumed printf args and get skipped. Regression tests cover the single-%s case and the array-as-metadata case (which still surfaces through splat). Reviewed-by: Codex (P2 finding on PR danny-avila#12737, commit bccbf11). * 🧹 fix: Skip Numeric Splat Keys When Extracting Log Metadata When a caller passes a primitive as the second argument — e.g. `logger.warn('Unhandled step creation type:', step.type)` — winston / `format.splat()` can leave character-index keys (`"0"`, `"1"`, …) on the `info` object. With the warn/error metadata trailer in play, those synthetic artifacts were being surfaced as bogus metadata, producing noisy console and debug-file output. Filter out numeric-string keys in `extractMetaObject` so only real metadata fields reach the trailer. Added a regression test. Reviewed-by: Codex (P2 finding on PR danny-avila#12737, commit b34628d). * 🧹 fix: Preserve Unconsumed Primitive Splat Args in Debug Traversal The previous round dropped every primitive SPLAT[0] value to avoid duplicating consumed %s args, but that removed useful context from calls like \`logger.debug('prefix:', detail)\` where the primitive was never interpolated — users lost the \`detail\` value. Refine the heuristic: skip a primitive splat value only when it already appears inside the (post-interpolation) \`info.message\`; otherwise surface it. Arrays and objects continue to surface unconditionally. Regression test covers the 'prefix:', detail case. Reviewed-by: Codex (P2 finding on PR danny-avila#12737, commit 6bf9548). * 🧹 fix: Traverse Filtered Metadata, Not Raw Metadata, In debugTraverse `debugTraverse` computes `extracted = extractMetaObject(metadata)` to strip reserved keys, underscore-prefixed internals, and numeric splat artifacts — but the later \`klona(metadata)\` + \`traverse\` path still read the raw object, putting all the filtered junk back into the rendered multi-line output. Clone and traverse \`debugValue\` (the already-filtered object) instead. Regression test exercises the case where numeric splat artifacts sit alongside a real metadata field. Reviewed-by: Codex (P2 finding on PR danny-avila#12737, commit c29c18e). * 🧹 refactor: Split Warn/Error From Debug-Level Traversal in Parsers Retrofitting `debugTraverse`'s multi-line object walker to cover warn/error created a minefield of splat-interaction edge cases (numeric artifacts, consumed %s args, bogus `_`-prefix filtering, over-eager suppression of unconsumed primitives). Each fix kept introducing new corner cases. Split the two concerns instead: - Warn/error now emit a compact single-line JSON metadata trailer via `formatConsoleMeta`, then pass the full line through `redactMessage`. This mirrors what the console formatter already does, so behavior between the console and debug-file outputs stays consistent for warn/error — and none of the splat/traversal edge cases apply. - Debug level keeps its original code path verbatim (including the raw `metadata` traversal and SPLAT\[0\] fallback). No regressions from my earlier iterations. - `extractMetaObject` no longer filters underscore-prefixed keys, so legitimate fields like MongoDB `_id` still appear. Reserved winston keys and numeric splat artifacts remain filtered. Updated tests reflect the simpler contract (underscore preservation, single-line trailer expectations already covered). Reviewed-by: Codex (two P2 findings on PR danny-avila#12737, commit 9ea1152: `_id` regression and over-eager primitive suppression). * 🧹 fix: Preserve Scalar Metadata When One Value Is Circular `formatConsoleMeta` previously wrapped a single `JSON.stringify` in try/catch — any circular reference inside any field (e.g. an attached request/response object) caused the entire trailer to be dropped. That defeats the goal of making failures diagnosable: one malformed field would mask the provider/model/status we wanted to surface. Use a `WeakSet`-based replacer that emits `[Circular]` for repeated object visits. On the whole-object serialization failing, fall back to per-field serialization so scalar keys always land and only the offending field is replaced with \`"[Unserializable]"\`. Reviewed-by: Codex (P2 finding on PR danny-avila#12737, commit d63742a). * 🧹 fix: Address Audit Findings (JSDoc, Case-Insensitive Api-Key, Tests) Audit review identified several MINOR/NIT items on top of the codex rounds. This commit closes the actionable ones: - **JSDoc (#1, #2)**: `extractMetaObject` no longer claims to filter underscore-prefixed keys (that filter was removed intentionally for MongoDB `_id`). `debugTraverse`'s docblock now describes the three code paths (warn/error compact trailer, debug multi-line traversal, other levels). - **Case-insensitive api-key regex (#6)**: `/gi` so the Azure style `Api-Key:` / `API-KEY:` also gets redacted. Pre-existing behavior was lowercase-only. - **Consolidated redundant branch (#5)**: `consoleFormat` printf was checking `isError || isWarn` twice; merged into one block. - **Pre-compiled regex (#9)**: `NUMERIC_KEY_RE` moved to module scope. - **Test coverage (#3, #4)**: Added regression tests for - per-field serialization fallback when a value's `toJSON` throws, - sensitive strings nested inside metadata objects, - the Azure-style `Api-Key:` header.
…avila#12737) * 🔊 fix: Preserve Log Metadata on Console for Warn/Error Levels The default console formatter discarded every structured metadata key on the winston info object — only `CONSOLE_JSON=true` preserved it. That meant failures emitted by the agents SDK (e.g. "Summarization LLM call failed") reached stdout without the provider, model, or underlying error attached, leaving users unable to diagnose the root cause. - Add `formatConsoleMeta` helper to serialize non-reserved metadata as a compact JSON trailer, with per-value string truncation and safe handling of circular references. - Append the metadata trailer to warn/error console lines; info/debug behavior is unchanged. - Relax `debugTraverse`'s debug-only gate so warn/error messages routed through the debug formatter also surface their metadata. - Add a `formatConsoleMeta` stub to the shared logger mock so existing tests keep working. * 🔐 fix: Also Redact Sensitive Patterns on Warn Console Lines The warn-level console output now includes a metadata trailer that may contain provider-returned error strings with embedded tokens or keys (e.g. `Bearer ...`, `sk-...`). Apply `redactMessage` to warn lines in addition to error, matching the new surface area. * 🔐 fix: Redact Sensitive Tokens Embedded in JSON Metadata Two gaps in the existing console redaction that became user-visible once warn/error lines started emitting structured metadata: 1. The OpenAI-key regex (`/^(sk-)[^\s]+/`) was anchored to start-of-line, so keys embedded inside JSON payloads (e.g. `{"apiKey":"sk-..."}`) were never redacted. Every console line begins with a timestamp, so the anchor effectively made this pattern dead code. 2. `formatConsoleMeta` stringified metadata values verbatim; a sensitive string value was only redacted by the whole-line regex pass, which missed the anchored `sk-` case above. Fix: - Drop the `^` anchor; add `/g` so every occurrence is redacted, not just the first. - Also exclude `"` and `'` from the token body so JSON-embedded values terminate at the closing quote rather than chewing into the next field. - Simplify `redactMessage` to apply patterns directly (dropping the `getMatchingSensitivePatterns` filter) — the filter used `.test()` which has stateful behavior on `/g` regexes and is no longer needed. - `formatConsoleMeta` now runs `redactMessage` over every string value before JSON serialization, so the metadata trailer is safe even on the warn path. - Add regression tests covering both fixes. Reviewed-by: Codex (P1 finding on PR danny-avila#12737, commit 68c31b6). * 🔐 fix: Redact Metadata in debugTraverse for Warn and Error Relaxing the debug-only gate in debugTraverse (in commit 59371be) routed warn/error records through the traversal path, which emits leaf string values verbatim (via truncateLongStrings only). Because DEBUG_LOGGING defaults to true, those records are also written to the rotating debug log file — which means payloads like `{ auth: 'Bearer ...' }` or `{ openaiKey: 'sk-...' }` were persisted unredacted once my earlier change took effect. Apply redactMessage to the final formatted string when the level is warn or error. Debug-level behavior is unchanged (matching prior art). Includes regression tests covering error/warn redaction and debug-level preservation. Reviewed-by: Codex (P1 finding on PR danny-avila#12737, commit e288f7f). * 🔐 fix: Anchor Secret Regexes at Word Boundaries to Prevent Over-Redaction Removing the `^` anchor in commit e288f7f let the OpenAI-key regex match anywhere in the line — including inside ordinary words like `task-runner` or `mask-value`, where `sk-` appears mid-word. Non-secret text was being rewritten to `task-[REDACTED]`, hiding real log content from operators. - Anchor every sensitive-key pattern with `\b` so matches only fire at word boundaries. - Constrain the OpenAI-key body to the documented charset (`[a-zA-Z0-9_-]+`) instead of the broader "not whitespace or quote" character class. - Add `&` to the `key=` exclusion so a query-string value stops at the next parameter separator. - Regression tests covering both the over-redaction cases (`task-runner`, `monkey=10`) and the intended redactions still firing. Reviewed-by: Codex (P2 finding on PR danny-avila#12737, commit c09d293). * 🔐 fix: Redact Before Colorize To Survive ANSI Word-Boundary Interference The console pipeline runs `redactFormat → colorize({ all: true }) → printf`. With `all: true`, winston wraps `info.message` in ANSI escapes whose trailing `m` is a word character. That means `\b(Bearer )…` placed at the start of a colorized segment can fall on a (word,word) boundary and miss — the earlier line-wise `redactMessage(line)` pass in printf suffers the same issue because it runs after colorize. Extend `redactFormat` to run for `warn` in addition to `error`, operating on the raw pre-colorize `info.message` + `Symbol.for('message')` strings. The later in-printf `redactMessage(line)` stays as a backstop, but the primary redaction now happens where the regex can actually see the text. Metadata redaction already operates on the raw info object via `formatConsoleMeta`, so it was never affected by ANSI — no change there. Includes regression tests for the new warn-level behavior and for the info/debug no-op path. Reviewed-by: Codex (P2 finding on PR danny-avila#12737, commit fdb6b36). * 🧹 fix: Prefer Structured Metadata Over Consumed Splat Args in Traversal `debugTraverse` previously read `metadata[Symbol.for('splat')][0]` first and only fell back to the structured metadata object. When a caller uses printf interpolation alongside a metadata object — for example `logger.warn('failed for %s', tenant, { provider })` — winston leaves the *consumed* positional arg (`tenant`) in `SPLAT[0]` after interpolation. The formatter would then append the tenant a second time and skip the real metadata, regressing debug-file and `DEBUG_CONSOLE` output quality now that warn/error share this path. Prefer the structured metadata object (via `extractMetaObject`) and only fall back to `SPLAT[0]` when there's nothing else, so the surviving log line surfaces the actual key/value pairs regardless of call shape. Reviewed-by: Codex (P2 finding on PR danny-avila#12737, commit 1e43d63). * 🧹 fix: Skip Consumed Splat Primitives in Warn/Error Debug Traversal When no structured metadata is attached, winston still leaves consumed `%s` / `%d` arguments in `Symbol.for('splat')`. Previous fix preferred the structured object but still fell back to whatever sat at `SPLAT[0]` — so `logger.warn('failed for %s', tenantId)` emitted `failed for tenant-7 tenant-7` in the traversal path (debug file and `DEBUG_CONSOLE`), now regressed outside of `debug` level because warn/error share the path. Only accept the splat fallback when the value is a plain object or an array (structural data worth surfacing). Primitives there are almost certainly consumed printf args and get skipped. Regression tests cover the single-%s case and the array-as-metadata case (which still surfaces through splat). Reviewed-by: Codex (P2 finding on PR danny-avila#12737, commit bccbf11). * 🧹 fix: Skip Numeric Splat Keys When Extracting Log Metadata When a caller passes a primitive as the second argument — e.g. `logger.warn('Unhandled step creation type:', step.type)` — winston / `format.splat()` can leave character-index keys (`"0"`, `"1"`, …) on the `info` object. With the warn/error metadata trailer in play, those synthetic artifacts were being surfaced as bogus metadata, producing noisy console and debug-file output. Filter out numeric-string keys in `extractMetaObject` so only real metadata fields reach the trailer. Added a regression test. Reviewed-by: Codex (P2 finding on PR danny-avila#12737, commit b34628d). * 🧹 fix: Preserve Unconsumed Primitive Splat Args in Debug Traversal The previous round dropped every primitive SPLAT[0] value to avoid duplicating consumed %s args, but that removed useful context from calls like \`logger.debug('prefix:', detail)\` where the primitive was never interpolated — users lost the \`detail\` value. Refine the heuristic: skip a primitive splat value only when it already appears inside the (post-interpolation) \`info.message\`; otherwise surface it. Arrays and objects continue to surface unconditionally. Regression test covers the 'prefix:', detail case. Reviewed-by: Codex (P2 finding on PR danny-avila#12737, commit 6bf9548). * 🧹 fix: Traverse Filtered Metadata, Not Raw Metadata, In debugTraverse `debugTraverse` computes `extracted = extractMetaObject(metadata)` to strip reserved keys, underscore-prefixed internals, and numeric splat artifacts — but the later \`klona(metadata)\` + \`traverse\` path still read the raw object, putting all the filtered junk back into the rendered multi-line output. Clone and traverse \`debugValue\` (the already-filtered object) instead. Regression test exercises the case where numeric splat artifacts sit alongside a real metadata field. Reviewed-by: Codex (P2 finding on PR danny-avila#12737, commit c29c18e). * 🧹 refactor: Split Warn/Error From Debug-Level Traversal in Parsers Retrofitting `debugTraverse`'s multi-line object walker to cover warn/error created a minefield of splat-interaction edge cases (numeric artifacts, consumed %s args, bogus `_`-prefix filtering, over-eager suppression of unconsumed primitives). Each fix kept introducing new corner cases. Split the two concerns instead: - Warn/error now emit a compact single-line JSON metadata trailer via `formatConsoleMeta`, then pass the full line through `redactMessage`. This mirrors what the console formatter already does, so behavior between the console and debug-file outputs stays consistent for warn/error — and none of the splat/traversal edge cases apply. - Debug level keeps its original code path verbatim (including the raw `metadata` traversal and SPLAT\[0\] fallback). No regressions from my earlier iterations. - `extractMetaObject` no longer filters underscore-prefixed keys, so legitimate fields like MongoDB `_id` still appear. Reserved winston keys and numeric splat artifacts remain filtered. Updated tests reflect the simpler contract (underscore preservation, single-line trailer expectations already covered). Reviewed-by: Codex (two P2 findings on PR danny-avila#12737, commit 9ea1152: `_id` regression and over-eager primitive suppression). * 🧹 fix: Preserve Scalar Metadata When One Value Is Circular `formatConsoleMeta` previously wrapped a single `JSON.stringify` in try/catch — any circular reference inside any field (e.g. an attached request/response object) caused the entire trailer to be dropped. That defeats the goal of making failures diagnosable: one malformed field would mask the provider/model/status we wanted to surface. Use a `WeakSet`-based replacer that emits `[Circular]` for repeated object visits. On the whole-object serialization failing, fall back to per-field serialization so scalar keys always land and only the offending field is replaced with \`"[Unserializable]"\`. Reviewed-by: Codex (P2 finding on PR danny-avila#12737, commit d63742a). * 🧹 fix: Address Audit Findings (JSDoc, Case-Insensitive Api-Key, Tests) Audit review identified several MINOR/NIT items on top of the codex rounds. This commit closes the actionable ones: - **JSDoc (#1, #2)**: `extractMetaObject` no longer claims to filter underscore-prefixed keys (that filter was removed intentionally for MongoDB `_id`). `debugTraverse`'s docblock now describes the three code paths (warn/error compact trailer, debug multi-line traversal, other levels). - **Case-insensitive api-key regex (#6)**: `/gi` so the Azure style `Api-Key:` / `API-KEY:` also gets redacted. Pre-existing behavior was lowercase-only. - **Consolidated redundant branch (#5)**: `consoleFormat` printf was checking `isError || isWarn` twice; merged into one block. - **Pre-compiled regex (#9)**: `NUMERIC_KEY_RE` moved to module scope. - **Test coverage (#3, #4)**: Added regression tests for - per-field serialization fallback when a value's `toJSON` throws, - sensitive strings nested inside metadata objects, - the Azure-style `Api-Key:` header.
Summary
Resolves Discussion #12733 (and makes Discussions #12614, Issue #12721 diagnosable).
The default console formatter discarded every structured metadata key on the winston
infoobject — onlyCONSOLE_JSON=truepreserved it. That meant failures emitted by the agents SDK (e.g.Summarization LLM call failed) reached stdout without provider, model, or underlying error attached, leaving users unable to diagnose the root cause.Before
After
Changes
api/config/parsers.jsformatConsoleMetahelper serializes non-reserved metadata as a compact JSON trailer, with per-value string truncation (reusingCONSOLE_JSON_STRING_LENGTH) and safe handling of circular references.debugTraverse's debug-only gate relaxed so warn/error messages routed through the debug formatter also surface their metadata.api/config/winston.jsconsoleFormatnow appendsformatConsoleMeta(info)on warn/error lines. Info/debug behavior is unchanged. Redaction on error is preserved.api/test/__mocks__/logger.js— add aformatConsoleMetastub so tests that mock~/config/parserskeep working.api/config/__tests__/parsers.spec.js— new unit tests forformatConsoleMeta(6 tests, all pass).Pairs with
Test plan
npx jest config/__tests__/parsers.spec.js— 6/6 pass.CONSOLE_JSON=trueand confirm structured output is unchanged.