refactor: extract difc.NewComponents to deduplicate DIFC initialization#4498
Conversation
…quence Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/a30dca7f-6bef-4e04-8361-ebe4015c918f Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR centralizes DIFC initialization into a single helper to remove duplicated setup logic across the unified server and the GitHub API proxy, aiming to keep enforcement-mode defaults consistent and easier to evolve.
Changes:
- Added
difc.DIFCComponentsanddifc.NewComponents(...)to encapsulate the standard DIFC initialization sequence. - Updated
internal/server/unified.goto usedifc.NewComponents(cfg.DIFCMode, difc.EnforcementStrict)instead of inline initialization. - Updated
internal/proxy/proxy.goto usedifc.NewComponents(cfg.DIFCMode, difc.EnforcementFilter)and preserve a proxy-specific warning for invalid mode strings.
Show a summary per file
| File | Description |
|---|---|
| internal/difc/evaluator.go | Introduces DIFCComponents + NewComponents constructor to standardize DIFC initialization. |
| internal/server/unified.go | Switches unified server DIFC setup to the new shared constructor. |
| internal/proxy/proxy.go | Switches proxy DIFC setup to the new shared constructor while keeping invalid-mode warning behavior. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
internal/difc/evaluator.go:113
NewComponentsintroduces new behavior (defaulting/initialization of AgentRegistry/Capabilities/Evaluator) but there are no unit tests covering it. Sinceinternal/difc/evaluator_test.goalready has comprehensive tests, please add tests that assert: (1) emptymodeStrusesdefaultMode, (2) invalidmodeStrusesdefaultMode, and (3) valid strings produce the expected mode and non-nil component pointers.
// NewComponents initializes the standard DIFC component set.
// modeStr is parsed via ParseEnforcementMode; defaultMode is used when modeStr
// is empty or invalid.
func NewComponents(modeStr string, defaultMode EnforcementMode) DIFCComponents {
mode, err := ParseEnforcementMode(modeStr)
if err != nil {
mode = defaultMode
}
return DIFCComponents{
Mode: mode,
AgentRegistry: NewAgentRegistryWithDefaults(nil, nil),
Capabilities: NewCapabilities(),
Evaluator: NewEvaluatorWithMode(mode),
}
- Files reviewed: 3/3 changed files
- Comments generated: 2
| // NewComponents initializes the standard DIFC component set. | ||
| // modeStr is parsed via ParseEnforcementMode; defaultMode is used when modeStr | ||
| // is empty or invalid. | ||
| func NewComponents(modeStr string, defaultMode EnforcementMode) DIFCComponents { | ||
| mode, err := ParseEnforcementMode(modeStr) | ||
| if err != nil { | ||
| mode = defaultMode | ||
| } | ||
| return DIFCComponents{ | ||
| Mode: mode, | ||
| AgentRegistry: NewAgentRegistryWithDefaults(nil, nil), | ||
| Capabilities: NewCapabilities(), | ||
| Evaluator: NewEvaluatorWithMode(mode), | ||
| } |
There was a problem hiding this comment.
NewComponents does not currently honor the documented behavior for empty modeStr. ParseEnforcementMode("") returns EnforcementStrict with no error, so modeStr == "" will never fall back to defaultMode. This changes proxy behavior: difc.NewComponents(cfg.DIFCMode, EnforcementFilter) will resolve to strict when cfg.DIFCMode is empty, instead of filter. Fix by handling modeStr == "" explicitly before calling ParseEnforcementMode (or adjust ParseEnforcementMode semantics and update callers), and keep the doc comment consistent with the actual behavior.
This issue also appears on line 100 of the same file.
| // Initialize DIFC components (defaults to filter mode for the proxy) | ||
| if cfg.DIFCMode != "" { | ||
| if _, err := difc.ParseEnforcementMode(cfg.DIFCMode); err != nil { | ||
| log.Printf("[proxy] WARNING: invalid DIFC mode %q, defaulting to filter", cfg.DIFCMode) | ||
| } | ||
| difcMode = difc.EnforcementFilter // default to filter for proxy | ||
| } | ||
| logProxy.Printf("Enforcement mode resolved: %s", difcMode) | ||
| difcComponents := difc.NewComponents(cfg.DIFCMode, difc.EnforcementFilter) | ||
| logProxy.Printf("Enforcement mode resolved: %s", difcComponents.Mode) |
There was a problem hiding this comment.
In proxy.New, enforcement mode parsing is now done twice: once to decide whether to log a warning and again inside difc.NewComponents. Consider changing NewComponents to also return the parse error (while still defaulting to defaultMode), so proxy can log based on that single parse and avoid duplicating the parsing logic.
|
@copilot address this review feedback #4498 (review) |
…ior; log warnings for invalid modes Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/00275f48-42cd-4498-8c01-f72ef31819a8 Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Addressed both points in d5126a1:
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:
If you need me to access, download, or install something from one of these locations, you can either:
|
The four-step DIFC initialization sequence (
ParseEnforcementMode→NewAgentRegistryWithDefaults→NewCapabilities→NewEvaluatorWithMode) was duplicated verbatim in bothserver/unified.goandproxy/proxy.go, with silently diverging default modes (EnforcementStrictvsEnforcementFilter).Changes
internal/difc/evaluator.go— AddsDIFCComponentsstruct andNewComponents(modeStr string, defaultMode EnforcementMode) DIFCComponentsconstructor that encapsulates the full initialization sequenceinternal/server/unified.go— Replaces inline DIFC init withdifc.NewComponents(cfg.DIFCMode, difc.EnforcementStrict)internal/proxy/proxy.go— Replaces inline DIFC init withdifc.NewComponents(cfg.DIFCMode, difc.EnforcementFilter), preserving the proxy-specific warning log for invalid mode stringsFuture changes to the DIFC initialization sequence (e.g. adding a new component) now have a single update point.
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-build2613986933/b513/launcher.test /tmp/go-build2613986933/b513/launcher.test -test.testlogfile=/tmp/go-build2613986933/b513/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build2613986933/b435/vet.cfg _.a -I x_amd64/vet --gdwarf-5 balancer/gracefu-atomic -o x_amd64/vet -W cks..." t.go x_amd64/compile . --gdwarf2 893947/b314/ x_amd64/compile(dns block)/tmp/go-build1963674460/b509/launcher.test /tmp/go-build1963674460/b509/launcher.test -test.testlogfile=/tmp/go-build1963674460/b509/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true tHjXILjQ_ -buildtags /opt/hostedtoolcache/go/1.25.9/xjson -errorsas -ifaceassert -nilfunc StwtP8fWF_W- logs�� tes.crt aw-mcpg/internal/proxy/graphql_r--log-format 7c330d9c11d84b8d-d -errorsas -ifaceassert -nilfunc ache/go/1.25.9/x-test.timeout=10m0s(dns block)invalid-host-that-does-not-exist-12345.com/tmp/go-build2613986933/b495/config.test /tmp/go-build2613986933/b495/config.test -test.testlogfile=/tmp/go-build2613986933/b495/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build2613986933/b368/vet.cfg _.a otection x_amd64/vet --gdwarf-5 credentials -o x_amd64/vet -E w6WVsrdoR -I x_amd64/vet -I .io/auto/sdk -imultiarch x_amd64/vet(dns block)/tmp/go-build1963674460/b491/config.test /tmp/go-build1963674460/b491/config.test -test.testlogfile=/tmp/go-build1963674460/b491/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true -unreachable=fal--log-format /tmp/go-build261json docker-buildx f19db7ad4b9e741c980b3140ec557422c5d/log.json nwind-tables p/bin/as docker-buildx -ato�� 40a26d5ba8f85386 -buildtags ine by/6e9cb3f11e119/usr/lib/open-iscsi/net-interface-handler -ifaceassert -nilfunc ine(dns block)nonexistent.local/tmp/go-build2613986933/b513/launcher.test /tmp/go-build2613986933/b513/launcher.test -test.testlogfile=/tmp/go-build2613986933/b513/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build2613986933/b435/vet.cfg _.a -I x_amd64/vet --gdwarf-5 balancer/gracefu-atomic -o x_amd64/vet -W cks..." t.go x_amd64/compile . --gdwarf2 893947/b314/ x_amd64/compile(dns block)/tmp/go-build1963674460/b509/launcher.test /tmp/go-build1963674460/b509/launcher.test -test.testlogfile=/tmp/go-build1963674460/b509/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true tHjXILjQ_ -buildtags /opt/hostedtoolcache/go/1.25.9/xjson -errorsas -ifaceassert -nilfunc StwtP8fWF_W- logs�� tes.crt aw-mcpg/internal/proxy/graphql_r--log-format 7c330d9c11d84b8d-d -errorsas -ifaceassert -nilfunc ache/go/1.25.9/x-test.timeout=10m0s(dns block)slow.example.com/tmp/go-build2613986933/b513/launcher.test /tmp/go-build2613986933/b513/launcher.test -test.testlogfile=/tmp/go-build2613986933/b513/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build2613986933/b435/vet.cfg _.a -I x_amd64/vet --gdwarf-5 balancer/gracefu-atomic -o x_amd64/vet -W cks..." t.go x_amd64/compile . --gdwarf2 893947/b314/ x_amd64/compile(dns block)/tmp/go-build1963674460/b509/launcher.test /tmp/go-build1963674460/b509/launcher.test -test.testlogfile=/tmp/go-build1963674460/b509/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true tHjXILjQ_ -buildtags /opt/hostedtoolcache/go/1.25.9/xjson -errorsas -ifaceassert -nilfunc StwtP8fWF_W- logs�� tes.crt aw-mcpg/internal/proxy/graphql_r--log-format 7c330d9c11d84b8d-d -errorsas -ifaceassert -nilfunc ache/go/1.25.9/x-test.timeout=10m0s(dns block)this-host-does-not-exist-12345.com/tmp/go-build2613986933/b522/mcp.test /tmp/go-build2613986933/b522/mcp.test -test.testlogfile=/tmp/go-build2613986933/b522/testlog.txt -test.paniconexit0 -test.timeout=10m0s 8939�� cfg elemetry.io/otel-ifaceassert x_amd64/vet --gdwarf-5 telabs/wazero/in/usr/bin/runc -o x_amd64/vet cfg 893947/b432/_pkg_.a -trimpath x_amd64/vet -p /internal/httpco/usr/bin/runc -lang=go1.17 x_amd64/vet(dns block)/tmp/go-build1963674460/b518/mcp.test /tmp/go-build1963674460/b518/mcp.test -test.testlogfile=/tmp/go-build1963674460/b518/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true y -buildtags(dns block)If you need me to access, download, or install something from one of these locations, you can either: