feat(libsy): advisor_gate review-gate algorithm - #371
Conversation
|
WalkthroughChangesAdvisor gate
Estimated code review effort: 4 (Complex) | ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@crates/switchyard-server/src/stats/algorithms.rs`:
- Line 31: Add concise Rust doc comments for the public advisor_gate field in
crates/switchyard-server/src/stats/algorithms.rs (lines 31-31), describing when
it appears in the serialized response. Also document every public field in
crates/switchyard-server/src/stats/algorithms/advisor_gate.rs (lines 42-53),
specifying each field’s count and grouping semantics.
In `@crates/switchyard-server/src/stats/algorithms/advisor_gate.rs`:
- Line 192: Update the stats binding in the test around StatsAccumulator::new to
be mutable, so the later StatsAccumulator::reset(&mut self) call compiles.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 585f7428-96a2-4510-9e3b-71963e67e64e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!Cargo.lock
📒 Files selected for processing (11)
Cargo.tomlcrates/libsy/Cargo.tomlcrates/libsy/src/algorithms.rscrates/libsy/src/algorithms/advisor_gate.rscrates/libsy/src/algorithms/util/llm_judge.rscrates/libsy/src/algorithms/util/prompts.rscrates/libsy/src/lib.rscrates/switchyard-server/src/config.rscrates/switchyard-server/src/stats/algorithms.rscrates/switchyard-server/src/stats/algorithms/advisor_gate.rscrates/switchyard-server/tests/server.rs
315d4b0 to
136bc21
Compare
|
@eric-liu-nvidia can you address coderabbit first |
nachiketb-nvidia
left a comment
nachiketb-nvidia
left a comment
There was a problem hiding this comment.
Let's break this up into 2 or 3 MRs
- one for core logic
- one more for potential telemetry
- one more for server side config changes
also, if we have any prompts, let's make sure they're also knobs
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn advisor_route_approve_flow_and_stats() -> TestResult { |
There was a problem hiding this comment.
Can you wrap all of your tests in a #[cfg(test)] mod tests {? That will ensure they don't get compiled unless you are running tests. Grep for those strings in most other files for an example.
There was a problem hiding this comment.
These live in tests/server.rs, which is a Cargo integration-test target — files under tests/ are compiled only by cargo test, so a #[cfg(test)] wrapper there is a no-op (the existing ~40 tests in this file are bare for the same reason; the #[cfg(test)] mod tests pattern you're pointing at applies to unit tests inside src/, which the libsy tests in this stack do use — advisor_gate/tests.rs is declared #[cfg(test)] mod tests;). Happy to wrap if you'd still prefer it for consistency.
There was a problem hiding this comment.
2k lines of code for one file is huge. May be try to move some utilities in the util folder and reduce the scope. Prompt should not live in the code file, we have prompts folder for that
There was a problem hiding this comment.
Done in 97ec7bd — the file is split into focused submodules: advisor_gate.rs (config, budget ledger, gate flow, ~640 lines), advisor_gate/turn.rs (turn buffering/inspection/replay), advisor_gate/transcript.rs (transcript serialization + verdict parsing), advisor_gate/telemetry.rs (metrics + audit lines), advisor_gate/tests.rs (the test suite). Prompts moved out of code into crates/libsy/src/prompts/advisor-gate/*.md via include_str!, same as capability-classifier. I used an advisor_gate/ submodule rather than algorithms/util/ because these helpers are gate-specific with a single consumer — util/ holds cross-algorithm shared code; happy to move any that become shared.
| fn has_tool_use(agg: &AggLlmResponse) -> bool { | ||
| agg.outputs.iter().any(|output| { | ||
| output.stop_reason == Some(StopReason::ToolUse) | ||
| || output | ||
| .content | ||
| .iter() | ||
| .any(|block| matches!(block, ContentBlock::ToolCall(_))) | ||
| }) | ||
| } | ||
|
|
||
| /// The turn's visible text: all text blocks joined; empty means none. | ||
| fn visible_text(agg: &AggLlmResponse) -> Option<String> { | ||
| let text: Vec<&str> = agg | ||
| .outputs | ||
| .iter() | ||
| .flat_map(|output| output.content.iter()) | ||
| .filter_map(|block| match block { | ||
| ContentBlock::Text { text } => Some(text.as_str()), | ||
| _ => None, | ||
| }) | ||
| .collect(); | ||
| if text.is_empty() { | ||
| return None; | ||
| } | ||
| let joined = text.join("\n"); | ||
| if joined.is_empty() { | ||
| None | ||
| } else { | ||
| Some(joined) | ||
| } | ||
| } |
There was a problem hiding this comment.
This can be util and removed from the main algo file
There was a problem hiding this comment.
Moved to advisor_gate/turn.rs in 97ec7bd (turn-inspection helpers live together there).
| fn reasoning_text(agg: &AggLlmResponse) -> Option<String> { | ||
| let text: Vec<&str> = agg | ||
| .outputs | ||
| .iter() | ||
| .flat_map(|output| output.content.iter()) | ||
| .filter_map(|block| match block { | ||
| ContentBlock::Reasoning { text, .. } => Some(text.as_str()), | ||
| _ => None, | ||
| }) | ||
| .collect(); | ||
| if text.is_empty() { | ||
| return None; | ||
| } | ||
| let joined = text.join("\n"); | ||
| if joined.is_empty() { | ||
| None | ||
| } else { | ||
| Some(joined) | ||
| } | ||
| } | ||
|
|
||
| /// Tool results carried by the conversation so far (both wires normalize | ||
| /// tool results into `ContentBlock::ToolResult`). | ||
| fn count_tool_results(messages: &[Message]) -> u32 { | ||
| let count = messages | ||
| .iter() | ||
| .flat_map(|message| message.content.iter()) | ||
| .filter(|block| matches!(block, ContentBlock::ToolResult(_))) | ||
| .count(); | ||
| u32::try_from(count).unwrap_or(u32::MAX) | ||
| } | ||
|
|
||
| /// Assistant turns already in the request — the stall checkpoint's clock. | ||
| fn assistant_turns(messages: &[Message]) -> u32 { | ||
| let count = messages | ||
| .iter() | ||
| .filter(|message| message.role == Role::Assistant) | ||
| .count(); | ||
| u32::try_from(count).unwrap_or(u32::MAX) | ||
| } |
There was a problem hiding this comment.
These too can move to util or some other file
There was a problem hiding this comment.
Moved to advisor_gate/turn.rs in 97ec7bd.
136bc21 to
e10e5f8
Compare
eric-liu-nvidia
changed the title
|
@nachiketb-nvidia done — split into a 3-PR stack along exactly those lines:
#382 targets this PR's branch and #383 targets #382's, so each diff shows only its own layer; they merge bottom-up. On prompts as knobs: they already are — @ayushag-nv CodeRabbit is addressed: the doc-comment suggestion is applied in #383, and the "does not compile" finding is incorrect — |
| //! whole instance — see [`budget_scope`]) is reviewed at most `max_reviews` | ||
| //! times; afterwards every call is a pure passthrough. | ||
| //! | ||
| //! This design is a near-superset of solo executor behavior: identical until |
There was a problem hiding this comment.
Do you have stats on when the algo claims executor to be "planning" ? How many turns and which turn is detected as needing advisor tool call?
| } | ||
|
|
||
| /// The trigger with its pattern compiled once at construction. | ||
| enum CompiledTrigger { |
There was a problem hiding this comment.
This should move to tool_signal.rs and can you use the existing signal buckets in addition to your patterns to detect planning mode? I have seen almost all turns using a tool call in TB 2.1 style tasks, curious what patterns you are detecting.
97ec7bd to
d50e1e7
Compare
Signed-off-by: zengyuanl <zengyuanl@nvidia.com>
Signed-off-by: zengyuanl <zengyuanl@nvidia.com>
Prompt contract review[P1] Preserve the requested deliverable on REDO — The reviewer prompt covers both a proposed plan and a completion claim, but every REDO tells the executor that the task is “NOT yet complete” and to “keep working until it is genuinely done.” [P2] Tell the reviewer when the transcript is truncated — The system prompt says the advisor receives “the full transcript” and “every action,” but |
6 participants
Stack 1/3 — this PR now contains only the core algorithm (libsy), per review. The rest of the original diff moved up the stack:
AdvisorGatelibsy algorithm + unit teststype = "advisor"server route config/v1/statsprojection + server e2e testsAdds the advisor review gate to the Rust server as a new route type:
type = "advisor".Benchmark results (Terminal-Bench 2.1)
With $64.76 ± $4.95 additional Opus 4.8 cost, the advisor review gate lifts Nemotron 3 Ultra accuracy by almost 25%.
How it works
There are two models: an executor (the model doing the work) and an advisor (a stronger model that checks the work).
REDO → the client never sees that reply. The proxy puts the advisor's feedback into the conversation ("not done yet — here is what is missing") and calls the executor again, so it keeps working.
max_reviews, default 1). After that, every call is a plain passthrough with zero overhead.If the advisor is down or replies with something unparseable, the gate fails open: the executor's reply goes to the client as if approved. Advisor problems never block the executor.
Why this shape: giving advice up front made the executor trust the plan and skip its own test-and-iterate loop. Checking only the final "I'm done" claim leaves the executor's normal behavior untouched and catches exactly one failure mode — stopping too early.
Example
Note for reviewers: this adds
regexas a production dependency of libsy (for the configurable review trigger pattern).🤖 Generated with Claude Code