tidaldb/.sdlc/features/p1-quality-diversity-baseline/review.md

Code Review: Quality & Diversity Baseline

Summary

This feature adds a brief built-in ranking profile with quality gates and diversity constraints, fixes two bugs in the scoring pipeline that prevented format diversity from working, and adds 6 integration tests.

Files Changed

File	Change
tidal/src/ranking/builtins.rs	New brief() profile function, 5 constants, registration in register_builtins()
tidal/src/query/executor/mod.rs	Extended needs_metadata_for_creator_grouping to check format_mix_max_fraction; added format population in metadata enrichment loop
tidal/tests/p1_quality_diversity.rs	New file: 6 integration tests

Review Checklist

Correctness

• Quality gates: Two gates (view >= 3, completion >= 1) using SignalAgg::Value + Window::AllTime. Evaluated in passes_gates() before scoring. Correct aggregation type — Value reads the windowed count, which is what we want for threshold checks.
• Exploration disabled: BRIEF_EXPLORATION = 0.0. Correct decision — inject_exploration() runs after gate filtering and would re-introduce candidates that failed quality gates. Well-documented with a comment explaining the rationale.
• Format enrichment fix: The metadata enrichment loop now populates both creator_id and format from item metadata. The needs_metadata_for_creator_grouping flag correctly checks both max_per_creator and format_mix_max_fraction. Without this fix, format_mix_max_fraction was silently ignored because ScoredCandidate.format was always None.
• Profile structure: Follows the existing skeleton() pattern. Sort, gates, boosts, diversity all use the correct types.
• Registration: Added after date_saved() in register_builtins(). No ordering issues.

Code Quality

• Constants are well-named and documented: BRIEF_VIEW_GATE_THRESHOLD, BRIEF_COMPLETION_GATE_THRESHOLD, BRIEF_MAX_PER_CREATOR, BRIEF_FORMAT_MIX_MAX_FRACTION, BRIEF_EXPLORATION.
• The brief() function has a comprehensive doc comment explaining the profile's purpose.
• The exploration constant has a detailed comment explaining why it's 0.0.
• The collapsible-if pattern in mod.rs follows clippy's recommendation (let-chain).
• cargo clippy -D warnings passes cleanly.
• cargo fmt --check passes cleanly.

Test Quality

• 6 integration tests covering all specified scenarios.
• Tests use TidalDb::builder().ephemeral() — no disk I/O, fast execution (0.01s total).
• Helper functions are well-factored: brief_schema(), test_db(), write_item(), record_views(), record_completions(), write_quality_item(), retrieve_brief().
• Test assertions include descriptive failure messages.
• Creator diversity test accounts for the DiversitySelector architecture (diversity operates on full scored set, then pagination takes a slice).

Risks

• None identified. The change is additive (new profile, new test file). The pipeline fixes are backward-compatible — format was previously always None, so format-based diversity was a no-op before. Now it works as intended.

Verdict

PASS. Clean implementation, well-tested, no regressions. The two pipeline fixes (format enrichment and metadata loading trigger) are important bug fixes that benefit all profiles using format_mix_max_fraction, not just brief.