Implement and track all items from Possible updates.md across app, agents, tests, and docs#1
Implement and track all items from Possible updates.md across app, agents, tests, and docs#1
Possible updates.md across app, agents, tests, and docs#1Conversation
Co-authored-by: learnsol <104726581+learnsol@users.noreply.github.com>
Possible updates.md with prioritized codebase improvement roadmap
|
@copilot implement the documented changes in Possible updates.md Do it step by step and keep updating this document till you are done with all the changes |
Co-authored-by: learnsol <104726581+learnsol@users.noreply.github.com>
Co-authored-by: learnsol <104726581+learnsol@users.noreply.github.com>
Co-authored-by: learnsol <104726581+learnsol@users.noreply.github.com>
Implemented the backlog step-by-step and kept |
Possible updates.md with prioritized codebase improvement roadmapPossible updates.md across app, agents, tests, and docs
There was a problem hiding this comment.
Pull request overview
This PR implements a comprehensive set of improvements documented in Possible updates.md, focusing on security, resilience, input validation, and code quality. The changes migrate from a legacy Django-based implementation to a FastAPI-only architecture while adding retry logic, improved error handling, and test coverage.
Changes:
- Enhanced security by returning generic error messages to clients while maintaining detailed server-side logging
- Added retry logic with exponential backoff for external API calls (editor and illustrator agents)
- Implemented input validation with length bounds and agent caching for improved performance
- Replaced legacy Django views with compatibility stubs directing users to FastAPI routes
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 18 comments.
Show a summary per file
| File | Description |
|---|---|
| webapp/views.py | Replaced Django view implementation with runtime error stubs directing usage to FastAPI |
| tests/test_agents_and_processor.py | Added new test suite covering agent filtering, retry behavior, HTML generation, and config handling |
| requirements.txt | Cleaned up comment clutter from dependency declarations |
| main.py | Added input validation, agent caching, generic error handling, and startup warm-up logic |
| agents/story_processor.py | Hardened Text Analytics configuration handling to gracefully handle missing config |
| agents/illustrator_agent.py | Added retry logic with exponential backoff for image generation API calls |
| agents/editor_agent.py | Made edit_story synchronous with retry logic; added async wrapper via process_story |
| README.md | Added minimal local .env template and test command documentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| processor = StoryProcessor() | ||
| html = processor.process("Line 1\nLine 2", "https://img.example/cover.png", "prompt") | ||
| self.assertIn("<p>Line 1</p>", html) | ||
| self.assertIn("https://img.example/cover.png", html) | ||
|
|
||
| def test_story_processor_text_analytics_client_missing_config_returns_none(self): | ||
| processor = StoryProcessor() |
There was a problem hiding this comment.
The test attempts to instantiate StoryProcessor which loads 'azure_config.json' from disk in its init method (line 11 of story_processor.py calls load_azure_config). This test will fail if the file doesn't exist or is not accessible during test execution. The test should either mock the file loading or use dependency injection to provide test configuration instead of relying on external files.
| processor = StoryProcessor() | |
| html = processor.process("Line 1\nLine 2", "https://img.example/cover.png", "prompt") | |
| self.assertIn("<p>Line 1</p>", html) | |
| self.assertIn("https://img.example/cover.png", html) | |
| def test_story_processor_text_analytics_client_missing_config_returns_none(self): | |
| processor = StoryProcessor() | |
| with patch("agents.story_processor.load_azure_config", return_value={}): | |
| processor = StoryProcessor() | |
| html = processor.process("Line 1\nLine 2", "https://img.example/cover.png", "prompt") | |
| self.assertIn("<p>Line 1</p>", html) | |
| self.assertIn("https://img.example/cover.png", html) | |
| def test_story_processor_text_analytics_client_missing_config_returns_none(self): | |
| with patch("agents.story_processor.load_azure_config", return_value={}): | |
| processor = StoryProcessor() |
| for attempt in range(max_attempts): | ||
| try: | ||
| response = await self.client.images.generate( | ||
| model=self.config['deployment_name'], | ||
| prompt=prompt, | ||
| n=self.config['generation_params']['n'], | ||
| size=self.config['image_size'], | ||
| quality="standard" | ||
| ) | ||
|
|
||
| logger.info("Cover image generated successfully.") | ||
| return response.data[0].url | ||
| except Exception as e: | ||
| if attempt == max_attempts - 1: | ||
| logger.exception(f"Cover image generation failed: {str(e)}") | ||
| raise | ||
| backoff = 2 ** attempt | ||
| logger.warning(f"Retrying image generation in {backoff}s after error: {str(e)}") | ||
| await asyncio.sleep(backoff) |
There was a problem hiding this comment.
The retry logic retries all exceptions indiscriminately. This means non-transient errors (e.g., authentication failures, invalid parameters, quota exceeded) will be retried unnecessarily, wasting time and potentially masking configuration issues. Consider catching only transient exceptions (network errors, rate limits, temporary service unavailability) or checking exception types/status codes to determine if retry is appropriate.
There was a problem hiding this comment.
Applied the feedback in b00a618: retry logic now retries only transient failures in IllustratorAgent (status-code and error-type checks), and I added targeted tests for transient retry and non-transient no-retry behavior.
| prompt = self._create_cover_prompt(final_story) | ||
| max_retries = int(self.config.get("max_retries", 2)) | ||
| max_attempts = max_retries + 1 | ||
| for attempt in range(max_attempts): | ||
| try: | ||
| response = await self.client.images.generate( | ||
| model=self.config['deployment_name'], | ||
| prompt=prompt, | ||
| n=self.config['generation_params']['n'], | ||
| size=self.config['image_size'], | ||
| quality="standard" | ||
| ) | ||
|
|
||
| logger.info("Cover image generated successfully.") | ||
| return response.data[0].url | ||
| except Exception as e: | ||
| if attempt == max_attempts - 1: | ||
| logger.exception(f"Cover image generation failed: {str(e)}") | ||
| raise | ||
| backoff = 2 ** attempt | ||
| logger.warning(f"Retrying image generation in {backoff}s after error: {str(e)}") | ||
| await asyncio.sleep(backoff) |
There was a problem hiding this comment.
The retry logic added to IllustratorAgent.generate_cover_image lacks test coverage. While EditorAgent's retry behavior is tested (test_editor_edit_story_retries_then_succeeds), there's no equivalent test for the illustrator's retry logic. Since both agents now implement similar retry patterns, they should both have test coverage to ensure the retry mechanism works correctly for image generation failures as well.
| try: | ||
| get_agents() | ||
| except Exception as e: | ||
| logger.exception(f"Agent initialization failed during startup: {str(e)}") | ||
| raise |
There was a problem hiding this comment.
The startup handler attempts to warm up agents by calling get_agents() but doesn't verify that the agents are actually functional. If agent initialization silently succeeds but the agents are misconfigured (e.g., invalid API credentials, wrong endpoints), the error won't be caught until the first request. Consider adding a lightweight health check or validation step during startup that verifies the agents can actually communicate with their respective Azure services.
| agent = EditorAgent.__new__(EditorAgent) | ||
| filtered = agent.filter_content("Happy start. A scary ending.") | ||
| self.assertIn("Happy start.", filtered) | ||
| self.assertNotIn("scary", filtered.lower()) | ||
|
|
||
| def test_editor_edit_story_retries_then_succeeds(self): | ||
| agent = EditorAgent.__new__(EditorAgent) | ||
| agent.client = _FakeCompletionClient() | ||
| agent.config = { | ||
| "deployment_name": "model", | ||
| "prompt": {"system": "system prompt"}, | ||
| "temperature": 0.7, | ||
| "max_tokens": 256, | ||
| "max_retries": 2, | ||
| } |
There was a problem hiding this comment.
The test uses EditorAgent.new(EditorAgent) to bypass init, which is a fragile testing approach. This creates an instance without proper initialization and doesn't test the actual initialization logic. Additionally, it requires manually setting attributes like 'client' and 'config' that would normally be set during init. Consider using mock.patch to mock the file loading and environment variables instead, allowing the real init to run safely in tests.
| agent = EditorAgent.__new__(EditorAgent) | |
| filtered = agent.filter_content("Happy start. A scary ending.") | |
| self.assertIn("Happy start.", filtered) | |
| self.assertNotIn("scary", filtered.lower()) | |
| def test_editor_edit_story_retries_then_succeeds(self): | |
| agent = EditorAgent.__new__(EditorAgent) | |
| agent.client = _FakeCompletionClient() | |
| agent.config = { | |
| "deployment_name": "model", | |
| "prompt": {"system": "system prompt"}, | |
| "temperature": 0.7, | |
| "max_tokens": 256, | |
| "max_retries": 2, | |
| } | |
| agent = EditorAgent() | |
| filtered = agent.filter_content("Happy start. A scary ending.") | |
| self.assertIn("Happy start.", filtered) | |
| self.assertNotIn("scary", filtered.lower()) | |
| def test_editor_edit_story_retries_then_succeeds(self): | |
| agent = EditorAgent() | |
| agent.client = _FakeCompletionClient() | |
| if not hasattr(agent, "config") or agent.config is None: | |
| agent.config = {} | |
| agent.config["deployment_name"] = "model" | |
| agent.config["prompt"] = {"system": "system prompt"} | |
| agent.config["temperature"] = 0.7 | |
| agent.config["max_tokens"] = 256 | |
| agent.config["max_retries"] = 2 |
| processor = StoryProcessor() | ||
| html = processor.process("Line 1\nLine 2", "https://img.example/cover.png", "prompt") | ||
| self.assertIn("<p>Line 1</p>", html) | ||
| self.assertIn("https://img.example/cover.png", html) | ||
|
|
||
| def test_story_processor_text_analytics_client_missing_config_returns_none(self): | ||
| processor = StoryProcessor() |
There was a problem hiding this comment.
The test attempts to instantiate StoryProcessor which loads 'azure_config.json' from disk in its init method (line 11 of story_processor.py calls load_azure_config). This test will fail if the file doesn't exist or is not accessible during test execution. The test should either mock the file loading or use dependency injection to provide test configuration instead of relying on external files.
| processor = StoryProcessor() | |
| html = processor.process("Line 1\nLine 2", "https://img.example/cover.png", "prompt") | |
| self.assertIn("<p>Line 1</p>", html) | |
| self.assertIn("https://img.example/cover.png", html) | |
| def test_story_processor_text_analytics_client_missing_config_returns_none(self): | |
| processor = StoryProcessor() | |
| with patch("agents.story_processor.load_azure_config", return_value={}): | |
| processor = StoryProcessor() | |
| html = processor.process("Line 1\nLine 2", "https://img.example/cover.png", "prompt") | |
| self.assertIn("<p>Line 1</p>", html) | |
| self.assertIn("https://img.example/cover.png", html) | |
| def test_story_processor_text_analytics_client_missing_config_returns_none(self): | |
| with patch("agents.story_processor.load_azure_config", return_value={}): | |
| processor = StoryProcessor() |
| story = story.strip() | ||
| if not story: | ||
| logger.warning("No story provided in request") | ||
| raise HTTPException(status_code=400, detail="No story provided") |
There was a problem hiding this comment.
After stripping the story input and checking if it's empty, the validation still allows stories that only contain whitespace to pass through the FastAPI Form validation (which only checks original length before stripping). If a user submits exactly MAX_STORY_LENGTH characters of whitespace, it will pass the form validation but then fail the empty check. Consider either: (1) moving the strip operation before the form validation by using a custom validator, or (2) adjusting the logic to validate the stripped length explicitly.
| for attempt in range(max_attempts): | ||
| try: | ||
| response = self.client.chat.completions.create( | ||
| model=self.config['deployment_name'], | ||
| messages=[ | ||
| {"role": "system", "content": self.config['prompt']['system']}, | ||
| {"role": "user", "content": story} | ||
| ], | ||
| temperature=self.config['temperature'], | ||
| max_tokens=self.config['max_tokens'] | ||
| ) | ||
|
|
||
| edited_story = response.choices[0].message.content | ||
| return { | ||
| "final_story": edited_story, | ||
| "illustrator_prompt": "Create illustrations for: " + edited_story[:200] | ||
| } | ||
| except Exception as e: | ||
| if attempt == max_attempts - 1: | ||
| logger.exception(f"Error editing story: {str(e)}") | ||
| raise | ||
| backoff = 2 ** attempt | ||
| logger.warning(f"Retrying story edit in {backoff}s after error: {str(e)}") | ||
| time.sleep(backoff) |
There was a problem hiding this comment.
The retry logic retries all exceptions indiscriminately. This means non-transient errors (e.g., authentication failures, invalid parameters, quota exceeded) will be retried unnecessarily, wasting time and potentially masking configuration issues. Consider catching only transient exceptions (network errors, rate limits, temporary service unavailability) or checking exception types/status codes to determine if retry is appropriate.
| def create_kids_book(*args, **kwargs): | ||
| raise RuntimeError("Django views are not used. Use FastAPI routes in main.py.") | ||
|
|
||
| logger = logging.getLogger('kidsbook') | ||
|
|
||
| @csrf_exempt | ||
| async def create_kids_book(request): | ||
| """ | ||
| Async view that handles story creation and illustration generation. | ||
| Detailed logging is added to trace async operations. | ||
| """ | ||
| if request.method != 'POST': | ||
| logger.warning("Request method not allowed") | ||
| return JsonResponse({'error': 'Method not allowed'}, status=405) | ||
|
|
||
| try: | ||
| # Get story input from POST data | ||
| story_input = request.POST.get('story') | ||
| if not story_input: | ||
| logger.warning("No story provided in the request") | ||
| return JsonResponse({'error': 'No story provided'}, status=400) | ||
|
|
||
| logger.info("Initializing agents") | ||
| editor = EditorAgent() | ||
| illustrator = IllustratorAgent() | ||
| story_processor = StoryProcessor() | ||
|
|
||
| # Create a timeout for the entire operation | ||
| async with asyncio.timeout(300): # 5 minutes timeout | ||
| try: | ||
| logger.info("Calling editor.edit_story()") | ||
| editor_result = await editor.edit_story(story_input) | ||
| logger.info("Editor returned its result") | ||
| if not editor_result: | ||
| logger.error("Editor returned no result") | ||
| return JsonResponse({'error': 'Story editing failed'}, status=500) | ||
|
|
||
| final_story = editor_result['final_story'] | ||
| illustrator_prompt = editor_result['illustrator_prompt'] | ||
| logger.debug(f"Final story obtained, length: {len(final_story)}") | ||
| logger.debug(f"Illustrator prompt: {illustrator_prompt}") | ||
|
|
||
| logger.info("Calling illustrator.generate_illustrations()") | ||
| illustrations = await illustrator.generate_illustrations(illustrator_prompt) | ||
| logger.info("Illustrator returned its result") | ||
| if not illustrations: | ||
| logger.error("Illustrator returned no illustrations") | ||
| return JsonResponse({'error': 'Illustration generation failed'}, status=500) | ||
|
|
||
| logger.info("Calling story_processor.process() in thread pool") | ||
| composite_story = await asyncio.to_thread( | ||
| story_processor.process, | ||
| final_story=final_story, | ||
| illustrations=illustrations, | ||
| illustrator_prompt=illustrator_prompt | ||
| ) | ||
| logger.info("StoryProcessor returned composite story") | ||
|
|
||
| # Return response only after all processing is complete | ||
| logger.info("All async operations complete. Returning JSON response") | ||
| return JsonResponse({ | ||
| 'status': 'success', | ||
| 'composite_story': composite_story, | ||
| 'final_story': final_story, | ||
| 'illustrations': illustrations, | ||
| 'processing_complete': True | ||
| }) | ||
|
|
||
| except asyncio.TimeoutError: | ||
| logger.error("Operation timed out") | ||
| return JsonResponse({ | ||
| 'error': 'Operation timed out', | ||
| 'status': 'error', | ||
| 'processing_complete': False | ||
| }, status=504) | ||
|
|
||
| except Exception as e: | ||
| logger.exception(f"Error processing request: {str(e)}") | ||
| return JsonResponse({ | ||
| 'error': str(e), | ||
| 'status': 'error', | ||
| 'processing_complete': False | ||
| }, status=500) | ||
|
|
||
| def index(request): | ||
| """Renders the main page.""" | ||
| return render(request, 'index.html') No newline at end of file | ||
| def index(*args, **kwargs): | ||
| raise RuntimeError("Django views are not used. Use FastAPI routes in main.py.") |
There was a problem hiding this comment.
The stub functions have inconsistent parameter signatures. They accept *args and **kwargs but the actual functions they're replacing had specific signatures. If any code attempts to call these functions and relies on proper error messages about incorrect parameters, they won't get meaningful feedback. Consider defining the proper function signatures (request parameter for Django view compatibility) or providing more descriptive error messages that explain what the correct alternative is.
| async def create_kids_book( | ||
| request: Request, | ||
| story: str = Form(...), | ||
| story: str = Form(..., min_length=1, max_length=MAX_STORY_LENGTH), |
There was a problem hiding this comment.
The new input validation using MAX_STORY_LENGTH (line 75) lacks test coverage. There are no tests verifying that stories exceeding the maximum length are rejected, or that the min_length validation works correctly. Consider adding tests for boundary conditions: empty strings, strings at exactly max length, strings exceeding max length, and whitespace-only strings.
Co-authored-by: learnsol <104726581+learnsol@users.noreply.github.com>
Co-authored-by: learnsol <104726581+learnsol@users.noreply.github.com>
|
@copilot apply changes based on the comments in this thread |
Co-authored-by: learnsol <104726581+learnsol@users.noreply.github.com>
Co-authored-by: learnsol <104726581+learnsol@users.noreply.github.com>
Applied the requested thread feedback in commits |
azure_config.json💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.