fix: Resolve backlog plan generation for Claude-compatible providers#809
Conversation
…sends model
When the Plan dialog sends a model (e.g. MiniMax-M2.1 from phase
settings), the server now:
- Calls getProviderByModelId() so the correct provider config
(baseUrl, credentials) is used for backlog plan generation.
- Falls back to getPhaseModelWithOverrides('backlogPlanningModel')
when model lookup finds no provider, so the phase's provider is
used when the model matches.
- Uses a plain system prompt instead of the claude_code preset when
a Claude-compatible provider is set; the preset is for native
Claude CLI and can break requests to MiniMax/GLM APIs.
Previously the request was sent to the default Anthropic endpoint
and/or used the preset, causing plan generation to fail for
MiniMax/GLM users.
Co-authored-by: Cursor <cursoragent@cursor.com>
Summary of ChangesHello @patrick-patel, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an issue where plan generation was failing for Claude-compatible providers like MiniMax due to incorrect provider resolution and system prompt formatting. The changes ensure that the system correctly identifies and utilizes the appropriate provider settings and formats requests correctly for these models, thereby restoring functionality for backlog plan generation. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
📝 WalkthroughWalkthroughAdded provider resolution logic to the backlog plan generation handler. When a model is explicitly provided or determined via phase overrides, the code resolves the associated Claude-compatible provider. If found, the handler bypasses the claude_code system prompt in favor of directly using the existing system prompt. Provider credentials are updated accordingly. Changes
Sequence DiagramsequenceDiagram
participant Caller
participant GeneratePlan as generate-plan handler
participant SettingsHelper as getProviderByModelId
participant PromptBuilder as Prompt Constructor
Caller->>GeneratePlan: Request backlog plan with model
alt Explicit model provided
GeneratePlan->>SettingsHelper: Resolve provider by model ID
SettingsHelper-->>GeneratePlan: Provider found (e.g., MiniMax, GLM)
GeneratePlan->>GeneratePlan: Set claudeCompatibleProvider
GeneratePlan->>GeneratePlan: Update credentials if needed
else No provider found
GeneratePlan->>GeneratePlan: Attempt fallback via phase overrides
end
alt Claude-compatible provider active
GeneratePlan->>PromptBuilder: Use existing systemPrompt directly
PromptBuilder-->>GeneratePlan: Bypass claude_code preset
else Native Claude provider
GeneratePlan->>PromptBuilder: Use claude_code preset-driven prompt
end
GeneratePlan-->>Caller: Return configured plan with appropriate prompt
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
There was a problem hiding this comment.
Code Review
This pull request effectively addresses the issue of backlog plan generation failing for Claude-compatible providers. The changes to resolve the provider and adjust the system prompt are logical and well-implemented. I have one suggestion to simplify the code by removing some redundant variable assignments, which should improve maintainability.
| if (providerResult.provider) { | ||
| claudeCompatibleProvider = providerResult.provider; | ||
| if (providerResult.credentials) { | ||
| credentials = providerResult.credentials; | ||
| } | ||
| } | ||
| // Fallback: use phase settings provider if model lookup found nothing (e.g. model | ||
| // string format differs from provider's model id, but backlog planning phase has providerId). | ||
| if (!claudeCompatibleProvider) { | ||
| const phaseResult = await getPhaseModelWithOverrides( | ||
| 'backlogPlanningModel', | ||
| settingsService, | ||
| projectPath, | ||
| '[BacklogPlan]' | ||
| ); | ||
| const phaseResolved = resolvePhaseModel(phaseResult.phaseModel); | ||
| if (phaseResult.provider && phaseResolved.model === effectiveModel) { | ||
| claudeCompatibleProvider = phaseResult.provider; | ||
| credentials = phaseResult.credentials ?? credentials; | ||
| } | ||
| } |
There was a problem hiding this comment.
The credentials variable is initialized on line 234, but then it's reassigned multiple times within this block with what appears to be the same value. The helper functions getProviderByModelId and getPhaseModelWithOverrides both retrieve the same global credentials object. You can simplify this logic by removing these redundant assignments, as the credentials object is passed to executeQuery later anyway.
if (providerResult.provider) {
claudeCompatibleProvider = providerResult.provider;
}
// Fallback: use phase settings provider if model lookup found nothing (e.g. model
// string format differs from provider's model id, but backlog planning phase has providerId).
if (!claudeCompatibleProvider) {
const phaseResult = await getPhaseModelWithOverrides(
'backlogPlanningModel',
settingsService,
projectPath,
'[BacklogPlan]'
);
const phaseResolved = resolvePhaseModel(phaseResult.phaseModel);
if (phaseResult.provider && phaseResolved.model === effectiveModel) {
claudeCompatibleProvider = phaseResult.provider;
}
}There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/server/src/routes/backlog-plan/generate-plan.ts (2)
235-263: Extract provider-resolution logic into a serviceThis block adds substantial provider/credential business logic inside the route handler. Please move it to
services/and keep the route focused on orchestration.As per coding guidelines, "Server business logic should be organized into services in the services/ directory, with Express route handlers in routes/ that delegate to services".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/backlog-plan/generate-plan.ts` around lines 235 - 263, Extract the provider/credential resolution logic from the route handler into a new service function (e.g., create a service in services/ like resolveProviderForModel) that encapsulates the sequence using getProviderByModelId, fallback to getPhaseModelWithOverrides and resolvePhaseModel, and returns { provider, credentials } given inputs (effectiveModel, settingsService, projectPath, and a context tag like '[BacklogPlan]'). In the route (generate-plan.ts) replace the inlined block that sets claudeCompatibleProvider and credentials with a single call to this new service and use its returned provider/credentials; ensure the new service handles both primary lookup and the phase fallback and preserves existing precedence (providerResult first, then phaseResult if model matches) and null-safety for settingsService.
38-39: ImportgetProviderByModelIdvia an@automaker/*packageThis new relative import adds another cross-module relative dependency in a TS server file. Please consume it from a shared
@automaker/*package export instead.As per coding guidelines, "Always import from shared packages (
@automaker/*), never from old paths or relative imports to other modules".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/backlog-plan/generate-plan.ts` around lines 38 - 39, Replace the relative import of getProviderByModelId with the shared package export: remove the '../../lib/settings-helpers.js' relative import and instead import getProviderByModelId from the appropriate `@automaker/`* package (e.g., `@automaker/settings-helpers` or the shared package that re-exports it); update the import statement that currently references getProviderByModelId so the symbol is consumed from the `@automaker` package and ensure any other references to getProviderByModelId remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/routes/backlog-plan/generate-plan.ts`:
- Around line 251-259: The equality check between phaseResolved.model and
effectiveModel can miss matches because phaseResolved.model may be an
unnormalized provider alias; update the comparison in the block handling
getPhaseModelWithOverrides / resolvePhaseModel so both sides are normalized via
resolveModelString (e.g., compute normalizedPhaseModel =
resolveModelString(phaseResolved.model) and normalizedEffective =
resolveModelString(effectiveModel)) and compare those normalized values before
assigning claudeCompatibleProvider; keep the rest of the logic intact.
---
Nitpick comments:
In `@apps/server/src/routes/backlog-plan/generate-plan.ts`:
- Around line 235-263: Extract the provider/credential resolution logic from the
route handler into a new service function (e.g., create a service in services/
like resolveProviderForModel) that encapsulates the sequence using
getProviderByModelId, fallback to getPhaseModelWithOverrides and
resolvePhaseModel, and returns { provider, credentials } given inputs
(effectiveModel, settingsService, projectPath, and a context tag like
'[BacklogPlan]'). In the route (generate-plan.ts) replace the inlined block that
sets claudeCompatibleProvider and credentials with a single call to this new
service and use its returned provider/credentials; ensure the new service
handles both primary lookup and the phase fallback and preserves existing
precedence (providerResult first, then phaseResult if model matches) and
null-safety for settingsService.
- Around line 38-39: Replace the relative import of getProviderByModelId with
the shared package export: remove the '../../lib/settings-helpers.js' relative
import and instead import getProviderByModelId from the appropriate `@automaker/`*
package (e.g., `@automaker/settings-helpers` or the shared package that re-exports
it); update the import statement that currently references getProviderByModelId
so the symbol is consumed from the `@automaker` package and ensure any other
references to getProviderByModelId remain unchanged.
| const phaseResult = await getPhaseModelWithOverrides( | ||
| 'backlogPlanningModel', | ||
| settingsService, | ||
| projectPath, | ||
| '[BacklogPlan]' | ||
| ); | ||
| const phaseResolved = resolvePhaseModel(phaseResult.phaseModel); | ||
| if (phaseResult.provider && phaseResolved.model === effectiveModel) { | ||
| claudeCompatibleProvider = phaseResult.provider; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/server/src/routes/backlog-plan/generate-plan.ts | sed -n '240,270p'Repository: AutoMaker-Org/automaker
Length of output: 1532
🏁 Script executed:
rg -n "resolveModelString|resolvePhaseModel" apps/server/src/routes/backlog-plan/generate-plan.ts -A 2 -B 2Repository: AutoMaker-Org/automaker
Length of output: 1320
🏁 Script executed:
fd -t f model-resolver.ts -o model-resolver.jsRepository: AutoMaker-Org/automaker
Length of output: 220
🏁 Script executed:
find . -path "*/model-resolver*" -type f | grep -E "\.(ts|tsx|js|jsx)$" | head -20Repository: AutoMaker-Org/automaker
Length of output: 275
🏁 Script executed:
cat -n ./libs/model-resolver/src/index.tsRepository: AutoMaker-Org/automaker
Length of output: 618
🏁 Script executed:
cat -n ./libs/model-resolver/src/resolver.ts | head -150Repository: AutoMaker-Org/automaker
Length of output: 6735
🏁 Script executed:
cat -n ./libs/model-resolver/src/resolver.ts | sed -n '150,250p'Repository: AutoMaker-Org/automaker
Length of output: 4325
🏁 Script executed:
rg -n "providerId" ./libs/model-resolver/src/resolver.ts | head -20Repository: AutoMaker-Org/automaker
Length of output: 646
🏁 Script executed:
rg -B5 -A5 "getPhaseModelWithOverrides" apps/server/src/routes/backlog-plan/generate-plan.ts | head -40Repository: AutoMaker-Org/automaker
Length of output: 1176
🏁 Script executed:
cat -n apps/server/src/lib/settings-helpers.ts | grep -A 30 "getPhaseModelWithOverrides"Repository: AutoMaker-Org/automaker
Length of output: 4144
🏁 Script executed:
rg -n "phaseModel.providerId|providerId.*phaseModel" apps/server/src -A 3 -B 3Repository: AutoMaker-Org/automaker
Length of output: 1298
Normalize model strings before comparison to handle alias/provider-model mismatches
When phaseModel.providerId is set, resolvePhaseModel() returns the model string unchanged (without normalization). This can cause the equality check at line 258 to fail when comparing against effectiveModel, which has been normalized through resolveModelString(). For example, if effectiveModel is "claude-3-5-sonnet-20241022" but phaseResolved.model is "sonnet" (from a provider configuration), the fallback provider won't be used despite both referring to the same model.
Normalize both values with resolveModelString() before comparing.
Proposed fix
-import { resolvePhaseModel } from '@automaker/model-resolver';
+import { resolvePhaseModel, resolveModelString } from '@automaker/model-resolver';
...
- const phaseResolved = resolvePhaseModel(phaseResult.phaseModel);
- if (phaseResult.provider && phaseResolved.model === effectiveModel) {
+ const phaseResolved = resolvePhaseModel(phaseResult.phaseModel);
+ const normalizedPhaseModel = resolveModelString(phaseResolved.model);
+ const normalizedEffectiveModel = resolveModelString(effectiveModel);
+ if (phaseResult.provider && normalizedPhaseModel === normalizedEffectiveModel) {📝 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.
| const phaseResult = await getPhaseModelWithOverrides( | |
| 'backlogPlanningModel', | |
| settingsService, | |
| projectPath, | |
| '[BacklogPlan]' | |
| ); | |
| const phaseResolved = resolvePhaseModel(phaseResult.phaseModel); | |
| if (phaseResult.provider && phaseResolved.model === effectiveModel) { | |
| claudeCompatibleProvider = phaseResult.provider; | |
| const phaseResult = await getPhaseModelWithOverrides( | |
| 'backlogPlanningModel', | |
| settingsService, | |
| projectPath, | |
| '[BacklogPlan]' | |
| ); | |
| const phaseResolved = resolvePhaseModel(phaseResult.phaseModel); | |
| const normalizedPhaseModel = resolveModelString(phaseResolved.model); | |
| const normalizedEffectiveModel = resolveModelString(effectiveModel); | |
| if (phaseResult.provider && normalizedPhaseModel === normalizedEffectiveModel) { | |
| claudeCompatibleProvider = phaseResult.provider; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/routes/backlog-plan/generate-plan.ts` around lines 251 - 259,
The equality check between phaseResolved.model and effectiveModel can miss
matches because phaseResolved.model may be an unnormalized provider alias;
update the comparison in the block handling getPhaseModelWithOverrides /
resolvePhaseModel so both sides are normalized via resolveModelString (e.g.,
compute normalizedPhaseModel = resolveModelString(phaseResolved.model) and
normalizedEffective = resolveModelString(effectiveModel)) and compare those
normalized values before assigning claudeCompatibleProvider; keep the rest of
the logic intact.
Summary
Plan generation was failing when using MiniMax or other Claude-compatible providers. This PR fixes provider resolution and request formatting in
apps/server/src/routes/backlog-plan/generate-plan.ts.getProviderByModelId()when the Plan dialog sends a model (e.g. MiniMax-M2.1) so the correct baseUrl and credentials are used instead of the default Anthropic endpointgetPhaseModelWithOverrides('backlogPlanningModel')when model lookup finds no provider, so the phase’s provider is used when the resolved model matchesclaude_codepreset when a Claude-compatible provider is set; the preset is for native Claude CLI and was breaking requests to MiniMax/GLM HTTP APIsTesting
npm run build:serverpassesnpm run format:checkpassesSummary by CodeRabbit
Release Notes