refactor: update type definitions and improve scope handling across files#310
Merged
frontegg-david merged 13 commits intorelease/1.0.xfrom Mar 27, 2026
Merged
refactor: update type definitions and improve scope handling across files#310frontegg-david merged 13 commits intorelease/1.0.xfrom
frontegg-david merged 13 commits intorelease/1.0.xfrom
Conversation
…h --target option
Contributor
📝 WalkthroughWalkthroughRefactors SDK typing and scope plumbing: removes empty marker interfaces, replaces interface-based registry types with concrete registry classes, migrates many Changes
Sequence Diagram(s)sequenceDiagram
participant Flow as Flow/Tool/Prompt
participant ScopeEntry as ScopeEntry
participant Hooks as HookRegistry
participant Provider as ProviderRegistry
Flow->>ScopeEntry: read scope.hooks
ScopeEntry->>Hooks: return HookRegistry
Flow->>Hooks: register/emit hooks
Flow->>ScopeEntry: (when needed) call runFlowForOutput(...)
ScopeEntry->>Provider: resolve additional deps (optional)
Provider->>Flow: provide dependencies
sequenceDiagram
participant Flow as Transport Flow
participant ScopeEntry as ScopeEntry
participant Transport as TransportService
participant Store as ElicitationStore
Flow->>ScopeEntry: access transportService
alt transportService present
ScopeEntry->>Transport: return instance
Flow->>Transport: proceed with send/handle
else transportService missing
ScopeEntry->>Flow: undefined
Flow->>Flow: throw TransportServiceNotAvailableError
end
Flow->>ScopeEntry: access elicitationStore
alt elicitationStore present
ScopeEntry->>Store: return store
Flow->>Store: setPending / subscribeResult / cleanup
else elicitationStore missing
ScopeEntry->>Flow: undefined
Flow->>Flow: throw or reject (ElicitationStoreNotInitializedError / InternalMcpError)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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: 101 tests across 21 projects 📊 View full report in workflow run Generated at: 2026-03-27T01:46:14.802Z |
![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)
libs/sdk/src/transport/flows/handle.sse.flow.ts (1)
17-17:⚠️ Potential issue | 🟡 MinorRemove unused
Scopeimport from line 17.The
Scopeclass is imported but never used in this file. Removing it eliminates unnecessary dead code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/transport/flows/handle.sse.flow.ts` at line 17, Remove the unused import of Scope from the top-level imports in handle.sse.flow.ts: locate the import statement "import { Scope } from '../../scope';" and delete it so there is no dead/unused import remaining; ensure remaining imports still satisfy lint rules and build.
🧹 Nitpick comments (5)
libs/sdk/src/elicitation/flows/elicitation-result.flow.ts (1)
14-14: Remove unused import.The
Scopetype import appears to be unused after the refactor toScopeEntry-based access. This should be cleaned up.🧹 Suggested fix
-import type { Scope } from '../../scope';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/elicitation/flows/elicitation-result.flow.ts` at line 14, The import "Scope" is no longer used after switching to ScopeEntry-based access; remove the unused type import (the `Scope` identifier) from the import statement so only needed symbols (e.g., ScopeEntry) remain and eliminate the dead import in elicitation-result.flow.ts where `import type { Scope } from '../../scope';` currently appears.libs/sdk/src/common/interfaces/app.interface.ts (1)
6-8: Consider usingunknowninstead ofanyfor the type parameter default.While the comment provides justification for
any("broad compatibility with untyped app classes"), the coding guidelines preferunknownoveranyfor generic type defaults. Usingunknownwould provide better type safety while still allowing broad compatibility through explicit type narrowing.If this change would cause breaking changes for existing consumers, the current approach with the eslint-disable comment is acceptable.
♻️ Suggested change
// Using 'unknown' default for type safety with generic app classes -// eslint-disable-next-line `@typescript-eslint/no-explicit-any` -export type AppType<T = any> = Type<T> | AppValueType<T> | RemoteAppMetadata; +export type AppType<T = unknown> = Type<T> | AppValueType<T> | RemoteAppMetadata;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/common/interfaces/app.interface.ts` around lines 6 - 8, Replace the generic default from `any` to `unknown` in the `AppType` alias (i.e., change `export type AppType<T = any>` to `export type AppType<T = unknown>`), remove the `// eslint-disable-next-line `@typescript-eslint/no-explicit-any`` line, and update the preceding comment to reflect the use of `unknown` for broader safety while preserving compatibility via explicit narrowing; ensure references to `Type<T>`, `AppValueType<T>`, and `RemoteAppMetadata` remain unchanged.libs/sdk/src/agent/flows/call-agent.flow.ts (1)
16-16: Remove unusedScopeimport.The
Scopeimport on line 16 is not used anywhere in this file and should be removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/agent/flows/call-agent.flow.ts` at line 16, Remove the unused import Scope from the top of the file: delete the import token "Scope" from the import statement that currently reads "import { Scope } from '../../scope';" so the file no longer imports Scope (ensure any remaining imports on that line are preserved if shared).libs/sdk/src/transport/flows/handle.streamable-http.flow.ts (1)
304-307: Consider using a specific error class for transport service unavailability.The guard clause pattern is consistent across all stage methods, which is good. However, per coding guidelines, specific error classes with MCP error codes are preferred over generic
Error. This error is thrown in 5 places (onInitialize,onElicitResult,onMessage,onSseListener,onExtApps).♻️ Suggested error class approach
Create a specific error class (e.g.,
TransportServiceNotAvailableError) with an appropriate MCP error code (likelyINTERNAL_ERROR: -32603), then use it consistently:- if (!transportService) { - throw new Error('Transport service not available'); - } + if (!transportService) { + throw new TransportServiceNotAvailableError(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/transport/flows/handle.streamable-http.flow.ts` around lines 304 - 307, Replace the generic throw new Error('Transport service not available') guard with a specific error class: create TransportServiceNotAvailableError (extend the project MCP error base) that encodes the MCP error code (e.g., INTERNAL_ERROR -32603) and use it in all stage methods (onInitialize, onElicitResult, onMessage, onSseListener, onExtApps) where the transportService guard appears; update each guard to throw new TransportServiceNotAvailableError(...) with a clear contextual message so callers can distinguish this failure type from other errors.libs/sdk/src/common/entries/app.entry.ts (1)
10-10: ConstrainMetadatahere unlessBaseEntryalready does it.This declaration no longer shows an
AppMetadatabound, soAppEntryloses the local guarantee that subclasses carry app-shaped metadata.Metadata extends AppMetadata = AppMetadatakeeps the newunknownmiddle generic without weakening the metadata contract.As per coding guidelines, "Use type parameter constraints with unknown defaults for generic classes. Create constrained generic types like `ResourceContext = Record, Out = unknown>` instead of unconstrained generics with `any`."♻️ Suggested type constraint
-export abstract class AppEntry<Metadata = AppMetadata> extends BaseEntry<AppRecord, unknown, Metadata> { +export abstract class AppEntry<Metadata extends AppMetadata = AppMetadata> + extends BaseEntry<AppRecord, unknown, Metadata> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/common/entries/app.entry.ts` at line 10, AppEntry currently declares a generic Metadata without constraining it, which weakens the expectation that subclasses carry app-shaped metadata; update the class generic on AppEntry to constrain Metadata to AppMetadata (e.g., Metadata extends AppMetadata = AppMetadata) so the middle generic of BaseEntry can remain unknown without losing the metadata contract—locate the export abstract class AppEntry declaration and adjust its generic parameter accordingly (referencing AppEntry, BaseEntry, and AppMetadata).
🤖 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/agent/agent.scope.ts`:
- Around line 368-390: Add an explicit implements declaration to the
AgentScopeEntry class so it declares that it conforms to the ScopeEntry
interface; update the class signature for AgentScopeEntry to include "implements
ScopeEntry" (the existing getters transportService, rateLimitManager,
elicitationStore, metadata, record, and ready already satisfy the contract),
ensuring no change to their return values (undefined for the absent services)
and leaving metadata and ready as-is.
In `@libs/sdk/src/app/instances/app.remote.instance.ts`:
- Around line 296-305: The getters tools, resources, and prompts return the
backing registries directly and never trigger lazy discovery, so callers see
empty sets unless they explicitly call ensureCapabilitiesLoaded(); update the
accessors (get tools, get resources, get prompts) to invoke or await
ensureCapabilitiesLoaded() before returning _tools, _resources, and _prompts
(respecting the existing ready/initialization flow so you don't double-load),
ensuring the first access to each registry triggers capability discovery via
ensureCapabilitiesLoaded() and returns the populated registries.
In `@libs/sdk/src/common/entries/scope.entry.ts`:
- Line 21: The ScopeEntry file is importing AuthRegistry (and wiring
authProviders) from an internal SDK auth path which leaks auth implementation
into common; remove the direct import of AuthRegistry and any direct
construction/wiring of authProviders in ScopeEntry (and related lines around the
authProviders usage) and instead obtain authentication dependencies via the
public auth boundary (inject an auth provider/registry interface into ScopeEntry
or accept it via constructor/parameters), or import them from the public package
symbol exported by `@frontmcp/auth`; update ScopeEntry to depend only on the
abstract auth interface passed in rather than ../../auth.auth.registry so all
auth implementation stays in libs/auth.
In `@libs/sdk/src/common/interfaces/provider.interface.ts`:
- Around line 12-16: The generic default type for the ProviderType alias is
unsafe: change the generic parameter default from Provide = any to Provide =
unknown so ProviderType (and its variants ProviderClassType, ProviderValueType,
ProviderFactoryType) no longer opt into unsafe any when consumers omit the
Provide type; update the ProviderType declaration to use Provide = unknown and
run type checks to ensure no call sites rely on implicit any.
In `@libs/sdk/src/elicitation/flows/elicitation-request.flow.ts`:
- Around line 166-169: Replace the generic throw in the block that checks
this.scope.elicitationStore with a framework-specific MCP error: create or reuse
a typed SDK error class (e.g., ElicitationStoreNotInitializedError) that
includes the appropriate MCP error code and implements toJsonRpcError(), and
throw an instance of that class from the check around
this.scope.elicitationStore so error mapping remains consistent across flows.
In `@libs/sdk/src/elicitation/helpers/fallback.helper.ts`:
- Around line 150-154: The early-return rejection when scope.elicitationStore is
missing must clear the pending timeout and use a typed error: before calling
reject(...) clearTimeout(timeoutHandle) so the timeout callback won't fire
later, and replace the generic Error with a domain-specific error (e.g.,
MissingElicitationStoreError) or an exported typed error class; update the call
site in the block that checks scope.elicitationStore (the code referencing
scope.elicitationStore and reject) to clear the timer and reject with the new
specific error type, and add/import the error type definition if needed.
In `@libs/sdk/src/elicitation/send-elicitation-result.tool.ts`:
- Line 129: The cast is used because ScopeEntry lacks the runFlowForOutput
method; add an abstract declaration runFlowForOutput(name: string, params:
Record<string, any>): Promise<any> (matching the existing concrete signature) to
the ScopeEntry abstract class, implement that method in the concrete Scope class
(delegate to the existing implementation), then remove the cast in
send-elicitation-result.tool.ts so you can call this.scope.runFlowForOutput(...)
directly; ensure the method signature matches usages (e.g., the
'tools:call-tool' call) and update any related type imports if needed.
In `@libs/sdk/src/scope/flows/http.request.flow.ts`:
- Around line 528-529: The current early return when transportService is falsy
can drop DELETE responses; modify the branch around transportService (in the
function handling the flow) so that instead of unconditional "if
(!transportService) return" you detect when authorization?.token exists and in
that case call httpRespond.noContent() (or the appropriate completion for
DELETE) before returning; keep the original early return only for cases without
authorization.token. Ensure you reference transportService, authorization.token
and httpRespond.noContent() when making the change so DELETE requests still
receive a response.
In `@libs/sdk/src/transport/flows/handle.stateless-http.flow.ts`:
- Around line 98-101: Replace the generic throw new Error('Transport service not
available') when this.scope.transportService is falsy with a typed MCP SDK error
(e.g., TransportServiceNotAvailableError) that extends your base MCP error
class, includes the appropriate MCP error code, and implements toJsonRpcError();
create the new error class if missing and use it in the check around
transportService (referencing transportService and this.scope.transportService)
so error translation remains consistent across the SDK.
---
Outside diff comments:
In `@libs/sdk/src/transport/flows/handle.sse.flow.ts`:
- Line 17: Remove the unused import of Scope from the top-level imports in
handle.sse.flow.ts: locate the import statement "import { Scope } from
'../../scope';" and delete it so there is no dead/unused import remaining;
ensure remaining imports still satisfy lint rules and build.
---
Nitpick comments:
In `@libs/sdk/src/agent/flows/call-agent.flow.ts`:
- Line 16: Remove the unused import Scope from the top of the file: delete the
import token "Scope" from the import statement that currently reads "import {
Scope } from '../../scope';" so the file no longer imports Scope (ensure any
remaining imports on that line are preserved if shared).
In `@libs/sdk/src/common/entries/app.entry.ts`:
- Line 10: AppEntry currently declares a generic Metadata without constraining
it, which weakens the expectation that subclasses carry app-shaped metadata;
update the class generic on AppEntry to constrain Metadata to AppMetadata (e.g.,
Metadata extends AppMetadata = AppMetadata) so the middle generic of BaseEntry
can remain unknown without losing the metadata contract—locate the export
abstract class AppEntry declaration and adjust its generic parameter accordingly
(referencing AppEntry, BaseEntry, and AppMetadata).
In `@libs/sdk/src/common/interfaces/app.interface.ts`:
- Around line 6-8: Replace the generic default from `any` to `unknown` in the
`AppType` alias (i.e., change `export type AppType<T = any>` to `export type
AppType<T = unknown>`), remove the `// eslint-disable-next-line
`@typescript-eslint/no-explicit-any`` line, and update the preceding comment to
reflect the use of `unknown` for broader safety while preserving compatibility
via explicit narrowing; ensure references to `Type<T>`, `AppValueType<T>`, and
`RemoteAppMetadata` remain unchanged.
In `@libs/sdk/src/elicitation/flows/elicitation-result.flow.ts`:
- Line 14: The import "Scope" is no longer used after switching to
ScopeEntry-based access; remove the unused type import (the `Scope` identifier)
from the import statement so only needed symbols (e.g., ScopeEntry) remain and
eliminate the dead import in elicitation-result.flow.ts where `import type {
Scope } from '../../scope';` currently appears.
In `@libs/sdk/src/transport/flows/handle.streamable-http.flow.ts`:
- Around line 304-307: Replace the generic throw new Error('Transport service
not available') guard with a specific error class: create
TransportServiceNotAvailableError (extend the project MCP error base) that
encodes the MCP error code (e.g., INTERNAL_ERROR -32603) and use it in all stage
methods (onInitialize, onElicitResult, onMessage, onSseListener, onExtApps)
where the transportService guard appears; update each guard to throw new
TransportServiceNotAvailableError(...) with a clear contextual message so
callers can distinguish this failure type from other errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6d70459d-f02a-43b3-bc80-421729bd2474
📒 Files selected for processing (61)
.gitignorelibs/sdk/src/__test-utils__/fixtures/plugin.fixtures.tslibs/sdk/src/__test-utils__/fixtures/provider.fixtures.tslibs/sdk/src/agent/agent.instance.tslibs/sdk/src/agent/agent.registry.tslibs/sdk/src/agent/agent.scope.tslibs/sdk/src/agent/flows/call-agent.flow.tslibs/sdk/src/app/instances/app.esm.instance.tslibs/sdk/src/app/instances/app.local.instance.tslibs/sdk/src/app/instances/app.remote.instance.tslibs/sdk/src/auth/auth.registry.tslibs/sdk/src/auth/instances/instance.remote-primary-auth.tslibs/sdk/src/common/entries/app.entry.tslibs/sdk/src/common/entries/plugin.entry.tslibs/sdk/src/common/entries/provider.entry.tslibs/sdk/src/common/entries/scope.entry.tslibs/sdk/src/common/interfaces/app.interface.tslibs/sdk/src/common/interfaces/internal/registry.interface.tslibs/sdk/src/common/interfaces/plugin.interface.tslibs/sdk/src/common/interfaces/provider.interface.tslibs/sdk/src/common/interfaces/scope.interface.tslibs/sdk/src/elicitation/flows/elicitation-request.flow.tslibs/sdk/src/elicitation/flows/elicitation-result.flow.tslibs/sdk/src/elicitation/helpers/fallback.helper.tslibs/sdk/src/elicitation/send-elicitation-result.tool.tslibs/sdk/src/flows/flow.instance.tslibs/sdk/src/hooks/hook.registry.tslibs/sdk/src/job/job.instance.tslibs/sdk/src/plugin/__tests__/plugin.registry.spec.tslibs/sdk/src/plugin/__tests__/plugin.utils.spec.tslibs/sdk/src/plugin/plugin.registry.tslibs/sdk/src/prompt/prompt.instance.tslibs/sdk/src/prompt/prompt.registry.tslibs/sdk/src/provider/provider.registry.tslibs/sdk/src/resource/flows/read-resource.flow.tslibs/sdk/src/resource/resource.instance.tslibs/sdk/src/resource/resource.registry.tslibs/sdk/src/scope/flows/http.request.flow.tslibs/sdk/src/scope/scope.instance.tslibs/sdk/src/skill/__tests__/memory-skill.provider.spec.tslibs/sdk/src/skill/__tests__/skill-http.utils.spec.tslibs/sdk/src/skill/__tests__/skill-validator.spec.tslibs/sdk/src/skill/flows/http/skills-api.flow.tslibs/sdk/src/skill/flows/load-skill.flow.tslibs/sdk/src/skill/skill-http.utils.tslibs/sdk/src/skill/skill-storage.factory.tslibs/sdk/src/skill/skill-validator.tslibs/sdk/src/skill/skill.instance.tslibs/sdk/src/skill/skill.registry.tslibs/sdk/src/skill/tools/load-skills.tool.tslibs/sdk/src/tool/flows/call-tool.flow.tslibs/sdk/src/tool/flows/tools-list.flow.tslibs/sdk/src/tool/tool.instance.tslibs/sdk/src/tool/tool.registry.tslibs/sdk/src/transport/adapters/transport.local.adapter.tslibs/sdk/src/transport/adapters/transport.sse.adapter.tslibs/sdk/src/transport/adapters/transport.streamable-http.adapter.tslibs/sdk/src/transport/flows/handle.sse.flow.tslibs/sdk/src/transport/flows/handle.stateless-http.flow.tslibs/sdk/src/transport/flows/handle.streamable-http.flow.tslibs/sdk/src/workflow/workflow.instance.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
libs/sdk/src/elicitation/flows/elicitation-result.flow.ts (1)
196-202: Asymmetric handling of missing store between stages.In
lookupPending, a missing store throws an error (line 114), but here inpublishResult, a missing store is silently treated as "not handled" (line 198-201). This inconsistency may cause confusion:
- If the store is missing in
lookupPending, the flow fails with an exception.- If the store becomes unavailable between stages (unlikely but possible),
publishResultsilently fails.Consider whether both stages should have the same behavior. If graceful degradation is intended,
lookupPendingshould also handle missing store gracefully. If store availability is a prerequisite,publishResultshould throw.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/elicitation/flows/elicitation-result.flow.ts` around lines 196 - 202, The handling of a missing store is inconsistent between lookupPending (which throws) and publishResult (which silently sets handled=false); choose and implement a consistent policy: either make lookupPending degrade gracefully like publishResult by checking this.scope.elicitationStore and setting this.state.set('handled', false) and logging via this.logger.verbose in lookupPending, or make publishResult enforce the prerequisite by throwing an Error when this.scope.elicitationStore is missing (e.g., mirror lookupPending’s behavior), updating publishResult’s early-return to raise the same exception and remove the silent handled=false path; update whichever function you change (lookupPending or publishResult) so both use the same error/logging pattern and reference this.scope.elicitationStore, this.state.set('handled', false), and this.logger.verbose consistently.
🤖 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/elicitation/flows/elicitation-result.flow.ts`:
- Around line 112-115: Replace the plain Error throw in the lookupPending stage
with an ElicitationStoreNotInitializedError (same subclass used by
storePendingRecord) so missing this.scope.elicitationStore consistently raises
the InternalMcpError subclass; also make the missing-store behavior in
publishResult symmetric (either throw the same
ElicitationStoreNotInitializedError instead of returning handled:false or
explicitly document why it differs) by updating the publishResult path that
checks this.scope.elicitationStore to throw ElicitationStoreNotInitializedError
to match lookupPending/storePendingRecord.
---
Nitpick comments:
In `@libs/sdk/src/elicitation/flows/elicitation-result.flow.ts`:
- Around line 196-202: The handling of a missing store is inconsistent between
lookupPending (which throws) and publishResult (which silently sets
handled=false); choose and implement a consistent policy: either make
lookupPending degrade gracefully like publishResult by checking
this.scope.elicitationStore and setting this.state.set('handled', false) and
logging via this.logger.verbose in lookupPending, or make publishResult enforce
the prerequisite by throwing an Error when this.scope.elicitationStore is
missing (e.g., mirror lookupPending’s behavior), updating publishResult’s
early-return to raise the same exception and remove the silent handled=false
path; update whichever function you change (lookupPending or publishResult) so
both use the same error/logging pattern and reference
this.scope.elicitationStore, this.state.set('handled', false), and
this.logger.verbose consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b076a11f-9e75-4c51-8768-c8f1091b6fbe
📒 Files selected for processing (19)
libs/sdk/src/__test-utils__/fixtures/plugin.fixtures.tslibs/sdk/src/agent/agent.scope.tslibs/sdk/src/agent/flows/call-agent.flow.tslibs/sdk/src/common/entries/scope.entry.tslibs/sdk/src/common/interfaces/app.interface.tslibs/sdk/src/common/interfaces/provider.interface.tslibs/sdk/src/elicitation/flows/elicitation-request.flow.tslibs/sdk/src/elicitation/flows/elicitation-result.flow.tslibs/sdk/src/elicitation/helpers/fallback.helper.tslibs/sdk/src/elicitation/send-elicitation-result.tool.tslibs/sdk/src/errors/index.tslibs/sdk/src/errors/transport.errors.tslibs/sdk/src/plugin/__tests__/plugin.registry.spec.tslibs/sdk/src/provider/provider.registry.tslibs/sdk/src/resource/flows/read-resource.flow.tslibs/sdk/src/scope/flows/http.request.flow.tslibs/sdk/src/transport/flows/handle.sse.flow.tslibs/sdk/src/transport/flows/handle.stateless-http.flow.tslibs/sdk/src/transport/flows/handle.streamable-http.flow.ts
✅ Files skipped from review due to trivial changes (3)
- libs/sdk/src/scope/flows/http.request.flow.ts
- libs/sdk/src/test-utils/fixtures/plugin.fixtures.ts
- libs/sdk/src/elicitation/send-elicitation-result.tool.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- libs/sdk/src/transport/flows/handle.stateless-http.flow.ts
- libs/sdk/src/elicitation/flows/elicitation-request.flow.ts
- libs/sdk/src/elicitation/helpers/fallback.helper.ts
- libs/sdk/src/common/interfaces/app.interface.ts
- libs/sdk/src/transport/flows/handle.sse.flow.ts
- libs/sdk/src/common/interfaces/provider.interface.ts
- libs/sdk/src/provider/provider.registry.ts
- libs/sdk/src/agent/agent.scope.ts
- libs/sdk/src/agent/flows/call-agent.flow.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
libs/sdk/src/elicitation/flows/elicitation-result.flow.ts (1)
198-200: Update skip log message to match the new guard condition.Line 200 says “no pending or no result”, but Line 198 also skips on missing store. Consider making the log explicit for easier debugging.
🔧 Proposed log text update
- this.logger.verbose('publishResult:skip (no pending or no result)'); + this.logger.verbose('publishResult:skip (no pending, no result, or no store)');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/elicitation/flows/elicitation-result.flow.ts` around lines 198 - 200, The guard in publishResult checks three variables (pendingRecord, elicitResult, store) but the verbose log only mentions missing pending/result; update the message emitted by this.logger.verbose in the branch that sets this.state.set('handled', false) to explicitly mention which of pendingRecord, elicitResult or store is missing (or a concise combined message like "publishResult:skip (missing pendingRecord, elicitResult or store)") so logs accurately reflect the guard; locate the check using symbols pendingRecord, elicitResult, store, this.state.set('handled', false) and this.logger.verbose to make the fix.
🤖 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/sdk/src/elicitation/flows/elicitation-result.flow.ts`:
- Around line 198-200: The guard in publishResult checks three variables
(pendingRecord, elicitResult, store) but the verbose log only mentions missing
pending/result; update the message emitted by this.logger.verbose in the branch
that sets this.state.set('handled', false) to explicitly mention which of
pendingRecord, elicitResult or store is missing (or a concise combined message
like "publishResult:skip (missing pendingRecord, elicitResult or store)") so
logs accurately reflect the guard; locate the check using symbols pendingRecord,
elicitResult, store, this.state.set('handled', false) and this.logger.verbose to
make the fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1988daf0-f257-4e38-91d1-22ece66c4bde
📒 Files selected for processing (1)
libs/sdk/src/elicitation/flows/elicitation-result.flow.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
Bug Fixes
Refactor