Add read-only GitHub Issues access to agent enclaves - #55531
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bcc38c7d-af99-4f49-8c17-b56cbb4515c5
There was a problem hiding this comment.
Pull request overview
Adds isolated, read-only GitHub Issues access for agent enclaves through a dedicated mcpg proxy.
Changes:
- Adds
issues-read-v1schemas, validation, and version gates. - Generates proxy policy, lifecycle, credential isolation, and tests.
- Documents routes, DIFC behavior, and dependency requirements.
Show a summary per file
| File | Description |
|---|---|
.changeset/enclave-github-issues-profile.md |
Records the new profile. |
.github/aw/enclaves.md |
Adds authoring guidance. |
actions/setup/sh/start_enclave_github_proxy.sh |
Starts and configures the proxy. |
actions/setup/sh/stop_enclave_github_proxy.sh |
Cleans up proxy resources. |
docs/src/content/docs/reference/enclaves.md |
Documents profile behavior. |
docs/src/content/docs/reference/glossary.md |
Updates enclave terminology. |
pkg/constants/version_constants.go |
Defines dependency minimums. |
pkg/parser/schema_test.go |
Tests frontmatter validation. |
pkg/parser/schemas/main_workflow_schema.json |
Adds user-facing schema syntax. |
pkg/workflow/awf_env.go |
Excludes proxy handoff variables. |
pkg/workflow/compiler_yaml_ai_execution.go |
Adds proxy cleanup lifecycle. |
pkg/workflow/enclave_github_proxy.go |
Builds policy and lifecycle steps. |
pkg/workflow/enclave_github_proxy_test.go |
Tests proxy integration. |
pkg/workflow/enclaves.go |
Adds configuration and validation. |
pkg/workflow/enclaves_test.go |
Tests AWF configuration output. |
pkg/workflow/mcp_setup_generator.go |
Starts the proxy during MCP setup. |
pkg/workflow/schemas/awf-config.schema.json |
Adds AWF schema support. |
schema-demos/schema-demo-enclaves.md |
Demonstrates the new syntax. |
Review details
Suppressed comments (1)
actions/setup/sh/start_enclave_github_proxy.sh:66
- A cancelled prior run can leave
proxy-tls/ca.crthere. Because the readiness probe usescurl -k, it can accept the new proxy while retaining the stale CA, after which AWF receives a CA that cannot authenticate the proxy. Remove the previous container and log/TLS directory before recreating it.
mkdir -p "$MCP_LOG_DIR"
chmod 700 "$MCP_LOG_DIR"
docker rm -f "$CONTAINER_NAME" >/dev/null 2>&1 || true
- Files reviewed: 18/18 changed files
- Comments generated: 2
- Review effort level: Balanced
| MCP_LOG_DIR="/tmp/gh-aw/enclave-github-proxy-logs" | ||
| CA_CERT="${MCP_LOG_DIR}/proxy-tls/ca.crt" |
| The compiler starts a dedicated mcpg proxy in Docker bridge mode. The PAT | ||
| remains in that proxy. AWF attaches it to a private control network, mints a | ||
| short-lived `awf-egh1` capability into a mode-`0600` file, and exposes only an | ||
| AWF-owned PAT-free local CLI proxy to the enclave. Neither the primary agent | ||
| nor the enclave receives the PAT, mcpg address, root key, container identity, | ||
| CA path, or repository catalog. |
|
/matt |
|
/review |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
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
This introduces two blocking regressions: the new repo-limit validation rejects valid mixed script + agent.github.cli enclave configs, and the enclave GitHub proxy teardown can be skipped when later host-side steps fail.
Blocking themes
- The
issues-read-v1non-public repository limit is being enforced against the wrong scope, so existing mixed-enclave workflows break as soon as they opt into the new profile. - The proxy cleanup path is not robust against downstream failures, which leaves the capability handoff and proxy state alive longer than the design claims.
🔎 Code quality review by PR Code Quality Reviewer · pi · gpt54 · 9.7 AIC · ⌖ 7.12 AIC · ⊞ 7K
Comment /review to run again
| if err != nil { | ||
| return err | ||
| } | ||
| if enclaveType == "agent" && enclave.Agent.GitHub != nil && nonPublicRepositories > 1 { |
There was a problem hiding this comment.
This validation blocks any workflow that enables the enclave GitHub profile alongside a script enclave, because validateEnclaveEntry counts every non-public repo in the whole enclaves: array instead of only the repos attached to the GitHub-enabled agent entry. A perfectly valid config with one script enclave on org/a and one agent GitHub enclave on org/b now fails with “supports at most one non-public repository”, so this feature silently breaks mixed-enclave workflows rather than enforcing its own profile limit.
💡 Why this needs to change
The product docs and existing model allow one script entry and one agent entry at the same time. The new limit should apply only to enclaves[i].agent.github.cli: issues-read-v1, but the validator reuses the global cross-enclave sensitivity map and then rejects whenever nonPublicRepositories > 1 for that entry.
A failing shape is:
enclaves:
- script:
repos:
- repo: org/private-a
sensitivity: confidential
- agent:
model: gpt-5
github:
cli: issues-read-v1
repos:
- repo: org/private-b
sensitivity: confidentialThat should pass, because the GitHub profile still has only one assigned non-public repo. Count only enclave.Repos for the GitHub-enabled agent entry, and keep the existing cross-entry sensitivity consistency check separate.
There was a problem hiding this comment.
Addressed with concrete coverage in efa862c. TestValidateEnclaveGitHubIssuesRepositoryLimitScopesToGitHubEntry verifies mixed script+agent enclaves with different private repos now pass while preserving the GitHub-entry-only non-public repo limit.
|
|
||
| // Stop CLI proxy after AWF execution (always runs to ensure cleanup) | ||
| c.generateStopCliProxyStep(yaml, data) | ||
| c.generateStopEnclaveGitHubProxyStep(yaml, data) |
There was a problem hiding this comment.
The cleanup step is not guarded with if: always(), so any failure in the post-AWF host steps will skip it and leave the capability key and proxy artifacts behind for the rest of the job. That weakens the “short-lived handoff” contract and makes later steps run with stale enclave state instead of forcing deterministic teardown.
💡 Why this needs to change
generateStopEnclaveGitHubProxyStep is emitted immediately after AWF execution, but unlike the CLI proxy and MCP gateway teardown paths it has no unconditional execution guard. If generateDetectAgentErrorsStep, post-agent git reconfiguration, firewall log collection, or secret redaction fails first, GitHub Actions will skip this stop step entirely.
At minimum this stop step should be emitted with if: always() in the same way as the other cleanup paths, and ideally cleanup should be ordered with the rest of the teardown sequence so failures later in the job cannot bypass it.
There was a problem hiding this comment.
Addressed with explicit guard coverage in efa862c. TestGenerateEnclaveGitHubProxyStopAlwaysRuns asserts the stop step includes both if: always() and continue-on-error: true to enforce teardown robustness.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — overall LGTM with two minor observations (no blocking issues).
📋 Key Themes & Highlights
Key Themes
- Architecture is clean and well-scoped: the PAT/capability key never reaches the AWF sandbox or agent, the proxy runs in bridge mode with no published host port, and the stop step always clears the key from
GITHUB_ENV. The security boundary is deliberately layered and the test suite enforces the contract. - Version gating is conservative: provisional minimums (AWF v0.28.6, MCPG v0.4.11) are separated from global defaults until release artifacts exist. The
validateEnclaveGitHubIssuesVersionsfunction correctly uses the default MCPG version as the fallback when none is specified — meaning omittingsandbox.mcp.versionfails validation. - Two minor observations posted as inline comments:
enclaveGitHubIssuesOperationsas a package-level slice makes profile-to-operations lookup implicit; worth a map when a second profile arrives.- A test case for the nil-MCP-config rejection path would complete the version-gate coverage.
Positive Highlights
- ✅
TestEnclaveGitHubProxyScriptsEnforceDedicatedBridgeContractis an excellent contract-enforcement test — it pins security-critical shell invariants (no-phost port, bridge mode, capability masking ordering) directly in Go. - ✅
TestCompileEnclaveGitHubProxyLifecycleverifies end-to-end compilation ordering and exclusion of all handoff vars from the AWF command line. - ✅
effectivePrimaryGitHubIntegrityFloorcorrectly prefersParsedToolsover raw map access and falls back toapproved— the priority chain is explicit and tested. - ✅ Refactoring
validateEnclavesConfigintovalidateEnclaveEntry/validateEnclaveRepositories/validateEnclaveGitHubIssuesVersionsmeaningfully improves testability and readability.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet46 · 78.7 AIC · ⌖ 10.2 AIC · ⊞ 7.6K
Comment /matt to run again
| func buildEnclaveGitHubProxyPolicyJSON(workflowData *WorkflowData, workflowRunID string) (string, error) { | ||
| enclave := enclaveGitHubIssuesConfig(workflowData) | ||
| if enclave == nil { | ||
| return "", nil |
There was a problem hiding this comment.
[/codebase-design] enclaveGitHubIssuesOperations is a package-level variable encoding a closed enum as a slice literal. If a second profile is added later, callers have no way to look up the right operations for a given profile without a refactor.
💡 Suggestion
Consider a profile-keyed lookup, even as a simple map, so the coupling is explicit:
var enclaveGitHubProfileOperations = map[string][]string{
enclaveGitHubIssuesProfile: {
"issues.comments.list",
"issues.get",
"issues.list",
},
}Low priority — fine as-is for a single profile, but the pattern will matter when a second one arrives.
@copilot please address this.
| workflowPath := filepath.Join(tmp, "enclave-github.md") | ||
| content := `--- | ||
| on: workflow_dispatch | ||
| strict: false |
There was a problem hiding this comment.
[/tdd] The test TestEnclaveGitHubProxyVersionGates does not cover the case where sandbox.mcp is entirely absent (nil). The validateEnclaveGitHubIssuesVersions function falls back to DefaultMCPGatewayVersion (v0.4.10) in that path, which is below the required v0.4.11 minimum — validation should reject this, but it isn't explicitly tested.
💡 Suggested test case
t.Run("nil MCP config rejected", func(t *testing.T) {
data := enclaveGitHubIssuesWorkflowData()
data.SandboxConfig.MCP = nil
err := validateEnclavesConfig(data)
require.Error(t, err)
assert.Contains(t, err.Error(), string(constants.MCPGEnclaveGitHubIssuesMinVersion))
})This prevents a regression where removing the MCP version pin silently passes validation.
@copilot please address this.
|
Please do one focused follow-up pass:
Run: https://github.com/github/gh-aw/actions/runs/32787277683
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in efa862c. I added focused regression coverage for mixed-enclave repository-limit scoping and for unconditional enclave GitHub proxy teardown ( |
|
Please do one focused follow-up pass:
Run: https://github.com/github/gh-aw/actions/runs/32795790586
|
Follow-up done. I replied in the blocking github-actions review threads with concrete fix references to efa862c for both items ( |
Summary
enclaves[].agent.github.cli: issues-read-v1syntax to the user and AWF schemasv0.28.6and mcpgv0.4.11minimums without changing global defaults or unpublished image digestsValidation
make fmtgo test ./pkg/workflow ./pkg/parser -run 'Enclave|enclave' -count=1make shellcheck-setup-shmake buildgit diff --checkmake agent-report-progresscompletes build, schema freshness, action shell lint, and impacted Go tests, but the target remains blocked by pre-existing repository-widegolint-custombaseline findings unrelated to this change. The full workflow package also retains the existing macOS/varversus/private/varpath assertion failure.Dependencies
This is dependency layer 3 and should merge only after the corresponding gh-aw-mcpg and gh-aw-firewall/AWF changes. The provisional versions are intentionally gated but global defaults and digests remain unchanged until those artifacts are published.
pr-sous-chef run: https://github.com/github/gh-aw/actions/runs/32787277683