Staging - #188
Closed
XiuJie2 wants to merge 9 commits into
Closed
Conversation
Adds a full NVIDIA NIM provider (OpenAI-compatible) alongside admin service entries for DeepSeek, OpenRouter and NVIDIA, so a deployment can supply model keys centrally instead of every request carrying its own. getApiKey() previously threw before any provider code ran when a request had no key, which would have made the new admin fields decorative. It now falls back to the admin-configured key: request key -> rotation slots (per-minute) -> single API key -> throw Rotation was reachable only when isHosted was true, which is a hardcoded hostname allowlist, so the OpenAI and Anthropic rotation slots were never read on self-hosted deployments. The fallback path is not gated on isHosted, so rotation now works anywhere; the hosted branch keeps its existing platform-key-wins behaviour. OpenAI's Default API Key stays reserved for embeddings and is not repurposed for completions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…mpose The image workflow could not run here: it targeted Blacksmith runners this fork has no access to, and hardcoded ghcr.io/tradinggoose, which the fork's GITHUB_TOKEN cannot push to. Because every tag went to one build-push step, that GHCR failure would have taken the Docker Hub push down with it. - run on ubuntu-latest with the upstream docker/* actions, plus a GHA cache to replace the build cache Blacksmith provided - derive the GHCR namespace from the repository owner, lowercased, since GHCR rejects uppercase paths - push all three images to GHCR, and mirror only the app image to Docker Hub, so the schema-carrying migrations image stays private for free - read DOCKERHUB_USERNAME from vars rather than secrets, and fail loudly when it is unset instead of pushing to an empty namespace - drop arm64: QEMU emulation on a standard runner is prohibitively slow docker-compose.prod.yml now resolves images through IMAGE_REGISTRY and publishes Postgres on loopback by default, so a public host does not expose the database. INTERNAL_SOCKET_URL is documented and wired through the compose manifests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Feat/model provider keys and registry
Copilot proxied every turn to the managed service at copilot.tradinggoose.ai, which runs the models on TradingGoose's own accounts. A self-hosted deployment therefore had to buy a Copilot API key even with its own OpenAI, Anthropic, NVIDIA or OpenRouter keys already configured, and was limited to the four models that service exposes. Add a local runtime that serves the same endpoints from inside the deployment. It returns byte-identical SSE, so the chat route, the mark-complete resume path, the abort route and the browser store are untouched: - lib/copilot/local-runtime/runtime.ts runs one model call per request, parks the turn on `awaiting_tools`, and resumes when the browser reports results. Tool execution stays in the browser exactly as before. - llm.ts adapts two wire formats: OpenAI chat-completions (OpenAI, DeepSeek, OpenRouter, NVIDIA, Ollama, xAI, Mistral, Fireworks) and Anthropic messages. - conversation-store.ts holds turn state in Redis, falling back to the in-process cache, with persisted chat history as a backstop when the cache has expired. - prompt.ts carries the system prompt, which previously lived on the service and left lib/copilot/prompts.ts holding a one-line stub. Admin > Services gains a Runtime Mode field (COPILOT_RUNTIME_MODE), defaulting to `local`; `hosted` restores the previous behaviour. The Copilot API key is now optional, since it is only used in hosted mode. The four-model whitelist is replaced by /api/copilot/models, derived from whichever providers have keys configured. OpenRouter models that cannot take tools are filtered out rather than failing at the first tool call. Also make the catalog's `envVar` fallbacks real: COPILOT_API_KEY, COPILOT_API_URL and OLLAMA_URL were documented but read by nothing, so a manifest-configured deployment silently had no Copilot credentials at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
# Conflicts: # apps/tradinggoose/widgets/widgets/copilot/components/user-input/components/model-selector.tsx
Feat/local copilot runtime
|
@XiuJie2 is attempting to deploy a commit to the TradingGoose Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe change adds a local Copilot runtime with conversation persistence, streaming, tool continuation, and dynamic model discovery. It adds NVIDIA provider support, environment-backed service configuration, rotating API keys, authenticated proxy propagation, and configurable Docker deployment settings. ChangesCopilot runtime
NVIDIA and service configuration
Deployment
Estimated code review effort: 5 (Critical) | ~90 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment Warning |
Contributor
|
| Filename | Overview |
|---|---|
| apps/tradinggoose/lib/copilot/local-runtime/runtime.ts | Implements local turn streaming and tool continuation, but parallel completion requests can lose state and permanently stall a turn. |
| apps/tradinggoose/lib/copilot/local-runtime/conversation-store.ts | Stores resumable conversations as single cached JSON blobs without atomic update support for concurrent tool results. |
| apps/tradinggoose/lib/copilot/local-runtime/llm.ts | Adds provider-specific streaming adapters and normalizes provider-prefixed model IDs before upstream requests. |
| apps/tradinggoose/lib/copilot/local-runtime/model-catalog.ts | Builds the local model picker from DB-backed provider configuration and dynamic provider listings. |
| apps/tradinggoose/lib/system-services/service.ts | Adds catalog-defined environment fallbacks while preserving stored service values as the primary runtime configuration. |
| .github/workflows/images.yml | Moves builds to GitHub-hosted runners, publishes amd64 images with GHA caching, and limits Docker Hub mirroring to the app image. |
Sequence Diagram
sequenceDiagram
participant UI as Copilot UI
participant API as Next.js Copilot API
participant Runtime as Local Copilot Runtime
participant Cache as Redis Conversation Store
participant LLM as Configured LLM Provider
UI->>API: Send chat message
API->>Runtime: Dispatch authenticated turn
Runtime->>Cache: Load/create conversation
Runtime->>LLM: Stream messages and tools
LLM-->>Runtime: Text and tool-call deltas
Runtime->>Cache: Save pending tool calls
Runtime-->>UI: SSE awaiting_tools
par Parallel tools
UI->>API: mark-complete(call A)
UI->>API: mark-complete(call B)
end
API->>Runtime: Resume requests
Runtime->>Cache: Read-modify-write conversation
Runtime->>LLM: Continue after all results
LLM-->>UI: Continuation SSE
Prompt To Fix All With AI
### Issue 1
apps/tradinggoose/lib/copilot/local-runtime/runtime.ts:2643-2665
**Parallel tool results race**
If two automatically approved tool calls complete concurrently, each handler loads and overwrites the same conversation snapshot independently. Both snapshots can retain the other pending call while one write discards the other tool result, causing neither request to resume the model and leaving the Copilot turn permanently stalled.
### Issue 2
.github/workflows/images.yml:19
**Staging changelog entry missing**
This staging-targeted PR adds no dated Markdown file under `changelog/`, omitting the branch change from the repository's required change record and leaving changelog-aware workflows without its scope and rollout context.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "Merge pull request #4 from XiuJie2/feat/..." | Re-trigger Greptile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (11)
apps/tradinggoose/providers/ai/utils-server.test.ts (1)
124-133: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename this test to match what it asserts.
The mock supplies
rotationKeys: ['only-slot-2'], which is an already-compacted list. Empty-slot filtering happens inreadRotationKeysinapps/tradinggoose/lib/system-services/runtime.ts, so this test only proves that a single-slot rotation resolves. Rename it, for example to "uses the only configured rotation slot".🤖 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 `@apps/tradinggoose/providers/ai/utils-server.test.ts` around lines 124 - 133, Rename the test case around getApiKey from “skips empty slots so a partially filled rotation still works” to describe that it resolves using the only configured rotation slot. Keep the test setup and assertions unchanged.apps/tradinggoose/lib/system-services/catalog.ts (1)
222-231: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider an
envVarfor the NVIDIAbaseUrlsetting.The Ollama base URL setting now accepts
OLLAMA_URL, and the Copilot base URL acceptsCOPILOT_API_URL. The NVIDIA base URL has no environment fallback, so a self-hosted NIM endpoint can only be set through the admin UI. Add anenvVarif deployments should configure it from a compose manifest.♻️ Proposed change
type: 'url', defaultValue: NVIDIA_API_BASE_URL_DEFAULT, required: false, + envVar: 'NVIDIA_API_BASE_URL', },🤖 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 `@apps/tradinggoose/lib/system-services/catalog.ts` around lines 222 - 231, Add an environment-variable mapping to the NVIDIA `baseUrl` setting in `settingFields`, matching the existing Ollama and Copilot base URL configuration pattern, so compose deployments can override `NVIDIA_API_BASE_URL_DEFAULT` without using the admin UI.apps/tradinggoose/app/api/providers/ai/nvidia/models/route.ts (1)
20-26: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a timeout to the upstream request.
fetchhas no timeout here, so a slow NVIDIA endpoint holds the request until the platform limit. A self-hosted NIM base URL makes this more likely. Add an abort signal.♻️ Proposed change
const response = await fetch(`${baseUrl}/models`, { headers: { 'Content-Type': 'application/json', Authorization: `Bearer ${config.apiKey}`, }, + signal: AbortSignal.timeout(10_000), next: { revalidate: 300 }, })🤖 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 `@apps/tradinggoose/app/api/providers/ai/nvidia/models/route.ts` around lines 20 - 26, Update the upstream fetch in the models route to use an AbortController-based timeout and pass its signal in the request options. Ensure the controller is scheduled to abort after the chosen timeout and that the timer is cleaned up once the fetch completes, while preserving the existing headers and revalidation behavior.apps/tradinggoose/providers/ai/utils-server.ts (1)
31-44: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winMake the
nvidiacase explicit and reject unknown providers.The
defaultbranch returns NVIDIA credentials. The only guard is the membership check ingetSystemServiceApiKey. If a provider is later added toSYSTEM_SERVICE_KEY_PROVIDERSwithout a matchingcase, that provider silently receives the NVIDIA API key and sends it to a different vendor endpoint.♻️ Proposed change
case 'openrouter': return await resolveOpenRouterServiceConfig() - default: + case 'nvidia': return await resolveNvidiaServiceConfig() + default: + throw new Error(`No system service keys configured for provider: ${provider}`) }🤖 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 `@apps/tradinggoose/providers/ai/utils-server.ts` around lines 31 - 44, Update the provider switch to add an explicit `case 'nvidia'` that returns NVIDIA credentials, and replace the `default` branch with an error for unsupported providers. Preserve the existing resolution behavior for all named providers while ensuring unknown values cannot receive NVIDIA credentials.apps/tradinggoose/lib/system-services/runtime.ts (1)
141-145: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winReuse
readRotatingApiKeyConfigfor NVIDIA configuration.
asStringmaps blank and whitespace-only strings tonull, so thebaseUrlfallback is correct. Use the helper to remove the duplicated API-key logic.🤖 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 `@apps/tradinggoose/lib/system-services/runtime.ts` around lines 141 - 145, Update resolveNvidiaServiceConfig to obtain its API-key configuration through the existing readRotatingApiKeyConfig helper instead of separately calling asString and readRotationKeys. Preserve the baseUrl handling, including the NVIDIA_API_BASE_URL_DEFAULT fallback.apps/tradinggoose/lib/copilot/local-runtime/model-catalog.ts (2)
67-71: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the configured OpenRouter base URL instead of a hardcoded literal.
LOCAL_COPILOT_OPENAI_COMPATIBLE_BASE_URLS.openrouterinapps/tradinggoose/lib/copilot/local-runtime/providers.tsalready declareshttps://openrouter.ai/api/v1. This listing hardcodes the same host. The two values can drift, and an operator who points OpenRouter at a gateway gets completions from the gateway but a model list from the public API.♻️ Proposed refactor
+import { LOCAL_COPILOT_OPENAI_COMPATIBLE_BASE_URLS } from '`@/lib/copilot/local-runtime/providers`' + async function listOpenRouterToolModels(): Promise<string[]> { - const response = await fetch('https://openrouter.ai/api/v1/models', { + const baseUrl = LOCAL_COPILOT_OPENAI_COMPATIBLE_BASE_URLS.openrouter + const response = await fetch(`${baseUrl}/models`, { headers: { 'Content-Type': 'application/json' }, signal: AbortSignal.timeout(8000), })🤖 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 `@apps/tradinggoose/lib/copilot/local-runtime/model-catalog.ts` around lines 67 - 71, Update listOpenRouterToolModels to derive its models endpoint from the configured LOCAL_COPILOT_OPENAI_COMPATIBLE_BASE_URLS.openrouter value in providers.ts, appending the appropriate models path instead of hardcoding the public OpenRouter URL. Preserve the existing request headers and timeout.
135-154: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRun the three dynamic listings in parallel.
Lines 137, 144, and 154 await OpenRouter, NVIDIA, and Ollama listings one after another. The timeouts are 8s, 8s, and 5s. On a cold cache where every provider is slow or unreachable,
/api/copilot/modelsblocks for about 21 seconds, andapps/tradinggoose/widgets/widgets/copilot/components/user-input/components/model-selector.tsxwaits on that response before it can replace an unavailable selection.⚡ Proposed refactor
- const openrouterKey = openrouter.rotationKeys[0] ?? openrouter.apiKey - if (openrouterKey) { - push('openrouter', await cachedListing('openrouter', listOpenRouterToolModels)) - } - - const nvidiaKey = nvidia.rotationKeys[0] ?? nvidia.apiKey - if (nvidiaKey) { - push( - 'nvidia', - await cachedListing('nvidia', () => - listOpenAiCompatibleModels({ - baseUrl: nvidia.baseUrl, - apiKey: nvidiaKey, - prefix: 'nvidia/', - }) - ) - ) - } - - push('ollama', await cachedListing('ollama', () => listOllamaModels(ollama.baseUrl))) + const openrouterKey = openrouter.rotationKeys[0] ?? openrouter.apiKey + const nvidiaKey = nvidia.rotationKeys[0] ?? nvidia.apiKey + + const [openrouterModels, nvidiaModels, ollamaModels] = await Promise.all([ + openrouterKey ? cachedListing('openrouter', listOpenRouterToolModels) : Promise.resolve([]), + nvidiaKey + ? cachedListing('nvidia', () => + listOpenAiCompatibleModels({ + baseUrl: nvidia.baseUrl, + apiKey: nvidiaKey, + prefix: 'nvidia/', + }) + ) + : Promise.resolve([]), + cachedListing('ollama', () => listOllamaModels(ollama.baseUrl)), + ]) + + push('openrouter', openrouterModels) + push('nvidia', nvidiaModels) + push('ollama', ollamaModels)🤖 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 `@apps/tradinggoose/lib/copilot/local-runtime/model-catalog.ts` around lines 135 - 154, Update the model catalog listing flow around cachedListing, listOpenRouterToolModels, listOpenAiCompatibleModels, and listOllamaModels so the eligible OpenRouter, NVIDIA, and Ollama requests start concurrently instead of awaiting each push sequentially. Preserve provider key checks and result ordering, then await the concurrent listing results before returning the catalog.apps/tradinggoose/app/api/copilot/proxy.ts (1)
2-2: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winImport the local dispatcher dynamically, as the completion handler already is.
Line 93 loads
handleLocalCopilotCompletionwith a dynamicimport(), but line 2 importsdispatchLocalCopilotRequeststatically.dispatch.tsreaches the local runtime, which pulls in theopenaiand@anthropic-ai/sdkclients. Every module that importsproxy.tsthen loads that graph, including hosted-only deployments that never run local mode.♻️ Proposed refactor
-import { dispatchLocalCopilotRequest } from '`@/lib/copilot/local-runtime/dispatch`'if (await isLocalCopilotMode()) { + const { dispatchLocalCopilotRequest } = await import('`@/lib/copilot/local-runtime/dispatch`') return dispatchLocalCopilotRequest({ endpoint, body, userId, signal }) }🤖 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 `@apps/tradinggoose/app/api/copilot/proxy.ts` at line 2, Update the proxy handler to remove the static dispatchLocalCopilotRequest import and dynamically import that dispatcher at the local-request call site, matching the existing handleLocalCopilotCompletion pattern. Preserve the current request behavior while ensuring hosted-only imports do not eagerly load the local runtime SDK graph.apps/tradinggoose/lib/copilot/local-runtime/completion.ts (1)
45-66: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLog the swallowed credential-resolution failures.
Both
catchblocks discard the error. When every provider fails, the operator sees only the 503 at line 118 and cannot tell which provider failed or why. Add a debug-level log in each branch.🔍 Proposed refactor
if (isLocalCopilotProvider(requested.provider)) { try { return { ...requested, apiKey: await getApiKey(requested.provider, requested.model) } - } catch { + } catch (error) { + logger.debug('Requested completion model unusable', { model: raw, error }) // Falls through to the catalog below. } } @@ apiKey: await getApiKey(group.provider, model), } - } catch { + } catch (error) { + logger.debug('Catalog provider unusable', { provider: group.provider, error }) // Try the next provider. }🤖 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 `@apps/tradinggoose/lib/copilot/local-runtime/completion.ts` around lines 45 - 66, Add debug-level logging to both credential-resolution catch blocks in the local provider selection flow: the requested-provider branch and the loop over group providers. Include the relevant provider (and model where available) plus the caught error, while preserving the existing fallback behavior.apps/tradinggoose/app/api/copilot/models/route.ts (1)
19-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHosted model grouping is derived twice from
claude-andgpt-id prefixes. Both sites split the hosted model list into an Anthropic group and an OpenAI group by testingstartsWith. Any hosted model id that matches neither prefix is silently dropped, and the two copies can drift because they read different source constants.
apps/tradinggoose/app/api/copilot/models/route.ts#L19-L30: replace the twoHOSTED_COPILOT_RUNTIME_MODELS.filter(...startsWith...)calls with a shared grouping helper exported fromapps/tradinggoose/lib/copilot/runtime-models.ts.apps/tradinggoose/widgets/widgets/copilot/components/user-input/components/model-selector.tsx#L50-L69: buildFALLBACK_GROUPSfrom that same shared helper instead of repeating the prefix filters overCOPILOT_RUNTIME_MODEL_OPTIONS.Prefer deriving the group from an explicit provider field on each runtime model definition so a new hosted provider does not require a new prefix test.
🤖 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 `@apps/tradinggoose/app/api/copilot/models/route.ts` around lines 19 - 30, The hosted model groups are duplicated and silently omit models without claude- or gpt- prefixes. In apps/tradinggoose/lib/copilot/runtime-models.ts, add and export a shared grouping helper based on an explicit provider field in each runtime model definition, supporting future providers without prefix checks; update apps/tradinggoose/app/api/copilot/models/route.ts lines 19-30 and apps/tradinggoose/widgets/widgets/copilot/components/user-input/components/model-selector.tsx lines 50-69 to derive their groups through that helper, with no direct change needed elsewhere.apps/tradinggoose/lib/copilot/local-runtime/runtime.test.ts (1)
230-248: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for abort and for superseded tool calls.
The suite covers resume paths well. Two behaviors stay untested:
abortLocalCopilotTurn, and a new user message that supersedes outstanding tool calls. The second case is the issue raised onruntime.tslines 108-137. A test that starts a turn with a pending call, sends a new message, then reports the old call should assert thatresumeLocalCopilotTurnreturnsnulland thatstreamLlmis not called a third time.🤖 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 `@apps/tradinggoose/lib/copilot/local-runtime/runtime.test.ts` around lines 230 - 248, Extend the runtime test suite with coverage for abortLocalCopilotTurn and superseded tool calls. Add a test that starts a turn with a pending tool call, sends a new user message, then resumes the original call and asserts null is returned and streamLlm is not invoked a third time; also test the expected abortLocalCopilotTurn behavior using the existing runtime helpers.
🤖 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 @.github/workflows/images.yml:
- Around line 24-29: Update the image publishing matrix in the workflow so the
realtime and migrations Dockerfiles are also published to Docker Hub alongside
the app image. Preserve their existing GHCR publishing configuration and ensure
the resulting tags match the docker.io/<namespace> registry expected by the
Compose configuration.
In `@apps/tradinggoose/.env.example.docker`:
- Line 35: Remove the duplicate POSTGRES_PORT declaration from the environment
configuration, retaining a single POSTGRES_PORT=5432 entry so Compose uses an
unambiguous host port.
- Around line 68-75: Update the tradinggoose.environment blocks in
docker-compose.local.yml, docker-compose.ollama.yml, and docker-compose.prod.yml
to forward COPILOT_RUNTIME_MODE using the default local value when the host
variable is blank or unset, unless an existing non-blank database setting
already overrides it.
In `@apps/tradinggoose/app/api/copilot/chat/route.ts`:
- Around line 882-892: Update the request payload construction around
conversationHistory to include history only when isLocalCopilotMode is true,
while preserving the existing non-empty check. Limit the forwarded
conversationHistory entries to a bounded recent-message subset before mapping
them to role and content, and reuse the existing isLocalCopilotMode import from
proxy.ts.
In `@apps/tradinggoose/lib/copilot/local-runtime/completion.ts`:
- Around line 133-143: Wrap the non-streaming delta iteration in the same
try/catch error-handling pattern used by the streaming path, covering failures
from streamLlm and aborted requests while collecting content. Ensure the catch
branch returns the established error Response shape so
proxyCopilotCompletionRequest always receives a Response instead of a rejected
promise.
- Around line 145-162: Update the ReadableStream around its start handler to
guard the terminal `[DONE]` enqueue and close operation so cancellation or an
already-closed controller cannot produce an unhandled rejection. Add a cancel()
handler that aborts the upstream deltas/provider request, reusing the existing
abort mechanism if available; preserve the error frame only if required by the
client contract and guard that enqueue as well.
In `@apps/tradinggoose/lib/copilot/local-runtime/conversation-store.ts`:
- Around line 44-56: Update saveConversation so trimming at MAX_STORED_MESSAGES
does not begin with an orphaned tool message: after calculating the slice
boundary, advance it past any leading messages whose role is "tool", then store
the remaining messages. Preserve the existing message limit and timestamp
behavior while ensuring each retained tool message follows its associated
assistant tool-calls message.
In `@apps/tradinggoose/lib/copilot/local-runtime/dispatch.ts`:
- Around line 44-51: Update handleContextUsage and its call site in
dispatchLocalCopilotRequest to accept the authenticated userId, then verify the
loaded conversation’s userId matches before returning context metadata. For
missing or unauthorized conversations, preserve the existing zero-value response
and do not expose the conversation’s model, contextWindow, or token usage.
In `@apps/tradinggoose/lib/copilot/local-runtime/llm.ts`:
- Around line 141-159: Update the tool-call delta handling around the
seenToolCallIndexes guard to buffer argument fragments per index when no usable
id/name pair has started the call; after yielding tool_call_start, flush that
index’s buffered fragments in order, then emit subsequent arguments directly.
Preserve per-index ordering and avoid emitting tool_call_arguments before the
corresponding start event.
In `@apps/tradinggoose/lib/copilot/local-runtime/runtime.ts`:
- Around line 108-137: When an existing conversation receives a new user
message, update the flow around the pendingToolCalls reset to also remove each
superseded tool-call pointer using the same cleanup performed by
abortLocalCopilotTurn. Ensure cleanup completes before returning from this
request, while preserving the existing reset of pendingToolCalls and normal
handling for new conversations.
In `@apps/tradinggoose/providers/ai/nvidia/index.ts`:
- Around line 34-43: The NVIDIA credential resolution must honor rotating keys
before the fallback API key. In apps/tradinggoose/providers/ai/nvidia/index.ts
lines 34-43, update the key selection to use request.apiKey, then
pickRotatingKey(serviceConfig.rotationKeys), then serviceConfig.apiKey. In
apps/tradinggoose/app/api/providers/ai/nvidia/models/route.ts lines 12-18,
resolve pickRotatingKey(config.rotationKeys) before config.apiKey, perform the
empty-key check on that result, and use it in the Authorization header.
- Around line 247-364: Update the tool-call handling loop so each response
appends exactly one assistant message containing all entries from
toolCallsInResponse before appending any matching tool messages. Preserve each
tool call’s id, name, type, and original arguments, and keep the existing
per-tool execution and result messages in the same order.
In `@apps/tradinggoose/providers/ai/utils.ts`:
- Around line 86-90: Update updateNvidiaProviderModels to deduplicate the
incoming models list before passing it to updateNvidiaModels, ensuring duplicate
API IDs are removed before providers.nvidia.models is rebuilt from the shared
catalog.
In
`@apps/tradinggoose/widgets/widgets/copilot/components/user-input/components/model-selector.tsx`:
- Around line 76-109: Update the model-loading useEffect to run only on mount by
removing selectedModel from its dependency array and reading the current
selection through a ref. Keep the ref synchronized with selectedModel, and use
that ref when validating availability before calling setSelectedModel, so the
/api/copilot/models request occurs only once.
---
Nitpick comments:
In `@apps/tradinggoose/app/api/copilot/models/route.ts`:
- Around line 19-30: The hosted model groups are duplicated and silently omit
models without claude- or gpt- prefixes. In
apps/tradinggoose/lib/copilot/runtime-models.ts, add and export a shared
grouping helper based on an explicit provider field in each runtime model
definition, supporting future providers without prefix checks; update
apps/tradinggoose/app/api/copilot/models/route.ts lines 19-30 and
apps/tradinggoose/widgets/widgets/copilot/components/user-input/components/model-selector.tsx
lines 50-69 to derive their groups through that helper, with no direct change
needed elsewhere.
In `@apps/tradinggoose/app/api/copilot/proxy.ts`:
- Line 2: Update the proxy handler to remove the static
dispatchLocalCopilotRequest import and dynamically import that dispatcher at the
local-request call site, matching the existing handleLocalCopilotCompletion
pattern. Preserve the current request behavior while ensuring hosted-only
imports do not eagerly load the local runtime SDK graph.
In `@apps/tradinggoose/app/api/providers/ai/nvidia/models/route.ts`:
- Around line 20-26: Update the upstream fetch in the models route to use an
AbortController-based timeout and pass its signal in the request options. Ensure
the controller is scheduled to abort after the chosen timeout and that the timer
is cleaned up once the fetch completes, while preserving the existing headers
and revalidation behavior.
In `@apps/tradinggoose/lib/copilot/local-runtime/completion.ts`:
- Around line 45-66: Add debug-level logging to both credential-resolution catch
blocks in the local provider selection flow: the requested-provider branch and
the loop over group providers. Include the relevant provider (and model where
available) plus the caught error, while preserving the existing fallback
behavior.
In `@apps/tradinggoose/lib/copilot/local-runtime/model-catalog.ts`:
- Around line 67-71: Update listOpenRouterToolModels to derive its models
endpoint from the configured
LOCAL_COPILOT_OPENAI_COMPATIBLE_BASE_URLS.openrouter value in providers.ts,
appending the appropriate models path instead of hardcoding the public
OpenRouter URL. Preserve the existing request headers and timeout.
- Around line 135-154: Update the model catalog listing flow around
cachedListing, listOpenRouterToolModels, listOpenAiCompatibleModels, and
listOllamaModels so the eligible OpenRouter, NVIDIA, and Ollama requests start
concurrently instead of awaiting each push sequentially. Preserve provider key
checks and result ordering, then await the concurrent listing results before
returning the catalog.
In `@apps/tradinggoose/lib/copilot/local-runtime/runtime.test.ts`:
- Around line 230-248: Extend the runtime test suite with coverage for
abortLocalCopilotTurn and superseded tool calls. Add a test that starts a turn
with a pending tool call, sends a new user message, then resumes the original
call and asserts null is returned and streamLlm is not invoked a third time;
also test the expected abortLocalCopilotTurn behavior using the existing runtime
helpers.
In `@apps/tradinggoose/lib/system-services/catalog.ts`:
- Around line 222-231: Add an environment-variable mapping to the NVIDIA
`baseUrl` setting in `settingFields`, matching the existing Ollama and Copilot
base URL configuration pattern, so compose deployments can override
`NVIDIA_API_BASE_URL_DEFAULT` without using the admin UI.
In `@apps/tradinggoose/lib/system-services/runtime.ts`:
- Around line 141-145: Update resolveNvidiaServiceConfig to obtain its API-key
configuration through the existing readRotatingApiKeyConfig helper instead of
separately calling asString and readRotationKeys. Preserve the baseUrl handling,
including the NVIDIA_API_BASE_URL_DEFAULT fallback.
In `@apps/tradinggoose/providers/ai/utils-server.test.ts`:
- Around line 124-133: Rename the test case around getApiKey from “skips empty
slots so a partially filled rotation still works” to describe that it resolves
using the only configured rotation slot. Keep the test setup and assertions
unchanged.
In `@apps/tradinggoose/providers/ai/utils-server.ts`:
- Around line 31-44: Update the provider switch to add an explicit `case
'nvidia'` that returns NVIDIA credentials, and replace the `default` branch with
an error for unsupported providers. Preserve the existing resolution behavior
for all named providers while ensuring unknown values cannot receive NVIDIA
credentials.
🪄 Autofix
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: CHILL
Plan: Pro Plus
Run ID: f0fc883d-b791-4c8f-8eea-a4496aebf7f5
📒 Files selected for processing (50)
.github/workflows/images.ymlapps/tradinggoose/.env.example.dockerapps/tradinggoose/app/api/copilot/chat/abort/route.test.tsapps/tradinggoose/app/api/copilot/chat/abort/route.tsapps/tradinggoose/app/api/copilot/chat/route.tsapps/tradinggoose/app/api/copilot/models/route.tsapps/tradinggoose/app/api/copilot/proxy.tsapps/tradinggoose/app/api/copilot/tools/mark-complete/route.test.tsapps/tradinggoose/app/api/copilot/tools/mark-complete/route.tsapps/tradinggoose/app/api/copilot/usage/route.tsapps/tradinggoose/app/api/providers/ai/nvidia/models/route.tsapps/tradinggoose/components/icons/provider-icons.tsxapps/tradinggoose/lib/admin/system-services.test.tsapps/tradinggoose/lib/admin/system-services.tsapps/tradinggoose/lib/copilot/agent/constants.tsapps/tradinggoose/lib/copilot/local-runtime/completion.test.tsapps/tradinggoose/lib/copilot/local-runtime/completion.tsapps/tradinggoose/lib/copilot/local-runtime/conversation-store.tsapps/tradinggoose/lib/copilot/local-runtime/dispatch.tsapps/tradinggoose/lib/copilot/local-runtime/events.tsapps/tradinggoose/lib/copilot/local-runtime/llm.test.tsapps/tradinggoose/lib/copilot/local-runtime/llm.tsapps/tradinggoose/lib/copilot/local-runtime/model-catalog.tsapps/tradinggoose/lib/copilot/local-runtime/prompt.tsapps/tradinggoose/lib/copilot/local-runtime/providers.tsapps/tradinggoose/lib/copilot/local-runtime/runtime.test.tsapps/tradinggoose/lib/copilot/local-runtime/runtime.tsapps/tradinggoose/lib/copilot/local-runtime/types.tsapps/tradinggoose/lib/copilot/runtime-models.tsapps/tradinggoose/lib/copilot/runtime-provider.server.tsapps/tradinggoose/lib/copilot/runtime-provider.tsapps/tradinggoose/lib/system-services/catalog.tsapps/tradinggoose/lib/system-services/runtime.tsapps/tradinggoose/lib/system-services/service.test.tsapps/tradinggoose/lib/system-services/service.tsapps/tradinggoose/lib/utils-server.tsapps/tradinggoose/providers/ai/index.tsapps/tradinggoose/providers/ai/models.tsapps/tradinggoose/providers/ai/nvidia/constants.tsapps/tradinggoose/providers/ai/nvidia/index.tsapps/tradinggoose/providers/ai/types.tsapps/tradinggoose/providers/ai/utils-server.test.tsapps/tradinggoose/providers/ai/utils-server.tsapps/tradinggoose/providers/ai/utils.tsapps/tradinggoose/stores/providers/store.tsapps/tradinggoose/stores/providers/types.tsapps/tradinggoose/widgets/widgets/copilot/components/user-input/components/model-selector.tsxdocker-compose.local.ymldocker-compose.ollama.ymldocker-compose.prod.yml
Comment on lines
+24
to
+29
| # Only the app image is mirrored to Docker Hub; migrations carries the | ||
| # database schema and realtime is only ever pulled alongside it, so both | ||
| # stay on GHCR where private packages are free. | ||
| - dockerfile: ./docker/app.Dockerfile | ||
| repo: tradinggoose | ||
| dockerhub: true |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 3 'dockerhub:|IMAGE_REGISTRY' \
.github/workflows/images.yml docker-compose.prod.ymlRepository: TradingGoose/TradingGoose-Studio
Length of output: 2003
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- workflow ---'
cat -n .github/workflows/images.yml
printf '%s\n' '--- production compose ---'
cat -n docker-compose.prod.yml
printf '%s\n' '--- registry references ---'
rg -n -C 3 'IMAGE_REGISTRY|docker\.io|ghcr\.io|tradinggoose|realtime|migrations' \
--glob '!node_modules' --glob '!dist' --glob '!build' .Repository: TradingGoose/TradingGoose-Studio
Length of output: 50389
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- workflow ---'
sed -n '1,140p' .github/workflows/images.yml
printf '%s\n' '--- production compose image declarations ---'
sed -n '1,90p' docker-compose.prod.yml
printf '%s\n' '--- registry references in configuration and documentation ---'
rg -n -C 3 'IMAGE_REGISTRY|docker\.io|ghcr\.io|dockerhub' \
-g '*.yml' -g '*.yaml' -g '*.md' -g '*.env*' -g 'Dockerfile*' \
-g '!node_modules' -g '!dist' -g '!build' .Repository: TradingGoose/TradingGoose-Studio
Length of output: 13500
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- production compose references ---'
rg -n -C 4 'docker-compose\.prod\.yml|IMAGE_REGISTRY|IMAGE_TAG' \
-g '*.md' -g '*.yml' -g '*.yaml' -g '*.env*' -g '*.sh' -g '*.json' \
-g '!node_modules' -g '!dist' -g '!build' .
printf '%s\n' '--- Docker Hub and image publication references ---'
rg -n -C 4 'DOCKERHUB|Docker Hub|dockerhub|docker\.io' \
.github README.md docs helm docker apps 2>/dev/null || trueRepository: TradingGoose/TradingGoose-Studio
Length of output: 17056
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
workflow = Path(".github/workflows/images.yml").read_text()
compose = Path("docker-compose.prod.yml").read_text()
matrix = {}
for repo, block in re.findall(
r"- dockerfile: [^\n]+\n\s+repo: ([^\n]+)(.*?)(?=\n\s+- dockerfile:|\Z)",
workflow,
re.S,
):
matrix[repo.strip()] = "dockerhub: true" in block
compose_repos = re.findall(
r"image: \$\{IMAGE_REGISTRY:-ghcr\.io/xiujie2\}/([^:]+):",
compose,
)
print("matrix Docker Hub publication:", matrix)
print("Compose project images:", compose_repos)
for registry in ("ghcr.io/xiujie2", "docker.io/example"):
available = {
repo: registry == "ghcr.io/xiujie2" or matrix.get(repo, False)
for repo in compose_repos
}
print(f"{registry}: {available}")
PYRepository: TradingGoose/TradingGoose-Studio
Length of output: 491
Support Docker Hub as a production image registry.
If IMAGE_REGISTRY=docker.io/<namespace> is a supported Compose configuration, publish realtime and migrations there too. Compose currently cannot pull those images from Docker Hub.
🤖 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 @.github/workflows/images.yml around lines 24 - 29, Update the image
publishing matrix in the workflow so the realtime and migrations Dockerfiles are
also published to Docker Hub alongside the app image. Preserve their existing
GHCR publishing configuration and ensure the resulting tags match the
docker.io/<namespace> registry expected by the Compose configuration.
| # Host interface the Postgres port is published on. Loopback-only by default; | ||
| # set 0.0.0.0 only when another machine must reach the database directly. | ||
| POSTGRES_BIND_HOST=127.0.0.1 | ||
| POSTGRES_PORT=5432 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Remove the duplicate POSTGRES_PORT declaration.
dotenv-linter reports that this key is duplicated in the same file. Keep one declaration so the host port used by Compose remains unambiguous.
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 35-35: [DuplicatedKey] The POSTGRES_PORT key is duplicated
(DuplicatedKey)
🤖 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 `@apps/tradinggoose/.env.example.docker` at line 35, Remove the duplicate
POSTGRES_PORT declaration from the environment configuration, retaining a single
POSTGRES_PORT=5432 entry so Compose uses an unambiguous host port.
Source: Linters/SAST tools
Comment on lines
+68
to
+75
| # Copilot runtime. `local` (the default) runs the Copilot agent inside this | ||
| # deployment using whichever model provider keys are configured in | ||
| # Admin > Services — no Copilot API key and no TradingGoose account needed. | ||
| # Set it to `hosted` to call the managed service instead, which requires a key | ||
| # generated at tradinggoose.ai > Settings > Copilot. | ||
| # COPILOT_RUNTIME_MODE=local | ||
| # COPILOT_API_KEY= | ||
| # COPILOT_API_URL=https://copilot.tradinggoose.ai |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
for compose_file in \
docker-compose.local.yml \
docker-compose.ollama.yml \
docker-compose.prod.yml
do
if ! rg -n 'COPILOT_RUNTIME_MODE' "$compose_file"; then
echo "Missing COPILOT_RUNTIME_MODE forwarding in $compose_file" >&2
exit 1
fi
doneRepository: TradingGoose/TradingGoose-Studio
Length of output: 238
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- compose files ---'
for compose_file in docker-compose.local.yml docker-compose.ollama.yml docker-compose.prod.yml; do
printf '\n[%s]\n' "$compose_file"
if [ -f "$compose_file" ]; then
rg -n -C 5 'environment:|COPILOT_RUNTIME_MODE|tradinggoose:' "$compose_file" || true
else
printf 'MISSING\n'
fi
done
printf '\n--- runtime-mode references ---\n'
rg -n -C 4 'COPILOT_RUNTIME_MODE|copilot.*runtime|runtime.*copilot' . \
-g '!node_modules' -g '!dist' -g '!build' || true
printf '\n--- env example context ---\n'
sed -n '55,82p' apps/tradinggoose/.env.example.dockerRepository: TradingGoose/TradingGoose-Studio
Length of output: 50389
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- exact runtime-mode references ---'
rg -n -F 'COPILOT_RUNTIME_MODE' . \
-g '!changelog/**' -g '!node_modules' -g '!dist' -g '!build' || true
printf '\n%s\n' '--- Copilot environment references ---'
rg -n -e 'COPILOT_(API_KEY|API_URL|RUNTIME_MODE)' apps packages docker-compose*.yml .env* \
-g '!node_modules' -g '!dist' -g '!build' || true
printf '\n%s\n' '--- tradinggoose environment blocks ---'
for compose_file in docker-compose.local.yml docker-compose.ollama.yml docker-compose.prod.yml; do
printf '\n[%s]\n' "$compose_file"
awk '
/^ tradinggoose:/ { in_service=1 }
in_service && /^ [A-Za-z0-9_-]+:/ && $0 !~ /^ tradinggoose:/ { in_service=0 }
in_service { print NR ":" $0 }
' "$compose_file"
doneRepository: TradingGoose/TradingGoose-Studio
Length of output: 14583
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- Copilot mode constants and resolver ---'
cat -n apps/tradinggoose/lib/copilot/agent/constants.ts
rg -n -C 8 'envVar|process\.env|defaultValue|resolve.*service|service.*value' \
apps/tradinggoose/lib/system-services \
apps/tradinggoose/lib/admin \
apps/tradinggoose/app/api/admin/services \
-g '*.ts' -g '*.tsx' | head -n 240
printf '\n%s\n' '--- catalog entries ---'
sed -n '345,400p' apps/tradinggoose/lib/system-services/catalog.ts
printf '\n%s\n' '--- service configuration call sites ---'
rg -n -C 6 'COPILOT_RUNTIME_MODE|resolve.*Copilot|copilot.*config|get.*Copilot.*config|system.*service' \
apps/tradinggoose/lib/copilot \
apps/tradinggoose/app/api/copilot \
apps/tradinggoose/lib/system-services \
-g '*.ts' -g '*.tsx' | head -n 300Repository: TradingGoose/TradingGoose-Studio
Length of output: 46939
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- setting resolution implementation ---'
sed -n '135,185p' apps/tradinggoose/lib/system-services/service.ts
sed -n '1,90p' apps/tradinggoose/lib/system-services/runtime.ts
rg -n -C 5 'getEnv|envVar' apps/tradinggoose/lib/env.ts apps/tradinggoose/lib/system-services/service.ts apps/tradinggoose/lib/system-services/service.test.ts
printf '\n%s\n' '--- resolver behavior tests ---'
rg -n -C 12 'environment|envVar|defaultValue|stored value|setting' \
apps/tradinggoose/lib/system-services/service.test.ts | head -n 260
printf '\n%s\n' '--- read-only Compose forwarding check ---'
python3 - <<'PY'
from pathlib import Path
import re
files = [
Path("docker-compose.local.yml"),
Path("docker-compose.ollama.yml"),
Path("docker-compose.prod.yml"),
]
for path in files:
text = path.read_text()
match = re.search(r"(?ms)^ tradinggoose:\n.*?(?=^ [A-Za-z0-9_-]+:|\Z)", text)
block = match.group(0) if match else ""
print(f"{path}:")
print(" service_found:", bool(match))
print(" runtime_forwarded:", bool(re.search(r"(?m)^\s*-\s*COPILOT_RUNTIME_MODE=", block)))
print(" api_key_forwarded:", bool(re.search(r"(?m)^\s*-\s*COPILOT_API_KEY=", block)))
print(" api_url_forwarded:", bool(re.search(r"(?m)^\s*-\s*COPILOT_API_URL=", block)))
PYRepository: TradingGoose/TradingGoose-Studio
Length of output: 22502
Forward COPILOT_RUNTIME_MODE into the Compose service.
When no non-blank database setting overrides it, add COPILOT_RUNTIME_MODE=${COPILOT_RUNTIME_MODE:-local} to the tradinggoose.environment block in docker-compose.local.yml, docker-compose.ollama.yml, and docker-compose.prod.yml. Without this entry, the container cannot read the documented environment setting and falls back to local.
🤖 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 `@apps/tradinggoose/.env.example.docker` around lines 68 - 75, Update the
tradinggoose.environment blocks in docker-compose.local.yml,
docker-compose.ollama.yml, and docker-compose.prod.yml to forward
COPILOT_RUNTIME_MODE using the default local value when the host variable is
blank or unset, unless an existing non-blank database setting already overrides
it.
Comment on lines
+882
to
+892
| // The local runtime keeps turn state in a cache that can expire or be lost | ||
| // on a restart, so it falls back to this. The hosted service owns its own | ||
| // conversation store and ignores the field. | ||
| ...(conversationHistory.length > 0 | ||
| ? { | ||
| history: conversationHistory.map((entry) => ({ | ||
| role: entry.role, | ||
| content: entry.content, | ||
| })), | ||
| } | ||
| : {}), |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Send history only when the local runtime needs it.
history is attached to every request, including hosted-mode requests. The comment states that the hosted service ignores the field. Two consequences follow:
- The full persisted conversation content is transmitted to the hosted third-party service on every turn, even though that service maintains its own conversation store keyed by
conversationId. This widens what leaves the deployment without a functional need. - The payload is unbounded. It grows linearly with session length and is re-sent on each turn.
Gate the field on local mode, and cap the number of forwarded messages.
🔒️ Proposed fix
+ const localMode = await isLocalCopilotMode()
+ const MAX_FORWARDED_HISTORY = 50
+
const requestPayload = {
@@
toolManifest: await getCopilotRuntimeToolManifest(),
// The local runtime keeps turn state in a cache that can expire or be lost
- // on a restart, so it falls back to this. The hosted service owns its own
- // conversation store and ignores the field.
- ...(conversationHistory.length > 0
+ // on a restart, so it falls back to this. The hosted service owns its own
+ // conversation store, so the field is omitted there.
+ ...(localMode && conversationHistory.length > 0
? {
- history: conversationHistory.map((entry) => ({
- role: entry.role,
- content: entry.content,
- })),
+ history: conversationHistory
+ .slice(-MAX_FORWARDED_HISTORY)
+ .map((entry) => ({ role: entry.role, content: entry.content })),
}
: {}),isLocalCopilotMode is exported from apps/tradinggoose/app/api/copilot/proxy.ts, which this file already imports from.
📝 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
| // The local runtime keeps turn state in a cache that can expire or be lost | |
| // on a restart, so it falls back to this. The hosted service owns its own | |
| // conversation store and ignores the field. | |
| ...(conversationHistory.length > 0 | |
| ? { | |
| history: conversationHistory.map((entry) => ({ | |
| role: entry.role, | |
| content: entry.content, | |
| })), | |
| } | |
| : {}), | |
| // The local runtime keeps turn state in a cache that can expire or be lost | |
| // on a restart, so it falls back to this. The hosted service owns its own | |
| // conversation store, so the field is omitted there. | |
| ...(localMode && conversationHistory.length > 0 | |
| ? { | |
| history: conversationHistory | |
| .slice(-MAX_FORWARDED_HISTORY) | |
| .map((entry) => ({ role: entry.role, content: entry.content })), | |
| } | |
| : {}), |
🤖 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 `@apps/tradinggoose/app/api/copilot/chat/route.ts` around lines 882 - 892,
Update the request payload construction around conversationHistory to include
history only when isLocalCopilotMode is true, while preserving the existing
non-empty check. Limit the forwarded conversationHistory entries to a bounded
recent-message subset before mapping them to role and content, and reuse the
existing isLocalCopilotMode import from proxy.ts.
Comment on lines
+133
to
+143
| if (!wantsStream) { | ||
| let content = '' | ||
| for await (const delta of deltas) { | ||
| if (delta.type === 'text') content += delta.delta | ||
| } | ||
|
|
||
| return new Response( | ||
| JSON.stringify({ choices: [{ message: { role: 'assistant', content } }] }), | ||
| { status: 200, headers: { 'Content-Type': 'application/json' } } | ||
| ) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle stream errors on the non-streaming path.
The streaming path wraps the delta iteration in try/catch at lines 147-157. This path does not. If streamLlm throws — an authentication failure, a network error, or an aborted signal — the returned promise rejects instead of resolving to a Response. proxyCopilotCompletionRequest in apps/tradinggoose/app/api/copilot/proxy.ts returns that value directly to callers that expect a Response, so the rejection propagates as an unhandled error.
🐛 Proposed fix
if (!wantsStream) {
let content = ''
- for await (const delta of deltas) {
- if (delta.type === 'text') content += delta.delta
+ try {
+ for await (const delta of deltas) {
+ if (delta.type === 'text') content += delta.delta
+ }
+ } catch (error) {
+ logger.error('Local completion failed', error)
+ return jsonError(error instanceof Error ? error.message : 'Completion failed', 502)
}📝 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
| if (!wantsStream) { | |
| let content = '' | |
| for await (const delta of deltas) { | |
| if (delta.type === 'text') content += delta.delta | |
| } | |
| return new Response( | |
| JSON.stringify({ choices: [{ message: { role: 'assistant', content } }] }), | |
| { status: 200, headers: { 'Content-Type': 'application/json' } } | |
| ) | |
| } | |
| if (!wantsStream) { | |
| let content = '' | |
| try { | |
| for await (const delta of deltas) { | |
| if (delta.type === 'text') content += delta.delta | |
| } | |
| } catch (error) { | |
| logger.error('Local completion failed', error) | |
| return jsonError(error instanceof Error ? error.message : 'Completion failed', 502) | |
| } | |
| return new Response( | |
| JSON.stringify({ choices: [{ message: { role: 'assistant', content } }] }), | |
| { status: 200, headers: { 'Content-Type': 'application/json' } } | |
| ) | |
| } |
🤖 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 `@apps/tradinggoose/lib/copilot/local-runtime/completion.ts` around lines 133 -
143, Wrap the non-streaming delta iteration in the same try/catch error-handling
pattern used by the streaming path, covering failures from streamLlm and aborted
requests while collecting content. Ensure the catch branch returns the
established error Response shape so proxyCopilotCompletionRequest always
receives a Response instead of a rejected promise.
Comment on lines
+108
to
+137
| const existing = request.conversationId ? await loadConversation(request.conversationId) : null | ||
| const conversation: LocalCopilotConversation = existing | ||
| ? { | ||
| ...existing, | ||
| // A new user message supersedes whatever the previous turn was waiting on. | ||
| pendingToolCalls: [], | ||
| model: request.model, | ||
| provider, | ||
| contexts: request.context ?? existing.contexts, | ||
| } | ||
| : { | ||
| id: request.conversationId || crypto.randomUUID(), | ||
| userId: request.userId, | ||
| model: request.model, | ||
| provider, | ||
| userName: request.userName, | ||
| contexts: request.context ?? [], | ||
| messages: rehydrateHistory(request.history), | ||
| pendingToolCalls: [], | ||
| updatedAt: Date.now(), | ||
| } | ||
|
|
||
| if (existing && existing.userId !== request.userId) { | ||
| return sseResponse(singleEvent(errorEvent('Conversation not found.'))) | ||
| } | ||
|
|
||
| conversation.messages.push({ role: 'user', content: request.message }) | ||
|
|
||
| return sseResponse(streamModelTurn(conversation, { emitStart: true, signal })) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Delete the tool-call pointers that the new user message supersedes.
Line 113 clears pendingToolCalls, but the Redis pointers written by indexPendingToolCalls stay for the full 24-hour TTL. findConversationIdForToolCall still resolves an abandoned call to this conversation. A late /api/tools/mark-complete then appends a tool message and, because pendingToolCalls is now empty, resumeLocalCopilotTurn starts an extra model turn that no user asked for and bills a completion. abortLocalCopilotTurn at lines 185-192 performs the cleanup that is missing here.
🐛 Proposed fix to forget superseded tool calls
const existing = request.conversationId ? await loadConversation(request.conversationId) : null
+
+ if (existing && existing.userId === request.userId && existing.pendingToolCalls.length > 0) {
+ await Promise.all(existing.pendingToolCalls.map((toolCall) => forgetToolCall(toolCall.id)))
+ }
+
const conversation: LocalCopilotConversation = existing📝 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
| const existing = request.conversationId ? await loadConversation(request.conversationId) : null | |
| const conversation: LocalCopilotConversation = existing | |
| ? { | |
| ...existing, | |
| // A new user message supersedes whatever the previous turn was waiting on. | |
| pendingToolCalls: [], | |
| model: request.model, | |
| provider, | |
| contexts: request.context ?? existing.contexts, | |
| } | |
| : { | |
| id: request.conversationId || crypto.randomUUID(), | |
| userId: request.userId, | |
| model: request.model, | |
| provider, | |
| userName: request.userName, | |
| contexts: request.context ?? [], | |
| messages: rehydrateHistory(request.history), | |
| pendingToolCalls: [], | |
| updatedAt: Date.now(), | |
| } | |
| if (existing && existing.userId !== request.userId) { | |
| return sseResponse(singleEvent(errorEvent('Conversation not found.'))) | |
| } | |
| conversation.messages.push({ role: 'user', content: request.message }) | |
| return sseResponse(streamModelTurn(conversation, { emitStart: true, signal })) | |
| } | |
| const existing = request.conversationId ? await loadConversation(request.conversationId) : null | |
| if (existing && existing.userId === request.userId && existing.pendingToolCalls.length > 0) { | |
| await Promise.all(existing.pendingToolCalls.map((toolCall) => forgetToolCall(toolCall.id))) | |
| } | |
| const conversation: LocalCopilotConversation = existing | |
| ? { | |
| ...existing, | |
| // A new user message supersedes whatever the previous turn was waiting on. | |
| pendingToolCalls: [], | |
| model: request.model, | |
| provider, | |
| contexts: request.context ?? existing.contexts, | |
| } | |
| : { | |
| id: request.conversationId || crypto.randomUUID(), | |
| userId: request.userId, | |
| model: request.model, | |
| provider, | |
| userName: request.userName, | |
| contexts: request.context ?? [], | |
| messages: rehydrateHistory(request.history), | |
| pendingToolCalls: [], | |
| updatedAt: Date.now(), | |
| } | |
| if (existing && existing.userId !== request.userId) { | |
| return sseResponse(singleEvent(errorEvent('Conversation not found.'))) | |
| } | |
| conversation.messages.push({ role: 'user', content: request.message }) | |
| return sseResponse(streamModelTurn(conversation, { emitStart: true, signal })) | |
| } |
🤖 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 `@apps/tradinggoose/lib/copilot/local-runtime/runtime.ts` around lines 108 -
137, When an existing conversation receives a new user message, update the flow
around the pendingToolCalls reset to also remove each superseded tool-call
pointer using the same cleanup performed by abortLocalCopilotTurn. Ensure
cleanup completes before returning from this request, while preserving the
existing reset of pendingToolCalls and normal handling for new conversations.
Comment on lines
+34
to
+43
| const serviceConfig = await resolveNvidiaServiceConfig() | ||
| const apiKey = request.apiKey || serviceConfig.apiKey | ||
| if (!apiKey) { | ||
| throw new Error('API key is required for NVIDIA NIM') | ||
| } | ||
|
|
||
| const client = new OpenAI({ | ||
| apiKey, | ||
| baseURL: serviceConfig.baseUrl.replace(/\/$/, ''), | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
NVIDIA credential resolution ignores the rotation slots. Both sites read only the single apiKey field from resolveNvidiaServiceConfig(). The catalog states that rotationKey1-rotationKey3 take precedence over the single API key, and getApiKey in apps/tradinggoose/providers/ai/utils-server.ts applies pickRotatingKey(rotationKeys) ?? apiKey. A deployment that fills only the rotation slots therefore fails on requests and returns no models.
apps/tradinggoose/providers/ai/nvidia/index.ts#L34-L43: resolve the key asrequest.apiKey || pickRotatingKey(serviceConfig.rotationKeys) || serviceConfig.apiKey.apps/tradinggoose/app/api/providers/ai/nvidia/models/route.ts#L12-L18: resolve the key aspickRotatingKey(config.rotationKeys) ?? config.apiKeybefore the empty-key check, and use it in theAuthorizationheader.
📍 Affects 2 files
apps/tradinggoose/providers/ai/nvidia/index.ts#L34-L43(this comment)apps/tradinggoose/app/api/providers/ai/nvidia/models/route.ts#L12-L18
🤖 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 `@apps/tradinggoose/providers/ai/nvidia/index.ts` around lines 34 - 43, The
NVIDIA credential resolution must honor rotating keys before the fallback API
key. In apps/tradinggoose/providers/ai/nvidia/index.ts lines 34-43, update the
key selection to use request.apiKey, then
pickRotatingKey(serviceConfig.rotationKeys), then serviceConfig.apiKey. In
apps/tradinggoose/app/api/providers/ai/nvidia/models/route.ts lines 12-18,
resolve pickRotatingKey(config.rotationKeys) before config.apiKey, perform the
empty-key check on that result, and use it in the Authorization header.
Comment on lines
+247
to
+364
| while (iterationCount < MAX_ITERATIONS) { | ||
| const toolCallsInResponse = currentResponse.choices[0]?.message?.tool_calls | ||
| if (!toolCallsInResponse || toolCallsInResponse.length === 0) { | ||
| break | ||
| } | ||
|
|
||
| const toolsStartTime = Date.now() | ||
| for (const toolCall of toolCallsInResponse) { | ||
| try { | ||
| const toolName = toolCall.function.name | ||
| const toolArgs = JSON.parse(toolCall.function.arguments) | ||
| const tool = request.tools?.find((t) => t.id === toolName) | ||
| if (!tool) continue | ||
|
|
||
| const toolCallStartTime = Date.now() | ||
| const { toolParams, executionParams } = prepareToolExecution(tool, toolArgs, request) | ||
| const result = await executeTool(toolName, executionParams) | ||
| const toolCallEndTime = Date.now() | ||
| const toolCallDuration = toolCallEndTime - toolCallStartTime | ||
|
|
||
| timeSegments.push({ | ||
| type: 'tool', | ||
| name: toolName, | ||
| startTime: toolCallStartTime, | ||
| endTime: toolCallEndTime, | ||
| duration: toolCallDuration, | ||
| }) | ||
|
|
||
| let resultContent: any | ||
| if (result.success) { | ||
| toolResults.push(result.output) | ||
| resultContent = result.output | ||
| } else { | ||
| resultContent = { | ||
| error: true, | ||
| message: result.error || 'Tool execution failed', | ||
| tool: toolName, | ||
| } | ||
| } | ||
|
|
||
| toolCalls.push({ | ||
| name: toolName, | ||
| arguments: toolParams, | ||
| startTime: new Date(toolCallStartTime).toISOString(), | ||
| endTime: new Date(toolCallEndTime).toISOString(), | ||
| duration: toolCallDuration, | ||
| result: resultContent, | ||
| success: result.success, | ||
| }) | ||
|
|
||
| currentMessages.push({ | ||
| role: 'assistant', | ||
| content: null, | ||
| tool_calls: [ | ||
| { | ||
| id: toolCall.id, | ||
| type: 'function', | ||
| function: { | ||
| name: toolName, | ||
| arguments: toolCall.function.arguments, | ||
| }, | ||
| }, | ||
| ], | ||
| }) | ||
|
|
||
| currentMessages.push({ | ||
| role: 'tool', | ||
| tool_call_id: toolCall.id, | ||
| content: JSON.stringify(resultContent), | ||
| }) | ||
| } catch (error) { | ||
| logger.error('Error processing tool call (NVIDIA):', { | ||
| error: error instanceof Error ? error.message : String(error), | ||
| toolName: toolCall?.function?.name, | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| const thisToolsTime = Date.now() - toolsStartTime | ||
| toolsTime += thisToolsTime | ||
|
|
||
| const nextPayload: any = { | ||
| ...payload, | ||
| messages: currentMessages, | ||
| } | ||
|
|
||
| if (typeof originalToolChoice === 'object' && hasUsedForcedTool && forcedTools.length > 0) { | ||
| const remainingTools = forcedTools.filter((tool) => !usedForcedTools.includes(tool)) | ||
| if (remainingTools.length > 0) { | ||
| nextPayload.tool_choice = { type: 'function', function: { name: remainingTools[0] } } | ||
| } else { | ||
| nextPayload.tool_choice = 'auto' | ||
| } | ||
| } | ||
|
|
||
| const nextModelStartTime = Date.now() | ||
| currentResponse = await client.chat.completions.create(nextPayload) | ||
| checkForForcedToolUsage(currentResponse, nextPayload.tool_choice) | ||
| const nextModelEndTime = Date.now() | ||
| const thisModelTime = nextModelEndTime - nextModelStartTime | ||
| timeSegments.push({ | ||
| type: 'model', | ||
| name: `Model response (iteration ${iterationCount + 1})`, | ||
| startTime: nextModelStartTime, | ||
| endTime: nextModelEndTime, | ||
| duration: thisModelTime, | ||
| }) | ||
| modelTime += thisModelTime | ||
| if (currentResponse.choices[0]?.message?.content) { | ||
| content = currentResponse.choices[0].message.content | ||
| } | ||
| if (currentResponse.usage) { | ||
| tokens.prompt += currentResponse.usage.prompt_tokens || 0 | ||
| tokens.completion += currentResponse.usage.completion_tokens || 0 | ||
| tokens.total += currentResponse.usage.total_tokens || 0 | ||
| } | ||
| iterationCount++ | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Compare the tool-continuation message construction across OpenAI-compatible providers.
fd -t f 'index.ts' -p apps/tradinggoose/providers/ai | xargs rg -nP -C6 "role: 'tool'"Repository: TradingGoose/TradingGoose-Studio
Length of output: 12833
🏁 Script executed:
#!/bin/bash
python3 - <<'PY'
from pathlib import Path
import re
root = Path("apps/tradinggoose/providers/ai")
providers = ["nvidia", "openai", "deepseek", "vllm", "groq", "azure-openai"]
for name in providers:
path = root / name / "index.ts"
text = path.read_text()
print(f"\n{name}:")
for match in re.finditer(r"currentMessages\.push\(\{", text):
block = text[match.start():match.start() + 900]
if "role: 'assistant'" in block or "role: 'tool'" in block:
roles = re.findall(r"role:\s*'([^']+)'", block)
print(" ", roles[:2], "tool_calls:", "tool_calls:" in block,
"tool_call_id:", "tool_call_id:" in block)
PY
printf '\nRelevant OpenAI provider section:\n'
sed -n '340,420p' apps/tradinggoose/providers/ai/openai/index.ts
printf '\nRelevant NVIDIA provider section:\n'
sed -n '265,330p' apps/tradinggoose/providers/ai/nvidia/index.tsRepository: TradingGoose/TradingGoose-Studio
Length of output: 5883
🌐 Web query:
OpenAI Chat Completions API tool_calls multiple tool calls assistant message tool messages ordering
💡 Result:
When using the OpenAI Chat Completions API with tool calls, strict message ordering requirements must be followed to maintain conversation integrity and avoid API errors [1][2]. Core Ordering Rules: 1. Assistant Message First: An assistant message containing tool_calls must be immediately followed by the corresponding tool messages [1][2]. 2. No Intervening Messages: You cannot insert other message types (such as system, user, or another assistant message) between the assistant message that generated the tool calls and the required tool output messages [1][2]. 3. Matching IDs: Each tool message must have a tool_call_id that exactly matches one of the id values provided in the preceding assistant message's tool_calls array [3][1][4]. Handling Multiple Tool Calls: When the model performs parallel tool calls (returning multiple calls in a single assistant message), you must provide a tool message for every single tool_call_id returned by the model [1][2]. Regarding the sequencing of these responses, empirical evidence from developers indicates that each tool message should be paired with the assistant message that requested it [5]. While the API expects all tool outputs to follow the assistant message that called them, if multiple parallel calls were made, it is most reliable to structure the conversation flow as follows [3][1]: - Assistant Message (containing multiple tool_calls) - Tool Message (for tool_call_id_1) - Tool Message (for tool_call_id_2) -...and so on. If you encounter errors like "Invalid parameter: messages with role 'tool' must be a response to a preceding message with 'tool_calls'", ensure that no other messages have been injected into the history between the assistant's request and your tool responses [1][2]. If you have dependencies between functions, the model does not inherently enforce execution order for parallel calls; you should either execute them sequentially yourself or disable parallel execution by setting parallel_tool_calls: false in your API request [6][7].
Citations:
- 1: https://community.openai.com/t/how-to-create-an-assistant-message-after-a-function-call-in-the-new-tools-api/578020
- 2: Invalid parameter: messages with role 'tool' must be a response to a preceeding message with 'tool_calls'. openai/openai-agents-python#873
- 3: https://github.com/openai/openai-node/blob/master/examples/tool-calls-stream.ts
- 4: https://developers.openai.com/api/docs/assistants/tools/function-calling
- 5: https://community.openai.com/t/incorrect-documentation-on-the-sequencing-of-function-outputs-for-parallel-function-calling/990209
- 6: https://developers.openai.com/api/docs/guides/function-calling?api-mode=chat
- 7: https://community.openai.com/t/parallel-tool-calling-where-there-is-an-ordering-dependency/1086995/9
Group all tool calls under one assistant message
When a response contains multiple tool_calls, append one assistant message containing all calls, then append the matching tool messages. The current loop inserts another assistant message between tool messages, which strict OpenAI-compatible backends can reject.
🤖 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 `@apps/tradinggoose/providers/ai/nvidia/index.ts` around lines 247 - 364,
Update the tool-call handling loop so each response appends exactly one
assistant message containing all entries from toolCallsInResponse before
appending any matching tool messages. Preserve each tool call’s id, name, type,
and original arguments, and keep the existing per-tool execution and result
messages in the same order.
Comment on lines
+86
to
+90
| export async function updateNvidiaProviderModels(models: string[]): Promise<void> { | ||
| const { updateNvidiaModels } = await import('@/providers/ai/models') | ||
| updateNvidiaModels(models) | ||
| providers.nvidia.models = getProviderModelsFromDefinitions('nvidia') | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Deduplicate the shared NVIDIA model catalog.
Line 86 receives the raw API model list. The provider store deduplicates only its own state before it calls this function. If the endpoint returns a duplicate ID, providers.nvidia.models retains it and can expose duplicate runtime models.
Proposed fix
export async function updateNvidiaProviderModels(models: string[]): Promise<void> {
const { updateNvidiaModels } = await import('`@/providers/ai/models`')
- updateNvidiaModels(models)
+ updateNvidiaModels(Array.from(new Set(models)))
providers.nvidia.models = getProviderModelsFromDefinitions('nvidia')
}📝 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
| export async function updateNvidiaProviderModels(models: string[]): Promise<void> { | |
| const { updateNvidiaModels } = await import('@/providers/ai/models') | |
| updateNvidiaModels(models) | |
| providers.nvidia.models = getProviderModelsFromDefinitions('nvidia') | |
| } | |
| export async function updateNvidiaProviderModels(models: string[]): Promise<void> { | |
| const { updateNvidiaModels } = await import('@/providers/ai/models') | |
| updateNvidiaModels(Array.from(new Set(models))) | |
| providers.nvidia.models = getProviderModelsFromDefinitions('nvidia') | |
| } |
🤖 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 `@apps/tradinggoose/providers/ai/utils.ts` around lines 86 - 90, Update
updateNvidiaProviderModels to deduplicate the incoming models list before
passing it to updateNvidiaModels, ensuring duplicate API IDs are removed before
providers.nvidia.models is rebuilt from the shared catalog.
Comment on lines
+76
to
+109
| useEffect(() => { | ||
| let cancelled = false | ||
|
|
||
| const loadModels = async () => { | ||
| try { | ||
| const response = await fetch('/api/copilot/models') | ||
| if (!response.ok) return | ||
|
|
||
| const data = (await response.json()) as { | ||
| groups?: CopilotModelGroup[] | ||
| defaultModel?: string | null | ||
| } | ||
| const nextGroups = (data.groups ?? []).filter((group) => group.models.length > 0) | ||
| if (cancelled || nextGroups.length === 0) return | ||
|
|
||
| const model = COPILOT_RUNTIME_MODEL_OPTIONS.find((option) => option.value === selectedModel) | ||
| const collapsedModeLabel = model ? model.label : DEFAULT_COPILOT_RUNTIME_MODEL | ||
| setGroups(nextGroups) | ||
|
|
||
| // The stored selection can be a model this deployment does not serve — | ||
| // a hosted default on a local runtime, or a provider whose key was | ||
| // removed. Move it onto something that actually works. | ||
| const available = new Set(nextGroups.flatMap((group) => group.models)) | ||
| if (!available.has(selectedModel)) { | ||
| setSelectedModel(data.defaultModel || nextGroups[0].models[0]) | ||
| } | ||
| } catch { | ||
| // Keep the fallback groups; the picker stays usable. | ||
| } | ||
| } | ||
|
|
||
| void loadModels() | ||
| return () => { | ||
| cancelled = true | ||
| } | ||
| }, [selectedModel, setSelectedModel]) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Fetch the model list once on mount, not on every selection change.
selectedModel is both a dependency of this effect and a value the effect writes through setSelectedModel. Two consequences follow:
- Every time the user picks a model in the dropdown, the effect re-runs and issues a new
GET /api/copilot/models. In local mode that endpoint performs live provider listings, so each selection can cost seconds. - When the stored selection is unavailable,
setSelectedModelat line 98 changesselectedModel, which re-runs the effect and issues a second request before it settles.
Read the current selection through a ref so the fetch runs once on mount.
🐛 Proposed fix
-import { useEffect, useState } from 'react'
+import { useEffect, useRef, useState } from 'react'
@@
const [groups, setGroups] = useState<CopilotModelGroup[]>(FALLBACK_GROUPS)
+ const selectedModelRef = useRef(selectedModel)
+ selectedModelRef.current = selectedModel
useEffect(() => {
let cancelled = false
@@
const available = new Set(nextGroups.flatMap((group) => group.models))
- if (!available.has(selectedModel)) {
+ if (!available.has(selectedModelRef.current)) {
setSelectedModel(data.defaultModel || nextGroups[0].models[0])
}
} catch {
// Keep the fallback groups; the picker stays usable.
}
}
void loadModels()
return () => {
cancelled = true
}
- }, [selectedModel, setSelectedModel])
+ }, [setSelectedModel])📝 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
| useEffect(() => { | |
| let cancelled = false | |
| const loadModels = async () => { | |
| try { | |
| const response = await fetch('/api/copilot/models') | |
| if (!response.ok) return | |
| const data = (await response.json()) as { | |
| groups?: CopilotModelGroup[] | |
| defaultModel?: string | null | |
| } | |
| const nextGroups = (data.groups ?? []).filter((group) => group.models.length > 0) | |
| if (cancelled || nextGroups.length === 0) return | |
| const model = COPILOT_RUNTIME_MODEL_OPTIONS.find((option) => option.value === selectedModel) | |
| const collapsedModeLabel = model ? model.label : DEFAULT_COPILOT_RUNTIME_MODEL | |
| setGroups(nextGroups) | |
| // The stored selection can be a model this deployment does not serve — | |
| // a hosted default on a local runtime, or a provider whose key was | |
| // removed. Move it onto something that actually works. | |
| const available = new Set(nextGroups.flatMap((group) => group.models)) | |
| if (!available.has(selectedModel)) { | |
| setSelectedModel(data.defaultModel || nextGroups[0].models[0]) | |
| } | |
| } catch { | |
| // Keep the fallback groups; the picker stays usable. | |
| } | |
| } | |
| void loadModels() | |
| return () => { | |
| cancelled = true | |
| } | |
| }, [selectedModel, setSelectedModel]) | |
| const [groups, setGroups] = useState<CopilotModelGroup[]>(FALLBACK_GROUPS) | |
| const selectedModelRef = useRef(selectedModel) | |
| selectedModelRef.current = selectedModel | |
| useEffect(() => { | |
| let cancelled = false | |
| const loadModels = async () => { | |
| try { | |
| const response = await fetch('/api/copilot/models') | |
| if (!response.ok) return | |
| const data = (await response.json()) as { | |
| groups?: CopilotModelGroup[] | |
| defaultModel?: string | null | |
| } | |
| const nextGroups = (data.groups ?? []).filter((group) => group.models.length > 0) | |
| if (cancelled || nextGroups.length === 0) return | |
| setGroups(nextGroups) | |
| // The stored selection can be a model this deployment does not serve — | |
| // a hosted default on a local runtime, or a provider whose key was | |
| // removed. Move it onto something that actually works. | |
| const available = new Set(nextGroups.flatMap((group) => group.models)) | |
| if (!available.has(selectedModelRef.current)) { | |
| setSelectedModel(data.defaultModel || nextGroups[0].models[0]) | |
| } | |
| } catch { | |
| // Keep the fallback groups; the picker stays usable. | |
| } | |
| } | |
| void loadModels() | |
| return () => { | |
| cancelled = true | |
| } | |
| }, [setSelectedModel]) |
🤖 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
`@apps/tradinggoose/widgets/widgets/copilot/components/user-input/components/model-selector.tsx`
around lines 76 - 109, Update the model-loading useEffect to run only on mount
by removing selectedModel from its dependency array and reading the current
selection through a ref. Keep the ref synchronized with selectedModel, and use
that ref when validating availability before calling setSelectedModel, so the
/api/copilot/models request occurs only once.
Tools like list_workflows and create_workflow take workspaceId as a required argument, and none of the 113 registered tools can look one up. The browser knows the id and sends it to /api/copilot/chat, but the chat route never forwarded it and the local runtime's system prompt never mentioned it, so the model had no source for the value and guessed — typically reusing an id from an attached context, which comes back as "Access denied: You do not have permission to read this workflow" and reads like a permissions problem rather than a wrong id. Forwards the incoming workspaceId through to the runtime, stores it on the conversation so a tool-call resume keeps it, and states it in the system prompt. When a chat has no workspace the prompt now says so explicitly and tells the model to ask rather than invent one. Also tells the model that ids are scoped to one entity kind, so an access error on a borrowed id reads as the wrong id rather than as missing permission. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
fix(copilot): tell the local runtime which workspace the chat is in
This was referenced Aug 14, 2026
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.
Summary
Why
Affected Areas
apps/tradinggooseapps/docspackages/*Issue Links( if any )
Validation
Risk / Rollout Notes
Config / Data Changes
Screenshots / Video
Checklist
Summary by CodeRabbit