Reviewed and merged GitHub PR #890

Summary

Closed, Completed

Understand
Read PR description, comments, commits, and changed files.
Understand
Understand
Identified and reviewed linked issues.
Understand
Understand
Analyzed PR diff and relevant code files.
Understand
Build
Rebased branch and resolved conflicts.
Build
Edit
Merged changes into main branch and pushed.
Edit

You are given one or more GitHub PR URLs: https://github.com/badlogic/pi-mono/pull/890

For each PR URL, do the following in order:

Read the PR page in full. Include description, all comments, all commits, and all changed files.
Identify any linked issues referenced in the PR body, comments, commit messages, or cross links. Read each issue in full, including all comments.
Analyze the PR diff. Read all relevant code files in full with no truncation from the current main branch and compare against the diff. Do not fetch PR file blobs unless a file is missing on main or the diff context is insufficient. Include related code paths that are not in the diff but are required to validate behavior.
Check for a changelog entry in the relevant packages/*/CHANGELOG.md files. Report whether an entry exists. If missing, state that a changelog entry is required before merge and that you will add it if the user decides to merge. Follow the changelog format rules in AGENTS.md. Verify:
- Entry uses correct section (### Breaking Changes, ### Added, ### Fixed, etc.)
- External contributions include PR link and author: Fixed foo ([#123](https://github.com/badlogic/pi-mono/pull/123) by [@user](https://github.com/user))
- Breaking changes are in ### Breaking Changes, not just ### Fixed
Check if packages/coding-agent/README.md, packages/coding-agent/docs/.md, packages/coding-agent/examples/**/.md require modification. This is usually the case when existing features have been changed, or new features have been added.
Provide a structured review with these sections:
- Good: solid choices or improvements
- Bad: concrete issues, regressions, missing tests, or risks
- Ugly: subtle or high impact problems
Add Questions or Assumptions if anything is unclear.
Add Change summary and Tests.

Output format per PR: PR: Changelog:

... Good:
... Bad:
... Ugly:
... Questions or Assumptions:
... Change summary:
... Tests:
...

If no issues are found, say so under Bad and Ugly.

Let me get the full diff and check the linked issue:

Now let me check the current main branch files to compare:

Let me also check one critical detail about the PR - whether the response.function_call_arguments.done handler is being preserved in the shared code:

I see a critical issue. Let me verify the content guard issue as well:

Let me verify if the coding-agent documentation is complete:

Now I have enough information to provide a comprehensive review.

PR: https://github.com/badlogic/pi-mono/pull/890

Changelog:

Entry exists: Yes, in packages/ai/CHANGELOG.md under ## [Unreleased] with correct ### Added section and proper external contributor attribution.

Good:

Well-structured code sharing: Extracts openai-responses-shared.ts with convertResponsesMessages(), convertResponsesTools(), and processResponsesStream() functions that are reused by openai-responses, openai-codex-responses, and azure-openai-responses providers.
Proper test coverage: Adds Azure OpenAI tests to all relevant test files: stream.test.ts, tokens.test.ts, abort.test.ts, empty.test.ts, context-overflow.test.ts, unicode-surrogate.test.ts, tool-call-without-result.test.ts, image-tool-result.test.ts, total-tokens.test.ts, cross-provider-handoff.test.ts.
Utility pattern for credentials: Creates azure-utils.ts with hasAzureOpenAICredentials() and resolveAzureDeploymentName() following the bedrock-utils.ts pattern.
Documentation complete: Updates both packages/ai/README.md and packages/coding-agent/README.md with Azure setup instructions, env vars, and deployment name mapping.
Type safety improvements: Replaces (block as any) casts with proper typed alternatives like (block as { index?: number }) and uses type guards (c): c is TextContent =>) in the shared code.
Defensive guards added: Adds null/empty checks before accessing currentItem.content which prevents potential runtime errors.
Flexible deployment mapping: Supports AZURE_OPENAI_DEPLOYMENT_NAME_MAP for custom model-to-deployment mappings (e.g., gpt-4o-mini=my-deployment).

Bad:

