-
Notifications
You must be signed in to change notification settings - Fork 495
Fix Go Logger safe-output accounting and failure diagnostics #52661
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
403eada
030e251
8fb3691
8ca697f
7d2dfb6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -845,7 +845,7 @@ func extractGHErrorLines(filePath string) []string { | |
| for line := range strings.SplitSeq(string(content), "\n") { | ||
| if strings.Contains(line, "##[error]") { | ||
| stripped := stripGHALogTimestamps(line) | ||
| if stripped != "" { | ||
| if stripped != "" && !isAgentToolResultAnnotation(stripped) { | ||
| errorLines = append(errorLines, stripped) | ||
| } | ||
| } | ||
|
|
@@ -854,6 +854,33 @@ func extractGHErrorLines(filePath string) []string { | |
| return errorLines | ||
| } | ||
|
|
||
| func isAgentToolResultAnnotation(line string) bool { | ||
| _, payload, found := strings.Cut(line, "##[error]") | ||
| if !found { | ||
| return false | ||
| } | ||
|
|
||
| var event struct { | ||
| Type string `json:"type"` | ||
| Message struct { | ||
| Content []struct { | ||
| Type string `json:"type"` | ||
| } `json:"content"` | ||
| } `json:"message"` | ||
| } | ||
| if err := json.Unmarshal([]byte(strings.TrimSpace(payload)), &event); err != nil || event.Type != "user" { | ||
| return false | ||
| } | ||
| hasToolResult := false | ||
| for _, content := range event.Message.Content { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/diagnosing-bugs] If a 💡 Suggested guardReturn 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. |
||
| if content.Type != "tool_result" { | ||
| return false | ||
| } | ||
| hasToolResult = true | ||
| } | ||
| return hasToolResult | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. L857-880: yagni: full JSON struct decode (nested Type/Message/Content) just to check for a |
||
|
|
||
| func extractAgentFailureError(agentRan bool, agentStdioPath string, maxMessageLen int) []ValidationIssue { | ||
| if !agentRan { | ||
| return nil | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/tdd] The new test case sets
wantServers: 1for a log that contains onlytools/listevents, 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
wantServersis 1 because theServerNamefield in atools/listentry 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.