[← Back to Reviews Index](../Stewards%20Reviews%20Index.md)

# Interface Design Review — Azure-AI-RAG-CSharp-Semantic-Kernel-Functions

| Field | Value |
|---|---|
| Project | Azure-AI-RAG-CSharp-Semantic-Kernel-Functions |
| Review Date | 2026-03-21 |
| Steward | Interface Design Steward |
| Critical | 0 |
| Notable | 6 |
| Minor | 3 |
| Info | 2 |
| Total | 11 |

---

## 1. Assessment / Overview

The ChatAPI project is a compact ASP.NET Core RAG (Retrieval-Augmented Generation) service backed by Azure OpenAI, Azure AI Search, and Azure Cosmos DB, orchestrated via Semantic Kernel. The codebase has **zero custom `I*.cs` interface definitions**. Every service and data-access class is registered and injected as a concrete type.

This is the defining architectural characteristic of the project: the entire application composes against concrete classes rather than abstractions. While this makes the code easy to follow, it creates a set of testability and substitution gaps that are meaningful for a production AI service where the core application logic (`ChatService`) directly depends on cloud infrastructure classes.

The audit covers all 11 `.cs` files in `src/ChatAPI`. No existing interface files were found to evaluate for naming, cohesion, or signature quality. All findings therefore concern the **absence** of interfaces where architectural value would be gained, and structural concerns on the concrete classes that would become interfaces.

---

## 2. Interface Inventory

No `I*.cs` files exist in `src/ChatAPI`. The table below lists all injectable classes and their DI registration mode.

| Class | Namespace | DI Registration | Injected Into |
|---|---|---|---|
| `ChatService` | `ChatAPI.Services` | `AddScoped<ChatService>()` | `ChatController` |
| `ProductData` | `ChatAPI.Data` | `AddScoped<ProductData>()` | `ProductDataPlugin`, `ChatService` (indirect) |
| `ChatHistoryData` | `ChatAPI.Data` | `AddScoped<ChatHistoryData>()` | `ChatService` |
| `AISearchData` | `ChatAPI.Data` | `AddScoped<AISearchData>()` | `AISearchDataPlugin` |
| `GenerateProductInfo` | `ChatAPI.Data` | `AddSingleton<GenerateProductInfo>()` | `PopulateData` startup method |
| `AISearchDataPlugin` | `ChatAPI.Plugins` | `AddFromType<AISearchDataPlugin>()` | Semantic Kernel |
| `ProductDataPlugin` | `ChatAPI.Plugins` | `AddFromType<ProductDataPlugin>()` | Semantic Kernel |
| `CosmosClient` | Azure SDK | `AddSingleton(...)` | `ProductData`, `ChatHistoryData`, `GenerateProductInfo` |
| `AzureOpenAIClient` | Azure SDK | `AddSingleton(...)` | SK internals |
| `SearchClient` | Azure SDK | `AddSingleton(...)` | `AISearchData` |
| `SearchIndexClient` | Azure SDK | `AddSingleton(...)` | (registered but not injected into any discovered class) |
| `ChatHistory` | Semantic Kernel | `AddSingleton(...)` | `ChatService` |

---

## 3. Naming Assessment

No custom interface names exist to evaluate. All class names are reasonably intention-revealing for their role:

- `ChatService` — clear service role.
- `ProductData` / `AISearchData` / `ChatHistoryData` — clear data-access naming, though the `Data` suffix blends repository and infrastructure responsibilities.
- `GenerateProductInfo` — name is verb-heavy and describes what it *does* at startup rather than what it *is*. A more intention-revealing name would be `ProductDataSeeder` or `CosmosProductSeeder`.

There are no vague `IManager`, `IService`, or `IHelper` names present, which is a positive baseline.

---

## 4. Cohesion Assessment

### `ChatService`

`ChatService` currently owns:
1. Chat history initialization from Cosmos DB.
2. User message persistence.
3. Semantic Kernel orchestration (function calling, chat completion).
4. Response serialization to JSON.

Concerns: Chat history hydration and message persistence are infrastructure concerns that are mixed directly into the orchestration flow. The class is not a god class, but it conflates "conversation state management" with "AI orchestration." If an interface were introduced for `ChatService`, these responsibilities would need to be cleanly separated or the interface would be over-broad.

