Add KV store all() method to retrieve all keys #205
Conversation
Co-Authored-By: Rick Blalock <[email protected]>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
WalkthroughA new Changes
Sequence DiagramsequenceDiagram
participant Client
participant KeyValueAPI
participant Tracer
participant HTTP
participant Storage
Client->>KeyValueAPI: all(name)
KeyValueAPI->>Tracer: startSpan("keyvalue.all")
Tracer->>Tracer: setStorageName attribute
KeyValueAPI->>HTTP: GET /storage/{name}/keys
alt Success (HTTP 200)
HTTP->>Storage: retrieve keys
Storage-->>HTTP: keys[]
HTTP-->>KeyValueAPI: keys[]
KeyValueAPI->>Tracer: span end
KeyValueAPI-->>Client: Promise<string[]>
else Failure
HTTP-->>KeyValueAPI: error
KeyValueAPI->>Tracer: recordException
KeyValueAPI->>Tracer: span end
KeyValueAPI-->>Client: throw error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The addition spans multiple file types (implementation, interface, tests) with varying concerns: new tracing instrumentation patterns, type constraint changes requiring semantic understanding, and comprehensive test coverage. Logic density is moderate with error handling and HTTP integration. Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/apis/keyvalue.ts (1)
258-260: Consider validating the response type.The JSON response is cast to
string[]without runtime validation. While the backend should return a string array, adding a runtime check would improve robustness.if (resp.status === 200) { const keys = await resp.response.json(); + if (!Array.isArray(keys) || !keys.every(k => typeof k === 'string')) { + throw new Error('Invalid response: expected array of strings'); + } span.setStatus({ code: SpanStatusCode.OK }); return keys as string[]; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/apis/keyvalue.ts(1 hunks)src/types.ts(1 hunks)test/apis/keyvalue.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
src/apis/**
📄 CodeRabbit inference engine (AGENT.md)
Place core API implementations under src/apis/ (email, discord, keyvalue, vector, objectstore)
Files:
src/apis/keyvalue.ts
{src,test}/**/!(*.d).ts
📄 CodeRabbit inference engine (AGENT.md)
{src,test}/**/!(*.d).ts: Use strict TypeScript and prefer unknown over any
Use ESM import/export syntax; avoid CommonJS require/module.exports
Use relative imports for internal modules
Keep imports organized (sorted, no unused imports)
Use tabs with a visual width of 2 spaces
Limit lines to a maximum of 80 characters
Use single quotes for strings
Use proper Error types; do not throw strings
Prefer template literals over string concatenation
Files:
src/apis/keyvalue.tstest/apis/keyvalue.test.tssrc/types.ts
src/apis/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/code-generation.mdc)
src/apis/**/*.ts: Do not hardcode generated prompt content (e.g., copyWriter) in source files; load it dynamically
Avoid overly complex TypeScript generics for generated content; prefer simple, maintainable types
Maintain type safety for dynamically loaded content by generating and referencing TypeScript definitions, with proper annotations for require() results
Do not use relative imports to generated artifacts; resolve via absolute node_modules paths or the package entry
Generated content is loaded at runtime, not build time; avoid static imports of generated modules
Prefer bracket notation for accessing slug-named properties with hyphens (e.g., prompts['slug-name'])
Avoid relative require('./generated/_index.js'); resolve absolute paths from process.cwd() into node_modules for generated assets
Files:
src/apis/keyvalue.ts
test/**
📄 CodeRabbit inference engine (AGENT.md)
Tests must mirror the source structure under the test/ directory
Files:
test/apis/keyvalue.test.ts
🧬 Code graph analysis (1)
src/apis/keyvalue.ts (2)
src/router/router.ts (2)
getTracer(61-66)recordException(108-128)src/apis/api.ts (1)
GET(222-241)
🔇 Additional comments (3)
test/apis/keyvalue.test.ts (1)
145-211: Test suite follows existing patterns but doesn't exercise real implementation.The test cases stub out
keyValueAPI.all()directly rather than testing the actual implementation. While this matches the pattern used for thegettests (lines 72-86, 105-114, 134-139), it means these tests only verify the interface contract, not the real logic.As noted in the PR objectives, consider adding integration or end-to-end tests that verify:
- Actual backend interaction with the
/kv/2025-03-17/keys/{name}endpoint- Empty namespace behavior returning
[]- Non-existent namespace behavior
- Performance with 100+ keys
src/types.ts (2)
333-339: LGTM!The
all()method is properly documented and matches the implementation insrc/apis/keyvalue.ts.
551-594: Verify that VectorSearchParams constraint change is intentional.The generic type constraint changed from
T = unknowntoT extends JsonObject = JsonObject(line 585), which is a breaking change not mentioned in the PR objectives or title. This constrains themetadatafield toJsonObjecttypes only.While this constraint makes semantic sense (metadata should be JSON-serializable), it's unclear whether this change is:
- An intentional related fix discovered during implementation
- An unrelated change that should be in a separate PR
- An unintended modification
Please confirm this change is intentional and part of this PR's scope. If it's unrelated to the KV store
all()method feature, consider moving it to a separate PR to keep changes focused.
| if (resp.status === 200) { | ||
| const keys = await resp.response.json(); | ||
| span.setStatus({ code: SpanStatusCode.OK }); | ||
| return keys as string[]; | ||
| } | ||
| throw new Error( | ||
| `error getting all keys: ${resp.response.statusText} (${resp.response.status})` | ||
| ); |
There was a problem hiding this comment.
Handle 404 for non-existent namespaces by returning empty array.
According to the PR objectives, "non-existent/empty namespace behavior (should return [])" is a key design decision. The current implementation throws an error for all non-200 responses, including 404 (non-existent namespace), which conflicts with this requirement.
The get() method (lines 51-55) provides a precedent for handling 404 specially. The all() method should follow the same pattern.
Apply this diff to handle 404 appropriately:
return await context.with(spanContext, async () => {
const resp = await GET(
`/kv/2025-03-17/keys/${encodeURIComponent(name)}`,
false,
undefined,
undefined,
undefined,
'keyvalue'
);
+ if (resp.status === 404) {
+ span.addEvent('namespace-not-found');
+ span.setStatus({ code: SpanStatusCode.OK });
+ return [];
+ }
if (resp.status === 200) {
const keys = await resp.response.json();
span.setStatus({ code: SpanStatusCode.OK });
return keys as string[];
}
throw new Error(
`error getting all keys: ${resp.response.statusText} (${resp.response.status})`
);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (resp.status === 200) { | |
| const keys = await resp.response.json(); | |
| span.setStatus({ code: SpanStatusCode.OK }); | |
| return keys as string[]; | |
| } | |
| throw new Error( | |
| `error getting all keys: ${resp.response.statusText} (${resp.response.status})` | |
| ); | |
| if (resp.status === 404) { | |
| span.addEvent('namespace-not-found'); | |
| span.setStatus({ code: SpanStatusCode.OK }); | |
| return []; | |
| } | |
| if (resp.status === 200) { | |
| const keys = await resp.response.json(); | |
| span.setStatus({ code: SpanStatusCode.OK }); | |
| return keys as string[]; | |
| } | |
| throw new Error( | |
| `error getting all keys: ${resp.response.statusText} (${resp.response.status})` | |
| ); |
🤖 Prompt for AI Agents
In src/apis/keyvalue.ts around lines 257 to 264, change the error handling for
non-200 responses so that a 404 (non-existent namespace) returns an empty array
instead of throwing; specifically, check if resp.status === 404 then set
span.setStatus({ code: SpanStatusCode.OK }) and return [] (mirroring the get()
method pattern), otherwise keep throwing the existing Error with the
statusText/status; ensure any response body is not assumed present for 404 and
that tracing/span status is updated for the successful-but-empty case.
Add KV store all() method to retrieve all keys
Summary
Adds a new
all(name: string): Promise<string[]>method to theKeyValueStorageinterface that retrieves all key names in a KV namespace. This was requested by a customer who needs to enumerate keys in their store.Changes:
all()method toKeyValueStorageinterface insrc/types.tsall()inKeyValueAPIclass with proper OpenTelemetry tracingGET /kv/2025-03-17/keys/{name}(added in https://github.com/agentuity/catalyst/pull/238)Review & Testing Checklist for Human
🔴 CRITICAL - Backend Dependency:
Testing Recommendations:
all(), verify it returns correct keysall()on non-existent namespace (should return empty array[])all()on empty namespace (should return empty array[])Code Review:
/kv/2025-03-17/keys/{name}matches what's deployed in CatalystNotes
get(),set(),delete()methodsSession: Requested by Rick Blalock (@rblalock) - https://app.devin.ai/sessions/03a2a159d03846cdaa7ef968274d6c2c
Summary by CodeRabbit
New Features
Tests