Missing response.function_call_arguments.done handler: The shared processResponsesStream() function does NOT include the handler for response.function_call_arguments.done that exists on main (added in commit fb364c89). This handler is required for LM Studio and other OpenAI-compatible providers that send full arguments via the done event instead of streaming deltas. This is a regression bug.
Missing currentBlock.partialJson fallback in function_call completion: The current main branch uses currentBlock.partialJson as a fallback when building the final toolCall arguments, but the shared code only uses JSON.parse(item.arguments). This was part of the response.function_call_arguments.done fix.
Model generation clones all OpenAI Responses models: The script clones ALL openai-responses models to azure-openai-responses. Some models (like gpt-5-codex, codex-mini-latest) are Codex-specific and may not be available on Azure.

Ugly:

The response.function_call_arguments.done regression is critical: Without this handler, the PR will break tool calling for providers like LM Studio that rely on it. The author confirmed tests pass with Azure, OpenAI, and Codex OAuth, but these providers use delta events. The bug only manifests with providers sending done events.

Questions or Assumptions:

Was the response.function_call_arguments.done handler (from commit fb364c89) intentionally omitted or accidentally missed during the refactor?
Are all cloned OpenAI Responses models actually available on Azure OpenAI?

Change summary:

Adds new azure-openai-responses provider using OpenAI SDK's AzureOpenAI client
Creates openai-responses-shared.ts with shared message conversion, tool conversion, and stream processing logic
Refactors openai-responses.ts and openai-codex-responses.ts to use the shared functions
Updates model generation to clone OpenAI Responses models for Azure with baseUrl: ""
Adds Azure-specific options: azureApiVersion, azureResourceName, azureBaseUrl, azureDeploymentName
Updates types, stream.ts, model-resolver.ts, model-registry.ts for new provider
Comprehensive test coverage and documentation updates

Tests:

Added Azure OpenAI tests to 10 test files
Author reports all tests pass with Azure OpenAI, OpenAI, and Codex OAuth
Missing: LM Studio or equivalent testing to verify response.function_call_arguments.done compatibility (which would fail due to the regression)

Verdict: The PR needs a fix for the missing response.function_call_arguments.done handler in openai-responses-shared.ts before merge. The handler should be ported from the current openai-responses.ts implementation, including the currentBlock.partialJson fallback logic.

merge into a local branch, resolve conflicts, add the fix for response.function_call_arguments.done

Let me resolve the conflicts. First, the CHANGELOG:

Now resolve the openai-responses.ts conflict - keeping the PR's shared code approach:

Now let me read the shared code file to add the missing handler:

The shared code already has the response.function_call_arguments.done handler and the currentBlock.partialJson fallback. The PR was updated to address these concerns. Let me verify the merge status and run checks:

All checks pass. Now commit the merge:

Merge complete. Let me verify the fix is in place by checking the shared code:

Done. The merge is complete with conflicts resolved.

Summary:

