feat: enhance type safety by extending context classes for agents, jobs, prompts, and resources#302
Merged
frontegg-david merged 9 commits intorelease/1.0.xfrom Mar 22, 2026
Merged
Conversation
…nd elicitation capabilities
…nd improve client capabilities setting
…bs, prompts, and resources
…and error handling
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughTightens TypeScript decorator typings to require classes extend Context base types; updates fixture classes to subclass PromptContext/ResourceContext and make execute() async with new signatures; adds multiple decorator type-safety specs and many unit tests; adjusts Jest coverage thresholds and a plugin config exclusion. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Performance Test ResultsStatus: ✅ All tests passed Summary
Total: 100 tests across 21 projects 📊 View full report in workflow run Generated at: 2026-03-22T03:18:38.892Z |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/plugin-approval/src/__tests__/approval.plugin.spec.ts (1)
129-140:⚠️ Potential issue | 🟠 MajorRemove
anytype casts and use proper provider types from SDK.The test uses repeated
as anyand(p: any)patterns when working with providers. Proper types are available:ProviderType[]is the return type ofdynamicProviders(), and individual providers can be typed asProviderFactoryType<ApprovalService, readonly [...]>(as shown inremember.plugin.spec.tsusingas ValueProvider). Replace all 8 instances ofanywith appropriate provider types from@frontmcp/sdk.Additionally, the userId extraction tests (lines 170–207) are incomplete. They only assert
expect(service).toBeDefined()without verifying the extraction logic itself. Each test should validate the actual extracted userId/fallback value against the expected result.Applies to lines: 129, 140, 147–148, 158–159, 169, 182, 195, 208
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/plugin-approval/src/__tests__/approval.plugin.spec.ts` around lines 129 - 140, The tests should stop using broad any casts and instead import and use the SDK provider types: change usages of ApprovalPlugin.dynamicProviders() to be typed as ProviderType[] and cast individual provider objects to the correct provider factory/value types (e.g., ProviderFactoryType or ValueProvider from `@frontmcp/sdk`) when asserting useFactory/useValue—locate usages around ApprovalPlugin.dynamicProviders and the ChallengeServiceToken provider; also expand the userId extraction tests (the tests that only assert service existence) to call the extraction function (or the provider factory) and assert the actual returned userId or fallback value equals the expected value for each scenario so the tests validate extraction logic rather than just presence.
🧹 Nitpick comments (6)
libs/testing/src/auth/__tests__/user-fixtures.spec.ts (1)
85-117: Add a regression test to ensure defaultscopesarrays are isolated.Current tests verify value defaults, but not reference isolation across calls. A shared default array would create cross-test/user mutation risk.
Suggested test addition
describe('createTestUser', () => { it('should create a user with just sub, defaulting scopes to empty', () => { const user = createTestUser({ sub: 'custom-001' }); expect(user.sub).toBe('custom-001'); expect(user.scopes).toEqual([]); expect(user.email).toBeUndefined(); expect(user.name).toBeUndefined(); expect(user.role).toBeUndefined(); }); + it('should not share default scopes array between users', () => { + const first = createTestUser({ sub: 'custom-iso-001' }); + const second = createTestUser({ sub: 'custom-iso-002' }); + expect(first.scopes).toEqual([]); + expect(second.scopes).toEqual([]); + expect(first.scopes).not.toBe(second.scopes); + }); + it('should allow overriding all properties', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/testing/src/auth/__tests__/user-fixtures.spec.ts` around lines 85 - 117, Add a regression test that verifies createTestUser returns an isolated scopes array per call (mutating one user's scopes should not affect another). In the test file referencing createTestUser and TestUserFixture, create two users (e.g., userA and userB) without passing scopes, push or modify userA.scopes, then assert userB.scopes remains unchanged (still an empty array) and that userA.scopes reflects only the mutation on userA. This ensures the default scopes array is not shared between instances.plugins/plugin-approval/src/__tests__/approval.plugin.spec.ts (1)
166-214: UserId extraction tests are currently non-verifying.Each case only checks
serviceis defined, so these tests can still pass if fallback extraction (extra.userId→extra.sub→clientId) breaks. Please assert the resolved identity via a public service output or an observable downstream call argument.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/plugin-approval/src/__tests__/approval.plugin.spec.ts` around lines 166 - 214, The tests currently only assert the service is defined, so update each spec to verify the resolved identity chosen by the factory (ApprovalPlugin.dynamicProviders and the provider whose provide === ApprovalServiceToken) by either invoking a public method on the returned service that returns the resolved userId (e.g., service.getUserId() or similar) or by mocking/instrumenting the downstream dependency passed into useFactory (mockStore) to capture the identity argument; replace the generic expect(service).toBeDefined() with an assertion that the returned identity equals the expected value for each ctx (expect(resolvedIdentity).toBe('user-from-extra'), 'sub-user', 'fallback-client', and an appropriate value/undefined for missing authInfo).libs/sdk/src/common/decorators/prompt.decorator.ts (1)
111-122: Consider adding justification comment foranyusage in type helpers.The
anytypes in__Ctor(lines 112) are necessary for TypeScript's constructor type inference in decorator patterns—usingunknown[]would break the instance type extraction in__R<C>. Per coding guidelines,anyusage should be justified.📝 Suggested documentation
// ---------- ctor & reflection ---------- +// Note: `any` is required here for TypeScript's constructor type inference. +// Using `unknown[]` would prevent proper instance type extraction in __R<C>. type __Ctor = (new (...a: any[]) => any) | (abstract new (...a: any[]) => any);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/common/decorators/prompt.decorator.ts` around lines 111 - 122, Add a brief in-code justification comment above the type helpers explaining why `any` is used in __Ctor (and that using unknown[] breaks constructor inference and instance type extraction in __R<C>), and note that this is intentional for decorator/constructor inference; reference the types __Ctor, __R, and __MustExtendPromptCtx so reviewers understand the rationale and that the `any` usage is deliberate and constrained to these helper types.libs/sdk/src/common/decorators/__tests__/agent-type-safety.spec.ts (1)
66-81: Consider adding a test for class not extending AgentContext.The Job, Prompt, Resource, and Tool specs all include an invalid case where the class doesn't extend the required context. Adding this case would maintain consistency:
📝 Missing test case for consistency
// ── Invalid: class not extending AgentContext ────────────────── function _testNotAgentContext() { // `@ts-expect-error` - class must extend AgentContext `@Agent`({ name: 'not-agent-context', inputSchema: { topic: z.string() }, llm: { provider: 'openai', model: 'gpt-4', apiKey: 'test-key' }, }) class NotAgentContext { async execute(input: { topic: string }) { return { result: input.topic }; } } void NotAgentContext; }And add to suppress warnings:
void _testNotAgentContext;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/common/decorators/__tests__/agent-type-safety.spec.ts` around lines 66 - 81, Add a new invalid test that mirrors other specs: create a function named _testNotAgentContext that applies the `@Agent` decorator to a class NotAgentContext which does NOT extend AgentContext and implements execute(input: { topic: string }) to trigger the expected TypeScript error; annotate the class declaration line with // `@ts-expect-error` - class must extend AgentContext and ensure you add a void _testNotAgentContext; call at the end to suppress unused warnings so the test file stays consistent with Job/Prompt/Resource/Tool specs.libs/sdk/src/common/decorators/__tests__/prompt-type-safety.spec.ts (1)
29-38: Consider adding an invalid return type test case.The test validates context inheritance but doesn't verify return type constraints. Adding a case where
execute()returns an incorrect structure could strengthen compile-time safety coverage (similar to Job/Tool specs).📝 Example additional test case
// ── Invalid: Prompt with wrong return type ──────────────────── function _testWrongReturnType() { // `@ts-expect-error` - execute() must return GetPromptResult `@Prompt`({ name: 'wrong-return', arguments: [] }) class WrongReturnPrompt extends PromptContext { async execute(args: Record<string, string>): Promise<{ invalid: string }> { return { invalid: 'not-a-prompt-result' }; } } void WrongReturnPrompt; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/common/decorators/__tests__/prompt-type-safety.spec.ts` around lines 29 - 38, Add a compile-time test that ensures execute() must return GetPromptResult by introducing a new local test function (e.g., _testWrongReturnType) decorated with `@Prompt` and extending PromptContext (use class name WrongReturnPrompt) where execute(args: Record<string,string>) is declared to return an incorrect type (e.g., Promise<{ invalid: string }>) and include a // `@ts-expect-error` comment above the class to assert the TypeScript error; this mirrors the existing _testNotPromptContext pattern but verifies wrong return type instead of inheritance.libs/sdk/src/common/decorators/tool.decorator.ts (1)
332-336: Preferunknownin the no-output overload.
ToolMetadataOptions<I, any>leaksanyback into a strict public API and makesui/return typing collapse toanywheneveroutputSchemais omitted. A dedicated no-output options type usingunknownwould keep this overload flexible without opting consumers out of type safety. As per coding guidelines "Use strict TypeScript type checking with noanytypes without justification; useunknowninstead ofanyfor generic type defaults".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/common/decorators/tool.decorator.ts` around lines 332 - 336, The no-output overload for Tool currently uses ToolMetadataOptions<I, any>, which leaks any; replace that with a dedicated no-output options type (e.g. ToolMetadataNoOutputOptions<I> or use ToolMetadataOptions<I, unknown>) so the overload uses unknown instead of any; update the overload signature in the Tool declaration to reference the new type (or change the second generic to unknown) and ensure related generic constraints (__MustParam, __MustReturn, ToolInputOf, ToolOutputOf) still align with the no-output variant so consumers keep strict typing when outputSchema is omitted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/sdk/src/common/decorators/agent.decorator.ts`:
- Around line 327-339: The two Agent decorator overload signatures (export
function Agent... for both I/O and input-only variants) are missing the context
inheritance constraint; update each overload's generic class intersection to
include __MustExtendCtx<C> (the same way `@Tool/`@Job/@Resource do) so the
decorated class is validated to extend AgentContext instead of just matching
AgentContextBase; locate the Agent overload declarations and add
__MustExtendCtx<C> into the &-joined type constraints for C in both overloads.
In `@libs/sdk/src/common/decorators/job.decorator.ts`:
- Around line 211-216: The overload in job.decorator.ts using JobMetadata<I,
any> & { outputSchema?: never } is unreachable because JobMetadata requires
outputSchema; remove this impossible overload branch (the <I extends
__Shape>(opts: JobMetadata<I, any> & { outputSchema?: never }) signature and its
corresponding returned function type) from the job decorator overload set, or
alternatively make outputSchema optional on JobMetadata in job.metadata.ts if
you intend to support a no-output-schema case—pick one approach and update the
overloads/signature of the Job decorator accordingly (referencing JobMetadata
and the overload signatures in job.decorator.ts).
- Around line 118-135: The decorator's output-type union (__OutputSchema via
__PrimitiveOutputType/__JobSingleOutputType) currently omits the ToolOutputType
literals ('image','audio','resource','resource_link'), causing valid JobMetadata
to be rejected; update __PrimitiveOutputType (or replace it with a union that
includes ToolOutputType literals or import/extend ToolOutputType) so
__OutputSchema accepts those media/resource string literals (and arrays thereof)
alongside the existing zod types. Also fix overload 2 by removing the use of raw
any and the incorrect outputSchema?: never constraint: make the overload generic
(e.g., JobMetadata<I, T> with T constrained to the same output schema type used
elsewhere) or remove the overload if redundant, ensuring it matches
JobMetadata's required outputSchema type rather than forcing it optional/never.
In `@libs/testing/jest.config.ts`:
- Around line 21-24: The global Jest coverageThreshold in
libs/testing/jest.config.ts currently sets statements/branches/functions/lines
to low values (the keys "statements", "branches", "functions", "lines" in the
coverageThreshold object) which violates the repo policy; update the
coverageThreshold to enforce the repository minimum by raising each of those
metrics to at least 95 (e.g., set statements: 95, branches: 95, functions: 95,
lines: 95) so Jest fails when coverage drops below the required threshold.
In `@libs/testing/src/assertions/__tests__/mcp-assertions.spec.ts`:
- Around line 51-57: Replace the widespread unsafe "as any" and "as any[]" casts
in this spec (e.g., the response fixture in the "should handle response with
error but no message gracefully" test and other fixtures listed) with proper
typed fixtures: use explicit inline types matching the expected interface, or
Partial<T> for intentionally incomplete/edge-case objects, or typed arrays
(e.g., Array<YourType>) so tests compile under strict TypeScript; update each
fixture variable (like response, the arrays of { name: 'a' }, and other mocks)
to use the correct interface/type instead of any and remove casts to "as
any"/"as any[]". Ensure failing/undefined fields are represented via Partial<T>
or optional properties so behavior stays the same while preserving strict
typing.
In `@libs/testing/src/interceptor/__tests__/mock-registry.spec.ts`:
- Line 84: Replace non-null assertions with explicit runtime/type-safe guards:
for each use of result! (e.g., where you already call
expect(result).toBeDefined()) change to assert then use the guarded variable
(e.g., expect(result).toBeDefined(); if (!result) return; /* now use
result.response */) and similarly for places that access result.response or
resp.error (add expect(resp).toBeDefined(); expect(resp.error).toBeDefined(); or
an if-check before using resp.error). For mockResponse.errors.*() calls,
explicitly guard the returned resp.error with expect(...).toBeDefined() or an
if-check before asserting on its fields. Update all occurrences referenced (uses
of result!, result.response, resp.error!, mockResponse.errors.*()) to this
pattern to satisfy strict TypeScript without using "!".
In `@libs/testing/src/platform/__tests__/platform-client-info.spec.ts`:
- Around line 30-33: The test uses 'unknown' which is actually a recognized
TestPlatformType, so update the spec to exercise the default switch branch by
calling getPlatformClientInfo with a truly unrecognized value (e.g., a string
that is not in TestPlatformType or by casting a unique string as unknown to
bypass the type, such as 'not-a-platform' or ('invalid-platform' as unknown as
TestPlatformType)); ensure the assertion still expects the fallback { name:
'mcp-test-client', version: '1.0' } and reference getPlatformClientInfo to
locate the test to change.
- Around line 115-117: Remove the non-null assertion on caps.experimental and
add an explicit guard before accessing MCP_APPS_EXTENSION_KEY: after the
existing expect(caps.experimental).toBeDefined(), assign or narrow
caps.experimental by checking if (!caps.experimental) throw or return so the
type is guaranteed non-null, then access experimental[MCP_APPS_EXTENSION_KEY]
(referencing caps, experimental, and MCP_APPS_EXTENSION_KEY) to assert its
value; this preserves the runtime check and avoids using the ! operator.
In `@plugins/plugin-approval/jest.config.ts`:
- Around line 41-47: The Jest config weakens the repository coverage gate by
lowering branch coverage to 90 and excluding src/approval.plugin.ts from
collectCoverageFrom; revert these changes by removing the exclusion for
'src/approval.plugin.ts' from the collectCoverageFrom array and restore the
coverageThreshold (branches) to at least 95 (or match the repository policy) in
the jest configuration so that all metrics (statements, branches, functions,
lines) enforce 95%+; locate references to collectCoverageFrom and
coverageThreshold in jest.config.ts to make these edits.
---
Outside diff comments:
In `@plugins/plugin-approval/src/__tests__/approval.plugin.spec.ts`:
- Around line 129-140: The tests should stop using broad any casts and instead
import and use the SDK provider types: change usages of
ApprovalPlugin.dynamicProviders() to be typed as ProviderType[] and cast
individual provider objects to the correct provider factory/value types (e.g.,
ProviderFactoryType or ValueProvider from `@frontmcp/sdk`) when asserting
useFactory/useValue—locate usages around ApprovalPlugin.dynamicProviders and the
ChallengeServiceToken provider; also expand the userId extraction tests (the
tests that only assert service existence) to call the extraction function (or
the provider factory) and assert the actual returned userId or fallback value
equals the expected value for each scenario so the tests validate extraction
logic rather than just presence.
---
Nitpick comments:
In `@libs/sdk/src/common/decorators/__tests__/agent-type-safety.spec.ts`:
- Around line 66-81: Add a new invalid test that mirrors other specs: create a
function named _testNotAgentContext that applies the `@Agent` decorator to a class
NotAgentContext which does NOT extend AgentContext and implements execute(input:
{ topic: string }) to trigger the expected TypeScript error; annotate the class
declaration line with // `@ts-expect-error` - class must extend AgentContext and
ensure you add a void _testNotAgentContext; call at the end to suppress unused
warnings so the test file stays consistent with Job/Prompt/Resource/Tool specs.
In `@libs/sdk/src/common/decorators/__tests__/prompt-type-safety.spec.ts`:
- Around line 29-38: Add a compile-time test that ensures execute() must return
GetPromptResult by introducing a new local test function (e.g.,
_testWrongReturnType) decorated with `@Prompt` and extending PromptContext (use
class name WrongReturnPrompt) where execute(args: Record<string,string>) is
declared to return an incorrect type (e.g., Promise<{ invalid: string }>) and
include a // `@ts-expect-error` comment above the class to assert the TypeScript
error; this mirrors the existing _testNotPromptContext pattern but verifies
wrong return type instead of inheritance.
In `@libs/sdk/src/common/decorators/prompt.decorator.ts`:
- Around line 111-122: Add a brief in-code justification comment above the type
helpers explaining why `any` is used in __Ctor (and that using unknown[] breaks
constructor inference and instance type extraction in __R<C>), and note that
this is intentional for decorator/constructor inference; reference the types
__Ctor, __R, and __MustExtendPromptCtx so reviewers understand the rationale and
that the `any` usage is deliberate and constrained to these helper types.
In `@libs/sdk/src/common/decorators/tool.decorator.ts`:
- Around line 332-336: The no-output overload for Tool currently uses
ToolMetadataOptions<I, any>, which leaks any; replace that with a dedicated
no-output options type (e.g. ToolMetadataNoOutputOptions<I> or use
ToolMetadataOptions<I, unknown>) so the overload uses unknown instead of any;
update the overload signature in the Tool declaration to reference the new type
(or change the second generic to unknown) and ensure related generic constraints
(__MustParam, __MustReturn, ToolInputOf, ToolOutputOf) still align with the
no-output variant so consumers keep strict typing when outputSchema is omitted.
In `@libs/testing/src/auth/__tests__/user-fixtures.spec.ts`:
- Around line 85-117: Add a regression test that verifies createTestUser returns
an isolated scopes array per call (mutating one user's scopes should not affect
another). In the test file referencing createTestUser and TestUserFixture,
create two users (e.g., userA and userB) without passing scopes, push or modify
userA.scopes, then assert userB.scopes remains unchanged (still an empty array)
and that userA.scopes reflects only the mutation on userA. This ensures the
default scopes array is not shared between instances.
In `@plugins/plugin-approval/src/__tests__/approval.plugin.spec.ts`:
- Around line 166-214: The tests currently only assert the service is defined,
so update each spec to verify the resolved identity chosen by the factory
(ApprovalPlugin.dynamicProviders and the provider whose provide ===
ApprovalServiceToken) by either invoking a public method on the returned service
that returns the resolved userId (e.g., service.getUserId() or similar) or by
mocking/instrumenting the downstream dependency passed into useFactory
(mockStore) to capture the identity argument; replace the generic
expect(service).toBeDefined() with an assertion that the returned identity
equals the expected value for each ctx
(expect(resolvedIdentity).toBe('user-from-extra'), 'sub-user',
'fallback-client', and an appropriate value/undefined for missing authInfo).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 66c62667-f319-4e49-8d5e-70504c780570
📒 Files selected for processing (26)
apps/e2e/demo-e2e-esm/src/esm-package-server/fixtures/decorated-package.tsapps/e2e/demo-e2e-esm/src/esm-package-server/fixtures/prompts-only-package.tsapps/e2e/demo-e2e-esm/src/esm-package-server/fixtures/resources-only-package.tslibs/sdk/src/common/decorators/__tests__/agent-type-safety.spec.tslibs/sdk/src/common/decorators/__tests__/job-type-safety.spec.tslibs/sdk/src/common/decorators/__tests__/prompt-type-safety.spec.tslibs/sdk/src/common/decorators/__tests__/resource-type-safety.spec.tslibs/sdk/src/common/decorators/__tests__/tool-type-safety.spec.tslibs/sdk/src/common/decorators/agent.decorator.tslibs/sdk/src/common/decorators/job.decorator.tslibs/sdk/src/common/decorators/prompt.decorator.tslibs/sdk/src/common/decorators/resource.decorator.tslibs/sdk/src/common/decorators/tool.decorator.tslibs/testing/jest.config.tslibs/testing/src/assertions/__tests__/mcp-assertions.spec.tslibs/testing/src/auth/__tests__/auth-headers.spec.tslibs/testing/src/auth/__tests__/user-fixtures.spec.tslibs/testing/src/errors/__tests__/errors.spec.tslibs/testing/src/interceptor/__tests__/mock-registry.spec.tslibs/testing/src/matchers/__tests__/mcp-matchers.spec.tslibs/testing/src/platform/__tests__/platform-client-info.spec.tslibs/testing/src/platform/__tests__/platform-types.spec.tsplugins/plugin-approval/jest.config.tsplugins/plugin-approval/src/__tests__/approval-storage.store.spec.tsplugins/plugin-approval/src/__tests__/approval.plugin.spec.tsplugins/plugin-approval/src/approval/__tests__/errors.spec.ts
…and error handling
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
libs/testing/src/interceptor/__tests__/mock-registry.spec.ts (1)
328-339:⚠️ Potential issue | 🟡 MinorRemove the remaining non-null assertions.
You already use the right narrowing pattern at Line 84, but these
result!.response/resp.error!sites still bypass strict typing. Apply the same guard pattern before dereferencing.♻️ Suggested pattern
const result = registry.match(makeRequest('tools/list')); - expect(result!.response).toBe(resp1); + expect(result).toBeDefined(); + if (!result) return; + expect(result.response).toBe(resp1); const resp = mockResponse.errors.methodNotFound('tools/call'); - expect(resp.error!.code).toBe(-32601); + expect(resp.error).toBeDefined(); + if (!resp.error) return; + expect(resp.error.code).toBe(-32601);As per coding guidelines, "Use proper error handling instead of non-null assertions. Throw specific error types when values are not found, rather than using non-null assertion operator (!)".
#!/bin/bash # Find remaining non-null assertions in this spec. rg -nP '!\.' libs/testing/src/interceptor/__tests__/mock-registry.spec.tsThis should still report the
result!.responseandresp.error!sites called out above.Also applies to: 377-379, 472-512
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/testing/src/interceptor/__tests__/mock-registry.spec.ts` around lines 328 - 339, Replace all non-null assertion usages when accessing match results and mock response error fields (e.g., result!.response, resp.error!) with explicit guards: call registry.match(makeRequest(...)) into a const (e.g., const result = registry.match(...)); if result is undefined throw a descriptive Error or use an expect/assert to fail the test before accessing result.response; similarly check if resp.error is defined before dereferencing or throw/assert with a clear message. Apply this pattern to the shown test cases using registry.match, makeRequest, and mockResponse and to the other occurrences that still use ! on result/resp so no non-null assertions remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/sdk/src/common/decorators/job.decorator.ts`:
- Around line 201-214: The FrontMcpJob export still exposes the loose
(providedMetadata: JobMetadata): ClassDecorator signature while Job uses the
strict JobDecorator overloads; update FrontMcpJob to use the same strict typing
by changing its declaration to the JobDecorator type (or replicate the strict
overloads) so it enforces the input/output schema checks, and then update
exports to either stop exporting the old loose signature or mark FrontMcpJob as
deprecated; refer to the FrontMcpJob identifier, the Job alias, the JobDecorator
interface and its esm/remote members (jobEsm, jobRemote) when making the change.
- Around line 182-191: The __MustReturn type currently only checks __IsAny<Out>
and misses when the actual execute() return type is any; update __MustReturn to
also guard against __IsAny<__Unwrap<__Return<C>>> (mirror the pattern used in
__MustParam) so that an actual any return fails validation, and add a negative
type test in job-type-safety.spec.ts (an `@ts-expect-error` test with a class
implementing async execute(): any { return 'any-result'; }) to prevent
regressions; reference the types __MustReturn, __IsAny, __Unwrap, __Return and
the existing __MustParam pattern when making the change.
- Line 150: The type __Param currently only infers the first parameter of
execute via `(arg: infer P, ...r: any)` which allows extra required parameters
to slip through; change the check to use Parameters<__R<C>['execute']> (or an
equivalent conditional) to ensure the entire parameter tuple exactly equals the
single-element tuple [In] (e.g. `Parameters<__R<C>['execute']> extends [In] ?
...` and the inverse to reject extra params), and add a // `@ts-expect-error` test
case in the decorator tests where a class defines execute(input: In, extra:
string) to assert the extra required parameter is rejected by the type guard;
reference __Param, __Ctor, __R and the execute method when making the change.
In `@libs/testing/src/interceptor/__tests__/mock-registry.spec.ts`:
- Around line 5-6: The helper makeRequest currently always sets a params
property (possibly undefined); change it so the returned JsonRpcRequest omits
params entirely when params is undefined. Modify makeRequest(method, params?,
id?) to build the result with jsonrpc and id, and only include the params key
when params !== undefined (e.g. via a conditional spread or an if that assigns
params), keeping the existing id default behavior; this ensures the produced
wire shape matches the JsonRpcRequest optional params definition.
---
Duplicate comments:
In `@libs/testing/src/interceptor/__tests__/mock-registry.spec.ts`:
- Around line 328-339: Replace all non-null assertion usages when accessing
match results and mock response error fields (e.g., result!.response,
resp.error!) with explicit guards: call registry.match(makeRequest(...)) into a
const (e.g., const result = registry.match(...)); if result is undefined throw a
descriptive Error or use an expect/assert to fail the test before accessing
result.response; similarly check if resp.error is defined before dereferencing
or throw/assert with a clear message. Apply this pattern to the shown test cases
using registry.match, makeRequest, and mockResponse and to the other occurrences
that still use ! on result/resp so no non-null assertions remain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d9ea21ec-f48c-41be-a51c-7a93069aaf05
📒 Files selected for processing (11)
libs/sdk/src/common/decorators/__tests__/agent-type-safety.spec.tslibs/sdk/src/common/decorators/agent.decorator.tslibs/sdk/src/common/decorators/job.decorator.tslibs/sdk/src/common/decorators/prompt.decorator.tslibs/testing/src/assertions/__tests__/mcp-assertions.spec.tslibs/testing/src/auth/__tests__/user-fixtures.spec.tslibs/testing/src/interceptor/__tests__/mock-registry.spec.tslibs/testing/src/platform/__tests__/platform-client-info.spec.tsplugins/plugin-approval/jest.config.tsplugins/plugin-approval/src/__tests__/approval.plugin.spec.tsplugins/plugin-approval/src/approval/__tests__/errors.spec.ts
✅ Files skipped from review due to trivial changes (3)
- plugins/plugin-approval/src/approval/tests/errors.spec.ts
- libs/testing/src/platform/tests/platform-client-info.spec.ts
- libs/sdk/src/common/decorators/prompt.decorator.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- plugins/plugin-approval/jest.config.ts
- libs/sdk/src/common/decorators/tests/agent-type-safety.spec.ts
- libs/testing/src/auth/tests/user-fixtures.spec.ts
- plugins/plugin-approval/src/tests/approval.plugin.spec.ts
- libs/testing/src/assertions/tests/mcp-assertions.spec.ts
- libs/sdk/src/common/decorators/agent.decorator.ts
…rove error handling
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
libs/testing/src/interceptor/__tests__/mock-registry.spec.ts (1)
5-7: Prefer a ternary object spread inmakeRequestfor stricter TypeScript compatibility.On line 6,
...(params !== undefined && { params })produces afalse | objectunion that violates TypeScript's strict spread type checking (TS2698). TypeScript requires spread expressions to be unambiguously object-typed. Use a ternary to guarantee an object:function makeRequest(method: string, params?: Record<string, unknown>, id?: string | number): JsonRpcRequest { - return { jsonrpc: '2.0', id: id ?? 1, method, ...(params !== undefined && { params }) }; + return { jsonrpc: '2.0', id: id ?? 1, method, ...(params !== undefined ? { params } : {}) }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/testing/src/interceptor/__tests__/mock-registry.spec.ts` around lines 5 - 7, The makeRequest helper's spread uses the expression ...(params !== undefined && { params }) which produces a false | object union and fails strict TypeScript spread checking; update the spread to a ternary that always yields an object (e.g., ...(params !== undefined ? { params } : {})) so the spread is unambiguously object-typed; adjust the makeRequest signature/return if needed and keep references to JsonRpcRequest and the makeRequest function name when making the change.libs/sdk/src/common/decorators/job.decorator.ts (1)
196-200: Consider usingunknowninstead ofanyfor the first twoJobContexttype parameters.Per coding guidelines, prefer
unknownoveranyfor generic type defaults. TheJobContext<any, any, In, Out>could beJobContext<unknown, unknown, In, Out>to maintain stricter type safety while still allowing flexibility in the schema type positions.♻️ Suggested change
type __Rewrap<C extends __Ctor, In, Out> = C extends abstract new (...a: __A<C>) => __R<C> - ? C & (abstract new (...a: __A<C>) => JobContext<any, any, In, Out> & __R<C>) + ? C & (abstract new (...a: __A<C>) => JobContext<unknown, unknown, In, Out> & __R<C>) : C extends new (...a: __A<C>) => __R<C> - ? C & (new (...a: __A<C>) => JobContext<any, any, In, Out> & __R<C>) + ? C & (new (...a: __A<C>) => JobContext<unknown, unknown, In, Out> & __R<C>) : never;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/common/decorators/job.decorator.ts` around lines 196 - 200, The __Rewrap type uses JobContext<any, any, In, Out> which should be stricter: update both occurrences inside the conditional branches to JobContext<unknown, unknown, In, Out> (i.e., change the first two generic parameters from any to unknown) so the type alias __Rewrap preserves the same shape but uses unknown for the schema-type positions; ensure you change it in both the abstract new (...) and new (...) branches of the __Rewrap definition.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/sdk/src/common/decorators/agent.decorator.ts`:
- Around line 345-346: The public overload of Agent currently uses
AgentMetadataOptions<I, any> for the no-outputSchema variant; change this to use
AgentMetadataOptions<I, __OutputSchema> (or otherwise reference __OutputSchema)
so you remove the unsafe any and preserve type strictness, and ensure the
overload signature still returns AgentOutputOf<{}> and that the outputSchema
optional path in the Agent function implementation/type resolution remains
compatible with AgentMetadataOptions, AgentOutputOf and the __OutputSchema
symbol.
In `@libs/sdk/src/common/decorators/job.decorator.ts`:
- Around line 129-134: The union/discriminated-union Zod types are using
incorrect generics and unsafe any types: update the type alias (e.g.,
__StructuredOutputType) and the same patterns in tool.decorator.ts,
agent.decorator.ts, tool.metadata.ts and flow.metadata.ts to (1) supply the
missing discriminator type parameter to z.ZodDiscriminatedUnion (e.g., add ",
string" as the second generic) and (2) replace z.ZodObject<any> with a safer
generic like z.ZodObject<unknown> (or z.ZodObject<z.ZodRawShape>), ensuring the
ZodUnion and ZodDiscriminatedUnion signatures match Zod 4.x. Ensure all
occurrences of ZodArray/ZodType/ZodUnion/ZodDiscriminatedUnion in those files
follow the corrected generics.
---
Nitpick comments:
In `@libs/sdk/src/common/decorators/job.decorator.ts`:
- Around line 196-200: The __Rewrap type uses JobContext<any, any, In, Out>
which should be stricter: update both occurrences inside the conditional
branches to JobContext<unknown, unknown, In, Out> (i.e., change the first two
generic parameters from any to unknown) so the type alias __Rewrap preserves the
same shape but uses unknown for the schema-type positions; ensure you change it
in both the abstract new (...) and new (...) branches of the __Rewrap
definition.
In `@libs/testing/src/interceptor/__tests__/mock-registry.spec.ts`:
- Around line 5-7: The makeRequest helper's spread uses the expression
...(params !== undefined && { params }) which produces a false | object union
and fails strict TypeScript spread checking; update the spread to a ternary that
always yields an object (e.g., ...(params !== undefined ? { params } : {})) so
the spread is unambiguously object-typed; adjust the makeRequest
signature/return if needed and keep references to JsonRpcRequest and the
makeRequest function name when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4ccb658b-616d-4280-80fe-aa630eec9b6b
📒 Files selected for processing (4)
libs/sdk/src/common/decorators/agent.decorator.tslibs/sdk/src/common/decorators/job.decorator.tslibs/sdk/src/common/decorators/tool.decorator.tslibs/testing/src/interceptor/__tests__/mock-registry.spec.ts
…rove error handling
…rove error handling
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
libs/testing/src/interceptor/__tests__/mock-registry.spec.ts (1)
376-384: Consider usingtoMatchObjectto avoid brittleness withundefinedproperties.Line 382 asserts
data: undefinedexplicitly. If the implementation changes to omit thedataproperty when undefined (which is common for JSON serialization), this test will fail. UsingtoMatchObjector checkingresp.error?.dataseparately would be more resilient.♻️ Proposed fix
describe('error', () => { it('should create a valid JSON-RPC error response', () => { const resp = mockResponse.error(-32600, 'Invalid Request'); - expect(resp).toEqual({ + expect(resp).toMatchObject({ jsonrpc: '2.0', id: 1, - error: { code: -32600, message: 'Invalid Request', data: undefined }, + error: { code: -32600, message: 'Invalid Request' }, }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/testing/src/interceptor/__tests__/mock-registry.spec.ts` around lines 376 - 384, The test asserts an explicit data: undefined which makes it brittle; update the spec that calls mockResponse.error in the 'error' test to use a looser assertion (e.g., expect(resp).toMatchObject({...}) or assert resp.error?.data === undefined separately) so the test doesn't fail if the implementation omits the data property when undefined; locate the call to mockResponse.error and replace the strict toEqual expectation with a toMatchObject-based check that verifies jsonrpc, id and error.code/message while not requiring an explicit data key.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@libs/testing/src/interceptor/__tests__/mock-registry.spec.ts`:
- Around line 376-384: The test asserts an explicit data: undefined which makes
it brittle; update the spec that calls mockResponse.error in the 'error' test to
use a looser assertion (e.g., expect(resp).toMatchObject({...}) or assert
resp.error?.data === undefined separately) so the test doesn't fail if the
implementation omits the data property when undefined; locate the call to
mockResponse.error and replace the strict toEqual expectation with a
toMatchObject-based check that verifies jsonrpc, id and error.code/message while
not requiring an explicit data key.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c93ac18e-a2df-48ba-afdd-6ef2231364da
📒 Files selected for processing (3)
libs/sdk/src/common/decorators/job.decorator.tslibs/sdk/src/common/decorators/tool.decorator.tslibs/testing/src/interceptor/__tests__/mock-registry.spec.ts
Contributor
Cherry-pick CreatedA cherry-pick PR to Please review and merge if this change should also be in If the cherry-pick is not needed, close the PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Tests
Chores