Repository navigation
Prompt eval sdk changes - #184
Conversation
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/server/context.test.ts (1)
79-92: Consider rejecting or generating valid IDs for empty inputs.The test validates that empty
sessionIdandrunIdresult in'sess_', which is just the prefix without an actual identifier. This may not be a valid session ID and could cause issues downstream.Consider either:
- Generating a unique ID when both are missing (e.g., using UUID)
- Throwing an error to require a valid session identifier
🧹 Nitpick comments (7)
src/apis/patchportal.ts (3)
13-32: Clarify "thread-safe" terminology for JavaScript context.The comment mentions "thread-safe," but JavaScript is single-threaded. The pattern protects against concurrent async calls to
getInstance()rather than true multi-threading. Consider updating the terminology to be more precise./** - * Get the singleton instance of PatchPortal in a thread-safe manner + * Get the singleton instance of PatchPortal, protecting against concurrent async initialization */Similarly, update the class-level JSDoc on line 2:
/** - * Thread-safe singleton class for PatchPortal + * Async-safe singleton class for PatchPortal */
34-40: Unnecessaryasyncmodifier.The
process()method doesn't perform any asynchronous operations. Remove theasyncmodifier unless you plan to add async logic.- public async process(key: string, data: unknown): Promise<unknown> { + public process(key: string, data: unknown): unknown { PatchPortal.state[key] = data; return data; }Note: The JSDoc comment describes this as an "example method," suggesting this is placeholder functionality. Consider replacing with actual implementation or documenting the intended use case.
1-48: Document the purpose and intended usage of PatchPortal.The implementation includes example/placeholder methods (
process(),getInstanceId()) but lacks documentation on:
- What is PatchPortal's actual purpose?
- When should it be used?
- What will replace the example methods?
Consider:
- Adding class-level JSDoc explaining the purpose and usage patterns
- Implementing the actual API surface or marking this as WIP
- Adding error handling and validation as needed
- Providing a way to inspect or clear the state for testing
- Considering if singleton is the right pattern - could dependency injection be clearer?
Example:
/** * PatchPortal manages [describe purpose here] * * Usage: * const portal = await PatchPortal.getInstance(); * await portal.process(...); * * Note: This is a singleton - only one instance exists per process. */ export default class PatchPortal { // ... }src/server/server.ts (1)
200-203: Redundant null check for patchportal.The
if (!patchportal)check is redundant sincePatchPortal.getInstance()already implements double-checked locking internally. The singleton pattern inPatchPortalensures thread-safe initialization.Consider simplifying to:
- // Initialize PatchPortal if not already done - if (!patchportal) { - patchportal = await PatchPortal.getInstance(); - } + // Initialize PatchPortal (singleton pattern ensures single instance) + patchportal = await PatchPortal.getInstance();src/apis/prompt/index.ts (3)
29-64: Consider more robust module resolution.The hardcoded paths assume a specific installation structure (
@agentuity/sdkinnode_modules). This approach is fragile and may break if the package is installed in a monorepo, linked vianpm link, or bundled differently.After applying the ESM conversion from the previous comment, consider using a more flexible resolution strategy:
// Option 1: Try import with error handling try { generatedModule = await import('@agentuity/sdk/dist/apis/prompt/generated/_index.js'); } catch { try { generatedModule = await import('@agentuity/sdk/src/apis/prompt/generated/_index.js'); } catch { throw new Error('Generated prompts file not found'); } }This relies on Node's module resolution instead of manual path construction.
75-75: Add type guard for generatedModule.prompts.After loading
generatedModule, accessing.promptslacks type safety. Consider adding a runtime check to ensure the loaded module has the expected structure.+ // Type guard for loaded module + const hasPrompts = (mod: unknown): mod is { prompts: typeof defaultPrompts } => { + return typeof mod === 'object' && mod !== null && 'prompts' in mod; + }; + if (!generatedModule) { throw new Error('Generated prompts file not found'); } - this.prompts = generatedModule.prompts || defaultPrompts; + if (hasPrompts(generatedModule)) { + this.prompts = generatedModule.prompts; + } else { + console.warn('Loaded module does not have expected prompts structure'); + this.prompts = defaultPrompts; + }
18-18: Remove commented-out debug statements.The commented-out
console.logstatements should be removed before merging. If debug logging is needed in the future, consider using a proper debug utility or logger.public async loadPrompts(): Promise<void> { - // console.log('loadPrompts() called'); try { ... - // console.log('Trying absolute paths:'); for (const possiblePath of possiblePaths) { - // console.log(' Checking:', possiblePath); - // console.log(' Exists:', existsSync(possiblePath)); if (existsSync(possiblePath)) { generatedModule = await import(possiblePath); - // console.log(' Successfully loaded from:', possiblePath); break; } } - // console.log('Generated module:', generatedModule); - // console.log( - // 'Prompts in module:', - // Object.keys(generatedModule.prompts || {}) - // ); this.prompts = generatedModule.prompts || defaultPrompts; - // console.log('Final prompts:', Object.keys(this.prompts)); }Also applies to: 54-57, 61-61, 70-76
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
package-lock.jsonis excluded by!**/package-lock.jsonsrc/apis/prompt/generated/_index.jsis excluded by!**/generated/**src/apis/prompt/generated/index.d.tsis excluded by!**/generated/**src/apis/prompt/generated/index.tsis excluded by!**/generated/**
📒 Files selected for processing (10)
package.json(2 hunks)src/apis/api.ts(2 hunks)src/apis/patchportal.ts(1 hunks)src/apis/prompt/index.ts(1 hunks)src/autostart/index.ts(1 hunks)src/index.ts(1 hunks)src/io/email.ts(1 hunks)src/server/server.ts(3 hunks)src/types.ts(2 hunks)test/server/context.test.ts(8 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
src/apis/**
📄 CodeRabbit inference engine (AGENT.md)
Place core API implementations under src/apis/ (email, discord, keyvalue, vector, objectstore)
Files:
src/apis/api.tssrc/apis/patchportal.tssrc/apis/prompt/index.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/api.tssrc/index.tssrc/apis/patchportal.tssrc/io/email.tssrc/autostart/index.tssrc/apis/prompt/index.tssrc/server/server.tssrc/types.tstest/server/context.test.ts
src/index.ts
📄 CodeRabbit inference engine (AGENT.md)
src/index.ts must be the entry point and export server, logger, types, and APIs
Files:
src/index.ts
src/io/**
📄 CodeRabbit inference engine (AGENT.md)
I/O handlers (Discord, Slack, Email, SMS, Telegram) live under src/io/
Files:
src/io/email.ts
src/server/{server,bun,node,agents}.ts
📄 CodeRabbit inference engine (AGENT.md)
Server components live in src/server/ as server.ts, bun.ts, node.ts, and agents.ts
Files:
src/server/server.ts
test/**
📄 CodeRabbit inference engine (AGENT.md)
Tests must mirror the source structure under the test/ directory
Files:
test/server/context.test.ts
🧬 Code graph analysis (4)
src/apis/patchportal.ts (1)
src/index.ts (1)
PatchPortal(11-11)
src/autostart/index.ts (1)
src/server/server.ts (1)
createServerContext(194-228)
src/server/server.ts (3)
src/apis/prompt/index.ts (1)
PromptAPI(8-89)src/apis/patchportal.ts (1)
PatchPortal(4-48)src/types.ts (1)
AgentContext(793-911)
test/server/context.test.ts (1)
src/server/server.ts (1)
createServerContext(194-228)
🪛 GitHub Actions: Run Tests
test/server/context.test.ts
[error] 1-1: SyntaxError: export 'PromptConfig' not found in './generated/index.ts'.
🔇 Additional comments (25)
package.json (2)
3-3: Version bump looks appropriate.The version increment from 0.0.146 to 0.0.150 aligns with the addition of new public API surface (PatchPortal, PromptAPI) and the service name extension.
97-97: Verify nodemailer 7.x compatibility for explicit internal importspackage.json uses nodemailer ^7.0.3 but src/io/email.ts imports internal paths ('nodemailer/lib/mail-composer/index.js' and 'nodemailer/lib/mailer/index.js'); node_modules isn't available in the PR so these subpaths couldn't be validated — confirm the installed nodemailer 7.x exposes those paths or switch to the library's supported public API to avoid runtime breakage.
src/io/email.ts (1)
7-8: LGTM: ESM-compliant import paths.The explicit
.jsextensions and full index path references align with strict ESM module resolution required by Node.js 22+ (as specified in package.json engines). This change ensures compatibility with the ESM-first environment.src/autostart/index.ts (1)
109-109: LGTM: Correctly awaits async context creation.The addition of
awaitproperly handles the now-asynchronouscreateServerContext, which loads prompts viapromptAPI.loadPrompts()and initializesPatchPortal.getInstance()before returning the context.src/apis/api.ts (2)
35-36: LGTM: ServiceName type extended correctly.The addition of
'prompt'to theServiceNameunion type is a non-breaking change that supports the new PromptAPI functionality.
109-112: LGTM: URL resolution follows established pattern.The new
'prompt'case correctly implements the service URL resolution pattern, usingAGENTUITY_PROMPT_URLwith a fallback toAGENTUITY_TRANSPORT_URL, consistent with other services likevectorandkeyvalue.src/index.ts (2)
8-11: LGTM: New public API exports.The addition of
PatchPortalandPromptAPIto the public export surface correctly exposes the new functionality introduced in this PR. The export pattern is consistent with existing API exports (EmailAPI,DiscordAPI,StreamAPIImpl).
15-15: LGTM: Import consolidation.Minor import path adjustment maintains code organization without functional changes.
src/types.ts (2)
5-5: Formatting-only change - no review needed.
425-468: No issues with constraining the generic parameter. Search across the codebase shows no external usages ofVectorSearchParams<T>with non-JSON types or explicit type arguments; the addedT extends JsonObjectconstraint aligns with both the runtime metadata check and JSON-serializability requirements.test/server/context.test.ts (5)
14-29: LGTM! Test correctly updated for async context creation.The test properly awaits
createServerContextand validates thatrunIdequalssessionIdfor backward compatibility.
31-45: LGTM! Correctly validates session ID prefix handling.
47-61: LGTM! Important edge case for prefix idempotence.
63-77: LGTM! Validates backward compatibility with legacy runId.
94-109: LGTM! Correctly validates sessionId precedence over runId.src/server/server.ts (6)
8-9: LGTM! Clean ESM imports.The new imports follow the project's ESM conventions and coding guidelines.
183-186: LGTM! Appropriate initialization strategy.The eager instantiation of
promptAPIwith deferredloadPrompts()and lazy initialization ofpatchportalalign well with the async singleton pattern.
220-220: LGTM! Late-binding prompts accessor.Using an arrow function for
prompts()correctly provides late binding to the current state ofpromptAPI.prompts, ensuring callers always access the loaded prompts.
223-223: LGTM! PatchPortal instance exposed.The
patchportalfield correctly exposes the singleton instance initialized earlier.
227-227: LGTM! Safe type assertion pattern.The two-step cast through
unknownfollows TypeScript best practices and aligns with the coding guideline to preferunknownoverany.
194-196: All createServerContext invocations correctly await the async function. No further updates required.src/apis/prompt/index.ts (4)
1-6: LGTM! Clean ESM imports and sensible default.The ESM import syntax and empty
defaultPromptsfallback are appropriate.
8-14: LGTM! Clean class structure.The class declaration and initialization are straightforward and appropriate.
79-86: Console usage in library code.Using
console.logandconsole.warndirectly in library code is not ideal. However, sincePromptAPIis instantiated at module scope before the logger context is available, this is a reasonable compromise for optional prompts loading.Note: The silent fallback to empty prompts with a warning is appropriate behavior for optional/generated content.
91-93: LGTM! Clean re-exports.The export structure follows ESM conventions and provides a clean public API.
| /** | ||
| * Example method for demonstrating the singleton | ||
| */ | ||
| public getInstanceId(): string { | ||
| return `PatchPortal-${Date.now()}`; | ||
| } |
There was a problem hiding this comment.
Misleading method name and behavior.
getInstanceId() returns a different value on each call (Date.now()), which contradicts the expectation of an instance identifier. An instance ID should be stable for the lifetime of the instance.
Consider either:
- Generating a stable ID once during construction:
private readonly instanceId: string;
private constructor() {
this.instanceId = `PatchPortal-${Date.now()}`;
}
public getInstanceId(): string {
return this.instanceId;
}- Renaming to clarify the behavior:
public getCurrentTimestamp(): string {
return `PatchPortal-${Date.now()}`;
}- Removing this example method if it's not part of the actual API.
🤖 Prompt for AI Agents
In src/apis/patchportal.ts around lines 42 to 47, the getInstanceId() method
returns a new value each call (using Date.now()), which is misleading for an
instance identifier; change it to generate and store a stable instanceId once
during construction (add a private readonly instanceId set in the constructor,
and have getInstanceId() return that field). If the example method is not
needed, remove it instead—do not leave a method that implies stability but
returns a different value each call.
| public async loadPrompts(): Promise<void> { | ||
| // console.log('loadPrompts() called'); | ||
| try { | ||
| // Try multiple possible paths for the generated prompts | ||
| let generatedModule: any; | ||
|
|
||
| // Skip relative path - doesn't work in bundled environment | ||
| // Try absolute path from node_modules | ||
| const path = require('path'); | ||
| const fs = require('fs'); |
There was a problem hiding this comment.
Replace CommonJS require with ESM imports.
Lines 25-26 use CommonJS require(), which violates the coding guideline: "Use ESM import/export syntax; avoid CommonJS require/module.exports." This mixing of module systems can cause issues in bundled environments.
Apply this diff to use ESM imports:
// Method to load prompts dynamically (called by context)
public async loadPrompts(): Promise<void> {
// console.log('loadPrompts() called');
try {
// Try multiple possible paths for the generated prompts
- let generatedModule: any;
+ let generatedModule: unknown;
- // Skip relative path - doesn't work in bundled environment
- // Try absolute path from node_modules
- const path = require('path');
- const fs = require('fs');
+ const { join } = await import('node:path');
+ const { existsSync } = await import('node:fs');
// Look for the generated file in common locations
const possiblePaths = [
- path.join(
+ join(
process.cwd(),
'node_modules',
'@agentuity',
'sdk',
'dist',
'apis',
'prompt',
'generated',
'_index.js'
),
- path.join(
+ join(
process.cwd(),
'node_modules',
'@agentuity',
'sdk',
'src',
'apis',
'prompt',
'generated',
'_index.js'
),
];
// console.log('Trying absolute paths:');
for (const possiblePath of possiblePaths) {
// console.log(' Checking:', possiblePath);
- // console.log(' Exists:', fs.existsSync(possiblePath));
- if (fs.existsSync(possiblePath)) {
- delete require.cache[possiblePath];
- generatedModule = require(possiblePath);
+ // console.log(' Exists:', existsSync(possiblePath));
+ if (existsSync(possiblePath)) {
+ generatedModule = await import(possiblePath);
// console.log(' Successfully loaded from:', possiblePath);
break;
}
}Note: Using ESM import() eliminates the need for manual cache invalidation (line 59), as ESM modules handle caching differently.
📝 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.
| public async loadPrompts(): Promise<void> { | |
| // console.log('loadPrompts() called'); | |
| try { | |
| // Try multiple possible paths for the generated prompts | |
| let generatedModule: any; | |
| // Skip relative path - doesn't work in bundled environment | |
| // Try absolute path from node_modules | |
| const path = require('path'); | |
| const fs = require('fs'); | |
| public async loadPrompts(): Promise<void> { | |
| // console.log('loadPrompts() called'); | |
| try { | |
| // Try multiple possible paths for the generated prompts | |
| let generatedModule: unknown; | |
| const { join } = await import('node:path'); | |
| const { existsSync } = await import('node:fs'); | |
| // Look for the generated file in common locations | |
| const possiblePaths = [ | |
| join( | |
| process.cwd(), | |
| 'node_modules', | |
| '@agentuity', | |
| 'sdk', | |
| 'dist', | |
| 'apis', | |
| 'prompt', | |
| 'generated', | |
| '_index.js' | |
| ), | |
| join( | |
| process.cwd(), | |
| 'node_modules', | |
| '@agentuity', | |
| 'sdk', | |
| 'src', | |
| 'apis', | |
| 'prompt', | |
| 'generated', | |
| '_index.js' | |
| ), | |
| ]; | |
| // console.log('Trying absolute paths:'); | |
| for (const possiblePath of possiblePaths) { | |
| // console.log(' Checking:', possiblePath); | |
| // console.log(' Exists:', existsSync(possiblePath)); | |
| if (existsSync(possiblePath)) { | |
| generatedModule = await import(possiblePath); | |
| // console.log(' Successfully loaded from:', possiblePath); | |
| break; | |
| } | |
| } | |
| // …rest of method… |
|
|
||
| /** | ||
| * get the prompts collection for compiling dynamic prompts | ||
| */ | ||
| prompts(): import('./apis/prompt/generated/index.js').PromptsCollection; |
There was a problem hiding this comment.
Critical: Pipeline failure indicates missing exports in generated file.
The pipeline reports: SyntaxError: export 'PromptConfig' not found in './generated/index.ts'. The new prompts() method references types from ./apis/prompt/generated/index.js, but the generated file appears to be missing required exports.
#!/bin/bash
# Description: Verify the generated index file exists and check its exports
# Check if the generated file exists
fd -p 'src/apis/prompt/generated/index.ts'
# If it exists, show its exports
if [ -f "src/apis/prompt/generated/index.ts" ]; then
echo "=== Exports from generated/index.ts ==="
rg -n '^export' src/apis/prompt/generated/index.ts
fi
# Also check for PromptsCollection and PromptConfig definitions
echo "=== Searching for PromptsCollection and PromptConfig definitions ==="
rg -n 'interface|type|class' src/apis/prompt/ -A1 | rg -i 'PromptsCollection|PromptConfig'🤖 Prompt for AI Agents
In src/types.ts around lines 906 to 910, the new prompts() signature references
types from ./apis/prompt/generated/index.js but the generated index is missing
required exports (e.g., PromptConfig and PromptsCollection), causing the
pipeline failure; fix by updating the generated file (or generator) to export
the missing types and re-export them from
src/apis/prompt/generated/index.(ts|js) so that PromptsCollection and
PromptConfig are publicly exported, verify the import path/extension matches the
build (use .ts in source, .js in compiled), and run the generation/build step to
ensure the exports are present.
- Add rules requiring generated code to never need optional chaining - Always generate both system and prompt fields, even if empty - Empty fields should return empty strings, not undefined - Prevents partial objects that require optional chaining in agent code
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/types.ts (1)
459-459: Align generic constraint invector.searchwithVectorSearchParams
Update the method signature insrc/apis/vector.ts(around line 221) from:async search<T = unknown>(to:
async search<T extends JsonObject = JsonObject>(so that
Tsatisfies theVectorSearchParams<T>constraint.src/server/server.ts (1)
227-227: Remove the unsafe double‐cast and ensure the returned object truly satisfies AgentContext
The return at src/server/server.ts:227 uses} as unknown as AgentContext;, bypassing type checks while only providing a handful of fields. Either:
- Populate every property declared on AgentContext (e.g. getAgent, waitUntil, slack, etc.)
- Or change to a narrower interface/type that matches the actual returned shape
🧹 Nitpick comments (3)
src/server/server.ts (2)
200-204: Consider adding error handling for initialization failures.The lazy initialization of
PatchPortalandpromptAPI.loadPrompts()can fail, but errors are not handled here. If initialization fails, the context will be created with potentially incomplete state.Consider wrapping initialization in try-catch and logging errors:
// Initialize PatchPortal if not already done if (!patchportal) { - patchportal = await PatchPortal.getInstance(); + try { + patchportal = await PatchPortal.getInstance(); + } catch (error) { + req.logger.error('Failed to initialize PatchPortal:', error); + throw error; + } } - await promptAPI.loadPrompts(); + try { + await promptAPI.loadPrompts(); + } catch (error) { + req.logger.warn('Failed to load prompts:', error); + // Continue with default prompts + }Note:
promptAPI.loadPrompts()already has internal error handling per the code snippets, but explicit error handling here would make the failure mode clearer.
183-186: Module-level state could cause issues in concurrent scenarios.The module-level
promptAPIandpatchportalvariables are shared across all context creations. WhilePatchPortaluses a singleton pattern, the sharedpromptAPIinstance meansloadPrompts()is called on every context creation, potentially causing race conditions or redundant work.Consider one of these approaches:
- Make promptAPI a singleton (like PatchPortal):
class PromptAPI { private static instance: PromptAPI | null = null; private loaded = false; static async getInstance(): Promise<PromptAPI> { if (!PromptAPI.instance) { PromptAPI.instance = new PromptAPI(); await PromptAPI.instance.loadPrompts(); } return PromptAPI.instance; } async loadPrompts(): Promise<void> { if (this.loaded) return; // ... loading logic this.loaded = true; } }
- Load prompts once at module initialization:
const promptAPI = new PromptAPI(); let promptsLoaded = false; async function ensurePromptsLoaded() { if (!promptsLoaded) { await promptAPI.loadPrompts(); promptsLoaded = true; } }.cursor/rules/code-generation.mdc (1)
1-151: Documentation is well-structured and aligns with implementation.The code generation documentation accurately describes the patterns implemented in
PromptAPIand server context creation. The examples match the actual code, and the guidance is clear.A few minor suggestions for improvement:
Line 9: The warning about keeping CLI rules in sync is good, but consider adding a link to the CLI rules file or documenting what specifically needs to stay in sync.
Lines 13-17: The optional field handling section is excellent guidance but seems specific to prompt generation rather than general code generation. Consider clarifying which generated content this applies to.
Lines 146-149: The note about using
require()vsimport()is helpful. Consider adding why (e.g., "dynamic import() returns promises that complicate synchronous fallbacks").
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/apis/prompt/generated/index.d.tsis excluded by!**/generated/**
📒 Files selected for processing (4)
.cursor/rules/code-generation.mdc(1 hunks)package.json(2 hunks)src/server/server.ts(3 hunks)src/types.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
📓 Path-based instructions (2)
src/server/{server,bun,node,agents}.ts
📄 CodeRabbit inference engine (AGENT.md)
Server components live in src/server/ as server.ts, bun.ts, node.ts, and agents.ts
Files:
src/server/server.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/server/server.tssrc/types.ts
🧬 Code graph analysis (1)
src/server/server.ts (3)
src/apis/prompt/index.ts (1)
PromptAPI(8-89)src/apis/patchportal.ts (1)
PatchPortal(4-48)src/types.ts (1)
AgentContext(793-911)
🔇 Additional comments (2)
src/types.ts (1)
907-910: Verify generated exports & runtime import
- PromptsCollection and PromptConfig are exported in src/apis/prompt/generated/index.ts and .d.ts.
- The dynamic import of './apis/prompt/generated/index.js' must match your build output—confirm that index.js is emitted to apis/prompt/generated and that tsconfig’s moduleResolution (e.g. "node16" or "node") and allowJs settings permit importing .js extensions.
- Ensure your CLI generator produces the JS files in the correct location before running the pipeline.
src/server/server.ts (1)
194-228: Confirm callers await the now-async createServerContext
The signature changed to return a Promise. I verified the only production call in src/autostart/index.ts (around line 109) and all tests already useawait. Please double-check for any other unawaited calls.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/apis/patchportal.ts (1)
33-35: Misleading method name:getInstanceId()returns a different value on each call.This method returns
Date.now()on every invocation, which contradicts the purpose of an instance identifier. An instance ID should remain stable for the lifetime of the instance.As noted in the previous review, consider one of these approaches:
- Generate a stable ID once during construction (recommended if you need a true instance ID):
+ private readonly instanceId: string; + private constructor() { - // Private constructor to prevent direct instantiation + this.instanceId = `PatchPortal-${Date.now()}`; } public getInstanceId(): string { - return `PatchPortal-${Date.now()}`; + return this.instanceId; }
- Rename to clarify the behavior (if you need a timestamp):
- public getInstanceId(): string { + public getCurrentTimestamp(): string { return `PatchPortal-${Date.now()}`; }
- Remove the method if it's just placeholder/example code and not part of the actual API.
🧹 Nitpick comments (5)
package.json (1)
99-99: Add trailing newline to package.json.The file is missing a trailing newline, which is a common convention and prevents unnecessary diff noise in version control.
Apply this diff:
} -} +} +src/apis/patchportal.ts (2)
15-20: Unnecessary async signature forgetInstance().The method is declared
asyncbut performs no asynchronous operations. The lazy initialization of the singleton is synchronous.Unless you're planning to add async initialization logic, consider making this synchronous:
- public static async getInstance(): Promise<PatchPortal> { + public static getInstance(): PatchPortal { if (!PatchPortal.instance) { PatchPortal.instance = new PatchPortal(); } return PatchPortal.instance; }
25-28: Unnecessary async signature forprocess().The method is declared
asyncbut performs only synchronous state storage. There's no await or asynchronous operation.Unless you plan to add async operations (e.g., validation, persistence), consider making this synchronous:
- public async process(key: string, data: unknown): Promise<unknown> { + public process(key: string, data: unknown): unknown { this.state[key] = data; return data; }src/utils/interpolate.ts (2)
54-72: Simplify variable extraction logic.The current implementation has several issues:
- Line 55 re-matches each match, which is redundant
- Lines 63-66 attempt to remove
:defaultsuffix, but the regex in line 55 already captures the variable name without the suffix in group 1. TheindexOf(':')will never find a colon invarMatch[1]because the regex pattern[^}:]+excludes colons- The two-pass approach (match all, then re-match each) is inefficient
Consider this simplified implementation:
const variables: string[] = []; const seen = new Set<string>(); for (const match of matches) { - const varMatch = match.match(/\{([!]?[^}:]+)(?::([^}]*))?\}/); - if (varMatch) { - let varName = varMatch[1].trim(); - // Remove ! prefix if present - if (varName.startsWith('!')) { - varName = varName.substring(1); - } - // Remove :default suffix if present - const colonIndex = varName.indexOf(':'); - if (colonIndex !== -1) { - varName = varName.substring(0, colonIndex); - } - if (!seen.has(varName)) { - variables.push(varName); - seen.add(varName); - } + // Extract variable name from {!variable:default} -> "variable" + const varMatch = match.match(/\{!?([^}:]+)/); + if (varMatch) { + const varName = varMatch[1].trim(); + if (!seen.has(varName)) { + variables.push(varName); + seen.add(varName); + } } }The regex
\{!?([^}:]+)captures the variable name after an optional!and before any:or}, eliminating the need for manual string manipulation.
13-14: Consider extracting duplicate normalization logic.The same {{variable}} to {variable} normalization appears in both functions (lines 14 and 44). Consider extracting this to a private helper function to follow the DRY principle.
function normalizeTemplate(template: string): string { return template.replace(/\{\{([^}]+)\}\}/g, '{$1}'); }Then use it in both functions:
const goCommonTemplate = normalizeTemplate(template);Also applies to: 43-44
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/apis/prompt/generated/_index.jsis excluded by!**/generated/**src/apis/prompt/generated/index.tsis excluded by!**/generated/**
📒 Files selected for processing (6)
.cursor/rules/code-generation.mdc(1 hunks)package.json(2 hunks)src/apis/patchportal.ts(1 hunks)src/apis/prompt/index.ts(1 hunks)src/index.ts(1 hunks)src/utils/interpolate.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/apis/prompt/index.ts
- .cursor/rules/code-generation.mdc
- src/index.ts
🧰 Additional context used
📓 Path-based instructions (2)
{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/utils/interpolate.tssrc/apis/patchportal.ts
src/apis/**
📄 CodeRabbit inference engine (AGENT.md)
Place core API implementations under src/apis/ (email, discord, keyvalue, vector, objectstore)
Files:
src/apis/patchportal.ts
🧬 Code graph analysis (1)
src/apis/patchportal.ts (1)
src/index.ts (1)
PatchPortal(12-12)
🔇 Additional comments (3)
package.json (1)
3-3: LGTM: Version bump aligns with PR changes.The version increment to 0.0.154 is appropriate for the addition of new APIs (PatchPortal, PromptAPI) and other SDK changes in this PR.
src/apis/patchportal.ts (1)
1-10: LGTM: Singleton pattern correctly implemented with strict typing.The private constructor and state typed as
Record<string, unknown>follow the coding guidelines. The previous concern aboutanytyping has been addressed.src/utils/interpolate.ts (1)
25-27: Reconsider empty string handling.The condition
value !== ''treats empty strings as missing values. If a user explicitly passes''for a variable, it will be ignored and replaced with the default or throw an error for required variables. This may not be the intended behavior.Consider whether empty strings should be treated as valid values:
- If empty strings are valid: check only
value !== undefined- If empty strings should be treated as missing: the current logic is correct
Also note that
String(value)on line 26 will convert objects/arrays to"[object Object]"or comma-separated strings. Consider whether non-primitive values should be JSON.stringified or rejected with a type error.
1 participant
Summary by CodeRabbit