Fix cross-repo dynamic target-repo silently dropping PR (forward GH_AW_INPUT_* to agent step env) - #48598
Conversation
… GH_AW_INPUT_* to agent step env Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (98 additions detected). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
Fixes dynamic cross-repository safe-output targets by forwarding generated GH_AW_INPUT_* variables into agent execution environments.
Changes:
- Copies safe-output input variables into engine step environments.
- Adds helper-level and Copilot/Codex regression tests.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/safe_outputs_env.go |
Forwards dynamic input variables to agent steps. |
pkg/workflow/safe_output_helpers_test.go |
Tests shared environment propagation. |
pkg/workflow/copilot_engine_test.go |
Verifies Copilot step forwarding. |
pkg/workflow/codex_engine_test.go |
Verifies Codex step forwarding. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Medium
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix is correct and well-tested. maps.Copy(env, data.SafeOutputsInputEnvVars) in applySafeOutputEnvToMap correctly ensures GH_AW_INPUT_* vars reach TOML-based engine steps, fixing the silent PR-drop when target-repo uses dynamic ${{ inputs.* }} expressions.
No issues found — the change is minimal, safe for gateway-based engines (redundant but harmless), and covered by three targeted tests.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 11.8 AIC · ⌖ 5.21 AIC · ⊞ 5K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — this is a well-targeted bug fix with solid regression tests. Approving.
📋 Key Themes & Highlights
Positive Highlights
- ✅ Root cause is correctly identified and precisely fixed with a single
maps.Copycall in the right place (applySafeOutputEnvToMap), affecting all engine types uniformly - ✅ Three regression tests added — one for the helper function, one each for Copilot and Codex engines — covering exactly the failing scenario
- ✅ Test doc comments clearly document the TOML forwarding chain dependency, making the intent readable for future maintainers
- ✅ The fix is additive (no deletions) and harmless for non-TOML engines (Claude, Pi) where the vars are already present in the gateway step
- ✅ PR description traces the root cause through the full env-var forwarding chain with precision
Minor Observations
- The test values for
SafeOutputsInputEnvVarsuse raw${{ inputs.owner }}expressions rather than resolved placeholders, which accurately reflects the compile-time state stored inWorkflowData. This is correct. - No integration or end-to-end test for the full dynamic target-repo path exists, but that is a pre-existing gap, not introduced here.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 21.2 AIC · ⌖ 4.63 AIC · ⊞ 6.8K
Comment /matt to run again
🧪 Test Quality Sentinel Report
📊 Metrics (3 tests)
📋 Test Details (3 tests)
Design Invariant Coverage✅ All 3 tests verify the core design invariant: Environment chain verified:
Strengths
Observations
Verdict
|
|
🎉 This pull request is included in a new release. Release: |
When
target-repouses a dynamic${{ inputs.owner }}/${{ inputs.repo }}expression, the patch file is named using the unexpanded${GH_AW_INPUT_OWNER}-${GH_AW_INPUT_REPO}slug while the consumer reconstructs the filename from the Actions-expanded value — the two never match, the PR is silently dropped, and the run concludes green.Root cause
buildSafeOutputsConfigRuntimeDatareplaces${{ inputs.* }}with${GH_AW_INPUT_*}placeholders inconfig.json. The safe-outputs MCP server is expected to resolve these at runtime viaresolveEnvPlaceholders(process.env). For TOML-based engines (Copilot, Codex), the container receives its environment via the TOMLenv_varsforwarding chain:The
GH_AW_INPUT_*vars were only injected into the "Start MCP Gateway" step env — not the agent execution step env. Because Copilot and Codex start the safe-outputs container from within their own process (not the long-running gateway), the vars were absent from the container, leaving the placeholders unexpanded in the patch filename.Fix
pkg/workflow/safe_outputs_env.goAdd
maps.Copy(env, data.SafeOutputsInputEnvVars)toapplySafeOutputEnvToMap, which is called by all engineGetExecutionStepsimplementations:SafeOutputsInputEnvVarsis populated bygenerateMCPSetup(phase 3), beforeGetExecutionStepsis called (phase 4), so the data is always available. For JSON/gateway-based engines (Claude, Pi), the vars are already present in the gateway step env — adding them to the agent step is redundant but harmless.Tests
TestApplySafeOutputEnvToMap— new table entry verifyingGH_AW_INPUT_*propagates throughapplySafeOutputEnvToMapTestCopilotEngineForwardsSafeOutputsInputEnvVars— verifies Copilot execution step includes the vars whenSafeOutputsInputEnvVarsis populatedTestCodexEngineForwardsSafeOutputsInputEnvVars— same for Codex