Skip to content

Commit 0e5e4c4

Browse files
authored
refactor: consolidate duplicate DIFC filtered-item struct and tagsToStrings (#2111)
Two duplication patterns introduced in `internal/server/difc_log.go` required parallel edits across files when modifying DIFC filter event data shapes. ## Changes - **Struct consolidation** — `FilteredItemLogEntry` (server) and `JSONLFilteredItem` (logger) shared identical 11-field data definitions. Moved the shared fields into a new `logger.FilteredItemLogEntry`; `JSONLFilteredItem` now embeds it, keeping only its own `Timestamp` and `Type` fields: ```go // Before: 11 fields duplicated verbatim in both structs type JSONLFilteredItem struct { Timestamp string `json:"timestamp"` Type string `json:"type"` ServerID string `json:"server_id"` // ... 9 more duplicated fields } // After: embed the shared struct type JSONLFilteredItem struct { Timestamp string `json:"timestamp"` Type string `json:"type"` FilteredItemLogEntry } ``` - **Remove `toJSONLFilteredItem()`** — field-by-field copy method replaced with a direct struct literal: `&logger.JSONLFilteredItem{FilteredItemLogEntry: entry}` - **Eliminate duplicate `tagsToStrings`** — local helper in `difc_log.go` was identical to `difc.TagsToStrings`; replaced all call sites with the canonical package function - **Test updates** — `difc_log_test.go` updated to reference `logger.FilteredItemLogEntry` instead of the removed local type > [!WARNING] > > <details> > <summary>Firewall rules blocked me from connecting to one or more addresses (expand for details)</summary> > > #### I tried to connect to the following addresses, but was blocked by firewall rules: > > - `example.com` > - Triggering command: `/tmp/go-build3728142662/b332/launcher.test /tmp/go-build3728142662/b332/launcher.test -test.testlogfile=/tmp/go-build3728142662/b332/testlog.txt -test.paniconexit0 -test.timeout=10m0s rev-�� /unix` (dns block) > - `invalid-host-that-does-not-exist-12345.com` > - Triggering command: `/tmp/go-build3728142662/b317/config.test /tmp/go-build3728142662/b317/config.test -test.testlogfile=/tmp/go-build3728142662/b317/testlog.txt -test.paniconexit0 -test.timeout=10m0s rev-�� ache/go/1.25.8/x-p cf9O/V70NMeR5oSymain x_amd64/vet` (dns block) > - `nonexistent.local` > - Triggering command: `/tmp/go-build3728142662/b332/launcher.test /tmp/go-build3728142662/b332/launcher.test -test.testlogfile=/tmp/go-build3728142662/b332/testlog.txt -test.paniconexit0 -test.timeout=10m0s rev-�� /unix` (dns block) > - `slow.example.com` > - Triggering command: `/tmp/go-build3728142662/b332/launcher.test /tmp/go-build3728142662/b332/launcher.test -test.testlogfile=/tmp/go-build3728142662/b332/testlog.txt -test.paniconexit0 -test.timeout=10m0s rev-�� /unix` (dns block) > - `this-host-does-not-exist-12345.com` > - Triggering command: `/tmp/go-build3728142662/b341/mcp.test /tmp/go-build3728142662/b341/mcp.test -test.testlogfile=/tmp/go-build3728142662/b341/testlog.txt -test.paniconexit0 -test.timeout=10m0s rev-�� ache/go/1.25.8/x64/src/runtime/cgo1.25.8` (dns block) > > If you need me to access, download, or install something from one of these locations, you can either: > > - Configure [Actions setup steps](https://gh.io/copilot/actions-setup-steps) to set up my environment, which run before the firewall is enabled > - Add the appropriate URLs or hosts to the custom allowlist in this repository's [Copilot coding agent settings](https://github.com/github/gh-aw-mcpg/settings/copilot/coding_agent) (admins only) > > </details> <!-- START COPILOT CODING AGENT TIPS --> --- 💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more [Copilot coding agent tips](https://gh.io/copilot-coding-agent-tips) in the docs.
2 parents dca3dde + 0ff0b83 commit 0e5e4c4

File tree

3 files changed

+24
-60
lines changed

3 files changed

+24
-60
lines changed

internal/logger/jsonl_logger.go

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -139,12 +139,10 @@ func LogRPCMessageJSONLWithTags(direction RPCMessageDirection, messageType RPCMe
139139
})
140140
}
141141

