feat(entity): context-aware loaders + Membase graph-backed NER + per-request auth token#1361
Open
ywang1110 wants to merge 3 commits into
Open
feat(entity): context-aware loaders + Membase graph-backed NER + per-request auth token#1361ywang1110 wants to merge 3 commits into
ywang1110 wants to merge 3 commits into
Conversation
Contributor
Review Summary by QodoContext-aware entity loaders with Membase graph-backed NER and per-request auth
WalkthroughsDescription• Add context-aware entity data loaders with runtime parameters support • Implement Membase graph-backed NER loader for dynamic vocabulary/synonym loading • Enable per-request authentication tokens for Membase API calls • Extend entity analysis options with loader parameters configuration Diagramflowchart LR
A["EntityAnalysisOptions"] -->|"LoaderParameters"| B["EntityDataLoadContext"]
B -->|"Parameters dict"| C["IEntityDataLoader"]
C -->|"context-aware methods"| D["MembaseNERDataLoader"]
D -->|"graphId parameter"| E["IGraphDb"]
F["MembaseAuthHandler"] -->|"per-request token"| G["IConversationStateService"]
G -->|"TokenStateKey"| H["HTTP Request"]
File Changes1. src/Infrastructure/BotSharp.Abstraction/Entity/IEntityDataLoader.cs
|
Contributor
Code Review by Qodo
1. cached vocabulary returned directly
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+84
to
+87
| var key = VocabKey(graphId); | ||
| var cached = await _cache.GetAsync<Dictionary<string, HashSet<string>>>(key); | ||
| if (cached != null) return cached; | ||
|
|
Contributor
There was a problem hiding this comment.
1. cached vocabulary returned directly 📘 Rule violation ⛨ Security
LoadVocabularyByGraphIdAsync and LoadSynonymMappingByGraphIdAsync return cached dictionary instances directly, allowing downstream mutation to alter shared cached state across requests. This can lead to cross-request data leakage, nondeterministic entity results, and inconsistent synonym resolution between requests/components.
Agent Prompt
## Issue description
`LoadVocabularyByGraphIdAsync` and `LoadSynonymMappingByGraphIdAsync` currently return cached dictionaries by reference, which exposes shared mutable cached state to callers; downstream mutation can corrupt the cache and cause cross-request leakage, nondeterministic entity results, and inconsistent synonym resolution.
## Issue Context
Both methods read from `_cache` and return `cached` directly when present. For the vocabulary loader, the cached value is a `Dictionary<string, HashSet<string>>`, so a deep copy is needed (copy the dictionary and copy each `HashSet<string>`) when returning cached results (and ideally when storing into the cache as well) to prevent mutation of shared state. For the synonym mapping loader, even though the values are tuples, the dictionary remains mutable and should be cloned/copied before returning to prevent shared-state mutation.
## Fix Focus Areas
- src/Plugins/BotSharp.Plugin.FuzzySharp/Services/DataLoaders/MembaseNERDataLoader.cs[82-87]
- src/Plugins/BotSharp.Plugin.FuzzySharp/Services/DataLoaders/MembaseNERDataLoader.cs[163-166]
- src/Plugins/BotSharp.Plugin.FuzzySharp/Services/DataLoaders/MembaseNERDataLoader.cs[169-174]
- src/Plugins/BotSharp.Plugin.FuzzySharp/Services/DataLoaders/MembaseNERDataLoader.cs[198-200]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+29
to
31
| var token = ResolveToken(); | ||
| requestMessage.Headers.TryAddWithoutValidation("Authorization", $"Bearer {token}"); | ||
| var response = await base.SendAsync(requestMessage, cancellationToken).ConfigureAwait(false); |
Contributor
There was a problem hiding this comment.
2. Authorization token not validated 📘 Rule violation ☼ Reliability
MembaseAuthHandler may send an Authorization header with an empty bearer token when neither per-request state nor _settings.ApiKey is set. This hides configuration/client bugs and can lead to unpredictable authentication failures against Membase.
Agent Prompt
## Issue description
The outbound Membase request can be sent with an empty bearer token because the resolved token is not validated for null/empty.
## Issue Context
`ResolveToken()` falls back to `_settings.ApiKey` (which defaults to an empty string). `SendAsync` unconditionally sets `Authorization: Bearer {token}`.
## Fix Focus Areas
- src/Plugins/BotSharp.Plugin.Membase/Handlers/MembaseAuthHandler.cs[27-31]
- src/Plugins/BotSharp.Plugin.Membase/Handlers/MembaseAuthHandler.cs[41-55]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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
1 participant
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.
No description provided.