Embed AggregatedSummaryBase in MCPFailureSummary#52670
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Great work on this refactor! 🚀 You have successfully eliminated the duplicated aggregation fields in What stands out:
This is exactly the kind of mechanical, low-risk refactoring that keeps the codebase maintainable. The PR is ready for review and merge.
|
There was a problem hiding this comment.
Pull request overview
Refactors MCP failure summaries to embed the shared aggregation model and reduce field duplication.
Changes:
- Embeds
AggregatedSummaryBaseinMCPFailureSummary. - Updates aggregation and test fixtures for embedded initialization.
- Adds flattened JSON serialization coverage.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/logs_models.go |
Refactors the summary model. |
pkg/cli/logs_report_mcp.go |
Initializes the embedded base. |
pkg/cli/logs_report_test.go |
Updates report fixtures. |
pkg/cli/logs_display_fields_test.go |
Updates console checks and adds JSON coverage. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Balanced
| WorkflowsDisplay string `json:"-" console:"header:Workflows,maxlen:60"` // Formatted display of workflows | ||
| RunIDs []int64 `json:"run_ids" console:"-"` // List of run IDs where this server failed | ||
| ServerName string `json:"server_name" console:"header:Server"` | ||
| AggregatedSummaryBase |
| // AggregatedSummaryBase holds the shared tail fields that appear byte-for-byte identically | ||
| // in MissingToolSummary and MissingDataSummary (and as a subset in MCPFailureSummary). | ||
| // in MissingToolSummary, MissingDataSummary, and MCPFailureSummary. |
|
@copilot Unresolved review threads (newest first):
Please refresh the branch if needed, address the unresolved review feedback, rerun validation, and use the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the unresolved feedback in |
PR Triage\n\n- Category: refactor\n- Risk: low\n- Priority: medium\n- Score: 50/100 (impact 25 + urgency 15 + quality 10)\n- Recommended action:
|
PR Triage
Automated triage by PR Triage Agent.
|
MCPFailureSummaryduplicated aggregation fields already defined inAggregatedSummaryBase, creating drift risk when shared summary fields change. This refactor aligns it with the existingMissingToolSummaryandMissingDataSummarypattern.Model refactor
AggregatedSummaryBaseinMCPFailureSummaryCount,Workflows,WorkflowsDisplay, andRunIDsfieldsAggregation update
Coverage
Run: https://github.com/github/gh-aw/actions/runs/31795158705> Generated by 👨🍳 PR Sous Chef · gpt54 · 9.47 AIC · ⌖ 7.2 AIC · ⊞ 8.5K · ◷