Conversation
Contributor
|
Looking for one thing? Review this PR in Change Stack to search files, summaries, diffs, and code without losing your place. Summary by CodeRabbit
WalkthroughAdds provider-level tool-format overrides and resolution (resolveToolFormat/getToolFormatOverride), integrates them into OpenAI/OpenAI-Vercel providers and request preparation, syncs CLI ephemeral config with persisted settings for tool-format, and fixes GitHub download stream cleanup with optional headers. ChangesTool Format Override Implementation
GitHub Download Cleanup
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
the
maintainer:e2e:ok
Contributor
LLxprt PR Review – PR #1949Issue AlignmentIssue #1943 reported that
Evidence tied to issue requirements:
Side Effects
Code QualityStrengths:
Concerns:
Tests and CoverageCoverage impact: Increase Four new behavioral test files with substantial coverage:
No mock theater detected. Tests use real VerdictReady The PR correctly fixes both parts of issue #1943: explicit |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/config/extensions/github.ts`:
- Around line 647-655: downloadFile currently only resolves on file.on('finish')
and rejects on 'error', so if the HTTP response/socket closes mid-transfer
without emitting 'error' the Promise can hang and leave a partial file; add
handlers for res.on('aborted') and res.on('close') and file.on('close') that
check the existing settled flag and, if not settled, mark settled, call
cleanupAfterFileClosed (or cleanupAndReject) to remove partial file and reject
the Promise with an appropriate error; ensure these handlers mirror the existing
file/res 'error' logic and remove their listeners when settled to avoid leaks
(reference downloadFile, settled, res, file, and
cleanupAfterFileClosed/cleanupAndReject).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: dc547928-1ece-4119-86a5-650416f4bb40
📒 Files selected for processing (1)
packages/cli/src/config/extensions/github.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 270000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: E2E Test (Linux) - sandbox:docker
- GitHub Check: E2E Test (Linux) - sandbox:none
- GitHub Check: CodeQL
- GitHub Check: Lint (Javascript)
- GitHub Check: Run LLxprt review
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1854
File: packages/core/src/core/subagentRuntimeSetup.test.ts:77-84
Timestamp: 2026-03-31T02:12:44.939Z
Learning: In vybestack/llxprt-code PR `#1854` (fix: preserve provider terminal metadata and tool schemas), the legacy `parameters` fallback field was intentionally and completely removed from all tool declaration interfaces and schema converters (openai/schemaConverter.ts, openai-vercel/schemaConverter.ts, anthropic/schemaConverter.ts, AnthropicProvider.ts, ToolFormatter.ts, AgentRuntimeLoader.ts, runtimeAdapters.ts). `parametersJsonSchema` is now the sole required contract for tool declarations. Converters throw an error when `parametersJsonSchema` is absent instead of falling back to `parameters`. Do not flag the absence of `parameters` assertions or suggest preserving backward compatibility with the legacy `parameters` field in any review of this codebase going forward — the single-field contract is intentional by design.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1736
File: packages/core/src/providers/openai/OpenAIRequestBuilder.test.ts:316-395
Timestamp: 2026-03-19T22:50:00.853Z
Learning: In the llxprt-code codebase (`packages/core/src/providers/openai/OpenAIRequestBuilder.ts`, previously `OpenAIProvider.ts` around line 1250), `validateToolMessageSequence` intentionally does NOT deduplicate tool messages by `tool_call_id`. Contiguous tool messages sharing the same `tool_call_id` are legitimate in conversation history (e.g., replay artifacts or adapter-level splitting across message boundaries). The function only removes *orphaned* tool messages (those whose `tool_call_id` does not match any declared tool call in the preceding assistant message). Eager deduplication of same-id tool messages has historically caused strict-provider HTTP 400 errors and must not be introduced.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1736
File: packages/core/src/providers/openai/OpenAIStreamProcessor.ts:110-123
Timestamp: 2026-03-19T23:27:48.689Z
Learning: In `packages/core/src/providers/openai/OpenAIStreamProcessor.ts` (extracted from the original `generatePipelineChatCompletionImpl` in `OpenAIProvider.ts`, lines 2050-2160), text-parsed tool calls from `extractKimiToolCallsFromText()` and `deps.textToolParser.parse()` (GemmaToolCallParser) are intentionally yielded directly as `ToolCallBlock[]` without being routed through `deps.toolCallPipeline.addFragment()`. The `toolCallPipeline` is exclusively used for delta-based streaming tool calls coming from the OpenAI API. This two-path design is deliberate — do not flag missing `addFragment()` calls for text-parsed tool calls as a bug.
📚 Learning: 2026-04-23T23:32:39.418Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1907
File: packages/cli/src/config/extensions/github.ts:70-70
Timestamp: 2026-04-23T23:32:39.418Z
Learning: In `packages/cli/src/config/extensions/github.ts` (vybestack/llxprt-code), `installMetadata.ref ?? 'HEAD'` intentionally uses nullish coalescing for git ref fallback. An empty-string ref is considered explicit misconfiguration and should not be silently replaced with 'HEAD'; only null/undefined (absent ref) should default to 'HEAD' per git conventions. Do not flag this as inconsistent with the codebase's falsy-coalescing pattern.
Applied to files:
packages/cli/src/config/extensions/github.ts
📚 Learning: 2026-02-06T15:52:42.315Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1305
File: scripts/generate-keybindings-doc.ts:1-5
Timestamp: 2026-02-06T15:52:42.315Z
Learning: In reviews of vybestack/llxprt-code, do not suggest changing existing copyright headers from 'Google LLC' to 'Vybestack LLC' for files that originated from upstream. Preserve upstream copyrights in files that came from upstream, and only apply 'Vybestack LLC' copyright on newly created, original LLxprt files. If a file is clearly LLxprt-original, it may carry the Vybestack header; if it is upstream-originated, keep the original sponsor header.
Applied to files:
packages/cli/src/config/extensions/github.ts
📚 Learning: 2026-03-31T02:12:43.093Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1854
File: packages/core/src/core/subagentRuntimeSetup.test.ts:77-84
Timestamp: 2026-03-31T02:12:43.093Z
Learning: In this codebase, tool declarations should follow the single required contract `parametersJsonSchema`; do not ask to preserve or reintroduce the legacy `parameters` fallback field. Reviewers should not flag assertions/checks for missing `parameters` or suggest backward-compatibility behavior for `parameters`. Schema converters/providers are expected to error if `parametersJsonSchema` is absent instead of falling back to `parameters`.
Applied to files:
packages/cli/src/config/extensions/github.ts
🔇 Additional comments (1)
packages/cli/src/config/extensions/github.ts (1)
559-574: LGTM!Also applies to: 593-643
Comment on lines
+647
to
+655
| cleanupAfterFileClosed(file, err, cleanupAndReject); | ||
| }); | ||
|
|
||
| res.pipe(file); | ||
| file.on('finish', () => file.close(resolve as () => void)); | ||
| file.on('finish', () => { | ||
| if (!settled) { | ||
| settled = true; | ||
| file.close(resolve as () => void); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the target file and show the relevant implementation region
FILE="packages/cli/src/config/extensions/github.ts"
echo "== file =="
ls -la "$FILE"
echo "== showing lines around 600-720 =="
nl -ba "$FILE" | sed -n '600,720p'
echo "== searching for downloadFile cleanup helpers =="
rg -n "function downloadFile|downloadFile\\(|cleanupAfterFileClosed\\(" "$FILE"
echo "== showing cleanupAfterFileClosed definition (if in-file) =="
# Try to find its definition and print around it
rg -n "cleanupAfterFileClosed" "$FILE" -n
# best-effort: print around each match
for line in $(rg -n "cleanupAfterFileClosed" "$FILE" | cut -d: -f1); do
start=$((line-40))
end=$((line+80))
echo "---- around $line ----"
nl -ba "$FILE" | sed -n "${start},${end}p" | head -n 200
done
echo "== looking for res.complete usage or settled logic =="
rg -n "settled|res\\.complete|on\\('close'\\)|on\\('finish'\\)|on\\('error'\\)" "$FILE"Repository: vybestack/llxprt-code
Length of output: 268
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/cli/src/config/extensions/github.ts"
echo "== file =="
ls -la "$FILE"
echo "== showing lines around 600-720 =="
cat -n "$FILE" | sed -n '600,720p'
echo "== searching for downloadFile cleanup helpers =="
rg -n "function downloadFile|downloadFile\\(|cleanupAfterFileClosed\\(" "$FILE" || true
echo "== showing cleanupAfterFileClosed definition (if in-file) =="
matches="$(rg -n "cleanupAfterFileClosed" "$FILE" || true)"
echo "$matches" | head -n 50
if [ -n "$matches" ]; then
while read -r line; do
ln="$(echo "$line" | cut -d: -f1)"
start=$((ln-60))
end=$((ln+120))
echo "---- around $ln ----"
cat -n "$FILE" | sed -n "${start},${end}p" | head -n 220
done <<< "$matches"
fi
echo "== looking for res.complete / settled / close / finish / error logic =="
rg -n "settled|res\\.complete|on\\('close'\\)|on\\('finish'\\)|on\\('error'\\)" "$FILE" || trueRepository: vybestack/llxprt-code
Length of output: 15754
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "res\\.complete|\\.complete\\b" packages/cli/src/config/extensions/github.ts packages/cli/src || true
rg -n "on\\('close'\\)|on\\(\"close\"\\)|on\\('aborted'\\)|aborted\\b|IncomingMessage" packages/cli/src || trueRepository: vybestack/llxprt-code
Length of output: 4593
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/cli/src/config/extensions/github.ts"
echo "== call sites around line ~430-520 (downloadFile usage) =="
cat -n "$FILE" | sed -n '420,520p'
echo "== find downloadFile usages in github.ts =="
rg -n "downloadFile\\(" "$FILE"
echo "== search repo for similar pipe/finish download patterns =="
rg -n "res\\.pipe\\(file\\)|file\\.on\\('finish'\\)|cleanupAfterFileClosed\\(" packages/cli/src || true
echo "== check package.json for `@types/node` (or types) =="
fd -t f -a "package.json" . | head -n 20
# show top-level package.json first if exists
if [ -f package.json ]; then echo "--- root package.json ---"; cat package.json; fi
rg -n "\"`@types/node`\"" -S . || true
rg -n "node\"\\s*:\\s*\"|engines\"\\s*:\\s*\\{|target\\s*:\\s*\"|lib\\s*:\\s*\\[" -S package.json packages/cli/package.json packages/cli/tsconfig*.json tsconfig*.json 2>/dev/null || true
echo "== list tsconfig targets for `@types/node` compatibility clues =="
fd -t f "tsconfig*.json" . | head -n 20
for f in $(fd -t f "tsconfig*.json" . | head -n 10); do
echo "--- $f ---"
cat "$f"
doneRepository: vybestack/llxprt-code
Length of output: 22172
🌐 Web query:
Node.js IncomingMessage close event complete property res.complete premature connection closed promise resolve finish pipe file never finish hang
💡 Result:
In Node.js, the IncomingMessage.complete property is a boolean that indicates whether an HTTP message was fully received and successfully parsed [1][2]. It is commonly used to detect if a connection was terminated prematurely (i.e., before the complete message was delivered) by checking its value during the close event [1][3]. Handling premature connection closures and stream hangs requires understanding how Node.js manages HTTP streams: 1. Detecting Premature Closure: You can check res.complete inside the close event listener to determine if the message was truncated [1][3]. If res.complete is false, the connection was likely terminated unexpectedly [1][2]. javascript res.on('close', () => { if (!res.complete) { console.error('The connection was terminated while the message was still being sent'); } }); 2. Avoiding Stream Hangs: Using res.pipe(destination) can lead to issues where the destination stream does not finish if the source (IncomingMessage) encounters a premature closure [3][4]. Since Node.js 13.0, res.pipe() may not automatically call dst.end() if the response is truncated [3]. 3. Recommended Practices: - Use stream.pipeline instead of manual .pipe() calls [3][4]. stream.pipeline is designed to handle cleanup, errors, and premature closures automatically, ensuring that errors are propagated and resources are not leaked [3]. - If you are waiting for a process to complete, rely on stream.finished or the finish event of the destination stream rather than just the end event of the IncomingMessage [4]. - If your application involves complex streaming (e.g., pipelines where you need to detect errors early), verify if you are accidentally destroying the IncomingMessage (often via for await loops or manual socket destruction), which can prevent proper completion events [5]. If you find that your application hangs or does not finish, ensure that you are not manually handling end events in a way that ignores close or error events, as a premature closure will emit close but may not emit end or finish in the expected manner [3][6][7]. Using modern stream utilities helps maintain consistent behavior across different Node.js versions [3].
Citations:
- 1: https://bun.sh/reference/node/http/IncomingMessage/complete
- 2: https://docs.deno.com/api/node/http/~/IncomingMessage.prototype.complete
- 3: https://wh0.github.io/2024/01/21/http-truncate.html
- 4: https.ServerResponse pipe doesn't fully consume the response nodejs/node#61881
- 5: Problems with cooperation between http and pipeline/"for await" nodejs/node#38262
- 6: http: emit error on aborted ServerResponse nodejs/node#63448
- 7: http: IncomingMessage emits 'end' after 'close' nodejs/node#29295
🌐 Web query:
@types/node IncomingMessage complete boolean property
💡 Result:
In the Node.js TypeScript definitions provided by @types/node, the IncomingMessage interface includes a boolean property named complete [1][2]. The message.complete property indicates whether a complete HTTP message has been received and successfully parsed [2][3]. It is commonly used to verify if a client or server fully transmitted a message before the connection was terminated [2][4]. For example, when listening for the end event on an IncomingMessage object, you can check this property to detect incomplete transfers [2][5]: const req = http.request({ /*... */ }, (res) => { res.resume; res.on('end', => { if (!res.complete) { console.error('The connection was terminated while the message was still being sent'); } }); }); This property has been part of the Node.js http module since version 0.3.0 [2][6].
Citations:
- 1: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/fcb257683af38b3e64981393946a35f2cc1b15fa/types/node/http.d.ts
- 2: https://cdn.jsdelivr.net/npm/@types/node@25.0.8/http.d.ts
- 3: https://docs.deno.com/api/node/http/~/IncomingMessage.prototype.complete
- 4: https://bun.sh/reference/node/http/IncomingMessage/complete
- 5: https://docs.deno.com/api/node/http/~/IncomingMessage
- 6: https://cdn.jsdelivr.net/npm/@types/node@17.0.31/http.d.ts
Handle premature HTTP response closes in downloadFile()
downloadFile() settles only on file.on('finish') (resolve) and file/res 'error' (reject). If the server/socket closes mid-transfer without triggering 'error', there’s no 'close'/'aborted' handler, so the Promise can hang and leave a partial file.
Proposed fix
// Handle response stream errors
res.on('error', (err) => {
cleanupAfterFileClosed(file, err, cleanupAndReject);
});
+ res.on('close', () => {
+ if (!res.complete && !settled) {
+ cleanupAfterFileClosed(
+ file,
+ new Error('Download connection closed before completion'),
+ cleanupAndReject,
+ );
+ }
+ });
res.pipe(file);
file.on('finish', () => {
if (!settled) {
settled = true;
file.close(resolve as () => void);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| cleanupAfterFileClosed(file, err, cleanupAndReject); | |
| }); | |
| res.pipe(file); | |
| file.on('finish', () => file.close(resolve as () => void)); | |
| file.on('finish', () => { | |
| if (!settled) { | |
| settled = true; | |
| file.close(resolve as () => void); | |
| } | |
| cleanupAfterFileClosed(file, err, cleanupAndReject); | |
| }); | |
| res.on('close', () => { | |
| if (!res.complete && !settled) { | |
| cleanupAfterFileClosed( | |
| file, | |
| new Error('Download connection closed before completion'), | |
| cleanupAndReject, | |
| ); | |
| } | |
| }); | |
| res.pipe(file); | |
| file.on('finish', () => { | |
| if (!settled) { | |
| settled = true; | |
| file.close(resolve as () => void); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cli/src/config/extensions/github.ts` around lines 647 - 655,
downloadFile currently only resolves on file.on('finish') and rejects on
'error', so if the HTTP response/socket closes mid-transfer without emitting
'error' the Promise can hang and leave a partial file; add handlers for
res.on('aborted') and res.on('close') and file.on('close') that check the
existing settled flag and, if not settled, mark settled, call
cleanupAfterFileClosed (or cleanupAndReject) to remove partial file and reject
the Promise with an appropriate error; ensure these handlers mirror the existing
file/res 'error' logic and remove their listeners when settled to avoid leaks
(reference downloadFile, settled, res, file, and
cleanupAfterFileClosed/cleanupAndReject).
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-24.x-ubuntu-latest' artifact from the main CI run. |
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
1 participant
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.
TLDR
Fixes OpenAI-family tool format override handling so an explicit /toolformat setting such as openai is honored even when the selected model name looks like Kimi, Mistral, or Qwen. Also persists /toolformat changes into Config ephemerals so saved profiles capture the selected tool format.
Dive Deeper
This change adds a shared settings-aware tool format resolver for OpenAI-family providers. It checks provider settings for an explicit toolFormat override first, validates the configured value, and only falls back to model-name auto-detection when the override is absent, invalid, or auto.
Updated paths:
Invalid hand-edited toolFormat strings are ignored with a warning and fall back to auto-detection rather than becoming an unsafe effective format.
Reviewer Test Plan
Recommended validation:
Automated coverage added for:
Testing Matrix
Validated on macOS:
Linked issues / bugs
Fixes #1943