# Review Provider

> Review a Merino suggest provider implementation against codebase patterns and best practices

- Skill: `mozilla-services/review-provider` (Agent Skill)
- Install (CLI): `npx skillmds@latest add mozilla-services/review-provider`
- Raw SKILL.md: https://api.skillmd.com/api/skills/mozilla-services/review-provider/raw
- Safety review: pending
- Works with: Claude Code, Claude.ai, OpenAI Codex
- Category: Coding & Dev Tools
- Author: mozilla-services (https://skillmd.com/u/mozilla-services)
- Updated: 2026-09-17
- Page: https://skillmd.com/skills/mozilla-services/review-provider

---


# Review a Suggest Provider

Review the provider specified in $ARGUMENTS against Merino's patterns and best practices. If no provider is specified, ask which provider to review.

## Step 1: Read the provider implementation

Read all files in `apps/merino/merino/providers/suggest/{provider_name}/` including:
- `provider.py`
- `backends/protocol.py` (if exists)
- All backend implementations
- Any other files in the directory

Also read the provider's registration in `apps/merino/merino/providers/suggest/manager.py` (find the matching `ProviderType` and `case` block).

Read the provider's config section in `apps/merino/merino/configs/default.toml`.

Read tests in `apps/merino/tests/unit/providers/suggest/{provider_name}/` and `apps/merino/tests/integration/providers/suggest/{provider_name}/`.

## Step 2: Check against patterns

Review each of these areas and report findings. For each area, state PASS, WARN, or FAIL with an explanation.

### Provider Class Structure
- [ ] Class is named `Provider` and extends `BaseProvider`
- [ ] `__init__` stores `self._name`, `self._enabled_by_default`, `self._query_timeout_sec`
- [ ] `__init__` calls `super().__init__()` (at end, without arguments, or with `**kwargs`)
- [ ] `initialize()` is async
- [ ] `query()` returns `list[BaseSuggestion]`
- [ ] `shutdown()` is implemented (even if no-op)
- [ ] `logger = logging.getLogger(__name__)` at module level

### Error Handling
- [ ] `query()` catches backend errors and returns `[]` instead of crashing
- [ ] Errors are logged at `warning` level (not `error` unless truly fatal)
- [ ] If calling external APIs: does it need a circuit breaker? (check if Weather/FlightAware pattern applies)
- [ ] No bare `except:` clauses (should catch specific exceptions)

### Query Normalization
- [ ] `normalize_query()` is implemented if the provider uses keyword matching
- [ ] At minimum strips whitespace and lowercases

### Metrics
- [ ] Uses `metrics_client` for timing external calls (e.g., `self.metrics_client.timeit(...)`)
- [ ] Provider-level metrics are tracked

### Caching (if applicable)
- [ ] Uses `RedisAdapter` / `NoCacheAdapter` pattern from manager
- [ ] Cache TTLs are configurable via settings

### Configuration
- [ ] Provider section exists in `default.toml` with `type`, `backend`, `enabled_by_default`, `score`, `query_timeout_sec`
- [ ] API keys/URLs in separate section (not under `providers.*`)
- [ ] Registered in `ProviderType` enum
- [ ] Has a `case` in `_create_provider()` factory

### Custom Details
- [ ] If returning extra fields: uses `CustomDetails` pattern (not top-level fields on `BaseSuggestion`)
- [ ] Details model defined in `custom_details.py`

### Backend Protocol (if applicable)
- [ ] Backend interface defined as a `Protocol` class
- [ ] Fake backend exists for testing
- [ ] Backend methods raise `BackendError` subclass on failures

### Testing
- [ ] Unit tests exist for the provider
- [ ] Tests cover: initialization, happy path query, error handling, query normalization
- [ ] Backend mocked with `mocker.AsyncMock(spec=BackendProtocol)`
- [ ] All async tests have `@pytest.mark.asyncio`
- [ ] Tests use fixtures from conftest, not inline setup

### Data Refresh (if applicable)
- [ ] Uses `packages/merino-common/merino_common/utils/cron.py` `Job` class for periodic refresh
- [ ] Cron task created with `asyncio.create_task()` in `initialize()`
- [ ] Has a `_should_fetch()` condition function
- [ ] Handles fetch failures gracefully (logs, doesn't crash)

## Step 3: Report

Present findings as a checklist with PASS/WARN/FAIL for each area. Include:
- A summary of the provider's health (good / needs attention / significant issues)
- Specific actionable recommendations for any WARN or FAIL items
- Code snippets showing exactly what to fix, referencing existing providers as examples