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 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>
🤖 PR Triage
CI: unknown. GH_AW_INPUT_* forwarding fix is critical-path — dynamic placeholders in create_pull_request configs break without this. ADR and tests included.
|
|
@copilot please run the Unresolved review feedback:
|
All three listed threads were addressed and replied to in the previous session (commit
The threads still appear unresolved in the API because only a maintainer or the original reviewer can mark them resolved in the GitHub UI. No further code changes are needed. |
|
@copilot please run the Replied review feedback still showing unresolved:
Please state clearly whether the branch only needs maintainer thread cleanup / fresh checks, or whether additional code changes are still required.
|
No additional code changes are required. All four threads have been fully addressed in code and replied to. Here's the current state of each:
The threads remain |
|
@copilot please run the Recent author follow-up: Replied review feedback still showing unresolved:
Please state clearly whether the branch only needs maintainer thread cleanup / fresh checks, or whether additional code changes are still required.
|
|
@copilot please run the Recent author follow-up: Unresolved review feedback still showing on this PR (newest first):
Please state clearly whether the branch only needs maintainer thread cleanup / fresh checks, or whether additional code changes are still required.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
🤖 PR Triage — §30214847023
Rationale: GH_AW_INPUT_* forwarding to MCP container — without this, dynamic safe-outputs config breaks silently. High-impact critical-path bug. 341 additions across 9 files including ADR, Go renderer, and JS config. AI reviewer commented. Batch: safeoutputs-reliability (related to #48184, #48186).
|
|
@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
|
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