Repository navigation
Fix Kody capability search matching - #272
Conversation
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
📝 WalkthroughWalkthroughThe search functionality is refactored to operate asynchronously. The Changes
Sequence DiagramsequenceDiagram
participant Agent as Agent/Tool
participant SearchUnified as searchUnified
participant BuildCandidates as buildCapabilityCandidates
participant SearchCap as searchCapabilities
participant Embedding as Vector Store
Agent->>SearchUnified: await searchUnified(input)
SearchUnified->>BuildCandidates: async call
BuildCandidates->>SearchCap: searchCapabilities(query, limit)
SearchCap->>Embedding: compute embedding & similarity
Embedding-->>SearchCap: capability matches with scores
SearchCap-->>BuildCandidates: CapabilitySearchHit[]
BuildCandidates->>BuildCandidates: map to SearchCandidate<br/>(clamp scores, filter base > 0)
BuildCandidates-->>SearchUnified: SearchCandidate[]
SearchUnified->>SearchUnified: merge packages + capabilities
SearchUnified-->>Agent: Promise<SearchUnifiedResult>
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
🔎 Preview deployed: https://kody-pr-272.kentcdodds.workers.dev Worker: Mocks:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/worker/src/mcp/tools/search.node.test.ts (1)
540-588: Optional: extract the sharedHOME_CONNECTOR_SESSIONstub.
buildHomeRegistryWithLutronCapabilitiesand the existingloadDownHomeConnectorStatustests at lines 678–732 each hand-roll anHOME_CONNECTOR_SESSIONdurable-object stub with the sameidFromName/get().fetch()shape. A smallmakeHomeConnectorSessionStub({ snapshot })helper would tighten the file and prevent drift if the snapshot contract changes.Also applies to: 678-732
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/tools/search.node.test.ts` around lines 540 - 588, Extract the duplicated durable-object stub into a helper like makeHomeConnectorSessionStub that accepts a snapshot (or tools) parameter and returns an object shaped like HOME_CONNECTOR_SESSION (implementing idFromName(name) { return name } and get().fetch(...) that returns Response.json(snapshot) for the /snapshot path and throws for unexpected paths); then replace the inline HOME_CONNECTOR_SESSION stubs used in buildHomeRegistryWithLutronCapabilities and the loadDownHomeConnectorStatus tests with calls to this helper (pass runtimeLutronTools or the appropriate snapshot) so both tests reuse the same contract-producing function.packages/worker/src/mcp/capabilities/capability-search.ts (1)
307-309: Consider a finite sentinel forvectorScoreon the public hit shape.
vectorScoreById[id] ?? Number.NEGATIVE_INFINITYis now propagated ontoCapabilitySearchHit.vectorScore. Today it works only because every downstream consumer (e.g.blendLexicalAndVectorScoreviaMath.max(0, vector), and the secondary tiebreaker on Line 330) tolerates-Infinity. Any new caller that does plain arithmetic onmatch.vectorScore(averaging, summing, exposing in telemetry/logs) will inherit-Infinityand likely produceNaNor misleading values. Consider normalizing to0(or another finite sentinel) at the boundary where the value is exposed on the hit, while keeping-Infinityonly as an internal ranking sentinel.♻️ Suggested change
- lexicalScore: lexicalScoreById[id] ?? 0, - vectorScore: vectorScoreById[id] ?? Number.NEGATIVE_INFINITY, + lexicalScore: lexicalScoreById[id] ?? 0, + vectorScore: Number.isFinite(vectorScoreById[id]) + ? vectorScoreById[id]! + : 0,Also applies to: 344-346
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/capabilities/capability-search.ts` around lines 307 - 309, The hit objects are exposing Number.NEGATIVE_INFINITY via CapabilitySearchHit.vectorScore which can break downstream math; change the construction of vectorScoreById (and the similar block at lines 344-346) to map any -Infinity (i.e., when vectorScoreMap.get(id) is undefined or -Infinity) to a finite sentinel such as 0 before assigning to the public hit shape; keep Number.NEGATIVE_INFINITY only internally for ranking logic (e.g., when creating vectorScoreMap or doing comparisons in blendLexicalAndVectorScore) but normalize to 0 (or another chosen finite value) when populating vectorScoreById and the values placed onto CapabilitySearchHit.vectorScore.packages/worker/src/mcp/tools/search.ts (1)
608-629: Consider adding an explicit invariant check for registry keying.The current code is correct:
capabilitySpecsis keyed byspec.name(percreateCapabilitySpecsinbuild-capability-registry.ts:87), andsearchCapabilitiesreturns matches wherematch.name === spec.name. The lookup at line 623 therefore succeeds. However, this invariant is not enforced by the type system—if a future refactor changes the registry keying (to a namespaced id, a distinctspec.id, etc.), the lookups at lines 623 and 1284 would silently returnundefinedand drop capabilities from results without error. Consider adding a runtime assertion inbuildCapabilityCandidatesto surface this assumption, or restructuresearchCapabilitiesto return the spec id alongside the summary, making the pairing explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/tools/search.ts` around lines 608 - 629, Add an explicit runtime invariant check in buildCapabilityCandidates to ensure that input.registry.capabilitySpecs is keyed by spec.name and that lookups from searchCapabilities never return undefined: after retrieving const spec = input.registry.capabilitySpecs[match.name] (in buildCapabilityCandidates) assert spec is defined and that spec.name === match.name (or throw a clear Error including match.name and any identifying info), so missing/misaligned keys fail fast instead of silently dropping candidates; alternatively, modify searchCapabilities to return the spec id alongside each match and use that id to index capabilitySpecs before calling capabilityMatchToCandidate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/worker/src/mcp/capabilities/capability-search.ts`:
- Around line 307-309: The hit objects are exposing Number.NEGATIVE_INFINITY via
CapabilitySearchHit.vectorScore which can break downstream math; change the
construction of vectorScoreById (and the similar block at lines 344-346) to map
any -Infinity (i.e., when vectorScoreMap.get(id) is undefined or -Infinity) to a
finite sentinel such as 0 before assigning to the public hit shape; keep
Number.NEGATIVE_INFINITY only internally for ranking logic (e.g., when creating
vectorScoreMap or doing comparisons in blendLexicalAndVectorScore) but normalize
to 0 (or another chosen finite value) when populating vectorScoreById and the
values placed onto CapabilitySearchHit.vectorScore.
In `@packages/worker/src/mcp/tools/search.node.test.ts`:
- Around line 540-588: Extract the duplicated durable-object stub into a helper
like makeHomeConnectorSessionStub that accepts a snapshot (or tools) parameter
and returns an object shaped like HOME_CONNECTOR_SESSION (implementing
idFromName(name) { return name } and get().fetch(...) that returns
Response.json(snapshot) for the /snapshot path and throws for unexpected paths);
then replace the inline HOME_CONNECTOR_SESSION stubs used in
buildHomeRegistryWithLutronCapabilities and the loadDownHomeConnectorStatus
tests with calls to this helper (pass runtimeLutronTools or the appropriate
snapshot) so both tests reuse the same contract-producing function.
In `@packages/worker/src/mcp/tools/search.ts`:
- Around line 608-629: Add an explicit runtime invariant check in
buildCapabilityCandidates to ensure that input.registry.capabilitySpecs is keyed
by spec.name and that lookups from searchCapabilities never return undefined:
after retrieving const spec = input.registry.capabilitySpecs[match.name] (in
buildCapabilityCandidates) assert spec is defined and that spec.name ===
match.name (or throw a clear Error including match.name and any identifying
info), so missing/misaligned keys fail fast instead of silently dropping
candidates; alternatively, modify searchCapabilities to return the spec id
alongside each match and use that id to index capabilitySpecs before calling
capabilityMatchToCandidate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 44ff59ee-26cd-44df-a061-b1d45ab98255
📒 Files selected for processing (4)
packages/worker/src/agent-turn/tools.tspackages/worker/src/mcp/capabilities/capability-search.tspackages/worker/src/mcp/tools/search.node.test.tspackages/worker/src/mcp/tools/search.ts
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
2 participants
Summary
search.tslutron) are not cancelled outvectorScoreand adding a runtime invariant for registry keying during unified capability candidate mappinglutron lights colorandturn office lights redRoot cause
Kody:searchandmeta_list_capabilitieswere reading from the same runtime capability registry, but they were not using the same retrieval path.meta_list_capabilitieslisted the runtime capability correctly, while MCPsearchused a separate lightweight scorer insidepackages/worker/src/mcp/tools/search.tsinstead of the dedicated capability search implementation inpackages/worker/src/mcp/capabilities/capability-search.ts.That separate scorer also blended lexical overlap with a raw vector cosine score that could go negative, which meant a real lexical token match could be suppressed rather than boosted.
Review follow-up
I validated the automated review comments against the actual fix. I accepted and implemented the two comments that represented real risks:
CapabilitySearchHit.vectorScoreto a finite value while preserving internal ranking sentinelssearchCapabilities()hits back to the capability registryI intentionally did not do the suggested test-helper extraction because it was only optional cleanup and did not affect correctness or the original search goal.
Testing
npm run test -- --run packages/worker/src/mcp/tools/search.node.test.ts packages/worker/src/mcp/capabilities/capability-search.workers.test.tsnpm run typecheckArtifact
Focused validation log
Summary by CodeRabbit
Release Notes
Bug Fixes
New Features
Tests