### `ProductData` and `AISearchData`

Both classes have a single focused responsibility (data retrieval from one backend). The `ProductData.GetProductByNameAsync` method returns `null` on not-found rather than a domain result type — this is a signature quality issue (see findings).

### `ChatHistoryData`

Mixes two distinct responsibilities: (a) persisting individual messages to Cosmos DB and (b) reconstituting a `ChatHistory` (SK type) from Cosmos DB. The Semantic Kernel `ChatHistory` type is an infrastructure/SDK type; passing it directly in the public method signature (`AddMessageToHistoryAndSaveAsync(ChatHistory chatHistory, ...)` and `InitializeChatHistoryFromCosmosDBAsync(ChatHistory chatHistory, ...)`) leaks an SK infrastructure concern into what should be a storage abstraction.

### `GenerateProductInfo`

This is a data-seeder / startup utility. Its placement in `Data/sample_data/products/` is reasonable for a sample project, but it duplicates the `Product` model definition — `Product.cs` in the `Data/` root already defines `Product`, and `GenerateProductInfo.cs` redefines it in the same `ChatAPI.Data` namespace. This is a compilation-level duplication risk.

---

## 5. Signature Quality Assessment

### Missing `CancellationToken` on all async I/O methods

Every async method that performs I/O (Cosmos DB reads/writes, Azure AI Search queries) lacks a `CancellationToken` parameter. This affects:

- `ChatHistoryData.AddMessageToHistoryAndSaveAsync`
- `ChatHistoryData.InitializeChatHistoryFromCosmosDBAsync`
- `ChatHistoryData.GetMessagesBySessionIdAsync` (private)
- `ChatHistoryData.AddUserMessageAsync` (private)
- `ChatHistoryData.AddAssistantMessageAsync` (private)
- `ProductData.GetProductAsync`
- `ProductData.GetProductByNameAsync`
- `AISearchData.RetrieveDocumentationAsync`
- `GenerateProductInfo.PopulateCosmosAsync`
- `ChatService.GetResponseAsync`

For an ASP.NET Core API, all of these are on the request path. Without `CancellationToken`, client disconnection cannot be propagated to in-flight Cosmos DB and AI Search calls.

### Weak return type on `ProductData.GetProductByNameAsync`

Returns `Dictionary<string, object>` (actually `Dictionary<string, object>?` without annotation). This is a weakly-typed, anonymous result that does not model the domain. Both `GetProductAsync` and `GetProductByNameAsync` return `Dictionary<string, object>` — downstream callers cannot reason about the shape of the data without string-keyed lookups. Additionally, `GetProductByNameAsync` returns `null` on not-found without annotating the return type as nullable (`Dictionary<string, object>?`), which risks a `NullReferenceException` at the call site.

### Infrastructure SDK type exposed through `ChatHistoryData`'s public methods

`ChatHistoryData.AddMessageToHistoryAndSaveAsync` and `InitializeChatHistoryFromCosmosDBAsync` accept `Microsoft.SemanticKernel.ChatCompletion.ChatHistory` as a parameter. This makes the data-access class depend on a Semantic Kernel infrastructure type, creating a cross-cutting concern leak.

### `ChatController.ChatRequest` defined in the controller file

The `ChatRequest` DTO is defined in `ChatController.cs` with non-nullable properties (`string Input`, `string SessionId`) that lack nullability annotations. These should be `string?` or marked `required`.

---

## 6. Architectural Placement Assessment

The project has no explicit layer separation (no `Domain`, `Application`, `Infrastructure` folders). All classes live flat in `Services/`, `Data/`, `Plugins/`, and `Controllers/`. This is acceptable for a small sample project, but has concrete architectural consequences:

1. **Infrastructure types in application/data-access classes**: `CosmosClient`, `SearchClient`, and `ChatHistory` (SK) are injected directly into multiple classes. There are no ports or adapters — the application layer is the infrastructure layer.

2. **`SearchIndexClient` registered but unused**: `SearchIndexClient` is registered in `Program.cs` (line 53–55) but is not injected into any visible class. This is a dead registration — either a class was deleted or this registration was leftover from a prior iteration.

