feat(mcp): add diagnose_and_fix agent tool - #233
tonythethompson wants to merge 8 commits into
Conversation
Implements the diagnose_and_fix MCP tool (agent_diagnosis.py) for the Phase 3 autonomous agent loop. - Validates error_message (1-4000 chars) and recipe (must be dict) - Calls troubleshoot_olive_error internally with config context - Applies RFC 7386 JSON Merge Patch when KB entry has updated_config - Generates human-readable change descriptions - Best-effort recipe validation through Studio bridge - Maps fix_confidence: high/medium/low/none based on KB match quality - Top-level try/except for internal_error safety net - Zero new pip dependencies (stdlib only + internal imports) Requirements: 5.1-5.9, 6.3, 11.1, 11.3, 12.1-12.7
Implements the execute_and_observe MCP tool for the Phase 3 autonomous agent loop. Submits a recipe to Olive Studio via the loopback bridge, polls job status at 2-second intervals until a terminal state is reached or the effective timeout expires. Key behaviors: - Timeout clamping: min(max(timeout or 600, 10), 1800) - Terminal states: completed, failed, cancelled - Terminal-at-timeout-boundary: terminal wins (timed_out: false) - Pre-submission errors: no side_effect field - Post-submission results: side_effect: True - Logs capped at 200 entries, artifact refs as basenames only - Top-level try/except for internal_error safety Requirements: 1.1-1.12, 2.3, 2.4, 2.5, 11.1, 11.3, 12.1-12.7
Implements the compare_results MCP tool (task 6.1) for multi-job
comparison with preference-weighted scoring.
- Validates job_ids count (2-10) and format (^[A-Za-z0-9_-]{1,128}$)
- Normalizes preference (latency/size/accuracy/balanced)
- Fetches job status via studio_request loopback bridge
- Excludes non-terminal, failed, or unfetchable jobs
- Min-max normalizes metrics with lower-is-better inversion
- Applies 2x weight for preferred metric, 1x for others
- Selects highest scored job as winner
- Returns structured comparison with side_effect: False
- Top-level try/except for internal_error safety
- Zero new pip dependencies (stdlib + studio_loopback only)
Requirements: 7.1-7.8, 8.3, 11.1, 11.3, 12.1-12.7
Add hypothesis property-based tests covering: - Property 1: Timeout Clamping Invariant - verifies _clamp_timeout produces values in [10, 1800] for any integer input and defaults to 600 for None (Requirements 1.10, 1.11, 1.12) - Property 2: Side-Effect Field Correctness - verifies side_effect: True is present on successful submissions and absent on pre-submission errors (Requirement 2.3) All tests use mocked studio_request with @settings(max_examples=100). Validates: Requirements 1.10, 1.11, 1.12, 2.3
Add olive-mcp-server/tests/test_agent_model_info.py with 18 pytest unit tests covering: - HF API success (params from safetensors and config.num_parameters) - HF timeout fallback to heuristic (family default, low confidence) - HF 404 fallback with explicit size token (medium confidence) - Invalid model_id validation error - Confidence level differentiation (medium vs low) - VRAM estimate formula (params_b * 2.0) - Recommended quantization threshold (int4/int8 at 6B) - Model type classification via _normalize_model_type - JSON serialization round-trip for all outputs All tests monkeypatch _fetch_hf_metadata - no network access. Requirements: 13.5, 13.6, 13.7
|
Deployment failed for project olive-studio with the following error: Learn More: https://vercel.com/trackdub?upgradeToPro=build-rate-limit |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @tonythethompson, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
Warning Review limit reached
Next review available in: 41 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoAdd Phase 3 agent MCP tools (diagnose/execute/compare) with unit tests
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ca6a12128
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return { | ||
| "latency_ms": _as_float(raw.get("latency_ms")), | ||
| "model_size_mb": _as_float(raw.get("model_size_mb")), | ||
| "accuracy": _as_float(raw.get("accuracy")), |
There was a problem hiding this comment.
For actual Studio jobs, src/server/services/olive/gpu.ts stores {timestamp, gpus} in latestMetrics, and src/server/routes/olive.ts returns that object unchanged; it never contains latency_ms, model_size_mb, or accuracy. Consequently every completed job produces three None values here, all scores become zero, and max() reports the first requested job as the winner despite having no comparison data. Source these measurements from optimization results, or withhold the winner when no requested metric is available.
Useful? React with 👍 / 👎.
| # If Studio is down or returned an error, treat as not validated | ||
| if isinstance(response.get("error"), str) and response["error"]: | ||
| return False | ||
| return True |
There was a problem hiding this comment.
When Studio successfully processes an invalid repaired recipe, validate_optimization_job returns a normal payload with valid: false and an errors list, not an error string. This branch therefore returns True and tells the autonomous retry loop that an invalid fix was validated. Return the payload's valid value after checking for bridge errors.
Useful? React with 👍 / 👎.
| # Capture exit code | ||
| exit_code = status_response.get("exitCode") or status_response.get("exit_code") | ||
| if exit_code is not None: | ||
| last_exit_code = exit_code |
There was a problem hiding this comment.
For every normally completed job, Studio reports exitCode: 0, but the truthiness-based or discards that value and falls through to the absent snake-case field, producing None. The result therefore loses the definitive success code; select the fallback based on key presence or None rather than truthiness.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR introduces Phase 3 MCP agent tools for diagnosing and repairing recipes, executing optimization jobs to completion, and comparing completed jobs.
Confidence Score: 3/5The PR is not safe to merge until rejected recipe validation is reported accurately and comparison scores use actual optimization-result metrics. Recipe validation still treats a normal Files Needing Attention: olive-mcp-server/olive_mcp_server/tools/agent_diagnosis.py; olive-mcp-server/olive_mcp_server/tools/agent_compare.py
|
| Filename | Overview |
|---|---|
| olive-mcp-server/olive_mcp_server/tools/agent_diagnosis.py | Adds troubleshooting-based recipe diagnosis, merge-patch repair, change descriptions, confidence mapping, and Studio validation. |
| olive-mcp-server/olive_mcp_server/tools/agent_compare.py | Adds completed-job filtering, metric normalization, preference weighting, winner selection, and exclusion reasoning. |
| olive-mcp-server/olive_mcp_server/tools/agent_execute.py | Adds Studio job submission, bounded polling, terminal and timeout handling, partial failure results, and artifact-reference extraction. |
| olive-mcp-server/tests/test_agent_execute.py | Covers successful, failed, timed-out, denied, unavailable, malformed, and interrupted execution flows. |
| olive-mcp-server/tests/test_agent_model_info.py | Adds coverage for API-derived and heuristic model metadata, confidence levels, derived recommendations, and serialization. |
Reviews (2): Last reviewed commit: "fix: Preserve zero exit codes" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # If Studio is down or returned an error, treat as not validated | ||
| if isinstance(response.get("error"), str) and response["error"]: | ||
| return False | ||
| return True |
There was a problem hiding this comment.
When Studio rejects a repaired recipe with valid=false and an errors list, this check sees no top-level error and returns True, causing an invalid recipe to be reported as validated.
| # If Studio is down or returned an error, treat as not validated | |
| if isinstance(response.get("error"), str) and response["error"]: | |
| return False | |
| return True | |
| # Require Studio to explicitly confirm that the recipe is valid. | |
| return response.get("valid") is True |
Prompt To Fix With AI
This is a comment left during a code review.
Path: olive-mcp-server/olive_mcp_server/tools/agent_diagnosis.py
Line: 127-130
Comment:
**Validation result is ignored**
When Studio rejects a repaired recipe with `valid=false` and an `errors` list, this check sees no top-level `error` and returns `True`, causing an invalid recipe to be reported as validated.
```suggestion
# Require Studio to explicitly confirm that the recipe is valid.
return response.get("valid") is True
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| "latency_ms": _as_float(raw.get("latency_ms")), | ||
| "model_size_mb": _as_float(raw.get("model_size_mb")), | ||
| "accuracy": _as_float(raw.get("accuracy")), |
There was a problem hiding this comment.
The Studio status endpoint supplies latestMetrics as GPU telemetry containing timestamp and gpus, but this code reads optimization fields directly from it. Every job therefore receives a zero score, causing the first requested job to be selected regardless of its actual optimization results.
Prompt To Fix With AI
This is a comment left during a code review.
Path: olive-mcp-server/olive_mcp_server/tools/agent_compare.py
Line: 57-59
Comment:
**Scoring reads GPU telemetry**
The Studio status endpoint supplies `latestMetrics` as GPU telemetry containing `timestamp` and `gpus`, but this code reads optimization fields directly from it. Every job therefore receives a zero score, causing the first requested job to be selected regardless of its actual optimization results.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| for jid in job_ids: | ||
| response = studio_request("GET", f"{_STATUS_PATH}/{jid}") | ||
|
|
There was a problem hiding this comment.
1. Sequential studio_request status fetches 📘 Rule violation ➹ Performance
compare_results fetches each job status sequentially, creating an avoidable request waterfall when multiple independent job IDs are compared. This can increase overall tool latency proportional to the number of job IDs.
Agent Prompt
## Issue description
`compare_results` currently issues one `studio_request()` per job ID in a loop, which creates an avoidable request waterfall for independent status fetches.
## Issue Context
This tool compares 2–10 jobs; since each status fetch is independent, the total time can be reduced by fetching statuses concurrently (e.g., via a small thread pool for I/O-bound calls, or via a batch endpoint if available).
## Fix Focus Areas
- olive-mcp-server/olive_mcp_server/tools/agent_compare.py[214-239]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| if mx == mn: | ||
| normalized = 1.0 # All values equal → full score | ||
| else: | ||
| normalized = (val - mn) / (mx - mn) |
There was a problem hiding this comment.
3. Inverted tie scores zero 🐞 Bug ≡ Correctness
In compare_results, when all jobs have the same latency/size value, the code sets normalized=1.0 and then inverts it to 0.0, contradicting the “full score” tie comment. This yields misleading scores and can skew ranking when some jobs are missing other metrics (e.g., a job with only latency populated gets 0.0 instead of a tie-best score).
Agent Prompt
### Issue description
For inverted metrics (`latency_ms`, `model_size_mb`), the tie branch (`mx == mn`) sets `normalized = 1.0` and then applies inversion, resulting in `0.0` for all jobs on that metric. This contradicts the tie comment and produces misleading scores; it can also affect winner selection when other metrics are missing.
### Issue Context
Latency/size are “lower is better” metrics. If all values are equal, every job should receive an equal *best/tie* contribution for that metric (not zero).
### Fix Focus Areas
- olive-mcp-server/olive_mcp_server/tools/agent_compare.py[126-134]
### Suggested fix
Handle the `mx == mn` case after considering directionality, e.g. either:
- Skip inversion in the tie case, or
- Set `normalized = 0.0` for inverted metrics when `mx == mn` (so `1 - normalized` becomes `1.0`), and keep `normalized = 1.0` for non-inverted metrics.
Keep behavior consistent with the comment and ensure scores remain meaningful even when only some metrics are present.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Qodo Fixer✅ Committed (1) · ☑ Fixed (1) Commits pushed directly to this PR — no separate fix PR opened. Process — 1 fixed
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
Superseded by consolidated PR #245 |
1 participant
Implements the diagnose_and_fix MCP tool (agent_diagnosis.py) for the
Phase 3 autonomous agent loop.
Requirements: 5.1-5.9, 6.3, 11.1, 11.3, 12.1-12.7