Conversation
There was a problem hiding this comment.
Sorry @LIghtJUNction, your pull request is larger than the review limit of 150000 diff characters
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 significantly refactors AstrBot's core architecture by modularizing tool management and improving the robustness of tool execution. It streamlines the integration of various tools, enhances the dashboard's configurability, and ensures more predictable behavior for LLM interactions. These changes contribute to a more maintainable, extensible, and user-friendly system. Highlights
Changelog
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 pull request introduces a significant and well-executed refactoring of the tool management system, centered around a new ToolProvider protocol. This change decouples tool registration from the main agent logic, greatly enhancing modularity and making the system more extensible. Key improvements include the introduction of ComputerToolProvider and CronToolProvider, a new sandbox capability check to prevent the use of browser tools in unsupported environments, and deterministic tool serialization to improve caching. The command-line interface and dashboard have also been substantially improved, offering better configuration options like a backend-only mode and flexible API URL settings. Overall, these changes represent a major architectural improvement, increasing the robustness, safety, and maintainability of the codebase. The implementation is solid, and I have no specific issues to raise.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Resolved merge conflicts in: - func_tool_manager.py: kept origin version (full implementation) - astr_main_agent*.py: kept HEAD version (complete refactor) - cron_tools.py: kept HEAD version (with get_all_tools) - config/default.py: kept HEAD version (DEFAULT_WEB_SEARCH_PROVIDER) - dashboard/*.vue: kept HEAD version - web_searcher engines: kept HEAD version - Added new tools: knowledge_base_tools, message_tools, registry, web_search_tools
…nds() collect_commands() was defined as a sync def but erroneously contained SDK bridge command registration logic with await sync_commands() inside. Extract SDK bridge registration into async _register_sdk_commands() and keep collect_commands() as pure sync (returns command list only). Also adds missing cast import.
… configured Before login attempts, check if apiBaseUrl is configured. If empty, emit openServerConfig event so LoginPage opens the server config dialog instead of silently sending requests to the frontend origin (causing 405).
Previously, a URL like api.lightjunction.online:3000 (without https://) was stored as-is and treated as a relative path, causing requests to hit the frontend origin instead of the configured backend. URLs with a single slash like https:/api... were also incorrectly normalized. Now normalizeConfiguredApiBaseUrl always prepends https:// if the input doesn't start with http:// or https://, and apiStore.setApiBaseUrl also normalizes before storing to keep state and localStorage consistent.
AstrBot uses a lock file (astrbot.lock) to prevent concurrent instances. Before allowing a password change via `astrbot conf admin`, the CLI now attempts to acquire the lock with a 1-second timeout. If acquisition fails (another process holds it), the command is rejected with a clear error message instructing the user to stop astrbot first.
…able When user installs a dev/beta/alpha/rc version (e.g. 4.25.0-dev), check_update would skip prerelease releases and compare against the latest stable (e.g. 4.24.0). Since 4.25.0-dev > 4.24.0 in semver comparison (numeric part takes precedence), it incorrectly prompted to update. Now checks if current version is prerelease and already newer than the latest stable - in that case, no update prompt is shown. Fixes: update button shown to users running dev/unstable versions
Resolved all conflicts using dev (HEAD) version: - func_tool_manager.py: migrated old implementation into new architecture - tool_loop_agent_runner.py: preserve lazy_load mode, remove step limit - Platform adapters, dashboard, core modules: use HEAD version
| logger.debug(f"请求 URL: {self.api_base}") | ||
| logger.debug(f"请求体: {json.dumps(payload, ensure_ascii=False)[:100]}...") | ||
| logger.debug( | ||
| f"请求体: {json.dumps(loggable_payload, ensure_ascii=False)[:100]}...", |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 18 hours ago
General fix: avoid logging request bodies (or any user/config-derived sensitive fields) in clear text. Log only non-sensitive metadata (e.g., URL, timeout, cluster/voice type if needed), and remove payload serialization from logs.
Best minimal fix in this file: in get_audio (around lines 125–127), replace the debug log that prints json.dumps(loggable_payload...) with a constant/metadata-only message that does not include payload values. This preserves debug usefulness without exposing secrets or user text and avoids CodeQL taint-to-log sink.
No new imports or dependencies are needed. We only edit astrbot/core/provider/sources/volcengine_tts.py in the shown snippet.
| @@ -118,13 +118,10 @@ | ||
| } | ||
|
|
||
| payload = self._build_request_payload(text) | ||
| loggable_payload = self._build_loggable_payload(payload) | ||
|
|
||
| # Keep the request metadata useful for debugging without exposing secrets. | ||
| # Keep logs useful for debugging without exposing request content/secrets. | ||
| logger.debug(f"请求 URL: {self.api_base}") | ||
| logger.debug( | ||
| f"请求体: {json.dumps(loggable_payload, ensure_ascii=False)[:100]}...", | ||
| ) | ||
| logger.debug("请求体: [REDACTED]") | ||
|
|
||
| try: | ||
| async with ( |
| c in "0123456789abcdefABCDEF" for c in stored_hash | ||
| ): | ||
| candidate_sha256 = hashlib.sha256( | ||
| candidate_password.encode("utf-8"), |
Check failure
Code scanning / CodeQL
Use of a broken or weak cryptographic hashing algorithm on sensitive data High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 18 hours ago
To fix this without changing intended functionality, remove the runtime SHA-256 hashing fallback path from password verification and treat unknown/legacy raw SHA-256 formats as unsupported (or only accept exact pre-hashed client input if still required). The safest minimal fix is to delete the branch that computes candidate_sha256 = hashlib.sha256(candidate_password.encode("utf-8")).hexdigest() and return False for that format. This keeps Argon2/PBKDF2/legacy-MD5 behavior intact while eliminating insecure password hashing on user input.
Edit only astrbot/core/utils/auth_password.py in verify_dashboard_password around lines 191–201 (the “Legacy SHA-256 fallback compatibility” block). No new methods/imports/dependencies are needed.
| @@ -188,18 +188,6 @@ | ||
| ) | ||
| return hmac.compare_digest(stored_key, candidate_key) | ||
|
|
||
| # Legacy SHA-256 fallback compatibility | ||
| if len(stored_hash) == 64 and all( | ||
| c in "0123456789abcdefABCDEF" for c in stored_hash | ||
| ): | ||
| candidate_sha256 = hashlib.sha256( | ||
| candidate_password.encode("utf-8"), | ||
| ).hexdigest() | ||
| return hmac.compare_digest( | ||
| stored_hash.lower(), | ||
| candidate_sha256.lower(), | ||
| ) or hmac.compare_digest(stored_hash.lower(), candidate_password.lower()) | ||
|
|
||
| return False | ||
|
|
||
|
|
- cmd_conf: show password rules before setting, display length after input - cmd_init/cmd_run: add Chinese prompts and HTTPS security warnings - openai_source/gemini_source: add missing provider adapter import - ConsoleDisplayer: add missing resolveApiUrl import for SSE - WelcomePage: show user-friendly message for 401 unauthorized errors
- Add `astrbot version` subcommand showing: - AstrBot version - Python version - System/machine info - Git branch and commit - AstrBot root path - Platform details - Also works with `astrbot --version` flag
Logo is now only shown in commands that have their own startup output (run, init), keeping other commands clean.
Runtime bootstrap (SSL context setup) is now only initialized in commands that actually need it (run), not on every CLI invocation.
- Move logo and print_logo to new astrbot/cli/banner.py module to avoid circular imports - Add is_interactive() helper to detect TTY - Show ASCII logo in run/init commands only in interactive mode - Show WeChat QR code ASCII art only in interactive mode; non-interactive mode shows just the QR link
- Add more SSL error patterns to _SSLDebugFilter: SSL: UNEXPECTED_RECORD, SSLWantRead, SSLWantWrite, SSL_ERROR_CODE, sslPP: error - Check exc_info for SSL error messages in filter - Add httpcore and httpx to _NOISY_LOGGER_LEVELS - Apply _SSLDebugFilter to LogQueueHandler
* feat(provider): add lm_studio timeout in advanced config * refactor(provider): strengthen template typing and ambiguity diagnostics * fix(provider): tighten template matching and clone defaults * remove
This pull request introduces a new workflow for deploying the dashboard to GitHub Pages and makes significant improvements to the
README.mdfor clarity, completeness, and consistency. It also includes minor formatting updates to the smoke test workflow and adds some convenience commands to.envrc.Summary of changes:
README.mdwith clearer descriptions, updated instructions, improved platform/model tables, and better contribution guidelines.Dashboard Deployment Automation
.github/workflows/deploy-dashboard.ymlto automate daily and manual dashboard builds and deployments to GitHub Pages, including build, artifact upload, and deployment steps.Documentation Improvements
README.md: clearer project description, improved feature list, updated deployment instructions, revised supported platforms/models tables, and enhanced contribution guidelines. [1] [2] [3] [4] [5]Workflow Consistency
.github/workflows/smoke_test.ymlto use consistent YAML quoting, improved comments, and clarified Python version formatting. [1] [2] [3]Developer Convenience
git pullandgit statuscommands to.envrcfor easier environment setup and status checking.