docs(contributing): add the two-tier provider acceptance rule - #1215
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChangesThe provider contribution guide now defines provider entry decisions, support tiers, configuration-only and code-based implementation paths, registration requirements, and tier-specific validation. Provider contribution guidance
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Closed
4 of 6 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CONTRIBUTING.md`:
- Around line 255-263: Update the streaming verification guidance in the
CONTRIBUTING documentation to account for providers where
supports_completion_streaming is false: explain that the provider must use a
code-based integration or configure the flag to False, and instruct contributors
to omit the stream=True check in that case. Keep the existing streaming example
for providers that support completion streaming.
- Line 243: Update the provider contribution guidance around the registry and
tests/conftest.py to make CI-matrix requirements conditional on support tier and
capabilities: verified config-only providers must be included in the CI matrix,
while other providers may omit it as appropriate. Require verified providers to
appear only in capability maps matching their supported features, and clarify
that unsupported capabilities such as embeddings do not need entries.
- Around line 280-281: Update the provider directory tree in CONTRIBUTING.md so
its root is src/any_llm/ rather than any_llm/, keeping the documented provider
structure aligned with the checklist path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b7cd416a-fc69-4daf-85df-2e182ad16a0e
📒 Files selected for processing (1)
CONTRIBUTING.md
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
CONTRIBUTING.md (2)
295-300: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDocument the
allextra requirement for code providers.The code-provider checklist adds an
LLMProvidermember and an optional dependency, but it does not require the dependency extra in theallgroup. The config-only section states thattests/unit/test_provider_pyproject_options.pychecks both for everyLLMProvidermember. A contributor can follow this checklist and fail CI. Add theall-group requirement here.Proposed wording
- [ ] Add to `pyproject.toml` optional dependencies + [ ] Add the provider extra to `pyproject.toml` optional dependencies and include the same extra in the `all` group🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CONTRIBUTING.md` around lines 295 - 300, Update the code-provider checklist in CONTRIBUTING.md to explicitly require adding the provider’s optional dependency to the pyproject.toml all extra, matching the validation performed by tests/unit/test_provider_pyproject_options.py.
284-290: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the
src/any_llm/root in the provider tree.The tree still starts at
any_llm/, but the checklist creates providers undersrc/any_llm/providers/. A contributor can create files outside the package. Change the tree root tosrc/any_llm/.Proposed wording
-any_llm/ +src/any_llm/🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CONTRIBUTING.md` around lines 284 - 290, Update the provider tree in CONTRIBUTING.md to start at src/any_llm/ instead of any_llm/, so the documented provider paths match the checklist location under src/any_llm/providers/.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CONTRIBUTING.md`:
- Around line 322-324: Update the “Add your test config to tests/conftest.py”
instruction so it explicitly applies only to verified-tier providers. State that
community-tier providers must skip these maps and provide the required live
verification output instead.
- Line 322: Update the provider verification guidance in CONTRIBUTING.md so
community-tier code providers are not directed only to the generic registry-row
example in section 2a. Add a provider-agnostic live-verification section or
explicitly instruct contributors to adapt commands for their registered provider
and verify only supported operations.
---
Outside diff comments:
In `@CONTRIBUTING.md`:
- Around line 295-300: Update the code-provider checklist in CONTRIBUTING.md to
explicitly require adding the provider’s optional dependency to the
pyproject.toml all extra, matching the validation performed by
tests/unit/test_provider_pyproject_options.py.
- Around line 284-290: Update the provider tree in CONTRIBUTING.md to start at
src/any_llm/ instead of any_llm/, so the documented provider paths match the
checklist location under src/any_llm/providers/.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6a17624e-1d04-44ec-a201-7c92a40d59a2
📒 Files selected for processing (1)
CONTRIBUTING.md
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
CONTRIBUTING.md (1)
326-327: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winMake community verification instructions implementation-aware.
Line 326 still directs every community provider to section [2a], which only shows registry-row setup. Code providers can require different construction and capability checks. Line 328 also still gives an unconditional instruction to add
tests/conftest.pyconfiguration.Direct community code providers to adapt a provider-agnostic verification example and test only supported operations. Limit the test-map instruction to verified providers. Tell community providers to skip those maps and provide live verification output.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CONTRIBUTING.md` around lines 326 - 327, Update the community-provider verification guidance near the referenced section so code providers follow the provider-agnostic verification example, adapt construction as needed, and test only supported capabilities; keep the registry-row path for config-only providers. Make adding tests/conftest.py maps conditional on verified-tier providers, while instructing community-tier providers to skip those maps and paste live verification output in the PR.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CONTRIBUTING.md`:
- Line 225: In the CONTRIBUTING.md guidance paragraph, remove the comma before
“because” in the sentence beginning “Registry rows waive it,” so it reads
“Registry rows waive it because a row costs one line to add and one line to
remove.”
---
Duplicate comments:
In `@CONTRIBUTING.md`:
- Around line 326-327: Update the community-provider verification guidance near
the referenced section so code providers follow the provider-agnostic
verification example, adapt construction as needed, and test only supported
capabilities; keep the registry-row path for config-only providers. Make adding
tests/conftest.py maps conditional on verified-tier providers, while instructing
community-tier providers to skip those maps and paste live verification output
in the PR.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e47ddfe0-9fc3-4bb2-a1fd-ce878dcd9fb0
📒 Files selected for processing (1)
CONTRIBUTING.md
Writes down the provider policy from #1197 so config-only gateway PRs have a documented path and the acceptance bar is consistent across reviews. - Adds a decision section: OpenAI-compatible endpoints need no PR at all (create_openai_compatible), config-only gateways get a registry row, and only protocol-driven behavior earns a code folder. - Documents the verified and community tiers as a support promise rather than a code-shape distinction, including the removal policy. - Specifies contribution-time live verification for community entries, since CI cannot use repository secrets on fork PRs. - Notes that a registry row still needs an LLMProvider entry for the "provider:model" string form to resolve. Also repairs stale references in the existing checklist: ProviderName in src/any_llm/provider.py is now LLMProvider in src/any_llm/constants.py, providers inherit AnyLLM rather than a Provider class, the __init__.py snippet is the provider package's own, and the renamed heading fixes a broken in-page anchor. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The first draft claimed a registry row needs no pyproject extra and no provider folder. Both are wrong, and a contributor following them would push a red PR: - test_provider_pyproject_options asserts every LLMProvider member has an optional-dependencies key and appears in the "all" group. - test_provider asserts a one-to-one mapping between LLMProvider members and directories under src/any_llm/providers/. Reproduced both failures by following the old text, then confirmed the corrected four-step recipe (row, enum member, empty extra plus "all", and a package __init__.py) leaves tests/unit green. Also: - Make the conftest guidance conditional on tier rather than on code shape, since a config-only provider can be verified tier, and only list maps for capabilities the provider supports (raised by CodeRabbit). - Describe community providers as skipping for lack of a configured key rather than being excluded from the matrix; the enum member does enroll them in the parametrization. - Cover unsupported streaming alongside unsupported model listing. - Lift the shim snippet out of the checklist item, where the nested fence rendered as a literal indented code block on GitHub. - Rename the section 2a placeholder to examplegw. "mygateway" is the name test_custom_name_is_not_an_enum_member asserts is never an enum member, so it was a poor choice for the section that adds one. Section 0 keeps it, matching the quickstart and that test's intent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…gaps Follow-up from an independent review pass on this PR. The first draft moved "substantial user base or unique capabilities" to verified-tier only, which dropped it from community code folders. That is backwards: a folder is the expensive category #1197 is about, so the bar now attaches to needing a folder in either tier, and registry rows waive it because a row is one line to add and one to remove. Other gaps the review found: - 2b omitted the "all" group from the pyproject step, the same defect that was already fixed in 2a. Both now name the asserting test. - The provider tree diagram said any_llm/ two lines above src/any_llm/. - Nothing said how a provider actually becomes verified. It takes a repo secret plus an EXPECTED_PROVIDERS entry in tests-integration.yaml, which forces the tests to run instead of skip. Both are maintainer actions, so contributors now know to open an issue rather than attempt it. - Described community providers as not in the CI provider list, rather than "excluded from the integration test matrix", which named no real mechanism. - Carved registry rows out of "provider integrations: comprehensive test suite required", since the generic registry tests already cover them. - Noted that the second <name>.py module in existing registry providers is a pre-migration compatibility shim, so the one-file layout in 2a does not read as inconsistent with the tree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three CodeRabbit findings on the previous commits. - The paragraph said only verified providers need the conftest maps, then the next line said "Add your test config" with no condition, so a community contributor would have filled them in anyway. - Community code providers were pointed at 2a for live verification without being told to adapt it; 2a's commands name a registry-row provider. They now say to substitute the provider name and drop unsupported operations. - Dropped a comma before an essential "because" clause. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
njbrake
force-pushed
the
docs/provider-acceptance-rule
branch
from
August 3, 2026 13:51
761b7c7 to
c566270
Compare
7 of 10 tasks
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CONTRIBUTING.md`:
- Line 287: Update the directory-tree code fence in CONTRIBUTING.md to use the
text language tag on its opening delimiter, changing the untagged fence to
```text while preserving the enclosed documentation.
- Line 203: Restore support for registry-only config providers by removing the
requirements for an empty pyproject.toml extra and a provider package directory
from the CONTRIBUTING.md guidance near the provider-tier requirements. Update
tests/unit/test_provider.py and tests/unit/test_provider_pyproject_options.py if
they enforce those requirements, while preserving the contract that a
config-only gateway needs only a registry entry and an LLMProvider entry.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ec22d620-e4ca-4f3a-b23c-c80616ad481e
📒 Files selected for processing (1)
CONTRIBUTING.md
|
|
||
| ### Provider Tiers | ||
|
|
||
| Every listed provider sits in one of two tiers. The tier is a support promise, not a statement about code shape: a config-only provider we hold keys for stays verified. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major
Restore the registry-only config path.
The guide states that support tier is independent of code shape. The PR objective also defines a config-only gateway as a registry row plus an LLMProvider entry. Lines 245-246 then require an empty pyproject.toml extra and a provider package directory.
Remove these two requirements. If tests/unit/test_provider.py or tests/unit/test_provider_pyproject_options.py still enforce them, update that contract before merging instead of documenting the workaround.
#!/bin/bash
set -euo pipefail
for file in \
tests/unit/test_provider.py \
tests/unit/test_provider_pyproject_options.py \
src/any_llm/providers/registry.py \
src/any_llm/constants.py \
pyproject.toml
do
if test -f "$file"; then
printf '\n--- %s ---\n' "$file"
rg -n -C 6 \
'PROVIDER_REGISTRY|LLMProvider|provider.*directory|optional.*depend|all.*group|extra' \
"$file" || true
fi
doneAlso applies to: 245-246
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CONTRIBUTING.md` at line 203, Restore support for registry-only config
providers by removing the requirements for an empty pyproject.toml extra and a
provider package directory from the CONTRIBUTING.md guidance near the
provider-tier requirements. Update tests/unit/test_provider.py and
tests/unit/test_provider_pyproject_options.py if they enforce those
requirements, while preserving the contract that a config-only gateway needs
only a registry entry and an LLMProvider entry.
|
|
||
| Implement the provider keeping this checklist in mind: | ||
|
|
||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language tag to the directory-tree fence.
markdownlint reports MD040 for this fence. Change the opening fence to ```text so the documentation check passes.
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 287-287: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CONTRIBUTING.md` at line 287, Update the directory-tree code fence in
CONTRIBUTING.md to use the text language tag on its opening delimiter, changing
the untagged fence to ```text while preserving the enclosed documentation.
Source: Linters/SAST tools
tbille
approved these changes
Aug 3, 2026
8 of 10 tasks
njbrake
added a commit
that referenced
this pull request
Aug 5, 2026
## Description Layer 3 of the #1197 stack, and the payoff for the other two. Docs only. #1215 documented the registry-row path honestly: four steps, because a row needed an `LLMProvider` member, a `pyproject.toml` extra plus `all` group entry, and a package `__init__.py` on top of the row itself. That was accurate but embarrassing, since #1197's whole pitch was "one row instead of touching six files". #1217 removed the reasons for the other three steps, so section 2a collapses to the row and the live verification evidence. Verified rather than assumed: I added a single row to `PROVIDER_REGISTRY` and nothing else, then confirmed the name appears in `get_supported_providers()`, resolves through `create()`, `"name:model"`, and `provider=`, reports the community tier, and leaves `tests/unit` plus `tests/docs` green. That check is also what surfaced the `get_provider_enum` bug fixed in #1217. Two notes kept in the text, because both would otherwise make the tree look like it contradicts the instructions: - The registry providers already in the repo carry a package directory and an `LLMProvider` member. Those are compatibility shims from before they were migrated to rows, kept so their deep-import paths and `LLMProvider.<NAME>` references keep working. When to retire them is still an open question on #1197. - A row is not added to the `tests/conftest.py` model maps. Promotion to the verified tier is a maintainer change and takes a repository secret, an `EXPECTED_PROVIDERS` entry, an `LLMProvider` member so the enum-driven test matrix picks the provider up, and conftest entries for the capabilities it supports. ## PR Type - 📚 Documentation ## Relevant issues Part of #1197. Together with #1215 this completes the CONTRIBUTING acceptance-rule item; the issue stays open for shim retirement and the registry-knobs question. ## Checklist - [x] I understand the code I am submitting. - [ ] I have added unit tests that prove my fix/feature works - [x] I have run this code locally and verified it fixes the issue. - [x] New and existing tests pass locally - [x] Documentation was updated where necessary - [x] I have read and followed the [contribution guidelines](https://github.com/mozilla-ai/any-llm/blob/main/CONTRIBUTING.md) - [x] **AI Usage:** - [ ] No AI was used. - [x] AI was used for drafting/refactoring. - [ ] This is fully AI-generated. No unit tests, since this is documentation only; the behavior it documents is tested in #1217. `tests/docs` only globs `docs/**/*.md`, so nothing in CI executes `CONTRIBUTING.md`, which is why the one-row claim was checked by hand against the real tree. Anchors, punctuation, and pre-commit verified. ## AI Usage Information - AI Model used: Claude Opus 5 - AI Developer Tool used: Claude Code - Any other info you'd like to share: Drafted by Claude through back and forth with @njbrake. The decisions are his; the prose is Claude's. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
8 of 11 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Writes down the provider policy from #1197 in CONTRIBUTING.md, so config-only gateway PRs have a documented path and the acceptance bar is consistent from one review to the next. Docs only, no code change.
The
Adding a New Providersection now leads with a decision step rather than assuming every provider needs a folder:AnyLLM.create_openai_compatible(...)(added in feat: add first-class OpenAI-compatible custom endpoint path #1198) already covers it. That is the right answer for private gateways and self-hosted servers, and it means nobody is ever blocked.src/any_llm/providers/registry.py(added in feat: add config registry for OpenAI-compatible gateway providers #1201), with no folder, nopyproject.tomlextra, and notests/conftest.pymodel maps.The rule is stated as the reviewer judging whether the protocol requires the code. Shipping an official SDK is not by itself a reason for a folder, and adding an override that is not needed does not turn a config-only gateway into a code provider.
Provider Tiersdocuments verified vs community as a support promise rather than a statement about code shape, so a config-only provider we hold keys for stays verified. Community entries are verified live by the contributor at PR time and excluded from the integration matrix; CI cannot use repository secrets on fork PRs, so contribution-time verification is the only bar that is actually enforceable. The removal policy is written down too, on the grounds that cheap addition is only sustainable if removal is equally cheap.Section 2a gives the concrete row, flag discipline (do not set a flag you have not exercised against the live endpoint), and a copy-pasteable verification script covering completion, streaming, and
list_models.Two details worth flagging for review:
AnyLLM.create("name")andget_provider_class("name"), but the"name:model"string form raisesUnsupportedProviderError, becausesplit_model_providerreturnsLLMProvider. I confirmed this by injecting a row with no enum member. The checklist therefore tells contributors to add theLLMProviderentry as well. Widening that type is still a follow-up on Provider policy: two tiers, a config registry, and a first-class OpenAI-compatible path #1197.Also repairs stale references in the existing checklist, since they sit in the section being rewritten:
ProviderNameinsrc/any_llm/provider.pyis nowLLMProviderinsrc/any_llm/constants.py(that file does not exist).AnyLLMfromany_llm.any_llm, not aProviderclass fromany_llm.provider.__init__.pysnippet used a non-existent import path; it is the provider package's own__init__.py.2. Implementation Checklistheading to2bbroke an in-page anchor further up the file, which is repointed.Happy to split the stale-reference cleanup into its own commit if you would rather review it separately.
PR Type
Relevant issues
Completes the
Update CONTRIBUTING.md with the acceptance ruleitem of #1197. Does not close the issue; the two-tier docs listing,provider:modelrouting for registry-only names, and the open questions about registry knobs and shim lifetime remain.Checklist
Notes on the checklist: no unit tests, since this is a documentation-only change.
tests/docsonly globsdocs/**/*.md, so the Python snippets in CONTRIBUTING.md are not executed by the suite; I validated them by hand against the real API instead (create_openai_compatiblesignature,AnyLLM.createwith an injected registry row, andModel.idfor thelist_modelsoutput).pre-commitandpytest tests/docsare clean.AI Usage Information
Summary by CodeRabbit