142-
// JSONLFilteredItem represents a DIFC-filtered item logged to the JSONL stream.
143-
// These entries appear alongside RPC messages so filter events are visible
144-
// in context with the request/response that triggered them.
145-
type JSONLFilteredItem struct {
146-
Timestamp string `json:"timestamp"`
147-
Type string `json:"type"` // Always "DIFC_FILTERED"
142+
// FilteredItemLogEntry holds the data fields for a DIFC-filtered item.
143+
// It is used for both text log output ([DIFC-FILTERED] JSON lines) and as
144+
// the embedded payload in JSONLFilteredItem for JSONL log output.
145+
type FilteredItemLogEntry struct {
148146
ServerID string `json:"server_id"`
149147
ToolName string `json:"tool_name"`
150148
Description string `json:"description"`
@@ -158,6 +156,17 @@ type JSONLFilteredItem struct {
158156
SHA string `json:"sha,omitempty"`
159157
}
160158

159+
// JSONLFilteredItem represents a DIFC-filtered item logged to the JSONL stream.
160+
// These entries appear alongside RPC messages so filter events are visible
161+
// in context with the request/response that triggered them.
162+
// It embeds FilteredItemLogEntry so that adding a data field requires only a
163+
// single edit rather than parallel edits in two structs.
164+
type JSONLFilteredItem struct {
165+
Timestamp string `json:"timestamp"`
166+
Type string `json:"type"` // Always "DIFC_FILTERED"
167+
FilteredItemLogEntry
168+
}
169+
161170
// LogDifcFilteredItem writes a DIFC filter event to the JSONL log.
162171
func LogDifcFilteredItem(entry *JSONLFilteredItem) {
163172
if entry == nil {

internal/server/difc_log.go

Lines changed: 6 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -8,24 +8,6 @@ import (
88
"github.com/github/gh-aw-mcpg/internal/logger"
99
)
1010

11-
// FilteredItemLogEntry is the structured log record emitted for each item
12-
// removed by DIFC filtering. It is written as JSON to the unified and
13-
// per-server text log files under the [DIFC-FILTERED] marker so that filter
14-
// events can be correlated with the MCP request/response that triggered them.
15-
type FilteredItemLogEntry struct {
16-
ServerID string `json:"server_id"`
17-
ToolName string `json:"tool_name"`
18-
Description string `json:"description"`
19-
Reason string `json:"reason"`
20-
SecrecyTags []string `json:"secrecy_tags"`
21-
IntegrityTags []string `json:"integrity_tags"`
22-
AuthorAssociation string `json:"author_association,omitempty"`
23-
AuthorLogin string `json:"author_login,omitempty"`
24-
HTMLURL string `json:"html_url,omitempty"`
25-
Number string `json:"number,omitempty"`
26-
SHA string `json:"sha,omitempty"`
27-
}
28-
2911
// logFilteredItems logs structured details for every item removed by DIFC filtering.
3012
// Each item is written as a [DIFC-FILTERED] JSON entry to both the unified and
3113
// per-server text log files (via LogInfoWithServer), and as a DIFC_FILTERED entry
@@ -40,22 +22,22 @@ func logFilteredItems(serverID, toolName string, filtered *difc.FilteredCollecti
4022
}
4123
jsonStr := string(b)
4224
logger.LogInfoWithServer(serverID, "difc", "[DIFC-FILTERED] %s", jsonStr)
43-
logger.LogDifcFilteredItem(entry.toJSONLFilteredItem())
25+
logger.LogDifcFilteredItem(&logger.JSONLFilteredItem{FilteredItemLogEntry: entry})
4426
}
4527
}
4628

47-
// buildFilteredItemLogEntry constructs a FilteredItemLogEntry from a filtered item.
48-
func buildFilteredItemLogEntry(serverID, toolName string, detail difc.FilteredItemDetail) FilteredItemLogEntry {
49-
entry := FilteredItemLogEntry{
29+
// buildFilteredItemLogEntry constructs a logger.FilteredItemLogEntry from a filtered item.
30+
func buildFilteredItemLogEntry(serverID, toolName string, detail difc.FilteredItemDetail) logger.FilteredItemLogEntry {
31+
entry := logger.FilteredItemLogEntry{
5032
ServerID: serverID,
5133
ToolName: toolName,
5234
Reason: detail.Reason,
5335
}
5436

5537
if detail.Item.Labels != nil {
5638
entry.Description = detail.Item.Labels.Description
57-
entry.SecrecyTags = tagsToStrings(detail.Item.Labels.Secrecy.Label.GetTags())
58-
entry.IntegrityTags = tagsToStrings(detail.Item.Labels.Integrity.Label.GetTags())
39+
entry.SecrecyTags = difc.TagsToStrings(detail.Item.Labels.Secrecy.Label.GetTags())
40+
entry.IntegrityTags = difc.TagsToStrings(detail.Item.Labels.Integrity.Label.GetTags())
5941
}
6042

6143
// Extract identifying metadata from the raw item data.
@@ -71,33 +53,6 @@ func buildFilteredItemLogEntry(serverID, toolName string, detail difc.FilteredIt
7153
return entry
7254
}
7355

74-
// toJSONLFilteredItem converts a FilteredItemLogEntry to a logger.JSONLFilteredItem
75-
// for writing to the JSONL log.
76-
func (e FilteredItemLogEntry) toJSONLFilteredItem() *logger.JSONLFilteredItem {
77-
return &logger.JSONLFilteredItem{
78-
ServerID: e.ServerID,
79-
ToolName: e.ToolName,
80-
Description: e.Description,
81-
Reason: e.Reason,
82-
SecrecyTags: e.SecrecyTags,
83-
IntegrityTags: e.IntegrityTags,
84-
AuthorAssociation: e.AuthorAssociation,
85-
AuthorLogin: e.AuthorLogin,
86-
HTMLURL: e.HTMLURL,
87-
Number: e.Number,
88-
SHA: e.SHA,
89-
}
90-
}
91-
92-
// tagsToStrings converts DIFC tags to string slice.
93-
func tagsToStrings(tags []difc.Tag) []string {
94-
s := make([]string, len(tags))
95-
for i, t := range tags {
96-
s[i] = string(t)
97-
}
98-
return s
99-
}
100-
10156
// getStringField returns the first non-empty string value from the map
10257
// matching any of the given field names.
10358
func getStringField(m map[string]interface{}, fields ...string) string {

internal/server/difc_log_test.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,7 @@ func TestLogFilteredItems_EmitsValidJSONWithExpectedFields(t *testing.T) {
106106
require.Len(t, unifiedLines, 1, "unified log should have exactly one DIFC-FILTERED entry")
107107

108108
jsonStr := extractJSONFromDIFCLine(t, unifiedLines[0])
109-
var entry FilteredItemLogEntry
109+
var entry logger.FilteredItemLogEntry
110110
require.NoError(t, json.Unmarshal([]byte(jsonStr), &entry), "log entry must be valid JSON")
111111

112112
assert.Equal(t, "github", entry.ServerID)
@@ -125,7 +125,7 @@ func TestLogFilteredItems_EmitsValidJSONWithExpectedFields(t *testing.T) {
125125
require.Len(t, serverLines, 1, "server log should have exactly one DIFC-FILTERED entry")
126126

127127
serverJSONStr := extractJSONFromDIFCLine(t, serverLines[0])
128-
var serverEntry FilteredItemLogEntry
128+
var serverEntry logger.FilteredItemLogEntry
129129
require.NoError(t, json.Unmarshal([]byte(serverJSONStr), &serverEntry), "server log entry must be valid JSON")
130130
assert.Equal(t, entry, serverEntry, "server log entry should match unified log entry")
131131
}
@@ -162,7 +162,7 @@ func TestLogFilteredItems_MultipleItems(t *testing.T) {
162162
numbers := map[string]bool{}
163163
for _, line := range lines {
164164
jsonStr := extractJSONFromDIFCLine(t, line)
165-
var entry FilteredItemLogEntry
165+
var entry logger.FilteredItemLogEntry
166166
require.NoError(t, json.Unmarshal([]byte(jsonStr), &entry))
167167
numbers[entry.Number] = true
168168
}

0 commit comments

Comments
 (0)