-
Notifications
You must be signed in to change notification settings - Fork 0
Refactor codebase: Consolidate shell scripts, fix type safety, standardize naming, complete modularization, update dependencies, modernize tests, and add unified configuration (Phases 0-4) #138
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…red shell library Co-authored-by: hessius <1499030+hessius@users.noreply.github.com>
- Replace all echo -e "${GREEN}✓ ..." with log_success "..."
- Replace all echo -e "${BLUE}..." with log_info "..."
- Replace all echo -e "${YELLOW}..." with log_warning "..."
- Replace all echo -e "${RED}✗ ..." with log_error "..."
- Remove local color variable definitions (now in common.sh)
- Keep non-colored echo statements unchanged
- Maintain all existing logic and functionality
Co-authored-by: hessius <1499030+hessius@users.noreply.github.com>
- Added sourcing of scripts/lib/common.sh at the top
- Removed duplicate color definitions (GREEN, BLUE, YELLOW, RED, NC)
- Removed duplicate checkout_latest_release() function (now in common.sh)
- Removed duplicate check_docker_compose() function (now in common.sh)
- Replaced simple logging calls throughout:
- echo -e "${GREEN}✓ ..." → log_success "..."
- echo -e "${BLUE}..." → log_info "..."
- echo -e "${YELLOW}..." → log_warning "..."
- echo -e "${RED}✗ ..." → log_error "..."
- Kept show_progress() function as-is (more complex than common.sh)
- Kept run_privileged() function as-is (needed before common.sh)
- Net reduction: 49 lines, improved maintainability
Co-authored-by: hessius <1499030+hessius@users.noreply.github.com>
…debase Co-authored-by: hessius <1499030+hessius@users.noreply.github.com>
Co-authored-by: hessius <1499030+hessius@users.noreply.github.com>
|
@copilot commence work on phase 2 |
Starting Phase 2: Modularization of main.py (7,230 lines, 110 functions, 46 endpoints). I'll extract the monolithic main.py into focused modules following the architecture plan. This will make the codebase easier to navigate, test, and maintain. |
Co-authored-by: hessius <1499030+hessius@users.noreply.github.com>
…842 lines modularized) Co-authored-by: hessius <1499030+hessius@users.noreply.github.com>
Co-authored-by: hessius <1499030+hessius@users.noreply.github.com>
|
@copilot finish phase 2 |
Completing Phase 2: Will now integrate the extracted services into main.py, remove duplicate functions, extract API routes, and run tests to verify everything works. |
Co-authored-by: hessius <1499030+hessius@users.noreply.github.com>
Co-authored-by: hessius <1499030+hessius@users.noreply.github.com>
Co-authored-by: hessius <1499030+hessius@users.noreply.github.com>
Co-authored-by: hessius <1499030+hessius@users.noreply.github.com>
Co-authored-by: hessius <1499030+hessius@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@copilot apply changes based on the comments in this thread |
… fix function shadowing, fix duplicate logic, fix datetime parsing, add exception comments, fix log output Co-authored-by: hessius <1499030+hessius@users.noreply.github.com>
Co-authored-by: hessius <1499030+hessius@users.noreply.github.com>
Co-authored-by: hessius <1499030+hessius@users.noreply.github.com>
- Remove duplicate DATA_DIR/TEST_MODE logic from cache_service.py, history_service.py, settings_service.py, scheduling_state.py - Import DATA_DIR, LLM_CACHE_TTL_SECONDS, SHOT_CACHE_STALE_SECONDS from central config.py - Export missing cache constants from config.py
- Add asyncio, datetime, Any imports to profiles.py - Add Any import to main.py - Update ScheduledShotsPersistence to use DATA_DIR from config - Update RecurringSchedulesPersistence to use DATA_DIR from config - Fix config.py line concatenation issue - Fix test import for process_image_for_profile - Fix pytest-asyncio version to 0.23.8 (1.3.0 doesn't exist) - Fix pytest-cov version to 4.1.0
Add re-exports for functions that have been moved to service modules: - DATA_DIR, MAX_UPLOAD_SIZE from config - atomic_write_json, deep_convert_to_dict from utils.file_utils - parse_gemini_error, get_vision_model from services.gemini_service - get_meticulous_api, decompress_shot_data from services.meticulous_service - SETTINGS_FILE, _load_settings, _ensure_settings_file from settings_service - _scheduled_shots, _scheduled_tasks, _recurring_schedules from scheduling_state - ScheduledShotsPersistence, RecurringSchedulesPersistence from api.routes.profiles - genai module for tests This allows existing tests to continue importing from 'main' module while the refactoring is completed.
- Dockerfile: Use Python 3.12 (3.14 incompatible with Pillow 10.3.0) - Dockerfile: Add COPY statements for new modular structure (api/, services/, utils/, models/, config.py) - docker-compose.yml: Remove conflicting read-only volume mount for /app - main.py: Add imports for lifespan functions from api.routes.profiles
- Updated pyMeticulous to 0.3.1 (adds ActionType.PREHEAT support) - Added TEST_MODE export to config.py - Fixed missing imports in profiles.py (tempfile, uuid, HISTORY_FILE, deep_convert_to_dict) - Updated test mock paths to target correct modules after refactor - Fixed _prepare_shot_summary_for_llm test to match new signature - Added service module imports for tests accessing module-level variables - 330 tests passing (113 -> 31 remaining failures mostly test isolation issues)
- Added MAX_UPLOAD_SIZE import to profiles.py - Fixed sanitize_profile_name_for_filename references in tests - 330 tests passing, 29 remaining failures (mostly environment/path issues)
- Fix HISTORY_FILE mock paths to target api.routes.profiles - Fix Path mock paths to target api.routes.system - Fix get_vision_model mock to target api.routes.coffee - Fix get_meticulous_api mock paths for error handling test - Fix load_settings mock for settings error test - Update TestDataDirectoryConfiguration imports to use correct modules - Add base64 import to profiles.py for apply-image endpoint - Fix recurring schedules fixture to clear api.routes.scheduling module - Fix test_logging.py Path mock to target api.routes.system All 359 tests now pass.
- meticai-server/requirements-test.txt: Use minimum versions (>=) instead of exact pins that were outdated (pytest 7.4.4 -> >=8.0.0, pytest-cov 4.1.0 -> >=5.0.0) - coffee-relay/requirements.txt: Update pyMeticulous >=0.1.0 -> >=0.3.1 for ActionType.PREHEAT support, consistent with meticai-server The test dependencies were pinned to old versions but pip was installing newer versions anyway. Using minimum version constraints is more appropriate here.
- test_local_install.bats: Update test to check for 'meticai-server' instead of old name 'coffee-relay' - test_uninstall.bats: Update image filter test to match actual script content (meticai-server instead of coffee-relay) The coffee-relay name was the old name before the rename to meticai-server.
- Update pytest coverage to include all new modules after refactoring: main, logging_config, config, prompt_builder, api, services, utils, models - Lower coverage threshold from 68% to 55% temporarily (actual coverage is 59% with all modules included) - Remove deleted coffee-relay/requirements.txt The refactoring moved code from main.py into modular services and routes, which wasn't being measured by the old coverage config that only checked main and logging_config.
- Removed ~1060 lines of legacy duplicate code from main.py (now 246 lines) - Added 30 new tests for scheduling_state and meticulous_service modules - Restored coverage threshold to 68% (was temporarily lowered to 55%) - Fixed BATS tests: color code check, meticai-server container name Coverage improvements: - services/scheduling_state.py: 28% -> 100% - services/meticulous_service.py: 34% -> 88% - main.py: 21% -> 83% - Total: 59% -> 68% Legacy code removed: - Duplicate BARISTA_PERSONA, SAFETY_RULES constants (exist in coffee.py) - Duplicate route handlers (exist in profiles.py, scheduling.py) - Duplicate helper functions (exist in analysis_service.py)
Added comprehensive tests for: - Shot dates and data endpoints - Shot files endpoint - Profile image upload - Watcher status endpoint - Cache service functions (LLM, shot, image caching) - Scheduling endpoints (scheduled shots, recurring schedules) - History endpoints - System endpoints (version, status) - Profile endpoints (get, API errors) - Machine control endpoints (preheat, run-profile) - Process image for profile function - Utility functions (atomic write, sanitization) - Gemini service - History service - Settings service - Prompt builder - Config module - Meticulous service Coverage improved from 67% to 70% with 440 tests passing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 65 out of 70 changed files in this pull request and generated 15 comments.
| async def load_recurring_schedules(): | ||
| """Load recurring schedules from persistence.""" | ||
| global _recurring_schedules | ||
| _recurring_schedules = await _recurring_schedules_persistence.load() | ||
|
|
||
|
|
||
| async def restore_scheduled_shots(): | ||
| """Restore scheduled shots from disk on startup.""" | ||
| global _scheduled_shots | ||
| _scheduled_shots = await _scheduled_shots_persistence.load() | ||
|
|
Copilot
AI
Feb 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
load_recurring_schedules() and restore_scheduled_shots() rebind the module-level dicts (_recurring_schedules = ..., _scheduled_shots = ...). Any other module that did from services.scheduling_state import _recurring_schedules/_scheduled_shots will keep a reference to the old dict and never see loaded data. Update these functions to mutate the existing dict in place (e.g., clear()/update()), or expose accessor functions and avoid importing the dicts directly.
|
|
||
| import json | ||
| import time | ||
| from pathlib import Path |
Copilot
AI
Feb 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import of 'Path' is not used.
| import re | ||
| import uuid | ||
| from datetime import datetime, timezone | ||
| from pathlib import Path |
Copilot
AI
Feb 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import of 'Path' is not used.
meticai-server/main.py
Outdated
| f"using temporary directory: {log_dir}", | ||
| extra={"original_error": str(e)} | ||
| ) | ||
| from datetime import datetime, timezone |
Copilot
AI
Feb 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import of 'datetime' is not used.
Import of 'timezone' is not used.
| # TODO: Update tests to import from the new locations and remove these. | ||
|
|
||
| from config import DATA_DIR, MAX_UPLOAD_SIZE, TEST_MODE | ||
| from utils.file_utils import atomic_write_json, deep_convert_to_dict |
Copilot
AI
Feb 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import of 'atomic_write_json' is not used.
Import of 'deep_convert_to_dict' is not used.
| from services.scheduling_state import ( | ||
| _scheduled_shots, _scheduled_tasks, _recurring_schedules, | ||
| SchedulePersistence, ScheduledShotsPersistence, RecurringSchedulesPersistence, | ||
| get_next_occurrence as _get_next_occurrence, | ||
| restore_scheduled_shots as _restore_scheduled_shots, | ||
| load_recurring_schedules as _load_recurring_schedules, | ||
| PREHEAT_DURATION_MINUTES, | ||
| ) |
Copilot
AI
Feb 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import of '_scheduled_shots' is not used.
Import of 'SchedulePersistence' is not used.
Import of 'ScheduledShotsPersistence' is not used.
Import of 'RecurringSchedulesPersistence' is not used.
Import of '_get_next_occurrence' is not used.
Import of 'PREHEAT_DURATION_MINUTES' is not used.
| from api.routes.profiles import ( | ||
| process_image_for_profile, | ||
| _schedule_next_recurring, _recurring_schedule_checker | ||
| ) |
Copilot
AI
Feb 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import of 'process_image_for_profile' is not used.
| ) | ||
|
|
||
| # Re-export genai for tests that mock it | ||
| import google.generativeai as genai |
Copilot
AI
Feb 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import of 'genai' is not used.
| from services.scheduling_state import ( | ||
| _scheduled_shots, | ||
| _scheduled_tasks, | ||
| _recurring_schedules, | ||
| ScheduledShotsPersistence, | ||
| RecurringSchedulesPersistence, | ||
| save_scheduled_shots as _save_scheduled_shots, | ||
| save_recurring_schedules as _save_recurring_schedules, | ||
| load_recurring_schedules as _load_recurring_schedules, | ||
| restore_scheduled_shots as _restore_scheduled_shots, | ||
| get_next_occurrence as _get_next_occurrence, | ||
| PREHEAT_DURATION_MINUTES, | ||
| ) |
Copilot
AI
Feb 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import of 'ScheduledShotsPersistence' is not used.
Import of 'RecurringSchedulesPersistence' is not used.
Import of '_load_recurring_schedules' is not used.
Import of '_restore_scheduled_shots' is not used.
| """Settings service for managing application configuration.""" | ||
|
|
||
| import json | ||
| from pathlib import Path |
Copilot
AI
Feb 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import of 'Path' is not used.
- Remove unused datetime/timezone import from main.py - Remove unused Path, timedelta imports from profiles.py - Remove unused shutil import from profiles.py - Remove unused ScheduledShotsPersistence, RecurringSchedulesPersistence, load_recurring_schedules from profiles.py scheduling_state imports - Remove unused Optional, timedelta from scheduling.py - Remove unused Path from cache_service.py Note: The re-exports in main.py are intentional for backward compatibility with tests that import from main.py. These will be cleaned up when tests are updated to import from the proper service modules. All 440 tests pass.
Use clear()/update() instead of rebinding module-level dicts. This ensures modules that imported _recurring_schedules or _scheduled_shots directly will see the loaded data. Addresses PR review feedback.
Comprehensive Codebase Refactoring - ALL PHASES COMPLETE ✅
This PR successfully addresses all identified technical debt through a systematic 4-phase refactoring approach.
Phase 0-3: Complete ✅
(See previous sections for details)
Phase 4: Documentation & Configuration ✅ COMPLETE
Code Review Fixes ✅ ALL RESOLVED
All 35 review comments addressed:
Final Quality Check:
Security
Module Structure (Final)
Impact
Status: All phases complete, all code review issues resolved, zero security vulnerabilities. Ready for merge and deployment.
Original prompt
This section details on the original issue you should resolve
<issue_title>Comprehensive Codebase Refactoring: Modularization, Test Coverage, and Shell Script Consolidation</issue_title>
<issue_description>## Overview
This issue consolidates identified refactoring needs to improve maintainability, reduce technical debt, and ensure long-term sustainability of the MeticAI codebase. The analysis revealed several areas that would benefit from architectural improvements.
Critical Issues
1. Monolithic Main File (7,234 lines)
File:
coffee-relay/main.pyProblem: The main application file contains 121 functions and 7,234 lines of code, making it extremely difficult to:
Recommended Split:
api/routes/coffee.pyapi/routes/history.pyapi/routes/shots.pyapi/routes/profiles.pyapi/routes/system.pyapi/routes/scheduling.pyservices/gemini_service.pyservices/meticulous_service.pyservices/cache_service.pyservices/analysis_service.pyutils/file_utils.pyutils/sanitization.pymodels/schemas.py2. Low Test Coverage (25%)
Current State:
main.pyonly has 25% test coverage (369/1504 statements)Target: Aim for 70%+ coverage on critical paths
Gaps to Address:
_analyze_stage_execution,_perform_local_shot_analysis)3. Shell Script Code Duplication
Problem: 7 shell scripts duplicate the same patterns:
Solution: Create a shared library file:
Scripts affected:
local-install.sh(1,681 lines)update.sh(1,100 lines)uninstall.sh(621 lines)rebuild-watcher.sh(531 lines)web_install.sh(384 lines)docker-up.sh(85 lines)check-updates-on-start.sh(45 lines)Moderate Issues
4. Type Hints
File:
coffee-relay/main.pyProblem: Line 3852 uses
anytype which bypasses type checking:Solution: Replace with proper Union types or define a
VariableValuetype alias.5. Missing API Documentation
Problem: Many endpoints lack comprehensive docstrings with:
Files to Update:
main.py6. Configuration Management
Problem: Configuration is scattered:
.envdata/settings.jsonmain.py(UPDATE_CHECK_INTERVAL, MAX_UPLOAD_SIZE)Solution: Create a unified config module:
Minor Issues
7. Logging Inconsistency
Problem: Logger is initialized twice in
main.py:8. Docker Compose Volume Mounts
Problem: 20+ volume mounts in
docker-compose.ymlfor coffee-relay make it:.git,docker.sock)Solution: Document each mount's purpose and consider reducing where possible.
9. Dependency Versions
File:
coffee-relay/requirements.txtObservation: Some packages lack version pins:
Recommendation: Pin to specific versions for reproducibility.
9. Naming
"Coffee-relay" was the project name before MeticAI was fully developed. We should change this to MeticAI-server to keep similar naming...
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.