Extract DiffEntryBase embed to reduce anomaly-field repetition in audit_diff.go #46972
+49
−46
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,19 +20,24 @@ var auditDiffLog = logger.New("cli:audit_diff") | |
| // >100% increase means the request count more than doubled. | ||
| const volumeChangeThresholdPercent = 100.0 | ||
|
|
||
| // DiffEntryBase holds common anomaly-flagging fields shared by all diff entry types. | ||
| type DiffEntryBase struct { | ||
| Status string `json:"status"` | ||
| IsAnomaly bool `json:"is_anomaly,omitempty"` // Flagged as anomalous | ||
| AnomalyNote string `json:"anomaly_note,omitempty"` // Human-readable anomaly explanation | ||
| } | ||
|
|
||
| // DomainDiffEntry represents the diff for a single domain between two runs | ||
| type DomainDiffEntry struct { | ||
| Domain string `json:"domain"` | ||
| DiffEntryBase | ||
|
Comment on lines
+32
to
+33
Contributor
There was a problem hiding this comment. [/codebase-design] The PR says "wire format is unchanged" but the JSON field order has shifted: 💡 SuggestionClarify in the PR body that field order changed, and verify no existing snapshot tests compare raw JSON strings. If strict trailing order is needed, move the embed to the end of each struct. @copilot please address this. |
||
| Run1Allowed int `json:"run1_allowed"` // Allowed requests in run 1 | ||
| Run1Blocked int `json:"run1_blocked"` // Blocked requests in run 1 | ||
| Run2Allowed int `json:"run2_allowed"` // Allowed requests in run 2 | ||
| Run2Blocked int `json:"run2_blocked"` // Blocked requests in run 2 | ||
| Run1Status string `json:"run1_status,omitempty"` // "allowed", "denied", or "" for new domains | ||
| Run2Status string `json:"run2_status,omitempty"` // "allowed", "denied", or "" for removed domains | ||
| VolumeChange string `json:"volume_change,omitempty"` // e.g. "+287%" or "-50%" | ||
| } | ||
|
|
||
| // FirewallDiff represents the complete diff between two runs' firewall behavior | ||
|
|
@@ -103,11 +108,11 @@ func computeFirewallDiff(run1ID, run2ID int64, run1, run2 *FirewallAnalysis) *Fi | |
| if !inRun1 && inRun2 { | ||
| // New domain in run 2 | ||
| entry := DomainDiffEntry{ | ||
| Domain: domain, | ||
| DiffEntryBase: DiffEntryBase{Status: "new"}, | ||
| Run2Allowed: stats2.Allowed, | ||
| Run2Blocked: stats2.Blocked, | ||
| Run2Status: classifyFirewallDomainStatus(stats2), | ||
| } | ||
| // Anomaly: new denied domain | ||
| if stats2.Blocked > 0 { | ||
|
|
@@ -119,11 +124,11 @@ func computeFirewallDiff(run1ID, run2ID int64, run1, run2 *FirewallAnalysis) *Fi | |
| } else if inRun1 && !inRun2 { | ||
| // Removed domain | ||
| entry := DomainDiffEntry{ | ||
| Domain: domain, | ||
| DiffEntryBase: DiffEntryBase{Status: "removed"}, | ||
| Run1Allowed: stats1.Allowed, | ||
| Run1Blocked: stats1.Blocked, | ||
| Run1Status: classifyFirewallDomainStatus(stats1), | ||
| } | ||
| // Anomaly: the removed domain was denied in the base run. This indicates a | ||
| // transient firewall block that prevented the agent from reaching an MCP server | ||
|
|
@@ -144,14 +149,14 @@ func computeFirewallDiff(run1ID, run2ID int64, run1, run2 *FirewallAnalysis) *Fi | |
| if status1 != status2 { | ||
| // Status changed | ||
| entry := DomainDiffEntry{ | ||
| Domain: domain, | ||
| DiffEntryBase: DiffEntryBase{Status: "status_changed"}, | ||
| Run1Allowed: stats1.Allowed, | ||
| Run1Blocked: stats1.Blocked, | ||
| Run2Allowed: stats2.Allowed, | ||
| Run2Blocked: stats2.Blocked, | ||
| Run1Status: status1, | ||
| Run2Status: status2, | ||
| } | ||
| // Anomaly: previously denied, now allowed | ||
| if status1 == "denied" && status2 == "allowed" { | ||
|
|
@@ -175,15 +180,15 @@ func computeFirewallDiff(run1ID, run2ID int64, run1, run2 *FirewallAnalysis) *Fi | |
| pctChange := (float64(total2-total1) / float64(total1)) * 100 | ||
| if math.Abs(pctChange) > volumeChangeThresholdPercent { | ||
| entry := DomainDiffEntry{ | ||
| Domain: domain, | ||
| DiffEntryBase: DiffEntryBase{Status: "volume_changed"}, | ||
| Run1Allowed: stats1.Allowed, | ||
| Run1Blocked: stats1.Blocked, | ||
| Run2Allowed: stats2.Allowed, | ||
| Run2Blocked: stats2.Blocked, | ||
| Run1Status: status1, | ||
| Run2Status: status2, | ||
| VolumeChange: formatVolumeChange(total1, total2), | ||
| } | ||
| diff.VolumeChanges = append(diff.VolumeChanges, entry) | ||
| } | ||
|
|
@@ -222,16 +227,14 @@ func classifyFirewallDomainStatus(stats DomainRequestStats) string { | |
|
|
||
| // MCPToolDiffEntry represents the diff for a single MCP tool between two runs | ||
| type MCPToolDiffEntry struct { | ||
| ServerName string `json:"server_name"` | ||
| ToolName string `json:"tool_name"` | ||
| DiffEntryBase | ||
| Run1CallCount int `json:"run1_call_count,omitempty"` // Call count in run 1 | ||
| Run2CallCount int `json:"run2_call_count,omitempty"` // Call count in run 2 | ||
| Run1ErrorCount int `json:"run1_error_count,omitempty"` | ||
| Run2ErrorCount int `json:"run2_error_count,omitempty"` | ||
| CallCountChange string `json:"call_count_change,omitempty"` // e.g. "+2", "-3" | ||
| } | ||
|
|
||
| // MCPToolsDiff represents the complete diff of MCP tool invocations between two runs | ||
|
|
@@ -280,8 +283,8 @@ type TokenUsageDiff struct { | |
| // ToolCallDiffEntry represents the diff for a single engine-level tool between two runs. | ||
| // Tool data comes from RunSummary.Metrics.ToolCalls (LogMetrics.ToolCalls). | ||
| type ToolCallDiffEntry struct { | ||
| Name string `json:"name"` | ||
| DiffEntryBase | ||
| Run1CallCount int `json:"run1_call_count"` // Call count in run 1 (0 if new) | ||
| Run2CallCount int `json:"run2_call_count"` // Call count in run 2 (0 if removed) | ||
| CallCountChange string `json:"call_count_change,omitempty"` // e.g. "+3", "-1" | ||
|
|
@@ -453,7 +456,7 @@ func computeMCPToolsDiff(run1, run2 *MCPToolUsageData) *MCPToolsDiff { | |
| entry := MCPToolDiffEntry{ | ||
| ServerName: s2.ServerName, | ||
| ToolName: s2.ToolName, | ||
| DiffEntryBase: DiffEntryBase{Status: "new"}, | ||
| Run2CallCount: s2.CallCount, | ||
| Run2ErrorCount: s2.ErrorCount, | ||
| } | ||
|
|
@@ -467,15 +470,15 @@ func computeMCPToolsDiff(run1, run2 *MCPToolUsageData) *MCPToolsDiff { | |
| diff.RemovedTools = append(diff.RemovedTools, MCPToolDiffEntry{ | ||
| ServerName: s1.ServerName, | ||
| ToolName: s1.ToolName, | ||
| DiffEntryBase: DiffEntryBase{Status: "removed"}, | ||
| Run1CallCount: s1.CallCount, | ||
| Run1ErrorCount: s1.ErrorCount, | ||
| }) | ||
| } else if s1.CallCount != s2.CallCount || s1.ErrorCount != s2.ErrorCount { | ||
| entry := MCPToolDiffEntry{ | ||
| ServerName: s1.ServerName, | ||
| ToolName: s1.ToolName, | ||
| DiffEntryBase: DiffEntryBase{Status: "changed"}, | ||
| Run1CallCount: s1.CallCount, | ||
| Run2CallCount: s2.CallCount, | ||
| Run1ErrorCount: s1.ErrorCount, | ||
|
|
@@ -679,7 +682,7 @@ func computeToolCallsDiff(m1, m2 *LogMetrics) *ToolCallsDiff { | |
| case !inRun1 && inRun2: | ||
| entry = ToolCallDiffEntry{ | ||
| Name: name, | ||
| DiffEntryBase: DiffEntryBase{Status: "new"}, | ||
| Run2CallCount: tc2.CallCount, | ||
| Run2MaxInputSize: tc2.MaxInputSize, | ||
| Run2MaxOutputSize: tc2.MaxOutputSize, | ||
|
|
@@ -688,7 +691,7 @@ func computeToolCallsDiff(m1, m2 *LogMetrics) *ToolCallsDiff { | |
| case inRun1 && !inRun2: | ||
| entry = ToolCallDiffEntry{ | ||
| Name: name, | ||
| DiffEntryBase: DiffEntryBase{Status: "removed"}, | ||
| Run1CallCount: tc1.CallCount, | ||
| Run1MaxInputSize: tc1.MaxInputSize, | ||
| Run1MaxOutputSize: tc1.MaxOutputSize, | ||
|
|
@@ -697,7 +700,7 @@ func computeToolCallsDiff(m1, m2 *LogMetrics) *ToolCallsDiff { | |
| case tc1.CallCount != tc2.CallCount: | ||
| entry = ToolCallDiffEntry{ | ||
| Name: name, | ||
| DiffEntryBase: DiffEntryBase{Status: "changed"}, | ||
| Run1CallCount: tc1.CallCount, | ||
| Run2CallCount: tc2.CallCount, | ||
| CallCountChange: formatCountChange(tc1.CallCount, tc2.CallCount), | ||
|
|
@@ -710,7 +713,7 @@ func computeToolCallsDiff(m1, m2 *LogMetrics) *ToolCallsDiff { | |
| default: | ||
| entry = ToolCallDiffEntry{ | ||
| Name: name, | ||
| DiffEntryBase: DiffEntryBase{Status: "unchanged"}, | ||
| Run1CallCount: tc1.CallCount, | ||
| Run2CallCount: tc2.CallCount, | ||
| Run1MaxInputSize: tc1.MaxInputSize, | ||
|
|
@@ -775,7 +778,7 @@ func computeBashCommandsDiff(run1Tools, run2Tools map[string]ToolCallInfo) *Bash | |
|
|
||
| cmd := ToolCallDiffEntry{ | ||
| Name: name, | ||
| DiffEntryBase: DiffEntryBase{Status: status}, | ||
| Run1CallCount: tc1.CallCount, | ||
| Run2CallCount: tc2.CallCount, | ||
| Run1MaxInputSize: tc1.MaxInputSize, | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
[/codebase-design] The valid status values per entry type were documented as inline comments on the old
Statusfields (e.g.// "new", "removed", "status_changed", "volume_changed"onDomainDiffEntry) but were dropped during extraction —DiffEntryBasenow carries no hint of what values are valid.💡 Suggestion
Either add a comment to
DiffEntryBasepointing readers to the per-type enum, or restore the per-type annotations on the embedding structs' doc comments:This keeps discoverability intact without duplicating the value lists.
@copilot please address this.