Refactor duplicate code patterns in DIFC package by Claude · Pull Request #1792 · github/gh-aw-mcpg

Claude · 2026-03-11T19:23:42Z

Summary

Eliminates ~71 lines of duplicate code across the DIFC package by extracting two generic helpers. Addresses issue #1719 patterns 2 and 3 (pattern 1 was previously resolved).

Changes

CheckFlow Consolidation (`labels.go`)

Problem: SecrecyLabel.CheckFlow and IntegrityLabel.CheckFlow contained 58 lines of nearly identical nil handling, locking, and tag iteration logic with only semantic differences (subset vs superset checks).

Solution: Extracted checkFlowHelper that parameterizes flow semantics:

// Generic helper with parameterized semantics
func checkFlowHelper(srcLabel *Label, targetLabel *Label, checkSubset bool, labelType string) (bool, []Tag)

// Thin wrappers (10 lines each)
func (l *SecrecyLabel) CheckFlow(target *SecrecyLabel) (bool, []Tag) {
    return checkFlowHelper(l.Label, target.Label, true, "Secrecy")  // subset check
}

func (l *IntegrityLabel) CheckFlow(target *IntegrityLabel) (bool, []Tag) {
    return checkFlowHelper(l.Label, target.Label, false, "Integrity")  // superset check
}

EvaluationResult Initialization (`evaluator.go`)

Problem: Three evaluation functions repeated identical 5-line initialization blocks.

Solution: Extracted newEmptyEvaluationResult() helper used by Evaluate, evaluateRead, and evaluateWrite.

Testing

