From 6381607db09701dd42e4d1afedc47bad59802ebe Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 6 Jul 2026 12:52:17 +0000 Subject: [PATCH 1/2] Initial plan From 42f47bed097704e8ec3548dfbcccdcccc627df90 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 6 Jul 2026 13:10:11 +0000 Subject: [PATCH 2/2] refactor(difc): extract tagSet helper to eliminate duplicate concurrent map methods Extract an unexported tagSet struct in internal/difc/tagset.go that holds the shared map[Tag]struct{} + sync.RWMutex fields and provides common concurrent mutation/read methods (add, addAll, remove, removeAll, contains, getAll, clear, count, isEmpty). Both Label (labels.go) and Capabilities (capabilities.go) now embed tagSet and delegate their public methods to it. This eliminates ~25 lines of duplicated mutex-locking logic and ensures any future concurrency fixes or logging changes need to be made in only one place. Closes #8772 --- internal/difc/capabilities.go | 46 +++++--------------- internal/difc/labels.go | 43 ++++--------------- internal/difc/tagset.go | 79 +++++++++++++++++++++++++++++++++++ 3 files changed, 98 insertions(+), 70 deletions(-) create mode 100644 internal/difc/tagset.go diff --git a/internal/difc/capabilities.go b/internal/difc/capabilities.go index de04d73b..46fab6de 100644 --- a/internal/difc/capabilities.go +++ b/internal/difc/capabilities.go @@ -1,63 +1,43 @@ package difc -import ( - "sync" - - "github.com/github/gh-aw-mcpg/internal/logger" -) +import "github.com/github/gh-aw-mcpg/internal/logger" var logCapabilities = logger.New("difc:capabilities") // Capabilities represents the global set of tags available in the system // This is used to validate and discover available DIFC tags type Capabilities struct { - tags map[Tag]struct{} - mu sync.RWMutex + tagSet } // NewCapabilities creates a new empty capabilities set func NewCapabilities() *Capabilities { logCapabilities.Print("Creating new capabilities set") - return &Capabilities{ - tags: make(map[Tag]struct{}), - } + return &Capabilities{tagSet: newTagSet()} } // Add adds a tag to the capabilities func (c *Capabilities) Add(tag Tag) { logCapabilities.Printf("Adding tag: %s", tag) - c.mu.Lock() - defer c.mu.Unlock() - c.tags[tag] = struct{}{} + c.add(tag) } // AddAll adds multiple tags to the capabilities func (c *Capabilities) AddAll(tags []Tag) { logCapabilities.Printf("Adding %d tags to capabilities", len(tags)) - c.mu.Lock() - defer c.mu.Unlock() - for _, tag := range tags { - c.tags[tag] = struct{}{} - } + c.addAll(tags) } // Contains checks if a tag is available in the capabilities func (c *Capabilities) Contains(tag Tag) bool { - c.mu.RLock() - _, ok := c.tags[tag] - c.mu.RUnlock() + ok := c.contains(tag) logCapabilities.Printf("Contains: tag=%s, found=%v", tag, ok) return ok } // GetAll returns all available tags func (c *Capabilities) GetAll() []Tag { - c.mu.RLock() - tags := make([]Tag, 0, len(c.tags)) - for tag := range c.tags { - tags = append(tags, tag) - } - c.mu.RUnlock() + tags := c.getAll() logCapabilities.Printf("GetAll: returning %d tags", len(tags)) return tags } @@ -65,22 +45,16 @@ func (c *Capabilities) GetAll() []Tag { // Remove removes a tag from the capabilities func (c *Capabilities) Remove(tag Tag) { logCapabilities.Printf("Removing tag: %s", tag) - c.mu.Lock() - defer c.mu.Unlock() - delete(c.tags, tag) + c.remove(tag) } // Clear removes all tags from the capabilities func (c *Capabilities) Clear() { logCapabilities.Print("Clearing all capabilities") - c.mu.Lock() - defer c.mu.Unlock() - c.tags = make(map[Tag]struct{}) + c.clear() } // Count returns the number of available tags func (c *Capabilities) Count() int { - c.mu.RLock() - defer c.mu.RUnlock() - return len(c.tags) + return c.count() } diff --git a/internal/difc/labels.go b/internal/difc/labels.go index 3fba978f..bb57be93 100644 --- a/internal/difc/labels.go +++ b/internal/difc/labels.go @@ -3,7 +3,6 @@ package difc import ( "fmt" "strings" - "sync" "github.com/github/gh-aw-mcpg/internal/logger" ) @@ -21,13 +20,12 @@ const WildcardTag = Tag("*") // Label represents a set of DIFC tags type Label struct { - tags map[Tag]struct{} - mu sync.RWMutex + tagSet } // NewLabel creates a new empty label func NewLabel() *Label { - return &Label{tags: make(map[Tag]struct{})} + return &Label{tagSet: newTagSet()} } // newLabelWithTags is a helper function that creates a label with the given tags. @@ -40,42 +38,27 @@ func newLabelWithTags(tags []Tag) *Label { // Add adds a tag to this label func (l *Label) Add(tag Tag) { - l.mu.Lock() - defer l.mu.Unlock() - l.tags[tag] = struct{}{} + l.add(tag) } // AddAll adds multiple tags to this label func (l *Label) AddAll(tags []Tag) { - l.mu.Lock() - defer l.mu.Unlock() - for _, tag := range tags { - l.tags[tag] = struct{}{} - } + l.addAll(tags) } // Remove removes a single tag from this label func (l *Label) Remove(tag Tag) { - l.mu.Lock() - defer l.mu.Unlock() - delete(l.tags, tag) + l.remove(tag) } // RemoveAll removes multiple tags from this label func (l *Label) RemoveAll(tags []Tag) { - l.mu.Lock() - defer l.mu.Unlock() - for _, tag := range tags { - delete(l.tags, tag) - } + l.removeAll(tags) } // Contains checks if this label contains a specific tag func (l *Label) Contains(tag Tag) bool { - l.mu.RLock() - defer l.mu.RUnlock() - _, ok := l.tags[tag] - return ok + return l.contains(tag) } // Union merges another label into this label @@ -144,20 +127,12 @@ func (l *Label) Clone() *Label { // GetTags returns all tags in this label as a slice func (l *Label) GetTags() []Tag { - l.mu.RLock() - defer l.mu.RUnlock() - tags := make([]Tag, 0, len(l.tags)) - for tag := range l.tags { - tags = append(tags, tag) - } - return tags + return l.getAll() } // IsEmpty returns true if this label has no tags func (l *Label) IsEmpty() bool { - l.mu.RLock() - defer l.mu.RUnlock() - return len(l.tags) == 0 + return l.isEmpty() } // cloneLabelOrNew clones inner if it is non-nil, otherwise returns a new empty Label. diff --git a/internal/difc/tagset.go b/internal/difc/tagset.go new file mode 100644 index 00000000..2f7bb87c --- /dev/null +++ b/internal/difc/tagset.go @@ -0,0 +1,79 @@ +package difc + +import "sync" + +// tagSet is an unexported concurrent set of Tags backed by a map and a RWMutex. +// It provides the common concurrent mutation and read operations shared by Label +// and Capabilities, eliminating duplicated locking logic across both types. +type tagSet struct { + tags map[Tag]struct{} + mu sync.RWMutex +} + +// newTagSet creates and initialises an empty tagSet. +func newTagSet() tagSet { + return tagSet{tags: make(map[Tag]struct{})} +} + +func (ts *tagSet) add(tag Tag) { + ts.mu.Lock() + defer ts.mu.Unlock() + ts.tags[tag] = struct{}{} +} + +func (ts *tagSet) addAll(tags []Tag) { + ts.mu.Lock() + defer ts.mu.Unlock() + for _, tag := range tags { + ts.tags[tag] = struct{}{} + } +} + +func (ts *tagSet) remove(tag Tag) { + ts.mu.Lock() + defer ts.mu.Unlock() + delete(ts.tags, tag) +} + +func (ts *tagSet) removeAll(tags []Tag) { + ts.mu.Lock() + defer ts.mu.Unlock() + for _, tag := range tags { + delete(ts.tags, tag) + } +} + +func (ts *tagSet) contains(tag Tag) bool { + ts.mu.RLock() + defer ts.mu.RUnlock() + _, ok := ts.tags[tag] + return ok +} + +func (ts *tagSet) getAll() []Tag { + ts.mu.RLock() + defer ts.mu.RUnlock() + tags := make([]Tag, 0, len(ts.tags)) + for tag := range ts.tags { + tags = append(tags, tag) + } + return tags +} + +func (ts *tagSet) clear() { + ts.mu.Lock() + defer ts.mu.Unlock() + ts.tags = make(map[Tag]struct{}) +} + +func (ts *tagSet) count() int { + ts.mu.RLock() + defer ts.mu.RUnlock() + return len(ts.tags) +} + +func (ts *tagSet) isEmpty() bool { + ts.mu.RLock() + defer ts.mu.RUnlock() + return len(ts.tags) == 0 +}