Skip to content

fix: forward GH_AW_INPUT_* to MCP container env for dynamic safe-outputs config#48099

Open
pelikhan with Copilot wants to merge 11 commits into
mainfrom
copilot/resolve-dynamic-base-branch-issue
Open

fix: forward GH_AW_INPUT_* to MCP container env for dynamic safe-outputs config#48099
pelikhan with Copilot wants to merge 11 commits into
mainfrom
copilot/resolve-dynamic-base-branch-issue

Conversation

Copilot AI commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Since v0.80.0, the safe-outputs MCP server runs in a Docker container with a filtered -e allowlist. GH_AW_INPUT_* vars were never added to that allowlist, so ${GH_AW_INPUT_BASE_BRANCH}-style placeholders in config.json remain unresolved inside the container — causing create_pull_request to fail with No remote refs available for merge-base calculation when using any dynamic safe-outputs field like base-branch: ${{ inputs.base_branch }}.

Changes

pkg/workflow/mcp_setup_generator.go

  • New extractSafeOutputsInputEnvVars(safeOutputConfig) — extracts all GH_AW_INPUT_* name→expression pairs referenced by the safe-outputs config and passes them to generateMCPGatewaySetup.

pkg/workflow/mcp_setup_gateway.go

  • writeMCPGatewayStepEnv now also emits GH_AW_INPUT_*: ${{ inputs.* }} in the Start MCP Gateway step env: block, so the runner process holds the values when docker run is invoked.
  • New appendMCPGatewaySafeOutputsInputEnvFlags appends -e GH_AW_INPUT_* to the docker run command so the container inherits those values.

The compiled output now looks like:

# Start MCP Gateway step
env:
  ...
  GH_AW_INPUT_BASE_BRANCH: ${{ inputs.base_branch }}   # ← new

# docker run command
... -e GH_AW_INPUT_BASE_BRANCH ...                     # ← new

actions/setup/js/safe_outputs_config.cjs

  • New collectUnresolvedInputPlaceholders() 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.go

  • Existing test updated: asserts GH_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.
  • New TestSafeOutputsDynamicBaseBranchPassedToMCPContainer regression test for the exact issue scenario (base-branch: ${{ inputs.base_branch }}).

Generated by 👨‍🍳 PR Sous Chef · gpt54 · 22.4 AIC · ⌖ 9.38 AIC · ⊞ 7.1K ·
Comment /souschef to run again


Generated by 👨‍🍳 PR Sous Chef · gpt54 · 13.6 AIC · ⌖ 7.58 AIC · ⊞ 7.1K ·
Comment /souschef to run again


Generated by 👨‍🍳 PR Sous Chef · gpt54 · 17.5 AIC · ⌖ 7.28 AIC · ⊞ 7.1K ·
Comment /souschef to run again


Generated by 👨‍🍳 PR Sous Chef · gpt54 · 28.5 AIC · ⌖ 13 AIC · ⊞ 7.1K ·
Comment /souschef to run again


run: https://github.com/github/gh-aw/actions/runs/30207935610

Generated by 👨‍🍳 PR Sous Chef · gpt54 · 23.3 AIC · ⌖ 7.77 AIC · ⊞ 7.1K ·
Comment /souschef to run again


Run: https://github.com/github/gh-aw/actions/runs/30221620246

Generated by 👨‍🍳 PR Sous Chef · gpt54 · 7.7 AIC · ⌖ 7.23 AIC · ⊞ 7.1K ·
Comment /souschef to run again

Copilot AI and others added 2 commits July 26, 2026 03:31
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…uts config

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix dynamic base-branch unresolved for safe-outputs fix: forward GH_AW_INPUT_* to MCP container env for dynamic safe-outputs config Jul 26, 2026
Copilot AI requested a review from pelikhan July 26, 2026 03:49
@pelikhan
pelikhan marked this pull request as ready for review July 26, 2026 03:49
Copilot AI review requested due to automatic review settings July 26, 2026 03:49

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread pkg/workflow/mcp_setup_gateway.go
Comment thread actions/setup/js/safe_outputs_config.cjs
@github-actions

github-actions Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Test Quality Sentinel completed test quality analysis.

@github-actions

github-actions Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

PR Code Quality Reviewer completed the code quality review.

@github-actions

github-actions Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Design Decision Gate 🏗️ completed the design decision gate check.

@github-actions

github-actions Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

@github-actions

Copy link
Copy Markdown
Contributor

🧪 Test Quality Sentinel Report

Test Quality Score: 100/100 — Excellent

Analyzed 3 test(s): 3 design, 0 implementation, 0 violation(s).

📊 Metrics (3 tests)
Metric Value
Analyzed 3 (Go: 3, JS: 0)
✅ Design 3 (100%)
⚠️ Implementation 0 (0%)
Edge/error coverage 3 (100%)
Duplicate clusters 0
Inflation No
🚨 Violations 0
Test File Classification Issues
TestSafeOutputsConfigUsesWorkflowInputEnvVarsForDynamicAllowedRepos safe_outputs_dynamic_allowed_repos_test.go design_test / behavioral_contract None
TestSafeOutputsConfigPreservesSecretPlaceholdersOnDisk safe_outputs_dynamic_allowed_repos_test.go design_test / behavioral_contract None
TestSafeOutputsDynamicBaseBranchPassedToMCPContainer safe_outputs_dynamic_allowed_repos_test.go design_test / behavioral_contract None

