Preserve setup-action PATH across privileged AWF startup - #58625
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The user-facing bug fix is missing the required patch changeset for release documentation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes #58458 by preserving setup-action PATH entries across privileged docker-sudo-iptables AWF startup.
Changes:
- Restores the runner PATH after
sudowhile invoking trusted absolute binaries. - Adds regression coverage for selected and cached Ruby precedence.
- Refreshes related assertions and generated workflow output.
File summaries
| File | Description |
|---|---|
pkg/constants/constants.go |
Defines the PATH-preserving privileged command. |
pkg/workflow/awf_command_builder.go |
Updates command documentation. |
pkg/workflow/awf_command_builder_test.go |
Verifies the command constant. |
pkg/workflow/engine_helpers_test.go |
Models the sudo PATH boundary. |
pkg/workflow/sandbox_network_isolation_rootless_test.go |
Tests privileged and rootless compilation. |
pkg/workflow/sandbox_custom_agent_test.go |
Updates custom-command assertions. |
pkg/workflow/sandbox_agent_false_test.go |
Updates disabled-sandbox detection. |
pkg/workflow/sandbox_agent_disabled_test.go |
Updates disabled-agent assertions. |
pkg/workflow/firewall_workflow_test.go |
Verifies PATH preservation in legacy mode. |
pkg/workflow/firewall_args_test.go |
Generalizes AWF command messaging. |
pkg/workflow/compiler_permissions_test.go |
Updates rootless-mode assertions. |
pkg/workflow/claude_engine_network_test.go |
Updates privileged-command detection. |
.github/workflows/smoke-service-ports.lock.yml |
Regenerates the smoke workflow command. |
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 1
- Review effort level: Balanced
| const AWFLegacySecurityCommand = "sudo -E awf" | ||
| // AWFLegacySecurityCommand runs the trusted system AWF binary with the runner's | ||
| // PATH restored after sudo applies secure_path. | ||
| const AWFLegacySecurityCommand = `sudo -E /usr/bin/env PATH="$PATH" /usr/local/bin/awf` |
There was a problem hiding this comment.
Addressed in 2f2e28d by adding .changeset/patch-preserve-awf-setup-path.md for the PATH preservation fix.
|
Category: bug | Risk: medium | Score: 72 | Breakdown: impact 38/urgency 20/quality 14 | Action: fast_track | Batch: awf-runtime
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ 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. No inline review comments were needed because no actionable changed-line defects survived adjudication.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
|
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.
|
There was a problem hiding this comment.
Verdict
No changed-line issues here rise to merge-blocking severity.
Notes
I checked the PATH-preserving sudo rewrite and the regression coverage around selected-vs-cached tool precedence. The new assertions stay aligned with the behavior change, and the one grumpy-coder finding was too brittle to keep because the test intentionally validates the exact privileged command shape introduced by this patch.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 26.1 AIC · ⌖ 8.61 AIC · ⊞ 21.8K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs — this is a solid, well-targeted root-cause fix with good regression coverage.
📋 Key Themes & Highlights
Key Themes
- Correct root cause: sudo's
secure_pathwas clobbering the runner's PATH before AWF captured it; restoring PATH via/usr/bin/env PATH="$PATH"and callingawfby absolute path both fixes the bug and closes a PATH-substitution risk. - Regression test quality:
TestDockerSudoIptablesPreservesSelectedRubyPathmirrors the existingTestGetNpmBinPathSetup_PreservesSelectedRubypattern well, fakingsudoto simulate the secure_path boundary and asserting the selected Ruby (not the cached or system one) wins. - Minor gap flagged inline: the hardcoded
/usr/local/bin/awfinstall path isn't cross-checked against the installer script constants — low risk today, but worth a comment or test to prevent silent drift. - Existing bot review comment already flags the missing changeset for this user-facing bug fix (
.changeset/has no entry matching this PR) — worth adding before merge per repo conventions.
Positive Highlights
- ✅ Fix addresses the actual mechanism (secure_path replacing PATH) rather than papering over symptoms
- ✅ New test faithfully reproduces the sudo boundary with selected/cached/system Ruby variants
- ✅ Widely mirrored test-string updates across the suite are consistent and mechanical, not risky
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 41.6 AIC · ⌖ 14.7 AIC · ⊞ 10.3K
Comment /matt to run again
| const AWFLegacySecurityCommand = "sudo -E awf" | ||
| // AWFLegacySecurityCommand runs the trusted system AWF binary with the runner's | ||
| // PATH restored after sudo applies secure_path. | ||
| const AWFLegacySecurityCommand = `sudo -E /usr/bin/env PATH="$PATH" /usr/local/bin/awf` |
There was a problem hiding this comment.
[/diagnosing-bugs] Good root-cause fix — forcing an absolute-path env restores PATH after sudo's secure_path clobbers it, and using absolute paths for env/awf avoids PATH-based binary substitution. One gap: the hardcoded /usr/local/bin/awf and /usr/bin/env paths are not validated against actions/setup/sh/install_awf_binary.sh's AWF_INSTALL_DIR/AWF_INSTALL_NAME constants anywhere in code or tests — if that install path is ever changed, this string constant will silently drift out of sync.
💡 Suggestion
Consider adding a small comment cross-referencing install_awf_binary.sh's AWF_INSTALL_DIR/AWF_INSTALL_NAME, or a test asserting the two stay consistent, so a future change to the installer doesn't silently break this hardcoded path.
@copilot please address this.
There was a problem hiding this comment.
Addressed in 2f2e28d by adding a regression test that parses actions/setup/sh/install_awf_binary.sh and asserts AWFLegacySecurityCommand stays aligned with the non-rootless AWF install path.
There was a problem hiding this comment.
Reviewed with the impeccable harden/audit lenses (bug-fix change type). This is a backend/CI infra fix (no UI surface), so those modes mostly reduce to correctness/edge-case checks.
Findings: No blocking issues. The AWFLegacySecurityCommand change (sudo -E /usr/bin/env PATH="$PATH" /usr/local/bin/awf) correctly restores the caller PATH after sudo applies secure_path, and pins the AWF invocation to an absolute path to avoid PATH-based binary substitution. All existing assertions that hardcoded the old sudo -E awf string were updated consistently across awf_command_builder_test.go, claude_engine_network_test.go, compiler_permissions_test.go, firewall_workflow_test.go, sandbox_agent_disabled_test.go, sandbox_agent_false_test.go, sandbox_custom_agent_test.go, and sandbox_network_isolation_rootless_test.go. The new TestDockerSudoIptablesPreservesSelectedRubyPath regression test in engine_helpers_test.go is a solid end-to-end simulation of the sudo boundary (selected vs. secure vs. cached Ruby) and correctly asserts the setup-selected Ruby wins while cache tools stay discoverable.
Non-blocking note: An existing bot review comment already flags that this user-facing bug fix is missing a .changeset/*.md entry — confirmed no changeset file was added in this branch. Worth adding before merge per repo release-notes convention, but not a code-correctness issue.
No additional inline comments needed.
Warning
Firewall blocked 2 domains
The following domains were blocked by the firewall during workflow execution:
github.comproxy.golang.org
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "github.com"
- "proxy.golang.org"See Network Configuration for more information.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 60.8 AIC · ⌖ 12.4 AIC · ⊞ 8.3K
@copilot Please take the next forward-progress pass on this PR.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Investigated the |
docker-sudo-iptablesallowed sudo’ssecure_pathto replace the runner PATH before AWF captured it, causing setup-selected runtimes such as Ruby 3.4.8 to resolve to system versions instead.Changes
sudo -E /usr/bin/env PATH="$PATH" /usr/local/bin/awfRegression coverage
Generated workflow
docker-sudo-iptablesservice-port smoke workflow lock file.