Skip to content

Commit 025d824

Browse files
CopilotCopilot
authored
Harden Pi provider retries and failure diagnostics (#65562)
1 parent d4d4b97 commit 025d824

9 files changed

Lines changed: 204 additions & 4 deletions

‎.changeset/patch-harden-pi-provider-failures.md‎

Lines changed: 3 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎actions/setup/js/pi_agent_core_driver.cjs‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,17 +5,29 @@ const fs = require("node:fs");
55
const path = require("node:path");
66
const { execFileSync } = require("node:child_process");
77
const { loadPiSDK, nativePiProvider, preparePiRuntime, DEFAULT_SESSION_DIR } = require("./pi_runtime.cjs");
8+
const { sanitizeProviderErrorMessage } = require("./pi_provider_error.cjs");
89
const { getErrorMessage } = require("./error_helpers.cjs");
910

1011
/** @param {any} event @returns {Record<string, any>} */
1112
function jsonEvent(event) {
12-
if (event.type !== "message_update") return event;
13+
if (event.type !== "message_update") {
14+
const sanitized = { ...event };
15+
if (typeof event.errorMessage === "string") sanitized.errorMessage = sanitizeProviderErrorMessage(event.errorMessage);
16+
if (typeof event.message?.errorMessage === "string") {
17+
sanitized.message = { ...event.message, errorMessage: sanitizeProviderErrorMessage(event.message.errorMessage) };
18+
}
19+
if (Array.isArray(event.messages)) {
20+
sanitized.messages = event.messages.map(message => (typeof message?.errorMessage === "string" ? { ...message, errorMessage: sanitizeProviderErrorMessage(message.errorMessage) } : message));
21+
}
22+
return sanitized;
23+
}
1324
const { partial, ...update } = event.assistantMessageEvent;
1425
if (update.type === "toolcall_start") {
1526
const call = partial?.content?.[update.contentIndex];
1627
update.id = call?.id;
1728
update.toolName = call?.name;
1829
}
30+
if (typeof update.errorMessage === "string") update.errorMessage = sanitizeProviderErrorMessage(update.errorMessage);
1931
return { type: event.type, usage: event.usage ?? event.message?.usage, assistantMessageEvent: update };
2032
}
2133

‎actions/setup/js/pi_agent_core_driver.test.cjs‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,4 +60,28 @@ describe("Pi coding-agent SDK driver", () => {
6060
});
6161
expect(event).toEqual({ type: "message_update", usage: { input: 1 }, assistantMessageEvent: { type: "toolcall_start", contentIndex: 0, id: "call-id", toolName: "bash" } });
6262
});
63+
64+
it("redacts secrets from provider errors in emitted session events", () => {
65+
vi.stubEnv("OPENAI_API_KEY", "provider-secret-value");
66+
const event = jsonEvent({
67+
type: "message_end",
68+
message: { role: "assistant", errorMessage: '429: {"api_key":"provider-secret-value"}' },
69+
});
70+
expect(event.message.errorMessage).toContain("[REDACTED]");
71+
expect(event.message.errorMessage).not.toContain("provider-secret-value");
72+
});
73+
74+
it("redacts secrets from assistant errors in agent_end message snapshots", () => {
75+
vi.stubEnv("OPENAI_API_KEY", "provider-secret-value");
76+
const event = jsonEvent({
77+
type: "agent_end",
78+
messages: [
79+
{ role: "assistant", errorMessage: "429: provider-secret-value" },
80+
{ role: "user", content: "Complete the task" },
81+
],
82+
});
83+
expect(event.messages[0].errorMessage).toContain("[REDACTED]");
84+
expect(event.messages[0].errorMessage).not.toContain("provider-secret-value");
85+
expect(event.messages[1]).toEqual({ role: "user", content: "Complete the task" });
86+
});
6387
});

