Fix gvisor+privileged sandbox codemod gap and add missing tools.bash codemod for min-integrity: none - #53964
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot the codemod gvisor should retain gvisor and drop the sudo field |
…ng runtime Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
|
✅ Ponytail Reviewer completed successfully!
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes
This still misses at least one valid frontmatter shape, so gh aw fix --write can claim success while leaving the strict-mode compile error unfixed.
Blocking theme
- The new
min-integrity-none-requires-bashcodemod only rewrites block-styletools:YAML and silently skips inline mappings such astools: { github: { min-integrity: none } }, even though the parsed frontmatter says the fix is required.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 12.9 AIC · ⌖ 6.91 AIC · ⊞ 7K
Comment /review to run again
There was a problem hiding this comment.
The two codemod fixes are correct and well-tested. The gvisor+privileged path correctly keeps runtime: gvisor unchanged (since targetRuntime == oldRuntime, needsRuntimeUpdate stays false) and only strips the unsupported sudo/legacy-security fields. The min-integrity-none codemod safely inserts bash: false as the first child of the tools: block. Both codemods have comprehensive no-op guard tests. No issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 20.2 AIC · ⌖ 8.8 AIC · ⊞ 5.8K
There was a problem hiding this comment.
Ponytail review (over-engineering only).
One finding: duplicated insert-point scan logic across three codemod files, extractable into a shared helper.
net: -30 lines possible.
Generated by ✂️ Ponytail Reviewer for #53964 · auto · 25 AIC · ⌖ 3.62 AIC · ⊞ 7.3K
Comment /ponytail to run again
Comments that could not be inline-anchored
pkg/cli/codemod_min_integrity_none_bash.go:107
L77-107: yagni: insertBashFalseIntoTopLevelTools reimplements the "find first-child insertion point under a top-level block" scan already written twice (insertAt logic in codemod_cli_proxy_bash.go and codemod_cli_proxy_mode.go). Extract one shared helper and reuse it in all three codemods instead of a third copy.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes for a PR-body/code contradiction and two test coverage gaps.
📋 Key Themes & Highlights
Key Themes
- PR description contradicts implementation: The PR body shows
gvisor → docker-sudo-iptablesmigration, but the code (and changeset) correctly keepsruntime: gvisorand drops the privileged fields. The description needs a fix before merge to avoid future confusion. - Test coverage gaps: The combined
sudo + legacy-security + gvisorcase is untested; theinsertBashFalseIntoTopLevelToolscomment-skipping logic has no targeted test.
Positive Highlights
- ✅ Clean, focused fixes — each codemod solves exactly one gap with minimal surface area.
- ✅
migrateSandboxAgentSecurityLinessignature improvement (oldRuntime, targetRuntime) makes the call-site intent explicit. - ✅ New codemod tests are well-structured, parallel-safe, and cover all the important no-op paths.
- ✅ Changeset description is accurate and matches the implementation.
- ✅ Registry ordering is maintained correctly in
GetAllCodemods.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 31.6 AIC · ⌖ 10.2 AIC · ⊞ 7.9K
Comment /matt to run again
There was a problem hiding this comment.
Pull request overview
Attempts to close two strict-mode codemod gaps involving privileged gVisor configurations and explicit Bash settings.
Changes:
- Adjusts sandbox runtime migration and runtime-line rewriting.
- Adds and registers the
min-integrity-none-requires-bashcodemod. - Adds unit tests and release notes.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/fix_codemods.go |
Registers the new codemod. |
pkg/cli/fix_codemods_test.go |
Updates registry-order expectations. |
pkg/cli/codemod_sandbox_runtime_profile.go |
Changes gVisor privilege migration behavior. |
pkg/cli/codemod_sandbox_runtime_profile_test.go |
Tests revised gVisor handling. |
pkg/cli/codemod_min_integrity_none_bash.go |
Implements explicit bash: false insertion. |
pkg/cli/codemod_min_integrity_none_bash_test.go |
Tests the new codemod. |
.changeset/fix-sandbox-gvisor-privileged-and-min-integrity-bash-codemods.md |
Documents both fixes. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 7/7 changed files
- Comments generated: 3
- Review effort level: Balanced
| getMentionsAllowTeamMembersCodemod(), // Rename allow-team-members to allowed-collaborators in safe-outputs.mentions | ||
| getEngineCopilotSDKDriverToDriverCodemod(), // Rename deprecated engine.copilot-sdk-driver to engine.driver | ||
| getEngineModelToTopLevelCodemod(), // Move engine.model to top-level model | ||
| getMinIntegrityNoneRequiresBashCodemod(), // Add tools.bash: false when tools.github.min-integrity is 'none' |
There was a problem hiding this comment.
Done — getMinIntegrityNoneRequiresBashCodemod() is now registered at fix_codemods.go:113, immediately before getCLIProxyBashDisabledCodemod() at line 114, so a single fix pass emits both bash: false and cli-proxy: false. Covered by a registry-level test in codemod_min_integrity_none_bash_test.go that runs the full registry through processWorkflowFileWithInfo and asserts both settings appear after one pass, for block and inline tools: syntax.
| } | ||
| trimmed := strings.TrimSpace(line) | ||
| if needsRuntimeUpdate && getIndentation(line) == indent && strings.HasPrefix(trimmed, "runtime:") { | ||
| result = append(result, indent+"runtime: "+targetRuntime) |
There was a problem hiding this comment.
Done — the runtime line rewrite now appends trailingCommentSuffix(...) (codemod_sandbox_runtime_profile.go:153), which uses the shared findTrailingCommentIndex helper to preserve any user-authored # ... suffix while replacing only the scalar value. Covered by the test case "rewritten runtime line keeps its trailing comment".
…ts in ambiguous codemod cases Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
PR Triage
Fixes gh aw fix --write codemod gaps for gvisor+privileged sandbox and missing tools.bash min-integrity:none codemod. Moderate size, addresses repeatable --strict failures.
|
|
@copilot PR still has unresolved review threads. Please address the newest blocking feedback, refresh the branch if needed, and then run the Open review threads (newest first):
Also attempt a branch refresh if GitHub offers one.
|
…ping bash insertion Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Remaining items addressed:
The other listed threads were already resolved in earlier commits on this branch:
On the "migrate gvisor + privileged options to Branch is refreshed from |
|
@copilot PR still has maintainer-visible unfinished work. Open blockers to address in one pass:
|
Handoff summary:
Remaining risk / maintainer action:
|
PR TriageCategory: bug · Risk: medium · Score: 68/100 (impact 30 + urgency 20 + quality 18) Sandbox codemod correctness fix, approved review, CI green; large diff but well-scoped. Automated triage — see run report for full details.
|
|
@copilot PR still has maintainer-visible unfinished work. Open review threads / blockers to close in one pass:
|
Handoff summary (no code changes needed this pass — the branch already contained every requested fix):
Remaining risks / maintainer actions:
|
|
@copilot PR still has maintainer-visible unfinished work. Please close out the remaining blockers in one pass:
Open review threads (newest first):
If those are already fixed on the current head, reply on-thread with the exact commit/test evidence so reviewers can resolve them quickly.
|
Two recurring gaps in
gh aw fix --writeleft--strictcompile failures unrepaired across independently-verified repos: thesandbox-runtime-profilescodemod hard-errored onruntime: gvisorcombined with privileged options instead of migrating, and no codemod existed to satisfy the newtools.bashrequirement whenmin-integrity: none.sandbox-runtime-profilescodemodsandbox.agent.runtime: gvisorcombined withsudo: trueorlegacy-security: enablenow auto-migrates toruntime: docker-sudo-iptables, preserving the original privileged/sudo intent, instead of aborting the fix pass with an error.runtime:line in place when its value changes (previously only inserted aruntime:key when one was absent).New
min-integrity-none-requires-bashcodemodtools.bash: falsewhentools.github.min-integrityisnoneandtools.bashis not already specified, satisfying the strict-mode requirement that shell access be explicit.bash: false.Both codemods are registered in the codemod registry with accompanying unit tests, and a changeset documents the fix.
run: https://github.com/github/gh-aw/actions/runs/32276767055> Generated by 👨🍳 PR Sous Chef · gpt54 · 26.4 AIC · ⌖ 9.42 AIC · ⊞ 9.3K · ◷