Conversation
isBenignAbortTeardown only recognized parenthesized (file:line:col) stack frames, so a real app TypeError whose stack used bare `at file:line:col` frames — or whose throw site lived only in JSC's hidden sourceURL/originalLine fields — was misclassified as benign abort teardown and suppressed from crash.log. Reject the benign shape when JSC source fields are present, or when any stack line carries a non-native line:col frame, parenthesized or not.
Contributor
|
✅ Deterministic PR hygiene checks passed. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: lidge-jun/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe crash guard now rejects errors with JSC source metadata or non-native JavaScript stack frames. Tests cover parenthesized and unparenthesized frames, hidden fields, native-only errors, and locked ChangesCrash guard classification
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request is already Ready for Review. |
Owner
리뷰 · 우선순위 64 / 80이 PR은 crash-guard가 “무시해도 되는 abort teardown”으로 잘못 분류하던 진짜 TypeError를 crash.log에 다시 남기게 고칩니다. 예전에는 스택에 라인 - 메인테이너의 판단이 필요한 지점 실제 Bun에서 “진짜 abort teardown” TypeError 한 건을 잡아 보면, 너의 추천 버그 방향(진짜 TypeError를 삼키지 않기)은 맞고 테스트도 요지를 덮습니다. Bun benign 샘플에서 숨은 필드가 비어 있다는 것만 확인되면 이 댓글은 grok-bot이 작성했습니다 |
luvs01
marked this pull request as ready for review
September 20, 2026 07:49
luvs01
marked this pull request as ready for review
September 20, 2026 08:03
Owner
|
The exact-head diff is narrow and the classification change looks sound: it rejects both parenthesized and unparenthesized JS frames and treats populated JSC source-location fields as non-benign, while preserving the two known native-only teardown shapes. I cannot refresh this branch through GitHub because current |
Collaborator
Author
|
Refreshed onto current dev via update-branch (head 8465055) — the classification change is untouched, only the dev merge was added. Re-completed the readiness checklist; exact-head CI is running. |
luvs01
marked this pull request as ready for review
September 21, 2026 23:51
3 tasks
lidge-jun
added a commit
that referenced
this pull request
Sep 22, 2026
…window Review of the carried #5286 frame fix found the remaining gap: a benign teardown whose JSC hidden sourceURL/line/column names a different throw site was still folded silently for five minutes, so a distinct fault could vanish between summaries. Log the summary when that hidden throw site differs from the last one logged; repeats of the same site, and errors without one, still fold. Classification is unchanged.
Owner
|
Reimplemented in #5598 with a Co-authored-by trailer. The unparenthesized-frame fix is carried as is. The hidden sourceURL/line/column veto is replaced: Bun can set those fields on errors raised from its own builtin frames, so the benign summary is instead written whenever the hidden throw site changes. Thank you, @luvs01. Closing in favor of #5598. |
lidge-jun
added a commit
that referenced
this pull request
Sep 23, 2026
…ck proxy, OAuth, remote hub docs) (#5598) * fix(kiro): avoid mixed-script estimator allocations (cherry picked from commit 24c3cd3) * test(kiro): register kiro-wire-estimate in test layout (cherry picked from commit e796b5c) * fix(crash-guard): count unparenthesized JS frames as real throw sites isBenignAbortTeardown only recognised parenthesized "(file:line:col)" frames, so a genuine TypeError whose stack carries "at /abs/x.ts:1:2", "at file:///x.ts:1:2" or a Windows drive path was folded into the rate-limited benign-teardown summary. Scan every "at ..." frame and treat any non-native line:col location as a JS source frame. Hidden JSC fields (sourceURL/line/column) deliberately do not veto the benign class: Bun can attach them to errors raised from builtin frames, and the benign summary already records them through diagnose(). Reimplements #5286. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> * fix(proxy): keep loopback traffic outside inherited SOCKS (cherry picked from commit 3746991) * fix(proxy): pin non-loopback SOCKS ownership and document the ambient noProxy scope (cherry picked from commit 7dd72d1) * fix(oauth): preserve hashes in command code callback JSON (cherry picked from commit 225eb81) * test(oauth): drive Command Code callback JSON through the shared submit path The provider parser, not the shared code#state gate, owns the state check for pasted Command Code callback JSON. Pin both halves end to end: a wrong-state paste is accepted by submitManualLoginCode, rejected by the provider loop and re-prompted without a whoami call, and the right-state paste keeps "#" inside its fields intact through to the stored key. Follow-up to the carried #5413 fix. * docs(remote-hub): warn about shared-host unauthenticated loopback companion (cherry picked from commit a0233ca) * docs(remote-hub): align the loopback warning with the reference and cover the ported form (cherry picked from commit b00ed1e) * fix(oauth): drop legacy credential backup on destructive mutations auth.json.pre-multiauth copies the whole legacy store for downgrade recovery, but removeCredential/removeAccount left it behind — and a legacy-shaped store re-created it — so logout and account deletion kept a file holding the very refresh tokens the user destroyed. mutateStore gains a removeLegacyBackup option: it skips the one-time create and unlinks the backup after persist. Removal is best-effort (ENOENT ignored, other failures warn) because it runs after the store is persisted — a failed unlink must not report a failed logout for an account that is already gone. A stale uninstall-manifest entry is harmless: removeOwnedConfigState skips missing paths. Covers the destructive-migration, pre-existing-backup, and stale-backup-on-migrated-store cases in oauth-store-multi tests. (cherry picked from commit f456a4f) * docs(structure): note legacy backup removal on destructive auth mutations (cherry picked from commit b00133c) * fix(oauth): only drop the downgrade backup when a credential was actually removed removeCredential and removeAccount passed removeLegacyBackup unconditionally, so a stale or concurrent request that returned "not-found" or false still unlinked auth.json.pre-multiauth and still skipped creating it for a legacy store. That backup is a whole-store copy, so a removal that deleted nothing destroyed downgrade recovery for every provider in it. Decide from the mutation result instead. The decision moves to just after the mutation body, which only edits the in-memory store, so nothing has touched disk when it is taken, and the create and remove paths stay mutually exclusive as before. Cover both no-op results. (cherry picked from commit 160d3ab) * fix(oauth): drop the downgrade backup when a provider is deleted Deleting a provider from the dashboard clears its credentials through replaceProviderAccountSet(name, null), which bypassed the carried logout/account-deletion rule and kept (or first created) the auth.json.pre-multiauth copy of the deleted tokens. Treat clearing a provider that had credentials as destructive; a no-op clear and a non-empty replacement keep the backup. Pin the warning path when the backup cannot be removed after the credential is already gone. Completes the reimplementation of #4949. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> * fix(crash-guard): log a new hidden throw site inside the benign fold window Review of the carried #5286 frame fix found the remaining gap: a benign teardown whose JSC hidden sourceURL/line/column names a different throw site was still folded silently for five minutes, so a distinct fault could vanish between summaries. Log the summary when that hidden throw site differs from the last one logged; repeats of the same site, and errors without one, still fold. Classification is unchanged. * fix(proxy): merge loopback into an inherited lowercase no_proxy Review of the carried #5430 fix: Bun's native fetch reads a non-empty lowercase no_proxy before NO_PROXY, so with HTTP_PROXY and an inherited no_proxy the loopback entries written to NO_PROXY never applied and local calls could still go through the proxy. Merge the same configured and loopback entries into an inherited no_proxy, keeping its own entries. The non-loopback SOCKS test now uses a local proxy that drops every connection and an IP-literal target, so it no longer waits on DNS. * fix(oauth): keep other providers in the downgrade backup on deletion Review of the carried #4949 change: deleting one provider removed the whole auth.json.pre-multiauth file, so a legacy store with several providers lost downgrade recovery for every provider the user kept (the migrated auth.json is unreadable to an older loader). Destructive mutations now remove only the affected provider's entry and delete the file once it is empty. A backup entry that is not a regular file is removed rather than followed, and the rewrite replaces the entry itself without resolving a symlink. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> * fix(oauth): check an explicit #state on a raw Command Code paste Review of the carried #5413 change found the direct CLI prompt path, which does not go through submitManualLoginCode, accepting a raw "key#state" paste whose state did not match: only URL- and query-shaped input was compared. Treat a raw paste with an explicit #state suffix as state-bearing, as the shared submit gate already does; a bare key still needs no state. The end-to-end submit test now observes the login promise from the start so an early failure cannot leave it unhandled. * test: tighten the carried Kiro estimate and remote-hub warning checks Review nits on the carried #5248 and #5444 units: document that the count-based estimator takes non-negative integer counts, pin the pure-CJK and empty-model-id cases against the replacement-string formula, and require the dedicated-host warning to sit inside the danger callout in both locales rather than anywhere in the page. * fix(crash-guard): keep the hidden throw-site read inside the handler Re-review found that reading the hidden JSC fields for the benign fold could throw from an unusual accessor before the rejection was logged, escaping the process unhandledRejection listener. Make the read best-effort, as diagnose() already is, and pin it with a Proxy whose sourceURL getter throws. * fix(oauth): never write the downgrade backup through a link or claim it Re-review of the backup scrub found two gaps. backupLegacyOnce used existsSync, which reports a dangling symlink as absent, and then copied through it, so credentials could land outside the config directory; it now treats any existing entry as occupied and copies exclusively. The scrub rewrite went through the shared atomic writer, which records the path for uninstall, so a backup this install never registered was claimed and later deleted; it now replaces the entry through an exclusive private temp and a rename, leaving ownership unchanged. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> * test(kiro): make the pure-CJK estimate case actually zero-Latin * fix(oauth): rewrite the downgrade backup through the shared writer Re-review: the local temp-and-rename used for the backup scrub skipped what the shared atomic writer guarantees (Windows ACL hardening before the temp holds a byte, an explicit 0600 on POSIX regardless of umask, scrub-and-unlink of a failed temp, the Windows rename retry), which structure/config.md forbids replacing. Add a no-follow variant of that writer that leaves the owner manifest untouched, and use it, so a backup an earlier install left unregistered still stays unclaimed. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> * docs(structure): state when an existing OAuth downgrade copy is rewritten * fix(proxy): scope the inherited-proxy loopback bypass to exact hosts Security review of the carried #5430 change: on the no-config.proxy path loopback names were added to NO_PROXY for any inherited proxy, and both matchers treat entries as domain suffixes, so "localhost" also sent any *.localhost name direct, which need not resolve to loopback. Add the loopback entries only when an inherited SOCKS proxy owns HTTP(S) traffic; that proxy is applied by the in-process matcher, which now treats a bare localhost or IP-literal entry as one host (a leading dot still means subdomains). An inherited HTTP(S) proxy is Bun's own and is left as it was. Tests cover *.localhost staying on the SOCKS proxy, the no-widening cases, and the exact-match rules. * fix(proxy): key the loopback bypass on SOCKS precedence Security re-review: with an inherited SOCKS proxy beside an inherited HTTP(S) proxy the fetch wrapper still applies SOCKS first, so skipping the loopback entries in that case let local requests pass through SOCKS. Add them whenever an inherited SOCKS proxy exists; its exact matcher decides first, so only a request already judged loopback reaches Bun's suffix-matching proxy. Pin the mixed case. * fix(proxy): keep 127.0.0.1 off an inherited HTTP proxy without suffix risk Delta review: with no config.proxy and only an inherited HTTP(S) proxy, loopback calls such as the CLI's own management and health requests to 127.0.0.1 could go to that proxy. Add the loopback addresses there (a URL host ending in a numeric label parses as IPv4, so Bun's suffix matching cannot widen them) but not "localhost", which Bun would match as *.localhost. An inherited SOCKS proxy keeps the full list, and a process with no inherited proxy is still left untouched. * fix(proxy): add bare localhost only when SOCKS is the sole inherited proxy Security re-review: beside an inherited HTTP(S) proxy, a bare localhost in process-wide NO_PROXY is also read by Bun, whose suffix matching can send an unwrapped fetch for any *.localhost name past that proxy. Write the full loopback list only when an inherited SOCKS proxy is the only inherited proxy; whenever Bun applies an inherited HTTP(S) proxy, add only the loopback addresses. The mixed case keeps 127.0.0.1 direct through the SOCKS wrapper and sends *.localhost to SOCKS. * fix(proxy): never add a bare localhost to an inherited lowercase no_proxy Review: with a configured HTTP(S) proxy and an inherited lowercase no_proxy, which Bun reads first and matches by domain suffix, adding a bare localhost newly let *.localhost names bypass the proxy. The lowercase value now gains the configured entries and the loopback addresses only. * fix(proxy): add only loopback addresses to an inherited lowercase no_proxy Delta review: configured noProxy entries were also copied into an inherited lowercase no_proxy, which Bun reads first and matches by suffix; on dev that inherited value always shadowed them, so a configured "localhost" newly let *.localhost bypass a configured proxy. The lowercase value now gains the loopback addresses only. * test(oauth): assert a cleared provider set as null, as getAccountSet returns Hosted CI shard 1/4 at 00e5111: getAccountSet returns null for a missing provider, so the two new backup tests' toBeUndefined() failed. * fix: bypass inherited HTTP ALL_PROXY for loopback Cover both ALL_PROXY casings and repair review notes for hub docs, Kiro fallback coverage, and OAuth backup comments. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> * fix: keep localhost direct with mixed inherited proxies Force native direct fetch for exact localhost when the inherited SOCKS wrapper is active, and cover both ALL_PROXY casing combinations. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> --------- Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> Co-authored-by: Epinephrine <luvs01@hanmail.net> Co-authored-by: lidge-jun <lidge-jun@users.noreply.github.com>
Dylan-Liew
pushed a commit
to Dylan-Liew/opencodex
that referenced
this pull request
Sep 23, 2026
…ck proxy, OAuth, remote hub docs) (lidge-jun#5598) * fix(kiro): avoid mixed-script estimator allocations (cherry picked from commit 24c3cd3) * test(kiro): register kiro-wire-estimate in test layout (cherry picked from commit e796b5c) * fix(crash-guard): count unparenthesized JS frames as real throw sites isBenignAbortTeardown only recognised parenthesized "(file:line:col)" frames, so a genuine TypeError whose stack carries "at /abs/x.ts:1:2", "at file:///x.ts:1:2" or a Windows drive path was folded into the rate-limited benign-teardown summary. Scan every "at ..." frame and treat any non-native line:col location as a JS source frame. Hidden JSC fields (sourceURL/line/column) deliberately do not veto the benign class: Bun can attach them to errors raised from builtin frames, and the benign summary already records them through diagnose(). Reimplements lidge-jun#5286. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> * fix(proxy): keep loopback traffic outside inherited SOCKS (cherry picked from commit 3746991) * fix(proxy): pin non-loopback SOCKS ownership and document the ambient noProxy scope (cherry picked from commit 7dd72d1) * fix(oauth): preserve hashes in command code callback JSON (cherry picked from commit 225eb81) * test(oauth): drive Command Code callback JSON through the shared submit path The provider parser, not the shared code#state gate, owns the state check for pasted Command Code callback JSON. Pin both halves end to end: a wrong-state paste is accepted by submitManualLoginCode, rejected by the provider loop and re-prompted without a whoami call, and the right-state paste keeps "#" inside its fields intact through to the stored key. Follow-up to the carried lidge-jun#5413 fix. * docs(remote-hub): warn about shared-host unauthenticated loopback companion (cherry picked from commit a0233ca) * docs(remote-hub): align the loopback warning with the reference and cover the ported form (cherry picked from commit b00ed1e) * fix(oauth): drop legacy credential backup on destructive mutations auth.json.pre-multiauth copies the whole legacy store for downgrade recovery, but removeCredential/removeAccount left it behind — and a legacy-shaped store re-created it — so logout and account deletion kept a file holding the very refresh tokens the user destroyed. mutateStore gains a removeLegacyBackup option: it skips the one-time create and unlinks the backup after persist. Removal is best-effort (ENOENT ignored, other failures warn) because it runs after the store is persisted — a failed unlink must not report a failed logout for an account that is already gone. A stale uninstall-manifest entry is harmless: removeOwnedConfigState skips missing paths. Covers the destructive-migration, pre-existing-backup, and stale-backup-on-migrated-store cases in oauth-store-multi tests. (cherry picked from commit f456a4f) * docs(structure): note legacy backup removal on destructive auth mutations (cherry picked from commit b00133c) * fix(oauth): only drop the downgrade backup when a credential was actually removed removeCredential and removeAccount passed removeLegacyBackup unconditionally, so a stale or concurrent request that returned "not-found" or false still unlinked auth.json.pre-multiauth and still skipped creating it for a legacy store. That backup is a whole-store copy, so a removal that deleted nothing destroyed downgrade recovery for every provider in it. Decide from the mutation result instead. The decision moves to just after the mutation body, which only edits the in-memory store, so nothing has touched disk when it is taken, and the create and remove paths stay mutually exclusive as before. Cover both no-op results. (cherry picked from commit 160d3ab) * fix(oauth): drop the downgrade backup when a provider is deleted Deleting a provider from the dashboard clears its credentials through replaceProviderAccountSet(name, null), which bypassed the carried logout/account-deletion rule and kept (or first created) the auth.json.pre-multiauth copy of the deleted tokens. Treat clearing a provider that had credentials as destructive; a no-op clear and a non-empty replacement keep the backup. Pin the warning path when the backup cannot be removed after the credential is already gone. Completes the reimplementation of lidge-jun#4949. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> * fix(crash-guard): log a new hidden throw site inside the benign fold window Review of the carried lidge-jun#5286 frame fix found the remaining gap: a benign teardown whose JSC hidden sourceURL/line/column names a different throw site was still folded silently for five minutes, so a distinct fault could vanish between summaries. Log the summary when that hidden throw site differs from the last one logged; repeats of the same site, and errors without one, still fold. Classification is unchanged. * fix(proxy): merge loopback into an inherited lowercase no_proxy Review of the carried lidge-jun#5430 fix: Bun's native fetch reads a non-empty lowercase no_proxy before NO_PROXY, so with HTTP_PROXY and an inherited no_proxy the loopback entries written to NO_PROXY never applied and local calls could still go through the proxy. Merge the same configured and loopback entries into an inherited no_proxy, keeping its own entries. The non-loopback SOCKS test now uses a local proxy that drops every connection and an IP-literal target, so it no longer waits on DNS. * fix(oauth): keep other providers in the downgrade backup on deletion Review of the carried lidge-jun#4949 change: deleting one provider removed the whole auth.json.pre-multiauth file, so a legacy store with several providers lost downgrade recovery for every provider the user kept (the migrated auth.json is unreadable to an older loader). Destructive mutations now remove only the affected provider's entry and delete the file once it is empty. A backup entry that is not a regular file is removed rather than followed, and the rewrite replaces the entry itself without resolving a symlink. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> * fix(oauth): check an explicit #state on a raw Command Code paste Review of the carried lidge-jun#5413 change found the direct CLI prompt path, which does not go through submitManualLoginCode, accepting a raw "key#state" paste whose state did not match: only URL- and query-shaped input was compared. Treat a raw paste with an explicit #state suffix as state-bearing, as the shared submit gate already does; a bare key still needs no state. The end-to-end submit test now observes the login promise from the start so an early failure cannot leave it unhandled. * test: tighten the carried Kiro estimate and remote-hub warning checks Review nits on the carried lidge-jun#5248 and lidge-jun#5444 units: document that the count-based estimator takes non-negative integer counts, pin the pure-CJK and empty-model-id cases against the replacement-string formula, and require the dedicated-host warning to sit inside the danger callout in both locales rather than anywhere in the page. * fix(crash-guard): keep the hidden throw-site read inside the handler Re-review found that reading the hidden JSC fields for the benign fold could throw from an unusual accessor before the rejection was logged, escaping the process unhandledRejection listener. Make the read best-effort, as diagnose() already is, and pin it with a Proxy whose sourceURL getter throws. * fix(oauth): never write the downgrade backup through a link or claim it Re-review of the backup scrub found two gaps. backupLegacyOnce used existsSync, which reports a dangling symlink as absent, and then copied through it, so credentials could land outside the config directory; it now treats any existing entry as occupied and copies exclusively. The scrub rewrite went through the shared atomic writer, which records the path for uninstall, so a backup this install never registered was claimed and later deleted; it now replaces the entry through an exclusive private temp and a rename, leaving ownership unchanged. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> * test(kiro): make the pure-CJK estimate case actually zero-Latin * fix(oauth): rewrite the downgrade backup through the shared writer Re-review: the local temp-and-rename used for the backup scrub skipped what the shared atomic writer guarantees (Windows ACL hardening before the temp holds a byte, an explicit 0600 on POSIX regardless of umask, scrub-and-unlink of a failed temp, the Windows rename retry), which structure/config.md forbids replacing. Add a no-follow variant of that writer that leaves the owner manifest untouched, and use it, so a backup an earlier install left unregistered still stays unclaimed. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> * docs(structure): state when an existing OAuth downgrade copy is rewritten * fix(proxy): scope the inherited-proxy loopback bypass to exact hosts Security review of the carried lidge-jun#5430 change: on the no-config.proxy path loopback names were added to NO_PROXY for any inherited proxy, and both matchers treat entries as domain suffixes, so "localhost" also sent any *.localhost name direct, which need not resolve to loopback. Add the loopback entries only when an inherited SOCKS proxy owns HTTP(S) traffic; that proxy is applied by the in-process matcher, which now treats a bare localhost or IP-literal entry as one host (a leading dot still means subdomains). An inherited HTTP(S) proxy is Bun's own and is left as it was. Tests cover *.localhost staying on the SOCKS proxy, the no-widening cases, and the exact-match rules. * fix(proxy): key the loopback bypass on SOCKS precedence Security re-review: with an inherited SOCKS proxy beside an inherited HTTP(S) proxy the fetch wrapper still applies SOCKS first, so skipping the loopback entries in that case let local requests pass through SOCKS. Add them whenever an inherited SOCKS proxy exists; its exact matcher decides first, so only a request already judged loopback reaches Bun's suffix-matching proxy. Pin the mixed case. * fix(proxy): keep 127.0.0.1 off an inherited HTTP proxy without suffix risk Delta review: with no config.proxy and only an inherited HTTP(S) proxy, loopback calls such as the CLI's own management and health requests to 127.0.0.1 could go to that proxy. Add the loopback addresses there (a URL host ending in a numeric label parses as IPv4, so Bun's suffix matching cannot widen them) but not "localhost", which Bun would match as *.localhost. An inherited SOCKS proxy keeps the full list, and a process with no inherited proxy is still left untouched. * fix(proxy): add bare localhost only when SOCKS is the sole inherited proxy Security re-review: beside an inherited HTTP(S) proxy, a bare localhost in process-wide NO_PROXY is also read by Bun, whose suffix matching can send an unwrapped fetch for any *.localhost name past that proxy. Write the full loopback list only when an inherited SOCKS proxy is the only inherited proxy; whenever Bun applies an inherited HTTP(S) proxy, add only the loopback addresses. The mixed case keeps 127.0.0.1 direct through the SOCKS wrapper and sends *.localhost to SOCKS. * fix(proxy): never add a bare localhost to an inherited lowercase no_proxy Review: with a configured HTTP(S) proxy and an inherited lowercase no_proxy, which Bun reads first and matches by domain suffix, adding a bare localhost newly let *.localhost names bypass the proxy. The lowercase value now gains the configured entries and the loopback addresses only. * fix(proxy): add only loopback addresses to an inherited lowercase no_proxy Delta review: configured noProxy entries were also copied into an inherited lowercase no_proxy, which Bun reads first and matches by suffix; on dev that inherited value always shadowed them, so a configured "localhost" newly let *.localhost bypass a configured proxy. The lowercase value now gains the loopback addresses only. * test(oauth): assert a cleared provider set as null, as getAccountSet returns Hosted CI shard 1/4 at 00e5111: getAccountSet returns null for a missing provider, so the two new backup tests' toBeUndefined() failed. * fix: bypass inherited HTTP ALL_PROXY for loopback Cover both ALL_PROXY casings and repair review notes for hub docs, Kiro fallback coverage, and OAuth backup comments. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> * fix: keep localhost direct with mixed inherited proxies Force native direct fetch for exact localhost when the inherited SOCKS wrapper is active, and cover both ALL_PROXY casing combinations. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> --------- Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> Co-authored-by: Epinephrine <luvs01@hanmail.net> Co-authored-by: lidge-jun <lidge-jun@users.noreply.github.com> (cherry picked from commit 19e124b)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
isBenignAbortTeardownonly treated parenthesized(file:line:col)stack frames as evidence of a real throw site, so a genuine app TypeError with unparenthesizedat file:line:colframes — or with its source location carried only in JSC's hiddensourceURL/originalLine/originalColumnfields — was misclassified as benign abort teardown and suppressed from crash.log.line:colstack frames whether or not they are parenthesized.Verification
bun test tests/service/crash-guard.test.ts— 16 pass, 0 fail, including new coverage for unparenthesized frames and hidden JSC source fields.bun x tsc --noEmit— clean.Checklist
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.
Summary by CodeRabbit