Fix compiler to pass --enable-opencode to AWF when engine is opencode#29431
Fix compiler to pass --enable-opencode to AWF when engine is opencode#29431
Conversation
|
@copilot look at this and make sure that enable-opencode is also passed through the stdin config file github/gh-aw-firewall#2337 (comment) |
…w-firewall#2337) When engine=opencode, BuildAWFArgs() now: - Emits --enable-opencode (gated on AWF >= v0.25.30) - Adds port 10004 to --allow-host-ports for the OpenCode API proxy listener Both behaviors are gated behind awfSupportsEnableOpenCode() which checks the AWFEnableOpenCodeMinVersion constant (v0.25.30). This follows the same pattern as awfSupportsCliProxy and awfSupportsAllowHostPorts. Also adds OpenCode to TestEngineAWFEnableApiProxy in enable_api_proxy_test.go. Agent-Logs-Url: https://github.com/github/gh-aw/sessions/0f37451d-6492-43cd-89cf-150affc954f4 Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
🧪 Test Quality Sentinel ReportTest Quality Score: 90/100✅ Excellent test quality
Test Classification DetailsView All Test Classifications (3 tests)
Test Details
|
lpcox
left a comment
lpcox
left a comment
There was a problem hiding this comment.
Review
Overall this is a solid implementation that follows existing patterns well. A few items to address:
🔴 Port 10004 should be a constant
All other LLM gateway ports are defined in pkg/constants/constants.go:
ClaudeLLMGatewayPort = 10000
CodexLLMGatewayPort = 10001
CopilotLLMGatewayPort = 10002
GeminiLLMGatewayPort = 10003Port 10004 is hardcoded as a string literal in both awf_helpers.go and the test file. Add:
// OpenCodeLLMGatewayPort is the port for the OpenCode LLM gateway
OpenCodeLLMGatewayPort = 10004Then reference constants.OpenCodeLLMGatewayPort instead of the magic number.
🟡 Missing spec_test.go entry
Existing AWF min version constants (AWFExcludeEnvMinVersion, AWFCliProxyMinVersion) have entries in pkg/constants/spec_test.go. The new AWFEnableOpenCodeMinVersion should be added there too for consistency.
🟡 Version v0.25.30 is speculative
Firewall PR #2337 hasn't been released yet. If it ships as a different version, this constant will be wrong. Consider adding a comment noting this dependency, e.g.:
// NOTE: version must be updated to match the actual release containing gh-aw-firewall#2337
const AWFEnableOpenCodeMinVersion Version = "v0.25.30"ℹ️ No lock file impact yet (expected)
Since DefaultFirewallVersion is still v0.25.29, the flag won't be emitted for any compiled workflows until the default is bumped. This is correct — a follow-up version bump PR will activate the feature.
✅ What looks good
opencodeEnabledpre-computed once — clean, no duplicate checks- Version gating mirrors
awfSupportsAllowHostPortspattern exactly - Correct observation that
AWFEnableOpenCodeMinVersion > AWFAllowHostPortsMinVersionso port support is guaranteed - Test coverage is thorough: default/old/latest versions, non-semver, custom MCP port, all non-opencode engines
- The
enable_api_proxy_test.goaddition for OpenCode is correct (empty model won't panic —extractProviderFromModel("")returns"", nil)
Records the architectural decision to emit --enable-opencode and port 10004 only when the workflow engine is opencode and AWF version >= v0.25.30. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Commit pushed:
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (267 new lines in AI has analyzed the PR diff and generated a draft ADR to help you get started: 📄 Draft ADR: What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. What the draft coversThe draft ADR captures the following from the diff:
Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
References: §25201347429
|
Adds apiProxy.enableOpenCode: true to the AWF JSON config file when engine=opencode and AWF >= v0.25.30, alongside the existing --enable-opencode CLI flag in BuildAWFArgs(). This follows the established pattern where --enable-api-proxy is expressed as apiProxy.enabled in the config file. - AWFAPIProxyConfig gains EnableOpenCode bool (omitempty) - BuildAWFConfigJSON sets it for opencode engine (gated on awfSupportsEnableOpenCode) - awf_config_test.go covers: enabled for opencode+new AWF, absent for other engines, absent for old AWF version - BuildAWFArgs comment updated to document the config-file mapping Agent-Logs-Url: https://github.com/github/gh-aw/sessions/2900a3d6-1809-4cbc-a64e-9dda48d019a3 Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Done in commit 1a0ed51. Added |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
💥 Automated smoke test review — all systems nominal! Minor documentation suggestions on the version constant and the omitempty JSON tag rationale.
💥 [THE END] — Illustrated by Smoke Claude · ● 340.4K
| // on port 10004 (dynamic provider routing). Workflows pinning an older AWF | ||
| // version must not emit --enable-opencode or the run will fail at startup. | ||
| // Introduced in gh-aw-firewall PR #2337. | ||
| const AWFEnableOpenCodeMinVersion Version = "v0.25.30" |
There was a problem hiding this comment.
Good use of a named constant for the version threshold. One suggestion: consider adding a link to the specific AWF release notes or changelog entry (not just the PR number) so future maintainers can quickly verify what changed in that version without needing access to the internal firewall repo.
| // Maps to: --enable-api-proxy | ||
| Enabled bool `json:"enabled"` | ||
|
|
||
| // EnableOpenCode enables the OpenCode API proxy listener on port 10004 |
There was a problem hiding this comment.
The omitempty tag is correct here — this avoids emitting "enableOpenCode":false in the JSON config for non-opencode engines. Worth adding a note in the comment that this field is intentionally omitted (not just false) when opencode is not active, to avoid confusion for anyone reading the generated AWF config files.
When
engine: opencodeis configured, the compiler now fully enables the OpenCode API proxy listener (port 10004) introduced in gh-aw-firewall#2337.Changes Made
pkg/constants/version_constants.go: AddedAWFEnableOpenCodeMinVersion = "v0.25.30"— minimum AWF version that supports the new flag.pkg/workflow/awf_helpers.go: AddedawfSupportsEnableOpenCode()version-gate function following the same pattern asawfSupportsCliProxy/awfSupportsAllowHostPorts. ModifiedBuildAWFArgs()to emit--enable-opencodeand include port 10004 in--allow-host-portsfor the opencode engine (both gated on AWF ≥ v0.25.30).pkg/workflow/awf_config.go: AddedEnableOpenCode bool(json:"enableOpenCode,omitempty") toAWFAPIProxyConfig.BuildAWFConfigJSON()now setsapiProxy.enableOpenCode: truefor the opencode engine (gated on AWF ≥ v0.25.30), following the same pattern asapiProxy.enabledfor--enable-api-proxy.Testing
pkg/workflow/awf_helpers_test.go: AddedTestBuildAWFArgsEnableOpenCode(flag emission, non-OpenCode engines, version gating,latest, custom MCP port) andTestAWFSupportsEnableOpenCode(version-gate edge cases).pkg/workflow/awf_config_test.go: Added tests verifyingenableOpenCodeis present in the config JSON for opencode+new AWF, absent for other engines, and absent for old AWF versions.pkg/workflow/enable_api_proxy_test.go: Added OpenCode toTestEngineAWFEnableApiProxy.Version Gating
AWFEnableOpenCodeMinVersion(v0.25.30) > currentDefaultFirewallVersion(v0.25.29), so the flag and config field are a no-op untilDefaultFirewallVersionis bumped in a follow-up once gh-aw-firewall#2337 ships.