Conversation
…f < 0 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
supportsLLMGateway bool to llmGatewayPort int in BaseEngine
|
@copilot remove SupportsLLMGateway() it must always be true |
…ndatory Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in dccbed6. Removed |
supportsLLMGateway bool to llmGatewayPort int in BaseEnginesupportsLLMGateway bool to llmGatewayPort int and remove SupportsLLMGateway() from interface
…variable Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot UsesAPIProxy is alway true. Remove and simplify code using it |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
📰 BREAKING: Smoke Copilot is now investigating this pull request. Sources say the story is developing... |
|
🎬 THE END — Smoke Claude MISSION: ACCOMPLISHED! The hero saves the day! ✨ |
|
🌑 The shadows whisper... Smoke Codex failed. The oracle requires further meditation... |
Agent Container Tool Check
Result: 12/12 tools available ✅ Overall Status: PASS
|
|
Commit pushed:
|
|
Smoke test results for
Overall:
|
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
There was a problem hiding this comment.
💥 Automated smoke test review - all systems nominal! The rename from SupportsLLMGateway() bool to llmGatewayPort int is a clean simplification that removes an unnecessary abstraction layer.
💥 [THE END] — Illustrated by Smoke Claude
There was a problem hiding this comment.
Pull request overview
Renames per-engine LLM gateway support flag/method to a single llmGatewayPort int on BaseEngine, removes SupportsLLMGateway() from engine APIs, and makes AWF API-proxy/host-access flags unconditional.
Changes:
- Replace
supportsLLMGateway bool+SupportsLLMGateway()overrides withllmGatewayPort intonBaseEngineand registry-time validation. - Make AWF argument building always include
--enable-host-accessand--enable-api-proxy; removeUsesAPIProxyfromAWFCommandConfig. - Simplify strict-mode firewall validation/tests and ensure api-proxy image collection no longer depends on per-engine gateway support.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/agentic_engine.go | Reworks engine capability surface; introduces llmGatewayPort and registry validation. |
| pkg/workflow/awf_helpers.go | Removes UsesAPIProxy config and unconditionally enables host-access + api-proxy flags. |
| pkg/workflow/docker.go | Simplifies docker image collection for AWF sidecars (api-proxy). |
| pkg/workflow/strict_mode_validation.go | Removes engine lookup / LLM gateway branching from strict firewall validation. |
| pkg/workflow/strict_mode_llm_gateway_test.go | Updates strict-mode tests to align with new “general error” behavior. |
| pkg/workflow/claude_engine.go | Sets llmGatewayPort and removes per-engine SupportsLLMGateway() override/usage. |
| pkg/workflow/codex_engine.go | Sets llmGatewayPort and removes per-engine SupportsLLMGateway() override/usage. |
| pkg/workflow/copilot_engine.go | Sets llmGatewayPort and removes per-engine SupportsLLMGateway() override. |
| pkg/workflow/copilot_engine_execution.go | Removes conditional UsesAPIProxy wiring into AWF command build. |
| pkg/workflow/gemini_engine.go | Sets llmGatewayPort and removes per-engine SupportsLLMGateway() override/usage. |
| pkg/workflow/gemini_engine_test.go | Removes SupportsLLMGateway() assertion from capability tests. |
| .changeset/minor-rename-llmgateway-port.md | Adds changeset entry describing the rename and behavior changes. |
Comments suppressed due to low confidence (1)
pkg/workflow/agentic_engine.go:378
- The
llmGatewayPortvalidation only checks< 0, which allows the zero value (0) when an engine forgets to initialize the field. Since this value represents a network port (and built-in ports are positive), it would be safer to validate it’s within a valid port range (e.g., 1–65535) or at least> 0to catch misconfiguration early.
type portProvider interface{ getLLMGatewayPort() int }
if p, ok := engine.(portProvider); ok && p.getLLMGatewayPort() < 0 {
panic(fmt.Sprintf("engine '%s': llmGatewayPort must be >= 0, got %d", engine.GetID(), p.getLLMGatewayPort()))
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if workflowData != nil && workflowData.AI != "" { | ||
| registry := GetGlobalEngineRegistry() | ||
| engine, err := registry.GetEngine(workflowData.AI) | ||
| if err == nil && engine.SupportsLLMGateway() > 0 { | ||
| apiProxyImage := constants.DefaultFirewallRegistry + "/api-proxy:" + awfImageTag | ||
| if !imageSet[apiProxyImage] { | ||
| images = append(images, apiProxyImage) | ||
| imageSet[apiProxyImage] = true | ||
| dockerLog.Printf("Added AWF api-proxy sidecar container for engine with LLM gateway support: %s", apiProxyImage) | ||
| } | ||
| apiProxyImage := constants.DefaultFirewallRegistry + "/api-proxy:" + awfImageTag | ||
| if !imageSet[apiProxyImage] { | ||
| images = append(images, apiProxyImage) | ||
| imageSet[apiProxyImage] = true | ||
| dockerLog.Printf("Added AWF api-proxy sidecar container: %s", apiProxyImage) | ||
| } | ||
| } |
There was a problem hiding this comment.
collectDockerImages only adds the api-proxy sidecar image when workflowData.AI != "". However, workflows that rely on the default engine can legitimately have AI == "" (the compiler falls back to GetDefaultEngine()), while AWF args now always include --enable-api-proxy and --skip-pull. In that case the workflow can fail at runtime because the required api-proxy image was never pre-downloaded. Consider removing the workflowData.AI != "" guard (or basing the decision on whether AWF will be invoked) so api-proxy is always included whenever the firewall/AWF is enabled.
See below for a potential fix:
apiProxyImage := constants.DefaultFirewallRegistry + "/api-proxy:" + awfImageTag
if !imageSet[apiProxyImage] {
images = append(images, apiProxyImage)
imageSet[apiProxyImage] = true
dockerLog.Printf("Added AWF api-proxy sidecar container: %s", apiProxyImage)
| func (r *EngineRegistry) Register(engine CodingAgentEngine) { | ||
| type portProvider interface{ getLLMGatewayPort() int } | ||
| if p, ok := engine.(portProvider); ok && p.getLLMGatewayPort() < 0 { | ||
| panic(fmt.Sprintf("engine '%s': llmGatewayPort must be >= 0, got %d", engine.GetID(), p.getLLMGatewayPort())) | ||
| } |
There was a problem hiding this comment.
Register validates llmGatewayPort only when the engine satisfies an interface with an unexported method (getLLMGatewayPort). Engines implemented outside the workflow package can’t implement that method, so the assertion will always fail and the validation will be skipped. If llmGatewayPort is mandatory, consider making the port accessor exported (or part of a public optional interface) and/or treating “missing port provider” as an error so invalid/misconfigured engines can’t be registered silently.
This issue also appears on line 375 of the same file.
BaseEnginestruct: replacesupportsLLMGateway boolwithllmGatewayPort intSupportsLLMGateway()overridesSupportsLLMGateway() intfrom theCodingAgentEngineinterfaceBaseEngine.SupportsLLMGateway()method; add unexportedgetLLMGatewayPort()forRegister()validationUsesAPIProxyfield fromAWFCommandConfigentirely —--enable-host-accessand--enable-api-proxyare now added unconditionally inBuildAWFArgsdocker.go: remove conditional registry lookupstrict_mode_validation.go: remove deadllmGatewayPort < 0branch and unusedGetEnginecallTestSupportsLLMGateway, removeSupportsLLMGateway()assertionOriginal prompt
🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.
Changeset
supportsLLMGatewayflag tollmGatewayPort, required/validated the gateway port, removed theSupportsLLMGatewayinterface hooks, and consolidated the API proxy/host-access workflow flags.Warning
The following domain was blocked by the firewall during workflow execution:
github.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.
✨ PR Review Safe Output Test - Run 22522442966