Verdict

passed. 0% implementation tests (threshold: 30%). All three tests enforce end-to-end behavioral contracts: correct env var injection into compiled YAML, single-quoted heredoc quoting to prevent shell expansion, and -e forwarding to the MCP gateway container. Each test includes both positive and negative regex assertions with descriptive failure messages.

🧪 Test quality analysis by Test Quality Sentinel · sonnet46 · 22.3 AIC · ⌖ 7.9 AIC · ⊞ 8.1K ·
Comment /review to run again

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Test Quality Sentinel: 100/100. 0% implementation tests (threshold: 30%).

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 -e vars 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

@github-actions

Copy link
Copy Markdown
Contributor

Design Decision Gate - ADR Required

This PR makes significant changes to core business logic (144 new lines in pkg/workflow/ and actions/setup/js/) but does not have a linked Architecture Decision Record (ADR).

Draft ADR committed: docs/adr/48099-forward-gh-aw-input-vars-to-mcp-gateway-container.md -- review and complete it before merging.

This PR cannot merge until an ADR is linked in the PR body.

What to do next
  1. Review the draft ADR committed to your branch -- it was generated from the PR diff

  2. Complete the missing sections -- add context the AI could not infer, refine the decision rationale, and list real alternatives you considered

  3. Commit the finalized ADR to docs/adr/ on your branch

  4. Reference the ADR in this PR body by adding a line such as:

    ADR: ADR-48099: Forward GH_AW_INPUT_* Vars to MCP Gateway Container

Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision.

Why ADRs Matter

ADRs 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 Reference

An ADR must contain these four sections to be considered complete:

  • Context -- What is the problem? What forces are at play?
  • Decision -- What did you decide? Why?
  • Alternatives Considered -- What else could have been done?
  • Consequences -- What are the trade-offs (positive and negative)?

All ADRs are stored in docs/adr/ as Markdown files numbered by PR number.

🏗️ ADR gate enforced by Design Decision Gate 🏗️ · sonnet46 · 56.8 AIC · ⌖ 13 AIC · ⊞ 8.5K ·
Comment /review to run again

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 step env: block supplies that value on the runner. The approach is sound.
  • extractSafeOutputsInputEnvVars correctly filters to GH_AW_INPUT_* keys; the nil-on-empty return is consistent with the rest of the codebase.
  • The regression test TestSafeOutputsDynamicBaseBranchPassedToMCPContainer is 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

Comment thread actions/setup/js/safe_outputs_config.cjs

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 collectUnresolvedInputPlaceholders warning is emitted at server.debug level, 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.cjs has no coverage for the new collectUnresolvedInputPlaceholders path
  • 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 TestSafeOutputsDynamicBaseBranchPassedToMCPContainer covers the exact failure scenario end-to-end
  • ✅ Updated test assertions now verify both step env AND docker -e flags — 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

Comment thread actions/setup/js/safe_outputs_config.cjs
Copilot AI requested a review from gh-aw-bot July 26, 2026 09:57
@gh-aw-bot

Copy link
Copy Markdown
Collaborator

@copilot please run the pr-finisher skill, address the unresolved blocking review threads, and confirm whether any ADR/body follow-up is still required before maintainers re-check merge readiness.

Unresolved review feedback:

Please make the next actionable blocker explicit in your reply.

Generated by 👨🍳 PR Sous Chef
Comment /souschef to run again

Generated by 👨‍🍳 PR Sous Chef · gpt54 · 22.4 AIC · ⌖ 9.38 AIC · ⊞ 7.1K ·
Comment /souschef to run again

…olders

Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
@github-actions

Copy link
Copy Markdown
Contributor

🤖 PR Triage

Field Value
Category bug
Risk 🔴 High
Priority Score 72/100
Score Breakdown Impact: 40, Urgency: 22, Quality: 10
Action fast_track — critical bug affecting dynamic input vars in MCP container

CI: unknown. GH_AW_INPUT_* forwarding fix is critical-path — dynamic placeholders in create_pull_request configs break without this. ADR and tests included.

Generated by 🔧 PR Triage Agent · sonnet46 · 26.8 AIC · ⌖ 4.99 AIC · ⊞ 5.7K ·

@gh-aw-bot

Copy link
Copy Markdown
Collaborator

@copilot please run the pr-finisher skill, address the unresolved blocking review threads, and confirm whether any ADR/body follow-up is still required before maintainers re-check merge readiness.

Unresolved review feedback:

Generated by 👨🍳 PR Sous Chef · gpt54 ·
Comment /souschef to run again

Generated by 👨‍🍳 PR Sous Chef · gpt54 · 13.6 AIC · ⌖ 7.58 AIC · ⊞ 7.1K ·
Comment /souschef to run again

