Preserve explicit HTTP schemes in AWF API proxy targets - #58608
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Release metadata and exported API documentation must be updated for the new target-host behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Preserves explicit HTTP schemes in AWF API proxy targets while maintaining backward compatibility.
Changes:
- Preserves
http://for OpenAI, Anthropic, and Gemini targets on AWF v0.28.13+. - Keeps older AWF versions, HTTPS, scheme-less, allowlist, and Copilot targets unchanged.
- Adds provider and version-gating tests.
File summaries
| File | Description |
|---|---|
pkg/workflow/engine_api_targets.go |
Adds scheme-aware proxy target extraction. |
pkg/workflow/awf_feature_flags.go |
Adds the AWF capability gate. |
pkg/workflow/awf_config_test.go |
Tests providers, schemes, and versions. |
pkg/workflow/awf_config_build.go |
Uses scheme-aware targets in AWF configuration. |
pkg/constants/version_constants.go |
Defines v0.28.13 as the minimum version. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Balanced
|
|
||
| // extractAPIProxyTargetHost returns the target host format expected by the | ||
| // effective AWF version, preserving an explicit http:// scheme when supported. | ||
| func extractAPIProxyTargetHost(workflowData *WorkflowData, envVar string, firewallConfig *FirewallConfig) string { |
| if strings.HasPrefix(workflowData.EngineConfig.Env[envVar], "http://") { | ||
| return "http://" + host |
@copilot Please take the next forward-progress pass on this PR.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
❌ Ponytail Reviewer failed. Please review the logs for details. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
|
|
❌ Design Decision Gate 🏗️ failed to deliver outputs during design decision gate check.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
REQUEST_CHANGES
The fix is headed in the right direction, but it still breaks a real class of HTTP endpoints by dropping explicit ports when rebuilding the target host.
Blocking theme
The new extractAPIProxyTargetHost path preserves (redacted) but reconstructs the target from a helper that has already normalized the URL. That means (host/redacted) turns into `(host/redacted) which silently changes the destination port and defeats the purpose of preserving explicit HTTP targets for local or internal gateways.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 34.5 AIC · ⌖ 7.38 AIC · ⊞ 23.5K
Comment /review to run again
| return host | ||
| } | ||
|
|
||
| if strings.HasPrefix(workflowData.EngineConfig.Env[envVar], "http://") { |
There was a problem hiding this comment.
This helper still rebuilds the target as "(redacted) + host, so any explicit non-default port in OPENAI_BASE_URL/ANTHROPIC_BASE_URL/GEMINI_API_BASE_URL is lost before it reaches AWF.
💡 Why this blocks the fix
extractAPITargetHost strips the path and normalizes the authority down to the bare host string. With an input like (localhost/redacted) this branch emits (localhost/redacted) instead of `(localhost/redacted) so the proxy silently reconnects to port 80 and breaks the exact local/internal gateway setups this PR is trying to restore.
Please parse the URL once and preserve `(host/redacted) for explicit HTTP targets, or return the original authority component instead of reconstructing it from the already-stripped host.
There was a problem hiding this comment.
Added regression coverage for http://openai-gateway.example.com:8080/v1, confirming the compiler preserves the explicit http:// scheme and :8080 authority in the emitted AWF target. AWF may still normalize that port at runtime, as documented on AWFAPITargetConfig.Host.
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Updated the exported |
There was a problem hiding this comment.
Backend-only change (AWF proxy target host/scheme handling in pkg/workflow and pkg/constants); no UI surface, so Impeccable audit/critique/harden modes do not apply here.
Reviewed the diff for correctness/security: extractAPIProxyTargetHost safely reuses the already-validated extractAPITargetHost output, only re-adds (redacted) when the original env var literally started with it, and is version-gated via the existing awfVersionAtLeasthelper consistent with other AWF feature flags. Test coverage inawf_config_test.goexercises all scheme/version combinations (http/https/bare x old/new AWF version) end-to-end throughBuildAWFConfigJSON`.
No new blocking issues found. Two pre-existing review comments already flag: (1) missing changeset entry for this user-visible bug fix, and (2) stale doc comment on AWFAPITargetConfig.Host that still claims hostname-only after this change now allows a scheme prefix. Both are valid and should be addressed before merge.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 19 AIC · ⌖ 13.3 AIC · ⊞ 8.3K
|
Warning The 🏗️ ADR Required - draft added for PR #58608I enforced the design-decision gate for this PR because it adds more than 100 new lines in business-logic directories ( Evidence reviewed
Gate resultNo existing ADR was found in the PR body or on the branch for this decision, so I generated a draft ADR and committed it to:
Next actionPlease review and refine the draft ADR, then keep it with this PR as the architectural record for the version-gated decision to preserve explicit HTTP API proxy target schemes.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — the core fix (preserving (redacted) for API proxy targets, gated behind AWFHTTPAPITargetMinVersion) is sound, well-tested across engines/version boundaries, and consistent with the existing feature-flag pattern (awfSupports*`).
📋 Key Themes & Highlights
Key Themes
- Two existing bot comments on
engine_api_targets.go(missing changeset, staleHostdoc comment) remain valid and unaddressed — both are quick fixes, not blockers. - Gemini fallback ordering is correct:
extractAPIProxyTargetHostis tried first, falling back toGetGeminiAPITarget's default host, preserving existing default-target behavior for thegeminiengine.
Positive Highlights
- ✅ New helper
extractAPIProxyTargetHostcleanly layers scheme-preservation on top ofextractAPITargetHostwithout duplicating hostname-extraction logic. - ✅ Version gating via
awfSupportsHTTPAPITargetsfollows the establishedawfSupports*feature-flag convention exactly. - ✅ Table-driven test covers min-version boundary, newer version, older version (bare fallback), HTTPS (bare), and scheme-less inputs — good edge-case coverage per
/tdd.
Approving since the logic and tests are solid; the two inline suggestions (changeset + doc comment) are worth a quick follow-up but don't block merge.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 48.5 AIC · ⌖ 16 AIC · ⊞ 10.3K
Comment /matt to run again
|
|
||
| // extractAPIProxyTargetHost returns the target host format expected by the | ||
| // effective AWF version, preserving an explicit http:// scheme when supported. | ||
| func extractAPIProxyTargetHost(workflowData *WorkflowData, envVar string, firewallConfig *FirewallConfig) string { |
There was a problem hiding this comment.
[/codebase-design] Missing changeset: this is a user-visible bug fix (HTTP scheme stripping caused wrong-port connections) but there's no .changeset/*.md entry, matching the existing bot review comment on this line.
💡 Suggested fix
Add .changeset/patch-preserve-http-api-proxy-target-scheme.md (or similar) with a patch bump describing the fix, following the pattern of other entries like patch-enforce-http-mcp-oidc-awf-version.md.
@copilot please address this.
There was a problem hiding this comment.
The patch changeset is present at .changeset/patch-preserve-http-api-target-schemes.md, documenting the user-visible HTTP target scheme fix.
| } | ||
|
|
||
| if strings.HasPrefix(workflowData.EngineConfig.Env[envVar], "http://") { | ||
| return "http://" + host |
There was a problem hiding this comment.
[/codebase-design] Doc/behavior mismatch: AWFAPITargetConfig.Host at pkg/workflow/awf_config.go:231 is documented as "the hostname (and optional port)", but this function can now return a value with an `(redacted) prefix, contradicting the field's stated contract.
💡 Suggested fix
Update the doc comment on Host to note it may include an explicit (redacted) scheme when the effective AWF version supports it (>= AWFHTTPAPITargetMinVersion`), so future readers of the struct aren't misled by the field name.
@copilot please address this.
There was a problem hiding this comment.
Updated AWFAPITargetConfig.Host documentation to cover explicit http:// targets when supported by the effective AWF version, including the current AWF custom-port normalization limitation.
lpcox
left a comment
lpcox
left a comment
There was a problem hiding this comment.
This is compatible with AWF v0.28.13+: AWF preserves an explicit http:// target, routes it over cleartext HTTP, and derives the matching firewall allowlist entry. The version gate also preserves the prior representation for older AWF versions.
One non-blocking caveat: AWF currently discards explicit target ports during normalization and dials fixed port 80 for HTTP (or 443 for HTTPS). Consequently, http://gateway.example.com works as intended, but http://gateway.example.com:8080 is emitted here with the port and then normalized by AWF to gateway.example.com:80. It would be useful to document that limitation or add a follow-up issue so users do not assume custom-port targets are supported.
@copilot Please take the next forward-progress pass on this PR.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Documented the AWF target-port limitation in |
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot PR sous-chef triage: please address the remaining review feedback (explicit-port handling plus the noted follow-up cleanup items), refresh the branch if needed, and run the Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the explicit-port feedback with regression coverage confirming the compiled target preserves |
|
Category: bug | Risk: medium | Score: 67 | Breakdown: impact 36/urgency 18/quality 13 | Action: batch_review | Batch: awf-runtime
|
Explicit
http://engine base URLs were compiled as bare hosts, causing AWF to default to HTTPS and connect on port 443.Proxy targets
http://for OpenAI, Anthropic, and Gemini targets.Compatibility
http://API targets: scheme stripped fromapiProxy.targets.<provider>.host, defeating AWF v0.28.13 HTTP support #58572