refactor: extract search_passages/cross_search_passages's shared cache-probe block (#605) - #636
Merged
Merged
Conversation
…e-probe block (#605) #534's registry search/cache duplication audit, fourth and final follow-up PR (first: #632, second: #605-search-vectorlane, third: #605-cache-layer-dedup). search_passages and cross_search_passages duplicated ~50 lines of lookup -> semantic-probe -> fill logic character-for-character but for one differing search-log field name (context vs contexts) and search_passages' span.record calls, which cross_search_passages has never had a span to make. Extracted into passage_search_cache_probe, parameterized by two logging closures (tracing field names are macro-time identifiers, so the differing field can't be shared inside one macro call) and an on-hit hook that search_passages uses to record its span and cross_search_passages leaves a no-op. Found while extracting, not fixed here: cross_search_passages carries no taguru.passage_search-equivalent span at all, so its cache-outcome attributes (ADR 0008 SS6) are invisible where search_passages' are visible. Giving cross searches their own span is a bigger design question (one span for the whole multi-target fan-out? per-target children?) than this duplication cleanup covers — worth a follow-up issue. Closes #605 Claude-Session: https://claude.ai/code/session_0198T2iKZ17g3tt2bdNq1ju8
Contributor
|
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 (2)
📝 WalkthroughWalkthrough検索キャッシュ探索を Changes検索キャッシュ探索
Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: ⚪ Minimal · up to This PR consolidates duplicated cache-probe logic without a reported behavior change; no actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant SearchAPI
participant passage_search_cache_probe
participant SemanticCache
SearchAPI->>passage_search_cache_probe: キャッシュキーと検索条件を渡す
passage_search_cache_probe->>SemanticCache: 完全一致またはセマンティック一致を探索
SemanticCache-->>passage_search_cache_probe: キャッシュ結果または補填情報
passage_search_cache_probe-->>SearchAPI: Answered または Fresh
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
search_passagesandcross_search_passagesduplicated ~50 lines of lookup → semantic-probe → fill logic character-for-character but for one differing search-log field name (contextvscontexts) andsearch_passages'span.recordcalls, whichcross_search_passageshas never had a span to make. Extracted intopassage_search_cache_probe, parameterized by two logging closures (tracingfield names are macro-time identifiers, so the differing field can't be shared inside one macro call) and an on-hit hook thatsearch_passagesuses to record its span andcross_search_passagesleaves a no-op.cross_search_passagescarries notaguru.passage_search-equivalent span at all, so its cache-outcome attributes (ADR 0008 §6) are invisible wheresearch_passages' are visible. Giving cross searches their own span is a bigger design question (one span for the whole multi-target fan-out? per-target children?) than this duplication cleanup covers — flagging as worth a follow-up issue rather than silently expanding this PR's scope.Test plan
cargo fmtcargo clippy --all-targets(clean)cargo test(full suite, all binaries green — http_api integration tests cover both handlers' exact/semantic cache paths andsearch_passages' ADR 0008 span attributes end-to-end, confirming the extraction preserved exact behavior for both)cargo mutants --in-diff): 2 caught + 2 unviable, 0 missedCloses #605
https://claude.ai/code/session_0198T2iKZ17g3tt2bdNq1ju8
Summary by CodeRabbit