Fix Go Logger safe-output accounting and failure diagnostics#52661
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #52661 does not have the 'implementation' label and has 85 new lines of code in business logic directories (threshold: 100).
|
|
✅ Ponytail Reviewer completed successfully!
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Test Quality Sentinel completed test quality analysis.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
I did not find a blocking correctness, security, or performance regression in the changed lines.
The patch tightens MCP tool accounting, filters noisy tool-result annotations from error extraction, and adds regression tests for both behaviors plus the workflow contract text. I looked for changed-line breakage around undercounting real tool calls, masking actionable errors, and test gaps; the updated conditions and coverage are consistent with the stated intent.
No actionable review comments to add.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 3.48 AIC · ⌖ 7.36 AIC · ⊞ 6.5K
Comment /review to run again
There was a problem hiding this comment.
The fixes are correct and well-tested. Gateway log accounting correctly excludes tools/list protocol discovery. Error line filtering prevents tool-result blobs from surfacing as errors. All test additions directly cover the fixed code paths.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 20.8 AIC · ⌖ 7 AIC · ⊞ 5.4K
There was a problem hiding this comment.
Pull request overview
Fixes Go Logger safe-output accounting, failure diagnostics, and terminal-action guidance.
Changes:
- Excludes protocol discovery from tool-call metrics.
- Filters Claude
tool_resulterror annotations. - Enforces one terminal safe-output action in Go Logger.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/go-logger.md |
Adds terminal-action guardrails. |
.github/workflows/go-logger.lock.yml |
Updates generated workflow metadata. |
pkg/cli/audit_mcp_tool_usage_test.go |
Tests discovery-only traffic. |
pkg/cli/audit_report.go |
Filters tool-result annotations. |
pkg/cli/audit_report_test.go |
Tests actionable error preservation. |
pkg/cli/gateway_logs_mcp.go |
Restricts MCP usage extraction. |
pkg/cli/gateway_logs_parsing.go |
Corrects tool-call accounting. |
pkg/cli/gateway_logs_test.go |
Updates discovery accounting assertions. |
pkg/workflow/prompts_test.go |
Verifies workflow guardrails. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Balanced
| // Track tool calls | ||
| if entry.ToolName != "" || entry.Method != "" { | ||
| // Track only actual tool invocations, not protocol requests such as tools/list. | ||
| if entry.Event == "tool_call" || entry.Method == "tools/call" { |
Test Quality Sentinel Report 🧪PR #52661 — "Fix Go Logger safe-output accounting and failure diagnostics" Key Metrics
Test Breakdown5 Tests Analyzed (all design contract tests)
Quality Strengths✅ All 5 tests enforce design invariants — not implementation details:
✅ 100% edge case / error path coverage — every test includes boundary or error assertions ✅ No code violations:
✅ Healthy test-to-production ratio (1.83:1, within 2:1 threshold) ✅ Clear failure context — all assertions use VerdictAPPROVED ✅ — Test Quality Sentinel: 100/100. 0% implementation tests (threshold: 30%). No violations. All tests enforce strong design contracts around tool usage accounting, error filtering, and safe-output policy compliance.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — the fixes are well-targeted; leaving a few improvement suggestions.
📋 Key Themes & Highlights
Key Themes
- Suppression over-breadth (
audit_report.go):isAgentToolResultAnnotationreturnstrueif any content item is atool_result, meaning mixed-content messages with a real error text are still silently dropped. - Brittle contract test (
prompts_test.go): Asserting exact prose strings couples the test to formatting rather than semantic intent; any rewording breaks it. - Server-counter asymmetry undocumented (
audit_mcp_tool_usage_test.go): Discovery-only traffic filters tool calls but still registers a server entry — the test asserts this but doesn't explain why.
Positive Highlights
- ✅ Root-cause fix in both
gateway_logs_mcp.goandgateway_logs_parsing.gois consistent and correct. - ✅ Regression test for annotation filtering is well-structured with a real-world timeout message.
- ✅ Workflow guardrails are clear, actionable, and directly address the reported failure mode.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 42.4 AIC · ⌖ 8.48 AIC · ⊞ 7K
Comment /matt to run again
| if err := json.Unmarshal([]byte(strings.TrimSpace(payload)), &event); err != nil || event.Type != "user" { | ||
| return false | ||
| } | ||
| for _, content := range event.Message.Content { |
There was a problem hiding this comment.
[/diagnosing-bugs] If a ##[error] line contains mixed content — a tool_result alongside another content type (e.g. "type":"text") — the entire line is silently suppressed, hiding a real error.
💡 Suggested guard
Return true only when all content items are tool_result, not when any one of them is:
hasToolResult := false
hasOtherContent := false
for _, c := range event.Message.Content {
if c.Type == "tool_result" {
hasToolResult = true
} else {
hasOtherContent = true
}
}
return hasToolResult && !hasOtherContentThis keeps suppression for pure annotation messages while surfacing mixed content that includes real error text.
@copilot please address this.
| logContent: `{"timestamp":"2024-01-12T10:00:00Z","level":"info","type":"request","event":"rpc_call","server_name":"safeoutputs","method":"tools/list","duration":50.0,"status":"success"} | ||
| {"timestamp":"2024-01-12T10:00:01Z","level":"info","type":"request","event":"request","server_name":"safeoutputs","method":"tools/list","duration":50.0,"status":"success"} | ||
| `, | ||
| wantServers: 1, |
There was a problem hiding this comment.
[/tdd] The new test case sets wantServers: 1 for a log that contains only tools/list events, but after the fix, discovery-only traffic should arguably not register a server at all — or the test should document why a server entry is still expected.
💡 Detail
If wantServers is 1 because the ServerName field in a tools/list entry still creates a server bucket, the test is correct but the assertion silently documents that the server counter is not subject to the same filtering as tool calls. Consider adding a comment explaining this intentional asymmetry, or reconsider whether discovery-only traffic should increment the server counter.
@copilot please address this.
| } | ||
|
|
||
| workflow := string(content) | ||
| for _, guidance := range []string{ |
There was a problem hiding this comment.
[/tdd] This test is brittle: it asserts exact prose strings from the workflow file. Any whitespace edit, rewording, or reflow of those sentences will cause the test to fail without a real contract violation.
💡 Suggested approach
Extract the semantic intent into constants or a helper and check for the key tokens rather than the full sentence:
checks := []struct{ name, keyword string }{
{"single terminal outcome", "exactly one terminal outcome"},
{"no probing", "Do not probe safe outputs"},
{"no retry after rejection", "stop immediately and surface its exact rejection message"},
}
for _, c := range checks {
require.Contains(t, workflow, c.keyword, "go-logger workflow missing contract: %s", c.name)
}This keeps the intent assertion stable across minor prose edits.
@copilot please address this.
There was a problem hiding this comment.
Generated by ✂️ Ponytail Reviewer for #52661 · auto · 32.2 AIC · ⌖ 4.34 AIC · ⊞ 6.8K
Comment /ponytail to run again
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
L857-880: yagni: full JSON struct decode (nested Type/Message/Content) just to check for a tool_result content type. strings.Contains(payload, "tool_result") gets the same filtering result in ~2 lines, no struct needed.
|
@copilot Unresolved review threads (newest first):
Failed checks:
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>
PR Triage\n\n- Category: bug\n- Risk: medium\n- Priority: medium\n- Score: 60/100 (impact 35 + urgency 15 + quality 10)\n- Recommended action:
|
PR Triage
Automated triage by PR Triage Agent.
|
Go Logger runs misclassified safe-output discovery as repeated tool execution, while raw Claude
tool_resultannotations obscured the actionable timeout. The workflow also lacked an explicit single-terminal-action contract.Tool accounting
tool_callevents andtools/callrequests.tools/list.Failure reporting
tool_resultpayloads.Workflow guardrails
create_pull_request,noop, orreport_incomplete.Regression 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 · ◷