refactor: deduplicate tool code and remove dead stubs - #98
Conversation
…tools Delete 4 never-registered stub tools (marketplace, restaurant, ecommerce, taskrabbit) removing ~625 lines of dead code. Add require_str/require_param helpers to tool.rs and refactor ~30 call sites across 10 tool files from 4-6 line inline extractions to single-line calls. Consolidate worker HTTP client with get_json/post_json helpers, reducing boilerplate in 4 methods. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary of ChangesHello @ilblackdragon, 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 significantly improves code quality and maintainability by eliminating unused tool stubs and introducing standardized helper functions for common operations. The changes streamline parameter handling in tools and centralize HTTP client logic, making the codebase cleaner, more robust, and easier to extend. The overall impact is a substantial reduction in code lines and improved consistency across the project. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly improves the codebase through excellent refactoring, including the removal of over 600 lines of dead code from unused tool stubs and the introduction of require_str/require_param helpers for simplified parameter handling. It also consolidates HTTP logic in the worker API with get_json/post_json helpers. However, a critical security issue was identified in the ShellTool's safety mechanisms: its blocklist and auto-approval prevention logic rely on easily bypassable string containment checks, which could lead to unintended execution of destructive commands. Implementing more robust command validation or whitespace normalization is recommended. Furthermore, a potential runtime failure may have been introduced by the refactoring of report_complete.
| let _: serde_json::Value = self | ||
| .post_json("complete", report, "report complete") | ||
| .await?; |
There was a problem hiding this comment.
The refactoring to use post_json here introduces a requirement that the /complete endpoint returns a valid JSON body. The previous implementation did not have this requirement and would have succeeded with an empty response body (e.g., from a 204 No Content or 200 OK with no body).
If the orchestrator's /complete endpoint can return a successful status with an empty body, this change will cause the worker to fail when reporting completion, as resp.json() will fail to parse an empty body. This could be a critical issue in the worker's lifecycle.
To address this, you could create a separate helper for POST requests that don't expect a JSON response body, or adjust this function to handle non-JSON success responses.
There was a problem hiding this comment.
Good catch. The orchestrator's report_complete handler was returning bare StatusCode::OK with no body, which would cause post_json's .json() call to fail. Fixed in 68faf85 by changing the handler to return Json({"status": "ok"}), consistent with the other worker endpoints.
There was a problem hiding this comment.
Pull request overview
This PR refactors the codebase to reduce duplication and remove dead code. It introduces helper functions for common parameter extraction patterns, consolidates HTTP client logic with reusable methods, and deletes 4 unregistered tool stubs that were never integrated into the system.
Changes:
- Added
require_strandrequire_paramhelper functions to eliminate repetitive parameter extraction boilerplate across 10 tool files - Introduced
get_jsonandpost_jsonprivate helpers inWorkerHttpClientto deduplicate bearer auth, status checking, and JSON parsing logic - Removed 4 dead tool stubs (marketplace, restaurant, ecommerce, taskrabbit) that were exported but never registered in
ToolRegistry
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/worker/api.rs | Refactored HTTP client with get_json and post_json helpers to consolidate bearer auth, status checks, and JSON parsing across 4 methods |
| src/tools/tool.rs | Added require_str and require_param helper functions with comprehensive tests for extracting required parameters from JSON objects |
| src/tools/builtin/time.rs | Replaced 3 inline parameter extractions with require_str calls |
| src/tools/builtin/taskrabbit.rs | Deleted 157-line dead stub tool that was never registered |
| src/tools/builtin/shell.rs | Replaced inline parameter extraction with require_str call |
| src/tools/builtin/routine.rs | Replaced 6 inline parameter extractions with require_str calls across 4 tool implementations |
| src/tools/builtin/restaurant.rs | Deleted 172-line dead stub tool that was never registered |
| src/tools/builtin/mod.rs | Removed module declarations and exports for 4 deleted tools |
| src/tools/builtin/memory.rs | Replaced 4 inline parameter extractions with require_str calls |
| src/tools/builtin/marketplace.rs | Deleted 160-line dead stub tool that was never registered |
| src/tools/builtin/json.rs | Replaced 2 inline parameter extractions with require_str and require_param calls |
| src/tools/builtin/job.rs | Replaced 4 inline parameter extractions with require_str calls |
| src/tools/builtin/http.rs | Replaced 2 inline parameter extractions with require_str calls |
| src/tools/builtin/file.rs | Replaced 5 inline parameter extractions with require_str calls |
| src/tools/builtin/extension_tools.rs | Replaced 4 inline parameter extractions with require_str calls |
| src/tools/builtin/ecommerce.rs | Deleted 136-line dead stub tool that was never registered |
| src/tools/builtin/echo.rs | Replaced inline parameter extraction with require_str call |
| src/estimation/time.rs | Removed duration estimates for 4 deleted tools |
| src/estimation/cost.rs | Removed cost estimates for 4 deleted tools and updated tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let _: serde_json::Value = self | ||
| .post_json("complete", report, "report complete") | ||
| .await?; |
There was a problem hiding this comment.
The report_complete method now uses post_json, which returns WorkerError::LlmProxyFailed for connection and status errors. However, the original implementation used WorkerError::ConnectionFailed for connection errors and WorkerError::OrchestratorRejected for non-success status codes. This is a semantic error because report_complete is not an LLM proxy operation - it's a completion report to the orchestrator.
Consider creating a separate helper method for non-LLM POST requests that uses appropriate error types, or pass an error type parameter to post_json to allow customization.
| let _: serde_json::Value = self | |
| .post_json("complete", report, "report complete") | |
| .await?; | |
| let resp = self | |
| .client | |
| .post(self.url("complete")) | |
| .bearer_auth(&self.token) | |
| .json(report) | |
| .send() | |
| .await | |
| .map_err(|e| WorkerError::ConnectionFailed { | |
| url: self.orchestrator_url.clone(), | |
| reason: e.to_string(), | |
| })?; | |
| if !resp.status().is_success() { | |
| return Err(WorkerError::OrchestratorRejected { | |
| job_id: self.job_id, | |
| reason: format!("complete endpoint returned {}", resp.status()), | |
| }); | |
| } | |
| // Consume and validate the JSON response from the orchestrator, if any. | |
| let _: serde_json::Value = | |
| resp.json().await.map_err(|e| WorkerError::OrchestratorRejected { | |
| job_id: self.job_id, | |
| reason: format!("failed to parse complete response: {}", e), | |
| })?; |
There was a problem hiding this comment.
Valid point about the empty-body issue and the error type mismatch. Fixed in 68faf85 by changing the orchestrator's report_complete handler to return Json({"status": "ok"}) instead of bare StatusCode::OK. This makes all worker POST endpoints consistently return JSON, so post_json works uniformly. The error variant difference (LlmProxyFailed vs OrchestratorRejected) is cosmetic in practice since the context string already identifies the operation.
The report_complete handler returned bare StatusCode::OK (no body),
which broke the post_json helper that expects a JSON response.
Return {"status": "ok"} for consistency with other worker endpoints.
Addresses review feedback on PR #98.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* refactor: deduplicate tool parameter extraction and remove dead stub tools
Delete 4 never-registered stub tools (marketplace, restaurant, ecommerce,
taskrabbit) removing ~625 lines of dead code. Add require_str/require_param
helpers to tool.rs and refactor ~30 call sites across 10 tool files from
4-6 line inline extractions to single-line calls. Consolidate worker HTTP
client with get_json/post_json helpers, reducing boilerplate in 4 methods.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: return JSON from orchestrator /complete endpoint
The report_complete handler returned bare StatusCode::OK (no body),
which broke the post_json helper that expects a JSON response.
Return {"status": "ok"} for consistency with other worker endpoints.
Addresses review feedback on PR nearai#98.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
* refactor: deduplicate tool parameter extraction and remove dead stub tools
Delete 4 never-registered stub tools (marketplace, restaurant, ecommerce,
taskrabbit) removing ~625 lines of dead code. Add require_str/require_param
helpers to tool.rs and refactor ~30 call sites across 10 tool files from
4-6 line inline extractions to single-line calls. Consolidate worker HTTP
client with get_json/post_json helpers, reducing boilerplate in 4 methods.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: return JSON from orchestrator /complete endpoint
The report_complete handler returned bare StatusCode::OK (no body),
which broke the post_json helper that expects a JSON response.
Return {"status": "ok"} for consistency with other worker endpoints.
Addresses review feedback on PR nearai#98.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Summary
marketplace,restaurant,ecommerce,taskrabbit) that were exported but never registered inToolRegistry, removing ~625 lines of dead coderequire_str/require_paramhelpers tosrc/tools/tool.rsand refactor ~30 call sites across 10 tool files from 4-6 line inline parameter extractions to single-line callsget_json/post_jsonprivate helpers insrc/worker/api.rs, deduplicating bearer-auth + status-check + JSON-parse boilerplate across 4 methodsNet: -720 lines (157 added, 877 removed) across 19 files.
Test plan
cargo fmtcleancargo clippy --all --benches --tests --examples --all-featureszero warningscargo testall 836 tests passcargo check --no-default-features --features libsqlcompiles clean.unwrap()/.expect()in production codesuper::imports in changed files🤖 Generated with Claude Code