Fix Pi Anthropic routing through the firewall - #58313
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
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.
|
|
✅ PR Code Quality Reviewer completed the code quality review. 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
- "github.com"See Network Configuration for more information.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
There was a problem hiding this comment.
🟡 Changes recommended
Stream failures can still be counted as successful responses, preventing the required nonzero exit.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Routes Pi Anthropic traffic through the native Messages API and adds provider-failure detection.
Changes:
- Uses
anthropic-messageswith/v1/messages. - Tracks provider outcomes and reports incomplete runs.
- Adds routing and failure-path regression tests.
File summaries
| File | Description |
|---|---|
actions/setup/js/pi_provider.test.cjs |
Tests routing and request outcomes. |
actions/setup/js/pi_provider.cjs |
Adds routing diagnostics and provider failure handling. |
actions/setup/js/pi_models_json.test.cjs |
Verifies Anthropic model configuration. |
actions/setup/js/pi_models_json.cjs |
Selects the native Anthropic protocol. |
Review details
Suppressed comments (2)
actions/setup/js/pi_provider.cjs:401
- Setting
process.exitCodehere does not make the actual Pi command fail. This engine invokes the pinned Pi CLI with--print --mode json; Pi v0.84.3's JSON print mode returns 0 for an assistant error, and its main function then callsprocess.exit(exitCode), so that explicit 0 overrides this value. Propagate the all-requests-failed result into Pi's returned status or a checked wrapper marker; otherwise the job still exits successfully.
process.exitCode = 1;
actions/setup/js/pi_provider.cjs:400
- This end-of-run gate does not make the tested failure-then-success path recover successfully in production. A failed assistant turn also fires the existing
message_endhandler, which immediately emitsreport_incompletewhile outputs are empty;handle_agent_failure.cjs:3564-3580later treats that signal as a failure even if a subsequent request returns 2xx. Defer that emission untilagent_endafter evaluating these counters (and exercisemessage_endin the recovery test) so transient failures can actually recover.
if (providerRequestCount > 0 && successfulProviderResponseCount === 0) {
emitInfrastructureIncompleteIfNoSafeOutputs(`All ${providerRequestCount} Pi provider requests failed before safe outputs were emitted.`, log);
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| if (event.status >= 200 && event.status < 300) { | ||
| successfulProviderResponseCount += 1; |
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. 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
- "github.com"See Network Configuration for more information.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes
This patch fixes one Anthropic routing bug, but it also introduces a compatibility break on the provider path rewrite.
Blocking theme
- The Anthropic request target now unconditionally switches from
/messagesto/v1/messagesfor every configured Anthropic base URL, with no compatibility gate or fallback. That can break existing non-firewall or older proxy endpoints immediately.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
github.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 31.4 AIC · ⌖ 7.39 AIC · ⊞ 23.5K
Comment /review to run again
| case "anthropic": | ||
| case "anthropic-messages": | ||
| return { api, method, url: joinApiUrl(baseUrl, "/messages") }; | ||
| return { api, method, url: joinApiUrl(baseUrl, "/v1/messages") }; |
There was a problem hiding this comment.
This change hard-switches Anthropic requests from /messages to /v1/messages for every anthropic and anthropic-messages base URL, so any existing proxy or custom base URL that still exposes the previous route will start returning hard failures.
💡 Why this blocks merge
The PR description talks about fixing the firewall proxy, but the code here changes the request path globally in `resolveProviderRequestTarget` with no compatibility check or fallback. That means this is not just a firewall-only fix: any environment using `ANTHROPIC_BASE_URL` or another non-firewall endpoint that still implements the old `/messages` route will break immediately. A 404/405 here kills every Anthropic request, so this needs either an explicit capability gate for the firewall path or a backward-compatible fallback.Suggested shape:
case "anthropic":
case "anthropic-messages":
return { api, method, url: joinApiUrl(baseUrl, supportsV1 ? "/v1/messages" : "/messages") };Or retry /messages when /v1/messages gets a route-level failure.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — this is a targeted, well-tested bug fix (native Anthropic /v1/messages routing + failure propagation via report_incomplete). Requesting changes on one correctness risk; the rest is minor.
📋 Key Themes & Highlights
Key Themes
- Possible duplicate
report_incompleteemission: the new all-requests-failed check inagent_endand the existing per-message check inmessage_endcan both fire for the same root cause, and the file-existence pre-check that's supposed to prevent double-emission doesn't see writes made through the CLI channel (see inline comment onpi_provider.cjs:401). - Minor test gap: no explicit case for zero responses ever recorded before
agent_end(see inline comment onpi_provider.test.cjs).
Positive Highlights
- ✅ Root cause is well-diagnosed: routes Anthropic through its native Messages API (
/v1/messages) instead of the OpenAI-style gateway path, restoring prompt caching — a real, specific fix rather than a workaround. - ✅ Solid regression coverage: model API resolution, request URL resolution, all-request-failure, and recovery-after-failure are all exercised with descriptive test names.
- ✅ Doc comment above
resolvePiApiForProviderwas updated to match the new behavior instead of going stale.
Applied via automated PR review (Matt Pocock skills reviewer).
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 42.9 AIC · ⌖ 14.9 AIC · ⊞ 10.3K
Comment /matt to run again
|
|
||
| if (providerRequestCount > 0 && successfulProviderResponseCount === 0) { | ||
| emitInfrastructureIncompleteIfNoSafeOutputs(`All ${providerRequestCount} Pi provider requests failed before safe outputs were emitted.`, log); | ||
| process.exitCode = 1; |
There was a problem hiding this comment.
[/diagnosing-bugs] This all-requests-failed check can double-emit report_incomplete for the same underlying failure, because the "already recorded" guard it relies on can't see prior emissions made through the CLI channel.
💡 Why this can duplicate
emitInfrastructureIncompleteIfNoSafeOutputs pre-checks for existing output by reading GH_AW_SAFE_OUTPUTS from the local filesystem (line 167). But per the docstring above it (lines 148-153), in the AWF sandbox that path is mounted read-only and the real write goes through the safeoutputs CLI to a different process — so the local file the pre-check reads never reflects a write that already happened via that CLI channel.
Concretely: if a request fails mid-session, message_end (line 349) already calls emitInfrastructureIncompleteIfNoSafeOutputs(...) and emits via the CLI. If that's also the last successful-response count staying at 0 by agent_end, the new check at line 399-401 fires again with a different details string ("All N Pi provider requests failed..."), and the local pre-check won't catch it because the CLI-written output isn't visible locally. Net effect: two report_incomplete safe outputs for one root cause.
Suggest tracking emission state in-memory within piProviderExtension (e.g. an incompleteEmitted flag set by both call sites) rather than relying solely on the file-existence pre-check, so the agent_end fallback only fires when message_end didn't already report it.
@copilot please address this.
| await handlers.after_provider_response({ type: "after_provider_response", status: 200, headers: {} }, ctx); | ||
| await handlers.agent_end(); | ||
|
|
||
| expect(process.exitCode).toBe(originalExitCode); |
There was a problem hiding this comment.
[/tdd] Good regression coverage for the all-failed and recovery-after-failure paths, but there's no test for the case where after_provider_response never fires at all (e.g. the process errors out before a response, which is exactly the scenario message_end's connection-error test already exercises separately).
💡 Suggested edge case
Consider adding a case where before_provider_request fires once, no after_provider_response ever fires, then agent_end runs directly. That path is currently only implicitly covered (since successfulProviderResponseCount stays 0), but an explicit test would pin the intended semantics distinctly from the "one failed response" test at line 122, and guard against a future refactor accidentally requiring a response event to be recorded before this branch triggers.
@copilot please address this.
There was a problem hiding this comment.
Reviewed the Anthropic routing fix (backend-only, no UI surface — Impeccable modes not applicable).
resolvePiApiForProvidernow correctly routesanthropictoanthropic-messages; verified against Go-side log fixtures (pkg/cli/token_usage_test.go) that confirm/v1/messagesis the real firewall proxy path.joinApiUrl(baseUrl, "/v1/messages")is safe sincebaseUrlis always a bare origin (viaendpointBaseUrl/URL(...).origin), so no double/v1prefix risk.- New failure-tracking (
providerRequestCount/successfulProviderResponseCount) correctly emitsreport_incompleteonly when all requests fail, and resets state properly on any success — matches the two new regression tests.
No blocking or actionable issues found.
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 · 51 AIC · ⌖ 13.3 AIC · ⊞ 8.3K
|
@copilot This PR still has unresolved review feedback. Please address these review outcomes and then run the
Also refresh the branch if needed after applying fixes.
|
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>
Confirmed the current branch includes the requested fixes: stream-error accounting invalidates provisional 2xx success, |
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
Pi routed Anthropic models through OpenAI Chat Completions, producing invalid upstream paths and disabling prompt caching. Failed inference-only sessions could then exit successfully without reporting useful output.
Native Anthropic routing
anthropic-messages./v1/messages, restoring native prompt-cache accounting.Failure propagation
report_incompletethrough the trusted safe-outputs CLI and exit nonzero when every request fails.Regression coverage
/v1, then the cache-miss guard aborts the run #58109