3. **`ChatHistory` registered as Singleton**: `ChatHistory` is registered as a singleton in DI. `ChatService` is registered as scoped. A scoped service holds a reference to a singleton `ChatHistory` instance. Since `ChatHistory` is a mutable, stateful object, this singleton means all concurrent requests share the same chat history object, which is a correctness defect in the current design (multi-user race condition).

---

## 7. One-Implementation Interface Analysis

Since there are no interfaces in the project, this section identifies the classes that **should** have interfaces from an architectural standpoint:

| Class | Reason interface is warranted | External boundary protected |
|---|---|---|
| `ChatService` | Injected into `ChatController` directly; no testability seam for the controller | No (but creates a unit-test gap) |
| `ProductData` | Wraps `CosmosClient`; an interface would allow test substitution without a live Cosmos connection | Yes — Azure Cosmos DB |
| `ChatHistoryData` | Wraps `CosmosClient`; same rationale as `ProductData` | Yes — Azure Cosmos DB |
| `AISearchData` | Wraps `SearchClient`; an interface would allow test substitution without a live AI Search connection | Yes — Azure AI Search |

The Semantic Kernel plugin classes (`AISearchDataPlugin`, `ProductDataPlugin`) are SK kernel-managed components. Introducing interfaces for them would provide limited value unless the plugins themselves are tested in isolation, which is a unit-test concern (see Unit Test Steward).

---

## 8. DI Alignment Assessment

| Class / SDK Type | Registered | Injected | Issue |
|---|---|---|---|
| `CosmosClient` | Yes (Singleton) | `ProductData`, `ChatHistoryData`, `GenerateProductInfo` | None |
| `AzureOpenAIClient` | Yes (Singleton) | SK internals | None |
| `SearchClient` | Yes (Singleton) | `AISearchData` | None |
| `SearchIndexClient` | Yes (Singleton) | None discovered | Dead registration — `INTF-DI-001` |
| `ChatHistory` | Yes (Singleton) | `ChatService` (Scoped) | Singleton mutable state injected into scoped service — `INTF-DI-002` |
| `ProductData` | Yes (Scoped) | `ProductDataPlugin` | None |
| `ChatHistoryData` | Yes (Scoped) | `ChatService` | None |
| `AISearchData` | Yes (Scoped) | `AISearchDataPlugin` | None |
| `ChatService` | Yes (Scoped) | `ChatController` | No interface — `INTF-MISS-001` |
| `GenerateProductInfo` | Yes (Singleton) | `PopulateData()` only | None |

---

## 9. Findings

| Severity | ID | Title | File |
|---|---|---|---|
| 🟡 Notable | INTF-MISS-001 | `ChatService` injected as concrete type with no interface seam | `Services/ChatService.cs` |
| 🟡 Notable | INTF-MISS-002 | `ProductData` has no interface — Cosmos DB boundary is not abstracted | `Data/ProductData.cs` |
| 🟡 Notable | INTF-MISS-003 | `ChatHistoryData` has no interface — Cosmos DB boundary is not abstracted | `Data/ChatHistoryData.cs` |
| 🟡 Notable | INTF-MISS-004 | `AISearchData` has no interface — AI Search boundary is not abstracted | `Data/AISearchData.cs` |
| 🟡 Notable | INTF-SIG-001 | All async I/O methods lack `CancellationToken` — client disconnection cannot propagate | Multiple files |
| 🟡 Notable | INTF-DI-002 | `ChatHistory` registered as Singleton, consumed by Scoped `ChatService` — mutable shared state | `Program.cs` |
| 🟢 Minor | INTF-SIG-002 | `ProductData.GetProductByNameAsync` returns unannotated nullable `Dictionary<string, object>` | `Data/ProductData.cs` |
| 🟢 Minor | INTF-SIG-003 | `ChatHistoryData` public methods accept `ChatHistory` (SK type) — infrastructure type leaks into data-access contract | `Data/ChatHistoryData.cs` |
| 🟢 Minor | INTF-DI-001 | `SearchIndexClient` registered in DI but never injected into any class | `Program.cs` |
| ℹ️ Info | INTF-STRCT-001 | `Product` class defined twice — in `Data/Product.cs` and `Data/sample_data/products/GenerateProductInfo.cs` | `Data/Product.cs`, `Data/sample_data/products/GenerateProductInfo.cs` |
| ℹ️ Info | INTF-STRCT-002 | `ChatRequest` DTO properties lack nullability annotations | `Controllers/ChatController.cs` |

