fix(utils): preserve message of thrown non-Error objects in crash diagnostics - #3823
fix(utils): preserve message of thrown non-Error objects in crash diagnostics#3823developjik wants to merge 3 commits into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Exact-head GPT-heavy adversarial batch review for 3e193322645fd5e18e8211408f5b5aff7b2a4d7b.
Recorded PR base: f9dffed426a433bf4948bdeb8fb2fe76f25ef5ca. Current dev: 732856b3ccb3fade6e9fbc17908a4fbca5a7682f. The current-base merge is clean, but the source change has blocking fatal-path and privacy defects.
Terminal verdict: REQUEST_CHANGES
Blocking findings
-
The claimed never-throw normalizer still throws on hostile/revoked Proxies (
packages/utils/src/postmortem.ts:194,204,379).reason instanceof ErrorandArray.isArray(reason)are outside any guard. On this exact head under Bun 1.3.14, a Proxy whosegetPrototypeOftrap throws changed the originalUncaught Exceptioninto a secondaryUnhandled Rejection: getPrototypeOf boom; a revoked Proxy behaved the same way. The original fatal handler rejects before persistence/stderr/cleanup and loses the original diagnostic. The new directrecordFatalCrashtests do not prove this path because that writer has an outer catch. -
The JSON fallback newly leaks arbitrary object fields and performs unbounded work before the crash-record cap (
postmortem.ts:212,242-248,331-334,384,387-390). Exact-head reproduction with{ code: "E_AUTH", token: "opaque_session_credential_0123456789" }persisted the token verbatim ingjc-crash.logand printed it to stderr; the current scrubber does not cover a baretokenfield. Large/deep objects and hostiletoJSON/getters are serialized synchronously before the 64 KiB bound, so the fatal path can allocate heavily, stall, or re-enter. -
Copied
.namebypasses durable redaction (postmortem.ts:220-221,333).err.nameis interpolated raw while only message/stack are scrubbed. An exact-head probe with a PAT-shaped name persisted that credential in the crash-log header. Raw name/message values can also inject newlines or terminal control sequences into the record and stderr. Redaction/sanitization must cover every sink, or the fallback must avoid arbitrary object snapshots.
Verification and automation
- Local exact-head focused test:
bun test packages/utils/test/postmortem-crash-log.test.ts— 24 pass, 0 fail. - Additional exact-head process probes reproduced the Proxy containment failure and both redaction leaks above.
- GitHub has 0 check runs/status contexts for this head. Dev CI and Public site sync both ended
action_requiredwith zero jobs. - Automated feedback consists only of the Codex usage-limit comment; there are no prior submitted reviews, inline comments, or review threads.
Required regression coverage: spawned fatal-handler tests for hostile/revoked Proxies, opaque-field/name redaction across crash log + stderr/logger, and bounded behavior for the new serialization path.
Signed terminal receipt: gajae.pr-review-verdict.v1 request-changes sha256:3e193322645fd5e18e8211408f5b5aff7b2a4d7b reviewer:Yeachan-Heo evidence:gpt-heavy-adversarial-batch-2026-08-05
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Exact-head adversarial read-only review of 3e193322645fd5e18e8211408f5b5aff7b2a4d7b (PR #3823, base dev @ f9dffed42, 1 commit, 3 files) against current dev (f359a9d7e). Merge is clean (mergeable: true), focused tests pass, and the intended feature works on the spawned fatal-handler path — but the change introduces two credential-leak regressions in the durable crash log and fails to deliver its own claimed never-throw guarantee.
Terminal verdict: REQUEST_CHANGES
Blocker 1 — JSON fallback persists arbitrary, unredacted object fields into the durable crash log
errorForDiagnostic snapshots any non-array object via JSON.stringify when .message is absent (postmortem.ts:212). recordFatalCrash scrubs only err.message/err.stack (postmortem.ts:331-334), and redactCrashSecrets has no rule for a bare token/secret field or arbitrary opaque values. Exact-head probe:
recordFatalCrash("Uncaught Exception", { code: "E_AUTH", token: "opaque_session_credential_0123456789" }, { path: "leak.log" });
persists Error: {"code":"E_AUTH","token":"opaque_session_credential_0123456789"} verbatim into the rotation-immune crash log. Before this PR the same throw recorded [object Object]; the leak is introduced by this change, in a module whose stated purpose is scrubbing credentials before indefinite persistence.
Blocker 2 — copied .name bypasses durable redaction
err.name is interpolated raw into the record header while only message/stack are scrubbed (postmortem.ts:333). Probe with { name: "ghp_abcdef0123456789abcdef0123", message: "x" } writes the raw PAT in the header line, while the identical value in message position is redacted («redacted-github-token»). Names can also carry newlines/control characters into a line-based record, letting a thrown object forge record boundaries.
Blocker 3 — the claimed never-throw guarantee does not hold for hostile/revoked Proxies
The change's own comment says a "Proxy get-trap ... cannot re-enter the crash path", but reason instanceof Error (postmortem.ts:194) and the new Array.isArray(reason) (postmortem.ts:204) are unguarded and both invoke the proxy's getPrototypeOf trap. Spawned exact-head probes on the uncaughtException path:
throw new Proxy({}, { getPrototypeOf() { throw new Error("getPrototypeOf boom") } })— the crash log records only[Unhandled Rejection] Error: getPrototypeOf boom(created atpostmortem.ts:194); the original exception is lost and the label is degraded.- a revoked proxy —
[Unhandled Rejection] TypeError: Proxy has already been revoked ...at the same site.
The new direct recordFatalCrash tests cannot prove this path because that writer has an outer catch.
Non-blocking observations
- The object branch catches all non-array objects, not just plain ones:
Date/RegExp/Map/Set/Promisenow serialize to a quoted string or{}, regressing fidelity vs the previousString()output (/re/,[object Map]). Consider gating on plain objects and letting non-plain objects fall through to the guardedString()path. JSON.stringifyruns unbounded before the 64 KiB record cap, so a huge/deep thrown object does arbitrarily heavy synchronous work on the fatal path.
Verification
bun test packages/utils/test/postmortem-crash-log.test.ts— 24 pass, 0 fail (exact head).- Positive control on the spawned fatal-handler path:
throw { phase, reason, message }— message preserved on stderr and in the crash log, exit 1. - Exact-head probes reproduced Blocker 1/2 leaks and Blocker 3 proxy containment failure.
- CI on head: 0 check runs, 0 status contexts (
mergeable_state: unstable); no green CI evidence for this commit.
Required before merge: route every persisted field (message, name, and the JSON snapshot) through redactCrashSecrets, avoid snapshotting opaque fields, and make the entire normalization (including instanceof/Array.isArray) never-throw with spawned fatal-handler regression tests.
Signed terminal receipt: gajae.pr-review-verdict.v1 request-changes sha256:3e193322645fd5e18e8211408f5b5aff7b2a4d7b reviewer:Yeachan-Heo evidence:owner-adversarial-batch-2026-08-05
3e19332 to
8c35ccf
Compare
…lization total Address review REQUEST_CHANGES on Yeachan-Heo#3823: - errorForDiagnostic now wraps the entire normalization in a single outer guard, so a hostile or revoked Proxy (whose getPrototypeOf/instanceof trap throws) cannot re-enter the crash path and mask the original fatal. Adds spawned fatal-handler regression coverage for both Proxy cases. - Drop the JSON.stringify fallback for plain objects: arbitrary opaque fields (e.g. a bare `token`) bypassed the regex redactor and persisted verbatim in the rotation-immune crash log. Only a string `.message` is surfaced now; plain objects without one fall back to the safe String() rendering. Arrays and non-plain objects (Date/RegExp/Map/Set/...) keep the existing fidelity. - Route `err.name` through redactCrashSecrets in both the crash-log header (recordFatalCrash) and the stderr banner (formatFatalError) so a PAT-shaped copied name no longer persists. - Sanitize name/message of terminal escape sequences and line/control characters at the Error source, the header, and the stderr banner so a hostile thrown name/message cannot forge record boundaries or inject terminal control sequences.
|
Rebased onto the current
Re-requesting review. |
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Exact-head adversarial review for 8c35ccfa2ef6eeb08a2d3dedc3823c81b68e7f13 (PR #3823, base dev).
Terminal verdict: REQUEST_CHANGES
The code change is sound and closes all three prior blockers, but the changelog entry lands in a released section and the head carries no green CI evidence.
Blocking: changelog entry under released ## [0.12.12]
The ### Fixed note is inserted directly under ## [0.12.12] - 2026-08-05 (tag v0.12.12 exists), not under the empty ## [Unreleased] section above it. Per the repo changelog contract (AGENTS.md: "add entries under ## [Unreleased], never edit released sections"), this must move up to ## [Unreleased]. This is the same blocker previously raised on #3704's earlier iterations; the just-merged #3704 correctly placed its entry under ## [Unreleased].
Verified solid (no changes requested there)
- Never-throw on hostile/revoked Proxy — the entire
errorForDiagnosticnormalization is wrapped in a single outer guard, soinstanceof/Object.getPrototypeOftrap throws cannot mask the original fatal. Spawned fatal-handler fixtures for both a throwing-getPrototypeOfProxy and a revoked Proxy each exit 1 and record[Uncaught Exception]with no secondarygetPrototypeOf boom. - No credential leak — the JSON-snapshot fallback is removed entirely; only a string
.messageis surfaced, and plain objects without one fall back to the safeString()rendering, so opaque fields (e.g. a baretoken) cannot bypass the redactor. .nameredacted in both sinks —err.nameis routed throughredactCrashSecretsin the crash-log header (recordFatalCrash) and the stderr banner (formatFatalError); a PAT-shaped name becomes«redacted-github-token».- Control-char / line-break injection —
sanitizeCrashHeaderstrips terminal escapes and C0/DEL control bytes at the Error source and both sinks, preventing record-boundary forgery.
Verification (exact head)
bun test packages/utils/test/postmortem-crash-log.test.ts— 28 pass, 106 assertions, 0 fail.- Merge-tree vs current
dev(b621997e): 0 conflicts. - GitHub CI on this head is
action_required(Dev CI and Public site sync both waiting for workflow approval, 0 jobs); no green exact-head CI evidence exists to date.
Required before merge: (1) move the changelog entry to ## [Unreleased]; (2) approve the pending Dev CI workflow and obtain green exact-head CI on a new head.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
|
Pushed The only outstanding item is exact-head CI: both Dev CI and Public site sync are still |
…lization total Address review REQUEST_CHANGES on Yeachan-Heo#3823: - errorForDiagnostic now wraps the entire normalization in a single outer guard, so a hostile or revoked Proxy (whose getPrototypeOf/instanceof trap throws) cannot re-enter the crash path and mask the original fatal. Adds spawned fatal-handler regression coverage for both Proxy cases. - Drop the JSON.stringify fallback for plain objects: arbitrary opaque fields (e.g. a bare `token`) bypassed the regex redactor and persisted verbatim in the rotation-immune crash log. Only a string `.message` is surfaced now; plain objects without one fall back to the safe String() rendering. Arrays and non-plain objects (Date/RegExp/Map/Set/...) keep the existing fidelity. - Route `err.name` through redactCrashSecrets in both the crash-log header (recordFatalCrash) and the stderr banner (formatFatalError) so a PAT-shaped copied name no longer persists. - Sanitize name/message of terminal escape sequences and line/control characters at the Error source, the header, and the stderr banner so a hostile thrown name/message cannot forge record boundaries or inject terminal control sequences.
0887ff4 to
edd0c13
Compare
|
Rebased again onto the latest |
…gnostics
errorForDiagnostic reduced any thrown non-Error to new Error(String(reason)),
which stringifies a plain object to "[object Object]". A structured startup-
failure shape such as { phase, reason, message } therefore surfaced in the
crash log and on stderr as an opaque "[object Object]", hiding the real reason
behind a generic fatal crash.
Surface .message (and .name) when present, otherwise fall back to a JSON
snapshot of the object's own properties. Error instances and primitive reasons
are unchanged, and arrays keep their existing String() format ("1,2,3").
errorForDiagnostic runs on the uncaughtException path and must never throw, so
every property read and serialization is guarded: a throwing getter (or a Proxy
get-trap) on .message/.name, a circular structure, or a non-serializable value
cannot re-enter the crash path. A throwing .name getter no longer clobbers a
valid .message. The terminal String(reason) fallback is also guarded, since
String(Symbol()) throws.
- packages/utils/src/postmortem.ts: errorForDiagnostic handles plain objects
- packages/utils/test/postmortem-crash-log.test.ts: regression + never-throw tests
- packages/utils/CHANGELOG.md: Unreleased entry
…lization total Address review REQUEST_CHANGES on Yeachan-Heo#3823: - errorForDiagnostic now wraps the entire normalization in a single outer guard, so a hostile or revoked Proxy (whose getPrototypeOf/instanceof trap throws) cannot re-enter the crash path and mask the original fatal. Adds spawned fatal-handler regression coverage for both Proxy cases. - Drop the JSON.stringify fallback for plain objects: arbitrary opaque fields (e.g. a bare `token`) bypassed the regex redactor and persisted verbatim in the rotation-immune crash log. Only a string `.message` is surfaced now; plain objects without one fall back to the safe String() rendering. Arrays and non-plain objects (Date/RegExp/Map/Set/...) keep the existing fidelity. - Route `err.name` through redactCrashSecrets in both the crash-log header (recordFatalCrash) and the stderr banner (formatFatalError) so a PAT-shaped copied name no longer persists. - Sanitize name/message of terminal escape sequences and line/control characters at the Error source, the header, and the stderr banner so a hostile thrown name/message cannot forge record boundaries or inject terminal control sequences.
The prior rebase landed the ### Fixed note under the released ## [0.12.12] section (tag v0.12.12 exists). Released sections are immutable: move the single entry up to the empty ## [Unreleased] and leave [0.12.12] matching the released tag verbatim.
edd0c13 to
5d33bfd
Compare
|
CI가 안 도는 이유를 확인했다 — 네 잘못이 아니다. 이 PR의 워크플로 런은 푸시를 더 해도 달라지지 않는다. 승인 없이는 새 런도 같은 상태로 들어간다. 같은 사유로 막힌 PR이 6건이라 #3940 으로 정리해 올렸다. 리뷰는 CI와 무관하게 진행하고 있으니 코드 피드백은 그대로 받으면 된다. 다만 이 저장소는 머지에 exact-head CI 증거를 요구하므로, 승인이 떨어지기 전까지는 머지가 불가능하다는 점만 알아두면 된다. |
|
Closing during the emergency maintenance freeze. This PR is not in the retained critical or maintainer-owned set. Do not open a replacement PR unless a maintainer explicitly directs it. — |
What
errorForDiagnosticnow preserves the.message/.nameof thrown non-Error objects in crash diagnostics instead of reducing them to[object Object], and is hardened so it can never throw on theuncaughtExceptionpath.Why
errorForDiagnosticreduced any thrown non-Error tonew Error(String(reason)), which stringifies a plain object to[object Object]. A structured startup-failure shape such as{ phase, reason, message }therefore surfaced in the crash log and on stderr as an opaque[object Object], hiding the real reason behind a generic fatal crash.Change
packages/utils/src/postmortem.ts:errorForDiagnosticsurfaces.message(and.name) when present on a thrown plain object, otherwise falls back to a best-effort JSON snapshot. Error instances and primitives are unchanged; arrays keep their existingString()format ("1,2,3").uncaughtException/unhandledRejectionpath, so every property read and serialization is guarded — a throwing getter (or Proxy get-trap) on.message/.name, a circular structure, or a non-serializable value cannot re-enter the crash path. A throwing.namegetter does not clobber a valid.message. The terminalString(reason)fallback is also guarded (an array element whosetoStringthrows reaches it).Testing
bun test packages/utils/test/postmortem-crash-log.test.ts→ 24 pass, including: throwing.messagegetter never throws; throwing.namegetter preserves.message; arrays keep"1,2,3"; array element with throwingtoStringnever throws; circular object.tsc --noEmit -p packages/utils/tsconfig.json→ clean.biome checkon changed files → clean.Focused checks per CONTRIBUTING ("focused tests first"); the full
bun checkis left for CI.GJC verdict
Independent architect re-review (second pass): the original HIGH (a throwing getter could re-throw on the never-throw crash path) and the array behavior change are both resolved — APPROVE / ship-with-nits.
devbun checkpasses (focused checks pass locally; full check pending CI)