Conversation
|
Hey A few things to address before this is ready for review:
When you're ready to implement, here's a prompt to continue:
|
…version, and sandbox.agent: false keys Agent-Logs-Url: https://github.com/github/gh-aw/sessions/2bfe937f-6c94-4adb-99af-e9bf1e4018c7 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot these codemods are only enabled if the script does not have strict: false |
🧪 Test Quality Sentinel ReportTest Quality Score: 67/100
Test Classification DetailsView all 20 test classifications
Flagged Tests — Requires Review
|
| Test file | Test lines | Prod lines | Ratio |
|---|---|---|---|
codemod_sandbox_agent_false_removal_test.go |
242 | 49 | 4.9:1 |
codemod_sandbox_mcp_internal_test.go |
301 | 75 | 4.0:1 |
Both exceed the 2:1 threshold. This is largely unavoidable here — each test case embeds a full YAML frontmatter fixture and a corresponding map[string]any. This is not a sign of low-quality tests; the high ratio reflects the verbosity of the test data, not redundant code.
Language Support
Tests analyzed:
- 🐹 Go (
*_test.go): 20 tests — all unit (//go:build !integration) ✅ - 🟨 JavaScript (
*.test.cjs,*.test.js): 0 tests changed in this PR
Verdict
✅ Check passed. 15% of new tests are implementation tests (threshold: 30%). The behavioral coverage is strong — 17 of 20 new tests verify observable codemod outputs across a thorough set of input scenarios including no-op paths, partial key presence, type variants, and markdown body preservation. The flagged items (metadata-only tests, missing assertion messages, test inflation) are non-blocking suggestions.
📖 Understanding Test Classifications
Design Tests (High Value) verify what the system does:
- Assert on observable outputs, return values, or state changes
- Cover error paths and boundary conditions
- Would catch a behavioral regression if deleted
- Remain valid even after internal refactoring
Implementation Tests (Low Value) verify how the system does it:
- Assert on internal function calls (mocking internals)
- Only test the happy path with typical inputs
- Break during legitimate refactoring even when behavior is correct
- Give false assurance: they pass even when the system is wrong
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.
References: §25210508657
🧪 Test quality analysis by Test Quality Sentinel · ● 577K · ◷
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Adds new gh aw fix codemods to automatically remove deprecated sandbox configuration keys that are now rejected in strict mode.
Changes:
- Register 3 new codemods in
GetAllCodemods()for removingsandbox.mcp.container,sandbox.mcp.version, andsandbox.agent: false. - Add implementations for sandbox MCP container/version removal and sandbox agent false removal.
- Extend codemod registry tests and add new unit tests for the new codemods.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/fix_codemods_test.go | Updates codemod registry count and expected ordering to include the new codemods. |
| pkg/cli/fix_codemods.go | Registers the new sandbox-related codemods in the global codemod list. |
| pkg/cli/codemod_sandbox_mcp_internal.go | Implements codemods intended to remove deprecated sandbox.mcp.container and sandbox.mcp.version. |
| pkg/cli/codemod_sandbox_mcp_internal_test.go | Adds unit tests covering the sandbox MCP codemods’ expected behavior. |
| pkg/cli/codemod_sandbox_agent_false_removal.go | Implements codemod to remove deprecated sandbox.agent: false (boolean false only). |
| pkg/cli/codemod_sandbox_agent_false_removal_test.go | Adds unit tests for the sandbox agent false removal codemod. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
pkg/cli/codemod_sandbox_mcp_internal.go:46
- Same issue as the container codemod:
removeFieldFromBlock(lines, "version", "mcp")is not scoped tosandbox.mcpand can removeversion:from unrelatedmcp:blocks (e.g.tools.<tool>.mcp). Please scope the removal to thesandbox:->mcp:block only.
Apply: func(content string, frontmatter map[string]any) (string, bool, error) {
if !hasSandboxMCPField(frontmatter, "version") {
return content, false, nil
}
newContent, applied, err := applyFrontmatterLineTransform(content, func(lines []string) ([]string, bool) {
return removeFieldFromBlock(lines, "version", "mcp")
})
- Files reviewed: 6/6 changed files
- Comments generated: 2
| Apply: func(content string, frontmatter map[string]any) (string, bool, error) { | ||
| if !hasSandboxMCPField(frontmatter, "container") { | ||
| return content, false, nil | ||
| } | ||
| newContent, applied, err := applyFrontmatterLineTransform(content, func(lines []string) ([]string, bool) { | ||
| return removeFieldFromBlock(lines, "container", "mcp") | ||
| }) |
There was a problem hiding this comment.
removeFieldFromBlock(lines, "container", "mcp") matches any mcp: block in the frontmatter, not specifically sandbox.mcp. Workflows can contain other mcp: sections (e.g. under tools.<id>.mcp, which can include a container key per mcp_add.go), so this codemod can inadvertently delete tool configuration.
Scope the line transform to sandbox: -> mcp: (two-level nesting) before removing container (similar to removeFieldFromPlaywright), or implement a generic helper to remove a field from a nested path like ["sandbox","mcp"].
This issue also appears on line 40 of the same file.
| func TestSandboxMCPContainerRemoval_RemovesContainer(t *testing.T) { | ||
| codemod := getSandboxMCPContainerRemovalCodemod() | ||
|
|
||
| content := `--- | ||
| on: workflow_dispatch | ||
| sandbox: | ||
| mcp: | ||
| container: ghcr.io/example/gateway | ||
| port: 8080 | ||
| permissions: |
There was a problem hiding this comment.
The tests for the sandbox.mcp container/version removals don't cover workflows that also have tools.<tool>.mcp blocks (which can include container per pkg/cli/mcp_add.go). Given the current implementation operates on raw lines, add a regression test ensuring the codemod only removes sandbox.mcp.container/sandbox.mcp.version and does not touch tools.*.mcp.container or other tool MCP settings.
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 (674 new lines in AI has analyzed the PR diff and generated a draft ADR to help you get started: 📄 Draft ADR: 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: §25210508659
|
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/ad9e4978-2e56-413f-8e3c-134c85ca39e5 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Three deprecated sandbox keys—
sandbox.mcp.container,sandbox.mcp.version, andsandbox.agent: false—are now rejected in strict mode butgh aw fixcouldn't remove them automatically, requiring manual edits across 28+ files in official repos.New codemods
sandbox-mcp-container-removal— removessandbox.mcp.container; the MCP gateway container is now managed internallysandbox-mcp-version-removal— removessandbox.mcp.version; same rationalesandbox-agent-false-removal— removessandbox.agent: falseonly when the value is booleanfalse; disabling the agent sandbox firewall is no longer permitted in strict modeAll three codemods are registered in
GetAllCodemods()and run as part ofgh aw fix --write. They are skipped when the workflow hasstrict: falsein its frontmatter, since that explicitly opts out of strict mode and the deprecated keys remain valid in that context.Example
Before:
After
gh aw fix --write:Other
mcp:sub-keys (e.g.port,api-key) and non-falseagentvalues are left untouched. Workflows withstrict: falseare left entirely unchanged.