fix: tools.github: false does not override tools.github.* from shared imports#26323
fix: tools.github: false does not override tools.github.* from shared imports#26323
Conversation
…orts In parser.MergeTools, when both base and additional have the same key but types differ (or for same non-array, non-map type conflicts), remove the overwrite that replaced the base value with the additional value. The base value (main workflow) already sits in result from maps.Copy and should take precedence over the additional (imported) value, consistent with the documented 'workflow overrides imports' principle. Agent-Logs-Url: https://github.com/github/gh-aw/sessions/65d4ab13-b9a5-4b1f-9521-cac1f0699508 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
🧪 Test Quality Sentinel ReportTest Quality Score: 90/100✅ Excellent test quality
Test Classification Details
Flagged Tests — Requires ReviewNone — all tests are high-quality behavioral tests. Language SupportTests analyzed:
Score Breakdown
Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.
|
![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 4 new test scenarios enforce the behavioral contract that tools.github: false overrides imported configurations, tested at both unit and end-to-end levels.
There was a problem hiding this comment.
Pull request overview
Fixes a merge-precedence bug where tools.github: false in a main workflow could be overridden by imported workflows that define tools.github as an object (e.g., mode: remote), unintentionally enabling the GitHub MCP server.
Changes:
- Adjust
parser.MergeToolsso base (main workflow) values win on type mismatches and scalar conflicts (imports can’t override). - Add unit tests covering
falsevs map precedence and nested scalar base-wins behavior; update overlapping-keys expectation for JSON tool merges. - Add an end-to-end compilation test ensuring
tools.github: falseprevents GitHub MCP header emission when an import enables remote mode.
Show a summary per file
| File | Description |
|---|---|
| pkg/parser/tools_merger.go | Changes merge behavior so base workflow values are not overwritten on type mismatches / scalar conflicts. |
| pkg/parser/frontmatter_merge_test.go | Adds/updates unit tests to lock in base-wins semantics for tool merges. |
| pkg/workflow/importable_tools_test.go | Adds integration-style test asserting tools.github: false disables GitHub MCP output even when imports enable it. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 2
| result[key] = recursiveMerged | ||
| } else { | ||
| // Not both same type, overwrite with new value | ||
| result[key] = newValue | ||
| } | ||
| // Type mismatch (or same non-array, non-map type): base value takes precedence. | ||
| // result[key] already contains existingValue from maps.Copy above — no action needed. |
There was a problem hiding this comment.
In the map+"allowed" merge path above, maps.Copy(mergedMap, newMap) means imported tool configs can overwrite other scalar fields from the base workflow whenever both maps include an allowed key (e.g., tools.github.mode could be overridden if both base and import specify allowed). This contradicts the documented "workflow overrides imports" semantics. Consider merging maps in a base-wins way (e.g., recursive merge that keeps existing scalar values) while still concatenating/deduping allowed.
| if strings.Contains(workflowData, "X-MCP-Toolsets") { | ||
| t.Error("Expected compiled workflow to NOT contain X-MCP-Toolsets (github MCP server should be disabled by tools.github: false)") | ||
| } |
There was a problem hiding this comment.
This assertion only checks for the X-MCP-Toolsets header, which is specific to GitHub remote mode. To make the test robust against future renderer changes (or a scenario where the GitHub MCP server is still enabled but without that header), also assert that GitHub MCP server indicators are absent (e.g., the remote URL api.githubcopilot.com/mcp/ and/or the local container image ghcr.io/github/github-mcp-server).
| if strings.Contains(workflowData, "X-MCP-Toolsets") { | |
| t.Error("Expected compiled workflow to NOT contain X-MCP-Toolsets (github MCP server should be disabled by tools.github: false)") | |
| } | |
| githubMCPIndicators := []string{ | |
| "X-MCP-Toolsets", | |
| "api.githubcopilot.com/mcp/", | |
| "ghcr.io/github/github-mcp-server", | |
| } | |
| for _, indicator := range githubMCPIndicators { | |
| if strings.Contains(workflowData, indicator) { | |
| t.Errorf("Expected compiled workflow to NOT contain GitHub MCP server indicator %q (github MCP server should be disabled by tools.github: false)", indicator) | |
| } | |
| } |
…tics 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 (120 new lines in AI has analyzed the PR diff and generated a draft ADR to help you get started: 📄 Draft ADR: The draft captures the core design decision: 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
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 review all comments |
… test assertions - In the tools_merger.go allowed-array merge path, reverse the maps.Copy order so import values are written first and base values overwrite them, preserving base-wins semantics for all scalar fields - In TestGitHubToolFalseOverridesImport, check all three GitHub MCP server indicators (X-MCP-Toolsets, api.githubcopilot.com/mcp/, ghcr.io/github/ github-mcp-server) to make the assertion robust against renderer changes Agent-Logs-Url: https://github.com/github/gh-aw/sessions/0e289010-b2b3-408e-8bcd-995f8a293a0a Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Both review comments addressed in 6c51636:
|
|
@copilot the GitHub MCP server should merge unless it is "false" in the main workflows, which turns it off. |
The current implementation (6c51636) already behaves this way:
The reversal of |
|
@copilot review changes in lock files, specially actions cache ids |
All lock file changes are correct side effects of the merge fix and were regenerated by
Cache key changes (9 workflows): The pattern is consistent — all changes come from
All resulting cache keys are correct. The direction of each change is: more specific parent-import configuration now wins over a generic child-import
|
|
Hey This PR looks ready for maintainer review. ✅
|
|
@copilot add tests to enforce those scenarios |
…hub:false merge precedence Agent-Logs-Url: https://github.com/github/gh-aw/sessions/af55a946-53e7-428f-b6ea-25d987f9fba4 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/af55a946-53e7-428f-b6ea-25d987f9fba4 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Added tests in e985e79: Unit tests (
End-to-end tests (
|
When a main workflow sets
tools.github: false, imported shared workflows withtools.github: mode: remote(or any other github tool config) silently win, leaving the GitHub MCP server enabled against the user's explicit intent.Root cause
parser.MergeTools(base, additional)had a fallback that overwrote the base value with the additional value on type mismatches (e.g.boolvsmap):baseis the main workflow;additionalis the import. The base value already lives inresultaftermaps.Copy(result, base), so the overwrite was backwards.In addition, the map+
allowed-array merge path was copying the import map on top of the base map (maps.Copy(mergedMap, newMap)last), which let imported scalar fields (e.g.mode) overwrite base scalar fields even when the base explicitly set them.Changes
pkg/parser/tools_merger.go— Remove theresult[key] = newValueoverwrite in the type-mismatch branch. Base value wins for all non-array, non-map conflicts (type mismatch and same scalar type), consistent with the documented "workflow overrides imports" principle. Fix the allowed-array merge path to write the import map first and the base map second, so base scalar fields win while import-only keys are still contributed andallowedarrays are concatenated.pkg/parser/frontmatter_merge_test.go— Add unit tests:falseoverrides map, map not overridden byfalse, nested scalar base-wins; updateobjects with overlapping keysexpectation to match new semantics.pkg/workflow/importable_tools_test.go— AddTestGitHubToolFalseOverridesImport: end-to-end compilation of a workflow withtools.github: falseimporting a shared workflow withtools.github: mode: remote, asserting all GitHub MCP server indicators (X-MCP-Toolsets,api.githubcopilot.com/mcp/,ghcr.io/github/github-mcp-server) are absent from the compiled output.