---

## 10. Detailed Findings

### INTF-MISS-001 — `ChatService` injected as concrete type with no interface seam

**Why the interface is needed**: `ChatController` takes a direct constructor dependency on `ChatService`. Without an interface, the controller cannot be unit-tested without instantiating all of `ChatService`'s real dependencies (Semantic Kernel, Azure OpenAI, Cosmos DB). An `IChatService` interface would define the single public contract (`GetResponseAsync`) and allow the controller to be tested in isolation.

**Layer**: Application service layer.
**Who implements**: `ChatService`.
**Boundary protected**: Isolates the controller from AI orchestration infrastructure.

```csharp
public interface IChatService
{
    Task<string> GetResponseAsync(string question, string sessionId, CancellationToken cancellationToken = default);
}
```

---

### INTF-MISS-002 — `ProductData` has no interface

**Why the interface is needed**: `ProductData` wraps `CosmosClient`, an Azure infrastructure type. Without an interface, any class depending on `ProductData` (e.g., `ProductDataPlugin`) cannot be tested without a live Cosmos DB. An `IProductRepository` interface would be the correct port for this external boundary.

**Layer**: Infrastructure adapter; the interface belongs to the application layer.
**Who implements**: `ProductData`.
**Boundary protected**: Azure Cosmos DB.

```csharp
public interface IProductRepository
{
    Task<Dictionary<string, object>> GetProductAsync(string productId, CancellationToken cancellationToken = default);
    Task<Dictionary<string, object>?> GetProductByNameAsync(string productName, CancellationToken cancellationToken = default);
}
```

Note: The return type `Dictionary<string, object>` itself should be replaced with a domain type (`ProductDetail`) in a further refactoring.

---

### INTF-MISS-003 — `ChatHistoryData` has no interface

**Why the interface is needed**: Same rationale as `ProductData`. `ChatHistoryData` directly uses `CosmosClient`. An `IChatHistoryRepository` would abstract Cosmos DB away from `ChatService`.

**Layer**: Infrastructure adapter; interface belongs to application layer.
**Who implements**: `ChatHistoryData`.
**Boundary protected**: Azure Cosmos DB.

---

### INTF-MISS-004 — `AISearchData` has no interface

**Why the interface is needed**: `AISearchData` wraps `SearchClient` (Azure AI Search SDK). An `IDocumentSearchService` or `IDocumentRetriever` interface would make the plugin testable without live Azure AI Search.

**Layer**: Infrastructure adapter; interface belongs to application layer.
**Who implements**: `AISearchData`.
**Boundary protected**: Azure AI Search.

---

### INTF-SIG-001 — All async I/O methods lack `CancellationToken`

Every method on the request path that performs I/O omits `CancellationToken`. In an ASP.NET Core context, `HttpContext.RequestAborted` should be threaded through to Cosmos DB and AI Search calls. The most important entry point is `ChatService.GetResponseAsync` and the plugin `ResourceLookup` method — both are on the live request path and interact with multiple external services.

**Recommendation**: Add `CancellationToken cancellationToken = default` to all public async methods and thread it through to SDK calls (`ReadItemAsync`, `CreateItemAsync`, `ReadNextAsync`, `SearchAsync`, `GetChatMessageContentAsync`).

---

### INTF-DI-002 — `ChatHistory` Singleton consumed by Scoped `ChatService`

`ChatHistory` is registered as `AddSingleton(...)` (line 30, `Program.cs`). `ChatService` is `AddScoped<ChatService>()`. Each HTTP request gets its own `ChatService` instance, but they all share the same `ChatHistory` instance. Because `ChatHistory` is mutable, concurrent requests will corrupt each other's conversation state.

**Recommendation**: Either register `ChatHistory` as `AddScoped` (giving each request its own history), or remove the DI-registered singleton and construct `ChatHistory` within the scoped `ChatService`. The current `InitializeChatHistoryFromCosmosDBAsync` pattern suggests per-request history is the intent.

---

