Skip to content

Comments

chore: plans for model facade overhaul#344

Open
nabinchha wants to merge 4 commits intomainfrom
nm/overhaul-model-facade-guts
Open

chore: plans for model facade overhaul#344
nabinchha wants to merge 4 commits intomainfrom
nm/overhaul-model-facade-guts

Conversation

@nabinchha
Copy link
Contributor

@nabinchha nabinchha commented Feb 19, 2026

  • Adds comprehensive plans to overhaul the guts of ModelFacade and under.
  • Currently only has the plan with snippets, but I'll iterate on the implementations in other prs.

part of #343

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Feb 19, 2026

Greptile Summary

This PR adds comprehensive planning documentation for overhauling the ModelFacade implementation by replacing LiteLLM with native provider adapters. The plans demonstrate thorough architectural design and include a feedback closure matrix addressing all reviewer concerns from previous iterations.

Key Changes:

  • Adds aggregated review document consolidating feedback from codex and opus reviewers
  • Includes detailed Step 1 plan (57KB) for migrating OpenAI-compatible and Anthropic adapters with explicit contracts for MCP compatibility, adaptive throttling, auth handling, and lifecycle management
  • Includes Step 2 plan for Bedrock support that extends Step 1 abstractions without forking patterns
  • Documents PR slicing strategy, testing approach, cutover gates, and rollback safety mechanisms

Strengths:

  • Exceptional level of detail with concrete implementation skeletons and type definitions
  • Addresses all HIGH and MEDIUM findings from the review document in the feedback closure matrix
  • Clear separation of concerns between provider-agnostic abstractions and provider-specific adapters
  • Well-defined parity requirements, testing strategy, and operational guardrails

This is documentation-only with no code changes, establishing a solid foundation for future implementation work.

Confidence Score: 5/5

  • This PR is safe to merge - it only adds planning documentation with no code changes
  • Documentation-only PR that adds comprehensive architectural plans for a future ModelFacade overhaul. No runtime code is modified, no dependencies changed, and no behavior altered. The plans show excellent engineering rigor with detailed design decisions, feedback closure, and testing strategies.
  • No files require special attention - all three files are well-structured planning documents

Important Files Changed

Filename Overview
plans/343/_review-model-facade-overhaul-plan.md Aggregated review document with findings from code reviews (codex and opus), identifies contradictions and missing contracts in the implementation plans
plans/343/model-facade-overhaul-plan-step-1.md Comprehensive migration plan for replacing LiteLLM with native adapters, includes feedback closure matrix addressing all reviewer findings
plans/343/model-facade-overhaul-plan-step-2-bedrock.md Bedrock adapter implementation plan that extends Step 1 abstractions, isolated to adapter/mapper/auth layers

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[ModelFacade Public API] --> B{Client Factory}
    B -->|provider_type=openai| C[OpenAI Compatible Adapter]
    B -->|provider_type=anthropic| D[Anthropic Adapter]
    B -->|provider_type=bedrock| E[Bedrock Adapter - Step 2]
    B -->|migration fallback| F[LiteLLM Bridge - Temporary]
    
    C --> G[Throttle Manager]
    D --> G
    E --> G
    F --> G
    
    G --> H[Retry Engine]
    H --> I[Provider HTTP APIs]
    
    J[Canonical Types] -.defines.-> C
    J -.defines.-> D
    J -.defines.-> E
    
    K[Auth Resolution] --> C
    K --> D
    K --> E
    
    subgraph "Step 1 - Non-Bedrock"
        C
        D
        F
    end
    
    subgraph "Step 2 - Bedrock"
        E
        L[Bedrock Mappers]
        E --> L
        L -->|claude/llama/nova/titan| M[AWS Bedrock Runtime]
    end
    
    style A fill:#4a9eff
    style B fill:#ffa94d
    style E fill:#51cf66
    style F fill:#ff6b6b
Loading

Last reviewed commit: f945d5b

| |
| +----------------------+ +-------------------------------+ |
| | Client Factory |-->| Adapter selected by | |
| | - provider_type | | provider_type | |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this new layer, it would be nice if we can start making the move to rename provider_type. It is confusing to see {"provider": "openai", "provider_type": "openai"}

- plans/343/model-facade-overhaul-plan-step-2-bedrock.md
---

# Aggregated Plan Review (Agreed Feedback)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nabinchha – "have your agent talk to my agent"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in f945d5b

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants