ai slop - #35410
Conversation
#35256 banned <iostream> and deleted the std::cerr lines ahead of each std::terminate() in bun-uws instead of replacing them, so these invariant guards now abort with a bare 'terminate called without an active exception'. Restore them as fputs(stderr) per the shim's own guidance, and add a source lint so they stay paired.
WalkthroughChangesuWS termination diagnostics
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: restores the five |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
This does not fix #15589. That issue is about the HTTP-level |
There was a problem hiding this comment.
LGTM — mechanical restore of the five stderr diagnostics that #35256 dropped, plus a source-lint guard mirroring no-iostream-include.test.ts. The inline nit about source-lints.yml paths: is worth doing but is a pre-existing gap (the sibling no-iostream-include test scans packages/bun-uws under the same filter) and merge_group: backstops it, so not blocking.
Extended reasoning...
Overview
Five fputs("Error: ...\n", stderr) lines added immediately before each std::terminate() in packages/bun-uws/src/{App.h,HttpContext.h,TopicTree.h}, plus #include <cstdio> in each, plus a new source-lint test that scans packages/bun-uws/src/**/*.{h,hpp,cpp} and asserts every std::terminate() is preceded by an fputs|fprintf ... stderr write. Grep confirms exactly five std::terminate() sites in bun-uws/src and all five are covered.
Security risks
None. These paths are internal invariant guards the PR description shows are unreachable from user JS today; the only runtime change is a fixed-string fputs to stderr immediately before a process-abort that already existed.
Level of scrutiny
Low. The C++ change is a mechanical restore of upstream-uWebSockets diagnostic strings that #35256 deleted rather than translated to fputs. No control-flow, allocation, or state changes. The lint test copies the structure of the adjacent no-iostream-include.test.ts (same glob root, same expect(count).toBeGreaterThan(0) vacuous-pass guard, same expect(violations).toEqual([]) shape) and its look-back loop matches the actual shape at every site (fputs on the line directly above std::terminate(), block-open the line before that).
Other factors
The one flagged nit — source-lints.yml paths: not listing packages/bun-uws/** — is real but (a) equally applies to the pre-existing no-iostream-include.test.ts which scans the same directories, (b) is backstopped by the unfiltered merge_group: trigger, and (c) is a two-line CI-config follow-up rather than a defect in this change. The PR description shows the test failing on the pre-fix tree and passing after, and no-iostream-include.test.ts still passing, which is the right USE_SYSTEM_BUN-style verification for a source-lint.
…s changes Bun installs its own std::set_terminate in JSCInitialize, so the crash is 'panic: A C++ exception occurred', not libstdc++'s default message. Also add packages/bun-uws and packages/bun-usockets to the source-lints workflow paths filter so a PR touching only those headers runs this lint (and no-iostream-include) at PR time rather than only at merge_group.
There was a problem hiding this comment.
LGTM — the workflow paths: addition addresses the earlier note; the rest is unchanged.
What was reviewed:
- Five
fputs(..., stderr)restorations beforestd::terminate()in App.h/HttpContext.h/TopicTree.h with<cstdio>added — matches upstream uWS wording, no behavior change on non-terminate paths. - Confirmed these are the only
std::terminate()sites underpackages/bun-uws/src, so the new lint'sterminates > 0guard and lookback both hold;no-iostream-includestill passes since only<cstdio>was added. source-lints.ymlpaths:now coverspackages/bun-uws/**andpackages/bun-usockets/**in bothpushandpull_request.
Extended reasoning...
Overview
This PR restores five diagnostic fputs("Error: ...\n", stderr) writes immediately before std::terminate() in packages/bun-uws/src/{App.h,HttpContext.h,TopicTree.h}, which #35256 dropped when it banned <iostream>. It adds <cstdio> to each header, adds test/internal/source-lints/uws-terminate-diagnostic.test.ts to keep every std::terminate() in bun-uws paired with a preceding stderr write, and (in response to the earlier review) adds packages/bun-uws/** / packages/bun-usockets/** to both paths: filters in .github/workflows/source-lints.yml.
Security risks
None. The only runtime effect is a fixed-string fputs to stderr on paths that immediately std::terminate() afterward — internal-invariant guards that (per the PR description and code) are not user-reachable today. No user-controlled data is formatted; no new inputs are parsed. The workflow change only widens a read-only lint job's trigger.
Level of scrutiny
Low. The C++ delta is purely additive diagnostic output on already-fatal paths and cannot affect any surviving control flow. The new test is a hermetic source-tree scan that mirrors the neighboring no-iostream-include.test.ts (same Glob/read pattern, same violations.sort(); expect(violations).toEqual([]) shape, same expect(count).toBeGreaterThan(0) vacuous-pass guard). The workflow edit is a two-line paths-filter widening.
Other factors
I grepped packages/bun-uws/src for std::terminate and confirmed exactly the five sites this PR touches, all now preceded by an fputs(..., stderr) on the immediately preceding non-blank line inside the same block — so the lint's lookback matches every current site and terminates > 0 cannot pass vacuously. <cstdio> is not covered by the <iostream> ban (the shim explicitly recommends fputs/fprintf). My prior review's only note (workflow paths filter) was addressed in 8df9fa7 and the thread is resolved. No CODEOWNERS apply to the touched paths.
There was a problem hiding this comment.
LGTM — the workflow paths: filter feedback from the earlier pass is addressed in 8df9fa7.
What was reviewed:
- Confirmed all five
std::terminate()sites inpackages/bun-uws/srcnow have a precedingfputs(..., stderr);<cstdio>is not insrc/banned-includes/andno-iostream-include.test.tsonly bans<iostream>. - The new lint follows the
no-iostream-includepattern and guards against a vacuous pass withexpect(terminates).toBeGreaterThan(0). - The
fputscalls sit only on crash-imminent paths, so there's no hot-path or behavioral impact.
Extended reasoning...
Overview
Restores five diagnostic error strings that #35256 dropped when it stripped std::cerr << ahead of each std::terminate() in bun-uws, replacing them with the fputs(..., stderr) form the iostream-ban shim itself prescribes. Adds <cstdio> to App.h, HttpContext.h, and TopicTree.h; adds a source-lint test enforcing the pairing; and (per the earlier review comment) adds packages/bun-uws/** and packages/bun-usockets/** to the source-lints workflow paths: filters.
Security risks
None. The only runtime change is writing a fixed string to stderr immediately before std::terminate() on internal-invariant guards that are not reachable from user JS today.
Level of scrutiny
Low. This is a mechanical restoration of upstream uWebSockets diagnostic strings on crash-only paths, plus a source-tree lint. There are no control-flow changes, no allocations, and no new codepaths reachable in normal operation. <cstdio> is lightweight and not on the banned-includes list; only iostream is shimmed.
Other factors
- Verified via grep that the five
std::terminate()sites inpackages/bun-uws/srcare exactly the five the PR touches — none missed. - The new lint mirrors
no-iostream-include.test.ts(sameGlob/readFileSync/sorted-violations shape) and includes a non-vacuous-scan guard. The look-back heuristic is simple but adequate for the current code shape and will name the exact file:line if a future site drops its diagnostic. - The one review comment from the previous pass (workflow
paths:gap) was addressed and the thread is resolved. - PR evidence shows the lint fails on the pre-fix headers and passes post-fix, and
no-iostream-include.test.tsstill passes.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/source-lints.yml:
- Around line 17-18: Update the source-lint test command in the workflow’s
affected job to run through the repository-required debug-build wrapper, using
“bun bd ...” rather than invoking “bun test test/internal/source-lints/”
directly. Apply this consistently to both newly added bun-uws and bun-usockets
trigger paths while preserving the existing lint target.
In `@packages/bun-uws/src/App.h`:
- Around line 422-434: Update the idleTimeout and maxLifetime validation
diagnostics before std::terminate() to include each rejected behavior value and
its valid range. Replace the current fixed fputs messages in the idleTimeout and
maxLifetime checks with formatted stderr output, preserving the existing
validation thresholds and termination behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 21344c9e-4f98-46d7-8617-4a89697341a2
📒 Files selected for processing (5)
.github/workflows/source-lints.ymlpackages/bun-uws/src/App.hpackages/bun-uws/src/HttpContext.hpackages/bun-uws/src/TopicTree.htest/internal/source-lints/uws-terminate-diagnostic.test.ts
|
This PR has been closed because it was flagged as AI slop. Many AI-generated PRs are fine, but this one was identified as having one or more of the following issues:
If you believe this was done in error, please leave a comment explaining why. |
This PR has been marked as AI slop and the description has been updated to avoid confusion or misleading reviewers.
Many AI PRs are fine, but sometimes they submit a PR too early, fail to test if the problem is real, fail to reproduce the problem, or fail to test that the problem is fixed. If you think this PR is not AI slop, please leave a comment.