refactor(difc): reduce boilerplate duplication in agent.go and labels.go#2591
Conversation
…els.go Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/b8b5924d-811b-4980-a352-16a89fdbd6a6 Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Refactors internal/difc/ to reduce repeated lock/mutate/log patterns around DIFC label cloning, bulk tag updates, and tag reads, making future DIFC strategy/log/label changes less error-prone.
Changes:
- Added
modifyTagsto centralize bulk tag mutation logic and refactored multi-tag methods to use it. - Added
getTagsLockedto reduce duplication in thread-safe tag reads. - Added
cloneLabelOrNewto centralize nil-guarded cloning shared by secrecy/integrity label clones.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| internal/difc/agent.go | Introduces modifyTags and getTagsLocked; refactors bulk add/drop and tag getters to use helpers. |
| internal/difc/labels.go | Introduces cloneLabelOrNew; refactors SecrecyLabel.Clone and IntegrityLabel.Clone to delegate. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // getTagsLocked returns a copy of tags from the given label under the agent's read lock. | ||
| func (a *AgentLabels) getTagsLocked(label *Label) []Tag { | ||
| a.mu.RLock() | ||
| defer a.mu.RUnlock() | ||
| return a.Secrecy.Label.GetTags() | ||
| if label == nil { | ||
| return nil | ||
| } | ||
| return label.GetTags() | ||
| } | ||
|
|
||
| // GetSecrecyTags returns a copy of secrecy tags (thread-safe) | ||
| func (a *AgentLabels) GetSecrecyTags() []Tag { | ||
| return a.getTagsLocked(a.Secrecy.Label) |
There was a problem hiding this comment.
getTagsLocked takes a *Label argument, but callers pass a.Secrecy.Label / a.Integrity.Label before getTagsLocked acquires a.mu.RLock(). That means the agent read-lock is not actually protecting the selection of which label pointer is used, which is at odds with the helper’s comment and weakens the intended thread-safety if those exported fields are ever swapped. Consider either (1) acquiring RLock in GetSecrecyTags/GetIntegrityTags before reading the pointer, or (2) changing getTagsLocked to accept a getter closure and dereference the label inside the lock.
Three structural duplication patterns in
internal/difc/required updating multiple identical code blocks for any locking strategy, log format, or label type changes.Changes
modifyTagshelper — bulk label mutations (AddSecrecyTags,AddIntegrityTags,DropIntegrityTags) were each inlining the same lock/operate/log pattern instead of using the existingmodifyTagprecedent:getTagsLockedhelper —GetSecrecyTagsandGetIntegrityTagseach duplicated the sameRLock+GetTags()body; extracted to a single nil-guarded helper.cloneLabelOrNewhelper —SecrecyLabel.CloneandIntegrityLabel.Cloneshared identical nil-guard logic; extracted to a standalone function so bothClonemethods become single-line delegations.Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
example.com/tmp/go-build220454796/b334/launcher.test /tmp/go-build220454796/b334/launcher.test -test.testlogfile=/tmp/go-build220454796/b334/testlog.txt -test.paniconexit0 -test.timeout=10m0s rtcf�� ache/go/1.25.8/x-s 64/src/crypto/hm-w x_amd64/vet(dns block)/tmp/go-build988514342/b330/launcher.test /tmp/go-build988514342/b330/launcher.test -test.testlogfile=/tmp/go-build988514342/b330/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 1afb76c6d5a1b24a/run/containerd/io.containerd.runtime.v2.task/moby/c066a1e0c75e18fce6c91b9a92a66sed y e-handler by/2dfd37f3cd6c3git lang.org/x/text/config(dns block)invalid-host-that-does-not-exist-12345.com/tmp/go-build220454796/b319/config.test /tmp/go-build220454796/b319/config.test -test.testlogfile=/tmp/go-build220454796/b319/testlog.txt -test.paniconexit0 -test.timeout=10m0s conf�� 64/src/runtime/cgo go x_amd64/compile(dns block)/tmp/go-build988514342/b315/config.test /tmp/go-build988514342/b315/config.test -test.testlogfile=/tmp/go-build988514342/b315/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true f6977767ec5dcecf/run/containerd/io.containerd.runtime.v2.task/moby/c066a1e0c75e18fce6c91b9a92a66/bin/sh runtime-runc/mob--log-format csi/net-interfacjson by/37580e7ab5c50git(dns block)nonexistent.local/tmp/go-build220454796/b334/launcher.test /tmp/go-build220454796/b334/launcher.test -test.testlogfile=/tmp/go-build220454796/b334/testlog.txt -test.paniconexit0 -test.timeout=10m0s rtcf�� ache/go/1.25.8/x-s 64/src/crypto/hm-w x_amd64/vet(dns block)/tmp/go-build988514342/b330/launcher.test /tmp/go-build988514342/b330/launcher.test -test.testlogfile=/tmp/go-build988514342/b330/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 1afb76c6d5a1b24a/run/containerd/io.containerd.runtime.v2.task/moby/c066a1e0c75e18fce6c91b9a92a66sed y e-handler by/2dfd37f3cd6c3git lang.org/x/text/config(dns block)slow.example.com/tmp/go-build220454796/b334/launcher.test /tmp/go-build220454796/b334/launcher.test -test.testlogfile=/tmp/go-build220454796/b334/testlog.txt -test.paniconexit0 -test.timeout=10m0s rtcf�� ache/go/1.25.8/x-s 64/src/crypto/hm-w x_amd64/vet(dns block)/tmp/go-build988514342/b330/launcher.test /tmp/go-build988514342/b330/launcher.test -test.testlogfile=/tmp/go-build988514342/b330/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 1afb76c6d5a1b24a/run/containerd/io.containerd.runtime.v2.task/moby/c066a1e0c75e18fce6c91b9a92a66sed y e-handler by/2dfd37f3cd6c3git lang.org/x/text/config(dns block)this-host-does-not-exist-12345.com/tmp/go-build220454796/b343/mcp.test /tmp/go-build220454796/b343/mcp.test -test.testlogfile=/tmp/go-build220454796/b343/testlog.txt -test.paniconexit0 -test.timeout=10m0s o_.o�� 64/src/net late/parse/lex.go x_amd64/vet -p github.com/tetra-test.testlogfile=/tmp/go-build220454796/b334/testlog.txt -lang=go1.24 x_amd64/vet 5991�� _.a 599188/b151/ x_amd64/vet --gdwarf-5 ternal/engine/wa-m -o dmA41Y_/RID864WwwFNZuxdU3EAU(dns block)/tmp/go-build988514342/b339/mcp.test /tmp/go-build988514342/b339/mcp.test -test.testlogfile=/tmp/go-build988514342/b339/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true /tmp/go-build220454796/b370/_pkg_.a by/ee0f09b892b0c84149e47062e494bfc467b029bcbe96e63e2b5ad75092fcc8cd ker/cli-plugins/docker-buildx by/ee0f09b892b0c/opt/hostedtoolcache/go/1.25.8/x64/pkg/tool/linux_amd64/vet 2b5ad75092fcc8cd-atomic 76224512e8a2c58c-bool ker/cli-plugins/-buildtags n-me�� tes.crt -goversion /home/REDACTED/wor-nilfunc -c=4 -nolocalimports dea2d1d50bb3e29a-stringintconv 4w8uw1gybk6(dns block)If you need me to access, download, or install something from one of these locations, you can either:
📱 Kick off Copilot coding agent tasks wherever you are with GitHub Mobile, available on iOS and Android.