Supercode cli - #297
Merged
Merged
Conversation
… AI streaming - Introduced a new function to send status heartbeats during upstream processing, preventing client timeouts. - Enhanced the AI response streaming by splitting reasoning content from user-facing results, ensuring cleaner output. - Updated various API calls to utilize the new heartbeat mechanism and improved error handling for response bodies. - Refactored markdown rendering to better separate reasoning and final answers, enhancing user experience during AI interactions. - Added tests for the new content splitting functionality to ensure proper behavior across different scenarios.
- Bumped version to 0.1.102 in package.json. - Added new dependencies for @opentui/core and @opentui/react to improve UI capabilities. - Updated build scripts to include new external dependencies. - Enhanced TypeScript configuration to support new JSX import source. - Refactored agent logic to incorporate reasoning and text streaming improvements. - Improved error handling and response management in AI services.
|
Too many files changed for review (116 files, 100 file limit). Bypass the limit by tagging |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedToo many files! This PR contains 130 files, which is 30 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (130)
You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
| export function stableArgsKey(args: unknown): string { | ||
| if (!args || typeof args !== "object") return JSON.stringify(args ?? null) | ||
| const obj = args as Record<string, unknown> | ||
| return JSON.stringify(obj, Object.keys(obj).sort()) |
There was a problem hiding this comment.
A replacer array is an allow-list applied at every depth, so nested object args lose all their keys and two distinct calls collapse to the same argsKey — the repetition guard then false-positives and kills legit loops. Sorting recursively with a replacer keeps the key stable and lossless:
Suggested change
| return JSON.stringify(obj, Object.keys(obj).sort()) | |
| export function stableArgsKey(args: unknown): string { | |
| if (!args || typeof args !== "object") return JSON.stringify(args ?? null) | |
| return JSON.stringify(args, (_key, value) => | |
| value && typeof value === "object" && !Array.isArray(value) | |
| ? Object.fromEntries(Object.entries(value).sort(([a], [b]) => (a < b ? -1 : 1))) | |
| : value, | |
| ) | |
| } |
Comment on lines
+167
to
+168
| const useNative = | ||
| toolLoop === "native" || (toolLoop === "auto" && false) // auto → execute |
There was a problem hiding this comment.
Dead branch — && false always kills the auto case, and the comment already says auto routes to execute.
Suggested change
| const useNative = | |
| toolLoop === "native" || (toolLoop === "auto" && false) // auto → execute | |
| const useNative = toolLoop === "native" // auto → execute |
| if (event.toolCalls?.length) { | ||
| for (const tc of event.toolCalls) { | ||
| const a = (tc as any).input ?? {} | ||
| const argsKey = JSON.stringify(a, Object.keys(a).sort()) |
There was a problem hiding this comment.
tool-result.ts now exports stableArgsKey (the proxy loop uses it) — reuse it here so the adapter loop and the proxy behave identically.
Suggested change
| const argsKey = JSON.stringify(a, Object.keys(a).sort()) | |
| const argsKey = stableArgsKey(a) |
(will need stableArgsKey added to the existing ../tool-result import)
| tools: args.tools as any, | ||
| stopWhen: stepCountIs(8), | ||
| abortSignal: args.signal, | ||
| prepareStep: async ({ messages: stepMessages }) => { |
There was a problem hiding this comment.
This prepareStep/onStepFinish guard block is a near-verbatim copy of the one in adapters/openai-compatible.ts (empty-result sentinel, denial loop, repetition guard, inline argsKey). Since stream-helpers.ts is already the shared home for adapter plumbing, extracting a createToolLoopGuards(callbacks) there would stop the two providers drifting — they already have: openai-compatible resets seenStepResults each step, this file doesn't.
Comment on lines
+217
to
+218
| if (toolResults?.length) { | ||
| for (const tr of toolResults) { |
There was a problem hiding this comment.
openai-compatible resets seenStepResults at the start of each onStepFinish so the sentinel only reflects the latest round; here it accumulates across all steps, so the "all empty" notice keeps growing with stale outcomes from earlier rounds.
Suggested change
| if (toolResults?.length) { | |
| for (const tr of toolResults) { | |
| if (toolResults?.length) { | |
| seenStepResults.length = 0 | |
| for (const tr of toolResults) { |
Comment on lines
+141
to
+143
| } catch { | ||
| // Fallback: direct build agent.generate when unified path lacks LanguageModel id | ||
| const buildAgent = agentService.get("build") |
There was a problem hiding this comment.
This swallows user aborts from the unified path and then silently starts a second generation through the fallback (which isn't given the signal at all). Rethrow aborts before falling back:
Suggested change
| } catch { | |
| // Fallback: direct build agent.generate when unified path lacks LanguageModel id | |
| const buildAgent = agentService.get("build") | |
| } catch (err: any) { | |
| if (err?.name === "AbortError") throw err | |
| // Fallback: direct build agent.generate when unified path lacks LanguageModel id | |
| const buildAgent = agentService.get("build") | |
| if (!buildAgent?.generate) throw new Error("build agent not available") |
Comment on lines
+288
to
+289
| process.env.SUPERCODE_AGENT_PROVIDER = provider | ||
| if (model) process.env.SUPERCODE_AGENT_MODEL = model |
There was a problem hiding this comment.
Smuggling the provider/model into runAgentTurn via process.env is global mutable state — it leaks across sessions and breaks if two sessions ever run in one process. Add provider/model to UnifiedTurnOptions and thread them through runUnifiedTurn instead.
Comment on lines
+143
to
+149
| if (final.reasoning && final.reasoning !== reasoningAcc) { | ||
| const extra = final.reasoning.slice(reasoningAcc.length).trim() | ||
| if (extra) { | ||
| emit({ type: "reasoning", delta: extra }) | ||
| opts.onReasoning?.(extra) | ||
| } | ||
| } |
There was a problem hiding this comment.
finalizeAnswerVsProcess doesn't guarantee the returned reasoning is a superset of what was already streamed (tagged CoT in the text channel gets folded in too), so slicing by length can emit a mangled fragment. chat.ts guards this with startsWith — same here:
Suggested change
| if (final.reasoning && final.reasoning !== reasoningAcc) { | |
| const extra = final.reasoning.slice(reasoningAcc.length).trim() | |
| if (extra) { | |
| emit({ type: "reasoning", delta: extra }) | |
| opts.onReasoning?.(extra) | |
| } | |
| } | |
| if (final.reasoning && final.reasoning !== reasoningAcc) { | |
| const extra = final.reasoning.startsWith(reasoningAcc) | |
| ? final.reasoning.slice(reasoningAcc.length).trim() | |
| : final.reasoning.trim() | |
| if (extra) { | |
| emit({ type: "reasoning", delta: extra }) | |
| opts.onReasoning?.(extra) | |
| } | |
| } |
Comment on lines
+295
to
+298
| if (final.reasoning && final.reasoning !== fullReasoning) { | ||
| const extra = final.reasoning.slice(fullReasoning.length).trim() | ||
| if (extra) opts.onReasoning?.(extra) | ||
| } |
There was a problem hiding this comment.
Same as the provider-bridge copy — final.reasoning isn't always a superset of fullReasoning, so slicing by length can emit a garbled chunk. Guard with startsWith:
Suggested change
| if (final.reasoning && final.reasoning !== fullReasoning) { | |
| const extra = final.reasoning.slice(fullReasoning.length).trim() | |
| if (extra) opts.onReasoning?.(extra) | |
| } | |
| const final = finalizeAnswerVsProcess(fullText, fullReasoning) | |
| if (final.reasoning && final.reasoning !== fullReasoning) { | |
| const extra = final.reasoning.startsWith(fullReasoning) | |
| ? final.reasoning.slice(fullReasoning.length).trim() | |
| : final.reasoning.trim() | |
| if (extra) opts.onReasoning?.(extra) | |
| } |
Comment on lines
+91
to
+96
| const onParentAbort = opts.signal | ||
| ? () => controller.abort() | ||
| : undefined | ||
| if (onParentAbort && opts.signal) { | ||
| opts.signal.addEventListener("abort", onParentAbort, { once: true }) | ||
| } |
There was a problem hiding this comment.
If the caller's signal is already aborted when the guard is created, the abort event never fires and the request runs un-aborted. Fast-path it:
Suggested change
| const onParentAbort = opts.signal | |
| ? () => controller.abort() | |
| : undefined | |
| if (onParentAbort && opts.signal) { | |
| opts.signal.addEventListener("abort", onParentAbort, { once: true }) | |
| } | |
| const onParentAbort = opts.signal | |
| ? () => controller.abort() | |
| : undefined | |
| if (opts.signal?.aborted) { | |
| controller.abort() | |
| } else if (onParentAbort && opts.signal) { | |
| opts.signal.addEventListener("abort", onParentAbort, { once: true }) | |
| } |
- Modified .gitignore to streamline generated files handling. - Updated package.json files across multiple apps to specify exact versions for @types/react and @types/react-dom. - Enhanced turbo.json outputs to include generated files for better build management. - Refactored postinstall script for consistent Prisma client generation across packages. - Adjusted Prisma schema output path for improved stability in monorepo setup.
- Specified exact versions for @types/react and @types/react-dom in bun.lock and various package.json files. - Added @prisma/client-runtime-utils dependency to enhance Prisma functionality in marketplace, web, and db packages.
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.
Description
Please include a summary of the change and which issue is fixed.
Fixes #(issue)
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes.
bun testpassesbun run typecheckpassesbun run lintpasses (if applicable)Checklist: