feat: route unified enclaves exclusively through mcpg - #6992
Conversation
There was a problem hiding this comment.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
Note
This error may be related to your runner configuration. You can now configure runners for Copilot code review separately from Copilot cloud agent by creating a copilot-code-review.yml file with your setup steps. Read the docs for details.
073146f to
54a92b4
Compare
|
✅ Copilot review passed with no inline comments. @lpcox Add the |
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 98.40% | 98.15% | 📉 -0.25% |
| Statements | 98.18% | 97.92% | 📉 -0.26% |
| Functions | 98.36% | 97.99% | 📉 -0.37% |
| Branches | 94.22% | 93.65% | 📉 -0.57% |
📁 Per-file Coverage Changes (7 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/container-lifecycle.ts |
98.7% → 96.6% (-2.08%) | 98.8% → 96.2% (-2.54%) |
src/cli-workflow.ts |
100.0% → 98.6% (-1.37%) | 100.0% → 98.6% (-1.37%) |
src/commands/main-action.ts |
97.4% → 97.0% (-0.44%) | 97.0% → 96.5% (-0.43%) |
src/services/enclave-mcp-service.ts |
97.6% → 97.6% (-0.02%) | 95.5% → 95.5% (-0.05%) |
src/compose-generator.ts |
98.6% → 98.6% (+0.04%) | 98.6% → 98.6% (+0.04%) |
src/enclave/manager.ts |
80.2% → 80.5% (+0.30%) | 79.2% → 79.5% (+0.34%) |
src/log-directory-setup.ts |
96.2% → 100.0% (+3.78%) | 96.3% → 100.0% (+3.71%) |
✨ New Files (1 files)
src/enclave/gateway.ts: 86.8% lines
Coverage comparison generated by scripts/ci/compare-coverage.ts
1 similar comment
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 98.40% | 98.15% | 📉 -0.25% |
| Statements | 98.18% | 97.92% | 📉 -0.26% |
| Functions | 98.36% | 97.99% | 📉 -0.37% |
| Branches | 94.22% | 93.65% | 📉 -0.57% |
📁 Per-file Coverage Changes (7 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/container-lifecycle.ts |
98.7% → 96.6% (-2.08%) | 98.8% → 96.2% (-2.54%) |
src/cli-workflow.ts |
100.0% → 98.6% (-1.37%) | 100.0% → 98.6% (-1.37%) |
src/commands/main-action.ts |
97.4% → 97.0% (-0.44%) | 97.0% → 96.5% (-0.43%) |
src/services/enclave-mcp-service.ts |
97.6% → 97.6% (-0.02%) | 95.5% → 95.5% (-0.05%) |
src/compose-generator.ts |
98.6% → 98.6% (+0.04%) | 98.6% → 98.6% (+0.04%) |
src/enclave/manager.ts |
80.2% → 80.5% (+0.30%) | 79.2% → 79.5% (+0.34%) |
src/log-directory-setup.ts |
96.2% → 100.0% (+3.78%) | 96.3% → 100.0% (+3.71%) |
✨ New Files (1 files)
src/enclave/gateway.ts: 86.8% lines
Coverage comparison generated by scripts/ci/compare-coverage.ts
54a92b4 to
8082499
Compare
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 98.40% | 98.15% | 📉 -0.25% |
| Statements | 98.18% | 97.92% | 📉 -0.26% |
| Functions | 98.36% | 97.99% | 📉 -0.37% |
| Branches | 94.22% | 93.65% | 📉 -0.57% |
📁 Per-file Coverage Changes (7 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/container-lifecycle.ts |
98.7% → 96.6% (-2.08%) | 98.8% → 96.2% (-2.54%) |
src/cli-workflow.ts |
100.0% → 98.6% (-1.37%) | 100.0% → 98.6% (-1.37%) |
src/commands/main-action.ts |
97.4% → 97.0% (-0.44%) | 97.0% → 96.5% (-0.43%) |
src/services/enclave-mcp-service.ts |
97.6% → 97.6% (-0.02%) | 95.5% → 95.5% (-0.05%) |
src/compose-generator.ts |
98.6% → 98.6% (+0.04%) | 98.6% → 98.6% (+0.04%) |
src/enclave/manager.ts |
80.2% → 80.5% (+0.30%) | 79.2% → 79.5% (+0.34%) |
src/log-directory-setup.ts |
96.2% → 100.0% (+3.78%) | 96.3% → 100.0% (+3.71%) |
✨ New Files (1 files)
src/enclave/gateway.ts: 86.8% lines
Coverage comparison generated by scripts/ci/compare-coverage.ts
There was a problem hiding this comment.
Review details
Suppressed comments (4)
docs/awf-config-spec.md:2527
- The table lists five handoff variables, not four. Update this sentence to avoid suggesting that one variable is not covered by the primary-agent exclusion guarantee.
All four names are unconditionally excluded from primary-agent environment
passthrough. The mcpg upstream is named `awf-enclave`, uses
src/enclave/gateway.ts:418
- Only retry the documented mcpg recovery response (
503with retryablebackend_unavailable). This catch currently retries every permanent failure—including authentication errors, malformed responses, and exact tool-contract mismatches—for the full 120-second default window. Preserve enough status/error classification to fail those cases immediately while continuing to avoid logging response bodies or credentials.
src/enclave/gateway.ts:297 - The configured readiness deadline is not a hard bound because each request always gets 5 seconds. With a 1-second readiness window, one request can already run about 5 seconds, and a handshake that begins before the deadline can continue through three such requests. Pass the remaining readiness budget into each request and cap its timeout accordingly.
src/commands/main-action.ts:157 - Drain the enclave server before copying its audit artifacts. The server emits cleanup telemetry only while handling the stop signal, so copying first deterministically omits that final record and can also miss in-flight invocation/session records during shutdown. Stopping this server does not prevent
docker cp, so preserve the artifacts after the bounded shutdown and before Compose removes the container.
- Files reviewed: 73/73 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 46fcf12f-bd83-47cf-a042-83f797b7fdeb
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 46fcf12f-bd83-47cf-a042-83f797b7fdeb
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 46fcf12f-bd83-47cf-a042-83f797b7fdeb
8082499 to
aae75f4
Compare
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 98.59% | 98.62% | 📈 +0.03% |
| Statements | 98.42% | 98.44% | 📈 +0.02% |
| Functions | 98.36% | 98.58% | 📈 +0.22% |
| Branches | 94.43% | 94.32% | 📉 -0.11% |
📁 Per-file Coverage Changes (7 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/container-lifecycle.ts |
98.7% → 96.6% (-2.08%) | 98.8% → 96.2% (-2.54%) |
src/cli-workflow.ts |
100.0% → 98.7% (-1.30%) | 100.0% → 98.7% (-1.30%) |
src/services/enclave-mcp-service.ts |
97.6% → 97.6% (-0.02%) | 95.5% → 95.5% (-0.05%) |
src/compose-generator.ts |
98.6% → 98.6% (+0.04%) | 98.6% → 98.6% (+0.04%) |
src/commands/main-action.ts |
97.4% → 97.5% (+0.06%) | 97.0% → 97.0% (+0.06%) |
src/enclave/manager.ts |
80.2% → 83.6% (+3.43%) | 79.2% → 82.6% (+3.37%) |
src/log-directory-setup.ts |
96.2% → 100.0% (+3.78%) | 96.3% → 100.0% (+3.71%) |
✨ New Files (1 files)
src/enclave/gateway.ts: 100.0% lines
Coverage comparison generated by scripts/ci/compare-coverage.ts
|
📰 DEVELOPING STORY: Smoke Copilot reports failed to deliver outputs. Our correspondents are investigating the incident... |
|
✅ Contribution Check completed successfully! Contribution check complete: the PR includes substantial new tests and documentation updates, and the description references related work and validation. No CONTRIBUTING.md issues requiring a review comment were found. |
|
📰 DEVELOPING STORY: Smoke Docker Sbx reports failed to deliver outputs. Our correspondents are investigating the incident... |
|
✅ Smoke Copilot BYOK AOAI (Entra) completed. Copilot AOAI BYOK (Entra) mode operational. 🔓 |
|
📡 Smoke OTel Tracing completed. All tracing scenarios validated. ✅ |
|
❌ Smoke Copilot BYOK AOAI (api-key) reports failed to deliver outputs. AOAI BYOK (api-key) mode investigation needed... |
|
EGRESS_RESULT allow=pass deny=pass ✅ Allowed domain (github.com) reachable: Overall status: PASS cc @lpcox 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
- "example.com"See Network Configuration for more information.
|
|
Smoke Test: Copilot BYOK (Direct Mode) ✅ PASS Tests:
Overall Status: PASS | Mode: Direct BYOK via api-proxy sidecar
|
🔥 Smoke Test: Copilot Engine
✅ GitHub MCP connectivity Overall: PASS cc @lpcox
|
Smoke Test: GitHub Actions Services Connectivity
Overall: FAIL —
|
Smoke Test: Claude Engine Validation
Overall result: PASS
|
🔐 Security Review: Network Architecture ChangeFinding: Unix Socket → Network Binding The enclave MCP server has been changed from a networkless Unix socket to TCP network binding: Changes:
Security Impact: Recommendation: The architectural shift appears intentional per PR title, but upstream auth mechanisms should be reviewed separately.
|
OTel Tracing Smoke Test Results
Overall: All checks passed. No regressions detected.
|
Chroot Version Comparison Results
Overall: FAILED — Node.js version mismatch between host and chroot environments.
|
🏗️ Build Test Suite Results
Overall: 8/8 ecosystems passed — PASS No errors encountered in any project across all ecosystems.
|
Smoke TestPR titles:
Overall: FAIL 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
- "registry.npmjs.org"See Network Configuration for more information.
|
Running in direct BYOK mode (COPILOT_PROVIDER_API_KEY + COPILOT_PROVIDER_BASE_URL) via api-proxy → Azure OpenAI (Foundry, o4-mini-aw) Overall: PASS
|
|
chore: upgrade gh-aw to v0.86.0 pre-release and recompile workflows: ✅
|
|
Smoke Test: Docker Sbx — @lpcox
Overall: PASS
|
Gemini Smoke Test Results
Overall status: FAIL
|
## Summary - keep routed mcpg healthy when an HTTP MCP upstream is configured before it starts - retry and serialize backend initialize/tool discovery before creating a routed MCP session, returning retryable HTTP 503 responses while unavailable - re-register only allowlisted tools once the upstream appears and preserve auth/HMAC/shutdown middleware ordering - apply `connectTimeout` to the plain JSON fallback as well as SDK-managed transports - document the exact `awf-enclave` JSON stdin contract and version requirement ## Dependency Prerequisite for github/gh-aw-firewall#6992 (enclave stack layer 4). That PR starts `awf-enclave-mcp` after the compiler-owned mcpg container, then proves `initialize` + `notifications/initialized` + allowlisted `tools/list` through `/mcp/awf-enclave` before starting the primary agent. A follow-up in github/gh-aw must emit the documented `awf-enclave` HTTP server entry, capability header substitution, enabled-tool allowlist, `connectTimeout`, and `toolTimeout`, and pin the first mcpg release containing this PR. Important contract clarification: `gateway.startupTimeout` governs stdio process startup; HTTP recovery is governed by per-server `connectTimeout` plus the caller's bounded readiness retry window. Firewall/compiler assumptions should use `connectTimeout: 120` and retry `initialize` on `backend_unavailable`, not rely on `gateway.startupTimeout` to keep an HTTP attempt alive. ## Validation - `GODEBUG=netdns=go make agent-finished` - `GODEBUG=netdns=go make test-integration` - focused delayed-start regression covers unavailable startup, retryable route, upstream bearer auth, initialize/initialized/tools-list discovery, routed allowlist filtering/rejection, and 150-second per-server tool timeout configuration - focused transport regression covers a silent plain-JSON upstream respecting `connectTimeout`
Stack
Layer 4 of the greenfield enclave migration.
Depends on:
Base branch:
lpcox-enclave-agent-executorSummary
awf-enclave-mcp-controlnetwork with no published port, Squid path,awf-netmembership, agent mount, direct URL, wrapper, or skill fallbackawmg-mcpgcontainer by run-scoped label, attaches only it to the control network, and rejects unexpected membersinitializeand the complete enabledtools/listcontracts through mcpg before starting either a Compose agent or sbx sandboxbackend_unavailable, with bounded 500 ms backoff untilAWF_ENCLAVE_MCP_READINESS_TIMEOUT_MSexpires; the first 503 is not terminal and response bodies/headers/capabilities are not loggedboundedQueriesandboundedAgentsfunctional for the final layer-5 cutoverCompiler handoff required
The gh-aw compiler must add an
awf-enclaveHTTP upstream athttp://awf-enclave-mcp:8080/mcp, substitute the run-scoped capability into its bearer authorization header, allowlist only enabled enclave tools, set per-serverconnectTimeout: 120and the derived tool timeout, labelawmg-mcpgwithcom.github.gh-aw.mcpg.run=<identity>, and pass the gateway identity/container/readiness endpoint handoff variables to AWF but never the primary agent.connectTimeoutgoverns each HTTP upstream attempt.gateway.startupTimeoutis stdio-only and must not be used as the HTTP recovery bound. The compiler must pin the first mcpg release after v0.4.8 containing #10784/spec 1.15.0. The current compiler does not yet emit this contract, so a gh-aw follow-up is required.Validation
backend_unavailablefollowed by successful initialize and exact tools/list discoverynode --checkThe exact locked
typescript-eslintpackage remains unavailable from the configured feed, so repository-wide lint could not use the lockfile toolchain. An isolated compatible lint toolchain found one pre-existing error outside this change; every changed TypeScript file passes lint.