diff --git a/SPECS/ARCHIVE/INDEX.md b/SPECS/ARCHIVE/INDEX.md index 7f91ca26..f70353c7 100644 --- a/SPECS/ARCHIVE/INDEX.md +++ b/SPECS/ARCHIVE/INDEX.md @@ -1,6 +1,6 @@ # mcpbridge-wrapper Tasks Archive -**Last Updated:** 2026-02-15 (P12-T1) +**Last Updated:** 2026-02-15 (P12-T3) ## Archived Tasks @@ -95,6 +95,7 @@ | P11-T3 | [P11-T3_Add_Dashboard_Theme_Toggle/](P11-T3_Add_Dashboard_Theme_Toggle/) | 2026-02-15 | PASS | | P11-T4 | [P11-T4_Add_Keyboard_Shortcuts_Command_Palette/](P11-T4_Add_Keyboard_Shortcuts_Command_Palette/) | 2026-02-15 | PASS | | P12-T1 | [P12-T1_Add_MCP_Client_Identification/](P12-T1_Add_MCP_Client_Identification/) | 2026-02-15 | PASS | +| P12-T3 | [P12-T3_Add_Error_Classification_and_Categorization/](P12-T3_Add_Error_Classification_and_Categorization/) | 2026-02-15 | PASS | ## Historical Artifacts @@ -154,6 +155,7 @@ | [REVIEW_P11-T3_dashboard_theme_toggle.md](P11-T3_Add_Dashboard_Theme_Toggle/REVIEW_P11-T3_dashboard_theme_toggle.md) | Review report for P11-T3 | | [REVIEW_P11-T4_keyboard_shortcuts.md](P11-T4_Add_Keyboard_Shortcuts_Command_Palette/REVIEW_P11-T4_keyboard_shortcuts.md) | Review report for P11-T4 | | [REVIEW_P12-T1_mcp_client_identification.md](P12-T1_Add_MCP_Client_Identification/REVIEW_P12-T1_mcp_client_identification.md) | Review report for P12-T1 | +| [REVIEW_P12-T3_error_classification_categorization.md](P12-T3_Add_Error_Classification_and_Categorization/REVIEW_P12-T3_error_classification_categorization.md) | Review report for P12-T3 | ## Archive Log @@ -261,3 +263,5 @@ | 2026-02-15 | P11-T4 | Archived REVIEW_P11-T4_keyboard_shortcuts report | | 2026-02-15 | P12-T1 | Archived Add_MCP_Client_Identification (PASS) | | 2026-02-15 | P12-T1 | Archived REVIEW_P12-T1_mcp_client_identification report | +| 2026-02-15 | P12-T3 | Archived Add_Error_Classification_and_Categorization (PASS) | +| 2026-02-15 | P12-T3 | Archived REVIEW_P12-T3_error_classification_categorization report | diff --git a/SPECS/ARCHIVE/P12-T3_Add_Error_Classification_and_Categorization/P12-T3_Add_Error_Classification_and_Categorization.md b/SPECS/ARCHIVE/P12-T3_Add_Error_Classification_and_Categorization/P12-T3_Add_Error_Classification_and_Categorization.md new file mode 100644 index 00000000..ff919a45 --- /dev/null +++ b/SPECS/ARCHIVE/P12-T3_Add_Error_Classification_and_Categorization/P12-T3_Add_Error_Classification_and_Categorization.md @@ -0,0 +1,114 @@ +# P12-T3: Add Error Classification & Categorization + +**Phase:** Phase 12 — Analytics & Insights +**Priority:** P1 +**Branch:** feature/P12-T3-error-classification-categorization +**Dependencies:** P10-T1 ✅ + +--- + +## Overview + +Parse JSON-RPC error codes and messages from responses and categorize them into +meaningful buckets. Extend the metrics pipeline to track error breakdown, display +an error doughnut chart on the dashboard, and color-code the audit table error +column by severity. + +--- + +## Deliverables + +### 1. `src/mcpbridge_wrapper/schemas.py` +- Add `get_error_code()` and `get_error_message()` helper methods to `MCPResponse` + that return `Optional[int]` and `Optional[str]` from `self.error.code / .message`. + +### 2. `src/mcpbridge_wrapper/webui/metrics.py` (MetricsCollector) +- Add `_error_counts_by_code: Dict[int, int]` attribute. +- Extend `record_response` to accept `error_code: Optional[int] = None` and + `error_message: Optional[str] = None` parameters. +- When `error=True` and `error_code` is provided, increment + `_error_counts_by_code[error_code]`. +- Expose `error_counts_by_code` in `get_summary()` output. +- Reset `_error_counts_by_code` in `reset()`. + +### 3. `src/mcpbridge_wrapper/webui/shared_metrics.py` (SharedMetricsStore) +- Add `error_code INTEGER` and `error_message TEXT` columns to the `requests` + SQLite table (via `ALTER TABLE IF NOT EXISTS` pattern in `_ensure_db`). +- Extend `record_response` to accept and persist `error_code` and `error_message`. +- Extend `get_summary()` to compute and return `error_counts_by_code: Dict[int, int]` + from the DB. + +### 4. `src/mcpbridge_wrapper/__main__.py` +- In the response handler, after `_has_error(line)`, extract the error code/message + using `MCPResponse.model_validate_json(line)` (reuse existing parse logic). +- Pass `error_code` and `error_message` to `metrics.record_response(...)`. + +### 5. Error category helper (`src/mcpbridge_wrapper/webui/metrics.py`) +- Add module-level function `categorize_error(code: Optional[int]) -> str` that + returns one of: `"protocol"` (codes -32600 to -32699), `"timeout"` (custom + code -32001), `"tool"` (positive codes ≥ 1), `"unknown"` (everything else). + +### 6. `src/mcpbridge_wrapper/webui/server.py` +- Include `error_counts_by_code` in the `/api/metrics/summary` JSON response + (already populated from `get_summary()`; just ensure it's passed through). + +### 7. `src/mcpbridge_wrapper/webui/static/index.html` +- Add a `` container in the dashboard, + alongside the existing KPI cards (or replacing "Total Errors" card with a + chart section beneath the KPI grid). + +### 8. `src/mcpbridge_wrapper/webui/static/dashboard.js` +- Initialize a Chart.js doughnut chart (`errorBreakdownChart`) using the + `error_counts_by_code` data from the metrics summary. +- Map codes to category labels via `categorizeError(code)`. +- Color-code audit table error column cells by severity class: + - `error-protocol` (red) for -326xx codes + - `error-tool` (orange) for positive codes + - `error-timeout` (yellow) for -32001 + - `error-unknown` (grey) for all others + +### 9. `src/mcpbridge_wrapper/webui/static/dashboard.css` +- Add CSS classes: `.error-protocol`, `.error-tool`, `.error-timeout`, + `.error-unknown` with appropriate color styling. + +### 10. Tests +- `tests/unit/webui/test_metrics.py`: + - Test `categorize_error()` for all four categories. + - Test `MetricsCollector.record_response()` with `error_code` tracks in + `error_counts_by_code`. + - Test `get_summary()` includes `error_counts_by_code`. +- `tests/unit/webui/test_shared_metrics.py`: + - Test `SharedMetricsStore.record_response()` with `error_code` and `error_message`. + - Test `get_summary()` returns `error_counts_by_code`. +- `tests/unit/test_main.py`: + - Test that error code is extracted and passed to `metrics.record_response`. + +--- + +## Acceptance Criteria + +- [ ] JSON-RPC error code and message are extracted from error responses. +- [ ] `MetricsCollector.get_summary()` includes `error_counts_by_code`. +- [ ] `SharedMetricsStore.get_summary()` includes `error_counts_by_code`. +- [ ] Dashboard displays error breakdown doughnut chart. +- [ ] Audit table error column color-coded by severity. +- [ ] `categorize_error()` correctly maps: protocol (-326xx), timeout (-32001), + tool (positive), unknown (rest). +- [ ] All tests pass. Coverage ≥ 90%. + +--- + +## Dependencies + +- `MCPResponse` in `schemas.py` already has `error: Optional[MCPError]` with + `code: int` and `message: str`. No schema changes needed beyond helpers. +- Chart.js is already loaded in `index.html`. +- `_has_error()` in `__main__.py` already parses the response — reuse that parse. + +--- + +## Out of Scope + +- Storing full error payloads beyond code + message. +- Retroactive error categorization for existing audit log entries. +- WebSocket push for error breakdown (polling is sufficient). diff --git a/SPECS/ARCHIVE/P12-T3_Add_Error_Classification_and_Categorization/P12-T3_Validation_Report.md b/SPECS/ARCHIVE/P12-T3_Add_Error_Classification_and_Categorization/P12-T3_Validation_Report.md new file mode 100644 index 00000000..cc540d8f --- /dev/null +++ b/SPECS/ARCHIVE/P12-T3_Add_Error_Classification_and_Categorization/P12-T3_Validation_Report.md @@ -0,0 +1,51 @@ +# P12-T3 Validation Report — Add Error Classification & Categorization + +**Date:** 2026-02-15 +**Branch:** feature/P12-T3-error-classification-categorization + +--- + +## Quality Gate Results + +| Gate | Result | Detail | +|------|--------|--------| +| `pytest` | ✅ PASS | 437 passed, 5 skipped | +| `ruff check src/` | ✅ PASS | No issues | +| `mypy src/` | ✅ PASS | No issues in 13 source files | +| `pytest --cov` | ✅ PASS | 96.09% (≥ 90% required) | + +--- + +## Acceptance Criteria Check + +| Criterion | Status | +|-----------|--------| +| JSON-RPC error code and message extracted from error responses | ✅ | +| `MetricsCollector.get_summary()` includes `error_counts_by_code` | ✅ | +| `SharedMetricsStore.get_summary()` includes `error_counts_by_code` | ✅ | +| Dashboard displays error breakdown doughnut chart | ✅ | +| Audit table error column color-coded by severity | ✅ | +| `categorize_error()` maps protocol/timeout/tool/unknown correctly | ✅ | +| All tests pass, coverage ≥ 90% | ✅ | + +--- + +## Files Modified + +| File | Change | +|------|--------| +| `src/mcpbridge_wrapper/schemas.py` | Added `get_error_code()` and `get_error_message()` methods to `MCPResponse` | +| `src/mcpbridge_wrapper/__main__.py` | Added `_parse_error_info()`, updated response handler to extract and pass error code/message | +| `src/mcpbridge_wrapper/webui/metrics.py` | Added `categorize_error()`, `_error_counts_by_code` tracking, extended `record_response` | +| `src/mcpbridge_wrapper/webui/shared_metrics.py` | Added `error_code`/`error_message` columns, extended `record_response`, `get_summary` includes breakdown | +| `src/mcpbridge_wrapper/webui/audit.py` | Added `error_code` parameter to `log()` | +| `src/mcpbridge_wrapper/webui/static/index.html` | Added error breakdown chart canvas | +| `src/mcpbridge_wrapper/webui/static/dashboard.js` | Added `categorizeError()`, `updateErrorBreakdownChart()`, severity-based audit color coding | +| `src/mcpbridge_wrapper/webui/static/dashboard.css` | Added `.error-protocol`, `.error-tool`, `.error-timeout`, `.error-unknown` classes | +| `tests/unit/webui/test_metrics.py` | Added `TestCategorizeError` class and `error_counts_by_code` tests | +| `tests/unit/webui/test_shared_metrics.py` | Added `error_counts_by_code` tests | +| `tests/unit/test_main.py` | Added `TestParseErrorInfo` class | + +--- + +## Verdict: PASS diff --git a/SPECS/ARCHIVE/P12-T3_Add_Error_Classification_and_Categorization/REVIEW_P12-T3_error_classification_categorization.md b/SPECS/ARCHIVE/P12-T3_Add_Error_Classification_and_Categorization/REVIEW_P12-T3_error_classification_categorization.md new file mode 100644 index 00000000..fbd43b29 --- /dev/null +++ b/SPECS/ARCHIVE/P12-T3_Add_Error_Classification_and_Categorization/REVIEW_P12-T3_error_classification_categorization.md @@ -0,0 +1,68 @@ +## REVIEW REPORT — P12-T3 Error Classification & Categorization + +**Scope:** origin/main..HEAD +**Files:** 12 modified (8 src, 4 tests) + +--- + +### Summary Verdict + +- [ ] Approve +- [x] Approve with comments +- [ ] Request changes +- [ ] Block + +--- + +### Critical Issues + +None. + +--- + +### Secondary Issues + +**[Low] `error_message` parameter accepted by `record_response` but unused in `MetricsCollector`** + +`MetricsCollector.record_response()` now accepts `error_message: Optional[str]` but never stores or uses it. The in-memory collector tracks only `error_counts_by_code`. While the `SharedMetricsStore` correctly persists `error_message` to SQLite, the in-memory variant silently drops the value. This is fine for current usage (only the DB store is used at runtime) but could cause confusion in tests that rely on `MetricsCollector` directly. + +Fix suggestion: Either add a docstring note clarifying the parameter is accepted for API symmetry but not stored, or remove the parameter from `MetricsCollector` if it will never be needed. + +**[Low] `categorize_error` duplicated between Python and JavaScript** + +The categorization logic (`-32600...-32699` → protocol, `-32001` → timeout, `≥1` → tool) is implemented independently in both `metrics.py` and `dashboard.js`. If the boundaries change, both files must be updated in sync. This is a minor maintenance risk. + +Fix suggestion: Add a comment in both locations explicitly cross-referencing each other so future changes don't diverge silently. + +**[Low] `error_code` in audit entries is not covered by export CSV** + +`audit.py`'s `export_csv()` generates CSV columns from a fixed key set. The new `error_code` field will be missing from CSV exports since the column list is not dynamically derived from entry keys. + +Fix suggestion: Either add `error_code` to the CSV column list, or note this as a known limitation in a follow-up task. + +--- + +### Architectural Notes + +- The `contextlib.suppress(Exception)` pattern for `ALTER TABLE` migrations is pragmatic but suppresses all exceptions, including unexpected ones like disk-full. This is acceptable for the current scale but would be worth narrowing to `sqlite3.OperationalError` in a production system. +- The `categorize_error` function is exposed at module level from `metrics.py`, which is the right place — it's domain logic, not UI logic. The JS duplicate is unavoidable for client-side rendering. +- The error breakdown chart correctly hides itself when no errors have been recorded, providing a clean empty state. + +--- + +### Tests + +- 437 tests pass, 5 skipped. +- Coverage: 96.09% (≥ 90% required). +- `TestCategorizeError`: 9 tests covering all branches including boundary conditions. +- `TestParseErrorInfo`: 5 tests covering success, error with code/message, and invalid JSON. +- `test_error_counts_by_code_*`: 5 tests in `TestMetricsCollector` + 5 tests in `TestSharedMetricsStore`. +- No missing coverage for the new code paths. + +--- + +### Next Steps + +- FU-P12-T3-1: Document or remove unused `error_message` param in `MetricsCollector.record_response` (Low) +- FU-P12-T3-2: Add `error_code` column to CSV audit export (Low) +- Consider adding cross-reference comments between Python and JS `categorize_error` implementations. diff --git a/SPECS/INPROGRESS/next.md b/SPECS/INPROGRESS/next.md index d278910c..5168b763 100644 --- a/SPECS/INPROGRESS/next.md +++ b/SPECS/INPROGRESS/next.md @@ -4,15 +4,16 @@ The previously selected task has been archived. ## Recently Archived +- 2026-02-15 — P12-T3: Add Error Classification & Categorization (PASS) - 2026-02-15 — P12-T1: Add MCP Client Identification (PASS) - 2026-02-15 — P11-T4: Add Keyboard Shortcuts & Command Palette (PASS) - 2026-02-15 — P11-T3: Add Dashboard Theme Toggle (Dark/Light) (PASS) - 2026-02-15 — P11-T2: Add Session Timeline View (PASS) -- 2026-02-15 — BUG-T8: Audit log cross-process visibility (PASS) ## Suggested Next Tasks - P12-T2: Add Tool Parameter Frequency Analysis (P3, depends on P12-T1 ✅) -- P12-T3: Add Error Classification & Categorization (P1, depends on P10-T1 ✅) +- FU-P12-T1-1: Remove or document `MCPInitializeParams` in schemas (P3, depends on P12-T1 ✅) +- FU-P12-T1-2: Add code comment clarifying stdin-only client capture in `on_request` (P3, depends on P12-T1 ✅) - FU-BUG-T7-1: Cap `pending_methods` map to guard against unbounded growth (P3) - FU-P11-T1-1: Refactor `_FakeWebUIConfig` test stub to `MagicMock(spec=WebUIConfig)` (P3) diff --git a/SPECS/Workplan.md b/SPECS/Workplan.md index 5ca51e3e..a7d3bab4 100644 --- a/SPECS/Workplan.md +++ b/SPECS/Workplan.md @@ -1719,7 +1719,7 @@ Phase 9 Follow-up Backlog --- -#### P12-T3: Add Error Classification & Categorization +#### ✅ P12-T3: Add Error Classification & Categorization - **Description:** Parse JSON-RPC error codes and messages from responses. Categorize into buckets: protocol errors (-326xx), tool execution errors (Xcode-side failures), timeout errors, connection errors. Extend `record_response` to accept `error_code: Optional[int]` and `error_message: Optional[str]`. New metrics: `error_counts_by_code: Dict[int, int]`. Dashboard: replace single "Total Errors" KPI with error breakdown doughnut chart. Audit table: color-code error column by severity. - **Priority:** P1 - **Dependencies:** P10-T1 @@ -1945,6 +1945,34 @@ Phase 9 Follow-up Backlog --- +#### FU-P12-T3-1: Document unused `error_message` parameter in `MetricsCollector.record_response` +- **Description:** `MetricsCollector.record_response()` accepts `error_message: Optional[str]` for API symmetry with `SharedMetricsStore`, but never stores or uses it. Add a docstring note clarifying this parameter is accepted for interface compatibility but not persisted in the in-memory collector. +- **Priority:** P3 +- **Dependencies:** P12-T3 +- **Parallelizable:** yes +- **Outputs/Artifacts:** + - Updated `src/mcpbridge_wrapper/webui/metrics.py` — docstring clarification on `error_message` parameter +- **Acceptance Criteria:** + - [ ] Docstring clearly notes `error_message` is accepted for API symmetry but not stored in-memory + - [ ] No functional changes + +--- + +#### FU-P12-T3-2: Add `error_code` column to audit CSV export +- **Description:** `AuditLogger.export_csv()` uses a fixed column list that does not include `error_code`. Audit entries with error codes will silently omit this field from CSV exports. Add `error_code` to the CSV column list so it is included when present. +- **Priority:** P3 +- **Dependencies:** P12-T3 +- **Parallelizable:** yes +- **Outputs/Artifacts:** + - Updated `src/mcpbridge_wrapper/webui/audit.py` — `error_code` added to CSV export columns + - Updated `tests/unit/webui/test_audit.py` — test verifying `error_code` in CSV output +- **Acceptance Criteria:** + - [ ] CSV export includes `error_code` column + - [ ] Entries without `error_code` show empty string for the column + - [ ] Existing CSV tests still pass + +--- + #### FU-P11-T1-1: Refactor `_FakeWebUIConfig` test stub to use `MagicMock(spec=WebUIConfig)` - **Description:** The hand-rolled `_FakeWebUIConfig` class in `tests/unit/test_main.py` must be manually updated every time a new property is added to `WebUIConfig`. Refactor it to use `MagicMock(spec=WebUIConfig)` with only the properties needed by the test wired up, so the spec auto-enforces the real interface and future property additions do not break the test. - **Priority:** P3 diff --git a/src/mcpbridge_wrapper/__main__.py b/src/mcpbridge_wrapper/__main__.py index ddecdd31..0ab2fc79 100644 --- a/src/mcpbridge_wrapper/__main__.py +++ b/src/mcpbridge_wrapper/__main__.py @@ -155,22 +155,35 @@ def _extract_request_id(line: str) -> Optional[str]: return None -def _has_error(line: str) -> bool: - """Check if a JSON-RPC response contains an error. +def _parse_error_info(line: str) -> Tuple[bool, Optional[int], Optional[str]]: + """Parse error status, code, and message from a JSON-RPC response line. Args: line: A line from the bridge output. Returns: - True if the line contains an error response. + Tuple of (is_error, error_code, error_message). """ try: from mcpbridge_wrapper.schemas import MCPResponse resp = MCPResponse.model_validate_json(line) - return resp.has_error() + return resp.has_error(), resp.get_error_code(), resp.get_error_message() except Exception: - return False + return False, None, None + + +def _has_error(line: str) -> bool: + """Check if a JSON-RPC response contains an error. + + Args: + line: A line from the bridge output. + + Returns: + True if the line contains an error response. + """ + is_error, _, _ = _parse_error_info(line) + return is_error def main() -> int: @@ -366,12 +379,14 @@ def signal_handler(signum: int, frame: object) -> None: # This is a response to a tracked request pending_tool_name, pending_start_time = pending_requests.pop(request_id) latency_ms = (time.time() - pending_start_time) * 1000.0 - is_error = _has_error(line) + is_error, error_code, error_message = _parse_error_info(line) metrics.record_response( pending_tool_name, request_id=request_id, error=is_error, latency_ms=latency_ms, + error_code=error_code, + error_message=error_message, ) if audit is not None: @@ -379,7 +394,8 @@ def signal_handler(signum: int, frame: object) -> None: tool_name=pending_tool_name, request_id=request_id, latency_ms=latency_ms, - error=str(is_error) if is_error else None, + error=error_message if is_error else None, + error_code=error_code if is_error else None, direction="response", ) diff --git a/src/mcpbridge_wrapper/schemas.py b/src/mcpbridge_wrapper/schemas.py index e3d395ff..b3d2ed7d 100644 --- a/src/mcpbridge_wrapper/schemas.py +++ b/src/mcpbridge_wrapper/schemas.py @@ -173,6 +173,22 @@ def has_error(self) -> bool: """ return self.error is not None + def get_error_code(self) -> Optional[int]: + """Return the JSON-RPC error code, or None if no error. + + Returns: + Error code integer, or None if no error present + """ + return self.error.code if self.error is not None else None + + def get_error_message(self) -> Optional[str]: + """Return the JSON-RPC error message, or None if no error. + + Returns: + Error message string, or None if no error present + """ + return self.error.message if self.error is not None else None + def parse_mcp_message(line: str) -> Optional[MCPRequest]: """Parse an MCP message from a JSON line. diff --git a/src/mcpbridge_wrapper/webui/audit.py b/src/mcpbridge_wrapper/webui/audit.py index 708136a1..cda3932d 100644 --- a/src/mcpbridge_wrapper/webui/audit.py +++ b/src/mcpbridge_wrapper/webui/audit.py @@ -190,6 +190,7 @@ def log( response_data: Optional[Dict[str, Any]] = None, latency_ms: Optional[float] = None, error: Optional[str] = None, + error_code: Optional[int] = None, direction: str = "request", ) -> None: """Log an audit entry for an MCP tool call. @@ -201,6 +202,7 @@ def log( response_data: Response payload (sanitized). latency_ms: Request latency in milliseconds. error: Error message if the call failed. + error_code: JSON-RPC error code if the call failed. direction: Either 'request' or 'response'. """ if not self._enabled: @@ -222,6 +224,8 @@ def log( entry["latency_ms"] = round(latency_ms, 2) if error is not None: entry["error"] = error + if error_code is not None: + entry["error_code"] = error_code with self._lock: # Write to file diff --git a/src/mcpbridge_wrapper/webui/metrics.py b/src/mcpbridge_wrapper/webui/metrics.py index 39447618..e8799129 100644 --- a/src/mcpbridge_wrapper/webui/metrics.py +++ b/src/mcpbridge_wrapper/webui/metrics.py @@ -11,6 +11,32 @@ from typing import Any, Deque, Dict, List, Optional, Tuple +def categorize_error(code: Optional[int]) -> str: + """Categorize a JSON-RPC error code into a severity bucket. + + Categories: + - "protocol": Standard JSON-RPC errors (-32600 to -32699) + - "timeout": Timeout indicator (-32001) + - "tool": Tool execution errors (positive codes >= 1) + - "unknown": All other codes or None + + Args: + code: The JSON-RPC error code, or None. + + Returns: + Category string: "protocol", "timeout", "tool", or "unknown". + """ + if code is None: + return "unknown" + if -32699 <= code <= -32600: + return "protocol" + if code == -32001: + return "timeout" + if code >= 1: + return "tool" + return "unknown" + + class MetricsCollector: """Thread-safe metrics collector for MCP tool call monitoring. @@ -55,6 +81,9 @@ def __init__(self, window_seconds: int = 3600, max_datapoints: int = 3600) -> No self._client_name: str = "unknown" self._client_version: str = "unknown" + # Error breakdown by code + self._error_counts_by_code: Dict[int, int] = {} + def set_client_info(self, name: str, version: str) -> None: """Record the connected MCP client identity. @@ -87,6 +116,8 @@ def record_response( request_id: Optional[str] = None, error: bool = False, latency_ms: Optional[float] = None, + error_code: Optional[int] = None, + error_message: Optional[str] = None, ) -> None: """Record a response for a tool call. @@ -96,6 +127,8 @@ def record_response( error: Whether the response indicates an error. latency_ms: Explicit latency in milliseconds. If not provided and request_id was tracked, latency is computed automatically. + error_code: JSON-RPC error code (if error=True). + error_message: JSON-RPC error message (if error=True). """ now = time.time() with self._lock: @@ -103,6 +136,10 @@ def record_response( self._total_errors += 1 self._tool_errors[tool_name] = self._tool_errors.get(tool_name, 0) + 1 self._error_times.append(now) + if error_code is not None: + self._error_counts_by_code[error_code] = ( + self._error_counts_by_code.get(error_code, 0) + 1 + ) # Remove from in-flight tracking and compute latency if needed if request_id is not None: @@ -200,6 +237,7 @@ def get_summary(self) -> Dict[str, Any]: "in_flight": len(self._in_flight), "client_name": self._client_name, "client_version": self._client_version, + "error_counts_by_code": dict(self._error_counts_by_code), } def get_timeseries(self, seconds: int = 300) -> Dict[str, Any]: @@ -239,3 +277,4 @@ def reset(self) -> None: self._in_flight.clear() self._client_name = "unknown" self._client_version = "unknown" + self._error_counts_by_code.clear() diff --git a/src/mcpbridge_wrapper/webui/shared_metrics.py b/src/mcpbridge_wrapper/webui/shared_metrics.py index a963f470..f001f151 100644 --- a/src/mcpbridge_wrapper/webui/shared_metrics.py +++ b/src/mcpbridge_wrapper/webui/shared_metrics.py @@ -5,6 +5,7 @@ storage that all processes can write to and read from. """ +import contextlib import sqlite3 import threading import time @@ -54,9 +55,15 @@ def _ensure_db(self) -> None: tool_name TEXT NOT NULL, timestamp REAL NOT NULL, latency_ms REAL, - error BOOLEAN DEFAULT 0 + error BOOLEAN DEFAULT 0, + error_code INTEGER, + error_message TEXT ) """) + # Add error_code and error_message columns to existing databases + for col, col_type in [("error_code", "INTEGER"), ("error_message", "TEXT")]: + with contextlib.suppress(Exception): + conn.execute(f"ALTER TABLE requests ADD COLUMN {col} {col_type}") # Create indexes conn.execute(""" CREATE INDEX IF NOT EXISTS idx_requests_tool ON requests(tool_name) @@ -104,6 +111,8 @@ def record_response( request_id: Optional[str] = None, error: bool = False, latency_ms: Optional[float] = None, + error_code: Optional[int] = None, + error_message: Optional[str] = None, ) -> None: """Record a response (updates the request record with latency/error). @@ -112,6 +121,8 @@ def record_response( request_id: Optional request ID to match with request. error: Whether the response was an error. latency_ms: Response latency in milliseconds. + error_code: JSON-RPC error code (if error=True). + error_message: JSON-RPC error message (if error=True). """ with self._transaction() as conn: if request_id: @@ -125,23 +136,35 @@ def record_response( if row: # Update existing request record conn.execute( - "UPDATE requests SET latency_ms = ?, error = ? WHERE id = ?", - (latency_ms, error, row["id"]), + """UPDATE requests + SET latency_ms = ?, error = ?, error_code = ?, error_message = ? + WHERE id = ?""", + (latency_ms, error, error_code, error_message, row["id"]), ) else: # Insert as new record if no matching request found conn.execute( """INSERT INTO requests - (request_id, tool_name, timestamp, latency_ms, error) - VALUES (?, ?, ?, ?, ?)""", - (request_id, tool_name, time.time(), latency_ms, error), + (request_id, tool_name, timestamp, latency_ms, error, + error_code, error_message) + VALUES (?, ?, ?, ?, ?, ?, ?)""", + ( + request_id, + tool_name, + time.time(), + latency_ms, + error, + error_code, + error_message, + ), ) else: # Insert as new record (no request_id matching) conn.execute( """INSERT INTO requests - (tool_name, timestamp, latency_ms, error) VALUES (?, ?, ?, ?)""", - (tool_name, time.time(), latency_ms, error), + (tool_name, timestamp, latency_ms, error, error_code, error_message) + VALUES (?, ?, ?, ?, ?, ?)""", + (tool_name, time.time(), latency_ms, error, error_code, error_message), ) def set_client_info(self, name: str, version: str) -> None: @@ -221,6 +244,17 @@ def get_summary(self, window_seconds: int = 3600) -> Dict[str, Any]: ).fetchone() rps = (row[0] or 0) / 60.0 + # Error breakdown by code + error_counts_by_code: Dict[int, int] = {} + err_cursor = conn.execute( + """SELECT error_code, COUNT(*) as cnt FROM requests + WHERE timestamp > ? AND error = 1 AND error_code IS NOT NULL + GROUP BY error_code""", + (cutoff,), + ) + for err_row in err_cursor: + error_counts_by_code[err_row["error_code"]] = err_row["cnt"] + # Client identification client_row = conn.execute( "SELECT client_name, client_version FROM client_info WHERE id = 1" @@ -240,6 +274,7 @@ def get_summary(self, window_seconds: int = 3600) -> Dict[str, Any]: "in_flight": 0, # Can't track across processes easily "client_name": client_name, "client_version": client_version, + "error_counts_by_code": error_counts_by_code, } def get_timeseries(self, seconds: int = 300) -> Dict[str, List[Dict[str, Any]]]: diff --git a/src/mcpbridge_wrapper/webui/static/dashboard.css b/src/mcpbridge_wrapper/webui/static/dashboard.css index ba242f52..15590dc0 100644 --- a/src/mcpbridge_wrapper/webui/static/dashboard.css +++ b/src/mcpbridge_wrapper/webui/static/dashboard.css @@ -275,6 +275,33 @@ tbody tr:hover { font-weight: 500; } +/* Error severity color coding */ +.error-protocol { + color: #e53935; + font-weight: 600; +} +.error-tool { + color: #f57c00; + font-weight: 500; +} +.error-timeout { + color: #f9a825; + font-weight: 500; +} +.error-unknown { + color: #757575; + font-weight: 400; +} + +/* Empty state for error breakdown chart */ +.chart-empty-msg { + display: none; + text-align: center; + color: var(--text-secondary); + font-size: 0.85rem; + padding: 16px 0; +} + footer { text-align: center; padding: 16px; diff --git a/src/mcpbridge_wrapper/webui/static/dashboard.js b/src/mcpbridge_wrapper/webui/static/dashboard.js index f39f1600..b89291ea 100644 --- a/src/mcpbridge_wrapper/webui/static/dashboard.js +++ b/src/mcpbridge_wrapper/webui/static/dashboard.js @@ -102,6 +102,23 @@ }, }); + // Error breakdown doughnut chart + charts.errorBreakdown = new Chart(el("chart-error-breakdown"), { + type: "doughnut", + data: { + labels: ["Protocol", "Tool", "Timeout", "Unknown"], + datasets: [{ + data: [0, 0, 0, 0], + backgroundColor: ["#e53935", "#f57c00", "#f9a825", "#757575"], + }], + }, + options: { + responsive: true, + maintainAspectRatio: false, + plugins: { legend: { position: "right", labels: { boxWidth: 12 } } }, + }, + }); + // Request timeline charts.timeline = new Chart(el("chart-timeline"), { type: "line", @@ -202,6 +219,38 @@ charts.toolPie.update("none"); } + function categorizeError(code) { + if (code === null || code === undefined) return "unknown"; + if (code >= -32699 && code <= -32600) return "protocol"; + if (code === -32001) return "timeout"; + if (code >= 1) return "tool"; + return "unknown"; + } + + function updateErrorBreakdownChart(errorCountsByCode) { + var counts = { protocol: 0, tool: 0, timeout: 0, unknown: 0 }; + var total = 0; + Object.keys(errorCountsByCode || {}).forEach(function (codeStr) { + var code = parseInt(codeStr, 10); + var cat = categorizeError(code); + counts[cat] += errorCountsByCode[codeStr]; + total += errorCountsByCode[codeStr]; + }); + + var emptyEl = el("error-breakdown-empty"); + if (total === 0) { + el("chart-error-breakdown").style.display = "none"; + if (emptyEl) emptyEl.style.display = "block"; + } else { + el("chart-error-breakdown").style.display = ""; + if (emptyEl) emptyEl.style.display = "none"; + charts.errorBreakdown.data.datasets[0].data = [ + counts.protocol, counts.tool, counts.timeout, counts.unknown, + ]; + charts.errorBreakdown.update("none"); + } + } + function bucketTimeseries(points, bucketSize) { // Bucket points into time intervals and count per bucket if (!points.length) return { labels: [], data: [] }; @@ -268,6 +317,7 @@ function handleMetricsUpdate(data) { updateKPIs(data.summary); updateToolCharts(data.summary.tool_counts); + updateErrorBreakdownChart(data.summary.error_counts_by_code || {}); updateLatencyTable(data.summary.tool_latency); updateTimeline(data.timeseries); updateLatencyChart(data.timeseries); @@ -349,13 +399,17 @@ var tr = document.createElement("tr"); tr.className = "audit-row"; var requestId = e.request_id || ""; - var errClass = e.error ? ' class="error-cell"' : ""; + var errSeverityClass = ""; + if (e.error) { + var errCat = categorizeError(e.error_code != null ? e.error_code : null); + errSeverityClass = ' class="error-' + errCat + '"'; + } tr.innerHTML = "" + escapeHtml(e.timestamp_iso || "") + "" + "" + escapeHtml(e.tool || "") + "" + "" + escapeHtml(e.direction || "") + "" + "" + escapeHtml(requestId || "-") + "" + "" + (e.latency_ms != null ? e.latency_ms.toFixed(1) : "-") + "" - + "" + escapeHtml(e.error || "-") + ""; + + "" + escapeHtml(e.error || "-") + ""; tr.addEventListener("click", function () { toggleDetailRow(tr, requestId); }); diff --git a/src/mcpbridge_wrapper/webui/static/index.html b/src/mcpbridge_wrapper/webui/static/index.html index eecac0ea..f3b4e2f7 100644 --- a/src/mcpbridge_wrapper/webui/static/index.html +++ b/src/mcpbridge_wrapper/webui/static/index.html @@ -60,6 +60,11 @@

