fix(sdk): harden guard client with policy pack schema fields and test coverage#173
fix(sdk): harden guard client with policy pack schema fields and test coverage#173kantorcodes wants to merge 1 commit into
Conversation
…y pack mock Signed-off-by: Michael Kantor <6068672+kantorcodes@users.noreply.github.com>
There was a problem hiding this comment.
Code Review
This pull request updates the RegistryBrokerClient tests by adding a new test case for the delegate method, including idempotencyKey in chat message tests, and updating security policy data. However, the feedback highlights significant discrepancies between the tests and the underlying implementation: the delegate method is being called with an unsupported second argument, and the sendMessage implementation does not yet handle the idempotencyKey field, which will lead to test failures.
| await client.delegate( | ||
| { | ||
| task: 'review this repo', | ||
| context: 'focus on broker chat parity', | ||
| }, | ||
| { | ||
| limit: 2, | ||
| filter: { | ||
| protocols: ['mcp'], | ||
| }, | ||
| workspace: { | ||
| openFiles: ['src/api/chat/contract.ts'], | ||
| }, | ||
| }, | ||
| ); |
There was a problem hiding this comment.
The test attempts to call client.delegate with two separate arguments (a payload and an options object), but the implementation in base-client.ts (line 742) only defines a single request argument. Consequently, the second argument will be ignored at runtime. Furthermore, the assertion at line 564 expects the request body to contain fields from both arguments merged together, which will cause the test to fail unless the implementation is updated to handle this overload.
| agentUrl: 'https://demo.agent', | ||
| sessionId: 'session-1', | ||
| message: 'Hi', | ||
| idempotencyKey: 'chat-send-1', |
There was a problem hiding this comment.
The idempotencyKey field is being passed to sendMessage here and verified in the request body in the assertion at line 1496. However, the implementation of sendMessage in src/services/registry-broker/client/chat.ts (lines 412-429) does not include logic to extract this field from the payload and add it to the request body. This will likely result in a test failure as the field will be missing from the actual outgoing request.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 95643200b2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| { | ||
| limit: 2, | ||
| filter: { | ||
| protocols: ['mcp'], | ||
| }, |
There was a problem hiding this comment.
Call delegate with one merged payload argument
RegistryBrokerClient.delegate currently accepts a single request object (it forwards only the first argument in src/services/registry-broker/client/base-client.ts and search.ts), so the second “options” argument passed here is ignored at runtime. That means this new test cannot observe limit/filter/workspace in the request body and will fail when executed, which breaks CI for this suite.
Useful? React with 👍 / 👎.
| fetchImplementation, | ||
| }); | ||
|
|
||
| await client.delegate( |
There was a problem hiding this comment.
CRITICAL: Invalid method invocation - delegate expects 1 argument but 2 provided
The delegate method signature is delegate(request: DelegationPlanRequest): Promise<DelegationPlanResponse>. The test calls it with two separate objects, which violates the type signature and will cause a TypeScript compilation error.
Additionally, even if this compiled, the implementation would only use the first argument as the request body, causing the assertion to fail.
Merge the two objects into a single DelegationPlanRequest:
| await client.delegate( | |
| await client.delegate({ | |
| task: 'review this repo', | |
| context: 'focus on broker chat parity', | |
| limit: 2, | |
| filter: { | |
| protocols: ['mcp'], | |
| }, | |
| workspace: { | |
| openFiles: ['src/api/chat/contract.ts'], | |
| }, | |
| }); |
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
Other Observations (not in diff)None Files Reviewed (1 file)
Fix these issues in Kilo Cloud Reviewed by step-3.5-flash · 5,355,377 tokens |
Summary
blockedPublishersandblockedDomainsfields (schema alignment)Verification