fix: forward GH_AW_INPUT_* to MCP container env for dynamic safe-outputs config#48099
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…uts config Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes dynamic safe-output inputs by forwarding GH_AW_INPUT_* variables toward the MCP runtime and improving unresolved-placeholder diagnostics.
Changes:
- Extracts and forwards input-derived safe-output environment variables.
- Adds runtime diagnostics and regression coverage.
- Updates release metadata and workflow skill references.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/mcp_setup_generator.go |
Extracts safe-output input variables. |
pkg/workflow/mcp_setup_gateway.go |
Forwards variables to the outer gateway container. |
actions/setup/js/safe_outputs_config.cjs |
Logs unresolved input placeholders. |
pkg/workflow/safe_outputs_dynamic_allowed_repos_test.go |
Adds compilation regression assertions. |
.github/skills/agentic-workflows/SKILL.md |
Adds the release-workflow reference. |
.changeset/fix-safe-outputs-dynamic-input-mcp-container.md |
Documents the patch. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Medium
| containerCmd.WriteString(" -v ${DOCKER_SOCK_PATH}:/var/run/docker.sock") | ||
| appendMCPGatewayBaseEnvFlags(&containerCmd, payloadPathPrefix) | ||
| appendMCPGatewayConditionalEnvFlags(&containerCmd, workflowData, engine, hasGitHub, githubTool, tools) | ||
| appendMCPGatewaySafeOutputsInputEnvFlags(&containerCmd, safeOutputsInputEnvVars) |
There was a problem hiding this comment.
This was addressed in commit 276a6bd. Both safe-outputs renderer formats now dynamically append GH_AW_INPUT_* entries:
- TOML (
renderSafeOutputsTOML): appends names fromworkflowData.SafeOutputsInputEnvVarsto theenv_varslist (seemcp_renderer_builtin.go:112-114) - JSON (
renderSafeOutputsMCPConfigWithOptions): appends{name, name, false}entries to theenvVarsslice (seemcp_renderer_builtin.go:297-305)
The nested safe-outputs server container now receives the GH_AW_INPUT_* values and can resolve ${GH_AW_INPUT_…} placeholders at runtime.
| const unresolvedInputs = collectUnresolvedInputPlaceholders(configFileContent); | ||
| if (unresolvedInputs.length > 0) { | ||
| const varList = unresolvedInputs.join(", "); | ||
| server.debug( | ||
| `ERR_CONFIG: Unresolved workflow input placeholder(s) in safe-outputs config: ${varList}. The values were not passed to the MCP container. Verify that the workflow was compiled with a version that forwards GH_AW_INPUT_* to the container env.` | ||
| ); | ||
| console.error(`[safe_outputs_config] ERR_CONFIG: Unresolved workflow input placeholder(s): ${varList}`); |
There was a problem hiding this comment.
Coverage was added in commit ec6280d. The test file now includes two cases for collectUnresolvedInputPlaceholders:
- Unresolved + duplicated:
GH_AW_INPUT_FOOappears twice but the env var is not set — verifies exactly oneconsole.errorand oneserver.errorcall (deduplication check) - Resolved:
GH_AW_INPUT_BASE_BRANCHis set in the env — verifies no diagnostic is emitted
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (3 tests)
Verdict
|
There was a problem hiding this comment.
The fix is correct and well-implemented. The root cause (GH_AW_INPUT_* vars missing from the docker -e allowlist) is clearly identified and addressed at both the compiler level (Go) and with appropriate diagnostic logging (JS). Tests are updated and a focused regression test added.
Two pre-existing review comments cover the remaining gaps:
- Nested-container forwarding (inner containers do not inherit
-evars from the outer MCP gateway) - Missing JS unit-test coverage for the new unresolved-placeholder warning path
No additional blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 21.4 AIC · ⌖ 4.51 AIC · ⊞ 5K
Design Decision Gate - ADR RequiredThis PR makes significant changes to core business logic (144 new lines in Draft ADR committed: This PR cannot merge until an ADR is linked in the PR body. What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. Why ADRs MatterADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
There was a problem hiding this comment.
REQUEST_CHANGES — one medium correctness issue must be fixed before merging.
Blocking issue
The new collectUnresolvedInputPlaceholders diagnostic in safe_outputs_config.cjs only logs and continues. Since resolveEnvPlaceholders uses ?? match as its fallback, an unresolved ${GH_AW_INPUT_BASE_BRANCH} is preserved verbatim in the parsed config. The safe-outputs MCP server then receives the literal placeholder as the base_branch value — the same broken behavior the PR is fixing — just with a warning attached. The check needs to throw after logging so the failure is explicit rather than silent.
Other observations (non-blocking)
- The Go changes are correct:
-e VARNAME(without=value) is the standard Docker pattern for inheriting a value from the host process environment, and the stepenv:block supplies that value on the runner. The approach is sound. extractSafeOutputsInputEnvVarscorrectly filters toGH_AW_INPUT_*keys; the nil-on-empty return is consistent with the rest of the codebase.- The regression test
TestSafeOutputsDynamicBaseBranchPassedToMCPContaineris well-structured and covers the exact failure scenario.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 · 52.1 AIC · ⌖ 4.74 AIC · ⊞ 5.7K
Comment /review to run again
| // Warn about any GH_AW_INPUT_* placeholders that cannot be resolved before substitution. | ||
| // This should not happen when the workflow is compiled correctly (the compiler ensures these | ||
| // env vars are in the MCP gateway step env and the docker -e allowlist), but if it does the | ||
| // error will surface later as a cryptic "No remote refs available for merge-base calculation" |
There was a problem hiding this comment.
Silent continuation after detecting unresolved placeholders: logging the error but not halting means resolveEnvPlaceholders will still embed the literal ${GH_AW_INPUT_*} string as the field value — reproducing the exact broken behavior this code is meant to diagnose.
💡 Suggested fix
resolveEnvPlaceholders falls back via ?? match, so when the env var is absent it preserves the raw ${GH_AW_INPUT_BASE_BRANCH} string in the config. The safe-outputs MCP server then receives that literal placeholder as base_branch, which causes the "No remote refs" error — same as before, just with a warning in the log.
The diagnostic is useful but the code should also throw:
if (unresolvedInputs.length > 0) {
const varList = unresolvedInputs.join(", ");
const msg = `ERR_CONFIG: Unresolved workflow input placeholder(s) in safe-outputs config: ${varList}. The values were not passed to the MCP container.`;
server.debug(msg);
console.error(`[safe_outputs_config] ${msg}`);
throw new Error(msg); // surface the root cause immediately
}This makes failures loud and attributable rather than silently producing a wrong config.
There was a problem hiding this comment.
Fixed in commit 40cc205. The unresolved placeholder detection now throws before calling resolveEnvPlaceholders, so the literal ${GH_AW_INPUT_*} string is never embedded as a config value. The throw is caught by the outer try-catch which falls back to an empty config — the MCP server starts with no tools enabled rather than silently using a broken config that produces a cryptic downstream error.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — the fix is well-targeted and accompanied by a solid regression test. One issue worth addressing before merge.
📋 Key Themes & Highlights
Key Themes
- Diagnostic severity: the new
collectUnresolvedInputPlaceholderswarning is emitted atserver.debuglevel, which is suppressed in normal runs — it won't help anyone diagnose a misconfigured deployment where this code path is actually reached - Test coverage gap (noted in existing review):
safe_outputs_config.test.cjshas no coverage for the newcollectUnresolvedInputPlaceholderspath - Container scope concern (noted in existing review): the MCP gateway container forwarding only helps the outer gateway; worth confirming the safe-outputs server actually reads env vars from the container context it runs in
Positive Highlights
- ✅ Clean extraction of
extractSafeOutputsInputEnvVars— single responsibility, easy to test - ✅ Regression test
TestSafeOutputsDynamicBaseBranchPassedToMCPContainercovers the exact failure scenario end-to-end - ✅ Updated test assertions now verify both step env AND docker
-eflags — the right invariants to protect - ✅ Good inline comments explaining why each piece is necessary
- ✅ Changeset entry accurately describes the impact
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 30.8 AIC · ⌖ 4.7 AIC · ⊞ 6.7K
Comment /matt to run again
| // error will surface later as a cryptic "No remote refs available for merge-base calculation" | ||
| // rather than pointing at the root cause. | ||
| const unresolvedInputs = collectUnresolvedInputPlaceholders(configFileContent); | ||
| if (unresolvedInputs.length > 0) { |
There was a problem hiding this comment.
[/diagnosing-bugs] The unresolved-placeholder diagnostic is emitted at server.debug level, which is suppressed in normal runs — defeating the goal of surfacing failures with a clear message instead of a cryptic error.
💡 Suggested fix
Change server.debug to server.error (or at minimum server.warn) so the message appears unconditionally:
server.error(
`ERR_CONFIG: Unresolved workflow input placeholder(s) in safe-outputs config: ${varList}. The values were not passed to the MCP container.`
);The console.error call on the next line is a good fallback, but server.error/server.warn matches the structured logging used elsewhere in this module.
@copilot please address this.
There was a problem hiding this comment.
Fixed in commit 40cc205. Changed server.debug → server.error so the unresolved placeholder diagnostic is emitted unconditionally (not suppressed by debug log filtering). Both server.error and console.error are called before throwing, ensuring the root cause is visible in the MCP server logs and on stderr.
🤖 PR Triage — §30192186368
Why: MCP container env vars — Next: Expedite human review. Verify CI passes before merge.
|
|
@copilot please run the Unresolved review feedback:
Please make the next actionable blocker explicit in your reply.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
…est.cjs Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the blocking correctness feedback. The previous fix only forwarded Changes in this update:
The code reviewer flagged using |
|
@copilot please run the Unresolved review feedback:
Please make the next actionable blocker explicit in your reply.
|
…olders Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Since v0.80.0, the safe-outputs MCP server runs in a Docker container with a filtered
-eallowlist.GH_AW_INPUT_*vars were never added to that allowlist, so${GH_AW_INPUT_BASE_BRANCH}-style placeholders inconfig.jsonremain unresolved inside the container — causingcreate_pull_requestto fail withNo remote refs available for merge-base calculationwhen using any dynamic safe-outputs field likebase-branch: ${{ inputs.base_branch }}.Changes
pkg/workflow/mcp_setup_generator.goextractSafeOutputsInputEnvVars(safeOutputConfig)— extracts allGH_AW_INPUT_*name→expression pairs referenced by the safe-outputs config and passes them togenerateMCPGatewaySetup.pkg/workflow/mcp_setup_gateway.gowriteMCPGatewayStepEnvnow also emitsGH_AW_INPUT_*: ${{ inputs.* }}in the Start MCP Gateway stepenv:block, so the runner process holds the values whendocker runis invoked.appendMCPGatewaySafeOutputsInputEnvFlagsappends-e GH_AW_INPUT_*to the docker run command so the container inherits those values.The compiled output now looks like:
actions/setup/js/safe_outputs_config.cjscollectUnresolvedInputPlaceholders()detects and logs any${GH_AW_INPUT_*}that remains unresolved at load-time, so failures surface with a clear message instead of a cryptic merge-base error.pkg/workflow/safe_outputs_dynamic_allowed_repos_test.goGH_AW_INPUT_*appears in both the Generate Safe Outputs Config and Start MCP Gateway step env blocks, and that-e GH_AW_INPUT_*is present in the docker run command.TestSafeOutputsDynamicBaseBranchPassedToMCPContainerregression test for the exact issue scenario (base-branch: ${{ inputs.base_branch }}).