Reviewed and merged GitHub PR #859

Summary

Closed, Completed

Understand
Read linked issue #843 and PR details.
Understand
Understand
Reviewed relevant source files.
Understand
Understand
Checked changelogs.
Understand
Edit
Added changelog entry.
Edit
Edit
Resolved conflicts and fixed test file.
Edit

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

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.

Now let me read issue #843 that's referenced in the PR:

Now let me read the relevant source files to fully understand the implementation:

Readopenai-completions.ts

Readtypes.ts

Readmodel-registry.ts

Now let me check the changelogs:

ReadCHANGELOG.md

Now let me check the documentation that might need updates:

ReadREADME.md

Now I have all the relevant context. Let me check if there's any related documentation that should be updated:

Let me verify the current state of the types by reading the end of the openai-completions file to understand the full context of the changes:

Readopenai-completions.ts

Now I have the complete picture. Here's my review:

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

Changelog:

Missing. A changelog entry is required in packages/ai/CHANGELOG.md before merge. The entry should be:
- Section: ### Added under ## [Unreleased]
- Format: Added OpenRouter provider routing support for custom models via \openRouterRouting` compat field (#859 by @v01dpr1mr0s3)`

Good:

Clean, minimal implementation addressing issue #843's requirements
Correct placement: openRouterRouting is added to OpenAICompletionsCompat which is the right home for OpenAI-compatible API quirks
Uses baseUrl detection (model.baseUrl.includes("openrouter.ai")) rather than provider name, which correctly handles custom "virtual providers" with OpenRouter-specific labels (as the PR author notes)
Schema validation in model-registry.ts updated to allow the new field without exploding
JSDoc comment with link to OpenRouter docs
The approach of requiring both only and order together aligns with the author's testing findings about OpenRouter ignoring only alone

Bad:

Missing openRouterRouting in getCompat() return object: In openai-completions.ts, the detectCompat() function adds openRouterRouting: {} at line 733, and getCompat() adds openRouterRouting: model.compat.openRouterRouting ?? {} at line 757, but the PR diff shows these are being added. However, looking at the full file, the getCompat() function is missing the openRouterRouting merge logic. Wait, looking again at the diff, it does add line 757. This looks correct.
No tests: There are no tests for the OpenRouter routing feature. The test suite should have at least a unit test verifying that the provider field is correctly added to the params when openRouterRouting is set.
Type safety with as any: Line 418 uses (params as any).provider = model.compat.openRouterRouting; which bypasses TypeScript type checking. This is acceptable given OpenAI's SDK types don't include OpenRouter-specific fields.

Ugly:

None. The implementation is appropriately scoped and follows existing patterns.

Questions or Assumptions:

The implementation assumes OpenRouter's provider field format matches what's described in their docs. The PR author has tested this manually.
Is there a scenario where openRouterRouting is set on a non-OpenRouter model? The baseUrl check guards against this, but the compat field could still be set incorrectly. Not a real concern given this is for custom models where the user controls the config.

Change summary:

packages/ai/src/types.ts: Added OpenRouterRouting interface with only and order fields. Added openRouterRouting optional field to OpenAICompletionsCompat.
packages/ai/src/providers/openai-completions.ts: Added logic in buildParams() to inject provider field into request params when model uses OpenRouter and has openRouterRouting set. Added openRouterRouting to detectCompat() defaults and getCompat() merge logic.
packages/coding-agent/src/core/model-registry.ts: Added TypeBox schema for OpenRouterRouting and added it to OpenAICompletionsCompatSchema for validation.

Tests:

No tests added. Should have at least a unit test in packages/ai/test/ to verify the routing parameters are correctly passed to OpenRouter requests.

Documentation:

packages/coding-agent/README.md does not need modification. The existing "Custom Models and Providers" section covers the compat field, and this PR adds a new sub-field. However, documenting the openRouterRouting field would be beneficial for discoverability. A brief mention in the compat field section would help users find this feature.

fuck it merge it via gh cli,

Readmodel-registry.ts80 lines

I see the conflict. The PR was based on an older version that didn't have the newer compat fields. Let me resolve it:

Editedmodel-registry.ts

manually went in and resolved, check if correct

Readmodel-registry.ts60 lines

Looks correct. The conflict is resolved properly: all the existing fields are kept (requiresToolResultName, requiresAssistantAfterToolResult, etc.) and openRouterRouting is added at the end.

There's a type error. The test file needs openRouterRouting added to the compat object:

Readopenai-completions-tool-result-images.test.ts40 lines

Editedopenai-completions-tool-result-images.test.ts

Now let me add the changelog entry:

ReadCHANGELOG.md30 lines

EditedCHANGELOG.md

Done. PR #859 has been:

Rebased onto main (resolved conflict in model-registry.ts)
Fixed the missing openRouterRouting field in the test file
Added changelog entry to packages/ai/CHANGELOG.md
Merged into main and pushed
PR closed with a thank-you comment