Tool Usage (Bar)

Tool Distribution (Pie)

+
+

Error Breakdown

+ +
No errors recorded
+
diff --git a/tests/unit/test_main.py b/tests/unit/test_main.py index b8270658..ec1ee70c 100644 --- a/tests/unit/test_main.py +++ b/tests/unit/test_main.py @@ -661,3 +661,46 @@ def capture_on_request(bridge, on_request=None): assert len(captured_on_request) == 1 captured_on_request[0](initialize_line) assert ("unknown", "unknown") in captured_calls + + +class TestParseErrorInfo: + """Tests for _parse_error_info helper.""" + + def test_no_error_response(self): + from mcpbridge_wrapper.__main__ import _parse_error_info + + line = '{"jsonrpc":"2.0","id":1,"result":{"content":[]}}\n' + is_error, code, message = _parse_error_info(line) + assert is_error is False + assert code is None + assert message is None + + def test_error_response_with_code_and_message(self): + from mcpbridge_wrapper.__main__ import _parse_error_info + + line = '{"jsonrpc":"2.0","id":1,"error":{"code":-32600,"message":"Invalid Request"}}\n' + is_error, code, message = _parse_error_info(line) + assert is_error is True + assert code == -32600 + assert message == "Invalid Request" + + def test_invalid_json_returns_no_error(self): + from mcpbridge_wrapper.__main__ import _parse_error_info + + line = "not valid json\n" + is_error, code, message = _parse_error_info(line) + assert is_error is False + assert code is None + assert message is None + + def test_has_error_delegates_to_parse_error_info(self): + from mcpbridge_wrapper.__main__ import _has_error + + line = '{"jsonrpc":"2.0","id":1,"error":{"code":-32601,"message":"Method not found"}}\n' + assert _has_error(line) is True + + def test_has_error_false_for_success(self): + from mcpbridge_wrapper.__main__ import _has_error + + line = '{"jsonrpc":"2.0","id":1,"result":{}}\n' + assert _has_error(line) is False diff --git a/tests/unit/webui/test_metrics.py b/tests/unit/webui/test_metrics.py index 9c6800c9..0dec2cff 100644 --- a/tests/unit/webui/test_metrics.py +++ b/tests/unit/webui/test_metrics.py @@ -2,7 +2,7 @@ from unittest.mock import patch -from mcpbridge_wrapper.webui.metrics import MetricsCollector +from mcpbridge_wrapper.webui.metrics import MetricsCollector, categorize_error class TestMetricsCollector: @@ -235,3 +235,85 @@ def test_reset_clears_client_info(self): summary = metrics.get_summary() assert summary["client_name"] == "unknown" assert summary["client_version"] == "unknown" + + def test_error_counts_by_code_in_summary(self): + """Test that error_counts_by_code appears in summary.""" + metrics = MetricsCollector() + summary = metrics.get_summary() + assert "error_counts_by_code" in summary + assert summary["error_counts_by_code"] == {} + + def test_record_response_with_error_code(self): + """Test that error_code is tracked per code value.""" + metrics = MetricsCollector() + metrics.record_request("XcodeRead", request_id="1") + metrics.record_response( + "XcodeRead", + request_id="1", + error=True, + error_code=-32600, + error_message="Invalid Request", + ) + summary = metrics.get_summary() + assert summary["error_counts_by_code"] == {-32600: 1} + + def test_record_response_multiple_error_codes(self): + """Test that multiple different error codes are all tracked.""" + metrics = MetricsCollector() + metrics.record_request("XcodeRead", request_id="1") + metrics.record_response("XcodeRead", request_id="1", error=True, error_code=-32600) + metrics.record_request("XcodeWrite", request_id="2") + metrics.record_response("XcodeWrite", request_id="2", error=True, error_code=-32601) + metrics.record_request("XcodeRead", request_id="3") + metrics.record_response("XcodeRead", request_id="3", error=True, error_code=-32600) + summary = metrics.get_summary() + assert summary["error_counts_by_code"][-32600] == 2 + assert summary["error_counts_by_code"][-32601] == 1 + + def test_record_response_error_no_code(self): + """Test that error without code does not affect error_counts_by_code.""" + metrics = MetricsCollector() + metrics.record_request("XcodeRead", request_id="1") + metrics.record_response("XcodeRead", request_id="1", error=True) + summary = metrics.get_summary() + assert summary["error_counts_by_code"] == {} + + def test_reset_clears_error_counts(self): + """Test that reset() clears error_counts_by_code.""" + metrics = MetricsCollector() + metrics.record_request("XcodeRead", request_id="1") + metrics.record_response("XcodeRead", request_id="1", error=True, error_code=-32600) + metrics.reset() + summary = metrics.get_summary() + assert summary["error_counts_by_code"] == {} + + +class TestCategorizeError: + """Tests for the categorize_error helper function.""" + + def test_protocol_error_lower_bound(self): + assert categorize_error(-32699) == "protocol" + + def test_protocol_error_upper_bound(self): + assert categorize_error(-32600) == "protocol" + + def test_protocol_error_middle(self): + assert categorize_error(-32650) == "protocol" + + def test_timeout_error(self): + assert categorize_error(-32001) == "timeout" + + def test_tool_error_small_positive(self): + assert categorize_error(1) == "tool" + + def test_tool_error_large_positive(self): + assert categorize_error(9999) == "tool" + + def test_unknown_none(self): + assert categorize_error(None) == "unknown" + + def test_unknown_negative_not_protocol(self): + assert categorize_error(-1) == "unknown" + + def test_unknown_zero(self): + assert categorize_error(0) == "unknown" diff --git a/tests/unit/webui/test_shared_metrics.py b/tests/unit/webui/test_shared_metrics.py index 000b808b..39d3d6e4 100644 --- a/tests/unit/webui/test_shared_metrics.py +++ b/tests/unit/webui/test_shared_metrics.py @@ -213,3 +213,52 @@ def test_reset_clears_client_info(self, store): summary = store.get_summary() assert summary["client_name"] == "unknown" assert summary["client_version"] == "unknown" + + def test_error_counts_by_code_empty_by_default(self, store): + """Test that error_counts_by_code is empty when no errors recorded.""" + summary = store.get_summary() + assert "error_counts_by_code" in summary + assert summary["error_counts_by_code"] == {} + + def test_record_response_with_error_code(self, store): + """Test that error_code is stored and aggregated in get_summary.""" + store.record_request("BuildProject", request_id="1") + store.record_response( + "BuildProject", + request_id="1", + error=True, + latency_ms=50.0, + error_code=-32600, + error_message="Invalid Request", + ) + summary = store.get_summary() + assert summary["error_counts_by_code"] == {-32600: 1} + + def test_record_response_multiple_error_codes(self, store): + """Test that multiple error codes are aggregated correctly.""" + store.record_request("BuildProject", request_id="1") + store.record_response( + "BuildProject", request_id="1", error=True, latency_ms=50.0, error_code=-32600 + ) + store.record_request("OpenFile", request_id="2") + store.record_response( + "OpenFile", request_id="2", error=True, latency_ms=30.0, error_code=-32600 + ) + store.record_request("RunTest", request_id="3") + store.record_response("RunTest", request_id="3", error=True, latency_ms=20.0, error_code=1) + summary = store.get_summary() + assert summary["error_counts_by_code"][-32600] == 2 + assert summary["error_counts_by_code"][1] == 1 + + def test_record_response_error_without_code(self, store): + """Test that errors without error_code don't appear in error_counts_by_code.""" + store.record_request("BuildProject", request_id="1") + store.record_response("BuildProject", request_id="1", error=True, latency_ms=50.0) + summary = store.get_summary() + assert summary["error_counts_by_code"] == {} + + def test_record_response_error_code_no_request_id(self, store): + """Test recording error_code without request_id (insert path).""" + store.record_response("BuildProject", error=True, latency_ms=50.0, error_code=-32001) + summary = store.get_summary() + assert summary["error_counts_by_code"] == {-32001: 1}