‎actions/setup/js/pi_provider.cjs‎

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ const { fetchAWFReflect, AWF_API_PROXY_REFLECT_URL, AWF_REFLECT_OUTPUT_PATH, AWF
3131
const { emitInfrastructureIncomplete } = require("./safeoutputs_cli.cjs");
3232
const fs = require("fs");
3333
const { getErrorMessage } = require("./error_helpers.cjs");
34+
const { getProviderErrorDetails } = require("./pi_provider_error.cjs");
3435

3536
// Default logger: prefixed with "[gh-aw/pi-provider]" for easy grepping.
3637
// prettier-ignore
@@ -309,6 +310,8 @@ function piProviderExtension(pi) {
309310
let lastProviderRequest = null;
310311
/** @type {{ status: number, responseHeaders: string, succeeded: boolean }|null} */
311312
let lastProviderResponse = null;
313+
/** @type {{ status?: number, message: string }|null} */
314+
let lastProviderFailure = null;
312315
let providerRequestCount = 0;
313316
let successfulProviderResponseCount = 0;
314317
registerConfiguredProviders(pi, log);
@@ -333,6 +336,10 @@ function piProviderExtension(pi) {
333336
responseHeaders: formatResponseHeaderNames(event.headers),
334337
succeeded,
335338
};
339+
if (!succeeded) {
340+
const responseBody = event.body ?? event.responseBody;
341+
lastProviderFailure = getProviderErrorDetails(responseBody, event.status);
342+
}
336343
const provider = ctx?.model?.provider || "(unknown provider)";
337344
const model = ctx?.model?.id || getConfiguredModel() || "(unknown model)";
338345
log(`provider_response provider=${provider} model=${model} status=${event.status} method=${request.method} url=${request.url} response_headers=${lastProviderResponse.responseHeaders}`);
@@ -344,10 +351,11 @@ function piProviderExtension(pi) {
344351
return;
345352
}
346353
const request = lastProviderRequest || { api: message.api || "(unknown api)", method: "POST", url: "(request unavailable)" };
347-
const status = lastProviderResponse ? String(lastProviderResponse.status) : "no-response";
354+
lastProviderFailure = getProviderErrorDetails(message.errorMessage, lastProviderResponse?.status);
355+
const status = typeof lastProviderFailure.status === "number" ? String(lastProviderFailure.status) : "no-response";
348356
const responseHeaders = lastProviderResponse ? lastProviderResponse.responseHeaders : "none";
349357
log(
350-
`provider_error provider=${message.provider || "(unknown provider)"} model=${message.model || "(unknown model)"} api=${request.api} status=${status} method=${request.method} url=${request.url} response_headers=${responseHeaders} error=${JSON.stringify(message.errorMessage)}`
358+
`provider_error provider=${message.provider || "(unknown provider)"} model=${message.model || "(unknown model)"} api=${request.api} status=${status} method=${request.method} url=${request.url} response_headers=${responseHeaders} error=${JSON.stringify(lastProviderFailure.message)}`
351359
);
352360
if (lastProviderResponse?.succeeded) {
353361
successfulProviderResponseCount -= 1;
@@ -403,7 +411,10 @@ function piProviderExtension(pi) {
403411
}
404412

405413
if (providerRequestCount > 0 && successfulProviderResponseCount === 0) {
406-
emitInfrastructureIncompleteIfNoSafeOutputs(`All ${providerRequestCount} Pi provider requests failed before safe outputs were emitted.`, log);
414+
const providerFailureDetails = lastProviderFailure
415+
? ` Last provider failure: ${typeof lastProviderFailure.status === "number" ? `HTTP ${lastProviderFailure.status}; ` : ""}message=${JSON.stringify(lastProviderFailure.message)}.`
416+
: "";
417+
emitInfrastructureIncompleteIfNoSafeOutputs(`All ${providerRequestCount} Pi provider requests failed before safe outputs were emitted.${providerFailureDetails}`, log);
407418
process.exitCode = 1;
408419
}
409420
});

‎actions/setup/js/pi_provider.test.cjs‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
2+
import childProcess from "node:child_process";
23
import fs from "fs";
34
import os from "os";
45
import path from "path";
@@ -119,6 +120,51 @@ describe("pi_provider.cjs", () => {
119120
expect(stderrOutput.some(line => line.includes("provider_response provider=copilot model=claude-sonnet-4 status=503 method=POST url=http://api-proxy:10002/v1/chat/completions response_headers=content-type,x-request-id"))).toBe(true);
120121
});
121122

123+
it("includes the provider status and redacted response message in the incomplete report", async () => {
124+
const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), "pi-provider-"));
125+
process.env.GH_AW_SAFE_OUTPUTS = path.join(tempDir, "outputs.jsonl");
126+
process.env.GH_AW_SAFEOUTPUTS_CLI = "safeoutputs";
127+
process.env.OPENAI_API_KEY = "provider-secret-value";
128+
const execFileSync = vi.spyOn(childProcess, "execFileSync").mockReturnValue("");
129+
130+
const handlers = {};
131+
const pi = {
132+
registerProvider: vi.fn(),
133+
on: vi.fn((event, handler) => {
134+
handlers[event] = handler;
135+
}),
136+
};
137+
const ctx = {
138+
model: {
139+
provider: "openai",
140+
id: "gpt-5.4",
141+
api: "openai-responses",
142+
baseUrl: "https://api.openai.com/v1",
143+
},
144+
};
145+
146+
module.default(pi);
147+
await handlers.before_provider_request({}, ctx);
148+
await handlers.after_provider_response({ status: 429, headers: {} }, ctx);
149+
await handlers.message_end({
150+
message: {
151+
role: "assistant",
152+
provider: "openai",
153+
model: "gpt-5.4",
154+
stopReason: "error",
155+
errorMessage: '429: {"message":"rate limited","api_key":"provider-secret-value"}',
156+
},
157+
});
158+
await handlers.agent_settled();
159+
160+
const reportArgs = execFileSync.mock.calls.find(([, args]) => args[0] === "report_incomplete")?.[1];
161+
expect(reportArgs.join(" ")).toContain("HTTP 429");
162+
expect(reportArgs.join(" ")).toContain("rate limited");
163+
expect(reportArgs.join(" ")).not.toContain("provider-secret-value");
164+
expect(stderrOutput.some(line => line.includes("provider_error provider=openai model=gpt-5.4 api=openai-responses status=429") && line.includes("rate limited"))).toBe(true);
165+
expect(stderrOutput.some(line => line.includes("provider-secret-value"))).toBe(false);
166+
});
167+
122168
it("resolves native Anthropic requests to the Messages API", () => {
123169
expect(
124170
module.resolveProviderRequestTarget({

‎actions/setup/js/pi_provider_error.cjs‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
// @ts-check
2+
"use strict";
3+
4+
const MAX_PROVIDER_ERROR_LENGTH = 1000;
5+
6+
/**
7+
* Reduce provider errors to bounded, single-line diagnostics without exposing credentials.
8+
*
9+
* @param {unknown} value
10+
* @returns {string}
11+
*/
12+
function sanitizeProviderErrorMessage(value) {
13+
let message;
14+
if (value === undefined || value === null) {
15+
message = "";
16+
} else if (typeof value === "string") {
17+
message = value;
18+
} else {
19+
try {
20+
message = JSON.stringify(value);
21+
} catch {
22+
message = String(value);
23+
}
24+
}
25+
26+
for (const [name, secret] of Object.entries(process.env)) {
27+
if (/(?:TOKEN|KEY|SECRET|PASSWORD|CREDENTIAL)/i.test(name) && secret && secret.length >= 6) {
28+
message = message.split(secret).join("[REDACTED]");
29+
}
30+
}
31+
32+
message = message
33+
.replace(/\bBearer\s+[^\s,;"}]+/gi, "******")
34+
.replace(/\b(?:gh[pousr]_|ghs_|github_pat_)[A-Za-z0-9._-]+\b/g, "[REDACTED]")
35+
.replace(/\bsk-(?:proj-|ant-api03-)?[A-Za-z0-9_-]{20,}\b/g, "[REDACTED]")
36+
.replace(/[\u0000-\u001f\u007f-\u009f]+/g, " ");
37+
if (message.length > MAX_PROVIDER_ERROR_LENGTH) {
38+
message = `${message.slice(0, MAX_PROVIDER_ERROR_LENGTH)}…`;
39+
}
40+
return message;
41+
}
42+
43+
/**
44+
* Extract the HTTP status and response message from a Pi provider error.
45+
*
46+
* @param {unknown} value
47+
* @param {number|undefined} responseStatus
48+
* @returns {{ status?: number, message: string }}
49+
*/
50+
function getProviderErrorDetails(value, responseStatus) {
51+
const rawMessage = sanitizeProviderErrorMessage(value);
52+
const match = /^\s*(?:HTTP\s*)?(\d{3})(?::\s*|\s+)([\s\S]*)$/i.exec(rawMessage);
53+
const parsedStatus = match ? Number(match[1]) : undefined;
54+
return {
55+
...(typeof responseStatus === "number" ? { status: responseStatus } : parsedStatus ? { status: parsedStatus } : {}),
56+
message: match ? match[2] : rawMessage,
57+
};
58+
}
59+
60+
module.exports = { sanitizeProviderErrorMessage, getProviderErrorDetails };

‎actions/setup/js/pi_provider_error.test.cjs‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
import { afterEach, describe, expect, it, vi } from "vitest";
2+
3+
const { getProviderErrorDetails, sanitizeProviderErrorMessage } = await import("./pi_provider_error.cjs");
4+
5+
afterEach(() => {
6+
vi.unstubAllEnvs();
7+
});
8+
9+
describe("Pi provider error helpers", () => {
10+
it("redacts configured provider secrets", () => {
11+
vi.stubEnv("PI_PROVIDER_TOKEN", "provider-secret-value");
12+
const message = sanitizeProviderErrorMessage("Authorization: provider-secret-value");
13+
expect(message).toContain("[REDACTED]");
14+
expect(message).not.toContain("provider-secret-value");
15+
});
16+
17+
it("removes control characters and bounds long messages", () => {
18+
expect(sanitizeProviderErrorMessage("line one\nline two")).toBe("line one line two");
19+
expect(sanitizeProviderErrorMessage("x".repeat(2000))).toHaveLength(1001);
20+
});
21+
22+
it("parses an HTTP status prefix when no response status is available", () => {
23+
expect(getProviderErrorDetails("429: rate limited", undefined)).toEqual({
24+
status: 429,
25+
message: "rate limited",
26+
});
27+
});
28+
});

‎actions/setup/js/pi_runtime.cjs‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,14 @@ function preparePiRuntime(config = parsePiConfig()) {
103103
sessionDir: DEFAULT_SESSION_DIR,
104104
enableInstallTelemetry: false,
105105
...config.settings,
106+
retry: {
107+
enabled: true,
108+
maxRetries: 2,
109+
baseDelayMs: 1000,
110+
maxAgentDelayMs: 30000,
111+
...installedSettings.retry,
112+
...config.settings?.retry,
113+
},
106114
defaultProjectTrust: "never",
107115
};
108116
writeSecureOutput(settingsPath, JSON.stringify(settings, null, 2));

‎actions/setup/js/pi_runtime.test.cjs‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,11 +52,19 @@ describe("Pi runtime configuration", () => {
5252
fs.writeFileSync(path.join(dir, "agent/settings.json"), '{"packages":["npm:example@1.0.0"]}');
5353
const result = preparePiRuntime({ settings: { defaultThinkingLevel: "high", defaultProjectTrust: "always" } });
5454
expect(result.settings.packages).toEqual(["npm:example@1.0.0"]);
55+
expect(result.settings.retry).toEqual({ enabled: true, maxRetries: 2, baseDelayMs: 1000, maxAgentDelayMs: 30000 });
5556
expect(result.settings.defaultTools).toEqual(["+codemode", "+tool_search"]);
5657
expect(result.settings.defaultProjectTrust).toBe("never");
5758
expect(result.settings.defaultThinkingLevel).toBe("high");
5859
});
5960

61+
it("merges retry defaults with partial installed and workflow settings", () => {
62+
fs.mkdirSync(path.join(dir, "agent"));
63+
fs.writeFileSync(path.join(dir, "agent/settings.json"), JSON.stringify({ retry: { maxRetries: 4 } }));
64+
const result = preparePiRuntime({ settings: { retry: { baseDelayMs: 500 } } });
65+
expect(result.settings.retry).toEqual({ enabled: true, maxRetries: 4, baseDelayMs: 500, maxAgentDelayMs: 30000 });
66+
});
67+
6068
it("uses only gateway-authorized MCP servers and preserves scoped headers", () => {
6169
fs.mkdirSync(path.join(dir, "gh-aw/mcp-config"), { recursive: true });
6270
fs.writeFileSync(path.join(dir, "gh-aw/mcp-config/mcp-servers.json"), JSON.stringify({ mcpServers: { safeoutputs: { url: "http://gateway/mcp/safeoutputs", headers: { Authorization: "gateway-session-token" } } } }));

0 commit comments

Comments
 (0)