Add Pi inference request diagnostics to provider logging#33886
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
💫 TO BE CONTINUED... Smoke Claude failed! Our hero faces unexpected challenges... |
|
|
|
🌑 The shadows whisper... Smoke Codex failed. The oracle requires further meditation... |
|
🧪 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 #33886 does not have the 'implementation' label and has 0 new lines of code in default business logic directories (≤100 threshold). |
|
📰 BREAKING: Smoke Copilot is now investigating this pull request. Sources say the story is developing... |
|
🚀 Smoke Gemini MISSION COMPLETE! Gemini has spoken. ✨ Caution agentic threat detected |
There was a problem hiding this comment.
Pull request overview
This PR improves Pi provider diagnostics by logging inferred inference request targets and response metadata so “Connection error.” failures can be attributed to a specific provider route.
Changes:
- Add
before_provider_requestlogging to record inferred provider API, method, and concrete inference URL. - Add
after_provider_responselogging to record status code and response header names (without values). - Add
message_enderror logging that includes the last known request/response context, plus new focused tests.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/pi_provider.cjs | Adds request/response/error diagnostic logging utilities and Pi event handlers. |
| actions/setup/js/pi_provider.test.cjs | Adds tests covering request/response diagnostics and assistant-side error correlation. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 2
| function formatResponseHeaderNames(headers) { | ||
| const names = Object.keys(headers || {}) | ||
| .map(name => String(name).toLowerCase()) | ||
| .sort(); | ||
| return names.length > 0 ? names.join(",") : "none"; | ||
| } |
There was a problem hiding this comment.
Good catch on Headers instance! Me agree this important fix.
Warning
Firewall blocked 6 domains
The following domains were blocked by the firewall during workflow execution:
accounts.google.comandroid.clients.google.comclients2.google.comcontentautofill.googleapis.comsafebrowsingohttpgateway.googleapis.comwww.google.com
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
📰 BREAKING: Report filed by Smoke Copilot · ● 933.3K
| function joinApiUrl(baseUrl, apiPath) { | ||
| return `${baseUrl.replace(/\/+$/, "")}${apiPath}`; | ||
| } |
This comment has been minimized.
This comment has been minimized.
|
@copilot disable cool-down for smoke-pi See https://github.com/github/gh-aw/actions/runs/26257437323/job/77283410051#step:16:1 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Approved with minor suggestions
This PR successfully adds diagnostic logging for Pi inference requests and responses, which will significantly improve debuggability when connection errors occur. The implementation is well-structured with proper test coverage.
Summary of suggestions
- Edge case in URL joining: Consider stripping leading slashes from
apiPathto handle malformedbaseUrlinputs - Type safety: Use a typedef for the
modelparameter to improve IntelliSense and catch type errors - State management: Current shared state is fine for single-agent workflows, but consider per-instance state if parallel sessions become a requirement
None of these are blocking issues — the code works correctly for the current use case.
🔎 Code quality review by PR Code Quality Reviewer · ● 571.5K
| * @returns {string} | ||
| */ | ||
| function joinApiUrl(baseUrl, apiPath) { | ||
| return `${baseUrl.replace(/\/+$/, "")}${apiPath}`; |
There was a problem hiding this comment.
Regex pattern may fail on URLs with multiple consecutive slashes: The regex /+$ only strips trailing slashes, but baseUrl could have internal doubled slashes (e.g., "(apiproxy/redacted) resulting in "(apiproxy/redacted)
💡 Suggested fix
function joinApiUrl(baseUrl, apiPath) {
const cleanBase = baseUrl.replace(/\/+$/, "");
const cleanPath = apiPath.replace(/^\/+/, "");
return `${cleanBase}/${cleanPath}`;
}This ensures exactly one slash between base and path regardless of input formatting.
| * | ||
| * @param {any} model | ||
| * @returns {{ api: string, method: string, url: string }} | ||
| */ |
There was a problem hiding this comment.
Type annotation could be more precise: model is typed as any, but based on usage it appears to have a known shape ({ api?: string, baseUrl?: string, ... }).
💡 Suggested improvement
/**
* `@typedef` {Object} PiModel
* `@property` {string} [api]
* `@property` {string} [baseUrl]
* `@property` {string} [provider]
* `@property` {string} [id]
*/
/**
* Resolve the Pi model's inferred provider request target for logging.
*
* `@param` {PiModel|undefined} model
* `@returns` {{ api: string, method: string, url: string }}
*/
function resolveProviderRequestTarget(model) {
// ...
}Improves IntelliSense and catches type errors at authoring time.
| function piProviderExtension(pi) { | ||
| const log = DEFAULT_LOGGER; | ||
| /** @type {{ api: string, method: string, url: string }|null} */ | ||
| let lastProviderRequest = null; |
There was a problem hiding this comment.
Mutable state in module scope could cause issues across multiple Pi sessions: lastProviderRequest and lastProviderResponse are shared across all invocations of event handlers.
💡 Consideration
If multiple Pi sessions run concurrently (unlikely in current workflows but possible in future parallel agent scenarios), their requests could overwrite each other's lastProviderRequest state, leading to misattributed error logs.
For now this is acceptable given single-agent workflows, but consider encapsulating this state per Pi instance if parallel sessions become a requirement:
function piProviderExtension(pi) {
const log = DEFAULT_LOGGER;
const state = {
lastProviderRequest: null,
lastProviderResponse: null,
};
registerConfiguredProviders(pi, log);
pi.on("before_provider_request", (_event, ctx) => {
state.lastProviderRequest = resolveProviderRequestTarget(ctx && ctx.model);
// ...
});
}This keeps state local to each Pi extension instance.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose and /tdd — requesting changes on test coverage gaps and potential concurrency issues.
📋 Key Themes & Highlights
Key Themes
- Test coverage gaps: Missing edge case tests for exported utility functions (
joinApiUrl,formatResponseHeaderNames) - Brittle test assertions: Full-string matching makes tests fragile to harmless log format changes
- Race condition risk: Module-scoped
lastProviderRequestcould misattribute concurrent requests - Silent fallback masking: Missing context is silently handled instead of logged as a warning
Positive Highlights
- ✅ Excellent privacy-preserving design: logs header names, not values
- ✅ Strong correlation across events: request → response → error logging chain is clear
- ✅ Good test coverage for happy paths: all three event handlers have integration tests
- ✅ Thoughtful API design:
resolveProviderRequestTargethandles unknown API types gracefully
🔍 /diagnose perspective
What makes debugging harder:
- Concurrent requests could misattribute log entries (see race condition comment)
- Silent fallback to
resolveProviderRequestTargethides event ordering issues - No explicit logging when context is incomplete
What makes debugging easier:
- Clear request → response → error correlation
- Privacy-safe logging prevents production log scrubbing
- Graceful degradation with sentinel values like
"(baseUrl unavailable)"
🧪 /tdd perspective
Test strengths:
- All three event handlers (
before_provider_request,after_provider_response,message_end) have dedicated tests - Tests verify log output structure and content
- Edge case covered: error logging when no HTTP response returned
Test gaps:
- Exported utility functions lack edge case coverage (see inline comments)
- Brittle assertions will fail on harmless log format changes (field reordering, whitespace)
- No tests for concurrent request scenarios (if Pi supports them)
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · ● 964K
| ctx | ||
| ); | ||
|
|
||
| expect(stderrOutput.some(line => line.includes("provider_request provider=copilot model=claude-sonnet-4 api=openai-completions method=POST url=http://api-proxy:10002/v1/chat/completions"))).toBe(true); |
There was a problem hiding this comment.
[/tdd] Brittle assertion: full string matching will break on any log format change.
💡 Suggested refactor
Split into multiple focused assertions that test the essential parts:
const requestLog = stderrOutput.find(line => line.includes("provider_request"));
expect(requestLog).toBeDefined();
expect(requestLog).toMatch(/provider=copilot/);
expect(requestLog).toMatch(/model=claude-sonnet-4/);
expect(requestLog).toMatch(/api=openai-completions/);
expect(requestLog).toMatch(/url=http:\/\/api-proxy:10002\/v1\/chat\/completions/);This makes test failures more informative (you know which field is wrong) and survives harmless log format changes like reordering fields.
| * @param {string} baseUrl | ||
| * @param {string} apiPath | ||
| * @returns {string} | ||
| */ |
There was a problem hiding this comment.
[/tdd] Missing edge case tests for joinApiUrl — what happens with multiple trailing slashes or empty apiPath?
💡 Suggested tests
describe("joinApiUrl", () => {
it("removes single trailing slash from baseUrl", () => {
expect(module.joinApiUrl("(api.com/redacted) "/path"))
.toBe("(api.com/redacted)
});
it("removes multiple trailing slashes from baseUrl", () => {
expect(module.joinApiUrl("(api.com/redacted) "/path"))
.toBe("(api.com/redacted)
});
it("handles baseUrl without trailing slash", () => {
expect(module.joinApiUrl("(api.com/redacted) "/path"))
.toBe("(api.com/redacted)
});
it("handles empty apiPath", () => {
expect(module.joinApiUrl("(api.com/redacted) ""))
.toBe("(api.com/redacted)
});
});The current implementation looks correct, but explicit tests prevent future regressions when someone "simplifies" it.
| function formatResponseHeaderNames(headers) { | ||
| const names = Object.keys(headers || {}) | ||
| .map(name => String(name).toLowerCase()) | ||
| .sort(); |
There was a problem hiding this comment.
[/tdd] Missing test coverage for formatResponseHeaderNames with edge cases.
💡 Suggested tests
describe("formatResponseHeaderNames", () => {
it("formats multiple headers alphabetically", () => {
expect(module.formatResponseHeaderNames({
"X-Request-ID": "123",
"Content-Type": "application/json",
"Accept": "*/*"
})).toBe("accept,content-type,x-request-id");
});
it("returns 'none' for empty object", () => {
expect(module.formatResponseHeaderNames({})).toBe("none");
});
it("handles null safely", () => {
expect(module.formatResponseHeaderNames(null)).toBe("none");
});
it("handles undefined safely", () => {
expect(module.formatResponseHeaderNames(undefined)).toBe("none");
});
});This function is exported and reusable — nail down its contract with tests.
| lastProviderRequest = resolveProviderRequestTarget(ctx && ctx.model); | ||
| lastProviderResponse = null; | ||
| const provider = ctx?.model?.provider || "(unknown provider)"; | ||
| const model = ctx?.model?.id || getConfiguredModel() || "(unknown model)"; |
There was a problem hiding this comment.
[/diagnose] Race condition risk: lastProviderRequest could be overwritten by concurrent requests.
💡 Diagnosis
If Pi makes overlapping inference calls:
- Request A fires →
lastProviderRequest= A's target - Request B fires →
lastProviderRequest= B's target (overwrites A) - Response A returns → logs B's target for A's response
This misattribution could confuse debugging.
Suggested fix: Use a WeakMap keyed by request context or a correlation ID:
const requestMap = new Map(); // or WeakMap if ctx objects are GC-able
pi.on("before_provider_request", (event, ctx) => {
const target = resolveProviderRequestTarget(ctx?.model);
const correlationId = event.correlationId || crypto.randomUUID();
requestMap.set(correlationId, { target, timestamp: Date.now() });
log(`provider_request correlation_id=${correlationId} ...`);
});
pi.on("after_provider_response", (event, ctx) => {
const correlationId = event.correlationId;
const request = requestMap.get(correlationId) || { target: resolveProviderRequestTarget(ctx?.model) };
requestMap.delete(correlationId);
log(`provider_response correlation_id=${correlationId} ...`);
});Only worth fixing if Pi actually makes concurrent requests — verify behavior first.
| log(`provider_response provider=${provider} model=${model} status=${event.status} method=${request.method} url=${request.url} response_headers=${lastProviderResponse.responseHeaders}`); | ||
| }); | ||
|
|
||
| pi.on("message_end", event => { |
There was a problem hiding this comment.
[/diagnose] Fallback logic masks null context: ctx && ctx.model may hide when context is unexpectedly missing.
💡 Instrumentation opportunity
If ctx is null/undefined or ctx.model is missing, the current code silently falls back:
const request = lastProviderRequest || resolveProviderRequestTarget(ctx && ctx.model);But this hides a potential Pi SDK contract violation. Consider logging when fallback is used:
let request = lastProviderRequest;
if (!request) {
log("warning: lastProviderRequest unavailable, resolving from context");
request = resolveProviderRequestTarget(ctx?.model);
if (request.url === "(baseUrl unavailable)") {
log("warning: model context incomplete, missing baseUrl");
}
}This makes it obvious when event ordering is unexpected or context is incomplete.
There was a problem hiding this comment.
Thanks for catching this! 🎯
Warning
Firewall blocked 6 domains
The following domains were blocked by the firewall during workflow execution:
accounts.google.comandroid.clients.google.comclients2.google.comcontentautofill.googleapis.comsafebrowsingohttpgateway.googleapis.comwww.google.com
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
📰 BREAKING: Report filed by Smoke Copilot · ● 1.7M
This comment has been minimized.
This comment has been minimized.
|
🚀 Smoke Gemini MISSION COMPLETE! Gemini has spoken. ✨ Caution agentic threat detected |
Agent Container Tool Check
Result: 12/12 tools available ✅ All required development tools are accessible in the agent container environment.
|
|
Caution agentic threat detected Smoke Test Result: FAIL\n- GitHub MCP Testing: ✅\n- Web Fetch Testing: ✅\n- File Writing Testing: ✅\n- Bash Tool Testing: ✅\n- Build gh-aw: ❌Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "localhost"See Network Configuration for more information.
|
|
@copilot add more logging for /reflect call failure in pi_harness.cjs |
|
👋 Smoke test agent pass through. Me test all things. All work! 🔥 Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
|
Me back again! Smoke test fun. Me leave happy message. Engine work good! 🎉 Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
There was a problem hiding this comment.
Code look good. Diagnostics helpful for debugging Pi requests. Me approve changes!
Warning
Firewall blocked 6 domains
The following domains were blocked by the firewall during workflow execution:
accounts.google.comandroid.clients.google.comclients2.google.comcontentautofill.googleapis.comsafebrowsingohttpgateway.googleapis.comwww.google.com
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
📰 BREAKING: Report filed by Smoke Copilot · ● 933.3K
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
Smoke Test: Claude — Run §26267127926Core tests #1–12: ✅ ✅ Overall: PARTIAL (Serena #3 returned 2 symbols before EOF; #19 skipped — no safe PR to close.) Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
There was a problem hiding this comment.
💥 Automated smoke test review - all systems nominal!
Warning
Firewall blocked 6 domains
The following domains were blocked by the firewall during workflow execution:
accounts.google.comandroid.clients.google.comclients2.google.comcontentautofill.googleapis.comsafebrowsingohttpgateway.googleapis.comwww.google.com
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
💥 [THE END] — Illustrated by Smoke Claude · ● 11.3M
| * @param {string} baseUrl | ||
| * @param {string} apiPath | ||
| * @returns {string} | ||
| */ |
There was a problem hiding this comment.
Nit (smoke-test): joinApiUrl cleanly handles trailing slashes — nice defensive coding. Consider an inline unit test alongside the helper.
| function formatResponseHeaderNames(headers) { | ||
| const names = Object.keys(headers || {}) | ||
| .map(name => String(name).toLowerCase()) | ||
| .sort(); |
There was a problem hiding this comment.
Nit (smoke-test): formatResponseHeaderNames returns sorted lowercase header names — looks good for deterministic logs.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in |
|
@copilot set reflect timeouts to 60seconds |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in |
|
✨ Gemini awakens... Smoke Gemini begins its journey on this pull request... |
|
🔮 The ancient spirits stir... Smoke Codex awakens to divine this pull request... |
|
📰 BREAKING: Smoke Copilot is now investigating this pull request. Sources say the story is developing... |
|
🥧 Pi initializing... Smoke Pi begins on this pull request... |
|
💥 WHOOSH! Smoke Claude springs into action on this pull request! [Panel 1 begins...] |
Pi failures in the harness currently collapse to a generic
"Connection error.", which makes it hard to tell what inference call failed. This change adds request/response context to Pi provider logs so failures include the HTTP method, target URL, status code, and response header names.Request diagnostics
before_provider_requestopenai-completions→/chat/completionsopenai-responses/ Codex / Azure responses →/responsesanthropic-messages→/messagesResponse diagnostics
after_provider_responseError correlation
stopReason: "error", log the last known request target alongside the surfaced errorTest coverage
✨ PR Review Safe Output Test - Run 26257437320
Warning
Firewall blocked 6 domains
The following domains were blocked by the firewall during workflow execution:
accounts.google.comandroid.clients.google.comclients2.google.comcontentautofill.googleapis.comsafebrowsingohttpgateway.googleapis.comwww.google.comSee Network Configuration for more information.
pr-sous-chef: automatic branch update (run: https://github.com/github/gh-aw/actions/runs/26263580470)
pr-sous-chef: automated branch update requested (workflow run 26265503881)
✨ PR Review Safe Output Test - Run 26267127926
Warning
Firewall blocked 6 domains
The following domains were blocked by the firewall during workflow execution:
accounts.google.comandroid.clients.google.comclients2.google.comcontentautofill.googleapis.comsafebrowsingohttpgateway.googleapis.comwww.google.comSee Network Configuration for more information.