Refactor/resilience simplification - #286
Merged
Merged
Conversation
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Phase 0: Decompose deployed_api.py god function - Break 210-line validate_deployed_api() into _post_challenge() + _verify_challenge() - Add try/finally cleanup for challenge entries (replaces 6 scattered checks) Phase 1: Centralize timeout configuration - Add 8 timeout fields to Settings (config.py) - Replace 13 hardcoded timeout values across 5 files - Remove AZURE_TOKEN_TIMEOUT constant from azure_auth.py Phase 2: Consolidate resilience infrastructure - Create errors.py: single source of truth for CB constants, ServerError hierarchy, make_retriable() factory, and error-to-result mappers - Drop server_error field from ValidationResult, use verification_completed instead (eliminates double negation in submissions_service.py) - Refactor check_github_url_exists/check_repo_is_fork_of from tuple returns to ValidationResult (callers use model_copy for message composition) - Update all services, tests, and token_base.py dict keys with boolean inversion (server_error=False -> verification_completed=True) All 736 tests pass. No behavior changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
PR and DevOps verification agents had no temperature setting, causing non-deterministic grading — same PR could fail then pass on retry. Setting temperature=0 makes grading reproducible. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…essed' The log fires for every attempt regardless of outcome, not just successful validations. 'processed' accurately reflects that a submission was handled and stored. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The phase detail page (/phase/N) computed its progress percentage from reading steps alone, ignoring hands-on verification requirements. A user who completed all reading steps but had incomplete hands-on tasks would see 100% progress. Changes: - PhaseDetailProgress now has hands_on_validated/hands_on_required fields - percentage is a computed_field factoring in both steps and hands-on - get_phase_detail_progress queries denormalized progress for hands-on count - Added test verifying hands-on requirements affect percentage Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ndpoint - Remove '/ 1ms' from api_latency KQL query: duration is already a real (milliseconds) in App Insights, dividing by timespan literal caused 'Arithmetic expression cannot be carried-out between R64 and TimeSpan' - Construct OpenAI endpoint URL explicitly instead of using the provider's .endpoint attribute which flips between cognitiveservices.azure.com and openai.azure.com between plan/apply (provider bug) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors resilience/timeout handling across the API verification flows, standardizes “attempt counted” semantics via verification_completed, and expands Azure monitoring + operational runbooks.
Changes:
- Centralize resiliency primitives (circuit breaker constants, retriable exception tuples, and error→
ValidationResultmappers) and migrate verification services fromserver_errortoverification_completed. - Replace hardcoded timeouts with
Settings-driven configuration (startup, migrations, DB ops, Azure token acquisition, deployed API checks, verification streaming). - Add/extend Azure Monitor alerting (DB + Container App metric alerts, availability web test, new log alerts) and update the “check-prod” skill to use
azCLI.
Show a summary per file
| File | Description |
|---|---|
| infra/monitoring.tf | Adds metric alerts, availability web test + alert, and additional scheduled query alerts. |
| api/tests/test_database.py | Updates Azure auth timeout test to patch settings-based timeout. |
| api/tests/services/test_progress_service.py | Extends progress tests for hands-on inclusion and patches new dependency calls. |
| api/tests/services/test_pr_verification_service.py | Updates assertions for verification_completed semantics. |
| api/tests/services/test_hands_on_verification_service.py | Updates mocks to use verification_completed. |
| api/tests/services/test_github_hands_on_verification_service.py | Updates GitHub profile tests to use ValidationResult returns and verification_completed. |
| api/tests/services/test_ci_status_verification_service.py | Updates assertions for verification_completed semantics. |
| api/services/verification/token_base.py | Renames result key from server_error to verification_completed across token verification flow. |
| api/services/verification/security_scanning.py | Uses shared circuit breaker constants from new errors module. |
| api/services/verification/pull_request.py | Forces deterministic grading by setting LLM temperature to 0. |
| api/services/verification/llm_base.py | Centralizes retriable exception tuple construction via shared helper. |
| api/services/verification/github_profile.py | Refactors GitHub checks to return ValidationResult and uses shared errors utilities/constants. |
| api/services/verification/errors.py | New shared module for circuit breaker constants, retriable exception composition, and error mappers. |
| api/services/verification/dispatcher.py | Propagates verification_completed through dispatcher results. |
| api/services/verification/devops_analysis.py | Forces deterministic grading by setting LLM temperature to 0. |
| api/services/verification/deployed_api.py | Centralizes deployed API error handling + moves timeouts to settings; refactors challenge/verify/cleanup flow. |
| api/services/submissions_service.py | Switches attempt-counting to validation_result.verification_completed and adjusts logging event name. |
| api/services/progress_service.py | Includes hands-on requirements in phase detail progress response. |
| api/schemas.py | Adds hands-on fields + computed percentage to PhaseDetailProgress; migrates schema field to verification_completed. |
| api/routes/htmx_routes.py | Uses settings-based timeout for verification result streaming. |
| api/main.py | Uses settings-based timeouts for startup init and migrations. |
| api/core/database.py | Uses settings-based timeouts for DB connection/operations and Azure asyncpg connect. |
| api/core/config.py | Adds centralized timeout configuration fields to Settings. |
| api/core/azure_auth.py | Uses settings-based Azure token acquisition timeout. |
| .github/skills/check-prod/SKILL.md | Updates production health check runbook to use az CLI and adds new checks/thresholds. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
infra/monitoring.tf:336
- The latency alert query computes
P95Ms = percentile(duration, 95), but in Application Insights therequests.durationcolumn is a timespan. Scheduled query rule v2 thresholds expect a numeric measure, so this may fail type checking or never trigger. Convert to milliseconds in the query (e.g.,P95Ms = todouble(percentile(duration, 95) / 1ms)) or compare against a timespan literal in-query and emit a numeric flag.
query = <<-QUERY
requests
| summarize P95Ms = percentile(duration, 95) by bin(timestamp, 5m)
QUERY
time_aggregation_method = "Maximum"
operator = "GreaterThan"
threshold = 500
metric_measure_column = "P95Ms"
- Files reviewed: 24/24 changed files
- Comments generated: 2
…etch - Count unique requirement IDs in hands_on_required to prevent potential inflation from duplicates - Add get_by_user_and_phase() repository method to fetch only the needed phase row instead of all phases - Update test mock to match new method signature Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Remove unnecessary complexity across 5 areas: Observability: Remove OTLP backend, SQLAlchemy/Agent Framework instrumentation. Keep Azure Monitor + FastAPI + HTTPX. Reduce metrics from 8 to 3 (verification counter, duration, LLM token counter). Verification system: Merge 3 token validator files into 1 compact module. Remove wrapper functions and intermediate result types. Dispatcher calls verify_ctf_token/verify_networking_token directly. Config: Consolidate 9 timeout settings into 4 (db_timeout, external_api_timeout, startup_timeout, verification_wait_timeout). Database indexes: Drop 3 redundant indexes from submissions table, keeping 4 that match actual query patterns. Add migration 0016. Infrastructure monitoring: Reduce 13 alerts to 3 (availability, 5xx errors, LLM failures). Remove unused portal dashboard. Also fix submission queries to use id instead of created_at for latest-row lookups, preventing NULL created_at from silently dropping rows. Net result: ~4000 lines removed across 67 files. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.