Stage Pi CLI in the docker-sbx-visible path so Pi agents run under microVM sandboxes - #53110
Conversation
…icroVM sandboxes Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. This PR mirrors the existing GenerateDockerSbxNpmCLIInstallStep/GetDockerSbxNpmCLIPathSetup pattern already used identically by Claude and Codex engines, applying it to Pi. No new abstractions, no reinvented logic, minimal targeted test addition. Nothing to cut.
|
|
✅ Test Quality Sentinel completed test quality analysis.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #53110 does not have the 'implementation' label and has only 30 new lines of code in business logic directories (threshold: 100).
|
|
✅ PR Code Quality Reviewer completed the code quality review. Completed PR review with no GitHub write because no actionable changed-line issues were found; sub-agent unavailable (grumpy-coder binary missing).
|
There was a problem hiding this comment.
Pull request overview
Stages the Pi CLI in a microVM-visible path so Pi workflows run correctly under docker-sbx and cloud-hypervisor.
Changes:
- Adds microVM-specific Pi installation and
PATHsetup. - Tests docker-sbx installation and execution wiring.
- Regenerates affected workflow lock files.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/pi_engine.go |
Adds Pi CLI staging and path export. |
pkg/workflow/docker_sbx_test.go |
Verifies Pi microVM CLI wiring. |
.github/workflows/daily-doc-updater.lock.yml |
Regenerates the affected Pi workflow. |
.github/workflows/daily-awf-spec-compiler-surfacing.lock.yml |
Regenerates the affected Pi workflow. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Balanced
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.
The changes correctly stage the Pi CLI binary under ${RUNNER_TEMP}/gh-aw/engine-cli/bin — consistent with the existing mcp-cli staging pattern — and expose it inside microVM sandboxes by prepending the path to PATH in the firewall-enabled execution branch. Lock files are compiler-generated and match the Go source. Test coverage is adequate.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 15.2 AIC · ⌖ 7.04 AIC · ⊞ 5.6K
There was a problem hiding this comment.
✅ Test Quality Sentinel: 70/100 Acceptable. Design test ratio 100% (1/1), implementation tests 0% (threshold 30%), no violations. Single new subcase validates Pi CLI docker-sbx path staging fix (#53097) with 4 focused assertions. Happy-path coverage sufficient for critical user-visible behavior.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs — requesting one change before merge.
📋 Key Themes & Highlights
Key Themes
- Incomplete fix scope: The
engine-cliPATH export is only wired into thefirewallEnabled == truebranch ofbuildPiExecutionCommand. Whendocker-sbxis combined withsandbox.agent: false(firewall disabled), the non-firewall code path at line 405 does not export the engine-cli PATH, so the bug reproduces.
Positive Highlights
- ✅ Root cause well-diagnosed: the original issue (tool cache invisibility inside microVM) is correctly identified.
- ✅ Install step pattern matches the established Claude/Codex approach exactly — consistent codebase.
- ✅ Test subtest is well-structured and uses the same
workflowDatafixture (firewall enabled + docker-sbx) as peers. - ✅ PR description is thorough and correctly flags the
pipefailgap as a separate concern.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 48.9 AIC · ⌖ 8.59 AIC · ⊞ 7.7K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/workflow/pi_engine.go:404
[/diagnosing-bugs] The engine-cli PATH export is only injected inside the if firewallEnabled branch (line 383), so this fix does not apply when docker-sbx is used with sandbox.agent: false (firewall disabled). The same pi: command not found failure would recur in that configuration.
<details>
<summary>💡 Suggested fix</summary>
Move the dockerSbxCLIPath injection outside the firewall guard so it applies unconditionally when the runtime requires it:
// Non-firewall p…
</details>|
@copilot PR #53110 still needs follow-up before merge. Quick triage
Next step
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Fixed the PATH export gap: The two failing checks ( |
|
🎉 This pull request is included in a new release. Release: |
The
Daily Documentation Updaterworkflow (engine.id: pi+sandbox.agent.runtime: docker-sbx) succeeded with zero safe outputs. The agent never ran:The Pi engine only installed the CLI via
npm install -g, which lands in the hosted tool cache — not visible inside the microVM. The trailing| teein the agent command masked the127exit status, so the job reported success. Claude and Codex already stage a second copy under${RUNNER_TEMP}/gh-aw/engine-cliand export it ontoPATH; the Pi engine was missing both halves.Changes
pkg/workflow/pi_engine.goGetInstallationSteps: appendsGenerateDockerSbxNpmCLIInstallStepfor@earendil-works/pi-coding-agentwhen the runtime isdocker-sbxorcloud-hypervisor.buildPiExecutionCommand: prependsGetDockerSbxNpmCLIPathSetup(workflowData), keeping the same export ordering as Claude/Codex (mcp-cli && engine-cli && hostedtoolcache && pi …).pkg/workflow/docker_sbx_test.go— newpi install and execution use sbx-visible CLI pathsubtest inTestDockerSbxEngineCLIWiring, asserting both the staged install/symlink and thePATHexport.make recompileregenerateddaily-doc-updater.lock.ymlanddaily-awf-spec-compiler-surfacing.lock.yml, the only Pi + docker-sbx workflows in the repo.Generated diff:
Not addressed
The
cmd | tee logpipeline inside the sandbox still has nopipefail, so a failing agent CLI is reported as a successful run rather than a hard failure. This affects every engine, not just Pi, and is left for a separate change.Run: https://github.com/github/gh-aw/actions/runs/31950301208> Generated by 👨🍳 PR Sous Chef · gpt54 · 12.5 AIC · ⌖ 5.77 AIC · ⊞ 8.7K · ◷