Repository navigation
feat(shared,desktop,providers,core): align connection test and invoca… - #223
Conversation
github-actions
Bot
added
area:desktop
…tion parity (#216) Signed-off-by: Sun-sunshine06 <Sun-sunshine06@users.noreply.github.com>
d8c4287 to
ad1136c
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Findings
-
[Major]
modelsHintnow disables connection checks for valid static-hint providers —defaultProviderCapabilities()tiessupportsModelsEndpointtomodelsHint, andrunProviderTest()now treatssupportsModelsEndpoint: falseas “skip /models entirely”. For an importedanthropicprovider with a static model list,connection:v1:test-providerwill now returnProvider capability profile disables /models, but this wire has no fallback probeeven when/v1/modelsis healthy. Evidence:packages/shared/src/config.ts:188,apps/desktop/src/main/connection-ipc.ts:546,apps/desktop/src/main/connection-ipc.ts:652,apps/desktop/src/main/connection-ipc.ts:900
Suggested fix:const supportsModelsEndpoint = entry.capabilities?.supportsModelsEndpoint ?? entry.wire !== 'openai-codex-responses'; const modelDiscoveryMode = entry.modelsHint !== undefined ? 'static-hint' : supportsModelsEndpoint ? 'models' : 'manual';
-
[Major] Passing resolved default capabilities into
inferReasoning()suppresses synthesized reasoning models on chat-based providers —resolveActiveModel()now forwardssupportsReasoning: falsefrom built-in OpenAI/OpenRouter capability defaults, and the new early return ininferReasoning()makes that override the existinggpt-5/o*/openai/*heuristics. Any model missing frompi-ai's registry will now be synthesized withreasoning=falseon the exact path that previously relied on those heuristics. Evidence:apps/desktop/src/main/provider-settings.ts:230,packages/shared/src/config.ts:260,packages/providers/src/index.ts:222,packages/providers/src/index.test.ts:539
Suggested fix:if (capabilities?.supportsReasoning === true) return true; const preserveChatHeuristics = wire === 'openai-chat' && (isOpenAIOfficial(baseUrl) || /^https:\/\/openrouter\.ai\/api\/v1(\/|$)/.test(baseUrl ?? '')); if (capabilities?.supportsReasoning === false && !preserveChatHeuristics) return false;
Summary
Review mode: initial. Found 2 major regressions in the new capability plumbing: one false-negative connection test for static-hint providers, and one reasoning-capability override that changes synthesized OpenAI/OpenRouter request shaping. Not found in repo/docs: docs/VISION.md, docs/PRINCIPLES.md in this checkout.
Testing
Not run (automation). pnpm was unavailable initially, and corepack pnpm in this runner is on Node v20.20.2 while the repo requires Node >=22; vitest was also not available from the current install.
open-codesign Bot
| @@ -182,14 +187,21 @@ export function defaultProviderCapabilities( | |||
| ): Required<ProviderCapabilities> { | |||
| const supportsModelsEndpoint = | |||
There was a problem hiding this comment.
modelsHint should not implicitly mean supportsModelsEndpoint = false for every wire. With the new runProviderTest() branch, a static-hint anthropic provider now fails connection:v1:test-provider immediately even if /v1/models is healthy.
Suggested fix:
const supportsModelsEndpoint =
entry.capabilities?.supportsModelsEndpoint ?? entry.wire !== openai-codex-responses;| baseUrl: string | undefined, | ||
| capabilities?: ProviderCapabilities, | ||
| ): boolean { | ||
| if (capabilities?.supportsReasoning !== undefined) { |
There was a problem hiding this comment.
This new early return makes the resolved default supportsReasoning: false override the existing gpt-5 / o* / OpenRouter heuristics whenever we synthesize a Pi model. That regresses the unknown-model path for chat-based providers even though the heuristic tests still expect true.
Suggested fix:
if (capabilities?.supportsReasoning === true) return true;
// keep the existing openai-chat heuristics for synthesized models…asoning heuristics Signed-off-by: Sun-sunshine06 <Sun-sunshine06@users.noreply.github.com>
There was a problem hiding this comment.
Findings
- [Major] Explicit
supportsReasoning: falseis still ignored for imported providers on official OpenAI/OpenRouter hosts —resolveProviderCapabilities()now always materializessupportsReasoning, so by the timeinferReasoning()runs it can no longer tell an explicitfalsefrom the built-in default. The newpreserveChatHeuristicsbranch therefore re-enables reasoning for any customopenai-chatprovider whosebaseUrlishttps://api.openai.com/v1orhttps://openrouter.ai/api/v1, even when the provider capability profile asked to disable it. That is a reachable path for imported custom providers such as OpenCode'sopencode-openrouterentries. Evidencepackages/providers/src/index.ts:230,packages/providers/src/index.ts:281,packages/shared/src/config.ts:215,apps/desktop/src/main/imports/opencode-config.ts:68.
Suggested fix:export function inferReasoning( providerId: string, wire: GenerateOptions['wire'], modelId: string, baseUrl: string | undefined, capabilities?: ProviderCapabilities, ): boolean { if (capabilities?.supportsReasoning === true) return true; const preserveChatHeuristics = wire === 'openai-chat' && (providerId === 'openai' || providerId === 'openrouter'); if (capabilities?.supportsReasoning === false && !preserveChatHeuristics) return false; ... } reasoning: inferReasoning(provider, wire, modelId, baseUrl, capabilities),
Summary
Review mode: follow-up after new commits. Found 1 major regression in the follow-up fix: explicit reasoning opt-outs are still ignored for imported openai-chat providers that point at official OpenAI/OpenRouter base URLs. Not found in repo/docs: docs/VISION.md, docs/PRINCIPLES.md in this checkout.
Testing
Not run (automation). pnpm is not installed in this runner; corepack pnpm resolves, but this checkout has no node_modules, the repo requires Node >=22 while the runner has Node v20.20.2, and turbo is therefore unavailable.
open-codesign Bot
| if (capabilities?.supportsReasoning === true) { | ||
| return true; | ||
| } | ||
| const preserveChatHeuristics = |
There was a problem hiding this comment.
resolveProviderCapabilities() now always passes a concrete supportsReasoning boolean downstream, so this hostname check can't distinguish an explicit opt-out from the built-in default. For imported custom providers on https://api.openai.com/v1 / https://openrouter.ai/api/v1, an explicit supportsReasoning: false is ignored here and we still synthesize reasoning: true for gpt-5 / o* model ids.
Suggested fix:
const preserveChatHeuristics =
wire === 'openai-chat' && (providerId === 'openai' || providerId === 'openrouter');… endpoints Signed-off-by: Sun-sunshine06 <Sun-sunshine06@users.noreply.github.com>
There was a problem hiding this comment.
Findings
- [Major] Capability propagation still stops at the legacy
generate()path — this diff addscapabilitiesto the generation input and passes them into the legacycompleteWithRetry()flow (packages/core/src/index.ts:101,packages/core/src/index.ts:373,apps/desktop/src/main/index.ts:827), but the default-ongenerateViaAgent()path still ignores them.buildPiModel()hardcodesreasoning: trueandthinkingLevelis still derived fromreasoningForModel()without consideringinput.capabilities(packages/core/src/agent.ts:254,packages/core/src/agent.ts:288,packages/core/src/agent.ts:808). As a result, importedopenai-chatproviders that explicitly setsupportsReasoning: falsecan still hit the same "connection test passes, actual invocation fails" mismatch wheneverUSE_AGENT_RUNTIME=true.
Suggested fix:import { inferReasoning } from '@open-codesign/providers'; import type { ProviderCapabilities } from '@open-codesign/shared'; function buildPiModel( model: ModelRef, wire: WireApi | undefined, baseUrl: string | undefined, httpHeaders?: Record<string, string>, apiKey?: string, capabilities?: ProviderCapabilities, ): PiModel { ... const out: PiModel = { ..., reasoning: inferReasoning(wire, model.modelId, canonicalBase, capabilities, model.provider), }; } const thinkingLevel = input.reasoningLevel ?? (inferReasoning(input.wire, input.model.modelId, input.baseUrl, input.capabilities, input.model.provider) ? reasoningForModel(input.model, input.baseUrl) : undefined) ?? 'off';
Summary
Review mode: follow-up after new commits. Found 1 major regression: the new capability propagation does not reach the default-on generateViaAgent() path, so imported providers can still send unsupported reasoning semantics under USE_AGENT_RUNTIME=true.
Testing
Not run (automation). Runner has Node v20.20.2, the repo requires Node 22, and this checkout has no node_modules.
open-codesign Bot
| @@ -827,6 +827,7 @@ function registerIpcHandlers(db: Database | null): void { | |||
| ...(baseUrl !== undefined ? { baseUrl } : {}), | |||
There was a problem hiding this comment.
This now threads capabilities into the legacy generate() path, but runGenerate() defaults to generateViaAgent() and that path still ignores input.capabilities: buildPiModel() hardcodes reasoning: true and thinkingLevel still comes from reasoningForModel() alone (packages/core/src/agent.ts:254-310, packages/core/src/agent.ts:808-809). Imported openai-chat providers that explicitly set supportsReasoning: false can therefore still hit the same invocation mismatch this PR is trying to eliminate.
Suggested fix:
const piModel = buildPiModel(
input.model,
input.wire,
input.baseUrl,
input.httpHeaders,
input.apiKey,
input.capabilities,
);
const thinkingLevel =
input.reasoningLevel ??
(inferReasoning(input.wire, input.model.modelId, input.baseUrl, input.capabilities, input.model.provider)
? reasoningForModel(input.model, input.baseUrl)
: undefined) ??
'off';
There was a problem hiding this comment.
Findings
- [Major] Builtin OpenAI/OpenRouter rows still force reasoning on when their base URL points at a proxy —
inferReasoning()now preserves the officialopenai-chatheuristics solely fromproviderId === 'openai' || 'openrouter'(packages/providers/src/index.ts:231), andgenerateViaAgent()now depends on that branch for the agent runtime (packages/core/src/agent.ts:721). Builtin rows can still be repointed to custom gateways, while their resolved capability profile continues to defaultsupportsReasoning: false(packages/shared/src/config.ts:260,packages/shared/src/config.ts:280). In that configuration, a builtinopenai/openrouterrow aimed at a proxy likehttps://api.duckcoding.ai/v1will still returnreasoning: trueforgpt-5*, so the “connection test passes, actual invocation fails” mismatch remains for the exact setup this PR is trying to close.
Suggested fix:const preserveChatHeuristics = wire === 'openai-chat' && ((providerId === 'openai' && isOpenAIOfficial(baseUrl)) || (providerId === 'openrouter' && isOpenRouterOfficial(baseUrl)));
Summary
Review mode: follow-up after new commits. Found 1 major regression: the new capability-aware reasoning guard still treats builtin OpenAI/OpenRouter providers as “official” after their base URL is repointed to a proxy, so agent-runtime requests can still send unsupported reasoning semantics.
Testing
Not run (automation). Runner has Node v20.20.2, the repo expects Node 22, and this checkout has no node_modules.
open-codesign Bot
| } | ||
| const preserveChatHeuristics = | ||
| wire === 'openai-chat' && (providerId === 'openai' || providerId === 'openrouter'); | ||
| if (capabilities?.supportsReasoning === false && !preserveChatHeuristics) { |
There was a problem hiding this comment.
This still keys the “preserve official heuristics” escape hatch on providerId alone. Builtin openai / openrouter rows can be repointed to a proxy base URL, so this branch keeps returning true for gpt-5* even when the resolved capability profile says supportsReasoning: false.
const preserveChatHeuristics =
wire === 'openai-chat' &&
((providerId === 'openai' && isOpenAIOfficial(baseUrl)) ||
(providerId === 'openrouter' && isOpenRouterOfficial(baseUrl)));
There was a problem hiding this comment.
Findings
- [Major] The new
capabilitiesplumbing still does not affect registeredpi-aimodels on the legacy completion path.runModel()now forwardscapabilitiesintocompleteWithRetry()(packages/core/src/index.ts:373), butcomplete()only consumes them inside the newsynthesizeWireModel(...)branch (packages/providers/src/index.ts:335). When the provider/model pair already exists inpi-ai's registry, that branch is skipped, soapplyComment,generateTitle, andgenerate()withUSE_AGENT_RUNTIME=0still keep the registry model's original reasoning behavior even after a builtinopenai/openrouterrow is repointed to a proxy.
Suggested fix:let piModel = pi.getModel(model.provider, effectiveModelId); if (piModel && opts.wire !== undefined) { piModel = { ...piModel, reasoning: inferReasoning( opts.wire, effectiveModelId, opts.baseUrl, opts.capabilities, model.provider, ), }; }
Summary
Review mode: follow-up after new commits. Found 1 major parity gap: the new provider capability profile is ignored whenever complete() reuses a registered pi-ai model, so the non-agent completion paths can still send unsupported reasoning semantics after a builtin provider is repointed to a proxy. Not found in repo/docs: docs/VISION.md, docs/PRINCIPLES.md.
Testing
Not run (automation). Missing coverage for complete() when pi.getModel() hits and capabilities.supportsReasoning should override the registry model.
open-codesign Bot
| effectiveModelId, | ||
| opts.wire, | ||
| opts.baseUrl, | ||
| opts.capabilities, |
There was a problem hiding this comment.
This only threads opts.capabilities into the synthesized-model branch. If pi.getModel(model.provider, effectiveModelId) returns a registry model first, the new capability profile is ignored entirely, so the legacy complete() paths can still keep the registry model's original reasoning behavior after a builtin openai/openrouter row is repointed to a proxy.
let piModel = pi.getModel(model.provider, effectiveModelId);
if (piModel && opts.wire !== undefined) {
piModel = {
...piModel,
reasoning: inferReasoning(
opts.wire,
effectiveModelId,
opts.baseUrl,
opts.capabilities,
model.provider,
),
};
}
There was a problem hiding this comment.
Findings
-
[Blocker] Missing DCO trailers on commits
726d53fa,b1541355, and6108a890— the repo hard-requiresSigned-off-by, so this head does not satisfy the PR gate yet.
Suggested fix:git rebase origin/main --exec 'git commit --amend --no-edit -s' git push --force-with-lease -
[Major]
complete()drops the builtin OpenAI/OpenRouter fallback whenbaseUrlis omitted — the new reasoning override now re-runsinferReasoning()against rawopts.baseUrl, so callers that rely on the registry model's official endpoint default can flip registeredgpt-5/o*rows back toreasoning: false, evidencepackages/providers/src/index.ts:336.
Suggested fix:const effectiveBaseUrl = opts.baseUrl ?? piModel.baseUrl; piModel = { ...piModel, reasoning: inferReasoning( opts.wire, effectiveModelId, effectiveBaseUrl, opts.capabilities, model.provider, ), };
-
[Major]
generateViaAgent()has the same omitted-baseUrlregression forthinkingLevel—buildPiModel()already resolves the builtin public endpoint, but the new gate recomputesinferReasoning()from rawinput.baseUrl, so builtin OpenAI/OpenRouter calls can fall back to'off', evidencepackages/core/src/agent.ts:814.
Suggested fix:const effectiveBaseUrl = piModel.baseUrl; const thinkingLevel = input.reasoningLevel ?? (inferReasoning( input.wire, input.model.modelId, effectiveBaseUrl, input.capabilities, input.model.provider, ) ? reasoningForModel(input.model, effectiveBaseUrl) : undefined) ?? 'off';
Summary
- Review mode: follow-up after new commits. Found 3 issues: 3 unsigned commits still violate the repo's DCO requirement, and the new reasoning-capability overrides regress builtin OpenAI/OpenRouter defaults when package callers omit
baseUrl. Not found in repo/docs:docs/VISION.md,docs/PRINCIPLES.md.
Testing
- Not run (automation). Missing coverage for
complete()andgenerateViaAgent()with builtinopenai/openrouter,wireset,capabilities.supportsReasoning=false, and omittedbaseUrl.
open-codesign Bot
| if (piModel && opts.wire !== undefined) { | ||
| piModel = { | ||
| ...piModel, | ||
| reasoning: inferReasoning( |
There was a problem hiding this comment.
This override now depends on raw opts.baseUrl. If a caller relies on the builtin registry model's default official endpoint and omits baseUrl, inferReasoning(...) sees undefined here and disables the official OpenAI/OpenRouter heuristic you just added. opts.baseUrl ?? piModel.baseUrl keeps the fallback intact.
const effectiveBaseUrl = opts.baseUrl ?? piModel.baseUrl;
piModel = {
...piModel,
reasoning: inferReasoning(
opts.wire,
effectiveModelId,
effectiveBaseUrl,
opts.capabilities,
model.provider,
),
};| const thinkingLevel = | ||
| input.reasoningLevel ?? reasoningForModel(input.model, input.baseUrl) ?? 'off'; | ||
| input.reasoningLevel ?? | ||
| (inferReasoning( |
There was a problem hiding this comment.
buildPiModel() already resolved the builtin public base URL above, but this second inferReasoning(...) call falls back to raw input.baseUrl. For builtin OpenAI/OpenRouter callers that omit baseUrl, that makes thinkingLevel drop to 'off' even though piModel.reasoning stayed enabled. Reuse piModel.baseUrl here so both checks see the same endpoint.
const effectiveBaseUrl = piModel.baseUrl;
const thinkingLevel =
input.reasoningLevel ??
(inferReasoning(
input.wire,
input.model.modelId,
effectiveBaseUrl,
input.capabilities,
input.model.provider,
)
? reasoningForModel(input.model, effectiveBaseUrl)
: undefined) ??
'off';
There was a problem hiding this comment.
Findings
-
[Blocker] Four non-merge commits on this head still lack
Signed-off-bytrailers, which violates the repo's DCO requirement and will block compliance checks, evidence.github/workflows/ci.yml:64. Missing trailers on726d53fa,8d68c833,b1541355, and6108a890.
Suggested fix:git rebase origin/main --exec 'git commit --amend --no-edit -s' git push --force-with-lease -
[Major] The non-agent
generate()/applyComment()path still computes the explicitreasoningoption without consultingcapabilities.supportsReasoning, so builtinopenai/openrouterrows repointed at a proxy can keep sendingreasoning: high|mediumeven though this diff now propagatescapabilitiesintocompleteWithRetry, evidencepackages/core/src/index.ts:373.
Suggested fix:import { inferReasoning } from '@open-codesign/providers'; const effectiveBaseUrl = input.baseUrl; let reasoning = input.reasoningLevel ?? (inferReasoning( input.wire, input.model.modelId, effectiveBaseUrl, input.capabilities, input.model.provider, ) ? reasoningForModel(input.model, effectiveBaseUrl) : undefined);
Summary
- Review mode: follow-up after new commits. Found 2 issues: 4 unsigned non-merge commits still violate the repo's DCO requirement, and the legacy non-agent invocation path still ignores
supportsReasoning=falsewhen deciding whether to send the explicit reasoning knob. Not found in repo/docs in this checkout:docs/VISION.md,docs/PRINCIPLES.md.
Testing
- Not run (automation):
pnpmis not installed in this runner. Missing coverage forgenerate()/applyComment()with builtinopenaioropenrouter, a proxybaseUrl, andcapabilities.supportsReasoning=false.
open-codesign Bot
| ...(input.baseUrl !== undefined ? { baseUrl: input.baseUrl } : {}), | ||
| ...(input.wire !== undefined ? { wire: input.wire } : {}), | ||
| ...(input.httpHeaders !== undefined ? { httpHeaders: input.httpHeaders } : {}), | ||
| ...(input.capabilities !== undefined ? { capabilities: input.capabilities } : {}), |
There was a problem hiding this comment.
This now forwards capabilities into completeWithRetry(), but runModel() still derives the explicit reasoning option from reasoningForModel(input.model, input.baseUrl) above. For builtin openai / openrouter rows repointed at non-reasoning proxies, that means the request can still send reasoning: high|medium even though complete() now sets piModel.reasoning to false.
import { inferReasoning } from '@open-codesign/providers';
const effectiveBaseUrl = input.baseUrl;
let reasoning =
input.reasoningLevel ??
(inferReasoning(
input.wire,
input.model.modelId,
effectiveBaseUrl,
input.capabilities,
input.model.provider,
)
? reasoningForModel(input.model, effectiveBaseUrl)
: undefined);- Pass capabilities into buildPiModel so reasoning is inferred from ProviderCapabilities instead of hardcoded true. - Gate thinkingLevel via inferReasoning so providers with explicit supportsReasoning=false do not get reasoning semantics sent under USE_AGENT_RUNTIME=true. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Sun-sunshine06 <Sun-sunshine06@users.noreply.github.com>
Signed-off-by: Sun-sunshine06 <Sun-sunshine06@users.noreply.github.com>
Signed-off-by: Sun-sunshine06 <Sun-sunshine06@users.noreply.github.com>
Signed-off-by: Sun-sunshine06 <Sun-sunshine06@users.noreply.github.com>
Signed-off-by: Sun-sunshine06 <Sun-sunshine06@users.noreply.github.com>
efa4c67 to
65ce853
Compare
There was a problem hiding this comment.
Findings
- [Major]
runModel()still ignorescapabilities.supportsReasoning=falsewhen deciding whether to send the explicitreasoningoption, so builtinopenai/openrouterrows repointed at non-reasoning proxies can still pass connection tests and then fail on realgenerate()/applyComment()requests, evidencepackages/core/src/index.ts:373,packages/core/src/generate.test.ts:150.
Suggested fix:import { inferReasoning } from '@open-codesign/providers'; const inferredReasoning = input.wire === undefined && input.capabilities === undefined ? true : inferReasoning( input.wire, input.model.modelId, input.baseUrl, input.capabilities, input.model.provider, ); let reasoning = input.reasoningLevel ?? (inferredReasoning ? reasoningForModel(input.model, input.baseUrl) : undefined);
Summary
- Review mode: follow-up after new commits. Found 1 remaining issue: the non-agent
generate()/applyComment()path still sends the explicit reasoning knob even after this PR started propagating capability profiles. Not found in repo/docs in this checkout:docs/VISION.md,docs/PRINCIPLES.md.
Testing
- Not run (automation):
pnpmis not installed in this runner. Suggested check after the fix:packages/core/src/generate.test.tsfor the new proxy reasoning opt-out case.
open-codesign Bot
| let reasoning = | ||
| input.reasoningLevel ?? | ||
| (inferredReasoning ? reasoningForModel(input.model, input.baseUrl) : undefined); | ||
| // Self-healing: if the upstream rejects on reasoning mismatch, flip the |
There was a problem hiding this comment.
This now forwards capabilities into completeWithRetry(), but runModel() still derives the explicit reasoning option from reasoningForModel(input.model, input.baseUrl) above. For builtin openai / openrouter rows repointed at non-reasoning proxies, generate() / applyComment() can still send reasoning: high|medium and fail at invocation time even though the new capability plumbing and generate.test.ts expect reasoning to be omitted.
import { inferReasoning } from '@open-codesign/providers';
const inferredReasoning =
input.wire === undefined && input.capabilities === undefined
? true
: inferReasoning(
input.wire,
input.model.modelId,
input.baseUrl,
input.capabilities,
input.model.provider,
);
let reasoning =
input.reasoningLevel ??
(inferredReasoning ? reasoningForModel(input.model, input.baseUrl) : undefined);…invocation-parity Signed-off-by: Sun-sunshine06 <Sun-sunshine06@users.noreply.github.com> # Conflicts: # packages/providers/src/index.ts
There was a problem hiding this comment.
Findings
- None.
Summary
- Review mode: follow-up after new commits. No additional diff-attributable issues found on the latest head. Residual risk: this runner does not include
pnpm, so the updated provider-capability propagation and connection-probe coverage were reviewed statically only. Not found in repo/docs in this checkout:docs/VISION.md,docs/PRINCIPLES.md.
Testing
- Not run (automation):
pnpmis not installed in this runner.
open-codesign Bot
Sun-sunshine06
left a comment
Sun-sunshine06
left a comment
There was a problem hiding this comment.
Findings
- [Major] Imported official OpenAI/OpenRouter providers without explicit
supportsReasoningnow lose reasoning heuristics
resolveProviderCapabilities() materializes every provider into Required<ProviderCapabilities>, so an imported openai-chat provider with no explicit capabilities.supportsReasoning becomes supportsReasoning: false. Then inferReasoning() treats that false as an opt-out unless providerId is exactly openai or openrouter.
That means a Codex/OpenCode imported provider like codex-openai / opencode-openai pointing at https://api.openai.com/v1 with model gpt-5.4 will now infer reasoning=false, whereas before this PR the official OpenAI base URL + gpt-5 heuristic enabled reasoning. Same shape applies to imported OpenRouter entries on https://openrouter.ai/api/v1.
Evidence:
packages/shared/src/config.ts:198packages/shared/src/config.ts:224packages/providers/src/wire-policy.ts:85packages/providers/src/wire-policy.ts:89
Suggested fix: preserve the distinction between “explicit false” and “default false”. For example, pass raw/explicit capabilities into inferReasoning() for the reasoning decision, or carry an explicitCapabilities/capabilitySource flag alongside the resolved profile. Add a regression test for an imported official OpenAI provider with no supportsReasoning field and gpt-5.4, plus the existing explicit opt-out case.
- [Major]
models:v1:list-for-providerstill ignoresmodelDiscoveryMode/supportsModelsEndpoint
runProviderTest() now respects supportsModelsEndpoint:false by probing inference directly, but models:v1:list-for-provider still always builds and fetches the /models endpoint unless modelsHint exists. So a manual/inference-only provider can pass the new connection test and then still fail model listing with “Cannot reach provider /models endpoint”.
That keeps one of the #216 false-negative paths alive in the Settings/model-picker flow, and it also leaves the new modelDiscoveryMode only partially honored.
Evidence:
apps/desktop/src/main/connection-ipc.ts:937apps/desktop/src/main/connection-ipc.ts:962apps/desktop/src/main/connection-ipc.ts:967
Suggested fix: resolve capabilities in models:v1:list-for-provider and branch before fetching:
static-hint: returnmodelsHintmanual: return a structured response the renderer can treat as manual entry, or return the default model as a hintmodels: fetch/models
At minimum, ifsupportsModelsEndpoint === false, do not attempt/models.
- [Medium] Agent runtime retry path does not pass
wireintoclassifyError()
completeWithRetry() now passes retryOpts.wire into classifyError(), which lets Anthropic gateway “messages API not implemented” errors avoid useless retries. The agent path uses withBackoff() directly but calls classifyError(err) without input.wire, so the same Anthropic-wire error will still retry in agent mode.
Evidence:
packages/core/src/agent.ts:925packages/core/src/agent.ts:929packages/providers/src/retry.ts:300
Suggested fix: call classifyError(err, input.wire) in the agent retry classifier and add a small regression test for anthropic-wire 5xx “not implemented” behavior.
Summary
Direction looks right for #216: provider config resolution, capabilities, runtime calls, and connection tests are much closer to one contract now. I would block on the first two because they affect real imported-provider behavior and the Settings flow. The third is smaller but worth fixing while this PR already touches retry parity.
Signed-off-by: Sun-sunshine06 <Sun-sunshine06@users.noreply.github.com>
Signed-off-by: Sun-sunshine06 <Sun-sunshine06@users.noreply.github.com>
There was a problem hiding this comment.
Findings
-
[Minor]
generateTitle()still dropsexplicitCapabilities— this new follow-up plumbs the field into the title IPC input, but the actualcompleteWithRetry()call still forwards only resolvedcapabilities, so imported OpenAI/OpenRouter rows on official bases keep the old reasoning-heuristic regression on title generationpackages/core/src/index.ts:853
Suggested fix:...(input.capabilities !== undefined ? { capabilities: input.capabilities } : {}), ...(input.explicitCapabilities !== undefined ? { explicitCapabilities: input.explicitCapabilities } : {}),
-
[Minor] Legacy
codesign:generatestill misses the same propagation — the new v1 generate path now passesexplicitCapabilities, but the compatibility shim kept for older renderers still callsrunGenerate()without it, so that one-minor-release fallback keeps the imported-provider reasoning bugapps/desktop/src/main/index.ts:888
Suggested fix:capabilities: active.capabilities, explicitCapabilities: active.explicitCapabilities, ...(allowKeyless ? { allowKeyless: true } : {}),
Summary
- Review mode: follow-up after new commits. Two compatibility gaps remain:
generateTitle()still dropsexplicitCapabilities, and the legacycodesign:generateshim still omits it. Not found in repo/docs in this checkout:docs/VISION.md,docs/PRINCIPLES.md.
Testing
- Not run (automation): static review only. PR code/tests were not executed per the review security policy.
open-codesign Bot
| ...(input.baseUrl !== undefined ? { baseUrl: input.baseUrl } : {}), | ||
| ...(input.wire !== undefined ? { wire: input.wire } : {}), | ||
| ...(input.httpHeaders !== undefined ? { httpHeaders: input.httpHeaders } : {}), | ||
| ...(input.capabilities !== undefined ? { capabilities: input.capabilities } : {}), |
There was a problem hiding this comment.
explicitCapabilities is added to GenerateTitleInput, but this call still forwards only resolved capabilities. For imported OpenAI/OpenRouter rows on the official base URL, title generation therefore keeps the old reasoning-heuristic regression even though generate()/applyComment() were updated.
Suggested fix:
...(input.capabilities !== undefined ? { capabilities: input.capabilities } : {}),
...(input.explicitCapabilities !== undefined
? { explicitCapabilities: input.explicitCapabilities }
: {}),| wire: active.wire, | ||
| ...(active.httpHeaders !== undefined ? { httpHeaders: active.httpHeaders } : {}), | ||
| capabilities: active.capabilities, | ||
| explicitCapabilities: active.explicitCapabilities, |
There was a problem hiding this comment.
This new path forwards explicitCapabilities, but the legacy codesign:generate shim below still omits it from runGenerate(...). Since that shim is intentionally kept for older renderers, those clients keep the imported-provider reasoning bug this PR is trying to close.
Suggested fix:
capabilities: active.capabilities,
explicitCapabilities: active.explicitCapabilities,
...(allowKeyless ? { allowKeyless: true } : {}),Signed-off-by: Sun-sunshine06 <Sun-sunshine06@users.noreply.github.com>
There was a problem hiding this comment.
Findings
- [Major]
explicitCapabilitiesstill stops at the provider boundary —runModel()now infers reasoning fromexplicitCapabilities, but the downstream provider call still forwards only resolvedcapabilities, andcomplete()only readsopts.capabilities. For imported OpenAI/OpenRouter rows on official hosts, normalgenerate()/applyComment()requests therefore still buildpiModel.reasoning=false, which keeps pi-ai on the non-official reasoning path even though the explicit override intentionally omittedsupportsReasoningpackages/core/src/index.ts:371,packages/providers/src/index.ts:68,packages/providers/src/index.ts:266,packages/providers/src/index.ts:277
Suggested fix:// packages/core/src/index.ts ...(input.explicitCapabilities !== undefined ? { explicitCapabilities: input.explicitCapabilities } : {}), // packages/providers/src/index.ts explicitCapabilities?: ProviderCapabilities; reasoning: inferReasoning( opts.wire, effectiveModelId, effectiveBaseUrl, opts.explicitCapabilities ?? opts.capabilities, model.provider, );
Summary
- Review mode: follow-up after new commits. One provider-layer gap remains: the explicit-capability fix still does not reach normal
generate()/applyComment()requests becausecomplete()never seesexplicitCapabilities. Not found in repo/docs in this checkout:docs/VISION.md,docs/PRINCIPLES.md.
Testing
- Not run (automation)
open-codesign Bot
| const effectiveBaseUrl = opts.baseUrl ?? piModel.baseUrl; | ||
| piModel = { | ||
| ...piModel, | ||
| reasoning: inferReasoning( |
There was a problem hiding this comment.
runModel() now distinguishes explicitCapabilities from resolved defaults, but the provider layer still only accepts/uses capabilities. For imported OpenAI/OpenRouter rows on official hosts, normal generate() / applyComment() calls therefore still build piModel.reasoning=false, so the explicit-override fix does not actually reach complete().
Suggested fix:
export interface GenerateOptions {
// ...
explicitCapabilities?: ProviderCapabilities;
}
reasoning: inferReasoning(
opts.wire,
effectiveModelId,
effectiveBaseUrl,
opts.explicitCapabilities ?? opts.capabilities,
model.provider,
)Signed-off-by: Sun-sunshine06 <Sun-sunshine06@users.noreply.github.com>
There was a problem hiding this comment.
Findings
- [Major]
runProviderTest()still returns a false positive when/modelssucceeds but the capability profile explicitly disables the wire's real inference endpoint. The newsupportsChatCompletions/supportsResponsesApichecks only run after a/models404 (or when/modelsis disabled), so Settings can still report "Connection OK" even though the first generation will fail on/chat/completionsor/responses. Evidence:apps/desktop/src/main/connection-ipc.ts:566,apps/desktop/src/main/connection-ipc.test.ts:1208,apps/desktop/src/main/connection-ipc.test.ts:1251.
Suggested fix:if (res.ok) { const unsupportedMessage = creds.wire === 'openai-chat' && creds.capabilities?.supportsChatCompletions === false ? 'Provider does not support Chat Completions API' : creds.wire === 'openai-responses' && creds.capabilities?.supportsResponsesApi === false ? 'Provider does not support Responses API' : null; if (unsupportedMessage !== null) { return { ok: false, code: 'NETWORK', message: unsupportedMessage, hint: 'Provider capability profile says this endpoint is not supported. Check wire selection.', diagnostics: makeConnectionDiagnostics( creds, 'models', [url], inferenceUrl === null ? [] : [inferenceUrl], ), }; } return { ok: true, probeMethod: 'models', diagnostics: ... }; }
Summary
- Review mode: follow-up after new commits. The
explicitCapabilitiespropagation fix looks addressed, but one connection-test parity gap remains: a provider can still pass onGET /modelseven when its capability profile says the actual wire endpoint is unsupported.
Testing
- Not run (automation): this runner has no installed workspace dependencies, and the checkout requires Node
>=22while the runner is on Nodev20.20.2. - Suggested regression coverage: add
runProviderTest()cases where/modelsreturns200whilesupportsChatCompletions=falseand where/modelsreturns200whilesupportsResponsesApi=false.
open-codesign Bot
| ), | ||
| }; | ||
| } | ||
| if (res.ok) { |
There was a problem hiding this comment.
supportsChatCompletions / supportsResponsesApi are only consulted after a /models 404 (or when /models is disabled). If /models returns 200 here, the connection test still reports success even when the capability profile explicitly says the real wire endpoint is unsupported, so Settings can say "Connection OK" while the first generation fails.
Suggested fix:
if (res.ok) {
const unsupportedMessage =
creds.wire === "openai-chat" && creds.capabilities?.supportsChatCompletions === false
? "Provider does not support Chat Completions API"
: creds.wire === "openai-responses" && creds.capabilities?.supportsResponsesApi === false
? "Provider does not support Responses API"
: null;
if (unsupportedMessage !== null) {
return {
ok: false,
code: "NETWORK",
message: unsupportedMessage,
hint: "Provider capability profile says this endpoint is not supported. Check wire selection.",
diagnostics: makeConnectionDiagnostics(creds, "models", [url], inferenceUrl === null ? [] : [inferenceUrl]),
};
}
}
|
补充看了一下最新这条 review，我的结论是：这更像一个后续可优化的连接测试策略问题，而不是当前 PR 的阻塞项。 这次 PR 主要修的是 provider capability 与真实 invocation 之间的语义对齐问题，尤其是：
这些主链路问题现在已经补齐，并且对应的回归测试也已经覆盖，当前 CI 也是通过的。 你这次提到的点，范围已经缩到
所以这里的确还存在一个“诊断比真实调用更宽松”的边角场景，但它成立的前提是 capability profile 本身处于一种自相矛盾的组合，例如：
这更像一个 follow-up 的连接测试收紧项，而不是当前 PR 新引入的 runtime/invocation regression。当前 PR 的核心问题已经解决，我认为这条不应继续阻塞合并。 如果后续要继续收紧 Settings 里的 connection parity，我们可以单独补一条 follow-up： |
…tion parity (#216)
Summary
Type of change
Linked issue
Checklist
docs/VISION.md,docs/PRINCIPLES.md, andCLAUDE.mdbefore startinggit commit -s)pnpm lint && pnpm typecheck && pnpm testpasses locallypnpm changeset) if user-visibleDependency additions (if any)
Screenshots / recordings (UI changes)