chore: remove dead code — evaluation APIs, chunk_by_paragraphs, bundled installer, Reasoning.safety (#648)#675
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on significant code cleanup by removing several unused or dead code components across the codebase. The changes streamline the project by eliminating unnecessary evaluation APIs, an unused text chunking strategy, an unutilized extension installation mechanism, and a redundant safety field within the reasoning engine. This refactoring improves maintainability, reduces the project's footprint, and ensures that the codebase only contains actively used and relevant components. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This is an excellent pull request that significantly improves the codebase by removing dead code and simplifying several components. The removal of the unused safety field from Reasoning and ContextCompactor is a major cleanup, and the cascading changes have been applied consistently across all affected files. Moving the RuleBasedEvaluator to be test-only and deleting the unused LlmEvaluator and chunk_by_paragraphs function are great improvements to code organization and maintainability. The changes are well-documented and the tests have been updated appropriately. Overall, this is a high-quality contribution.
Note: Security Review did not run due to the size of the PR.
There was a problem hiding this comment.
Pull request overview
Removes multiple unused APIs and dead code paths to reduce maintenance overhead and eliminate #[allow(dead_code)] annotations, including simplifying Reasoning construction now that tool-output sanitization is handled outside Reasoning.
Changes:
- Deleted unused evaluation APIs and moved the rule-based evaluator to test-only code with a
with_config()helper. - Removed unused
chunk_by_paragraphs()implementation and its tests. - Deleted an unused bundled channel installer helper and removed the unused
SafetyLayerfield/parameter fromReasoning, cascading signature updates through callers and tests.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/heartbeat_integration.rs | Updates heartbeat test to use new HeartbeatRunner::new signature (no SafetyLayer). |
| tests/e2e_routine_heartbeat.rs | Removes safety setup from routine heartbeat E2E tests and updates runner construction. |
| src/workspace/chunker.rs | Deletes unused chunk_by_paragraphs() and associated tests. |
| src/worker/runtime.rs | Updates Reasoning::new call site to new signature. |
| src/tools/registry.rs | Removes SafetyLayer from builder-tool registration API and updates builder construction. |
| src/tools/builder/core.rs | Removes unused SafetyLayer threading and updates Reasoning::new call sites. |
| src/llm/reasoning.rs | Removes SafetyLayer field/constructor parameter from Reasoning and updates internal tests. |
| src/extensions/manager.rs | Deletes unused bundled channel installer helper. |
| src/evaluation/success.rs | Removes unused evaluator APIs; makes rule-based evaluator test-only and adjusts tests. |
| src/app.rs | Updates builder-tool registration call site for new signature. |
| src/agent/worker.rs | Updates Reasoning::new call site for new signature. |
| src/agent/thread_ops.rs | Updates ContextCompactor::new call sites for new signature. |
| src/agent/heartbeat.rs | Removes SafetyLayer from HeartbeatRunner and updates reasoning construction. |
| src/agent/dispatcher.rs | Updates Reasoning::new call sites in dispatcher and tests. |
| src/agent/compaction.rs | Removes SafetyLayer from ContextCompactor and updates reasoning construction + tests. |
| src/agent/commands.rs | Updates heartbeat runner and Reasoning::new call sites for new signatures. |
| src/agent/agent_loop.rs | Updates heartbeat spawn call to match new spawn_heartbeat signature. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub fn new(llm: Arc<dyn LlmProvider>) -> Self { | ||
| Self { llm } |
There was a problem hiding this comment.
ContextCompactor::new is public, and removing the SafetyLayer parameter is a breaking API change for any external callers. Consider leaving a deprecated constructor overload/shim that accepts the old parameter list (and ignores it), or ensure the change is called out in the changelog/versioning if this is intended to be breaking.
| pub fn new( | ||
| config: HeartbeatConfig, | ||
| hygiene_config: HygieneConfig, | ||
| workspace: Arc<Workspace>, | ||
| llm: Arc<dyn LlmProvider>, | ||
| safety: Arc<SafetyLayer>, | ||
| ) -> Self { |
There was a problem hiding this comment.
HeartbeatRunner::new is public, and dropping the SafetyLayer argument is a breaking API change for downstream callers (including any external integrations). If maintaining API stability matters, consider adding a deprecated new(..., _safety) shim or documenting the breaking change + version bump.
| pub async fn register_builder_tool( | ||
| self: &Arc<Self>, | ||
| llm: Arc<dyn LlmProvider>, | ||
| safety: Arc<SafetyLayer>, | ||
| config: Option<BuilderConfig>, | ||
| ) { |
There was a problem hiding this comment.
ToolRegistry::register_builder_tool is part of the public API (re-exported via the prelude) and its signature change (removing SafetyLayer) is breaking for downstream crates. Consider adding a deprecated wrapper with the old signature (ignoring the safety argument) to preserve compatibility, or ensure the change is reflected in changelog/versioning.
| pub fn new( | ||
| config: BuilderConfig, | ||
| llm: Arc<dyn LlmProvider>, | ||
| safety: Arc<SafetyLayer>, | ||
| tools: Arc<ToolRegistry>, | ||
| ) -> Self { |
There was a problem hiding this comment.
LlmSoftwareBuilder::new is a public constructor; removing the SafetyLayer parameter is a breaking change for any external callers constructing builders directly. If API stability is desired, consider a deprecated shim that accepts the old parameter list (and ignores it) or documenting the breaking change in changelog/versioning.
| impl Reasoning { | ||
| /// Create a new reasoning engine. | ||
| pub fn new(llm: Arc<dyn LlmProvider>, safety: Arc<SafetyLayer>) -> Self { | ||
| pub fn new(llm: Arc<dyn LlmProvider>) -> Self { |
There was a problem hiding this comment.
Reasoning::new is a public constructor (re-exported from crate::llm) and its signature change is a breaking API change for downstream users. If this crate is consumed as a library, consider keeping a backwards-compatible/deprecated constructor (e.g., new_with_safety(llm, safety) or new(llm) plus a deprecated new(llm, _safety) shim) and/or documenting the breaking change in the changelog with an appropriate version bump.
…ed installer, Reasoning.safety (nearai#648) Remove four pieces of dead code identified in nearai#648: 1. evaluation/success.rs: Remove unused RuleBasedEvaluator methods (with_min_success_rate, with_max_failures) and LlmEvaluator::new(). Replace RuleBasedEvaluator::new() with a default_evaluator() test helper since the constructor was only used in tests. 2. workspace/chunker.rs: Remove unused chunk_by_paragraphs() function and its tests. Only chunk_document() is used in production. 3. extensions/manager.rs: Remove install_bundled_channel_from_artifacts() which was marked for "upcoming hot-activation flow" but never called. 4. llm/reasoning.rs: Remove unused safety field from Reasoning struct. This cascades through all callers (dispatcher, compaction, heartbeat, commands, worker, thread_ops, tool_builder) removing the now-unnecessary Arc<SafetyLayer> parameter from Reasoning::new(). All #[allow(dead_code)] annotations on removed items are also removed. cargo clippy --all-targets produces zero warnings. cargo test passes all tests.
9b82a0a to
4b9ec1b
Compare
Summary
Closes #648.
Remove four pieces of dead code:
1. Evaluation module unused APIs (
src/evaluation/success.rs)RuleBasedEvaluator::with_min_success_rate()andwith_max_failures()— never calledLlmEvaluator::new()— never calledRuleBasedEvaluator::new()with adefault_evaluator()test helper (constructor was only used in tests)#[allow(dead_code)]annotations2.
chunk_by_paragraphs()(src/workspace/chunker.rs)chunk_document()is used in production3.
install_bundled_channel_from_artifacts()(src/extensions/manager.rs)4.
Reasoning::safetyfield (src/llm/reasoning.rs)safety: Arc<SafetyLayer>field fromReasoningstructReasoning::new()no longer takes asafetyparameterVerification
cargo clippy --all-targets— zero warningscargo test— all tests passStats
17 files changed, 69 insertions(+), 287 deletions(-)