Extract DiffEntryBase embed to reduce anomaly-field repetition in audit_diff.go#46972
Conversation
…it_diff.go Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #46972 does not have the 'implementation' label and has only 49 new lines of code in business logic directories (threshold: 100). |
There was a problem hiding this comment.
Pull request overview
Extracts shared diff-entry fields to reduce duplication in audit output models.
Changes:
- Adds
DiffEntryBase. - Embeds it across three diff entry types.
- Updates all construction sites.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/audit_diff.go |
Consolidates shared status and anomaly fields. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Medium
| Domain string `json:"domain"` | ||
| DiffEntryBase |
There was a problem hiding this comment.
Clean, correct refactor. Go struct embedding promotes Status, IsAnomaly, and AnomalyNote inline in JSON output, so wire format is unchanged. All 8 construction sites updated consistently. No issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 16.8 AIC · ⌖ 4.32 AIC · ⊞ 5K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — two observations, neither blocking.
📋 Key Themes & Highlights
Key Themes
- Lost status-value documentation: the per-type
Statusfield inline comments (e.g.// "new", "removed", "status_changed", "volume_changed") were dropped;DiffEntryBasehas no equivalent — discoverability for new contributors suffers. - JSON field order changed: the PR claims the wire format is unchanged, but
is_anomaly/anomaly_notenow serialize betweenstatusand the type-specific fields rather than at the end. JSON key order does not affectencoding/jsonunmarshal, but it can break raw-string snapshot comparisons.
Positive Highlights
- ✅ Clean, focused extraction — exactly the three fields that are duplicated, nothing more.
- ✅ All 8 construction sites updated consistently.
- ✅ Go embedding correctly promotes fields into the JSON flat structure as documented.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 26.8 AIC · ⌖ 4.61 AIC · ⊞ 6.7K
Comment /matt to run again
|
|
||
| // DiffEntryBase holds common anomaly-flagging fields shared by all diff entry types. | ||
| type DiffEntryBase struct { | ||
| Status string `json:"status"` |
There was a problem hiding this comment.
[/codebase-design] The valid status values per entry type were documented as inline comments on the old Status fields (e.g. // "new", "removed", "status_changed", "volume_changed" on DomainDiffEntry) but were dropped during extraction — DiffEntryBase now carries no hint of what values are valid.
💡 Suggestion
Either add a comment to DiffEntryBase pointing readers to the per-type enum, or restore the per-type annotations on the embedding structs' doc comments:
// DiffEntryBase holds common anomaly-flagging fields shared by all diff entry types.
// Status values are entry-type specific; see DomainDiffEntry, MCPToolDiffEntry, ToolCallDiffEntry.
type DiffEntryBase struct {This keeps discoverability intact without duplicating the value lists.
@copilot please address this.
| Domain string `json:"domain"` | ||
| Status string `json:"status"` // "new", "removed", "status_changed", "volume_changed" | ||
| Domain string `json:"domain"` | ||
| DiffEntryBase |
There was a problem hiding this comment.
[/codebase-design] The PR says "wire format is unchanged" but the JSON field order has shifted: is_anomaly / anomaly_note now appear between status and the type-specific fields instead of at the end. Consumers relying on key ordering (snapshot tests, jq positional scripts) may see unexpected diffs.
💡 Suggestion
Clarify 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.
There was a problem hiding this comment.
Clean, mechanical refactor — no correctness issues.
### Review summary
The extraction of DiffEntryBase is correct:
- Go struct embedding promotes
Status,IsAnomaly, andAnomalyNoteinline in JSON output, so the wire format is unchanged. - All 8 construction sites are updated consistently.
- No logic paths were altered.
One non-blocking doc comment issue flagged inline.
🔎 Code quality review by PR Code Quality Reviewer · 27.4 AIC · ⌖ 4.43 AIC · ⊞ 5.6K
Comment /review to run again
Comments that could not be inline-anchored
pkg/cli/audit_diff.go:10
Misleading doc comment: DiffEntryBase is described as holding "anomaly-flagging fields" but Status is a general entry discriminator ("new", "removed", "changed") unrelated to anomaly detection.
<details>
<summary>💡 Suggested fix</summary>
// DiffEntryBase holds common fields shared by all diff entry types.
type DiffEntryBase struct {Status is the primary discriminator; the anomaly fields (IsAnomaly, AnomalyNote) are secondary. The current comment misrepresents…
|
🎉 This pull request is included in a new release. Release: |
DomainDiffEntry,MCPToolDiffEntry, andToolCallDiffEntryeach duplicated the same three anomaly-flagging fields inline. This extracts them into a sharedDiffEntryBaseembed.Changes
DiffEntryBasestruct — holds the common fields:DomainDiffEntry,MCPToolDiffEntry, andToolCallDiffEntryeach replace their inline copies withDiffEntryBaseDiffEntryBase: DiffEntryBase{Status: "..."}syntaxGo struct embedding promotes fields inline in JSON output, so wire format is unchanged.