### INTF-SIG-002 — Unannotated nullable return in `GetProductByNameAsync`

`ProductData.GetProductByNameAsync` returns `null` when no product is found (line 61) but the declared return type is `Task<Dictionary<string, object>>` (non-nullable). This will suppress the compiler's null warning at call sites, risking `NullReferenceException`.

**Recommendation**: Change the return type to `Task<Dictionary<string, object>?>` and annotate with `#nullable enable` at the file or project level.

---

### INTF-SIG-003 — `ChatHistoryData` public API accepts `ChatHistory` (SK infrastructure type)

`AddMessageToHistoryAndSaveAsync` and `InitializeChatHistoryFromCosmosDBAsync` both take `Microsoft.SemanticKernel.ChatCompletion.ChatHistory` as a parameter. This couples the data-access class to a Semantic Kernel infrastructure type. If `ChatHistoryData` were abstracted behind an interface, this parameter would force the interface to import the SK package.

**Recommendation**: Refactor so that `ChatHistoryData` only persists and retrieves `ChatMessage` domain objects. The caller (`ChatService`) is responsible for mapping between `ChatMessage` and `ChatHistory`. This would decouple the persistence contract from the SK type.

---

### INTF-DI-001 — `SearchIndexClient` registered but never injected

`Program.cs` registers `SearchIndexClient` as a singleton (lines 53–55) but no constructor in any discovered class accepts it. This is likely dead code from a prior feature (possibly a search index population path).

**Recommendation**: Remove the registration, or document the intent if a future class will consume it.

---

### INTF-STRCT-001 — Duplicate `Product` class definition

`Product` is defined in both `Data/Product.cs` (root-level, no namespace) and `Data/sample_data/products/GenerateProductInfo.cs` (namespace `ChatAPI.Data`). The root-level definition has different property names (`Id`, `Name`, `Description`, `Rid`, `Self`, `Etag`, `Attachments`, `Timestamp`, `Paths`) versus the inner definition (`id`, `name`, `description`). This represents a stale or incomplete model. The global-scope `Product` in `Data/Product.cs` should be removed or merged.

---

### INTF-STRCT-002 — `ChatRequest` DTO properties lack nullability annotations

`ChatRequest.Input` and `ChatRequest.SessionId` are declared as `string` (non-nullable reference types) but will be `null` if the request body is missing those fields. The compiler cannot enforce non-nullability here without `[Required]` validation attributes or init-required properties.

**Recommendation**: Annotate as `string?` or add `[Required]` validation and enable model validation error responses.

---

## 11. Recommended Improvements

| Finding | Recommended Action | Priority |
|---|---|---|
| INTF-DI-002 | Change `ChatHistory` to scoped registration or construct it per-request inside `ChatService` | High |
| INTF-SIG-001 | Add `CancellationToken` to all public async I/O methods; thread through to SDK calls | High |
| INTF-MISS-001 | Introduce `IChatService` interface; register and inject via interface in `ChatController` | Medium |
| INTF-MISS-002 | Introduce `IProductRepository` interface; register `ProductData` as implementation | Medium |
| INTF-MISS-003 | Introduce `IChatHistoryRepository` interface; register `ChatHistoryData` as implementation | Medium |
| INTF-MISS-004 | Introduce `IDocumentRetriever` interface; register `AISearchData` as implementation | Medium |
| INTF-SIG-003 | Remove `ChatHistory` parameter from `ChatHistoryData` methods; return `ChatMessage` lists and let `ChatService` map to SK types | Medium |
| INTF-SIG-002 | Fix return type annotation on `GetProductByNameAsync` to `Task<Dictionary<string, object>?>` | Low |
| INTF-DI-001 | Remove dead `SearchIndexClient` DI registration from `Program.cs` | Low |
| INTF-STRCT-001 | Remove or consolidate the duplicate `Product` class in `Data/Product.cs` | Low |
| INTF-STRCT-002 | Add `[Required]` or nullable annotations to `ChatRequest` properties | Low |

---

## Footer

This review is based on static analysis of source files as of 2026-03-21. No build, runtime execution, or test run was performed. Findings reflect the code structure at the time of audit and may not capture runtime behavior.

Generated by: Interface Design Steward (`interface-steward.md`)