Added 12 comprehensive test cases for new helpers
All existing tests pass (100+ in difc package)
No behavior changes

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
- Triggering command: /tmp/go-build817523219/b332/launcher.test /tmp/go-build817523219/b332/launcher.test -test.testlogfile=/tmp/go-build817523219/b332/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true ache/go/1.25.7/x64/src/runtime/c-errorsas J4aB/2fPHF0HdMhn8ydxhJ4aB 64/pkg/tool/linux_amd64/vet -I /tmp/go-build151-n -I 64/pkg/tool/linux_amd64/vet go_.�� 8036719/b171=/tmp/go-build .cfg 64/pkg/tool/linux_amd64/vet -p ache/go/1.25.7/x-d -lang=go1.16 64/pkg/tool/linux_amd64/vet (dns block)
- Triggering command: /tmp/go-build2582390263/b336/launcher.test /tmp/go-build2582390263/b336/launcher.test -test.testlogfile=/tmp/go-build2582390263/b336/testlog.txt -test.paniconexit0 -test.timeout=10m0s [ �� --prefix=/net/ipv4/neigh/veth8c3e74e --prefix=/net/ipv6/conf/veth8c3e74e ache/uv/0.10.9/x86_64/git --64 ache/go/1.25.7/x-d /snap/bin/git bash /usr�� --root /var/run/docker/runtime-runc/moby 64/pkg/tool/linux_amd64/vet /run/containerd/bash --log-format 619fd60bb04098d5--version 64/pkg/tool/linux_amd64/vet (dns block)
invalid-host-that-does-not-exist-12345.com
- Triggering command: /tmp/go-build817523219/b314/config.test /tmp/go-build817523219/b314/config.test -test.testlogfile=/tmp/go-build817523219/b314/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true o QT x_amd64/vet -p g/x/net/idna -lang=go1.25 x_amd64/vet go_.�� 64/src/net ache/go/1.25.7/x64/src/runtime/c-nolocalimports ache/go/1.25.7/x64/pkg/tool/linu-importcfg 8036719/b171/ crypto/internal/-d (dns block)
- Triggering command: /tmp/go-build2582390263/b318/config.test /tmp/go-build2582390263/b318/config.test -test.testlogfile=/tmp/go-build2582390263/b318/testlog.txt -test.paniconexit0 -test.timeout=10m0s e755�� moby -address x_amd64/vet -publish-binary /usr/bin/contain/home/REDACTED/work/_temp/ghcca-node/node -id x_amd64/vet de/n�� /run/containerd/io.containerd.runtime.v2.task/moby/d20f582628202OUTPUT delete /opt/containerd/bin/bash s-through=-lpthrbase64 ache/go/1.25.7/x-d ine bash (dns block)
nonexistent.local
- Triggering command: /tmp/go-build817523219/b332/launcher.test /tmp/go-build817523219/b332/launcher.test -test.testlogfile=/tmp/go-build817523219/b332/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true ache/go/1.25.7/x64/src/runtime/c-errorsas J4aB/2fPHF0HdMhn8ydxhJ4aB 64/pkg/tool/linux_amd64/vet -I /tmp/go-build151-n -I 64/pkg/tool/linux_amd64/vet go_.�� 8036719/b171=/tmp/go-build .cfg 64/pkg/tool/linux_amd64/vet -p ache/go/1.25.7/x-d -lang=go1.16 64/pkg/tool/linux_amd64/vet (dns block)
- Triggering command: /tmp/go-build2582390263/b336/launcher.test /tmp/go-build2582390263/b336/launcher.test -test.testlogfile=/tmp/go-build2582390263/b336/testlog.txt -test.paniconexit0 -test.timeout=10m0s [ �� --prefix=/net/ipv4/neigh/veth8c3e74e --prefix=/net/ipv6/conf/veth8c3e74e ache/uv/0.10.9/x86_64/git --64 ache/go/1.25.7/x-d /snap/bin/git bash /usr�� --root /var/run/docker/runtime-runc/moby 64/pkg/tool/linux_amd64/vet /run/containerd/bash --log-format 619fd60bb04098d5--version 64/pkg/tool/linux_amd64/vet (dns block)
slow.example.com
- Triggering command: /tmp/go-build817523219/b332/launcher.test /tmp/go-build817523219/b332/launcher.test -test.testlogfile=/tmp/go-build817523219/b332/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true ache/go/1.25.7/x64/src/runtime/c-errorsas J4aB/2fPHF0HdMhn8ydxhJ4aB 64/pkg/tool/linux_amd64/vet -I /tmp/go-build151-n -I 64/pkg/tool/linux_amd64/vet go_.�� 8036719/b171=/tmp/go-build .cfg 64/pkg/tool/linux_amd64/vet -p ache/go/1.25.7/x-d -lang=go1.16 64/pkg/tool/linux_amd64/vet (dns block)
- Triggering command: /tmp/go-build2582390263/b336/launcher.test /tmp/go-build2582390263/b336/launcher.test -test.testlogfile=/tmp/go-build2582390263/b336/testlog.txt -test.paniconexit0 -test.timeout=10m0s [ �� --prefix=/net/ipv4/neigh/veth8c3e74e --prefix=/net/ipv6/conf/veth8c3e74e ache/uv/0.10.9/x86_64/git --64 ache/go/1.25.7/x-d /snap/bin/git bash /usr�� --root /var/run/docker/runtime-runc/moby 64/pkg/tool/linux_amd64/vet /run/containerd/bash --log-format 619fd60bb04098d5--version 64/pkg/tool/linux_amd64/vet (dns block)
this-host-does-not-exist-12345.com
- Triggering command: /tmp/go-build817523219/b341/mcp.test /tmp/go-build817523219/b341/mcp.test -test.testlogfile=/tmp/go-build817523219/b341/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true p=/opt/hostedtoolcache/go/1.25.7/x64=/_/GOROOT .cfg 64/pkg/tool/linux_amd64/vet --gdwarf-5 --64 -o 64/pkg/tool/linux_amd64/vet 8036�� /server/auth.go /server/handlers.go 64/pkg/tool/linux_amd64/vet /tmp/go-build151/usr/bin/runc.original -imultiarch x86_64-linux-gnu 64/pkg/tool/linux_amd64/vet (dns block)
- Triggering command: /tmp/go-build2582390263/b345/mcp.test /tmp/go-build2582390263/b345/mcp.test -test.testlogfile=/tmp/go-build2582390263/b345/testlog.txt -test.paniconexit0 -test.timeout=10m0s crea�� b1f0f2a3ed4d89922f26e357de08e619fd60bb04098d570e /run/containerd/io.containerd.runtime.v2.task/moby/f41fcf883e64cbb1a392bb10395bbjson 64/pkg/tool/linux_amd64/vet /run/containerd//usr/bin/runc.original l || true && eva--version /usr/bin/base64 64/pkg/tool/linux_amd64/vet /usr�� .cfg /var/run/docker/runtime-runc/moby 64/pkg/tool/linux_amd64/vet /run/containerd/bash --log-format json with other checks..."; \ elif command -v golan (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Configure 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 (admins only)

- Extract checkFlowHelper in labels.go to eliminate duplication between SecrecyLabel.CheckFlow and IntegrityLabel.CheckFlow - Extract newEmptyEvaluationResult helper in evaluator.go to centralize EvaluationResult initialization - Add comprehensive tests for new helper functions - All existing tests pass, no behavior changes Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>

Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>

Copilot

Pull request overview

Refactors duplicate DIFC logic by extracting shared helpers for label flow checks and evaluation result initialization, aiming to reduce repetition while preserving behavior in the internal/difc package.

Changes:

Added checkFlowHelper and rewired SecrecyLabel.CheckFlow / IntegrityLabel.CheckFlow to use it.
Added newEmptyEvaluationResult() and replaced repeated EvaluationResult initialization in evaluator code.
Added targeted unit tests for the new helpers.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
internal/difc/labels.go	Introduces `checkFlowHelper` and routes `CheckFlow` methods through it.
internal/difc/labels_test.go	Adds new table tests targeting `checkFlowHelper` behavior.
internal/difc/evaluator.go	Introduces `newEmptyEvaluationResult()` and uses it in evaluation paths.
internal/difc/evaluator_test.go	Adds tests validating `newEmptyEvaluationResult()` defaults/independence.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Copilot · 2026-03-11T19:48:40Z

internal/difc/evaluator.go

 			return writeResult
 		}
 	}

-	return result
+	return newEmptyEvaluationResult()


In the OperationReadWrite path, once both readResult and writeResult are allowed, the function falls through and returns a fresh newEmptyEvaluationResult(). This discards any non-default decision/details from readResult (notably AccessAllowWithPropagate + tag deltas in propagate mode). Consider returning a combined result (e.g., prefer readResult if it requires propagation, or merge the tag deltas) instead of always returning an empty allow result.

Copilot · 2026-03-11T19:48:40Z

internal/difc/labels_test.go

+	tests := []struct {
+		name        string
+		src         []Tag
+		target      []Tag
+		nilSrc      bool
+		nilTarget   bool
+		wantOK      bool
+		wantViolate []Tag
+	}{
+		{
+			name:   "nil source can flow to anything (secrecy)",
+			nilSrc: true,
+			target: []Tag{"any"},
+			wantOK: true,
+		},
+		{
+			name:      "empty source can flow to nil target (secrecy)",
+			src:       nil,
+			nilTarget: true,
+			wantOK:    true,
+		},
+		{
+			name:        "non-empty source cannot flow to nil target (secrecy)",
+			src:         []Tag{"secret"},
+			nilTarget:   true,
+			wantOK:      false,
+			wantViolate: []Tag{"secret"},
+		},
+		{
+			name:   "source subset of target allowed (secrecy)",
+			src:    []Tag{"tag1"},
+			target: []Tag{"tag1", "tag2"},
+			wantOK: true,
+		},
+		{
+			name:        "source has extra tags denied (secrecy)",
+			src:         []Tag{"tag1", "extra"},
+			target:      []Tag{"tag1"},
+			wantOK:      false,
+			wantViolate: []Tag{"extra"},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			var src, target *Label
+			if !tt.nilSrc {
+				src = newLabelWithTags(tt.src)
+			}
+			if !tt.nilTarget {
+				target = newLabelWithTags(tt.target)
+			}
+
+			ok, violatingTags := checkFlowHelper(src, target, true, "Secrecy")
+			assert.Equal(t, tt.wantOK, ok)
+			if tt.wantOK {
+				assert.Empty(t, violatingTags)
+			} else {
+				assert.NotEmpty(t, violatingTags)
+				for _, expectedTag := range tt.wantViolate {
+					assert.Contains(t, violatingTags, expectedTag)
+				}
+			}
+		})
+	}


