fix(tools): validate cached tool-embedding dimension (stale-dim after model swap poisons routing)#453
Closed
alichherawalla wants to merge 1 commit into
Closed
fix(tools): validate cached tool-embedding dimension (stale-dim after model swap poisons routing)#453alichherawalla wants to merge 1 commit into
alichherawalla wants to merge 1 commit into
Conversation
…ent model The tool-routing embedding cache keys entries on the tool TEXT (content hash), not the embedding model. If the embedding model's output dimension changes (a different MiniLM variant), the hash still matches and embedTool returned a STALE-DIMENSION vector. cosineSimilarity then looped on the query's length and read garbage from the shorter vector — silently poisoning tool routing after a model swap. Fix: require a cached vector to match the current model's dimension (the live query embedding's length) or re-embed it; and guard cosineSimilarity to return 0 on a dimension mismatch (defense-in-depth). Self-consistent — no reliance on a separate getDimension(). Test: fails-before/passes-after — a persisted 4-dim vector against a 3-dim model is re-embedded (not served stale), and routing still yields a sane top-K. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
Author
Self-auditSOLID / abstraction
Tests — no false green
Provit (on-device E2E)
Platform parity
Standards
|
📝 WalkthroughWalkthroughAdds a dimension check to the tool embedding cache: cosineSimilarity now returns 0 for mismatched vector lengths, and embedTool validates cached embedding dimension against the current expected dimension before reuse, re-embedding when stale. A unit test covers this invalidation path. ChangesEmbedding Cache Dimension Validation
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Router as selectToolsByEmbedding
participant EmbedTool as embedTool
participant Cache as AsyncStorage
participant Model as EmbedModel
Router->>EmbedTool: embedTool(tool, queryVec.length)
EmbedTool->>Cache: read cached embedding
Cache-->>EmbedTool: stale vector (wrong dimension)
EmbedTool->>EmbedTool: compare cached length vs expectedDim
alt dimension mismatch
EmbedTool->>Model: re-embed tool text
Model-->>EmbedTool: new embedding vector
EmbedTool->>Cache: store updated embedding
end
EmbedTool-->>Router: valid embedding
Compact metadata
Poem 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
PR Summary by QodoFix stale cached tool embeddings after model dimension changes
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/services/toolEmbeddingRouter.ts (1)
148-161: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSolid fix; consider a log line on cache invalidation.
The dimension check correctly forces re-embedding when a cached vector's length diverges from the live model's output (
expectedDim), preventing the stale-vector poisoning described in the PR. Optionally, a debug log whencached && cached.h === hash && cached.v.length !== expectedDimwould help diagnose future model-swap migrations, similar to the existinghydrateCache/schedulePersistwarn logs.🤖 Prompt for 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. In `@src/services/toolEmbeddingRouter.ts` around lines 148 - 161, The cache-dimension guard in embedTool correctly re-embeds stale vectors, but add a debug log when the cached entry matches the hash and is rejected because cached.v.length differs from expectedDim. Use the existing embedTool, toolEmbeddingCache, and schedulePersist context so the log clearly indicates cache invalidation during model swaps, similar in style to the current hydrateCache/schedulePersist logging.
🤖 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.
Nitpick comments:
In `@src/services/toolEmbeddingRouter.ts`:
- Around line 148-161: The cache-dimension guard in embedTool correctly
re-embeds stale vectors, but add a debug log when the cached entry matches the
hash and is rejected because cached.v.length differs from expectedDim. Use the
existing embedTool, toolEmbeddingCache, and schedulePersist context so the log
clearly indicates cache invalidation during model swaps, similar in style to the
current hydrateCache/schedulePersist logging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 01275a72-3a09-4a60-b84a-ad99352d0530
📒 Files selected for processing (2)
__tests__/unit/services/toolEmbeddingRouter.test.tssrc/services/toolEmbeddingRouter.ts
Code Review by Qodo
Context used✅ Compliance rules (platform):
25 rules 1. Real-time sleep in tests
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+86
to
+97
| await selectToolsByEmbedding('find my page', TOOLS, 4); | ||
| await new Promise(r => setTimeout(r, 1100)); // let the debounced persist fire | ||
|
|
||
| // Simulate an embedding-model swap: ONE persisted vector was produced by the OLD | ||
| // model and has a DIFFERENT dimension (4) than the current model (mock returns 3). | ||
| // The content hash keys on the tool text, not the model, so it still "matches" — | ||
| // without a dimension guard, embedTool returns the stale 4-dim vector and | ||
| // cosineSimilarity compares across dimensions, silently poisoning routing. | ||
| const stored = JSON.parse((await AsyncStorage.getItem('tool-embedding-cache-v1'))!); | ||
| const staleName = Object.keys(stored)[0]; | ||
| stored[staleName] = { ...stored[staleName], v: [0.1, 0.2, 0.3, 0.4] }; // 4-dim; model is 3-dim | ||
| await AsyncStorage.setItem('tool-embedding-cache-v1', JSON.stringify(stored)); |
There was a problem hiding this comment.
1. Real-time sleep in tests 🐞 Bug ☼ Reliability
The new unit test uses a real 1100ms sleep to wait for the 1000ms debounced persist, which slows the suite and can become timing-flaky under CI load. If the debounce callback runs late, AsyncStorage.getItem() can still be null and JSON.parse(...) will throw due to the non-null assertion.
Agent Prompt
## Issue description
Unit tests wait for debounced persistence using real-time `setTimeout(1100)`, which slows the test suite and risks flakiness if timers are delayed.
## Issue Context
`schedulePersist()` debounces persistence with a 1000ms timer; tests currently sleep 1100ms to "let the debounced persist fire".
## Fix Focus Areas
- __tests__/unit/services/toolEmbeddingRouter.test.ts[41-107]
- src/services/toolEmbeddingRouter.ts[67-77]
## Suggested fix
- Switch the test suite to Jest fake timers (`jest.useFakeTimers()`), and after invoking `selectToolsByEmbedding(...)` call `jest.advanceTimersByTime(1000)` (or `runOnlyPendingTimers`).
- Flush the microtask queue after advancing timers so the `AsyncStorage.setItem(...)` promise chain completes before reading from storage (e.g., `await new Promise(setImmediate)` or an equivalent flush helper).
- Remove/avoid real-time sleeps, and optionally add an explicit assertion that `stored` is non-null before `JSON.parse` to make failures clearer.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
❌ The last analysis has failed. |
Collaborator
Author
|
Closing — bundling the fix into #510 (refactor/parse-once-boundary). A falsifying red-flow test for this bug is committed on that branch (integration-level, mocks only the native boundary; verified red on HEAD, flips green when the fix lands). |
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.
Problem
The tool-routing embedding cache (
toolEmbeddingRouter.ts) keys entries on the tool text (content hash), not the embedding model. If the embedding model's output dimension changes, the hash still matches andembedToolreturns a stale-dimension vector.cosineSimilaritythen loops on the query's length and reads garbage from the shorter vector — silently poisoning tool routing after a model swap. (Audit finding A12; the test fails onmain.)Fix
embedToolrequires a cached vector to match the current model's dimension (the live query embedding's length) or re-embeds it.cosineSimilarityreturns 0 on a dimension mismatch (defense-in-depth).getDimension().Test (jest, fails-before / passes-after)
A persisted 4-dim vector against a 3-dim model is re-embedded (not served stale), and routing still yields a sane top-K. Fails on
main(stale vector served, only query embedded), passes here.Provit
Pending — this is exercised by the tools/MCP routing journeys (KUF: Remote/Tools); will run on-device once the vision oracle is back up. Self-audit below.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests