Conversation
Contributor
|
✅ Deterministic PR hygiene checks passed. |
Contributor
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 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; 7 remain after this review. 📝 WalkthroughWalkthroughThe shared token estimator now accepts pre-counted Latin and CJK characters. Kiro wire estimation uses this helper instead of constructing placeholder strings. Tests cover equivalent counts, wire expansion, rounding, empty input, and test-layout registration. ChangesToken estimation flow
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Refactor Merge Risk: ⚪ Minimal · up to This change avoids large replacement strings while preserving token estimates and Kiro wire behavior. The current evidence supports merging after normal checks. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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. Hygiene✅ Deterministic PR hygiene checks passed. |
Owner
리뷰 · 우선순위 62 / 80이 PR은 Kiro 와이어 토큰 추정에서 섞인 글(라틴+한글/한자)을 셀 때, 매번 라인 - 메인테이너의 판단이 필요한 지점 공개 헬퍼에 음수/NaN 가드를 넣을지, 아니면 “호출자가 이미 센 값만 넣는다”고 두고 가드를 생략할지. 또 너의 추천 머지해도 된다고 봅니다. 할당을 피하는 목적이 코드·테스트와 잘 맞고, base 이 댓글은 grok-bot이 작성했습니다 |
Owner
|
I approved the hosted runs for exact head |
Owner
추가 리뷰 · 우선순위 64 / 80이전 리뷰 이후 head가 라인 - 이번 델타에는 PR 본문 파일 변경이 없습니다. 이전 지적( 메인테이너의 판단이 필요한 지점 체크리스트·exact-head CI만 다시 통과시키면 이전 리뷰와 같은 기준으로 머지해도 되는지, 아니면 접두 공유/전부-CJK 테스트까지 이 PR에서 손볼지. 기능 변경은 없으므로 후자는 선택입니다. 너의 추천 추가 머지 블로커는 없습니다. 이 댓글은 grok-bot이 작성했습니다 |
Ingwannu
approved these changes
Sep 20, 2026
Ingwannu
left a comment
Ingwannu
left a comment
Owner
There was a problem hiding this comment.
Approved on the current dev merge head. The pre-counted script buckets preserve the existing estimate formula while removing large replacement-string allocations, and the parity/large-count regressions cover the intended boundary. Merge only after all replacement-head required checks are green.
luvs01
marked this pull request as ready for review
September 20, 2026 04:44
Collaborator
Author
|
The final integration review found one repository-contract omission: the new Head The implementation task ran Requesting a renewed maintainer review for the new exact head. The previous approval remains attached to |
This was referenced Sep 22, 2026
3 tasks
lidge-jun
added a commit
that referenced
this pull request
Sep 22, 2026
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.
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
estimateKiroWireTokensbuilds two replacement strings ("x".repeat(latin)and"\uac00".repeat(cjk)) on every mixed-script payload just to feed the shared estimator — two full-size transient allocations per call on large inputs.estimateTokensFromCharacterCounts(latin, cjk, modelId, contextWindow)to the shared estimator and delegatesestimateTokensto it, so callers that already counted scripts estimate without materializing text.Verification
bun test tests/lib/token-estimate.test.ts tests/providers/kiro/kiro-wire-estimate.test.ts— 22 pass, 0 fail.bun x tsc --noEmit— clean.Checklist
Summary by CodeRabbit
Improvements
Tests