Copilot AI commented Jul 26, 2026

Copy link
Copy Markdown
Contributor Author

@copilot please run the pr-finisher skill, address the unresolved blocking review threads, and confirm whether any ADR/body follow-up is still required before maintainers re-check merge readiness.

...

All three listed threads were addressed and replied to in the previous session (commit 40cc205):

  • r3651692896 (forwarding to nested container): addressed in 276a6bd — both TOML env_vars and JSON env blocks in mcp_renderer_builtin.go now dynamically append GH_AW_INPUT_* names. Reply posted.
  • r3651692910 (test coverage): coverage added in ec6280d / 40cc205 — two new test cases in safe_outputs_config.test.cjs for deduplication and false-positive behavior. Reply posted.
  • r3651725169 (server.debugserver.error): fixed in 40cc205 — changed to server.error and added throw new Error(msg) so failures are loud and halt before resolveEnvPlaceholders embeds the literal placeholder. Reply posted.

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.

@gh-aw-bot

Copy link
Copy Markdown
Collaborator

@copilot please run the pr-finisher skill, verify that the replied review threads below are fully resolved, and confirm whether any maintainer-visible follow-up is still needed before merge readiness is re-checked.

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.

Generated by 👨‍🍳 PR Sous Chef · gpt54 · 17.5 AIC · ⌖ 7.28 AIC · ⊞ 7.1K ·
Comment /souschef to run again

Copilot AI commented Jul 26, 2026

Copy link
Copy Markdown
Contributor Author

@copilot please run the pr-finisher skill, verify that the replied review threads below are fully resolved, and confirm whether any maintainer-visible follow-up is still needed before merge readines...

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:

Thread What was done Commit
r3651692896 — pass GH_AW_INPUT_* through nested server config renderSafeOutputsTOML and renderSafeOutputsMCPConfigWithOptions in mcp_renderer_builtin.go both dynamically append GH_AW_INPUT_* names to the nested container's env_vars/env lists 276a6bd
r3651692910 — add test coverage for unresolved-placeholder path Two new test cases in safe_outputs_config.test.cjs: one for deduplication (unresolved+duplicated), one for false-positive guard (resolved) ec6280d / 40cc205
r3651724189 — throw instead of silently continuing throw new Error(msg) added before resolveEnvPlaceholders, so a broken config never gets embedded 40cc205
r3651725169 — use server.error instead of server.debug Changed to server.error so the diagnostic appears unconditionally in MCP server logs 40cc205

The threads remain is_resolved: false in GitHub's API because only a human reviewer can click "Resolve conversation" in the UI. The branch needs maintainer thread resolution and a CI re-trigger (close/reopen or workflow_dispatch) before merge — no further code changes are needed from me.

@gh-aw-bot

Copy link
Copy Markdown
Collaborator

@copilot please run the pr-finisher skill, verify that the replied review threads below are fully resolved, and confirm whether any maintainer-visible follow-up is still needed before merge readiness is re-checked.

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.

Generated by 👨🍳 PR Sous Chef
Comment /souschef to run again

Generated by 👨‍🍳 PR Sous Chef · gpt54 · 28.5 AIC · ⌖ 13 AIC · ⊞ 7.1K ·
Comment /souschef to run again

@gh-aw-bot

Copy link
Copy Markdown
Collaborator

@copilot please run the pr-finisher skill, verify that the author's latest follow-up fully covers the still-unresolved review feedback below, and confirm whether this branch now only needs maintainer thread cleanup and fresh checks or still needs additional code changes.

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.

Generated by 👨🍳 PR Sous Chef
Comment /souschef to run again

Generated by 👨‍🍳 PR Sous Chef · gpt54 · 23.3 AIC · ⌖ 7.77 AIC · ⊞ 7.1K ·
Comment /souschef to run again

Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
@github-actions

Copy link
Copy Markdown
Contributor

🤖 PR Triage — §30214847023

Field Value
Category bug
Risk 🔴 High
Priority 72/100
Breakdown Impact 38 · Urgency 22 · Quality 12
Action fast_track

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).

Generated by 🔧 PR Triage Agent · sonnet46 · 34.1 AIC · ⌖ 7.94 AIC · ⊞ 5.7K ·

@gh-aw-bot

Copy link
Copy Markdown
Collaborator

@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

Generated by 👨‍🍳 PR Sous Chef · gpt54 · 16.9 AIC · ⌖ 7.13 AIC · ⊞ 5.6K ·
Comment /souschef to run again

@gh-aw-bot

Copy link
Copy Markdown
Collaborator

@copilot please run the pr-finisher skill, verify the branch is up to date, and address these unresolved review threads:

Generated by 👨‍🍳 PR Sous Chef · gpt54 · 7.7 AIC · ⌖ 7.23 AIC · ⊞ 7.1K ·
Comment /souschef to run again

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[ARC/DinD] safe-outputs: dynamic base-branch unresolved — GH_AW_INPUT_* not passed to MCP container

4 participants