fix: harden small runtime and installer edge cases#100258
Conversation
|
Codex review: needs maintainer review before merge. Reviewed July 5, 2026, 8:07 AM ET / 12:07 UTC. Summary PR surface: Source +63, Tests +211, Other +9. Total +283 across 24 files. Reproducibility: yes. from source inspection: current main still shows the replacement-string transcript paths, unbounded response.json reads, pacman-or-Arch checks, TLS 18789 default, and fatal Undici shape gap that the PR targets. I did not execute tests in this read-only review. Review metrics: none identified. Stored data model Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Maintainer decision needed
Security Review detailsBest possible solution: Land the batch after maintainers accept the narrow fail-closed compatibility changes and the current-head required checks finish, keeping the focused regression tests with each hardened boundary. Do we have a high-confidence way to reproduce the issue? Yes, from source inspection: current main still shows the replacement-string transcript paths, unbounded response.json reads, pacman-or-Arch checks, TLS 18789 default, and fatal Undici shape gap that the PR targets. I did not execute tests in this read-only review. Is this the best way to solve the issue? Yes; the fixes are applied at the owning boundaries and reuse existing SDK/time/truncation helpers where appropriate. The remaining question is merge-risk acceptance for the deliberate fail-closed behavior, not a better code location. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 785ab7477982. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +63, Tests +211, Other +9. Total +283 across 24 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
Review history (5 earlier review cycles)
|
|
Land-ready at Work completed:
Focused proof:
Broad proof:
Known proof gaps: none. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6afca08712
ℹ️ 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; | ||
| } | ||
| selectedDecision = decision; | ||
| allowDecisionArmed = decision !== "deny"; |
There was a problem hiding this comment.
Keep allow approvals unarmed after navigation
When an approval prompt includes Deny plus an allow option, keyboard navigation from the default Deny row triggers this selection-change handler before Enter. Setting allowDecisionArmed here means the first Enter on Allow resolves the approval immediately, bypassing the onSelect double-confirm gate added below; accidental arrow/Enter approval is exactly the scenario the confirmation is meant to prevent. Keep the allow option unarmed until the first select action for that selected row.
Useful? React with 👍 / 👎.
Co-authored-by: uditDewan <[email protected]>
Co-authored-by: 陈宪彪0668000387 <[email protected]>
Co-authored-by: simon-w <[email protected]>
Co-authored-by: sunlit-deng <[email protected]>
Co-authored-by: 杨浩宇0668001029 <[email protected]>
Co-authored-by: hailory <[email protected]>
Co-authored-by: connermo <[email protected]>
Co-authored-by: Iliya Abolghasemi <[email protected]>
Co-authored-by: ben.li <[email protected]>
Co-authored-by: harjoth <[email protected]>
dbd1baf to
12b0886
Compare
* fix(media): preserve dollar sequences in transcript echoes Co-authored-by: uditDewan <[email protected]> * fix(mcp): catch rejected gateway event handlers Co-authored-by: 陈宪彪0668000387 <[email protected]> * fix(auto-reply): support punctuated silent token prefixes Co-authored-by: simon-w <[email protected]> * fix(discord): keep voice log previews UTF-16 safe Co-authored-by: sunlit-deng <[email protected]> * fix(clawrouter): bound usage response reads Co-authored-by: 杨浩宇0668001029 <[email protected]> * fix(browser): bound CDP JSON response reads Co-authored-by: hailory <[email protected]> * fix(status): include current time in session status Co-authored-by: connermo <[email protected]> * fix(installer): require Arch detection before pacman Co-authored-by: Iliya Abolghasemi <[email protected]> * fix(apps): default TLS gateway deep links to port 443 Co-authored-by: ben.li <[email protected]> * fix(infra): keep Undici terminated exceptions nonfatal Co-authored-by: harjoth <[email protected]> * fix(auto-reply): avoid Unicode-unsafe token spread --------- Co-authored-by: uditDewan <[email protected]> Co-authored-by: 陈宪彪0668000387 <[email protected]> Co-authored-by: simon-w <[email protected]> Co-authored-by: sunlit-deng <[email protected]> Co-authored-by: 杨浩宇0668001029 <[email protected]> Co-authored-by: hailory <[email protected]> Co-authored-by: connermo <[email protected]> Co-authored-by: Iliya Abolghasemi <[email protected]> Co-authored-by: ben.li <[email protected]> Co-authored-by: harjoth <[email protected]>
Related: #100181, #100116, #100007, #99974, #99860, #100192, #100187, #100102, #99887, #100098
What Problem This Solves
Fixes ten small correctness gaps where users could see corrupted transcript echoes, dropped MCP event failures, missed custom silent tokens, malformed Discord previews, unbounded diagnostic responses, incomplete session status, wrong Linux package-manager routing, an incorrect TLS deep-link port, or fatal handling of a known transient Undici failure.
Why This Change Was Made
The source PRs were each reviewed against current
main, then rebuilt as focused maintainer commits because the rewritten repository history made safe fork-branch refreshes impractical. The batch keeps each fix bounded, preserves contributor co-authorship, adds or retains focused regression coverage, and includes one lint-driven simplification for Unicode-safe custom-token handling.The first exact-head CI run also exposed an unrelated deterministic assertion omission already present on
main: the new gateway MCP source-checkout consumer was routed but absent fromtest-projects' expected list. This branch includes the one-line expectation repair so current-main CI is green again.Source PRs and contributor credit:
TypeError("terminated")shape as nonfatalUser Impact
These edge cases now fail safely or produce the expected output, without broadening compatibility surfaces or changing unrelated behavior. Linux installers no longer confuse Debian's pacman game with Arch Linux, secure gateway links work without an explicit port, and network/diagnostic paths remain bounded.
Evidence
DeepLinksSecurityTestsSwift tests passed.src/scripts/test-projects.test.tsassertions passed after repairing the current-main routing expectation.bash -n scripts/install.shgit diff --checkpnpm exec oxfmt --checkon all changed TypeScript filescheck:changed: passed all selected typecheck, lint, guard, cycle, and tooling-test lanes (tbx_01kwrssmpd8f5wek4ec5ce2mxe, run 28736283867).patch is correct, confidence 0.94).