Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Removes unused domain helper wrappers and updates tests to use the shared GetAllowedDomainsForEngine API, while also deleting additional unused workflow helper functions.
Changes:
- Deleted engine-specific allowed-domain wrapper functions in
domains.goand updated test callers to useGetAllowedDomainsForEngine. - Removed several unused validation/map/engine helper functions and their associated tests.
- Updated tests to import/use
pkg/constantsengine identifiers where needed.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/domains.go | Removes dead domain wrapper helpers; keeps engine-based API as the primary entry point. |
| pkg/workflow/domains_protocol_test.go | Updates protocol-domain tests to call GetAllowedDomainsForEngine and adds constants import. |
| pkg/workflow/domains_sort_test.go | Updates sorting tests to call GetAllowedDomainsForEngine and adds constants import. |
| pkg/workflow/domains_test.go | Removes tests that targeted deleted wrapper functions. |
| pkg/workflow/http_mcp_domains_test.go | Updates HTTP MCP domain tests to use GetAllowedDomainsForEngine. |
| pkg/workflow/safe_inputs_firewall_test.go | Updates tests to use GetAllowedDomainsForEngine and imports constants. |
| pkg/workflow/validation_helpers.go | Removes unused exported validation helpers and related imports/docs. |
| pkg/workflow/validation_helpers_test.go | Removes tests for deleted validation/map helper functions. |
| pkg/workflow/map_helpers.go | Removes unused map helper utilities and updates file header/docs accordingly. |
| pkg/workflow/engine_helpers.go | Removes unused exported engine helper utilities. |
| pkg/workflow/engine_helpers_test.go | Removes tests for deleted engine helper utilities. |
Comments suppressed due to low confidence (2)
pkg/workflow/engine_helpers.go:275
- The docstring for GetNpmBinPathSetup references GetHostedToolcachePathSetup(), but that helper was removed in this PR. Please update the comment to reference the current/remaining PATH setup behavior (or remove the comparison) so the documentation doesn’t point to a non-existent function.
// GetNpmBinPathSetup returns a simple shell command that adds hostedtoolcache bin directories
// to PATH. This is specifically for npm-installed CLIs (like Claude and Codex) that need
// to find their binaries installed via `npm install -g`.
//
// Unlike GetHostedToolcachePathSetup(), this does NOT use GH_AW_TOOL_BINS because AWF's
// native chroot mode already handles tool-specific paths (GOROOT, JAVA_HOME, etc.) via
// AWF_HOST_PATH and the entrypoint.sh script. This function only adds the generic
// hostedtoolcache bin directories for npm packages.
pkg/workflow/engine_helpers.go:113
- PR title/description indicate this change is limited to removing dead domain helper functions, but this PR also removes other exported helpers (e.g. ExtractAgentIdentifier and several PATH/toolcache helpers). Please update the PR title/description (or split the changes) so the scope is accurately communicated for reviewers and release notes.
// ResolveAgentFilePath returns the properly quoted agent file path with GITHUB_WORKSPACE prefix.
// This helper extracts the common pattern shared by Copilot, Codex, and Claude engines.
//
// The agent file path is relative to the repository root, so we prefix it with ${GITHUB_WORKSPACE}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
61
to
67
| Allowed: []string{"github.com"}, | ||
| } | ||
|
|
||
| result := GetCopilotAllowedDomainsWithSafeInputs(network, true) | ||
| result := GetAllowedDomainsForEngine(constants.CopilotEngine, network, nil, nil) | ||
|
|
||
| if !strings.Contains(result, "host.docker.internal") { | ||
| t.Errorf("Expected result to contain 'host.docker.internal', got: %s", result) |
There was a problem hiding this comment.
These subtests are now calling GetAllowedDomainsForEngine directly, but the surrounding test/subtest names and comments still refer to “WithSafeInputs” / “backward compatibility with GetCopilotAllowedDomains”. Please rename/reword the test cases to match the new API and what’s actually being validated (e.g., Copilot defaults include host.docker.internal).
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.
Removes 10 dead functions from domains.go and updates callers in other test files.
Deleted functions (all were thin wrappers around the live GetAllowedDomainsForEngine):
Test updates — references in other test files replaced with GetAllowedDomainsForEngine calls:
Also removes 2 exclusive test functions from domains_test.go.
All kept tests pass. Part of the ongoing dead code removal series.