docs(internal): correct the reference-graph ranking comment - #2285
Conversation
_GRAPH_HIT_SCORE said fuzzy scores are capped at 100, so a confirmed reference ranks at or above every fuzzy name match. The cap lives in _search_in_dict and bounds the config-body score. The fuzzy name score comes from _calculate_entity_score, which accumulates its bonuses with no ceiling, so a name echoing the queried entity's words can pass 100 on its own and outrank a graph hit's fixed 100. The exact-mode half of the claim holds and stays. The comment now states what fuzzy mode does, scoped to graph-ONLY hits (_merge_graph_hits leaves an already-scored record's score alone) and to the case where visibility enforcement does not scrub the record. Also name the scene caller in _individual_fetch_budgeted's deprioritize paragraph: it passes the set too, alongside automation and script.
|
@codex review — apply the review criteria in .gemini/styleguide.md in addition to AGENTS.md guidance |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe change updates smart-search documentation. It adds scene callers to the budget-ordering description and clarifies fuzzy-search ranking, score behavior, and ChangesSmart search documentation
Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to This change only corrects comments and docstring wording without changing runtime behavior, so no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Description checkExplanation The description explains both documentation corrections, states that behavior is unchanged, identifies the testing performed, and completes the relevant template sections. The unchecked broader test and lint items are acceptable for this documentation-only change. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
The comment said no value of _GRAPH_HIT_SCORE makes fuzzy mode symmetric, and rested that on _calculate_entity_score having no ceiling. The absent clamp does not carry the claim: that score is a sum of bounded parts, so a large enough constant would dominate it. What actually rules such a constant out is the exact-mode property the paragraph above asserts - every survivor scores 100 there, and raising the constant breaks that tie. Name the condition instead of leaving it implicit.
|
Choices Made: The comment on The replacement speaks about graph-only hits because The Problems Encountered: The original claim was plausible because a 100 cap does exist — it just belongs to a different scorer. My first replacement traded one overstatement for another. It said no value of the constant makes fuzzy mode symmetric, and rested that on the missing clamp — but a sum of bounded parts is itself bounded, so a large enough constant would dominate the name score. What actually rules such a value out is the exact-mode tie asserted one paragraph above: I also built and then dropped a change to the code this comment describes: skipping the deprioritization when The code comment carried this one round longer than the reasoning did: it still said the name score accumulates "without a ceiling" after the paragraph above had already recorded that a sum of bounded parts is bounded. The same loose wording was in this summary's own description of |
|
@codex review — apply the review criteria in .gemini/styleguide.md in addition to AGENTS.md guidance |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Merging master in for you to clear the dead ci. I cannot officially review till tonight or tomorrow though |
kingpanther13
left a comment
kingpanther13
left a comment
There was a problem hiding this comment.
One accuracy correction is needed in src/ha_mcp/tools/smart_search/_graph.py.
The new comment says _calculate_entity_score “accumulates without a ceiling.” That is not accurate: the function is a finite sum of bounded 0–100 similarity values and fixed bonuses. The intended distinction is that its result is not capped at 100.
Please change this to “is not capped at 100” (or equivalent). The rest of the diff is accurate.
…unded The comment described `_calculate_entity_score` as accumulating "without a ceiling". The function is a finite sum of bounded terms — three `calculate_*_ratio` helpers that each return 0-100, plus fixed exact/partial and keyword bonuses — so a bound does exist. What matters for the surrounding argument is only that the bound is not 100, which is what the sentence now says. The rest of the comment is unchanged.
|
Fixed in d54ae3b. The comment now says You are right about the shape of the function. Every term is either a fixed bonus or the single weighted term The same claim had leaked into two other places, so I corrected those as well. The PR body said the score accumulates "with no clamp" and the implementation summary said it "returns it with no ceiling"; both now say it is not capped at 100. The code change is the two comment lines and nothing else, so the AST is unchanged. I also appended a note to the implementation summary recording that the comment kept the loose wording after the reasoning there had already ruled it out. |
kingpanther13
left a comment
kingpanther13
left a comment
There was a problem hiding this comment.
The requested accuracy correction is addressed: the comment now correctly says _calculate_entity_score is not capped at 100, rather than claiming it has no ceiling. I rechecked the full current diff and found no remaining concerns. Approved.
🧪 Your changes are now in the dev channel!Your PR has been merged to master and is available for testing in the dev channel. Test your changes before the next stable release (biweekly Wednesday): Quick start# Run dev version
uvx ha-mcp-dev
# Check version
uvx ha-mcp-dev --versionDocker: docker pull ghcr.io/homeassistant-ai/ha-mcp:dev
docker run --rm -i \
-v ha-mcp-dev-data:/home/mcpuser/.ha-mcp \
-e HOMEASSISTANT_URL=http://your-ha:8123 \
-e HOMEASSISTANT_TOKEN=your_token \
ghcr.io/homeassistant-ai/ha-mcp:devFound an issue? Please open a new bug report and mention this PR for context. |
3 participants
What does this PR do?
Corrects two pieces of prose in
smart_searchthat state something the code does not do — one#comment and one docstring. No behaviour change; the AST is identical with docstrings stripped._GRAPH_HIT_SCORE's ranking claim. It says fuzzy scores are capped at 100, so a confirmed reference ranks at or above every fuzzy name match. The cap it describes is in_search_in_dictand bounds the config-body score. The fuzzy name score comes from_calculate_entity_score, which has a single return and is not capped at 100.This is not the common case — most unrelated names score well under 100 — and it does not need to be. One name echoing the queried entity's words is enough, and you can check this one yourself:
_calculate_entity_score("automation.kitchen_lights_evening", "Kitchen Lights Evening", "automation", "light.kitchen_ceiling")returns 118, against a graph hit's fixed 100. That is sufficient to push the confirmed reference below a config that merely resembles the query. The exact-mode half of the original claim holds and stays.The replacement comment is scoped to what I could verify: it speaks about graph-only hits, because
_merge_graph_hitsleaves an already-scored record's score alone, and it hedges the "still counted" clause, because_scrub_results_for_enforceruns after the merge and can drop a graph hit outright under visibility enforcement._individual_fetch_budgeted's deprioritize docstring names the automation and script callers; the scene branch passes the set too.This documents the fuzzy ordering rather than changing it. If you meant the stronger guarantee the comment originally claimed, that needs
match_in_referencesin the sort key in_paginate_and_build_response. Note that also touches exact mode, where graph hits tie with body hits at 100 and follow them within a bucket — across buckets the flattening order in_paginate_and_build_responsedominates, not the append order. That is a behaviour change, so it stayed out of this comment correction.One negative result worth recording, since it is about the code this comment describes. I built and then dropped a change that skipped the deprioritization when
include_config=True, on the theory that a caller asking for bodies wants the confirmed reference's body most. Measuring it killed the idea. In the case I ran — two automations, a budget affording one fetch — the current ordering returned two matches (a templated reference with its body, plus the graph hit) and the change returned one, because the unfetched config scores 0 and falls below threshold. Which id wins the budget is incidental to the change rather than caused by it, and the caller is told the scan was partial either way, so the reference goes unnamed rather than undisclosed. What the change does remove is the guarantee that non-graph ids get the budget first. Losing a reference is worse than returning one without its body, so the ordering as it stands is right. That experiment is not in this branch — treat it as an anecdote, not evidence.Type of change
Testing
Nothing to test — prose only.
tests/src/unit/test_search_related_graph.pycollects 41 and passes 41 with--maxfail=0. CI covers the rest.uv run pytest)uv run ruff check)Checklist
Summary by CodeRabbit