These new checkFlowHelper table tests largely duplicate the existing TestSecrecyLabel_CheckFlow / TestIntegrityLabel_CheckFlow cases earlier in this file, which increases maintenance cost without adding much new coverage. Consider reusing the existing tables (or driving both the wrapper methods and checkFlowHelper from a shared table) so there’s a single source of truth for the semantics.

Suggested change

tests := []struct {

name string

src []Tag

target []Tag

nilSrc bool

nilTarget bool

wantOK bool

wantViolate []Tag

}{

{

name: "nil source can flow to anything (secrecy)",

nilSrc: true,

target: []Tag{"any"},

wantOK: true,

},

{

name: "empty source can flow to nil target (secrecy)",

src: nil,

nilTarget: true,

wantOK: true,

},

{

name: "non-empty source cannot flow to nil target (secrecy)",

src: []Tag{"secret"},

nilTarget: true,

wantOK: false,

wantViolate: []Tag{"secret"},

},

{

name: "source subset of target allowed (secrecy)",

src: []Tag{"tag1"},

target: []Tag{"tag1", "tag2"},

wantOK: true,

},

{

name: "source has extra tags denied (secrecy)",

src: []Tag{"tag1", "extra"},

target: []Tag{"tag1"},

wantOK: false,

wantViolate: []Tag{"extra"},

},

}

