fix(provider): unify model config resolution - #2053
Open
yyhhyyyyyy wants to merge 4 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR separates raw provider facts from user model intent, centralizes effective model resolution, adds user-only storage migrations, updates provider metadata and tests, refreshes model catalog entries, and adds SHA-256 validation for ACP registry binaries. ChangesModel configuration source of truth
ACP registry refresh
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Provider as Provider discovery
participant Store as ProviderSettingsTable
participant Settings as ProviderSettings
participant Config as ModelConfigHelper
participant Manager as ModelManager
Provider->>Store: Persist sparse provider MODEL_META facts
Manager->>Settings: Request model list
Settings->>Store: Read raw provider facts
Settings->>Config: Resolve catalog, fallback, facts, and user intent
Config-->>Settings: Return effective ModelConfig
Settings-->>Manager: Return effective MODEL_META list
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
test/main/provider/providerModelCapabilityMapping.test.ts (1)
253-262: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
modelhere is actually the array index.
rawModels.entries()yields[index, value], so the destructuredmodelis a number, which is whymodel + 1andrawModels[model]work. Rename for clarity, or iterate values directly.♻️ Proposed rename
- for (const [model] of rawModels.entries()) { + for (const [index, rawModel] of rawModels.entries()) { expect(resolveModelConfigWithProvider).toHaveBeenNthCalledWith( - model + 1, - rawModels[model].id, + index + 1, + rawModel.id, 'custom-relay', undefined, - rawModels[model], + rawModel, 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 `@test/main/provider/providerModelCapabilityMapping.test.ts` around lines 253 - 262, Rename the destructured key from model to an index in the rawModels.entries() loop, and update the related call-count and rawModels[index] references accordingly; keep the existing provider-resolution assertions unchanged.test/main/provider/providerModelFacts.test.ts (1)
42-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider asserting the negative case of
hasPersistedDerivedProviderModelFields.The predicate gates the write in
migrateProviderModelsToRawFacts(src/main/provider/data/settingsTable.tsLines 391-422); a false-positive there rewrites every row on each migration run. A one-line assertion that it returnsfalsefor an already-stripped row (andtruewhen onlyselectableEndpointTypesis present) would lock that contract.🤖 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 `@test/main/provider/providerModelFacts.test.ts` around lines 42 - 69, The tests around stripDerivedProviderModelFields should also cover hasPersistedDerivedProviderModelFields: assert it returns false for an already-stripped row, and true when selectableEndpointTypes is the only persisted derived field. Use the existing remotely discovered or custom-model fixtures without changing their current fact assertions.test/main/provider/providerModelHelper.test.ts (1)
800-814: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated
getModelConfigmock.This exact 14-line mock is repeated at Lines 875-889. Since the real
modelConfigHelper.getModelConfigsignature just changed in this PR, a shared factory keeps both tests from drifting on the next signature change.♻️ Sketch
const createGetModelConfigMock = (configState: Map<string, unknown>) => vi.fn( ( modelId: string, providerId?: string, _capabilityProviderId?: string, _resolvedIdentity?: unknown, providerFacts?: ReturnType<typeof createBaseModel> ) => (providerId ? configState.get(cacheKey(providerId, modelId)) : undefined) ?? createModelConfig({ maxTokens: providerFacts?.maxTokens, contextLength: providerFacts?.contextLength, type: providerFacts?.type }) )🤖 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 `@test/main/provider/providerModelHelper.test.ts` around lines 800 - 814, Extract the repeated getModelConfig mock logic into a shared createGetModelConfigMock factory near the test setup, accepting configState and preserving the current five-argument signature and fallback behavior. Replace both getModelConfig mock definitions, including the one near the second occurrence, with calls to this factory so future signature changes remain synchronized.src/main/provider/providers/aiSdkProvider.ts (2)
2011-2028: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNit: reuse
toPositiveFiniteNumberfor the New API limit candidates.The inline predicates duplicate the new helper added at Line 186.
♻️ Proposed refactor
- const contextLengthCandidate = [ - rawModel.context_length, - rawModel.contextLength, - rawModel.input_token_limit, - rawModel.max_input_tokens - ].find( - (candidate): candidate is number => - typeof candidate === 'number' && Number.isFinite(candidate) && candidate > 0 - ) - - const maxTokensCandidate = [ - rawModel.max_tokens, - rawModel.max_output_tokens, - rawModel.output_token_limit - ].find( - (candidate): candidate is number => - typeof candidate === 'number' && Number.isFinite(candidate) && candidate > 0 - ) + const contextLengthCandidate = [ + rawModel.context_length, + rawModel.contextLength, + rawModel.input_token_limit, + rawModel.max_input_tokens + ] + .map(toPositiveFiniteNumber) + .find((candidate) => candidate !== undefined) + + const maxTokensCandidate = [ + rawModel.max_tokens, + rawModel.max_output_tokens, + rawModel.output_token_limit + ] + .map(toPositiveFiniteNumber) + .find((candidate) => candidate !== undefined)🤖 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 `@src/main/provider/providers/aiSdkProvider.ts` around lines 2011 - 2028, Update the contextLengthCandidate and maxTokensCandidate selection in the provider parsing flow to reuse the existing toPositiveFiniteNumber helper instead of duplicating its inline positive-finite number predicates, while preserving the current candidate ordering and fallback behavior.
1744-1819: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: extract per-strategy fact extraction into a lookup table.
Five parallel 5-branch nested ternaries over the same
strategyvalue is hard to scan and easy to break when a strategy is added. ARecord<strategy, (model) => {contextLength, maxTokens, functionCall, vision, reasoning}>would keep each provider's quirks colocated and make the sparse-fact contract explicit. Behavior today looks correct.🤖 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 `@src/main/provider/providers/aiSdkProvider.ts` around lines 1744 - 1819, Optionally replace the five parallel strategy-based ternary chains in the fact extraction block with a typed lookup table keyed by strategy, whose handlers return contextLength, maxTokens, functionCall, vision, and reasoning. Keep each provider’s existing extraction behavior and sparse undefined values unchanged, including tokenflux and reasoning handling, while preserving the current model metadata inputs and fallback logic.test/main/provider/basicApiKeyProviders.test.ts (1)
240-283: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional:
it.eachover the manualforloop.A failing case currently aborts the remaining providers and reports a single test name;
it.each(cases)gives per-provider isolation and naming. Coverage of the-1/NaN/Infinity/0boundaries is well chosen.🤖 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 `@test/main/provider/basicApiKeyProviders.test.ts` around lines 240 - 283, Optionally refactor the parameterized test around the cases array to use it.each(cases), moving the fetch mock, provider creation, model fetch, and assertions into the per-case test callback. Preserve all existing boundary-value coverage and assertions while giving each provider/model case an independent test name and failure result.src/main/provider/modelConfig.ts (1)
421-519: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
isUserDefinedassignment.Line 519 repeats what line 517 already sets;
applyProviderSpecificPoliciesnever clears it. Drop the trailing assignment (or the one in the spread) to keep a single source.🤖 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 `@src/main/provider/modelConfig.ts` around lines 421 - 519, Remove the trailing normalizedFinalConfig.isUserDefined assignment in getModelConfig, since the value is already set in the object passed to applyProviderSpecificPolicies and that method does not clear it. Keep the existing Boolean(userConfig) assignment as the single source of truth.test/main/provider/userModelConfig.test.ts (1)
23-36: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePut
legacyUserKeyin the case table instead of matching on the label.Deriving the option from
_label === 'legacy metadata key'(and naming a used parameter_label) hides the actual input under test.♻️ Suggested table shape
- it.each([ - ['missing source', entry({ config: { isUserDefined: true } })], - ['null source', entry({ source: null, config: { isUserDefined: true } })], - ['legacy metadata key', entry()] - ])('recognizes legacy user intent from %s', (_label, value) => { - expect( - normalizeUserModelConfigEntry(value, { - legacyUserKey: _label === 'legacy metadata key' - }) - ).toMatchObject({ + it.each([ + { label: 'missing source', value: entry({ config: { isUserDefined: true } }), legacyUserKey: false }, + { label: 'null source', value: entry({ source: null, config: { isUserDefined: true } }), legacyUserKey: false }, + { label: 'legacy metadata key', value: entry(), legacyUserKey: true } + ])('recognizes legacy user intent from $label', ({ value, legacyUserKey }) => { + expect(normalizeUserModelConfigEntry(value, { legacyUserKey })).toMatchObject({ source: 'user', config: { isUserDefined: true } }) })🤖 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 `@test/main/provider/userModelConfig.test.ts` around lines 23 - 36, Update the table-driven test for normalizeUserModelConfigEntry so each case explicitly includes its legacyUserKey value alongside the label and entry, then pass that table value directly into the options object. Remove the _label-based condition and rename destructured parameters to reflect the separate label, value, and legacy flag inputs.
🤖 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 `@resources/acp-registry/registry.json`:
- Around line 1031-1076: Update src/main/agent/acp/catalog/acpRegistryService.ts
and AcpRegistryBinaryDistribution to preserve the sha256 value during
normalization and verify the downloaded archive before extraction or execution.
Apply this to the OpenCode entries in resources/acp-registry/registry.json lines
1031-1076 and the siGit entries in resources/acp-registry/registry.json lines
1219-1247, retaining their hashes and enforcing checksum validation for each
platform artifact.
In `@resources/model-db/providers.json`:
- Around line 275739-275764: Update the gemini-omni-flash provider entry to use
the actual gemini-omni-flash-preview identifier and contract: support text,
image, and video inputs only, video output only, and a 1,048,576-token context
limit while preserving the existing output limit unless the provider schema
requires otherwise. Remove unsupported audio modalities and ensure the entry’s
type/request metadata matches video-generation workflows.
In `@src/main/provider/providerModelHelper.ts`:
- Around line 229-236: Update the supportedEndpointTypes mapping in
resolveModelConfigWithProvider so filtering model.supportedEndpointTypes with
isNewApiEndpointType normalizes an empty result to undefined. Preserve undefined
for non-array input and retain the filtered array when it contains at least one
supported endpoint type, allowing the later fallback to
rawProviderFacts.supportedEndpointTypes.
In `@src/main/provider/providers/ollamaProvider.ts`:
- Around line 643-650: Update attachModelInfo to merge the existing
model.model_info with the reconstructed model_info using the file’s existing
mergeModelInfo helper, preserving list-payload facts while allowing show-derived
values to take precedence when present. Keep the current conditional fields and
capabilities handling unchanged.
---
Nitpick comments:
In `@src/main/provider/modelConfig.ts`:
- Around line 421-519: Remove the trailing normalizedFinalConfig.isUserDefined
assignment in getModelConfig, since the value is already set in the object
passed to applyProviderSpecificPolicies and that method does not clear it. Keep
the existing Boolean(userConfig) assignment as the single source of truth.
In `@src/main/provider/providers/aiSdkProvider.ts`:
- Around line 2011-2028: Update the contextLengthCandidate and
maxTokensCandidate selection in the provider parsing flow to reuse the existing
toPositiveFiniteNumber helper instead of duplicating its inline positive-finite
number predicates, while preserving the current candidate ordering and fallback
behavior.
- Around line 1744-1819: Optionally replace the five parallel strategy-based
ternary chains in the fact extraction block with a typed lookup table keyed by
strategy, whose handlers return contextLength, maxTokens, functionCall, vision,
and reasoning. Keep each provider’s existing extraction behavior and sparse
undefined values unchanged, including tokenflux and reasoning handling, while
preserving the current model metadata inputs and fallback logic.
In `@test/main/provider/basicApiKeyProviders.test.ts`:
- Around line 240-283: Optionally refactor the parameterized test around the
cases array to use it.each(cases), moving the fetch mock, provider creation,
model fetch, and assertions into the per-case test callback. Preserve all
existing boundary-value coverage and assertions while giving each provider/model
case an independent test name and failure result.
In `@test/main/provider/providerModelCapabilityMapping.test.ts`:
- Around line 253-262: Rename the destructured key from model to an index in the
rawModels.entries() loop, and update the related call-count and rawModels[index]
references accordingly; keep the existing provider-resolution assertions
unchanged.
In `@test/main/provider/providerModelFacts.test.ts`:
- Around line 42-69: The tests around stripDerivedProviderModelFields should
also cover hasPersistedDerivedProviderModelFields: assert it returns false for
an already-stripped row, and true when selectableEndpointTypes is the only
persisted derived field. Use the existing remotely discovered or custom-model
fixtures without changing their current fact assertions.
In `@test/main/provider/providerModelHelper.test.ts`:
- Around line 800-814: Extract the repeated getModelConfig mock logic into a
shared createGetModelConfigMock factory near the test setup, accepting
configState and preserving the current five-argument signature and fallback
behavior. Replace both getModelConfig mock definitions, including the one near
the second occurrence, with calls to this factory so future signature changes
remain synchronized.
In `@test/main/provider/userModelConfig.test.ts`:
- Around line 23-36: Update the table-driven test for
normalizeUserModelConfigEntry so each case explicitly includes its legacyUserKey
value alongside the label and entry, then pass that table value directly into
the options object. Remove the _label-based condition and rename destructured
parameters to reflect the separate label, value, and legacy flag inputs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8b78238e-05e7-4c0f-b31f-86aa2b5fda9a
📒 Files selected for processing (46)
docs/architecture/model-capability-identity/spec.mddocs/architecture/model-config-source-of-truth/plan.mddocs/architecture/model-config-source-of-truth/spec.mddocs/architecture/model-config-source-of-truth/tasks.mdresources/acp-registry/registry.jsonresources/model-db/providers.jsonsrc/main/config/migration.tssrc/main/provider/data/settingsTable.tssrc/main/provider/managers/modelManager.tssrc/main/provider/modelConfig.tssrc/main/provider/providerId.tssrc/main/provider/providerImportService.tssrc/main/provider/providerModelFacts.tssrc/main/provider/providerModelHelper.tssrc/main/provider/providers/acpProvider.tssrc/main/provider/providers/aiSdkProvider.tssrc/main/provider/providers/githubCopilotProvider.tssrc/main/provider/providers/ollamaProvider.tssrc/main/provider/providers/voiceAIProvider.tssrc/main/provider/settings.tssrc/main/provider/userModelConfig.tssrc/main/sync/configImportService.tssrc/main/sync/index.tstest/main/config/migration.test.tstest/main/provider/anthropicProvider.test.tstest/main/provider/awsBedrockProvider.test.tstest/main/provider/basicApiKeyProviders.test.tstest/main/provider/data/settingsTable.test.tstest/main/provider/doubaoProvider.test.tstest/main/provider/geminiProvider.test.tstest/main/provider/githubCopilotProvider.test.tstest/main/provider/kimiForCodingProvider.test.tstest/main/provider/mistralProvider.test.tstest/main/provider/modelConfig.test.tstest/main/provider/modelManager.test.tstest/main/provider/newApiProvider.test.tstest/main/provider/ollamaProvider.test.tstest/main/provider/openaiCodexProvider.test.tstest/main/provider/providerDbModelConfig.test.tstest/main/provider/providerModelCapabilityMapping.test.tstest/main/provider/providerModelFacts.test.tstest/main/provider/providerModelHelper.test.tstest/main/provider/providerRuntime.test.tstest/main/provider/userModelConfig.test.tstest/main/sync/configImportService.test.tstest/main/sync/syncService.test.ts
💤 Files with no reviewable changes (1)
- src/main/provider/providers/acpProvider.ts
Comment on lines
229
to
236
| return { | ||
| endpointType, | ||
| supportedEndpointTypes: model.supportedEndpointTypes | ||
| ? [...model.supportedEndpointTypes] | ||
| supportedEndpointTypes: Array.isArray(model.supportedEndpointTypes) | ||
| ? model.supportedEndpointTypes.filter(isNewApiEndpointType) | ||
| : undefined, | ||
| type, | ||
| ownedBy | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -t f 'model.ts' src/shared --exec sed -n '1,200p'
rg -nP 'isNewApiEndpointType|NEW_API_ENDPOINT_TYPES' src/shared -C3Repository: ThinkInAIXYZ/deepchat
Length of output: 13057
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== providerModelHelper.ts relevant sections =="
sed -n '1,280p' src/main/provider/providerModelHelper.ts
echo
echo "== settings.ts usages of supportedEndpointTypes =="
rg -n 'supportedEndpointTypes|resolveCapabilityRouteWithProvider|resolveModelConfigWithProvider' src/main/provider/settings.ts -C 5
echo
echo "== all supportedEndpointTypes assignments/usages in main/shared =="
rg -n 'supportedEndpointTypes|resolveProviderModelRouteMetadata|NewApiRouteMeta' src/main src/shared -C 2Repository: ThinkInAIXYZ/deepchat
Length of output: 34581
Normalize empty filtered supportedEndpointTypes to undefined.
When model.supportedEndpointTypes is a non-empty array whose entries all fail isNewApiEndpointType, this returns []. resolveModelConfigWithProvider later merges raw facts with route.supportedEndpointTypes ?? rawProviderFacts.supportedEndpointTypes, so the empty array replaces the raw provider facts and can affect downstream endpoint routing.
♻️ Proposed normalization
- supportedEndpointTypes: Array.isArray(model.supportedEndpointTypes)
- ? model.supportedEndpointTypes.filter(isNewApiEndpointType)
- : undefined,
+ supportedEndpointTypes: {
+ const filtered = Array.isArray(model.supportedEndpointTypes)
+ ? model.supportedEndpointTypes.filter(isNewApiEndpointType)
+ : undefined
+ return Array.isArray(filtered) && filtered.length === 0 ? undefined : filtered
+ },🤖 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 `@src/main/provider/providerModelHelper.ts` around lines 229 - 236, Update the
supportedEndpointTypes mapping in resolveModelConfigWithProvider so filtering
model.supportedEndpointTypes with isNewApiEndpointType normalizes an empty
result to undefined. Preserve undefined for non-array input and retain the
filtered array when it contains at least one supported endpoint type, allowing
the later fallback to rawProviderFacts.supportedEndpointTypes.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/agent/acp/launch/acpLaunchSpecService.ts`:
- Around line 406-441: Update downloadArchive to enforce a bounded timeout on
the fetch(url) call by creating and passing an AbortSignal, preserving the
existing response validation, checksum handling, and temp-directory cleanup.
Ensure a stalled archive server aborts and propagates an error instead of
allowing ensureRegistryAgentInstalled to hang indefinitely.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e0b14317-83f8-4a43-8929-c855dec29b48
📒 Files selected for processing (13)
src/main/agent/acp/catalog/acpRegistryService.tssrc/main/agent/acp/launch/acpLaunchSpecService.tssrc/main/provider/modelConfig.tssrc/main/provider/providers/aiSdkProvider.tssrc/main/provider/providers/ollamaProvider.tssrc/shared/types/acp.tstest/main/agent/acp/catalog/acpRegistryService.test.tstest/main/agent/acp/launch/acpLaunchSpecService.test.tstest/main/provider/ollamaProvider.test.tstest/main/provider/providerModelCapabilityMapping.test.tstest/main/provider/providerModelFacts.test.tstest/main/provider/providerModelHelper.test.tstest/main/provider/userModelConfig.test.ts
💤 Files with no reviewable changes (1)
- src/main/provider/modelConfig.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- test/main/provider/userModelConfig.test.ts
- test/main/provider/providerModelFacts.test.ts
- test/main/provider/ollamaProvider.test.ts
- test/main/provider/providerModelCapabilityMapping.test.ts
- src/main/provider/providers/ollamaProvider.ts
- src/main/provider/providers/aiSdkProvider.ts
- test/main/provider/providerModelHelper.test.ts
Comment on lines
+406
to
+441
| private async downloadArchive( | ||
| url: string, | ||
| agent: AcpRegistryAgent, | ||
| expectedSha256?: string | ||
| ): Promise<DownloadedArchive> { | ||
| const safeAgentId = sanitizeInstallSegment(agent.id, 'agent id') | ||
| const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), `deepchat-acp-${safeAgentId}-`)) | ||
| const archivePath = path.join(tempDir, path.basename(new URL(url).pathname)) | ||
| const response = await fetch(url) | ||
| if (!response.ok) { | ||
| throw new Error(`Failed to download archive: ${response.status} ${response.statusText}`) | ||
| } | ||
| try { | ||
| const archiveName = path.basename(new URL(url).pathname) | ||
| if (!archiveName) { | ||
| throw new Error(`Invalid archive URL for ACP registry agent ${agent.id}`) | ||
| } | ||
| const archivePath = path.join(tempDir, archiveName) | ||
| const response = await fetch(url) | ||
| if (!response.ok) { | ||
| throw new Error(`Failed to download archive: ${response.status} ${response.statusText}`) | ||
| } | ||
|
|
||
| const arrayBuffer = await response.arrayBuffer() | ||
| fs.writeFileSync(archivePath, Buffer.from(arrayBuffer)) | ||
| return archivePath | ||
| const archive = Buffer.from(await response.arrayBuffer()) | ||
| if (expectedSha256 !== undefined) { | ||
| const normalizedSha256 = expectedSha256.trim().toLowerCase() | ||
| if (!SHA256_PATTERN.test(normalizedSha256)) { | ||
| throw new Error(`Invalid SHA-256 checksum for ACP registry agent ${agent.id}`) | ||
| } | ||
| const actualSha256 = createHash('sha256').update(archive).digest('hex') | ||
| if (actualSha256 !== normalizedSha256) { | ||
| throw new Error(`SHA-256 checksum mismatch for ACP registry agent ${agent.id}`) | ||
| } | ||
| } | ||
|
|
||
| fs.writeFileSync(archivePath, archive) | ||
| return { archivePath, tempDir } | ||
| } catch (error) { | ||
| fs.rmSync(tempDir, { recursive: true, force: true }) | ||
| throw error | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
fetch(url) has no timeout — a stalled server hangs agent install indefinitely.
downloadArchive awaits fetch(url) with no AbortSignal. If the download server never responds, ensureRegistryAgentInstalled (and thus the caller) hangs forever with no way to recover short of app restart. This is an external-call hazard on a path that already carries retry/checksum logic, so a bounded timeout is cheap insurance.
🔧 Suggested fix
- const response = await fetch(url)
+ const response = await fetch(url, { signal: AbortSignal.timeout(30_000) })📝 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
| private async downloadArchive( | |
| url: string, | |
| agent: AcpRegistryAgent, | |
| expectedSha256?: string | |
| ): Promise<DownloadedArchive> { | |
| const safeAgentId = sanitizeInstallSegment(agent.id, 'agent id') | |
| const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), `deepchat-acp-${safeAgentId}-`)) | |
| const archivePath = path.join(tempDir, path.basename(new URL(url).pathname)) | |
| const response = await fetch(url) | |
| if (!response.ok) { | |
| throw new Error(`Failed to download archive: ${response.status} ${response.statusText}`) | |
| } | |
| try { | |
| const archiveName = path.basename(new URL(url).pathname) | |
| if (!archiveName) { | |
| throw new Error(`Invalid archive URL for ACP registry agent ${agent.id}`) | |
| } | |
| const archivePath = path.join(tempDir, archiveName) | |
| const response = await fetch(url) | |
| if (!response.ok) { | |
| throw new Error(`Failed to download archive: ${response.status} ${response.statusText}`) | |
| } | |
| const arrayBuffer = await response.arrayBuffer() | |
| fs.writeFileSync(archivePath, Buffer.from(arrayBuffer)) | |
| return archivePath | |
| const archive = Buffer.from(await response.arrayBuffer()) | |
| if (expectedSha256 !== undefined) { | |
| const normalizedSha256 = expectedSha256.trim().toLowerCase() | |
| if (!SHA256_PATTERN.test(normalizedSha256)) { | |
| throw new Error(`Invalid SHA-256 checksum for ACP registry agent ${agent.id}`) | |
| } | |
| const actualSha256 = createHash('sha256').update(archive).digest('hex') | |
| if (actualSha256 !== normalizedSha256) { | |
| throw new Error(`SHA-256 checksum mismatch for ACP registry agent ${agent.id}`) | |
| } | |
| } | |
| fs.writeFileSync(archivePath, archive) | |
| return { archivePath, tempDir } | |
| } catch (error) { | |
| fs.rmSync(tempDir, { recursive: true, force: true }) | |
| throw error | |
| } | |
| private async downloadArchive( | |
| url: string, | |
| agent: AcpRegistryAgent, | |
| expectedSha256?: string | |
| ): Promise<DownloadedArchive> { | |
| const safeAgentId = sanitizeInstallSegment(agent.id, 'agent id') | |
| const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), `deepchat-acp-${safeAgentId}-`)) | |
| try { | |
| const archiveName = path.basename(new URL(url).pathname) | |
| if (!archiveName) { | |
| throw new Error(`Invalid archive URL for ACP registry agent ${agent.id}`) | |
| } | |
| const archivePath = path.join(tempDir, archiveName) | |
| const response = await fetch(url, { signal: AbortSignal.timeout(30_000) }) | |
| if (!response.ok) { | |
| throw new Error(`Failed to download archive: ${response.status} ${response.statusText}`) | |
| } | |
| const archive = Buffer.from(await response.arrayBuffer()) | |
| if (expectedSha256 !== undefined) { | |
| const normalizedSha256 = expectedSha256.trim().toLowerCase() | |
| if (!SHA256_PATTERN.test(normalizedSha256)) { | |
| throw new Error(`Invalid SHA-256 checksum for ACP registry agent ${agent.id}`) | |
| } | |
| const actualSha256 = createHash('sha256').update(archive).digest('hex') | |
| if (actualSha256 !== normalizedSha256) { | |
| throw new Error(`SHA-256 checksum mismatch for ACP registry agent ${agent.id}`) | |
| } | |
| } | |
| fs.writeFileSync(archivePath, archive) | |
| return { archivePath, tempDir } | |
| } catch (error) { | |
| fs.rmSync(tempDir, { recursive: true, force: true }) | |
| throw error | |
| } |
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 435-435: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFileSync(archivePath, archive)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🤖 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 `@src/main/agent/acp/launch/acpLaunchSpecService.ts` around lines 406 - 441,
Update downloadArchive to enforce a bounded timeout on the fetch(url) call by
creating and passing an AbortSignal, preserving the existing response
validation, checksum handling, and temp-directory cleanup. Ensure a stalled
archive server aborts and propagates an error instead of allowing
ensureRegistryAgentInstalled to hang indefinitely.
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
safe fallback < catalog < provider facts < user config < policymodel_configs.provider_modelsas the sparse source of upstream facts and strip catalog-derived projections at the physical write boundary.ownedByand endpoint information.gpt-5.6-solresolving to16000 / 4096instead of1050000 / 32000.Architecture
ProviderSettingsowns effective config resolution.ProviderModelHelperreads raw provider facts and performs no hidden writes.Migration and compatibility
created_atfor retained user configuration rows.200000 / 32000.8192 / 4096.Regression coverage
The New API
gpt-5.6-solflow now consistently resolves to1050000 / 32000after:Summary by CodeRabbit