Conversation
4 tasks
…ant engine overrides Agent-Logs-Url: https://github.com/github/gh-aw/sessions/cb6ca7ad-6cbb-46ed-8dcf-f3468b8416e2 Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Refactor duplicate and outlier functions in Go source files
refactor: consolidate duplicate provider extraction and remove redundant engine overrides
Apr 27, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors pkg/workflow to remove duplicate provider parsing logic and eliminate redundant engine method overrides while keeping existing behavior centralized in shared helpers.
Changes:
- Consolidates OpenCode/Crush model provider parsing into a single
extractProviderFromModelhelper and updates call sites. - Updates the associated unit test to use the consolidated helper.
- Removes redundant
GetFirewallLogsCollectionStepoverrides from Claude/Codex engines and documents the default behavior inBaseEngine.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/domains.go | Replaces duplicate provider extraction helpers with one shared function and updates domain selection call sites/comments. |
| pkg/workflow/crush_engine_test.go | Updates provider-extraction tests to target the shared helper. |
| pkg/workflow/codex_engine.go | Removes redundant GetFirewallLogsCollectionStep override (now inherits BaseEngine behavior). |
| pkg/workflow/claude_engine.go | Removes redundant GetFirewallLogsCollectionStep override (now inherits BaseEngine behavior). |
| pkg/workflow/agentic_engine.go | Centralizes rationale for the default empty firewall log collection step in BaseEngine. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 1
Comment on lines
+189
to
195
| // extractProviderFromModel parses "provider/model" format and returns the | ||
| // lowercase provider prefix; returns "copilot" when the format is absent. | ||
| // Both OpenCode and Crush use this same "provider/model" convention. | ||
| func extractProviderFromModel(model string) string { | ||
| if model == "" { | ||
| return "copilot" | ||
| } |
There was a problem hiding this comment.
extractProviderFromModel can return an empty provider for inputs like "/gpt-4.1" (leading slash). That will prevent any provider-specific domain from being added, and may cause unexpected firewall blocks. Consider treating an empty provider prefix as invalid and falling back to the default ("copilot"), and add a small test case to cover this.
Closed
Contributor
🧪 Test Quality Sentinel ReportTest Quality Score: 90/100✅ Excellent test quality
Test Classification DetailsAll changes in 📋 Modified Subtests (4)
Quality NotesThe renamed function tests verify observable behavioral contracts:
These are all edge/boundary cases that would catch behavioral regressions. Minor (non-blocking): Assertions use bare Build Tag Check✅ Language SupportTests analyzed:
Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
References: §25006617949
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Collaborator
|
@copilot review all comments, add tests |
… add test Agent-Logs-Url: https://github.com/github/gh-aw/sessions/aee5f435-c151-434e-9043-2de18236b869 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Contributor
Author
Collaborator
|
@copilot no fallback provider, raise a compilation error |
… extractProviderFromModel Agent-Logs-Url: https://github.com/github/gh-aw/sessions/974d4531-6166-43e4-b6ee-a4153cc5cb44 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Contributor
Author
Done in 3418a4c. 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:
|
Closed
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.
Two pairs of dead/duplicate code identified by semantic function clustering analysis in
pkg/workflow.Consolidate
extractProviderFromModel(domains.go)extractOpenCodeProviderFromModelandextractCrushProviderFromModelwere byte-for-byte identical. Replaced both with a single helper:A leading slash like
/gpt-4.1(empty provider prefix) is now a compilation error rather than a silent fallback. The error propagates through the full compilation pipeline:GetOpenCodeDefaultDomains/GetCrushDefaultDomains→([]string, error)GetAllowedDomainsForEngineWithModeland the two publicWithToolsAndRuntimeswrappers →(string, error)computeAllowedDomainsForSanitization/computeExpandedAllowedDomainsForSanitization→(string, error), surfacing through the activation job builder, safe-outputs steps, and YAML main-job builderGetExecutionStepscallers (crush/opencode) guard with apanicsince the model is already validated before reaching domain computationGetAllowedDomainsForEngine(no-model variant, used for Copilot/Claude/Codex/Gemini) is unchanged — an empty model can never trigger the provider-prefix error.Updated both call sites (
GetOpenCodeDefaultDomains,GetCrushDefaultDomains), stale doc-comments on the two provider-domain maps, and the tests.Remove redundant
GetFirewallLogsCollectionStepoverridesClaudeEngineandCodexEngineboth overrodeGetFirewallLogsCollectionStepwith bodies identical toBaseEngine(return []GitHubActionStep{}). Deleted both overrides; moved the consolidated rationale comment toBaseEngineinagentic_engine.go.