Fix Pi engine report_incomplete emission failing silently in sandboxed agent container (EROFS) - #57861
Conversation
…tead of direct fs writes Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
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
|
|
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.
|
|
✅ 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.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review. No inline review comments were needed because the changed lines did not expose a blocking issue beyond what the overall review already covered.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs — this is a clean, well-targeted fix.
📋 Analysis
The root cause (raw fs.appendFileSync writing to a read-only sandbox mount, failing with EROFS and only logging) is correctly addressed by delegating to the existing emitInfrastructureIncomplete() helper in safeoutputs_cli.cjs, which routes through the safeoutputs CLI → MCP gateway channel — the same pattern already used by codex_harness.cjs, copilot_harness.cjs, and harness_retry_guard.cjs. This brings pi_provider.cjs in line with the established convention rather than reimplementing raw fs writes.
Regression coverage is solid: pi_provider.test.cjs now exercises both the CLI-success and CLI-failure emission paths via the GH_AW_SAFEOUTPUTS_CLI stub-binary override, asserting on the new log messages instead of direct file contents — appropriately reflecting that emission no longer touches the file directly. The pre-existing "skip if safe outputs already recorded" check is preserved and still correctly relies on reads (which succeed on the read-only mount).
No dead code, no unused imports (path removal is correct since fs.mkdirSync(path.dirname(...)) was removed along with the direct write), and the buildInfrastructureIncompletePayload removal is clean since its only caller was replaced.
Positive Highlights
- ✅ Delegates to already-tested shared helper instead of duplicating logic
- ✅ Regression tests cover both success and failure of the new CLI channel
- ✅ Clear doc comment explaining the sandboxed read-only mount constraint
No actionable issues found.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 16.9 AIC · ⌖ 14.6 AIC · ⊞ 10.3K
Comment /matt to run again
There was a problem hiding this comment.
🟢 Approval recommended
The focused change addresses the confirmed EROFS failure while preserving the existing safe-output pre-check.
Pull request overview
Fixes Pi provider diagnostics in read-only sandboxes by routing report_incomplete through the safeoutputs CLI.
Changes:
- Replaces direct filesystem writes with
emitInfrastructureIncomplete(). - Tests successful, failed, and skipped CLI emission paths.
File summaries
| File | Description |
|---|---|
actions/setup/js/pi_provider.cjs |
Routes infrastructure diagnostics through the MCP gateway. |
actions/setup/js/pi_provider.test.cjs |
Updates coverage for CLI-based emission behavior. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Reviewed with harden/audit lens (bug-fix category). The fix correctly delegates emission to the shared emitInfrastructureIncomplete() helper via the safeoutputs CLI channel, matching the pattern already used by other harnesses (codex_harness.cjs, copilot_harness.cjs, harness_retry_guard.cjs). Both success and failure paths are logged distinctly and covered by new tests (CLI-success and CLI-failure via GH_AW_SAFEOUTPUTS_CLI stub). No blocking issues found; no UI/design-system code involved so Impeccable modes were not directly applicable beyond the harden/audit correctness lens.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
registry.npmjs.org
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "registry.npmjs.org"See Network Configuration for more information.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 19.8 AIC · ⌖ 14.3 AIC · ⊞ 8.3K
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.
No actionable blocking issues found in the changed lines.
### Review notes
The change removes direct filesystem writes to GH_AW_SAFE_OUTPUTS and routes the synthetic report_incomplete signal through the same safeoutputs CLI path the other harnesses already use. The remaining risk I checked for was whether the new path could double-emit or skip existing outputs; the pre-check still short-circuits when the safe-outputs file already contains task output, and the new tests cover both CLI success and CLI failure paths. I did not find a changed-line regression that should block merge.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 45.2 AIC · ⌖ 7.32 AIC · ⊞ 23.5K
Comment /review to run again
|
🎉 This pull request is included in a new release. Release: |
LintMonster (and any
pi-engine workflow) could complete a run with an internal provider error but produce zero safe outputs, instead of the expectedreport_incompletediagnostic.Root cause
pi_provider.cjs'smessage_endhandler correctly detects a Pi provider infrastructure error and tries to emit a syntheticreport_incompletesafe output.GH_AW_SAFE_OUTPUTSviafs.appendFileSync. This extension runs inside the AWF gvisor-sandboxed agent container, where the directory backingGH_AW_SAFE_OUTPUTSis mounted read-only.EROFS, and the failure was logged but never surfaced as a safe output — so the run appeared to produce nothing at all.Fix
pi_provider.cjsnow delegates emission to the existingemitInfrastructureIncomplete()helper insafeoutputs_cli.cjs, which sends the payload through thesafeoutputsCLI → MCP gateway channel (a process outside the sandbox with real write access) instead of writing the file directly.codex_harness.cjs,copilot_harness.cjs, andharness_retry_guard.cjs—pi_provider.cjswas the one harness still reimplementing raw fs writes.buildInfrastructureIncompletePayload()helper along with the directfs.appendFileSync/fs.mkdirSynccalls; the existing "skip if safe outputs already recorded" pre-check (which only reads the file) is preserved since reads still succeed on the read-only mount.Tests
pi_provider.test.cjsupdated to cover both the CLI-success and CLI-failure emission paths, using theGH_AW_SAFEOUTPUTS_CLIstub-binary override already used in other harness tests, in place of asserting on direct file contents.