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
|
✅ 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
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
|
@copilot please run the skill, review the current branch state, address any still-open reviewer feedback and failed checks, and reply with whether this PR is ready for maintainer review. Run: https://github.com/github/gh-aw/actions/runs/30215321924
|
|
@copilot please run the
|
… merge Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
All four review threads were already resolved on the previous commits. This pass also fixed a local test failure ( Local validation green: |
|
@copilot please run the
|
Branch is up to date and all review threads are resolved. Local validation ( |
|
@copilot please run the
|
Branch is up to date at |
🤖 PR Triage
Summary: Critical runtime bug — Next action: Human review + resolve outstanding CHANGES_REQUESTED, then fast-track merge.
|
|
@copilot please run the
|
All previous review threads are resolved and local validation passes (branch HEAD
Hand-off: A maintainer must (a) dismiss or re-trigger the stale |
|
@copilot please run the
|
No new actionable items. Current state (branch HEAD
Nothing further is actionable from the agent side. A maintainer must dismiss the stale |
|
@copilot please run the
|
Branch is already up to date with
|
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 }}).run: https://github.com/github/gh-aw/actions/runs/30207935610
Run: https://github.com/github/gh-aw/actions/runs/30221620246
Run: https://github.com/github/gh-aw/actions/runs/30223671533
Run: https://github.com/github/gh-aw/actions/runs/30224763533
Run: https://github.com/github/gh-aw/actions/runs/30228781602
Run: https://github.com/github/gh-aw/actions/runs/30230557448
Run: https://github.com/github/gh-aw/actions/runs/30233154470