Fix static GitHub agent enclave startup - #59878
Conversation
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
|
Great work on this fix! 🎯 This PR properly addresses the static GitHub agent enclave initialization issues by fixing guard policy generation, startup validation, deferred CLI tool mounting, and finite-disclosure usability. The implementation looks solid:
This looks ready for review! All checklist items are met — the diff is focused, tests are included, and the changes are well-documented. 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.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. 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.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff 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.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ 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.
|
🏗️ ADR gate: implementation diverges from the existing ADRResultThe PR already has ADR coverage via Evidence reviewed
Divergences / scope creep
Next actionPlease update the ADR (or add a follow-up ADR) so it explicitly covers the expanded decision scope in this PR: enclave-specific GitHub startup probing, deferred
|
There was a problem hiding this comment.
🟡 Changes recommended
The deferred CLI refresh marker is not propagated to the later agent step, leaving the stale-tool defect unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes static GitHub enclave startup, deferred CLI discovery, and finite-disclosure guidance.
Changes:
- Generates enclave-scoped guard and write-sink policies.
- Uses the enclave identity for GitHub startup checks.
- Refreshes deferred CLI schemas and adds budget diagnostics.
File summaries
| File | Description |
|---|---|
.changeset/fix-enclave-only-github-guard-policy.md |
Documents the fixes. |
pkg/workflow/mcp_setup_gateway.go |
Selects the enclave check identity. |
pkg/workflow/mcp_github_config.go |
Derives static-enclave write-sink policy. |
pkg/workflow/mcp_cli_mount.go |
Mounts enclave CLI and adds budget guidance. |
pkg/workflow/mcp_cli_mount_test.go |
Tests generated guidance. |
pkg/workflow/enclave_github_proxy_test.go |
Tests compiled policies and identity. |
actions/setup/sh/check_mcp_servers.sh |
Overrides GitHub probe authentication. |
actions/setup/sh/check_mcp_servers_test.sh |
Tests enclave-authenticated probes. |
actions/setup/js/mcp_cli_bridge.cjs |
Refreshes tools and adds budget hints. |
actions/setup/js/mcp_cli_bridge.test.cjs |
Tests refresh and error hints. |
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| * @returns {Promise<Array<{name: string, description?: string, inputSchema?: {properties?: Record<string, {description?: string, type?: string}>, required?: string[]}}>>} | ||
| */ | ||
| async function refreshDeferredToolsIfNeeded(tools, serverName, serverUrl, apiKey, toolsFile) { | ||
| if (tools.length > 0 || !serverInCommaList(serverName, process.env[DEFERRED_SERVERS_ENV] || "")) { |
There was a problem hiding this comment.
Fixed in ddb0712. The deferred refresh path now treats awf-enclave as deferred even when GH_AW_MCP_DEFERRED_SERVERS is step-local/missing, so the wrapper no longer depends on that env handoff.
| case "internal", "sealed": | ||
| repoLines = append(repoLines, fmt.Sprintf("- `%s` (`%s`) has a finite per-run budget; keep response schema cardinality within the budget reported by `awf-enclave --help`.", repo.Repo, repo.Sensitivity)) |
There was a problem hiding this comment.
Fixed in ddb0712. sealed repos now emit explicit 0-bit/no-enclave guidance and instruct agents not to invoke awf-enclave enclave_run_agent for that repository.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed with harden/audit lens (bug-fix change touching startup/error-state paths). One actionable finding on error-state hardening in the deferred tools refresh path (missing try/catch around the tools/list call could turn a transient "server not ready" condition into a hard CLI failure). The rest of the change — guard-policy generation, enclave identity checks, and bit-budget prompt guidance — looks correct and is covered by new tests.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 75 AIC · ⌖ 13.7 AIC · ⊞ 8.4K
| if (sessionId) { | ||
| headers["Mcp-Session-Id"] = sessionId; | ||
| } | ||
| const resp = await httpPostJSON(serverUrl, headers, { jsonrpc: "2.0", id: TOOLS_LIST_REQUEST_ID, method: "tools/list" }, DEFAULT_HTTP_TIMEOUT_MS); |
There was a problem hiding this comment.
mcpInitialize internally catches its own errors (returns "" on failure), but this raw httpPostJSON call for tools/list has no try/catch. If the deferred awf-enclave server hasn't finished registering yet (connection refused, timeout, non-JSON response), this throws and propagates out of refreshDeferredToolsIfNeeded → main(), causing core.setFailed('mcp_cli_bridge fatal: ...') for what should be a soft "not ready yet" case — exactly the scenario this function is meant to handle gracefully (empty cache due to late registration). Consider wrapping this call in try/catch and falling back to the original tools (mirroring the refreshed.length === 0 branch) so a transient failure doesn't hard-fail the CLI invocation.
@copilot please address this.
There was a problem hiding this comment.
Fixed in ddb0712. Deferred tools/list refresh is now wrapped with bounded retry and error hardening; transport/protocol failures are handled as soft warnings and fall back to cached tools instead of hard-failing the bridge.
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.
|
There was a problem hiding this comment.
Request changes
The deferred awf-enclave schema refresh is still flaky. This patch replaces a permanently stale cache with a single live tools/list probe, but the mount path already documents that tools/list can return 0 while the backend is still building its schema. Without the same bounded retry here, the first post-start invocation can still surface an empty CLI/help state for the exact feature this PR is trying to make reliable.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 104.7 AIC · ⌖ 5.57 AIC · ⊞ 22.3K
Comment /review to run again
| const result = resultMessage && typeof resultMessage === "object" && "result" in resultMessage && resultMessage.result && typeof resultMessage.result === "object" ? resultMessage.result : null; | ||
| const refreshed = result && "tools" in result && Array.isArray(result.tools) ? result.tools : []; | ||
| if (refreshed.length === 0) { | ||
| core.warning(`[${serverName}] live tools/list still returned 0 tools for deferred server`); |
There was a problem hiding this comment.
This refresh still does a single live tools/list call, so awf-enclave can stay stuck with an empty schema when the deferred backend has started but has not finished building its tool definitions yet.
💡 Why this blocks
mount_mcp_as_cli.cjs already has fetchMCPToolsWithRetry() because a successful MCP handshake can still race the backend's schema construction and return zero tools for a while. This new fallback skips that retry path and treats the first empty live result as final, which means the first real awf-enclave invocation can still show no commands or parse args without a schema even after startup has otherwise succeeded.
Please reuse the existing empty-tools retry/backoff logic here, or add the same bounded retry behavior before preserving the empty cache.
There was a problem hiding this comment.
Fixed in ddb0712. The deferred refresh now uses bounded retries for empty tools/list results, and tests cover both retry-success and retry-exhaustion behavior.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — approving with a few non-blocking suggestions.
📋 Key Themes & Highlights
Key Themes
- Constant drift risk: the finite-disclosure bit-budget constants in
mcp_cli_mount.gomirror gh-aw-firewall's internal formula by comment convention only, with no version gate or test that would catch drift. - Doc gap:
deriveWriteSinkGuardPolicyFromWorkflow's doc comment doesn't mention the new static-enclave-only write-sink derivation path this PR adds. - Test gap:
refreshDeferredToolsIfNeeded's "live gateway still returns 0 tools" fallback branch (the exact failure mode being fixed) isn't covered by a test.
Positive Highlights
- ✅ Root-cause fixes, not symptom patches — guard policy, startup identity, and CLI cache staleness are each addressed at their source with matching regression tests (
TestCompileEnclaveOnlyGitHubToolsGuardPolicy, shell Test 20, JS refresh test). - ✅ Clean reuse of existing helpers (
writeSinkGuardPolicy,writeSinkAcceptLabelsForRepos) to keep the static/dynamic enclave write-sink derivation DRY. - ✅ New behavior is opt-in and narrowly scoped:
refreshDeferredToolsIfNeededonly triggers for servers listed inGH_AW_MCP_DEFERRED_SERVERSwith an empty cache, and the enclave-only GitHub identity override incheck_mcp_servers.shis gated ongithubBackendIsStaticEnclaveDelegationOnly. - ✅ Ran targeted Go tests (
TestCompileEnclaveOnlyGitHubToolsGuardPolicy,TestBuildMCPCLIPromptSection_StaticEnclaveBudgetGuidance,TestGetMCPCLIServerNames_CopilotIncludesManifestServersInPromptList) — all pass.
@copilot please address the review comments above.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
registry.npmjs.org
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "registry.npmjs.org"See Network Configuration for more information.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 159.9 AIC · ⌖ 15.3 AIC · ⊞ 10.3K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/workflow/mcp_cli_mount.go:412
[/codebase-design] These bit-budget constants (enclaveResultStatusBitCost, enclaveTimingBucketBits, enclaveConfidentialRunBits) are a hand-copied mirror of gh-aw-firewall's internal charge formula, kept in sync only by a code comment.
<details>
<summary>💡 Why this matters</summary>
If gh-aw-firewall changes RESULT_STATUS_BIT_COST, TIMING_BUCKET_BITS, or ENCLAVE_SENSITIVITY_RUN_BITS.confidential in a future release, this generated prompt guidance will silently start reporting …
pkg/workflow/mcp_github_config.go:683
[/codebase-design] The doc comment above deriveWriteSinkGuardPolicyFromWorkflow still only describes two write-sink derivation cases (explicit guard policy, auto-lockdown) plus the dynamic-enclave fallback, but this PR adds a third: the static-enclave-only path (staticEnclaveWriteSinkGuardPolicy) now short-circuits before dynamicEnclaveWriteSinkGuardPolicy in two places.
<details>
<summary>💡 Suggested doc update</summary>
Add a bullet noting that when GitHub tools are disabled for …
actions/setup/js/mcp_cli_bridge.test.cjs:229
[/tdd] Good coverage of the happy path, but the "still returns 0 tools after a live refresh" branch in refreshDeferredToolsIfNeeded (line ~1163 in mcp_cli_bridge.cjs) has no test.
<details>
<summary>💡 Suggested test</summary>
That branch is exactly the failure mode this PR is fixing ("deferred CLI tooling could cache an empty schema"), so it's the highest-value case to pin down with a regression test:
it("keeps the empty cache and warns when the live gateway also returns 0 too…
</details>
|
@copilot Please address the current review feedback, refresh the branch if needed, and run the Open review signals:
Once updated, rerun the relevant validation and resolve addressed review threads. 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.
|
|
@copilot address review feedback |
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>
Updated in ddb0712 and addressed the open review feedback: deferred |
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Addressed in 0386fdb. I applied the remaining hardening feedback by caching exhausted deferred refresh attempts per server (process-local) so repeated calls don’t pay the full retry loop, added regression coverage, and reran validation ( |
|
@copilot The latest author updates indicate the requested review feedback has been addressed on the current head. Please refresh branch metadata if needed, rerun the Open signals to verify on the current head:
Run: https://github.com/github/gh-aw/actions/runs/34467468242 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.
|
Verified on head 0386fdb: prior review requests are against earlier commits and each thread already has a substantive fix reply from @copilot; all remaining open threads are outdated. I reran local validation ( |
|
🎉 This pull request is included in a new release. Release: |
Static GitHub agent enclaves with
tools.github: falsegenerated an unusable MCP Gateway topology: guarded enclave-only GitHub backends lacked matching safeoutputs write-sink policy, startup checks used the wrong identity, and deferred CLI tooling could cache an empty schema. Agents also had no actionable guidance for finite-disclosure bit budgets, causing valid-looking enclave calls to fail opaquely.write-sinkaccept labels from static enclave repository declarations.{ "guard-policies": { "write-sink": { "accept": ["private:githubnext/gh-aw-enclave-demo-private"] } } }Startup validation
AWF_ENCLAVE_GITHUB_MCP_AGENT_ID.Deferred enclave CLI
awf-enclavetool inventory after late backend registration when the startup-time cache is empty.Finite-disclosure usability
bit-budget-exhaustedas a distinguishable enclave CLI error hint.Run: https://github.com/github/gh-aw/actions/runs/34467468242
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
hub.lumenfield.workTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.