refactor(libsy): classifier::score returns the response alongside the score - #203
Conversation
WalkthroughChangesThe classifier scoring contract now returns Classifier response routing
Estimated code review effort: 3 (Moderate) | ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/libsy/src/core/classifier.rs (1)
97-102: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the served-response contract.
The public docs do not explain when
Some(Response)is valid or that it bypasses the target call. Document that it must accompany a deciding classification and becomes the final turn response.As per coding guidelines, add documentation for public Rust items.
🤖 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 `@crates/libsy/src/core/classifier.rs` around lines 97 - 102, Document the public async score method’s served-response contract: explain that Some(Response) is valid only with a deciding Classification, becomes the final turn response, and bypasses the target call. Add this Rust documentation directly to score while preserving its existing signature and behavior.Source: Coding guidelines
🧹 Nitpick comments (1)
crates/libsy/src/algorithms/fall_through.rs (1)
198-209: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a served-response short-circuit test.
No test returns
Some(Response)and proves the selected target is not invoked. Cover this path with a classifier-provided response and a target client that records or rejects calls, preventing regressions that reintroduce the duplicate paid turn.🤖 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 `@crates/libsy/src/algorithms/fall_through.rs` around lines 198 - 209, Add a test for the served-response branch in the fall-through algorithm, supplying a classifier-provided Some(Response) and a target client that records or rejects invocation. Assert the response is returned unchanged and the selected target is never called, while preserving existing coverage for the None path.
🤖 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.
Outside diff comments:
In `@crates/libsy/src/core/classifier.rs`:
- Around line 97-102: Document the public async score method’s served-response
contract: explain that Some(Response) is valid only with a deciding
Classification, becomes the final turn response, and bypasses the target call.
Add this Rust documentation directly to score while preserving its existing
signature and behavior.
---
Nitpick comments:
In `@crates/libsy/src/algorithms/fall_through.rs`:
- Around line 198-209: Add a test for the served-response branch in the
fall-through algorithm, supplying a classifier-provided Some(Response) and a
target client that records or rejects invocation. Assert the response is
returned unchanged and the selected target is never called, while preserving
existing coverage for the None path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e7cb54c7-e16d-4b50-afe7-852d351afc4b
📒 Files selected for processing (11)
crates/libsy/src/algorithms/fall_through.rscrates/libsy/src/algorithms/llm_class.rscrates/libsy/src/algorithms/rand.rscrates/libsy/src/algorithms/stage.rscrates/libsy/src/algorithms/subagent_affinity_tests.rscrates/libsy/src/algorithms/util/affinity.rscrates/libsy/src/algorithms/util/llm_judge.rscrates/libsy/src/algorithms/util/stage.rscrates/libsy/src/algorithms/util/subagent.rscrates/libsy/src/core/classifier.rscrates/libsy/tests/prompts.rs
|
Escalation feature should be a configuration knob in LLM classifier to optionally latch to a strong tier. Consider this trajectory: wwww(l)sssss
|
|
can you add escalation as a flag/composable config inside llm classifier config?
|
… score Signed-off-by: Lin Jia <linj@nvidia.com>
… PR 4 Signed-off-by: Lin Jia <linj@nvidia.com>
45caf7a to
15e3dd8
Compare
Signed-off-by: Lin Jia <linj@nvidia.com>
Extends the Classifier trait so score() returns (Classification, Option) instead of just Classification. The Option lets a classifier that already called a model hand back that response directly, so fall_through skips the outbound call and the turn is not paid for twice. All implementors updated. Part 2 of breaking up PR #172.
Summary by CodeRabbit
New Features
Bug Fixes