for _, tt := range tests {

t.Run(tt.name, func(t *testing.T) {

var src, target *Label

if !tt.nilSrc {

src = newLabelWithTags(tt.src)

}

if !tt.nilTarget {

target = newLabelWithTags(tt.target)

}

ok, violatingTags := checkFlowHelper(src, target, true, "Secrecy")

assert.Equal(t, tt.wantOK, ok)

if tt.wantOK {

assert.Empty(t, violatingTags)

} else {

assert.NotEmpty(t, violatingTags)

for _, expectedTag := range tt.wantViolate {

assert.Contains(t, violatingTags, expectedTag)

}

}

})

}

t.Run("source subset of target allowed (secrecy)", func(t *testing.T) {

src := newLabelWithTags([]Tag{"tag1"})

target := newLabelWithTags([]Tag{"tag1", "tag2"})

ok, violatingTags := checkFlowHelper(src, target, true, "Secrecy")

assert.True(t, ok)

assert.Empty(t, violatingTags)

})

t.Run("source with extra tags denied (secrecy)", func(t *testing.T) {

src := newLabelWithTags([]Tag{"tag1", "extra"})

target := newLabelWithTags([]Tag{"tag1"})

ok, violatingTags := checkFlowHelper(src, target, true, "Secrecy")

assert.False(t, ok)

assert.NotEmpty(t, violatingTags)

assert.Contains(t, violatingTags, Tag("extra"))

})

Copilot · 2026-03-11T19:48:40Z

internal/difc/labels_test.go

+			target: nil,
+			wantOK: true,
+		},
+		{


TestCheckFlowHelper_Integrity doesn’t currently cover the distinct nil-source + non-empty-target case (srcLabel == nil, targetLabel has tags), which should be denied and return the target’s tags as violations. Adding a table entry for that scenario would exercise the dedicated nil-source integrity branch in checkFlowHelper.

Suggested change

{

{

name: "nil source non-empty target denied (integrity)",

nilSrc: true,

target: []Tag{"t1"},

wantOK: false,

wantViolate: []Tag{"t1"},

},

{

lpcox · 2026-03-11T19:51:04Z

@copilot look at the pull request review and create a new PR that addresses the feedback

Initial plan

01b3a5e

Claude AI assigned Claude and lpcox Mar 11, 2026

Claude started work on behalf of lpcox March 11, 2026 19:23 View session

Claude AI linked an issue Mar 11, 2026 that may be closed by this pull request

[duplicate-code] Duplicate Code Analysis Report #1719

Closed

Claude AI and others added 2 commits March 11, 2026 19:27

Complete duplicate code refactoring - all tests pass

6b0c002

Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>

Claude AI changed the title ~~[WIP] Fix duplicate code in internal/difc package~~ Refactor duplicate code patterns in DIFC package Mar 11, 2026

Claude finished work on behalf of lpcox March 11, 2026 19:30

lpcox marked this pull request as ready for review March 11, 2026 19:43

Copilot AI review requested due to automatic review settings March 11, 2026 19:43

lpcox merged commit 7451642 into main Mar 11, 2026
12 checks passed

lpcox deleted the claude/fix-duplicate-code-issues-again branch March 11, 2026 19:43

Copilot started reviewing on behalf of lpcox March 11, 2026 19:43 View session

Copilot AI reviewed Mar 11, 2026

View reviewed changes

This was referenced Mar 11, 2026

Smoke Test: Copilot - 22971161535 #1797

Closed

Smoke Test: Copilot - 22971240966 #1798

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor duplicate code patterns in DIFC package#1792

Refactor duplicate code patterns in DIFC package#1792
lpcox merged 3 commits intomainfrom
claude/fix-duplicate-code-issues-again

Claude AI commented Mar 11, 2026 •

edited

Loading

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 11, 2026

Uh oh!

Copilot AI Mar 11, 2026

Uh oh!

Copilot AI Mar 11, 2026

Uh oh!

lpcox commented Mar 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

-		{
+		{
+			name:        "nil source non-empty target denied (integrity)",
+			nilSrc:      true,
+			target:      []Tag{"t1"},
+			wantOK:      false,
+			wantViolate: []Tag{"t1"},
+		},
+		{

Conversation

Claude AI commented Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

CheckFlow Consolidation (labels.go)

EvaluationResult Initialization (evaluator.go)

Testing

I tried to connect to the following addresses, but was blocked by firewall rules:

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

lpcox commented Mar 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Claude AI commented Mar 11, 2026 •

edited

Loading

CheckFlow Consolidation (`labels.go`)

EvaluationResult Initialization (`evaluator.go`)