sgl-router/src/routers/openai/provider.rs (240-356)

A significant number of tests have been removed in this change. These tests covered important provider-specific logic, such as request transformations for different providers (OpenAI, XAI, Gemini) and provider registry lookups. Removing tests without replacing them with equivalent or better coverage is a regression and can lead to undetected bugs in the future.

Please restore these tests or move them to a more appropriate location if they have been relocated. If they are no longer relevant due to other changes, please provide a justification for their removal.

sgl-router/src/routers/openai/context.rs (60-92)

The accessor methods mcp_manager, response_storage, conversation_storage, and conversation_item_storage panic if called on the Shared variant. This can lead to unrecoverable runtime errors if the component type is not what's expected.

To improve safety, consider changing these methods to return an Option or Result. This would make potential failures explicit at the call site, allowing for more robust error handling. The caller would then use .expect() or ? to handle the None case, making the panic point explicit or allowing the error to be propagated.

    pub fn mcp_manager(&self) -> Option<&Arc<McpManager>> {
        match self {
            ComponentRefs::Shared(_) => None,
            ComponentRefs::Responses(r) => Some(&r.mcp_manager),
        }
    }

    pub fn response_storage(&self) -> Option<&Arc<dyn ResponseStorage>> {
        match self {
            ComponentRefs::Shared(_) => None,
            ComponentRefs::Responses(r) => Some(&r.response_storage),
        }
    }

    pub fn conversation_storage(&self) -> Option<&Arc<dyn ConversationStorage>> {
        match self {
            ComponentRefs::Shared(_) => None,
            ComponentRefs::Responses(r) => Some(&r.conversation_storage),
        }
    }

    pub fn conversation_item_storage(&self) -> Option<&Arc<dyn ConversationItemStorage>> {
        match self {
            ComponentRefs::Shared(_) => None,
            ComponentRefs::Responses(r) => Some(&r.conversation_item_storage),
        }
    }

sgl-router/src/routers/openai/context.rs (181-206)

The accessor methods worker, provider, payload, and take_payload use expect(), which will cause a panic if the required state is not set. This enforces a state machine-like flow but can make the code brittle if the state invariants are not perfectly maintained.

Similar to the feedback on ComponentRefs, consider returning an Option from these methods. This would move the responsibility of handling the missing state to the caller, making the code more robust and panics more explicit. The caller would then use .expect("..."), which clearly indicates a programmer error if the state is incorrect.

    pub fn worker(&self) -> Option<&Arc<dyn Worker>> {
        self.state.worker.as_ref().map(|w| &w.worker)
    }

    #[allow(dead_code)]
    pub fn provider(&self) -> Option<&dyn Provider> {
        self.state.worker.as_ref().map(|w| w.provider.as_ref())
    }

    pub fn payload(&self) -> Option<&PayloadState> {
        self.state.payload.as_ref()
    }

    pub fn take_payload(&mut self) -> Option<PayloadState> {
        self.state.payload.take()
    }

sgl-router/src/routers/openai/router.rs (247-258)

This change removes the detailed documentation comment for the select_worker_for_model function. The removed comment provided valuable information about the function's logic, priorities, and behavior, which is important for maintainability. Other useful inline comments explaining the logic have also been removed.

Please restore the documentation comment for select_worker_for_model and other removed explanatory comments to ensure the code remains easy to understand and maintain for other developers.