Created branch azure-openai-responses-merge from main
Merged PR #890 and resolved 2 conflicts:
- packages/ai/CHANGELOG.md: Combined both the Bun fix (#922) and Azure provider addition (#890)
- packages/ai/src/providers/openai-responses.ts: Kept PR's approach (using shared processResponsesStream())

Good news about response.function_call_arguments.done: The PR already includes the fix in openai-responses-shared.ts:

Handler for response.function_call_arguments.done event (line 383)
currentBlock.partialJson fallback in function_call completion
currentBlock = null cleanup after function_call

My earlier review concern was based on a truncated diff. The author addressed this before my review (likely when refactoring).

All checks pass. The branch is ready for further testing or merge to main.

please read @packages/ai/src/providers/openai-codex-responses.ts @packages/ai/src/providers/openai-responses.ts on the main branch and ensure that we have ported the logic of converting pi-ai contexts to the responses format and stream parsing 100% correctly

Now let me read the shared code from our merged branch:

Let me also check the merged codex file to see how it uses the shared code:

Now I'll do a detailed comparison of the implementations:

ok, codex, i want you to also evaluate this. is the port faithful? do we do all the same things we used to do wrt cost tracking, message conversion, etc. pp?

Let me do a systematic, line-by-line comparison of all the key functionality:

Now I have the full picture. Here's my comprehensive analysis:

FAITHFUL PORT ANALYSIS

✅ Message Conversion - FAITHFUL + IMPROVEMENTS

Feature	Main OpenAI	Main Codex	Shared	Status
`normalizeToolCallId` callback	✅	✅	✅	Same
`transformMessages`	✅	✅	✅	Same
System prompt (developer/system)	✅	via `instructions`	✅ with `includeSystemPrompt` option	Same
User message string	✅	✅	✅	Same
User message array	✅	✅	✅	Same
Image filtering	✅	✅	✅	Same
`shortHash` for long IDs	✅	❌	✅	Codex improved
`msgId` generation	✅	❌ (no id field)	✅	Codex changed
`isDifferentModel` check	✅	❌	✅	Codex improved
`thinkingSignature` parsing	✅	✅	✅	Same
toolResult with images	✅	✅	✅	Same

Codex behavior change: Assistant messages now include id field. This shouldn't cause issues since OpenAI Responses API accepts it.

✅ Tool Conversion - FAITHFUL

Feature	Main OpenAI	Main Codex	Shared	Status
`strict: false`	✅	-	✅ (default)	Same
`strict: null`	-	✅	✅ (via option)	Same

✅ Stream Event Handling - FAITHFUL + IMPROVEMENTS

Event	Main OpenAI	Main Codex	Shared	Status
`response.output_item.added`	✅	✅	✅	Same
`response.reasoning_summary_part.added`	✅	✅	✅	Same
`response.reasoning_summary_text.delta`	✅	✅	✅	Same
`response.reasoning_summary_part.done`	✅	✅	✅	Same
`response.content_part.added`	✅	✅	✅	Same
`response.output_text.delta`	✅	✅	✅ + null guard	Improved
`response.refusal.delta`	✅	✅	✅ + null guard	Improved
`response.function_call_arguments.delta`	✅	✅	✅	Same
`response.function_call_arguments.done`	✅	❌	✅	Codex improved
`response.output_item.done`	✅	✅	✅	Same
`response.completed`	✅	✅	✅	Same
`response.done`	❌	✅	✅ (mapped)	Same
`error`	✅	✅	✅	Same
`response.failed`	✅	✅	✅	Same

✅ Cost/Usage Tracking - FAITHFUL

Feature	Main OpenAI	Shared	Status
`calculateCost(model, output.usage)`	✅	✅	Same
`applyServiceTierPricing`	✅ (inline)	✅ (via callback)	Same
`input_tokens - cached_tokens`	✅	✅	Same
`output_tokens`	✅	✅	Same
`cacheRead = cached_tokens`	✅	✅	Same
`totalTokens`	✅	✅	Same

✅ Stop Reason Mapping - FAITHFUL

Status	Main OpenAI	Shared	Status
`completed` → `stop`	✅	✅	Same
`incomplete` → `length`	✅	✅	Same
`failed` → `error`	✅	✅	Same
`cancelled` → `error`	✅	✅	Same
`in_progress` → `stop`	✅	✅	Same
`queued` → `stop`	✅	✅	Same
toolUse override	✅	✅	Same
exhaustive check	✅	✅	Same

✅ function_call Completion - FAITHFUL + IMPROVEMENT

Feature	Main OpenAI	Main Codex	Shared	Status
`partialJson` fallback	✅	❌	✅	Codex improved
`currentBlock = null` cleanup	✅	❌	✅	Codex improved

SUMMARY

✅ The port is FAITHFUL

All functionality from main is preserved:

Message conversion logic is identical
Tool conversion with configurable strict option
All stream events handled
Cost/usage tracking identical
Stop reason mapping identical

🔧 Improvements in the shared code

Codex gains isDifferentModel check - fixes cross-model handoff issues
Codex gains response.function_call_arguments.done handler - fixes LM Studio compatibility
Codex gains partialJson fallback - more robust function_call argument handling
Added null guards for currentItem.content - prevents edge case crashes
Codex gains shortHash/msgId - consistent with OpenAI behavior

⚠️ One behavior change (low risk)

Codex assistant messages now include id field on message objects. This matches the OpenAI behavior and shouldn't cause issues since the Codex endpoint uses the same underlying API format.

Verdict: The port is faithful and includes several improvements. No regressions found.

all tests green, rebase on main and merge with main

Summary:

Rebased azure-openai-responses-merge on origin/main, resolved conflicts in packages/ai/src/providers/openai-responses.ts and packages/ai/CHANGELOG.md.
Fast-forward merged into main and pushed to origin/main.

Tests:

Not run (per instruction that tests are already green).

push

Already pushed to origin/main.

pr is still open, can we close?

Closed PR #890 with comment: "Merged to main via rebase."