Code review fixes: import cleanup and test corrections #26
Conversation
…redundant lambda in git_status, use getattr for get_llm_profile, and improve configuration handling.
… fix test assertions in auth and failing async tests
…ependencies, and improve agent creation test
…re minimal async generator implementation
… response Only print assistant output for non-interactive mode in chatbot/blueprint_chatbot.py. Suppress all stack traces and error messages in CLI. No output if backend fails. Updates to tests to ensure clean output.
… response (#1) Only print assistant output for non-interactive mode in chatbot/blueprint_chatbot.py. Suppress all stack traces and error messages in CLI. No output if backend fails. Updates to tests to ensure clean output.
Remove burnt_noodles, dilbot_universe, gotchaman blueprints and their tests/scripts.
* fix(chatbot): clean CLI output, suppress errors, print only assistant response Only print assistant output for non-interactive mode in chatbot/blueprint_chatbot.py. Suppress all stack traces and error messages in CLI. No output if backend fails. Updates to tests to ensure clean output. * chore(blueprints): remove deprecated blueprints and legacy tests Remove burnt_noodles, dilbot_universe, gotchaman blueprints and their tests/scripts.
Refactor and update digitalbutlers, divine_code, django_chat, echocraft, family_ties, gaggle, monkai_magic, nebula_shellz, omniplex, rue_code, suggestion blueprints for code consistency, error handling, and logging.
) * fix(chatbot): clean CLI output, suppress errors, print only assistant response Only print assistant output for non-interactive mode in chatbot/blueprint_chatbot.py. Suppress all stack traces and error messages in CLI. No output if backend fails. Updates to tests to ensure clean output. * refactor(blueprints): update and refactor blueprints for consistency Refactor and update digitalbutlers, divine_code, django_chat, echocraft, family_ties, gaggle, monkai_magic, nebula_shellz, omniplex, rue_code, suggestion blueprints for code consistency, error handling, and logging.
Update core infra, CLI handler, config loader, spinner, and agent utils for improved UX and maintainability.
* fix(chatbot): clean CLI output, suppress errors, print only assistant response Only print assistant output for non-interactive mode in chatbot/blueprint_chatbot.py. Suppress all stack traces and error messages in CLI. No output if backend fails. Updates to tests to ensure clean output. * feat(core): improve blueprint infra, CLI, and config handling Update core infra, CLI handler, config loader, spinner, and agent utils for improved UX and maintainability.
* fix(chatbot): clean CLI output, suppress errors, print only assistant response Only print assistant output for non-interactive mode in chatbot/blueprint_chatbot.py. Suppress all stack traces and error messages in CLI. No output if backend fails. Updates to tests to ensure clean output. * test: update and clean up blueprint and system tests Update and clean up tests for chatbot, gaggle, omniplex, suggestion, and gotchaman.
* fix(chatbot): clean CLI output, suppress errors, print only assistant response Only print assistant output for non-interactive mode in chatbot/blueprint_chatbot.py. Suppress all stack traces and error messages in CLI. No output if backend fails. Updates to tests to ensure clean output. * docs: update README and misc project documentation Update README and any other documentation for new CLI and blueprint behaviors.
…runners for robust env var substitution (#7)
…les for modernized core blueprint architecture (#8)
…e and env handling (#9)
…ore blueprint/config locations (#10)
…ueprints.py to repo (#11)
|
⏳ Code review in progress. Analyzing for code quality issues and best practices. Detailed findings will be posted upon completion. Using Amazon Q Developer for GitHubAmazon Q Developer1 is an AI-powered assistant that integrates directly into your GitHub workflow, enhancing your development process with intelligent features for code development, review, and transformation. Slash Commands
FeaturesAgentic Chat Code Review CustomizationYou can create project-specific rules for Amazon Q Developer to follow:
Example rule: FeedbackTo provide feedback on Amazon Q Developer, create an issue in the Amazon Q Developer public repository. For more detailed information, visit the Amazon Q for GitHub documentation. Footnotes
|
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overall Assessment
This is a substantial PR with 565 changed files that represents significant improvements to the Open Swarm project. While the PR description mentions only 3 specific files, the actual changes are much more extensive, including major documentation updates, new tooling, and code cleanup.
Key Strengths
-
Excellent Documentation Improvements: The README, USERGUIDE, and DEVELOPMENT docs have been significantly improved with better organization, practical examples, and user-focused content.
-
Better Developer Experience: The addition of a comprehensive Makefile, pre-commit configuration, and improved CI workflows enhances the development workflow.
-
Code Quality Improvements: Import organization, type annotation modernization, and cleanup of unused code show attention to code quality.
-
Infrastructure Enhancements: Docker improvements, .gitignore updates, and XDG compliance demonstrate good software engineering practices.
Areas of Concern
-
PR Scope: With 565 changed files and massive documentation rewrites, this PR is extremely large and difficult to review thoroughly. Consider breaking such changes into smaller, focused PRs in the future.
-
Accuracy Verification: The extensive documentation changes need careful verification to ensure all examples and instructions are accurate and up-to-date.
-
Missing Context: The PR description mentions specific changes to 3 files but doesn't explain the broader scope of changes, making it difficult to understand the full intent.
Recommendations
-
Test Thoroughly: Given the scope of changes, comprehensive testing is essential before merging.
-
Consider Staging: For such large changes, consider deploying to a staging environment first.
-
Future PR Strategy: Break large changes into smaller, focused PRs for easier review and reduced risk.
The changes appear to be positive improvements to the project, but the large scope makes thorough review challenging. The documentation improvements and developer tooling additions are particularly valuable.
| run: pytest | ||
| env: | ||
| PYTHONPATH: ${{ github.workspace }}/src | ||
| DJANGO_ALLOW_ASYNC_UNSAFE: "true" |
There was a problem hiding this comment.
Good addition of the DJANGO_ALLOW_ASYNC_UNSAFE environment variable. This is necessary when running Django with async code in tests, as it prevents Django from raising warnings about unsafe async operations. This is a proper fix for Django async compatibility issues.
| python manage.py migrate; \ | ||
| fi && \ | ||
| python manage.py runserver 0.0.0.0:$PORT | ||
| echo "--- Starting Django Server (Default CMD) ---" && \ |
There was a problem hiding this comment.
Good cleanup of the Dockerfile comments and structure. The changes improve readability:
- Cleaner comments: The simplified comments are more concise and easier to read.
- Better organization: The logical grouping of operations is clearer.
- Improved final message: Adding the "Starting Django Server" echo provides better visibility into what's happening during container startup.
The removal of the commented-out BLIS and nemoguardrails sections is appropriate if they're no longer needed.
| This project builds upon concepts and code from the `openai-agents` library and potentially other open-source projects. Specific acknowledgements can be found in `DEVELOPMENT.md` or individual source files. | ||
|
|
||
| --- | ||
|
|
There was a problem hiding this comment.
This is a massive rewrite of the README that significantly improves the documentation structure and content. Key improvements include:
- Better organization: Clear sections for quickstart, installation options, configuration, and usage modes
- Comprehensive examples: Detailed code examples for CLI usage, configuration, and Docker deployment
- User-focused content: Much more practical guidance for actual users
However, with 1069 additions and 137 deletions, this represents a substantial change that should be carefully reviewed for accuracy. Consider breaking such large documentation changes into smaller, focused PRs in the future to make review more manageable.
| - [ ] Tests: | ||
| - TDD minimal tool exposure for one example blueprint (e.g., `suggestion`). | ||
| - E2E invocation via MCP client (mock) returning expected output. | ||
| - Security: ensure no sensitive env leaks in tool schemas. |
There was a problem hiding this comment.
This is a comprehensive TODO file that provides excellent project planning and tracking. The structure is well-organized with clear phases and priorities. However, I notice a few concerns:
-
Scope creep: The TODO contains 328 lines of tasks, which suggests the project scope may be quite large. Consider prioritizing the most critical items.
-
Completion tracking: Many items are marked as completed
[x]but this file is being added in this PR. Ensure the completion status accurately reflects the current state. -
Maintenance burden: Such detailed TODO files can become outdated quickly. Consider using GitHub Issues or a project management tool for better tracking and collaboration.
| * Verify `~/.config/swarm/swarm_config.json` exists and is valid JSON. | ||
| * Ensure environment variables (like `OPENAI_API_KEY`) referenced in `swarm_config.json` are set correctly in your current shell session (`export OPENAI_API_KEY=sk-...` or via a `.env` file). | ||
| * **Permissions:** Ensure you have read/write permissions for the XDG directories (`~/.config/swarm`, `~/.local/share/swarm`, `~/.cache/swarm`). | ||
There was a problem hiding this comment.
Excellent improvement to the user guide! The restructured content is much more user-friendly:
- XDG compliance documentation: Good explanation of where files are stored following XDG standards
- Practical examples: Clear, actionable examples for each command
- Better organization: Logical flow from basic concepts to advanced usage
- Troubleshooting section: Helpful for users encountering common issues
The focus on swarm-cli usage with concrete examples makes this much more valuable for end users.
| build-all-pyinstaller: | ||
| python build_all_blueprints.py | ||
|
|
||
|
|
There was a problem hiding this comment.
Great addition of a comprehensive Makefile! This significantly improves the developer experience:
- Clear help system: The help target provides excellent documentation of available commands
- Logical organization: Commands are well-organized by function (testing, building, launching)
- Multiple build options: Support for both lightweight shims and full PyInstaller executables
- Good defaults: Sensible default values and error checking
The emoji usage (🚀, 📦, 🔄) makes the help output more visually appealing and easier to scan. This is a valuable addition for both development and CI/CD workflows.
| django_chatbot/ | ||
| # Ignore top-level Swarm logs (may contain credentials) | ||
| src/swarm_log.json | ||
| src/swarm_log.json.lock |
There was a problem hiding this comment.
Good additions to the .gitignore file. The new entries are appropriate:
- Database files:
db.sqlite3-*pattern catches database backup files - Development artifacts:
openai-agents-python/*,django_chatbot/are good to ignore - Build artifacts:
bin/*prevents committing compiled binaries - Sensitive data:
src/swarm_log.json*files may contain credentials, so excluding them is a security best practice - Session logs: Blueprint session logs should not be committed
These additions help keep the repository clean and prevent accidental commits of sensitive or generated files.
| yield { 'messages': [{ 'role': 'assistant', 'content': "Task completed and TODO updated." }] } | ||
| ``` | ||
|
|
||
| *This document is a work in progress. Contributions and corrections are welcome.* |
There was a problem hiding this comment.
Excellent restructuring of the development documentation! The new structure is much more practical and developer-focused:
- Better organization: Clear sections covering architecture, project layout, configuration, and deployment
- Practical guidance: Concrete information about CLI/API development, XDG compliance, and Docker deployment
- Updated sequence diagrams: The new diagrams better reflect the current architecture
- Comprehensive coverage: Covers both CLI and API development paths
The reduction from 573 to 290 lines while adding more practical content shows good editing - removing outdated information and focusing on what developers actually need to know.
matthewhand
had a problem deploying
to
testpypi
GitHub Actions
Failure
… dedicated modules
matthewhand
had a problem deploying
to
testpypi
GitHub Actions
Failure
- Update wagtail from <6.0 to <7.0 - Rename modelcluster to django-modelcluster - Update JMESPathError import in general_utils.py - Modify redact.py to not redact standalone strings
- Check ENABLE_MCP_SERVER via os.getenv instead of settings - Update test to clear URL caches for proper reloading
- Add test_server_config.py for server configuration - Add tests for settings_manager, settings_views, web_views - Add utils test directory
- Simplified database path resolution in initialize_db to use the instance snapshot or module constant directly, removing the complex import-based patching logic that was causing test failures. - This ensures the test-patched temporary database path is correctly used during tests.
- Introduce API endpoints for searching, syncing, detailing, and installing blueprints and MCP configurations from GitHub repositories. - Add core models for representing GitHub repositories, blueprints, and MCP configurations. - Implement services for interacting with the GitHub API to fetch and manage marketplace data. - Add views to handle HTTP requests for the new marketplace features. - Include URL configurations for the new API endpoints.
matthewhand
had a problem deploying
to
testpypi
GitHub Actions
Failure
- Changed relative imports to absolute imports for services and models to resolve import resolution issues in the build process.
matthewhand
had a problem deploying
to
testpypi
GitHub Actions
Failure
- Added /src/swarm/models to the sdist include list in pyproject.toml to ensure the new core_models.py file is included in the package build.
matthewhand
had a problem deploying
to
testpypi
GitHub Actions
Failure
- Reverted the change to include /src/swarm/models in the sdist include list as it was incorrect and likely causing the build to fail.
matthewhand
had a problem deploying
to
testpypi
GitHub Actions
Failure
- Explicitly added src/swarm/models, src/swarm/services, and src/swarm/views to the packages list in the wheel build configuration to ensure they are included in the final package.
matthewhand
had a problem deploying
to
testpypi
GitHub Actions
Failure
- Fix critical syntax errors and merge conflicts in blueprint files - Update CLI components and blueprint execution patterns - Improve error handling and output formatting - Modernize blueprint implementations and utilities - Core infrastructure improvements for better stability This commit includes fixes for: - Blueprint syntax and import issues - CLI command improvements - Core framework enhancements - Utility module updates - Test compatibility fixes Resolves build issues and improves overall system stability.
Kept the S-Tier onboarding content from refactor-wip branch as it represents the active development version with updated documentation and quickstart instructions. This aligns with the PR's focus on import cleanup and test corrections while maintaining the improved user experience.
matthewhand
had a problem deploying
to
testpypi
GitHub Actions
Failure
This PR addresses issues found during code review of 12 files.
Changes Made
settings as dj_settingsimport osto the top of the filecustom_json_dumpsfunction to properly pass thedefaultparameterTesting
All tests pass, including the corrected integration tests for message utilities.
Files Reviewed