Fix dangling determine-automatic-lockdown step reference in static enclave sink-visibility policy - #60338
Conversation
…e sink-visibility Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
…clusions Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
…rtifact Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
… gen/reference in sync Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
…test Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
…bBackendIsEnclaveOnly Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The new invariant can miss dangling references when a matching step ID exists in another job.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Fixes static enclave-only workflows that referenced a nonexistent lockdown step output.
Changes:
- Aligns lockdown-step generation with sink-visibility emission.
- Centralizes enclave-only backend detection.
- Adds regression coverage and a dangling-reference invariant.
File summaries
| File | Description |
|---|---|
pkg/workflow/compiler_github_mcp_steps.go |
Generates the lockdown step when static enclave sink visibility requires it. |
pkg/workflow/mcp_environment.go |
Gates guard and visibility variables using shared predicates. |
pkg/workflow/mcp_github_config.go |
Adds shared enclave-only backend detection. |
pkg/workflow/enclave_github_proxy_test.go |
Adds regression tests and the workflow-reference invariant. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| emittedIDs := make(map[string]bool) | ||
| for _, match := range stepIDPattern.FindAllStringSubmatch(lock, -1) { | ||
| emittedIDs[match[1]] = true | ||
| } | ||
| for _, match := range stepOutputRefPattern.FindAllStringSubmatch(lock, -1) { | ||
| stepID := match[1] | ||
| assert.True(t, emittedIDs[stepID], "reference %q has no corresponding emitted step id %q", match[0], stepID) |
|
@copilot address review feedback |
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Addressed in |
When
tools.github: falseis combined with a static GitHub enclave and safe-outputs, the compiler emittedGH_AW_SINK_VISIBILITY: ${{ steps.determine-automatic-lockdown.outputs.visibility }}even though thedetermine-automatic-lockdownstep was never generated (it only synthesized GitHub tool access for the enclave identity, not the primary agent). The dangling reference expanded to an empty string, which MCP Gateway rejects as an invalidsink-visibilityvalue, blocking the agent and enclave from starting.Root cause
collectMCPEnvironmentVariablesgatedGH_AW_SINK_VISIBILITYon a locally synthesized "GitHub tool enabled" map (toolsWithEnclaveGitHubIssues) used to render the enclave-scoped GitHub backend, instead of the authoritativegithubLockdownDetectionStepEnabled(workflowData)predicate that actually controls whether the step is generated.Changes
compiler_github_mcp_steps.go):githubLockdownDetectionStepEnablednow also returnstruewhenstaticEnclaveWriteSinkGuardPolicy(data) != nil, so the lockdown step is generated whenever a static enclave's write-sink policy needs the target visibility — even with the primary agent's GitHub tool disabled.mcp_environment.go):GH_AW_SINK_VISIBILITYand the primary GitHub guard-policy vars (GITHUB_MCP_GUARD_MIN_INTEGRITY/REPOS) are now gated on the samegithubLockdownDetectionStepEnabledpredicate, eliminating the drift between "step exists" and "step output referenced."mcp_github_config.go): extractedgithubBackendIsEnclaveOnly(static or dynamic enclave-only delegation) to a single helper, reused by both the env var logic andgithubGuardPoliciesFromStep, so enclave-only backends consistently derive their guard/write-sink policy from the enclave declaration rather than step outputs.enclave_github_proxy_test.go): addedTestCompileStaticEnclaveOnlyGitHubDisabledSinkVisibilityreproducing the exact reported frontmatter, plus a genericassertNoDanglingStepOutputReferencesinvariant that scans the compiled lock file for anysteps.<id>.outputs.*reference and verifies the referenced step id was actually emitted — catching this class of bug generically, not just for this one field.