fix: legacy-security mode installs awf non-rootless, uses plain sudo for log parsing#48098
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…do for log parsing When `legacy-security: enable` is set, the AWF execution step correctly uses `sudo -E awf` (which requires the binary to be in sudo's secure_path, i.e. /usr/local/bin). However, two other steps incorrectly added `--rootless`: 1. The `install_awf_binary.sh` step passed `--rootless` (installing to ~/.local/bin), making `sudo awf` fail with "command not found". 2. The `print_firewall_logs.sh` step passed `--rootless` (using non-interactive sudo with a non-sudo fallback), even though AWF had full sudo access. Fix: In both `generateAWFInstallationStep` and `generateFirewallLogParsingStep`, skip `--rootless` when `LegacySecurity` is true, even if `NetworkIsolation` is also true (which is the default). This also fixes the compiled `.github/workflows/smoke-service-ports.lock.yml` which already used `legacy-security: enable`. Closes #47804 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ Test Quality Sentinel completed test quality analysis. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix is correct and well-scoped. Both generateAWFInstallationStep and generateFirewallLogParsingStep now correctly skip --rootless when LegacySecurity is true, matching the documented behavior that sudo -E awf requires the binary in sudos secure_path. The reuse of getAgentConfig in engine_firewall_support.go is consistent with the existing pattern. Tests cover the unit cases and an end-to-end compilation check.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 17.4 AIC · ⌖ 5.14 AIC · ⊞ 5K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs — one minor observation on the new code; no blocking issues.
📋 Key Themes & Highlights
Findings
- Dead nil guard (
engine_firewall_support.go:139): theagentCfg == nilbranch is unreachable — see inline comment.
Positive Highlights
- ✅ Root cause correctly identified and fixed in both affected callsites
- ✅ Regression tests added for the
LegacySecurity=true+NetworkIsolation=truecombination in three different test files - ✅ End-to-end compilation test (
TestLegacySecurityInstallNonRootless) verifies the lock file output end-to-end - ✅
smoke-service-ports.lock.ymlregenerated — confirming the previously silently broken workflow is now correct - ✅ Comment in
copilot_engine_installation.goclearly documents thesudo secure_pathconstraint
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 26.1 AIC · ⌖ 4.67 AIC · ⊞ 6.7K
Comment /matt to run again
Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (>100 new lines in Draft ADR committed:
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 MatterADRs 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
|
🧪 Test Quality Sentinel Report
📊 Metrics (6 tests)
|
There was a problem hiding this comment.
Review: legacy-security non-rootless install fix
The core fix is correct — LegacySecurity mode must install AWF to /usr/local/bin (non-rootless) so sudo -E awf can find it, and the firewall log parsing step must match. Two issues flagged; neither is a blocker but one warrants attention.
Findings
Medium — Dead nil guard in engine_firewall_support.go:139
The agentCfg == nil branch in (agentCfg == nil || !agentCfg.LegacySecurity) is unreachable. isAWFNetworkIsolationEnabled already returns false when agentConfig is nil, so the outer && short-circuits before the inner || is evaluated. This is not a correctness bug today, but it misleads readers and could silently regress if isAWFNetworkIsolationEnabled is refactored. See inline comment.
Low — TestLegacySecurityInstallNonRootless relies on implicit network-isolation defaults
The test frontmatter does not declare network-isolation: true explicitly, meaning the test only exercises the LegacySecurity exception if network isolation happens to be the default. See inline comment.
Out-of-diff observation (no comment posted)
Other callers of isAWFNetworkIsolationEnabled — notably difc-proxy host selection in awf_helpers.go:738 and compiler_difc_proxy.go:266/585 — do not have a LegacySecurity exception. For legacy-security+network-isolation, AWF runs on the host network (non-rootless), so the difc-proxy should route via host.docker.internal rather than awmg-cli-proxy. This PR does not touch those files; worth a follow-up issue.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 · 63.7 AIC · ⌖ 4.87 AIC · ⊞ 5.7K
Comment /review to run again
🤖 PR Triage
Summary: Next action: Fast-track — ready for human approval and merge.
|
|
@lpcox review please |
|
@copilot please run the Recent context:
|
🤖 PR Triage
Score breakdown: Impact 30/50 · Urgency 16/30 · Quality 14/20 Rationale:
|
|
@lpcox can we merge "sudo" mode with "legacy-security"? |
|
@copilot please run the Unresolved review feedback / follow-up (newest first):
Recent maintainer context: Run: https://github.com/github/gh-aw/actions/runs/30259748826
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the three unresolved
Validation:
Current maintainer-visible blockers: PR API currently reports |
|
@copilot please run the Recent maintainer context:
Run: https://github.com/github/gh-aw/actions/runs/30264173356
|
Current status after triage:
Maintainer-visible blockers right now:
|
🤖 PR Triage Report
Score Breakdown:
CI Status: ✅ Passing (copilot check: success) Reviews: APPROVED (x2 by github-actions[bot]) Notes: Well-documented bug fix with ADR, test coverage, and clear root cause. CI passes. Approved by automation. Ready for human fast-track review.
|
🤖 PR Triage
Carry-over (1 day old). Fixes
|
🤖 PR Triage
legacy-security mode broken: installs awf non-rootless causing EACCES failures. Fix is targeted (165 adds, 5 dels, 7 files). AI reviewer approved. Carry-over from prior run.
|
|
@copilot resolve the merge conflicts on this branch. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Resolved on commit |
legacy-security: enableopts the agent job intosudo -E awf, but the install step unconditionally passed--rootlesswhenNetworkIsolation=true(the default), placing the binary in~/.local/bin— outside sudo'ssecure_path. Result: agent startup fails withsudo: awf: command not found. Theprint_firewall_logs.shstep had the same logic flaw.Changes
pkg/workflow/copilot_engine_installation.go—generateAWFInstallationStep: skip--rootlesswhenagentConfig.LegacySecurityis true, so the binary lands in/usr/local/binpkg/workflow/engine_firewall_support.go—generateFirewallLogParsingStep: skip--rootlesswhenLegacySecurityis true; AWF already has full sudo access in this mode.github/workflows/smoke-service-ports.lock.yml— regenerated (this workflow was already usinglegacy-security: enableand was silently broken)LegacySecurity=true+NetworkIsolation=truecombination, plus an end-to-end compilation testThe condition in both places went from:
to:
sudo -E awfbut installs awf with --rootless, so the agent fails at startup withsudo: awf: command not found#47804Run: https://github.com/github/gh-aw/actions/runs/30240308794
pr-sous-chef run: https://github.com/github/gh-aw/actions/runs/30259748826
pr-sous-chef run: https://github.com/github/gh-aw/actions/runs/30293408015
Run: https://github.com/github/gh-aw/actions/runs/30304405147