fix: add pi.dev to Pi engine firewall allowlist - #47243
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot pi source code and determine if these domains can be avoided with cli args or env vars |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
The Pi CLI (
All three can be suppressed with a single env var documented in
( Changes in this commit (
Note: |
|
@copilot remove api.pi.ai as well |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done — removed |
There was a problem hiding this comment.
Pull request overview
Reworks Pi startup networking to avoid blocked pi.dev requests by enabling offline mode and removing the obsolete api.pi.ai allowlist entry.
Changes:
- Sets
PI_OFFLINE=1for Pi execution. - Removes
api.pi.aifrom Pi default domains. - Regenerates documentation and workflow lock files.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/pi_engine.go |
Enables Pi offline mode. |
pkg/workflow/domains.go |
Removes the legacy Pi domain. |
pkg/workflow/README.md |
Updates generated domain documentation. |
.github/workflows/schema-consistency-checker.lock.yml |
Regenerates Pi runtime configuration. |
.github/workflows/poem-bot.lock.yml |
Regenerates Pi runtime configuration. |
.github/workflows/lint-monster.lock.yml |
Regenerates Pi runtime configuration. |
.github/workflows/issue-monster.lock.yml |
Regenerates Pi runtime configuration. |
.github/workflows/hippo-embed.lock.yml |
Regenerates Pi runtime configuration. |
.github/workflows/eslint-monster.lock.yml |
Regenerates Pi runtime configuration. |
.github/workflows/daily-multi-device-docs-tester.lock.yml |
Regenerates Pi runtime configuration. |
.github/workflows/daily-function-namer.lock.yml |
Regenerates Pi runtime configuration. |
.github/workflows/daily-awf-spec-compiler-surfacing.lock.yml |
Regenerates Pi runtime configuration. |
.github/workflows/commit-changes-analyzer.lock.yml |
Regenerates Pi runtime configuration. |
.github/workflows/chaos-pr-bundle-fuzzer.lock.yml |
Regenerates Pi runtime configuration. |
.github/workflows/agent-persona-explorer.lock.yml |
Regenerates Pi runtime configuration. |
.github/workflows/ab-testing-advisor.lock.yml |
Regenerates Pi runtime configuration. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
pkg/workflow/pi_engine.go:495
PI_OFFLINEis broader than the pi.dev startup fix: Pi's package manager refuses to install missing npm/git package sources while it is set. On a fresh runner, an approved project.pi/settings.jsonthat declares packages will therefore run without those extensions or skills. Use Pi's targeted controls for the two unwanted pi.dev operations instead, preserving package loading and catalog behavior.
env["PI_OFFLINE"] = "1"
- Files reviewed: 24/24 changed files
- Comments generated: 2
- Review effort level: Medium
| // in CI: the AWF api-proxy provides models.json, version checks add latency, | ||
| // and telemetry is not relevant in an automated runner context. | ||
| // See: https://github.com/earendil-works/pi/blob/main/packages/coding-agent/docs/settings.md | ||
| env["PI_OFFLINE"] = "1" |
| // Disable Pi startup network operations (version check, install telemetry, | ||
| // and remote model-catalog refresh against pi.dev). None of these are needed | ||
| // in CI: the AWF api-proxy provides models.json, version checks add latency, | ||
| // and telemetry is not relevant in an automated runner context. | ||
| // See: https://github.com/earendil-works/pi/blob/main/packages/coding-agent/docs/settings.md |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #47243 does not have the 'implementation' label and has only 9 new lines of code in business logic directories (threshold: 100). |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Review Summary
The implementation correctly fixes the TCP_DENIED 403 issue by setting PI_OFFLINE=1 to suppress Pi startup network calls (version checks, telemetry, model-catalog refresh against pi.dev), and removing the now-redundant api.pi.ai from both PiBaseDefaultDomains and PiDefaultDomains.
Two issues need attention (also flagged inline):
-
PR title/description mismatch — The title says "add pi.dev to allowlist" but the code takes the opposite approach: suppressing the calls via
PI_OFFLINE=1. Please update the PR title/description to reflect the actual fix. -
Golden test not updated —
pkg/workflow/testdata/TestWasmGolden_AllEngines/pi.goldenlikely needs to be regenerated to include thePI_OFFLINE=1env assignment, otherwiseTestWasmGolden_AllEngines/piwill fail.
Both are blocking. Please address before merging.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 21.4 AIC · ⌖ 4.53 AIC · ⊞ 5K
There was a problem hiding this comment.
Requesting changes on two blocking issues before this can merge.
### Blocking issues
1. PI_OFFLINE=1 is set unconditionally (new comment on pi_engine.go:1078): The env var is written before the firewallEnabled check, activating offline mode in all Pi runs — including non-firewall paths where pi.dev/api.pi.ai are legitimately needed. Either scope it to firewallEnabled or explicitly document why offline mode is safe in every execution context.
2. api.pi.ai removed without confirming runtime scope (new comment on domains.go:1054): The removal is justified by PI_OFFLINE=1 suppressing startup calls, but the prior comment on the domain only said "telemetry / update checks." If api.pi.ai is also hit during runtime model inference (e.g. the no-prefix PiDefaultDomains path), this deletion will cause silent 403s in production runs. Verify and document scope before removing.
3. Golden test fixture likely broken (noted in prior comment on pi_engine.go:495): PI_OFFLINE=1 must appear in the compiled output checked by TestWasmGolden_AllEngines/pi. Regenerate the fixture and include it in this PR.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 63.3 AIC · ⌖ 4.81 AIC · ⊞ 5.6K
Comment /review to run again
Comments that could not be inline-anchored
pkg/workflow/pi_engine.go:1078
PI_OFFLINE set unconditionally — non-firewall paths may break silently. This forces offline mode for every Pi run, not just AWF-firewall-enabled runs.
<details>
<summary>💡 Details</summary>
The comment says PI_OFFLINE=1 disables only version checks, telemetry, and remote model-catalog refresh. But the env var is set before the firewallEnabled branch, meaning it is active in non-firewall contexts (local dev, CI without AWF) where pi.dev network access is legitimate.
If `PI_OFFLIN…
pkg/workflow/domains.go:1054
api.pi.ai removed without documenting whether runtime LLM calls still need it. The removal is intentional per the PR, but the domain was commented as "Pi CLI telemetry / update checks" — if it is also used for runtime model routing, removing it will cause request failures for Copilot-routed Pi runs.
<details>
<summary>💡 Details</summary>
domains.go previously had:
"api.pi.ai", // Pi CLI telemetry / update checksThe commit removes this line, and the lock files confirm `ap…
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs — the fix is sound but the PR metadata is misleading and there is a potential gap for users who relied on api.pi.ai being allowlisted.
📋 Key Themes & Highlights
Key Themes
- PR title/description mismatch: The title says "add pi.dev to allowlist" but the actual change removes
api.pi.aiand setsPI_OFFLINE=1. No domain is added to any allowlist. Existing review comments already flag this. - Correctness of fix: Using
PI_OFFLINE=1to suppress Pi startup network calls is a clean root-cause fix — it eliminates the outboundpi.devCONNECT before it happens. Well-commented with an upstream reference link. - Removing
api.pi.ai: The removal is not explained. If any Pi CLI runtime operation (not just startup) routes throughapi.pi.ai, removing it silently breaks those calls in non-offline mode.
Positive Highlights
- ✅
PI_OFFLINE=1approach eliminates the problem at the source rather than expanding the allowlist - ✅ Clear comment + upstream settings doc link in
pi_engine.go - ✅ Lock files and README.md API table kept in sync
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 47.4 AIC · ⌖ 4.57 AIC · ⊞ 6.7K
Comment /matt to run again
|
🎉 This pull request is included in a new release. Release: |
The Pi CLI contacts
pi.devduring initialization (auth/web portal) but onlyapi.pi.aiwas in the allowlist, causingTCP_DENIED 403~8s into every Pi-engine run.Changes
pkg/workflow/domains.go— addpi.devto bothPiBaseDefaultDomains(used bygetPiDefaultDomainsdynamic path) andPiDefaultDomains(backward-compat static list)Evidence (run artifact
sandbox/firewall/logs/audit.jsonl){ "timestamp": "2026-07-22T07:08:49.059Z", "host": "pi.dev:443", "method": "CONNECT", "status": 403, "decision": "TCP_DENIED" }