cleanup: remove guard CLI flags, auto-detect DIFC from policies#1723
Merged
cleanup: remove guard CLI flags, auto-detect DIFC from policies#1723
Conversation
Remove --enable-guards, --enable-config-extensions, --session-secrecy, and --session-integrity CLI flags. Guards are now auto-enabled when an allow-only policy is detected in the configuration. Changes: - Remove EnableDIFC field from config.Config - Remove SessionConfig struct and session label parsing - Remove enableDIFC, enableConfigExt, sessionSecrecy, sessionIntegrity flag variables and their env var defaults - Auto-detect DIFC in NewUnified() after guard registration: enabled when any non-noop guard or global policy override is present - Add HasNonNoopGuard() to guard.Registry for auto-detection - Move guards status logging into NewUnified() (before backend launch) - Always skip strict JSON schema validation for extension fields - Change requireGuardPolicyIfGuardEnabled to warn+noop fallback instead of fatal error - Fix flaky integration tests: replace hardcoded ports with dynamic allocation via getFreePort(t) - Update guard-policies docs to use canonical allow-only format - Update scripts to remove all references to removed flags - Update DIFC_RULES.md, config.example.toml, OVERVIEW.md Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR simplifies DIFC/guards configuration by removing the legacy “enable guards/config extensions/session labels” flags and shifting DIFC enablement to automatic detection based on configured guards/policies. It also updates tests, scripts, and documentation to align with the new behavior.
Changes:
- Remove
--enable-guards,--enable-config-extensions, and session label flags/env vars; remove related config fields. - Auto-detect DIFC enablement in
NewUnified()based on registered guards/policy overrides; add registry helperHasNonNoopGuard(). - Update integration/unit tests and docs/scripts; make integration tests less flaky by using dynamic ports.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/integration/difc_config_test.go | Removes coverage for deleted env vars/flags; uses dynamic ports; updates assertions to new auto-enable behavior. |
| internal/server/unified_test.go | Updates tests after removing EnableDIFC; simplifies edge-case cases. |
| internal/server/unified.go | Removes config-driven DIFC enable/session labels; adds DIFC auto-enable logic and policy-required fallback-to-noop behavior. |
| internal/server/routed_test.go | Updates DIFC enabled tests to use guard policy (auto-enable) instead of EnableDIFC. |
| internal/server/register_tools_from_backend_test.go | Removes EnableDIFC from test configs to match new config shape. |
| internal/server/label_agent_test.go | Removes EnableDIFC usage; relies on guard policy/guard-policies to trigger DIFC where needed. |
| internal/server/call_backend_tool_test.go | Removes EnableDIFC usage from test configs. |
| internal/guard/registry.go | Adds HasNonNoopGuard() helper for DIFC auto-detection. |
| internal/envutil/envutil_test.go | Replaces removed env var test (MCP_GATEWAY_ENABLE_GUARDS) with allow-only scope env var. |
| internal/difc/DIFC_RULES.md | Updates CLI/env var docs and examples to reflect auto-enable and allow-only policy format. |
| internal/config/validation_test.go | Adjusts expectations for stdin validation after schema validation behavior change. |
| internal/config/config_test.go | Updates assertions/error expectations related to stdin loading behavior changes. |
| internal/config/config_stdin.go | Removes config-extensions gating and session parsing; skips strict schema validation and logs that choice. |
| internal/config/config_difc_test.go | Removes session label parsing tests; updates extension-field acceptance tests to no longer require a flag. |
| internal/config/config_core.go | Removes EnableDIFC and Gateway.Session/SessionConfig from core config types. |
| internal/cmd/root.go | Removes propagation/validation for removed flags and session label wiring; retains DIFC mode validation and policy override resolution. |
| internal/cmd/flags_difc_test.go | Removes tests for deleted defaults/session label parsing/config extensions helpers. |
| internal/cmd/flags_difc.go | Removes deleted flags and their default/env helpers; keeps DIFC mode/sink/policy flags. |
| internal/cmd/flags.go | Updates documentation comment to reflect current DIFC env default helpers. |
| guards/github-guard/scripts/run_integration_tests.sh | Removes deleted CLI args/env vars from integration script. |
| guards/github-guard/scripts/run_copilot_test.sh | Removes deleted CLI args/env vars from copilot test script modes. |
| guards/github-guard/docs/OVERVIEW.md | Removes deleted env vars from docs examples. |
| config.example.toml | Updates guard-policies example to canonical allow-only key and removes enable_guards mention. |
| README.md | Updates guard-policies examples/docs to canonical allow-only format. |
Comments suppressed due to low confidence (2)
internal/server/unified_test.go:188
TestRequireSessionstill claims to cover behavior “even with DIFC enabled”, but the test config no longer enables DIFC (no policy / non-noop guard), so it’s currently exercising the disabled/default path and overlaps withTestRequireSession_DifcDisabled. Consider either (a) updating the assertions/messages to not mention DIFC, or (b) enabling DIFC in this test via a guard policy so both enabled/disabled cases remain covered.
func TestRequireSession(t *testing.T) {
cfg := &config.Config{
Servers: map[string]*config.ServerConfig{},
}
ctx := context.Background()
us, err := NewUnified(ctx, cfg)
require.NoError(t, err, "NewUnified() failed")
defer us.Close()
// Create a session
sessionID := "valid-session"
us.sessionMu.Lock()
us.sessions[sessionID] = NewSession(sessionID, "token")
us.sessionMu.Unlock()
// Test with valid session
ctxWithSession := context.WithValue(ctx, SessionIDContextKey, sessionID)
err = us.requireSession(ctxWithSession)
assert.NoError(t, err, "requireSession() failed for valid session")
// Test with non-existent session - should auto-create even with DIFC enabled
ctxWithNewSession := context.WithValue(ctx, SessionIDContextKey, "new-session")
err = us.requireSession(ctxWithNewSession)
require.NoError(t, err, "requireSession() should auto-create session even when DIFC is enabled")
internal/server/unified.go:195
- When logging the enabled guards mode, this uses
cfg.DIFCModedirectly; in tests/other call sitesDIFCModecan be empty which results in a blank mode in logs even though strict is the effective default. Consider logging the resolved/effective mode (e.g., defaulting to strict when empty, or using the evaluator’s mode).
// Log guards status early (before backend launch which may take time)
if us.enableDIFC {
log.Printf("Guards enforcement enabled with mode: %s", cfg.DIFCMode)
} else {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/config/config_stdin.go
Outdated
Comment on lines
187
to
191
| // Validate against JSON schema (fail-fast, spec-compliant) | ||
| // Skip schema validation when config extensions are enabled, as extensions like | ||
| // "guards", "guard", and "session" are not in the official schema yet. | ||
| if isConfigExtensionsEnabled() { | ||
| logConfig.Print("Config extensions enabled - skipping strict JSON schema validation") | ||
| log.Println("Config extensions enabled - schema validation relaxed for extension fields") | ||
| } else { | ||
| if err := validateJSONSchema(data); err != nil { | ||
| return nil, err | ||
| } | ||
| } | ||
| // Schema validation is skipped because extension fields like "guards", "guard", | ||
| // and "guard-policies" are not in the official schema yet. | ||
| logConfig.Print("Skipping strict JSON schema validation (extension fields may be present)") | ||
|
|
There was a problem hiding this comment.
LoadFromStdin now skips JSON schema validation unconditionally. This makes unsupported/typo fields silently accepted (e.g., a command field on stdio servers), which can lead to confusing failures later and reduces spec compliance. To keep extension-field support without dropping validation entirely, consider stripping known extension fields (guards/guard/guard-policies/etc.) from a copy of the JSON before schema validation, or explicitly validating/rejecting unknown fields for standard server types while still allowing known extensions.
Comment on lines
644
to
660
| name: "command field not supported", | ||
| config: `{ | ||
| "mcpServers": { | ||
| "test": { | ||
| "type": "stdio", | ||
| "command": "node", | ||
| "container": "test:latest" | ||
| } | ||
| }, | ||
| "gateway": { | ||
| "port": 8080, | ||
| "domain": "localhost", | ||
| "apiKey": "test-key" | ||
| } | ||
| }`, | ||
| shouldErr: true, | ||
| errorMsg: "validation error", | ||
| shouldErr: false, | ||
| }, |
There was a problem hiding this comment.
This case is named “command field not supported” but now expects no error, which codifies silent acceptance of an unsupported field. If the intent is to allow only specific extension fields, consider either rejecting command explicitly (with a targeted error) or renaming the test to reflect the new behavior (e.g., “unknown fields ignored”).
internal/server/unified.go
Outdated
Comment on lines
+186
to
+190
| // Auto-enable DIFC if any non-noop guard was registered or a global policy override exists | ||
| if !us.enableDIFC && (us.guardRegistry.HasNonNoopGuard() || cfg.GuardPolicy != nil) { | ||
| us.enableDIFC = true | ||
| logUnified.Printf("Auto-enabled DIFC: non-noop guard or policy detected") | ||
| } |
There was a problem hiding this comment.
Auto-enable logic only checks cfg.GuardPolicy (global override) or HasNonNoopGuard(), but does not consider per-server guard-policies when no non-noop guard ends up registered (e.g., guard missing/failed to load). This diverges from the PR description (“--guard-policy-json or per-server guard-policies”) and can leave DIFC disabled even though policies are present. Consider detecting policies by scanning cfg.Servers[*].GuardPolicies (or calling resolveGuardPolicy per server) and enabling DIFC if any resolve to a non-nil policy.
This issue also appears on line 192 of the same file.
Collaborator
Author
|
@copilot open a new pull request to apply changes based on the comments in this thread |
… DIFC auto-detection - Restore JSON schema validation in LoadFromStdin by stripping known extension fields (guards, per-server guard) before validating against the upstream schema; command field is now properly rejected again - Add 3 positive test cases verifying guard/guards/guard-policies pass - Restore original errorMsg values in tests that were loosened when schema validation was disabled (InvalidMountFormat, DirectCommand, invalid gateway port, command field not supported) - Add hasServerGuardPolicies() helper and use it in DIFC auto-detection so per-server guard-policies enable DIFC even without a non-noop guard Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Contributor
Applied all three changes from the review thread in commit 5d5c56a:
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:
|
This was referenced Mar 10, 2026
lpcox
added a commit
that referenced
this pull request
Mar 10, 2026
## Summary
Adds a daily agentic workflow (`smoke-allowonly.md`) that validates
AllowOnly guard policy enforcement through the MCP Gateway.
## Guard Policy
- **repos**: `["github/gh-aw*"]` (wildcard match)
- **min-integrity**: `approved` (OWNER, MEMBER, COLLABORATOR only)
## Test Coverage
### In-Scope Access (expected: allowed)
- `list_issues`, `list_pull_requests`, `list_commits`,
`get_file_contents`, `list_branches`, `search_code` against
`github/gh-aw-mcpg`
- `list_issues`, `get_file_contents` against `github/gh-aw`
### Out-of-Scope Blocking (expected: blocked)
- All repo-scoped calls against `octocat/Hello-World`
### Global API Blocking (expected: blocked)
- `search_repositories`, `search_users`
### Integrity Filtering (expected: filtered)
- Validates that content from non-approved authors is filtered
## Frontmatter
Uses `repos` and `min-integrity` fields directly on `tools.github` per
the gh-aw compiler format:
```yaml
tools:
github:
toolsets: [repos, issues, pull_requests, search]
repos: ["github/gh-aw*"]
min-integrity: approved
```
## Related
- PR #1723 (guard flag cleanup)
- PR #1726 (per-server guard mode)
- Issue #1711 (guards tracking)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Remove unnecessary guard CLI flags and simplify DIFC configuration. Guards are now auto-enabled when an allow-only policy is detected — no explicit flags needed.
Flags Removed
--enable-guardsMCP_GATEWAY_ENABLE_GUARDS--enable-config-extensionsMCP_GATEWAY_CONFIG_EXTENSIONS--session-secrecyMCP_GATEWAY_SESSION_SECRECY--session-integrityMCP_GATEWAY_SESSION_INTEGRITYHow DIFC Auto-Detection Works
After guard registration in
NewUnified(), the gateway checks:--guard-policy-json)?guard-policiesconfigured?If any condition is true, DIFC is automatically enabled. Otherwise, all servers get noop guards and DIFC remains disabled.
Other Changes
EnableDIFCfield fromconfig.ConfigandSessionConfigstructHasNonNoopGuard()toguard.Registryfor auto-detectionhasServerGuardPolicies()helper to detect per-server guard policiesrequireGuardPolicyIfGuardEnabled()now warns and falls back to noop instead of fatal errorLoadFromStdin: extension fields (guards, per-serverguard) are stripped from a copy before validation so spec-compliance is maintained while still accepting gateway-specific extensions; unsupported fields likecommandare properly rejectedgetFreePort(t))allow-onlyformatTesting
make agent-finishedpasses (format, build, lint, unit + integration tests)✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.