chore: delete proven-dead helpers and stale dead_code allows - #5791
Conversation
Remove call-graph-dead builders, wrappers, and leftover identifiers from the TUI crate, and drop allow(dead_code) on items production already calls. Lower the dead-code budget 448 → 416. No behavior change. Co-authored-by: Hunter Bown <Hmbown@users.noreply.github.com>
rustc -D dead_code proved the persist_as_default helper's only remaining callee was also unreferenced. Drop the unused import with it. Co-authored-by: Hunter Bown <Hmbown@users.noreply.github.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_39947f5c-e307-4f49-88d5-42003fed5162) |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
Claude finished @Hmbown's task in 1m 54s —— View job Reviewing PR #5791
ReviewI walked every hunk in Deletions verified clean — no remaining references anywhere for: Import hygiene checked: Provider/model route safety (EPIC #2608): Live-item allow removals checked against real callers:
Doc-comment consistency: Budget JSON: Scope discipline: the PR correctly leaves alone the #5587 contributor-owned No issues found. This is a well-scoped, well-evidenced deletion PR — every removed item was independently confirmed unreferenced, no accidental removal of live code, and no provider/model safety regression. |
There was a problem hiding this comment.
🟢 Approval recommended
The removed items have no remaining in-repo references and the remaining edits are consistent dead-code allow cleanup plus bookkeeping updates.
Pull request overview
Removes a set of proven-unreferenced Rust helpers and trims stale #[allow(dead_code)] attributes in codewhale-tui, lowering the repository dead-code allowance budget without changing runtime behavior.
Changes:
- Deleted unused helpers/builders/wrappers across TUI, MCP, retry config, features, and world-state utilities.
- Dropped
#[allow(dead_code)]where items are now demonstrably live (or moved the allow to only the truly unwired persistence API methods). - Updated dead-code budget tracking plus changelog entries to reflect the sweep.
File summaries
| File | Description |
|---|---|
| scripts/dead-code-budget.json | Lowers the tracked dead-code allow ceiling to match the sweep result. |
| crates/tui/src/tui/translation.rs | Removes unused TranslationStatus enum and its doc mention. |
| crates/tui/src/tui/paste_burst.rs | Deletes unused paste-burst helper methods. |
| crates/tui/src/tui/app/types.rs | Removes unused ReasoningEffort::cycle_next_for_provider and updates docs to the new cycler. |
| crates/tui/src/tui/active_cell.rs | Deletes unused ActiveCell::forget_tool. |
| crates/tui/src/tools/registry.rs | Removes unused text-only execution wrapper method. |
| crates/tui/src/tools/handle.rs | Drops stale #[allow(dead_code)] now that handle primitives are used internally. |
| crates/tui/src/task_manager.rs | Removes unused TaskManager::is_shutdown and stale allow on shutdown. |
| crates/tui/src/skills/audit.rs | Deletes unused audit constant re-exports. |
| crates/tui/src/settings.rs | Removes unused provider/model persistence helpers and trims stale allows; keeps item-level allows only for unwired persistence API. |
| crates/tui/src/sandbox/policy.rs | Removes unused SandboxExecutor trait stub and now-unused imports. |
| crates/tui/src/runtime_threads.rs | Removes unused shutdown/is_shutdown APIs and unused resume wrapper; updates related comment. |
| crates/tui/src/model_context/world_state.rs | Removes unused world-diff rendering helpers while keeping the live diff type. |
| crates/tui/src/mcp.rs | Removes unused pool APIs (disconnect_all, config) and drops stale allow on shutdown_all. |
| crates/tui/src/llm_client/mod.rs | Removes unused retry timeout builder methods. |
| crates/tui/src/lib.rs | Updates internal docs to reference the remaining tool execution path. |
| crates/tui/src/features.rs | Deletes unused convenience accessors and enabled-features enumerator. |
| crates/tui/src/cost_status.rs | Removes unused builder method (with_identity). |
| crates/tui/CHANGELOG.md | Records the dead-code sweep entry for the TUI crate changelog. |
| CHANGELOG.md | Records the dead-code sweep entry for the workspace changelog. |
Review details
- Files reviewed: 20/20 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…t-used - HandleStore::retained_bytes had zero callers anywhere: deleted, not re-allowed. - TaskManager::shutdown is exercised only from automation_manager tests; scope its allow to non-test compilations so test builds stay honest. Also drops the branch-time CHANGELOG entry: entries are written to main at merge time as receipts now.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_1f6026b9-a090-4f65-86f5-79f33996d3eb) |
|
Claude finished @Hmbown's task in 3m 2s —— View job Reviewing PR #5791 (re-review after synchronize)
ReviewI pulled the full diff ( 🔴 Compile-breaking bug: Commit - /// Bytes currently held across every session.
- #[must_use]
- pub fn retained_bytes(&self) -> usize {
- self.retained_bytes
- }with the message "
The private Fix: restore the accessor (simplest, matches the existing test API): #[must_use]
pub fn retained_bytes(&self) -> usize {
self.retained_bytes
}Everything else checks out. I re-verified the remaining deletions with fresh greps (not reused from any prior pass) and found no other dangling references:
Not independently verified: the Net: one real, concrete regression — a test-build compile failure from an over-eager deletion in the last "keep the sweep green" commit, ironically. Everything else in this fairly large sweep is clean and well-scoped. Should be a quick fix (restore one 4-line method) before merge. |
![codewhale-agent[bot]](https://avatars.githubusercontent.com/u/101357273?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codewhale review
Dead-code sweep removes unused helpers and stale allows across the TUI crate and updates the dead-code budget. The deletions are mostly localized and supported by the stated reference search, but the PR also carries unrelated release-note changes and removes public API items without a workspace-wide build or an explicit internal/public policy note.
Findings
- [WARNING] Unrelated CHANGELOG entries included in dead-code-only PR (
CHANGELOG.md)
The diff adds user-facing changelog entries for remote-control recovery (#5605), compaction survival/history (#4394), and the phantom session duplicate fix to both CHANGELOG.md and crates/tui/CHANGELOG.md. This is inconsistent with the PR's 'No architecture, TUI layout, or product behavior change' statement and risks misattributing unrelated work to this cleanup sweep. Drop these entries or split them into the owning PR. - [WARNING] Public API deletions only validated in codewhale-tui --lib
The PR removes a number ofpubitems (e.g. Features::key/stage/default_enabled/enabled_features, McpPool::disconnect_all/config, RuntimeThreadManager::resume_thread_with_agent_rebind/shutdown/is_shutdown, TaskManager::is_shutdown, TranslationStatus, ToolRegistry::execute_full_with_context, ReasoningEffort::cycle_next_for_provider, SandboxExecutor). The validation described is a workspace-wide reference search plusrustc -D dead_codeon codewhale-tui --lib, but there is nocargo check --workspace --all-targetsevidence. If another crate or target calls these public methods, this will fail after merge. Also noterustc dead_codedoes not flag public lib items as dead, so the proof for these is the search, not the compiler. - [INFO] TaskManager::shutdown remains dead in production with a cfg_attr allow (
crates/tui/src/task_manager.rs)
The comment now says it is only exercised from automation_manager tests, but the method is still compiled in non-test builds and marked#[cfg_attr(not(test), allow(dead_code))]. If it is test-only,#[cfg(test)]would be cleaner; if it is intended public API, it should have a documented consumer/contract rather than an allow.
Assessment
The core dead-code deletions are low-risk and mostly well documented, but the PR should not merge until the unrelated changelog entries are removed and the deletion of public API surfaces is validated against the whole workspace with an explicit decision on whether those APIs are internal or public.
Advisory review by Codewhale (codewhale review --pr 5791 --post, head ae335cab84f6c18f538af7383a8ba93892f345b2). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_8d6045eb-ac3b-4c4b-bfda-a6dc6a342726) |
|
Claude finished @Hmbown's task in 2m 23s —— View job Reviewing PR #5791 (re-review after synchronize)
Review🔴 The Timeline in
But the method-call sites were never touched. Confirmed by grep against current HEAD — ( Fix: either restore the accessor: #[must_use]
pub fn retained_bytes(&self) -> usize {
self.retained_bytes
}or replace the four Everything else re-checked clean. No other files changed between my prior review and this HEAD except
Net: this PR still cannot land as-is — the test build is broken by the |
… scoped allow The sweep's reference search missed four #[cfg(test)] callers in this file. Same treatment as TaskManager::shutdown: scoped allow, not deletion.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_bbc7652c-228a-41ac-ba61-5d5d240f32c4) |
|
Claude finished @Hmbown's task in 2m 57s —— View job Reviewing PR #5791 (re-review after synchronize — commit
|
There was a problem hiding this comment.
Codewhale review
The PR performs a high-confidence dead-code sweep in codewhale-tui, deleting several unused helpers, methods, and constants while relaxing stale #[allow(dead_code)] attributes. The changes mostly align with the description and are backed by compiler and focused test evidence, but the diff also includes unrelated changelog entries and removes multiple pub items that comments previously described as public API.
Findings
- [WARNING] Unrelated CHANGELOG entries are included in the PR (
CHANGELOG.md)
BothCHANGELOG.mdandcrates/tui/CHANGELOG.mdadd entries for behavior changes (#5605 remote-control recovery, #4394 compaction survival contract, session-id crash recovery, and locale-aware auth routes) that are unrelated to this dead-code sweep. The PR body states no product behavior change, so these entries likely belong to other PRs and should be removed or the branch rebased onto currentmain. - [WARNING] Public API items removed without deprecation or external-consumer check
Several removed items arepuband their original#[allow(dead_code)]comments explicitly described them as public API:McpPool::disconnect_all,McpPool::config,RuntimeThreadManager::shutdown,RuntimeThreadManager::is_shutdown,TaskManager::is_shutdown,Settings::set_provider_model_selection,RetryConfig::with_request_timeout,RetryConfig::with_total_timeout,AUDIT_MAX_PACKAGE_BYTES,AUDIT_MAX_FILES,Feature::{key,stage,default_enabled},ReasoningEffort::cycle_next_for_provider, andSandboxExecutor. The PR leftprotocol::workroomalone because it is a public protocol API, but applies a different standard here. Ifcodewhale-tuihas any external consumers, these removals are breaking changes. - [INFO] TaskManager::shutdown still carries a dead_code allow despite being listed as live (
crates/tui/src/task_manager.rs:1191)
The PR body listsTaskManager::shutdownunder "Stale allows dropped (item is live)" and says it is called from automation manager + tests. The diff instead converts the unconditional allow to#[cfg_attr(not(test), allow(dead_code))]and documents "Only exercised from automation_manager tests today." This means the method is still dead in non-test builds. Either update the PR description or verify whether production automation_manager should call this method and remove the remaining allow if so.
Assessment
The dead-code removals are mostly well-evidenced and mechanically sound. Before merge, the PR should be cleaned to remove unrelated changelog entries, and the public API implications should be explicitly confirmed. The TaskManager discrepancy should also be reconciled.
Advisory review by Codewhale (codewhale review --pr 5791 --post, head d381b5f0ce985b06681ed5178ef58698cfb151e5). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.
| } | ||
|
|
||
| #[allow(dead_code)] // Public API for external callers (runtime API) | ||
| /// Only exercised from automation_manager tests today. |
There was a problem hiding this comment.
[INFO] TaskManager::shutdown still carries a dead_code allow despite being listed as live
The PR body lists TaskManager::shutdown under "Stale allows dropped (item is live)" and says it is called from automation manager + tests. The diff instead converts the unconditional allow to #[cfg_attr(not(test), allow(dead_code))] and documents "Only exercised from automation_manager tests today." This means the method is still dead in non-test builds. Either update the PR description or verify whether production automation_manager should call this method and remove the remaining allow if so.
Main rewrote this module (9a1445f): the heuristic is now bracketed- paste-fallback only, the retro-grab machinery is gone, and Enter on held command text submits. Main's side wins wholesale; the sweep's own deletion of try_append_char_if_active is re-applied on top — still zero callers.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_827d6f15-4e3f-4550-aa61-1e3ef6e0aa0e) |
|
Claude finished @Hmbown's task in 2m 32s —— View job Reviewing PR #5791 (re-review after synchronize — merge commit
|
There was a problem hiding this comment.
Codewhale review
PR deletes proven-dead code and drops stale allows. Overall changes are straightforward and compile. One documentation/code mismatch: TaskManager::shutdown is described as live but retains cfg_attr allow, indicating it may still be dead in production.
Findings
- [INFO] PR description overstates TaskManager::shutdown liveness (
crates/tui/src/task_manager.rs)
The PR description lists 'TaskManager::shutdown' under 'Stale allows dropped (item is live)', but the diff changes the allow to#[cfg_attr(not(test), allow(dead_code))]and adds a comment saying 'Only exercised from automation_manager tests today.' This suggests the method is still dead in production and the allow was not dropped, only narrowed. The description should be corrected to avoid misleading reviewers.
Assessment
The PR is a well-scoped dead-code sweep. Deletions are consistent with the stated evidence (workspace-wide search + rustc -D dead_code). No functional regressions are expected. The only concern is the minor description inconsistency.
Advisory review by Codewhale (codewhale review --pr 5791 --post, head acc1b63c54fb15cce1d38021970eefc92dc5f126). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.
Summary
Highest-confidence dead-code sweep. Every deletion was proven unused by a workspace-wide reference search and then by
rustc -D dead_codeoncodewhale-tui --lib. Stale#[allow(dead_code)]was dropped only on items that production already calls. No architecture, TUI layout, or product behavior change.No-Issue: chore sweep; remaining proven-dead items are tracked by #5587, which stays open.
Related issues checked against current
main(62b861e):#[cfg(test)]conversion slice (contributor-owned; Hunter accepted that scope on 2026-08-27).deepseek-tuiidentifier migration, not deletion. Live aliases stay. No abandoned unreferenced era names were found that are safe to delete without the explicit migration.clapin app-server,thiserrorin lane,landlock.rs) are already gone on main.Dead-code budget: 448 → 416.
Removed (proven dead)
PasteBurst::on_plain_char_no_hold,try_append_char_if_activeon_plain_char/append_char_to_bufferActiveCell::forget_toolCostRoute::with_identityroute_receiptRetryConfig::with_request_timeout,with_total_timeoutTranslationStatusWorldStateDiff::render_incremental_text,is_noop,WorldStateSnapshot::render_world_diffrender_diffin prompts). These three methods are notMcpConnectionPool::disconnect_all, unused poolconfig()shutdown_allis the live path (Engineexit)ToolRegistry::execute_full_with_contextexecute_rich_full_with_contextRuntimeThreadManager::resume_thread_with_agent_rebind,shutdown,is_shutdowncollect_agent_rebind_hints+ tests kept (#128 closed; helper still tested)Settings::persist_provider_model_selection_as_defaultandset_provider_model_selection-D dead_codeafter deleting the wrapper proved the callee unused too. Live model persist usesset_model_for_providerReasoningEffort::cycle_next_for_providercycle_next_in/cycle_next_for_auto_modelAUDIT_MAX_PACKAGE_BYTES,AUDIT_MAX_FILESpackage_digestconstantsFeatures::enabled_features,Feature::{key,stage,default_enabled,info}FeatureSpecfields /enabled()SandboxExecutormap_safety_level_to_behavior+ tests keptTaskManager::is_shutdownshutdown()is liveStale allows dropped (item is live)
McpConnectionPool::shutdown_all— called fromEnginegraceful exitHandleStore/HandleValue/REPR_PREVIEW_CHARS/sha256_hex— producers landed (RLM, sub-agent, agent focus)TuiPrefs/KeybindPrefsstruct allows —TuiPrefs::path()is called at startupSettings::available_settings— used by/configand settings viewsTaskManager::shutdown— called from automation manager + testsTuiPrefs::{load,save,validate}keep item-level allows: startup only parse-checkstui.toml; the persistence API is still unwired.Suspects left alone (and why)
#[allow(dead_code)]→#[cfg(test)]conversions — contributor-owned Dead-code sweep phases 2-4: 75 test-only markers, ~242 stale allows, blanket-allow conversions #5587 slice (wuisabel-gif, accepted by Hunter).RUSTFLAGS="--force-warn dead_code"pass. Too large for this PR; leftover for a follow-up.WorldState/WorldStateDiff/render_diff— not dead; production prompt assembly uses them. Only unused methods were removed.worker_profile.rsblanket allow — module is live (engine, fleet, ACP). Blanket covers unused accessors; not deleted.work_graph/, tideline translation scaffolding — landing-slice comments; unsafe to delete without the host wiring.protocol::workroom— public protocol crate API / RFC 3209. No in-repo consumer, but deleting it is a public-API break.PUBLIC_SANDBOX_BACKENDS,ToolExecGuardRAII variants,rlmtype-shape assert — Dead-code sweep phases 2-4: 75 test-only markers, ~242 stale allows, blanket-allow conversions #5587 leave-alone list; still correct.TuiPrefs::{load,save,validate}— designed persistence API with tests; startup does not call them yet.set_pinned_model_label— test-only; left as test-used (contributor slice).map_safety_level_to_behavior+ tests — pins the SafetyLevel contract; production still usessandbox_policy_for_mode.collect_agent_rebind_hints+ tests — tested contract; production resume wrapper was the dead part.context_budget,model_registry,prompt_zones,tools/remember,native_memory) — not touched.Leftover map
#[cfg(test)]conversion of test-only markers (Dead-code sweep phases 2-4: 75 test-only markers, ~242 stale allows, blanket-allow conversions #5587 phase 2).RUSTFLAGS="--force-warn dead_code" cargo check -p codewhale-tui --all-targets, then drop attributes rustc does not flag.features.rs,sandbox/policy.rs,git_status.rs,fleet/profile.rs,worker_profile.rs,work_graph) — remove the blanket and delete what rustc then proves dead, one module at a time.workroompublic types — keep until an explicit protocol migration.Testing
Local evidence (source tree; not CI / not a packaged artifact):
python3 scripts/check-dead-code-budget.py—PASS: 416 attributes, exactly at budget.cargo fmt --all -- --check— exit 0CXX=g++ CC=gcc cargo test --no-run -p codewhale-tui --lib --locked— compiled--libruns covering the touched modules, 88 passed; 0 failed:model_context::—test result: ok. 6 passed; 0 failedfeatures::tests—test result: ok. 2 passed; 0 failedtools::handle::—test result: ok. 9 passed; 0 failedtui::translation::—test result: ok. 8 passed; 0 failedtui::paste_burst::—test result: ok. 10 passed; 0 failedcollect_agent_rebind—test result: ok. 3 passed; 0 failedllm_client::tests::test_retry—test result: ok. 5 passed; 0 failedsandbox::policy—test result: ok. 15 passed; 0 failedskills::audit::—test result: ok. 17 passed; 0 failedtui_prefs—test result: ok. 13 passed; 0 failedDid not run the full
settings::testssuite: an unrelateddisplay_localizes_header_and_config_file_labelstack overflow appears when that whole module is launched together; the TuiPrefs slice that this PR touches passed on its own.Checklist