feat: upstream PR triage — 6 fixes + e2e tests#2
Merged
Conversation
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The hard-coded model ID list in
handleModelsand theMODEL_MAP/related logic inopenai-to-cli.tsnow need to be kept in sync manually; consider extracting a shared source of truth (e.g., a models config module) so adding or renaming models is less error-prone. - In
normalizeModelName, silently defaultingundefinedto"claude-sonnet-4"may mask upstream issues; consider either surfacing an explicit error or at least logging/handling the undefined case earlier so unintended model selection is easier to detect. - The
usagefield added to the final streaming chunk is typed via ananycast ondoneChunk; it would be more robust to extend the response/chunk type definition so this shape is enforced by TypeScript instead of bypassing type checking.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The hard-coded model ID list in `handleModels` and the `MODEL_MAP`/related logic in `openai-to-cli.ts` now need to be kept in sync manually; consider extracting a shared source of truth (e.g., a models config module) so adding or renaming models is less error-prone.
- In `normalizeModelName`, silently defaulting `undefined` to `"claude-sonnet-4"` may mask upstream issues; consider either surfacing an explicit error or at least logging/handling the undefined case earlier so unintended model selection is easier to detect.
- The `usage` field added to the final streaming chunk is typed via an `any` cast on `doneChunk`; it would be more robust to extend the response/chunk type definition so this shape is enforced by TypeScript instead of bypassing type checking.
## Individual Comments
### Comment 1
<location> `src/server/routes.ts:248-249` </location>
<code_context>
- // Send final done chunk with finish_reason
+ // Send final done chunk with finish_reason and usage data
const doneChunk = createDoneChunk(requestId, lastModel);
+ if (result.usage) {
+ (doneChunk as any).usage = {
+ prompt_tokens: result.usage.input_tokens || 0,
+ completion_tokens: result.usage.output_tokens || 0,
</code_context>
<issue_to_address>
**suggestion:** Avoid `as any` by extending the doneChunk type to include `usage`.
Rather than casting to `any`, update the type returned by `createDoneChunk` (or the shared streaming chunk type) to optionally include `usage`. That way the SSE payload shape stays type-safe and any mismatches are caught at compile time.
Suggested implementation:
```typescript
if (!res.writableEnded) {
// Send final done chunk with finish_reason and usage data
const doneChunk = createDoneChunk(requestId, lastModel);
if (result.usage) {
doneChunk.usage = {
prompt_tokens: result.usage.input_tokens || 0,
completion_tokens: result.usage.output_tokens || 0,
total_tokens:
(result.usage.input_tokens || 0) + (result.usage.output_tokens || 0),
};
}
res.write(`data: ${JSON.stringify(doneChunk)}\n\n`);
```
You will also need to update the type definition used by `createDoneChunk`:
1. Locate the type/interface that describes the chunk returned by `createDoneChunk` (for example, something like `SseChunk`, `StreamChunk`, or the explicit return type of `createDoneChunk`).
2. Extend it with an optional `usage` field, e.g.:
```ts
type Usage = {
prompt_tokens: number;
completion_tokens: number;
total_tokens: number;
};
interface DoneChunk {
// existing fields...
usage?: Usage;
}
```
3. Ensure `createDoneChunk` is declared to return this updated type (or that the shared streaming chunk type also has `usage?: Usage` if `createDoneChunk` returns that).
4. If you have a discriminated union of chunk types, make sure the `done`/terminal variant supports `usage?: Usage` so the assignment in this handler is type-safe.
</issue_to_address>
### Comment 2
<location> `src/adapter/openai-to-cli.ts:15-46` </location>
<code_context>
"claude-haiku-4": "haiku",
- // With provider prefix
+ "claude-haiku-4-5": "haiku",
+ // With provider prefix (claude-code-cli/)
"claude-code-cli/claude-opus-4": "opus",
+ "claude-code-cli/claude-opus-4-6": "opus",
"claude-code-cli/claude-sonnet-4": "sonnet",
+ "claude-code-cli/claude-sonnet-4-5": "sonnet",
+ "claude-code-cli/claude-sonnet-4-6": "sonnet",
"claude-code-cli/claude-haiku-4": "haiku",
- // Aliases
+ "claude-code-cli/claude-haiku-4-5": "haiku",
+ // With provider prefix (claude-max/)
+ "claude-max/claude-opus-4": "opus",
+ "claude-max/claude-opus-4-6": "opus",
+ "claude-max/claude-sonnet-4": "sonnet",
+ "claude-max/claude-sonnet-4-5": "sonnet",
+ "claude-max/claude-sonnet-4-6": "sonnet",
+ "claude-max/claude-haiku-4": "haiku",
+ "claude-max/claude-haiku-4-5": "haiku",
+ // Bare aliases
"opus": "opus",
</code_context>
<issue_to_address>
**suggestion:** There’s duplication between explicit provider-prefixed entries and the later prefix-stripping logic.
Because `extractModel` removes `claude-code-cli/` and `claude-max/` before consulting `MODEL_MAP`, the provider-prefixed keys appear redundant. You could keep only the bare IDs (e.g. `claude-opus-4`, `claude-opus-4-6`, etc.) and rely on the prefix-stripping to map all variants, simplifying this map without changing behavior.
```suggestion
const MODEL_MAP: Record<string, ClaudeModel> = {
// Direct model names (provider prefixes like `claude-code-cli/` and `claude-max/`
// are stripped by extractModel before consulting this map)
"claude-opus-4": "opus",
"claude-opus-4-6": "opus",
"claude-sonnet-4": "sonnet",
"claude-sonnet-4-5": "sonnet",
"claude-sonnet-4-6": "sonnet",
"claude-haiku-4": "haiku",
"claude-haiku-4-5": "haiku",
// Bare aliases
"opus": "opus",
"sonnet": "sonnet",
"haiku": "haiku",
"opus-max": "opus",
"sonnet-max": "sonnet",
};
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Triaged all 14 open PRs from atalovesyou/claude-max-api-proxy, implemented the valuable fixes, and added end-to-end test coverage. Changes: - Fix normalizeModelName crash on undefined model (atalovesyou#7 regression) - Pass prompt via stdin instead of CLI arg to avoid E2BIG (atalovesyou#12) - Increase subprocess timeout from 5 to 15 minutes (atalovesyou#20) - Add Claude 4.5/4.6 model IDs and claude-max/ prefix (atalovesyou#10, atalovesyou#20) - Include usage data in final streaming SSE chunk (atalovesyou#16) - Wrap subprocess logging with DEBUG_SUBPROCESS env check (atalovesyou#5, atalovesyou#16) - Strip CLAUDECODE env var from subprocesses (own fix) - Add e2e test suite (7 tests covering health, models, completions) Co-Authored-By: kevinfealey <10552286+kevinfealey@users.noreply.github.com> Co-Authored-By: Max <257223904+Max-shipper@users.noreply.github.com> Co-Authored-By: James Hansen <1359077+jamshehan@users.noreply.github.com> Co-Authored-By: bitking <213560776+smartchainark@users.noreply.github.com> Co-Authored-By: Alex Rudloff's AI Agents <258647843+alexrudloffBot@users.noreply.github.com>
5baa14f to
474169d
Compare
- Add optional `usage` field to OpenAIChatChunk type, removing `as any` cast - Remove redundant provider-prefixed MODEL_MAP entries (extractModel already strips prefixes before lookup)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Triaged all 14 open PRs on atalovesyou/claude-max-api-proxy and cherry-picked the valuable fixes into our fork.
Implemented (6 changes)
normalizeModelNamecrash on undefined modelbbeb4c7) accidentally reverted this fix. WhenmodelUsageis{}(rate limits),normalizeModelNamereceivesundefinedand crashes. Restored thestring | undefinedsignature + guard.ARG_MAX(128KB–2MB), causingspawn()to fail withE2BIG. Now writes prompt to stdin instead.claude-opus-4-6,claude-sonnet-4-5,claude-sonnet-4-6,claude-haiku-4-5toMODEL_MAPand/v1/models. Also addedclaude-max/*provider prefix andopus-max/sonnet-maxaliases.usagein the final streaming chunk. Downstream consumers (session compaction, cost tracking) need it.console.errorcalls now gated behindDEBUG_SUBPROCESSenv var. Production logs are no longer noisy.Additional fix (not from upstream)
CLAUDECODEenv var from subprocesses — Claude CLI refuses to start whenCLAUDECODE=1is set (nested session protection). The proxy was inheriting this from the parent environment, breaking it when launched from within a Claude Code session.Already implemented in our fork (skipped)
[object Object]serializationextractText()already handles both string and array content[object Object]serializationCLAUDE_DANGEROUSLY_SKIP_PERMISSIONSenv var--dangerously-skip-permissionsalwaysDeferred
cwd: /tmpand disabled tools)Test plan
npm run buildcompiles cleanlyGET /healthreturns okGET /v1/modelslists all 7 model IDs