Repository navigation
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Ponytail Reviewer completed successfully! Ponytail ReviewerOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Design Decision Gate 🏗️Outcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality SentinelOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
🔎 PR Code Quality Reviewer is reviewing code quality for this pull request... |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd (via pr-triage) on this proxy-forwarding fix. The change is well-tested and correctly addresses the root cause (#70266) — only minor maintainability suggestions below, no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Duplicated proxy variable list: the six-name proxy env list (HTTP_PROXY/HTTPS_PROXY/NO_PROXY + lowercase) is copy-pasted across
start_cli_proxy.sh,redact_cli_proxy_diagnostics.cjs, andstart_mcp_gateway.cjs, risking silent drift if a future variable (e.g. ALL_PROXY) is added to only some of them. - Composition invariant untested:
injectCustomGatewayEnvArgsandinjectProxyEnvArgsare unit-tested individually, but their ordering invariant when composed inmain()(name-only proxy flags must precede explicit custom env values so explicit wins) has no direct test.
Positive Highlights
- ✅ Proxy values are forwarded by name only (
-e NAME), never embedded in Docker argument strings or diagnostics — solid root-cause fix for the credential-leak and injection-risk classes this area is sensitive to. - ✅ New upstream
/api/v3/rate_limitprobe correctly distinguishes "local proxy up" from "upstream reachable through proxy", which is exactly the gap described in #70266 (local health checks passing while upstream requests still 502). - ✅ Good test coverage: parameterized tests for each proxy var name, empty-value preservation, unset-defaults-to-Docker-client behavior, and the new upstream-failure path (container cleanup + exit code).
- ✅
--noproxy '*' --connect-timeout --max-timebounds the local health/upstream probes so they can't hang or get routed back through the runner's own proxy.
Posted 2 inline suggestions (both non-blocking) on the duplicated constant list and the missing composition test.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 120.2 AIC · ⌖ 15.1 AIC · ⊞ 10.1K
Comment /matt to run again
| fi | ||
|
|
||
| PROXY_ENV_ARGS=() | ||
| for proxy_var in HTTP_PROXY HTTPS_PROXY NO_PROXY http_proxy https_proxy no_proxy; do |
There was a problem hiding this comment.
[/codebase-design] The proxy variable name list (HTTP_PROXY HTTPS_PROXY NO_PROXY http_proxy https_proxy no_proxy) is now duplicated in three places: this loop, redact_cli_proxy_diagnostics.cjs:16, and start_mcp_gateway.cjs:506. Nothing enforces they stay in sync.
💡 Suggested fix
Extract a single source of truth — e.g. a PROXY_ENV_NAMES constant in constants.cjs shared by both JS files, and a comment in the shell script cross-referencing it as the canonical list. If a future proxy variable (e.g. ALL_PROXY) is added, it's easy to update only one or two of the three copies and silently leave a redaction or forwarding gap.
@copilot please address this.
There was a problem hiding this comment.
Done in d4319bc: proxy variable names now come from the shared actions/setup/sh/proxy_env_names.list, consumed by the shell and both JavaScript paths.
| return; | ||
| } | ||
| args = injectCustomGatewayEnvArgs(args); | ||
| args = injectProxyEnvArgs(args); |
There was a problem hiding this comment.
[/tdd] injectCustomGatewayEnvArgs and injectProxyEnvArgs are each unit-tested in isolation, but main() composes them sequentially on the same args array and relies on an ordering invariant (proxy name-only flags must precede explicit custom -e NAME=value flags so the explicit value wins). No test exercises both functions together.
💡 Suggested test
it('keeps explicit custom env values winning over inherited proxy forwarding', () => {
const args = injectCustomGatewayEnvArgs(['run', '__CUSTOM_ENV__', 'gateway:latest'], {
GH_AW_MCP_GATEWAY_CUSTOM_ENV_NAMES: JSON.stringify(['HTTP_PROXY']),
GH_AW_MCP_GATEWAY_CUSTOM_ENV_0: 'explicit-value',
});
const result = injectProxyEnvArgs(args, { HTTP_PROXY: 'inherited-value' });
expect(result.indexOf('HTTP_PROXY')).toBeLessThan(result.indexOf('HTTP_PROXY=explicit-value'));
});This pins down the real invariant the two functions depend on, rather than trusting it holds because each function is independently correct.
@copilot please address this.
There was a problem hiding this comment.
Done in d4319bc: added a composition test confirming inherited name-only proxy flags precede explicit custom gateway -e NAME=value flags.
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved credential exposure and local MCP connectivity regressions block approval.
3 open issues
What changed in this PR
Updates gh-aw setup to support proxy-only self-hosted runners.
Changes:
- Forward defined runner proxy variables to both containers.
- Check upstream API access before reporting CLI proxy readiness.
- Extend diagnostic redaction and regression coverage.
| File | Description |
|---|---|
| actions/setup/sh/start_cli_proxy.sh | Forwards proxy settings and validates upstream connectivity. |
| actions/setup/js/start_mcp_gateway.test.cjs | Tests forwarding, overrides, redaction, and startup ordering. |
| actions/setup/js/start_mcp_gateway.cjs | Injects proxy variables and extends diagnostic sanitization. |
| actions/setup/js/start_cli_proxy.test.cjs | Covers proxy forwarding and upstream startup failures. |
| actions/setup/js/redact_cli_proxy_diagnostics.cjs | Adds proxy environment values to diagnostic masking. |
Review effort: Max
| redacted = redacted.replace(pattern, replacement); | ||
| } | ||
| return redacted; | ||
| return redactCliProxyDiagnostics(redacted, env); |
There was a problem hiding this comment.
Done in d4319bc: generic credential-pattern spans and registered environment values are matched against the original diagnostics and coalesced before replacement. Regression tests cover both overlap directions.
| const proxyArgs = names.filter(name => env[name] !== undefined).flatMap(name => ["-e", name]); | ||
| // Insert after "run" so explicitly configured container settings still win. | ||
| return [...args.slice(0, 1), ...proxyArgs, ...args.slice(1)]; |
There was a problem hiding this comment.
Done in d4319bc: defined NO_PROXY/no_proxy values now retain existing exclusions and add host.docker.internal plus awf-enclave-mcp; unset Docker defaults and explicit gateway overrides remain intact.
| if ! curl -sf --noproxy '*' --connect-timeout 5 --max-time 15 \ | ||
| --cacert "$TLS_DIR/ca.crt" "https://localhost:18443/api/v3/rate_limit" -o /dev/null 2>/dev/null; then |
There was a problem hiding this comment.
Done in d4319bc: upstream readiness now makes up to three bounded probes before cleanup; a regression test confirms startup succeeds when the second probe recovers.
|
@copilot address the following outstanding work in one pass:
Push the necessary fixes, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved. Use the pr-finisher skill and stop when only human review or CI remains; do not trigger CI. Sous-chef head: 8e87708
|
…y-issues 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>
|
@lpcox review |
|
There is one remaining correctness issue in I reproduced this using the helper from head Please ensure effective internal-host exclusions whenever proxy forwarding is enabled, including when both |
On proxy-only self-hosted runners, the CLI proxy container lacks the runner’s proxy settings. Local health checks pass, but upstream GitHub API requests return HTTP 502 and prevent agent startup.
HTTP_PROXY,HTTPS_PROXY, andNO_PROXYvariables, including lowercase forms, to both containers by name. Preserve Docker defaults for unset variables and explicit gateway overrides.