Remove the bun feedback command - #38444
Conversation
Drops the feedback subcommand: the embedded eval/feedback.ts script, the dispatch in RunCommand::exec_with_cfg, its line in the top-level help text, the docs pages and the test. The cli CMD static only existed for that dispatch, so it goes too; the crash handler still gets the command tag via set_cmd_char.
|
Status: removal is complete on this branch (head 480a9fb). Verified by hand with a debug build: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (8)
💤 Files with no reviewable changes (7)
WalkthroughThe pull request removes the ChangesFeedback CLI removal
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — clean removal of bun feedback.
What was reviewed:
- Verified
cli::CMDhas no remaining readers anywhere insrc/; the crash handler still gets its tag viaset_cmd_char, which is untouched. - Checked
EVAL_TRIGGERand thesrc/js/eval/embed path are still used by repl/eval, so leaving them was correct. - Grepped for dangling references to the deleted docs —
docs/docs.json's Feedback tab points to/feedback(kept), notproject/feedback(deleted); no stale nav entries. - Confirmed no dead imports left in
run_command.rs(cli/CommandTagstill used).
Extended reasoning...
Overview
This PR retires the bun feedback command end-to-end. It is almost entirely deletions: the embedded src/js/eval/feedback.ts script (779 lines), the dispatch block and bun_feedback helper in run_command.rs, the help-text line in mod.rs, three docs files/sections, the test file, and its expected-durations.json entry. The one non-deletion change is removing the cli::CMD static (a RacyCell<Option<Tag>> written once at startup) and its write in create_context_data, plus rewording two comments that referenced it.
Security risks
None. This removes a feature that POSTed user-entered text and system telemetry to bun.report; deleting it strictly reduces surface. No new inputs are parsed, no new code paths are reachable.
Level of scrutiny
Low. Pure feature removal with no behavior added. The only thing that could go wrong is a dangling reference to the removed pieces, so I grepped for each: cli::CMD has no remaining readers in src/ (the old comment claiming "debug logging" read it was stale — nothing does); EVAL_TRIGGER and runtime_embed_file! are still used by exec_eval/repl so were correctly kept; docs/docs.json points its Feedback tab to /feedback (the file that was trimmed, not deleted) and nothing references project/feedback or snippets/cli/feedback; add_completions.txt's only "feedback" hit is the react-native-haptic-feedback package name; run_command.rs still uses its crate::cli / CommandTag imports elsewhere.
Other factors
The crash-handler command-char path is unaffected — bun_crash_handler::cli_state::set_cmd_char(cmd.char()) is still called in create_context_data; only the redundant CMD.write mirror was dropped. The PR description documents local verification against bun.test.ts, the crash-report-command-char test, and the repl suite. The removed test file's expected-durations.json entry is cleaned up in the same PR, matching the "delete dead code in the same PR" rule.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Two follow-up commits since the approval, in case you want to re-check before merging: 52a067e adds the "removed commands" tests to test/cli/bun.test.ts (the PR previously only deleted tests), and a74bd8f drops the two leftover comments from the CMD removal. No functional changes beyond the first commit. |
REVIEW.md asks not to add tests that check dead code stays dead; the removal is covered by deleting the dispatch, help line, docs and test.
|
Current state for re-review: the "removed commands" tests from 52a067e are gone again (480a9fb) after the review pointed at the REVIEW.md rule against tests for removed code, so the branch is the original removal plus the comment trim in a74bd8f. Diff against main is deletions only apart from the two doc comments in cli/mod.rs that referenced the removed CMD static. |
There was a problem hiding this comment.
LGTM — the earlier nit (the describe("removed commands") block) was dropped in 480a9fb, and the branch is now deletions only apart from trimming the two doc comments that referenced the removed CMD static.
What was reviewed:
- Confirmed
cli::CMDhas no remaining readers anywhere insrc/andset_cmd_charis still called, so crash-report command tags are unaffected. - Checked
docs/docs.jsondoes not reference the deletedproject/feedback.mdxorsnippets/cli/feedback.mdx; the survivingdocs/feedback.mdxis still linked. - Verified
EVAL_TRIGGER,CommandTag, anduse crate::cliinrun_command.rsare still used by the repl / hot-reload paths, so no imports go dead. src/js/eval/still containsnode-repl.tsandfuzzilli-reprl.ts, so leaving the codegen glob in place is correct.
Extended reasoning...
Overview
This PR retires the bun feedback command. It deletes the embedded src/js/eval/feedback.ts script, the dispatch and bun_feedback helper in src/runtime/cli/run_command.rs, the help-text line in src/runtime/cli/mod.rs, three docs files/sections, test/cli/feedback.test.ts, and its test/expected-durations.json entry. It also removes the cli::CMD process-global (whose only reader was the feedback dispatch) and trims two pre-existing doc comments that cross-referenced it. After my earlier review the author reverted the "removed commands" negative tests (480a9fb), so the diff against main is deletions only apart from those doc-comment trims.
Security risks
None. The deleted script POSTed user-supplied text and system metadata to bun.report; removing it strictly reduces attack surface. No auth, crypto, path handling, or untrusted-input parsing is added.
Level of scrutiny
Low-to-moderate. This is a mechanical feature removal with no new logic. The only non-deletion changes are (a) rewording two doc comments to drop references to the removed CMD static and (b) dropping the comment above set_cmd_char. The one thing worth checking — that CMD really had no other readers and that crash-report command tagging still works via set_cmd_char — I verified by grep, and the author ran test/cli/run/crash-report-command-char.test.ts. Removing a user-facing subcommand is a product decision, but the PR states it was requested by a maintainer.
Other factors
All prior review threads are resolved: the comment-cop flags on mod.rs were addressed in a74bd8f (the flagged doc comment predates the PR; only the [CMD] cross-references were removed), and my nit about the dead-code tests was addressed in 480a9fb. I confirmed no dangling references remain — docs/docs.json never listed project/feedback, src/js/eval/ still has node-repl.ts/fuzzilli-reprl.ts so the bundle-modules glob is still live, and the use crate::cli / CommandTag / EVAL_TRIGGER imports in run_command.rs are still consumed by the repl and hot-reload paths. The bug hunting system found nothing this run.
#39445) Stacked on #36463 (the base branch is that PR's branch, so the diff here is only the additions). Merging this into #36463 adds the behavior changes listed below; #36463 itself now covers the #38333 install batch, the optional-peer correction, and the TOML / `bun init` fixes, so this PR no longer touches those. ### Problem - These 1.3 to 1.4 behavior changes are not in the guide at `701b3e2a0`: - MySQL: the first `caching_sha2_password` connection over plain TCP is refused unless `allowPublicKeyRetrieval: true` (#31129; 1.3.14 requested the key automatically, `MySQLConnection.zig` in the 1.3.14 tag). SQL `tls` / `ssl` options now require TLS instead of falling back to plaintext, and `?ssl=` / `?ssl-mode=` are read (`shared.ts` 1.3.14 only read `?sslmode=`; #37669). - Install: `~/.npmrc` fallback when `XDG_CONFIG_HOME` is set (#36289), credentials in `--registry` / env / bunfig object URLs are sent and outrank same-host `.npmrc` tokens (#38796, #38824), `bun outdated` exits 1 on fetch failures (#38809), new `dedupe` / `up` commands shadow scripts of those names and `bun feedback` is removed (#38333, #38444), `workspace:` ranges inside registry packages (#37669), isolated store entry names (#39014). - Runtime: `module.enableCompileCache()` / `NODE_COMPILE_CACHE` implemented (#34660), `require()` / `import` not-found messages (#34660), `AbortError` message without the period (#39277; 1.3.14's `BunCommonStrings.h` has the period), GCM IV length (#34092), `mkdtemp("")` (#34908), vm options (#38381), `server.reload` (#38697), ICU 75/73 to 78 (#38013), Compression stream chunking (#38695), `Bun.SQL` sqlite bindings (#35950). - Bundler: `splitting` with `cjs` / `iife` is an error (#32685), block-scoped `enum` lowers to `let` (#34249), exports emitted ascending instead of descending (#35957; `doStep5.zig` in 1.3.14 used `sortDesc`), minified `$` (#35668). - The TOML integer bullet did not say what the limit or the fix is. ### Fix - Adds a MySQL public key section (plus a summary table row), a TLS note under the `PGSSLMODE` section, an `.npmrc` / credentials addendum to the `bunfig.toml` section, a `module.enableCompileCache()` section, and the rest as bullets in the existing lists. - `docs/pm/overrides.mdx`: one-line change adding a pointer to this guide in the existing `lockfileVersion` 3 limitation. (The base branch briefly had a duplicate "Nested overrides" section; it removed that itself in `8257d01acb`, and this PR was rebased over it.) - Verification: each runtime claim was run against `1.4.0-canary.1+8326d1bd3` (22 commits behind main; contains every change referenced), and each install or bundler claim was checked against the source on main, with the 1.3 side taken from the `bun-v1.3.14` tag where the PR body did not state it. The `/runtime/sql#mysql` and `/upgrade-to-1.4` links resolve. `prettier --check` passes. ### Not included on purpose - Lifecycle scripts no longer receiving `npm_package_name` / `npm_package_version` / `npm_package_json` / `npm_config_local_prefix` during `bun install`, and transitive `"*"` ranges no longer deduplicating onto the root's version: regressions with open fixes (#36690, #38110, #38770). They need either the fixes or a guide line before release. - Postgres `sslmode=prefer` / `allow` (including `PGSSLMODE=prefer`, which 1.4 newly reads) hangs until the connection timeout against a server without SSL because nothing sends the startup message after the `N` reply. Same code in 1.3.14; filed as a bug instead of documented. <details> <summary>Commands used to verify the runtime claims</summary> ``` timers/promises setTimeout with an aborted signal # "The operation was aborted" bun req.cjs # Cannot find module ... Require stack: bun b.mjs (import() of a missing package / relative file) # Cannot find package 'x' imported from /path, ERR_MODULE_NOT_FOUND bun a_static.mjs (unhandled static import) # printed line still: Cannot find package 'x' from '/path' process.versions.icu # 78.3 createCipheriv("aes-128-gcm", key, Buffer.alloc(129)) # ERR_CRYPTO_INVALID_IV DecompressionStream of a 1 MiB gzip member # 16 chunks of 65536 bytes new SQL("sqlite://:memory:") with ${[1,2]} / ${new Date()} # Binding expected ... fs.mkdtempSync("") # EINVAL vm.runInThisContext("1", []) # ERR_INVALID_ARG_TYPE NODE_COMPILE_CACHE=/tmp/cc bun cc.cjs # creates /tmp/cc/v1.4.0-x86_64-<sha>-<uid> NODE_DISABLE_COMPILE_CACHE=1 + enableCompileCache() # status 3 (DISABLED) bun dedupe / bun up with package.json scripts of those names # built-in command runs bun feedback # Script not found "feedback" Bun.build({ splitting: true, format: "cjs" }) # Code splitting is currently only supported ... bun build of a function-scoped enum and import * as ns # let Color; exports a, m, z new SQL({ url: "postgres://...", tls: true }) on a non-TLS server # ERR_POSTGRES_TLS_NOT_AVAILABLE Bun.TOML.parse("a = 9007199254740993") # Integer cannot be losslessly represented ... ``` </details> <details> <summary>Previous revision</summary> The first revision of this PR (`3c5611454a`) also rewrote the package manager section for #38333 / #38853 (nested overrides and `lockfileVersion: 3`, the optional-peer correction, `bun update`, `bunfig.toml` over `.npmrc`, `--filter`) and fixed the TOML date and `bun init` lines. #36463 picked those up in its own commits the same day, so this PR was rebased onto its new head and reduced to the items above. </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 0 · docs-only change; test-proof not applicable <!-- robobun:evidence:end -->
#39445) Stacked on #36463 (the base branch is that PR's branch, so the diff here is only the additions). Merging this into #36463 adds the behavior changes listed below; #36463 itself now covers the #38333 install batch, the optional-peer correction, and the TOML / `bun init` fixes, so this PR no longer touches those. ### Problem - These 1.3 to 1.4 behavior changes are not in the guide at `701b3e2a0`: - MySQL: the first `caching_sha2_password` connection over plain TCP is refused unless `allowPublicKeyRetrieval: true` (#31129; 1.3.14 requested the key automatically, `MySQLConnection.zig` in the 1.3.14 tag). SQL `tls` / `ssl` options now require TLS instead of falling back to plaintext, and `?ssl=` / `?ssl-mode=` are read (`shared.ts` 1.3.14 only read `?sslmode=`; #37669). - Install: `~/.npmrc` fallback when `XDG_CONFIG_HOME` is set (#36289), credentials in `--registry` / env / bunfig object URLs are sent and outrank same-host `.npmrc` tokens (#38796, #38824), `bun outdated` exits 1 on fetch failures (#38809), new `dedupe` / `up` commands shadow scripts of those names and `bun feedback` is removed (#38333, #38444), `workspace:` ranges inside registry packages (#37669), isolated store entry names (#39014). - Runtime: `module.enableCompileCache()` / `NODE_COMPILE_CACHE` implemented (#34660), `require()` / `import` not-found messages (#34660), `AbortError` message without the period (#39277; 1.3.14's `BunCommonStrings.h` has the period), GCM IV length (#34092), `mkdtemp("")` (#34908), vm options (#38381), `server.reload` (#38697), ICU 75/73 to 78 (#38013), Compression stream chunking (#38695), `Bun.SQL` sqlite bindings (#35950). - Bundler: `splitting` with `cjs` / `iife` is an error (#32685), block-scoped `enum` lowers to `let` (#34249), exports emitted ascending instead of descending (#35957; `doStep5.zig` in 1.3.14 used `sortDesc`), minified `$` (#35668). - The TOML integer bullet did not say what the limit or the fix is. ### Fix - Adds a MySQL public key section (plus a summary table row), a TLS note under the `PGSSLMODE` section, an `.npmrc` / credentials addendum to the `bunfig.toml` section, a `module.enableCompileCache()` section, and the rest as bullets in the existing lists. - `docs/pm/overrides.mdx`: one-line change adding a pointer to this guide in the existing `lockfileVersion` 3 limitation. (The base branch briefly had a duplicate "Nested overrides" section; it removed that itself in `8257d01acb`, and this PR was rebased over it.) - Verification: each runtime claim was run against `1.4.0-canary.1+8326d1bd3` (22 commits behind main; contains every change referenced), and each install or bundler claim was checked against the source on main, with the 1.3 side taken from the `bun-v1.3.14` tag where the PR body did not state it. The `/runtime/sql#mysql` and `/upgrade-to-1.4` links resolve. `prettier --check` passes. ### Not included on purpose - Lifecycle scripts no longer receiving `npm_package_name` / `npm_package_version` / `npm_package_json` / `npm_config_local_prefix` during `bun install`, and transitive `"*"` ranges no longer deduplicating onto the root's version: regressions with open fixes (#36690, #38110, #38770). They need either the fixes or a guide line before release. - Postgres `sslmode=prefer` / `allow` (including `PGSSLMODE=prefer`, which 1.4 newly reads) hangs until the connection timeout against a server without SSL because nothing sends the startup message after the `N` reply. Same code in 1.3.14; filed as a bug instead of documented. <details> <summary>Commands used to verify the runtime claims</summary> ``` timers/promises setTimeout with an aborted signal # "The operation was aborted" bun req.cjs # Cannot find module ... Require stack: bun b.mjs (import() of a missing package / relative file) # Cannot find package 'x' imported from /path, ERR_MODULE_NOT_FOUND bun a_static.mjs (unhandled static import) # printed line still: Cannot find package 'x' from '/path' process.versions.icu # 78.3 createCipheriv("aes-128-gcm", key, Buffer.alloc(129)) # ERR_CRYPTO_INVALID_IV DecompressionStream of a 1 MiB gzip member # 16 chunks of 65536 bytes new SQL("sqlite://:memory:") with ${[1,2]} / ${new Date()} # Binding expected ... fs.mkdtempSync("") # EINVAL vm.runInThisContext("1", []) # ERR_INVALID_ARG_TYPE NODE_COMPILE_CACHE=/tmp/cc bun cc.cjs # creates /tmp/cc/v1.4.0-x86_64-<sha>-<uid> NODE_DISABLE_COMPILE_CACHE=1 + enableCompileCache() # status 3 (DISABLED) bun dedupe / bun up with package.json scripts of those names # built-in command runs bun feedback # Script not found "feedback" Bun.build({ splitting: true, format: "cjs" }) # Code splitting is currently only supported ... bun build of a function-scoped enum and import * as ns # let Color; exports a, m, z new SQL({ url: "postgres://...", tls: true }) on a non-TLS server # ERR_POSTGRES_TLS_NOT_AVAILABLE Bun.TOML.parse("a = 9007199254740993") # Integer cannot be losslessly represented ... ``` </details> <details> <summary>Previous revision</summary> The first revision of this PR (`3c5611454a`) also rewrote the package manager section for #38333 / #38853 (nested overrides and `lockfileVersion: 3`, the optional-peer correction, `bun update`, `bunfig.toml` over `.npmrc`, `--filter`) and fixed the TOML date and `bun init` lines. #36463 picked those up in its own commits the same day, so this PR was rebased onto its new head and reduced to the items above. </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 0 · docs-only change; test-proof not applicable <!-- robobun:evidence:end -->
2 participants
Problem
bun feedbackis being retired (requested by @alii).src/js/eval/feedback.ts, the dispatch at the bottom ofRunCommand::exec_with_cfg(src/runtime/cli/run_command.rs) plus thebun_feedbackhelper that booted the script, its line in the top-level help text (src/runtime/cli/mod.rs), and the docs/test for it.Fix
docs/project/feedback.mdx,docs/snippets/cli/feedback.mdx, thebun feedbacksection ofdocs/feedback.mdx, andtest/cli/feedback.test.ts(plus itstest/expected-durations.jsonentry).cli::CMDstatic: the feedback dispatch was its only reader. The crash handler gets the command tag throughset_cmd_char, which is unchanged, so crash-report command characters are unaffected.src/js/eval/and thebundle-modules.tsglob that builds it stay, sincenode-repl.tsandfuzzilli-reprl.tsstill live there.bun feedback ...now falls through to the normalerror: Script not found "feedback"(exit 1), same as any other unknown script name, andbun --helpno longer lists it.test/cli/feedback.test.ts. Checked by hand with a debug build:bun feedback --helpin an empty directory printserror: Script not found "feedback"and exits 1 (the build from main prints the feedback usage text and exits 0), andbun --helpno longer lists it.bun bd test test/cli/bun.test.ts test/cli/run/crash-report-command-char.test.ts test/internal/parallel-allowlist.test.ts test/internal/source-lints/vm-thread-door.test.tsandtest/js/bun/repl/repl.test.ts(the repl covers the remainingeval/embed path) all pass.Background
src/js/eval/are not builtin modules;bundle-modules.tsminifies each one intocodegen/eval/and Rust pulls them in withruntime_embed_file!and runs them as if passed tobun -e.bun feedbackwas the only command dispatched this way from the "script not found" fallback inbun run; the repl uses the same mechanism from its own command entry point.cli::CMDwas a process-global copy of the running subcommand tag, written once at startup. The feedback dispatch read it to make sure it only fired for the barebun feedbackform (notbun run feedback). Nothing else read it.