Conversation
- Remove GH_HOST, GITHUB_API_URL, GITHUB_GRAPHQL_URL, NODE_EXTRA_CA_CERTS, GH_AW_ORIGINAL_* writes from start_difc_proxy.sh - Remove all GITHUB_ENV restore/clear logic from stop_difc_proxy.sh - Add injectProxyEnvIntoCustomSteps() in compiler_difc_proxy.go that uses YAML parse+reserialize to add proxy env vars as step-level env on each custom step (preserving existing env vars like GH_TOKEN) - Remove generateSetGHRepoAfterDIFCProxyStep() standalone step; GH_REPO is now included in the per-step env block - Inject proxy env into custom steps in compiler_yaml_main_job.go when hasDIFCProxyNeeded returns true - Update tests: replace TestBuildSetGHRepoStepYAML/TestGenerateSetGHRepo with TestProxyEnvVars/TestInjectProxyEnvIntoCustomSteps; update TestDIFCProxyStepOrderInCompiledWorkflow for new behavior - Recompile 4 affected workflow lock files Agent-Logs-Url: https://github.com/github/gh-aw/sessions/c3114091-8d7b-4283-8647-91185fd5eb60 Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
… proxy Generated by the Design Decision Gate workflow (run #24431428234). The PR makes >100 new lines of changes to pkg/workflow/ without a linked ADR. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…for-review-comments Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
…omments and use ordered fields Agent-Logs-Url: https://github.com/github/gh-aw/sessions/4e545256-bb6a-4606-be79-362d0f6dcd96 Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
…ProxyEnvIntoCustomSteps Agent-Logs-Url: https://github.com/github/gh-aw/sessions/4e545256-bb6a-4606-be79-362d0f6dcd96 Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes DIFC proxy custom-step env injection so compiled workflows preserve inline uses: version comments and keep a stable, reviewer-friendly step field order (e.g., name/uses before injected env), reducing noisy lockfile diffs and restoring human-readable action versions for manifest generation.
Changes:
- Update main-job step emission to inject DIFC proxy routing env vars into custom steps (step-level
env) before rendering. - Rework
injectProxyEnvIntoCustomStepsto preserveuses: ... # vXcomments, enforce step field ordering, and unquoteusesvalues after YAML re-marshaling. - Update DIFC proxy scripts/tests and recompile affected workflow lock files to reflect the new stable output.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/compiler_yaml_main_job.go | Emits custom steps via a proxy-env-injected YAML when DIFC proxy is active. |
| pkg/workflow/compiler_difc_proxy.go | Implements step YAML round-trip that preserves uses version comments and orders step fields before marshaling. |
| pkg/workflow/compiler_difc_proxy_test.go | Updates tests to validate env injection, ordering, and uses # vX comment preservation/unquoting. |
| actions/setup/sh/start_difc_proxy.sh | Removes $GITHUB_ENV writes; start script becomes lifecycle-only per step-scoped env design. |
| actions/setup/sh/stop_difc_proxy.sh | Removes $GITHUB_ENV restore/clear logic; stop script becomes lifecycle-only. |
| docs/adr/26322-step-scoped-env-injection-for-difc-proxy.md | Adds ADR documenting step-scoped env injection approach and rationale. |
| .mcp.json | Adds MCP server configuration for gh aw mcp-server. |
| .github/workflows/stale-repo-identifier.lock.yml | Recompiled to reflect stable step ordering and restored uses # vX comments. |
| .github/workflows/issue-arborist.lock.yml | Recompiled to reflect stable step ordering and injected proxy env. |
| .github/workflows/daily-issues-report.lock.yml | Recompiled to reflect stable step ordering and injected proxy env. |
| .github/workflows/contribution-check.lock.yml | Recompiled to reflect stable step ordering and injected proxy env. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 11/11 changed files
- Comments generated: 4
| 1. When the DIFC proxy is active for a main-job workflow, implementations **MUST** inject proxy routing env vars (`GH_HOST`, `GH_REPO`, `GITHUB_API_URL`, `GITHUB_GRAPHQL_URL`, `NODE_EXTRA_CA_CERTS`) as step-level `env:` blocks on each custom step. | ||
| 2. Implementations **MUST NOT** write proxy routing env vars to `$GITHUB_ENV` (the runner's persistent env file) for the purpose of routing custom steps through the proxy. | ||
| 3. Implementations **MUST** preserve existing step-level env vars (e.g. `GH_TOKEN`) when injecting proxy vars; proxy vars **MUST NOT** overwrite user-defined env vars that share the same key. | ||
| 4. If YAML parsing of the custom steps string fails, implementations **SHOULD** log the error and return the original custom steps string unchanged rather than aborting compilation. |
There was a problem hiding this comment.
ADR normative requirement says proxy vars MUST NOT overwrite user-defined step env values that share the same key, but the current proxy env injection implementation overwrites any existing values for GH_HOST/GH_REPO/GITHUB_API_URL/GITHUB_GRAPHQL_URL/NODE_EXTRA_CA_CERTS. Please reconcile the ADR with the intended behavior (either update the ADR requirement or change the merge logic so existing values win).
| **Date**: 2026-04-15 | ||
| **Status**: Draft | ||
| **Deciders**: lpcox, Copilot | ||
|
|
There was a problem hiding this comment.
This ADR is marked Status: Draft and includes a note that it must be reviewed and finalized before the PR can merge. Please update the status/metadata (and remove or revise the draft-only footer) so the document reflects a finalized decision before merge.
| envMap, ok := step["env"].(map[string]any) | ||
| if !ok { | ||
| envMap = make(map[string]any) | ||
| } | ||
| for k, v := range proxyEnv { | ||
| envMap[k] = v | ||
| } | ||
| step["env"] = envMap |
There was a problem hiding this comment.
injectProxyEnvIntoCustomSteps unconditionally assigns proxy env keys into the step env map, which will overwrite any user-provided values for those same keys. If overwriting is intentional for routing, it should be explicitly documented and tested; if not intentional, change the merge so existing step env values take precedence and only missing keys are added.
| "github": map[string]any{"min-integrity": "approved"}, | ||
| desc: "existing env var GH_TOKEN should be preserved alongside proxy vars", | ||
| }, | ||
| { |
There was a problem hiding this comment.
The test suite doesn't currently cover the conflict case where a custom step already sets one of the proxy routing env keys (e.g., GH_HOST/GITHUB_API_URL). Add a test case that asserts the intended precedence (overwrite vs preserve) so future refactors don't accidentally change routing behavior.
| { | |
| { | |
| name: "conflicting proxy routing env vars are overwritten by proxy values", | |
| customSteps: "steps:\n- name: Step with conflicting proxy env\n env:\n GH_TOKEN: ${{ github.token }}\n GH_HOST: example.com\n GITHUB_API_URL: https://example.com/api/v3\n run: gh issue list\n", | |
| expectedContains: []string{ | |
| "GH_TOKEN: ${{ github.token }}", | |
| "GH_HOST: localhost:18443", | |
| "GITHUB_API_URL: https://localhost:18443/api/v3", | |
| "GH_REPO: ${{ github.repository }}", | |
| "GITHUB_GRAPHQL_URL: https://localhost:18443/api/graphql", | |
| "NODE_EXTRA_CA_CERTS: /tmp/gh-aw/proxy-logs/proxy-tls/ca.crt", | |
| }, | |
| expectedAbsent: []string{ | |
| "GH_HOST: example.com", | |
| "GITHUB_API_URL: https://example.com/api/v3", | |
| }, | |
| desc: "proxy routing env vars should take precedence over conflicting custom values", | |
| }, | |
| { |
🧪 Test Quality Sentinel ReportTest Quality Score: 90/100✅ Excellent
Test Classification Details
Analysis Detail📋 Per-Test Breakdown
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Test Quality Sentinel: 90/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). All three changed/added tests verify observable behavioral contracts, including the YAML version-comment preservation and field-ordering invariants.
…env injection Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Commit pushed:
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic ( AI has analyzed the PR diff and generated a draft ADR to help you get started: 📄 Draft ADR: The draft captures the three design decisions implicit in this fix:
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 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 ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
References: §24435732366 Note 🔒 Integrity filter blocked 1 itemThe following item were blocked because they don't meet the GitHub integrity level.
To allow these resources, lower tools:
github:
min-integrity: approved # merged | approved | unapproved | none
|
|
@copilot resolve the merge conflicts in this pull request |
…ict test Agent-Logs-Url: https://github.com/github/gh-aw/sessions/1d06d5d6-aa21-49cb-803a-685a0c692d48 Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Merge conflicts resolved in 89a6106. Also took the opportunity to address the reviewer feedback in the same commit:
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot resolve the merge conflicts in this pull request |
…review-comments # Conflicts: # .github/workflows/contribution-check.lock.yml # .github/workflows/daily-issues-report.lock.yml # .github/workflows/issue-arborist.lock.yml # .github/workflows/stale-repo-identifier.lock.yml # docs/adr/26322-step-scoped-env-injection-for-difc-proxy.md # pkg/workflow/compiler_difc_proxy.go # pkg/workflow/compiler_difc_proxy_test.go Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
injectProxyEnvIntoCustomSteps(introduced in #26322) re-marshaled custom steps viayaml.MarshalWithOptions(map[string]any{...}), which stripped inline# vXcomments fromuses:lines and serialized fields alphabetically (env:beforename:/uses:). This causedgh-aw-manifestto record bare SHAs instead of human-readable tags, and produced noisy diffs in compiled lock files.Changes
# vXannotations fromuses:lines beforeyaml.Unmarshal(which strips comments), then re-apply them to theusesvalue post-processing — same pattern asDeduplicateRuntimeSetupStepsFromCustomSteps.OrderMapFields(constants.PriorityStepFields)before marshaling soname/usesstay ahead ofenv.unquoteUsesWithCommentson the result since the YAML marshaller quotes strings containing#.name-before-envordering, and proxy routing var precedence over conflicting user-defined values.actions/upload-artifactandgithub/stale-reposmanifest entries now record"version":"v7"/"version":"v9.0.8"instead of SHA strings, anduses:lines regain their# vXannotations.GH_HOST,GH_REPO,GITHUB_API_URL,GITHUB_GRAPHQL_URL,NODE_EXTRA_CA_CERTS) intentionally overwrite any user-defined values for those keys to guarantee correct proxy routing; all other step env vars are preserved.Before/after in compiled lock file: