-
Notifications
You must be signed in to change notification settings - Fork 9
v1.0 #9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
8 issues found across 49 files
Prompt for AI agents (all 8 issues)
Understand the root cause of the following 8 issues and fix them.
<file name="README.md">
<violation number="1" location="README.md:104">
The Quick Start example now relies on the dotenv variable loader, but there is still no `import '@utcp/dotenv-loader';`, so the loader never registers and `UtcpClient.create` will fail to resolve the `dotenv` loader at runtime. Please add the side-effect import alongside the other plugin imports.</violation>
</file>
<file name="packages/core/src/implementations/post_processors/filter_dict_post_processor.ts">
<violation number="1" location="packages/core/src/implementations/post_processors/filter_dict_post_processor.ts:190">
Rebuilding the config object here drops any extra keys that FilterDictPostProcessorConfigSchema intentionally allows via .passthrough(), so those settings are lost during serialization/deserialization.</violation>
</file>
<file name="packages/http/src/sse_communication_protocol.ts">
<violation number="1" location="packages/http/src/sse_communication_protocol.ts:47">
API key authentication logic for defaulting location and applying API keys is duplicated. This exact pattern exists in `packages/http/src/http_communication_protocol.ts` (lines 246-252) and `packages/http/src/streamable_http_communication_protocol.ts` (lines 46-52). This should be extracted into a shared utility function to ensure consistent behavior and easier maintenance for critical authentication logic.</violation>
</file>
<file name="packages/core/src/implementations/post_processors/limit_strings_post_processor.ts">
<violation number="1" location="packages/core/src/implementations/post_processors/limit_strings_post_processor.ts:128">
Manually reconstructing the config here drops any additional keys allowed by the `.passthrough()` schema, so configurations with custom fields will lose data during serialization. Please preserve the full dictionary returned by `obj.toDict()`.</violation>
</file>
<file name="packages/file/README.md">
<violation number="1" location="packages/file/README.md:96">
`client.searchTools` returns an array of tools, so `staticDataReaderTool.name` is undefined and the sample would throw at runtime. Use the first element before accessing `.name`.</violation>
<violation number="2" location="packages/file/README.md:107">
`projectDescTool` is an array of tools from `client.searchTools`, so accessing `.name` directly will throw. Index into the array before calling `client.callTool`.</violation>
</file>
<file name="packages/file/tests/file_communication_protocol.test.ts">
<violation number="1" location="packages/file/tests/file_communication_protocol.test.ts:165">
This expectation wraps an async registerManual call with `.toThrow()`, which only checks for synchronous throws; the method rejects instead, so the test will always fail. Assert the rejection with `.rejects.toThrow()` on the promise.</violation>
</file>
<file name="packages/core/src/implementations/default_variable_substitutor.ts">
<violation number="1" location="packages/core/src/implementations/default_variable_substitutor.ts:80">
Calling JSON.stringify here causes substitute to throw whenever the payload contains a BigInt (or another non-serializable value), regressing support for such inputs compared to the prior implementation.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| import { HttpCallTemplateSerializer } from '@utcp/http'; | ||
| import { McpCallTemplateSerializer } from '@utcp/mcp'; | ||
| import { TextCallTemplateSerializer } from '@utcp/text'; | ||
| import { FileCallTemplateSerializer } from '@utcp/file'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Quick Start example now relies on the dotenv variable loader, but there is still no import '@utcp/dotenv-loader';, so the loader never registers and UtcpClient.create will fail to resolve the dotenv loader at runtime. Please add the side-effect import alongside the other plugin imports.
Prompt for AI agents
Address the following comment on README.md at line 104:
<comment>The Quick Start example now relies on the dotenv variable loader, but there is still no `import '@utcp/dotenv-loader';`, so the loader never registers and `UtcpClient.create` will fail to resolve the `dotenv` loader at runtime. Please add the side-effect import alongside the other plugin imports.</comment>
<file context>
@@ -98,13 +101,13 @@ main().catch(console.error);
import { HttpCallTemplateSerializer } from '@utcp/http';
import { McpCallTemplateSerializer } from '@utcp/mcp';
-import { TextCallTemplateSerializer } from '@utcp/text';
+import { FileCallTemplateSerializer } from '@utcp/file';
async function main() {
</file context>
| toDict(obj: FilterDictPostProcessor): { [key: string]: any } { | ||
| return obj.toDict(); | ||
| const filterDictConfig = obj.toDict() | ||
| return { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rebuilding the config object here drops any extra keys that FilterDictPostProcessorConfigSchema intentionally allows via .passthrough(), so those settings are lost during serialization/deserialization.
Prompt for AI agents
Address the following comment on packages/core/src/implementations/post_processors/filter_dict_post_processor.ts at line 190:
<comment>Rebuilding the config object here drops any extra keys that FilterDictPostProcessorConfigSchema intentionally allows via .passthrough(), so those settings are lost during serialization/deserialization.</comment>
<file context>
@@ -186,7 +186,16 @@ type FilterDictPostProcessorConfig = z.infer<typeof FilterDictPostProcessorConfi
toDict(obj: FilterDictPostProcessor): { [key: string]: any } {
- return obj.toDict();
+ const filterDictConfig = obj.toDict()
+ return {
+ tool_post_processor_type: filterDictConfig.tool_post_processor_type,
+ exclude_keys: filterDictConfig.exclude_keys,
</file context>
| if (apiKeyAuth.api_key) { | ||
| if (apiKeyAuth.location === 'header') { | ||
| // Default to 'header' if location is not specified | ||
| const location = apiKeyAuth.location || 'header'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API key authentication logic for defaulting location and applying API keys is duplicated. This exact pattern exists in packages/http/src/http_communication_protocol.ts (lines 246-252) and packages/http/src/streamable_http_communication_protocol.ts (lines 46-52). This should be extracted into a shared utility function to ensure consistent behavior and easier maintenance for critical authentication logic.
Prompt for AI agents
Address the following comment on packages/http/src/sse_communication_protocol.ts at line 47:
<comment>API key authentication logic for defaulting location and applying API keys is duplicated. This exact pattern exists in `packages/http/src/http_communication_protocol.ts` (lines 246-252) and `packages/http/src/streamable_http_communication_protocol.ts` (lines 46-52). This should be extracted into a shared utility function to ensure consistent behavior and easier maintenance for critical authentication logic.</comment>
<file context>
@@ -43,11 +43,13 @@ export class SseCommunicationProtocol implements CommunicationProtocol {
if (apiKeyAuth.api_key) {
- if (apiKeyAuth.location === 'header') {
+ // Default to 'header' if location is not specified
+ const location = apiKeyAuth.location || 'header';
+ if (location === 'header') {
headers[apiKeyAuth.var_name] = apiKeyAuth.api_key;
</file context>
| toDict(obj: LimitStringsPostProcessor): { [key: string]: any } { | ||
| return obj.toDict(); | ||
| const limitStringsConfig = obj.toDict() | ||
| return { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Manually reconstructing the config here drops any additional keys allowed by the .passthrough() schema, so configurations with custom fields will lose data during serialization. Please preserve the full dictionary returned by obj.toDict().
Prompt for AI agents
Address the following comment on packages/core/src/implementations/post_processors/limit_strings_post_processor.ts at line 128:
<comment>Manually reconstructing the config here drops any additional keys allowed by the `.passthrough()` schema, so configurations with custom fields will lose data during serialization. Please preserve the full dictionary returned by `obj.toDict()`.</comment>
<file context>
@@ -124,7 +124,15 @@ type LimitStringsPostProcessorConfig = z.infer<typeof LimitStringsPostProcessorC
toDict(obj: LimitStringsPostProcessor): { [key: string]: any } {
- return obj.toDict();
+ const limitStringsConfig = obj.toDict()
+ return {
+ tool_post_processor_type: limitStringsConfig.tool_post_processor_type,
+ limit: limitStringsConfig.limit,
</file context>
| try { | ||
| const projectDescTool = await client.searchTools('project description'); | ||
| if (projectDescTool.length > 0) { | ||
| const result = await client.callTool(projectDescTool.name, {}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
projectDescTool is an array of tools from client.searchTools, so accessing .name directly will throw. Index into the array before calling client.callTool.
Prompt for AI agents
Address the following comment on packages/file/README.md at line 107:
<comment>`projectDescTool` is an array of tools from `client.searchTools`, so accessing `.name` directly will throw. Index into the array before calling `client.callTool`.</comment>
<file context>
@@ -0,0 +1,137 @@
+ try {
+ const projectDescTool = await client.searchTools('project description');
+ if (projectDescTool.length > 0) {
+ const result = await client.callTool(projectDescTool.name, {});
+ console.log('Result from "describe_project" tool (first 100 chars):', String(result).substring(0, 100) + '...');
+ }
</file context>
| const result = await client.callTool(projectDescTool.name, {}); | |
| const result = await client.callTool(projectDescTool[0].name, {}); |
| try { | ||
| const staticDataReaderTool = await client.searchTools('read static data'); | ||
| if (staticDataReaderTool.length > 0) { | ||
| const result = await client.callTool(staticDataReaderTool.name, {}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
client.searchTools returns an array of tools, so staticDataReaderTool.name is undefined and the sample would throw at runtime. Use the first element before accessing .name.
Prompt for AI agents
Address the following comment on packages/file/README.md at line 96:
<comment>`client.searchTools` returns an array of tools, so `staticDataReaderTool.name` is undefined and the sample would throw at runtime. Use the first element before accessing `.name`.</comment>
<file context>
@@ -0,0 +1,137 @@
+ try {
+ const staticDataReaderTool = await client.searchTools('read static data');
+ if (staticDataReaderTool.length > 0) {
+ const result = await client.callTool(staticDataReaderTool.name, {});
+ console.log('Result from "read_static_data" tool:', result);
+ }
</file context>
| const result = await client.callTool(staticDataReaderTool.name, {}); | |
| const result = await client.callTool(staticDataReaderTool[0].name, {}); |
|
|
||
| const action = async () => await protocol.registerManual(mockClient, callTemplate); | ||
| await expect(action()).rejects.toThrow(/Either file_path or content must be provided/); | ||
| await expect(async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This expectation wraps an async registerManual call with .toThrow(), which only checks for synchronous throws; the method rejects instead, so the test will always fail. Assert the rejection with .rejects.toThrow() on the promise.
Prompt for AI agents
Address the following comment on packages/file/tests/file_communication_protocol.test.ts at line 165:
<comment>This expectation wraps an async registerManual call with `.toThrow()`, which only checks for synchronous throws; the method rejects instead, so the test will always fail. Assert the rejection with `.rejects.toThrow()` on the promise.</comment>
<file context>
@@ -121,71 +121,60 @@ describe("TextCommunicationProtocol", () => {
- const action = async () => await protocol.registerManual(mockClient, callTemplate);
- await expect(action()).rejects.toThrow(/Either file_path or content must be provided/);
+ await expect(async () => {
+ await protocol.registerManual(mockClient, callTemplate);
+ }).toThrow();
</file context>
| throw new Error(`Variable namespace '${namespace}' contains invalid characters. Only alphanumeric characters and underscores are allowed.`); | ||
| } | ||
|
|
||
| const objString = JSON.stringify(obj); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling JSON.stringify here causes substitute to throw whenever the payload contains a BigInt (or another non-serializable value), regressing support for such inputs compared to the prior implementation.
Prompt for AI agents
Address the following comment on packages/core/src/implementations/default_variable_substitutor.ts at line 80:
<comment>Calling JSON.stringify here causes substitute to throw whenever the payload contains a BigInt (or another non-serializable value), regressing support for such inputs compared to the prior implementation.</comment>
<file context>
@@ -77,6 +77,11 @@ export class DefaultVariableSubstitutor implements VariableSubstitutor {
throw new Error(`Variable namespace '${namespace}' contains invalid characters. Only alphanumeric characters and underscores are allowed.`);
}
+ const objString = JSON.stringify(obj);
+ if (objString && objString.includes('$ref')) {
+ return obj;
</file context>
Summary by cubic
Introduces explicit plugin registration and new plugins for v1.0: a Node-only File protocol for local manuals and a DotEnv variable loader. Also improves the text protocol (browser-friendly), search accuracy, and OpenAPI conversion, with fixes for variable substitution and API key handling.
New Features
Migration
import '@utcp/http'; import '@utcp/file'; import '@utcp/mcp'; import '@utcp/dotenv-loader';).load_variables_fromwith{ variable_loader_type: 'dotenv', env_file_path: '.env' }.