Validate custom output names before model registration

[tuanzirwar]

Passing an empty string or a non-string output_name currently registers a custom model without validating the value. The invalid entry remains in the registry, and retrying with the same model name is rejected as already registered.

This follows up on the registration-validation request in the review of #756: #756 (review). The check is missing from the merged implementation and the current main branch.

Validate output_name in CustomTextEmbedding.add_model before either registry is modified. Accept None and non-empty strings, preserving the exact name without trimming whitespace.

Regression tests cover four invalid values, unchanged existing registry entries, successful retry with the same name, and three accepted values. These tests do not download models.

Validation:

• Before the fix: the four invalid-value cases fail; three accepted-value cases pass.
• After the fix: 11 offline custom-model tests pass (7 new cases plus 4 existing registration/postprocessing cases).
• Ruff 0.3.4 check and format check pass using the repository's pyproject.toml; git diff --check passes.

The five tests requiring model downloads were not run locally.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 42792fce-d544-47a9-8338-391c46397c3c

📥 Commits

Reviewing files that changed from the base of the PR and between 0c7e33a and 1baba34.

📒 Files selected for processing (2)

• fastembed/text/custom_text_embedding.py
• tests/test_custom_models.py

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.

---

📝 Walkthrough

Walkthrough

CustomTextEmbedding.add_model now raises ValueError when output_name is a non-string value or an empty string. Tests verify that rejected values leave both registries unchanged, valid retries work, and None and non-empty strings are accepted and preserved.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~5 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 1baba

Custom model registration rejects invalid output names without leaving a partial registration. No actionable merge risk remains after normal checks.

Security Architecture Review

Security architecture risk: ⚪ Minimal · up to 1baba

Rejecting invalid values before registration prevents failed attempts from changing shared state. Valid values retain their existing behavior, and the change introduces no new access or privileges.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The changed behavior is bounded to caller-supplied output names entering class-level custom-model registries and their worker reconstruction. The guard rejects inputs before shared-state mutation and adds no new caller, authority transfer, or sensitive sink.

Trust Boundaries and Controls

• observed — Public registration retains case-insensitive duplicate-name rejection. Internal worker reconstruction bypasses that public check using the parent-provided configuration, as before; the new guard does not expand this existing authority.

Resilience and Maintainability Implications

• observed — The successful two-registry update still appends before assigning the mapping, without an explicit transaction or rollback. Repeated internal registration can append duplicate descriptions. These pre-existing transition limitations are not worsened by the new validation; invalid output names now fail before either operation.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: validating custom output names before model registration.
Description check	✅ Passed	The description explains the validation change, its expected behavior, regression tests, and reported test results.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[joein]

Thanks for following up on this, and for #756!

I'm going to close this one. Empty and non-string values already fail when the model loads, with ValueError: Output '' not found in the model, available outputs: [...]. That message is more useful than the new one, since it lists the outputs you can pick from. The str | None annotation already covers non-strings, and we don't type-check the other add_custom_model arguments at runtime either.