fix(rule_engine): restore ranking inside tree retrieval (was flat-rating to 1.0)#169
fix(rule_engine): restore ranking inside tree retrieval (was flat-rating to 1.0)#169
Conversation
…ing to 1.0) apply_rules_with_tree previously hard-coded relevance=1.0 for every returned AppliedRule, silently bypassing the FSRS / scope-weight ranker. Council finding (council_2026-05-04T15-53-40.md, Skeptic perspective): this is a correctness bug — adding a token-budget cap on top of broken ranking just produces 'confidently-wrong rules with finer granularity'. Fix (option c, hybrid): tree retrieval now filters to a wide path-aware candidate pool (4x max_rules), then runs the same scoring stack as apply_rules over that pool — scope-weight relevance × CT boost, sorted by (state_priority, difficulty, relevance, effective_confidence). A small relevance floor (0.05) keeps tree- selected lessons in the result when their scope_json is empty. Backwards compat: when no lesson carries a path (legacy / pre- migration corpora), apply_rules_with_tree still delegates to apply_rules — locked in by tests/test_rule_engine_ranking_invariant.py. The 'path' column was added by the inline migration at src/gradata/_migrations/__init__.py:96 (lesson_transitions). Tests: - tests/test_tree_retrieval_ranking.py — three regression tests proving relevance is no longer flat-rated to 1.0 and ranking preserves confidence ordering. - tests/test_rule_engine_ranking_invariant.py — top-K + relevance equivalence between apply_rules and apply_rules_with_tree on a no-path corpus.
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughSummaryBug Fix: Fixed Key Changes:
Tests Added:
Risk: Low; improves correctness for path-bearing corpora, unchanged behavior for legacy corpora. Walkthrough
ChangesRule Engine Ranking Refinement
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.20.0)OpenGrep fatal error (exit code 2): �[32m✔�[39m �[1mOpengrep OSS�[0m �[1m Loading rules from local config...�[0m Comment |
Summary
apply_rules_with_treewas hard-codingrelevance=1.0for every returnedAppliedRule(src/gradata/rules/rule_engine/_engine.py:444), silently bypassing FSRS / scope-weight ranking.Council finding (
~/.hermes/council_reports/council_2026-05-04T15-53-40.md, Skeptic): this is a correctness bug — adding a token-budget cap on top of broken ranking would just buy 'confidently-wrong rules with finer granularity'. Must fix before any cap work.Fix (option c — hybrid)
(state_priority, difficulty, relevance, effective_confidence).rule_graphmatchesapply_rulesparity.scope_jsonis empty.Migration
The
pathcolumn onlesson_transitionswas added by the inline migration atsrc/gradata/_migrations/__init__.py:96— that migration was the trigger for the tree-retrieval shortcut.Test plan
tests/test_tree_retrieval_ranking.py(3 new): proves relevance is no longer flat-rated to 1.0 and ranking preserves confidence ordering. Confirmed FAILING on pre-fix code, PASSING after fix.tests/test_rule_engine_ranking_invariant.py(2 new): legacy-compat — top-K and relevance scores must match betweenapply_rulesandapply_rules_with_treeon a no-path corpus.pytest -k 'rule or tree').Layering check
No Layer 0 → Layer 2 imports introduced. Public
apply_rulessignature unchanged.Risk
Low. Tree path now produces better ranking on path-bearing corpora; legacy (no-path) corpora unchanged via fallback delegation.