[Repo Assist] refactor(cmd): add getDefault helpers for all DIFC flag env vars#2569
Conversation
Several DIFC flags were using os.Getenv() directly in the init() function instead of following the documented getDefault*() helper pattern. Additionally, the table in flags.go referenced getDefaultDIFCSinkServerIDs() which did not exist. This change: - Adds getDefault helpers for all remaining DIFC flags: getDefaultDIFCSinkServerIDs(), getDefaultGuardPolicyJSON(), getDefaultAllowOnlyOwner(), getDefaultAllowOnlyRepo(), getDefaultAllowOnlyMinIntegrity() - Updates init() to use these helpers consistently - Updates the helper table in flags.go to include all helpers, including the previously undocumented getDefaultAllowOnlyScopePublic() - Simplifies tests to call the helper functions directly and use t.Setenv() for automatic cleanup Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Refactors DIFC-related CLI flag default handling to consistently use getDefault*() helpers (envutil-backed), and updates documentation/tests to match the helper pattern described in internal/cmd/flags.go.
Changes:
- Added missing DIFC
getDefault*()helpers (sink server IDs, guard policy JSON, allow-only scope owner/repo/min-integrity) and updated DIFC flag registration to use them. - Expanded the
flags.gohelper table to document all currently available helpers. - Updated DIFC flag tests to validate the helpers directly using
t.Setenv().
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/cmd/flags_difc.go | Adds new DIFC env-backed default helpers and wires them into flag registration. |
| internal/cmd/flags.go | Updates the documented table of getDefault*() helpers/env vars to reflect reality. |
| internal/cmd/flags_difc_test.go | Adjusts tests to exercise the new helpers directly and uses t.Setenv() for env isolation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // DIFC flag defaults | ||
| const ( | ||
| defaultAllowOnlyMinIntegrity = "" | ||
| ) |
There was a problem hiding this comment.
defaultAllowOnlyMinIntegrity is defined as an empty string and only used once; this constant doesn’t add meaning and slightly increases indirection. Consider inlining the empty string default, or (if you expect non-empty defaults later) defining defaults consistently for all new DIFC env-backed string flags in this file.
| if tt.setEnv { | ||
| t.Setenv("MCP_GATEWAY_GUARDS_SINK_SERVER_IDS", tt.envValue) | ||
| } else { | ||
| t.Setenv("MCP_GATEWAY_GUARDS_SINK_SERVER_IDS", "") |
There was a problem hiding this comment.
In TestGetDefaultDIFCSinkServerIDs, the “no env var” case currently sets the env var to an empty string, which makes it behaviorally identical to the “empty env var” case and doesn’t actually exercise the unset path. Consider either removing one of the redundant cases, or explicitly unsetting the variable for the “no env var” case (with a t.Cleanup restore) so the test differentiates unset vs empty if defaults ever change.
| t.Setenv("MCP_GATEWAY_GUARDS_SINK_SERVER_IDS", "") | |
| // Ensure the env var is truly unset for this test case. | |
| const envKey = "MCP_GATEWAY_GUARDS_SINK_SERVER_IDS" | |
| origValue, hadEnv := os.LookupEnv(envKey) | |
| // Restore previous state after the subtest completes. | |
| t.Cleanup(func() { | |
| if hadEnv { | |
| _ = os.Setenv(envKey, origValue) | |
| } else { | |
| _ = os.Unsetenv(envKey) | |
| } | |
| }) | |
| _ = os.Unsetenv(envKey) |
🤖 This PR was created by Repo Assist, an automated AI assistant.
Summary
Several DIFC flags in
internal/cmd/flags_difc.gousedos.Getenv()directly in theinit()function instead of following thegetDefault*()helper pattern documented inflags.go. Additionally, the helper table inflags.goreferencedgetDefaultDIFCSinkServerIDs()which did not exist — a stale documentation error.Changes
internal/cmd/flags_difc.gogetDefaultDIFCSinkServerIDs()(was referenced inflags.gotable but missing)getDefaultGuardPolicyJSON()getDefaultAllowOnlyOwner()getDefaultAllowOnlyRepo()getDefaultAllowOnlyMinIntegrity()init()to use all helper functions consistentlyinternal/cmd/flags.gogetDefault*()helpers, includinggetDefaultAllowOnlyScopePublic()which existed but was not documentedinternal/cmd/flags_difc_test.goTestGetDefaultDIFCSinkServerIDsto callgetDefaultDIFCSinkServerIDs()directly (previously testedos.Getenv()directly, not the helper)TestGetDefaultGuardPolicyInputsto call all new helpers, usingt.Setenv()for cleaner automatic cleanupRationale
The
getDefault*()pattern exists precisely to make each flag's environment variable override explicit and testable. Flags bypassing this pattern:This is a pure refactor — no behavioural changes.
envutil.GetEnvString("X", "")is equivalent toos.Getenv("X")for string flags (both return empty string as default), and the helpers forowner,repo,min-integrity, andpolicy-jsonall default to"".Test Status
proxy.golang.orgis blocked), somake agent-finishedcould not be run. The changes are a pure refactor with no logic changes, and the updated tests exercise the new helper functions directly usingt.Setenv()for safe parallelism.The PR CI should run full tests in the normal build environment.
Warning
The following domain was blocked by the firewall during workflow execution:
proxy.golang.orgTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.
Note
🔒 Integrity filter blocked 14 items
The following items were blocked because they don't meet the GitHub integrity level.
list_pull_requests: has lower integrity than agent requires. The agent cannot read data with integrity below "merged".list_pull_requests: has lower integrity than agent requires. The agent cannot read data with integrity below "merged".list_issues: has lower integrity than agent requires. The agent cannot read data with integrity below "merged".list_issues: has lower integrity than agent requires. The agent cannot read data with integrity below "merged".list_issues: has lower integrity than agent requires. The agent cannot read data with integrity below "merged".list_issues: has lower integrity than agent requires. The agent cannot read data with integrity below "merged".list_issues: has lower integrity than agent requires. The agent cannot read data with integrity below "merged".list_issues: has lower integrity than agent requires. The agent cannot read data with integrity below "merged".list_issues: has lower integrity than agent requires. The agent cannot read data with integrity below "merged".list_issues: has lower integrity than agent requires. The agent cannot read data with integrity below "merged".list_issues: has lower integrity than agent requires. The agent cannot read data with integrity below "merged".list_issues: has lower integrity than agent requires. The agent cannot read data with integrity below "merged".search_issues: has lower integrity than agent requires. The agent cannot read data with integrity below "merged".issue_read: has lower integrity than agent requires. The agent cannot read data with integrity below "merged".To allow these resources, lower
min-integrityin your GitHub frontmatter: