refactor(fim): introduce FimHandler interface for cleaner FIM support #4845
Conversation
- Add FimHandler interface with streamFim, getModel, and getTotalCost methods - Add fimSupport() method to ApiHandler that returns FimHandler | undefined - Refactor MistralHandler to implement fimSupport() pattern - Refactor KilocodeOpenrouterHandler to implement fimSupport() pattern - Update GhostModel to use getFimHandler() helper function - Keep supportsFim() as deprecated for backward compatibility - Update tests to use the new fimSupport() API This structural improvement replaces the boolean supportsFim() check with a more idiomatic pattern where fimSupport() returns the handler directly if FIM is supported, or undefined if not.
|
…ity code - Remove supportsFim() method from MistralHandler and KilocodeOpenrouterHandler - Remove backward compatibility fallback in GhostModel's getFimHandler() - Remove tests for deprecated supportsFim() method - Clean up comments that referenced the old pattern
Code Review SummaryStatus: No Issues Found | Recommendation: Merge This PR refactors the FIM (Fill-In-the-Middle) support from a duck-typing approach to a proper interface-based design. The changes are well-structured and follow good TypeScript practices. Key Improvements
Files Reviewed (7 files)
|
| // Return a FimHandler implementation | ||
| return { | ||
| streamFim: this.streamFim.bind(this), | ||
| getModel: () => { | ||
| const { id, info, maxTokens } = this.getModel() | ||
| return { id, info, maxTokens } | ||
| }, | ||
| getTotalCost: (usage: CompletionUsage) => this.getTotalCost(usage), | ||
| } |
There was a problem hiding this comment.
| // Return a FimHandler implementation | |
| return { | |
| streamFim: this.streamFim.bind(this), | |
| getModel: () => { | |
| const { id, info, maxTokens } = this.getModel() | |
| return { id, info, maxTokens } | |
| }, | |
| getTotalCost: (usage: CompletionUsage) => this.getTotalCost(usage), | |
| } | |
| return this |
isn't enough?
| } | ||
|
|
||
| async *streamFim( | ||
| private async *streamFim( |
There was a problem hiding this comment.
well, without this change anyhow if you want to return this above
| import { KilocodeOpenrouterHandler } from "./providers/kilocode-openrouter" | ||
| import { InceptionLabsHandler } from "./providers/inception" | ||
| import type { FimHandler } from "./providers/kilocode/FimHandler" // kilocode_change | ||
| export type { FimHandler } from "./providers/kilocode/FimHandler" |
There was a problem hiding this comment.
any reason to export this here other than everyone/Opus does it?
There was a problem hiding this comment.
i assumed this to be best practice al this is the index file, and also has a barrel function. That's not the case?
Summary
This PR introduces a structural improvement to the FIM (Fill-In-the-Middle) support by replacing the boolean
supportsFim()check with a more idiomaticfimSupport()method that returns aFimHandlerinterface.Changes
New
FimHandlerInterfaceNew
fimSupport()Method onApiHandlerThis replaces the previous pattern of:
With the cleaner:
Files Changed
src/api/providers/kilocode/FimHandler.ts- New file defining theFimHandlerinterfacesrc/api/index.ts- AddedfimSupportoptional method toApiHandlerinterfacesrc/api/providers/mistral.ts- ImplementedfimSupport()patternsrc/api/providers/kilocode-openrouter.ts- ImplementedfimSupport()patternsrc/services/ghost/GhostModel.ts- Updated to usegetFimHandler()helpersrc/api/providers/__tests__/mistral-fim.spec.ts- Updated testssrc/api/providers/__tests__/kilocode-openrouter.spec.ts- Updated testsBackward Compatibility
The
supportsFim()method is kept as deprecated for backward compatibility. It now simply returnsthis.fimSupport() !== undefined.Related
This is a follow-up to #3856 which added FIM support for Mistral/Codestral models. This PR addresses the structural improvement suggested in the review comments.