feat: Update output mode logic for Claude client and enhance documentation #123
Conversation
WalkthroughAdds a Verdaccio "publish alpha" workflow, converts push CI E2E runs to a matrix, tightens ESLint rules and removes many eslint-disable comments, updates several package manifests, refactors SDK flow orchestration into PRE/EXECUTE/POST/FINALIZE phases, and adds a UI isReactAvailable() helper. (50 words) Changes
sequenceDiagram sequenceDiagram
participant Runner as FlowRunner
participant Flow as FlowClass
participant Registry as HookRegistry
participant Hook as HookInstance
Runner->>Flow: runWithContext(request, ctx)
Flow->>Registry: materialize hooks for PRE/EXECUTE/POST/FINALIZE
Registry-->>Flow: hooks per stage
alt PRE phase
Flow->>Hook: run "will" hooks
Hook-->>Flow: outcome (ok/respond/fail/abort)
end
alt EXECUTE phase (if not terminal)
Flow->>Flow: invoke execute handler
Flow->>Hook: run execute-stage hooks
Hook-->>Flow: outcome
end
alt POST phase
Flow->>Hook: run "did"/around hooks
Hook-->>Flow: outcome (may alter control)
end
Flow->>Flow: FINALIZE (cleanup, propagate final outcome)
Flow-->>Runner: return final outcome or throw FlowControl
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
libs/sdk/src/auth/session/record/session.stateless.ts (1)
23-29: Use specific MCP error classes instead of generic errors.SDK methods should throw specific MCP error classes (such as
InternalMcpErrororGenericServerError) with proper error codes instead of genericErrorinstances. This helps callers distinguish error types and improves debuggability.For the stateless session, which intentionally does not support token refresh, use an appropriate MCP error class:
+ constructor(ctx: StatefulCreateCtx) { + super(ctx as any); + throw new GenericServerError('StatelessSession not yet implemented'); + } + override getToken(providerId?: string): Promise<string> | string { + throw new GenericServerError('Token refresh not supported in stateless mode'); + }libs/sdk/package.json (1)
50-56: Remove stale peerDependenciesMeta entries.The
peerDependenciesMetasection referencesreactandreact-dom, but these dependencies are no longer present in thepeerDependenciesobject. This configuration is incorrect and will cause confusion.Apply this diff to remove the stale entries:
}, - "peerDependenciesMeta": { - "react": { - "optional": true - }, - "react-dom": { - "optional": true - } - }, "dependencies": {
🧹 Nitpick comments (8)
libs/ui/src/types/ui-runtime.ts (1)
1236-1239: Preferunknownoveranyfor generic type defaults.The ComponentType declaration uses
anyfor both the default props type and return type. Per coding guidelines, generic type defaults should useunknowninstead ofanyfor better type safety.Apply this diff to improve type safety:
// eslint-disable-next-line @typescript-eslint/no-namespace declare namespace React { - // eslint-disable-next-line @typescript-eslint/no-explicit-any - type ComponentType<P = any> = (props: P) => any; + type ComponentType<P = unknown> = (props: P) => unknown; }As per coding guidelines: "Enable strict TypeScript mode with no
anytypes without strong justification - useunknowninstead for generic type defaults."libs/sdk/src/transport/adapters/transport.streamable-http.adapter.ts (1)
69-82: Consider extracting the logging logic to eliminate thethisalias.The ESLint disable is documenting a necessary suppression for a common pattern. However, you could refactor to avoid the alias entirely by extracting the logging into a separate method.
Example refactor:
private logInitializeResponse(statusCode: number, headers: Record<string, unknown>, bodyPreview: string): void { this.logger.info('[StreamableHttpAdapter] initialize response', { statusCode, headers, bodyPreview, }); }Then replace lines 76-80:
- adapter.logger.info('[StreamableHttpAdapter] initialize response', { - statusCode: res.statusCode, - headers: res.getHeaders?.() ?? {}, - bodyPreview: responseBody.slice(0, 1000), - }); + adapter.logInitializeResponse( + res.statusCode, + res.getHeaders?.() ?? {}, + responseBody.slice(0, 1000) + );This would eliminate the need for the ESLint disable while maintaining the same functionality.
libs/ui/src/bridge/runtime/iife-generator.ts (1)
937-945: Consider using a dedicated minifier for production.While the current simple minification approach works correctly, you might want to consider using a proper minifier like
terseroresbuildfor better compression and more sophisticated optimizations. However, if keeping this lightweight and dependency-free is intentional, the current approach is adequate.libs/sdk/src/flows/flow.instance.ts (1)
410-444: Refactor opportunity: redundant throw statements.The pattern on lines 421-422 (and similar at 437-438) has redundant throws:
if (post.outcome === 'fail') throw post.control!; throw post.control!;Both branches throw
post.control!. Theifcheck appears to be a remnant of differentiated handling that no longer applies.Consider simplifying to a single throw:
if (post.outcome === 'unknown_error' || post.outcome === 'fail') { try { await runErrorStage(); } finally { await runFinalizeStage(); } - if (post.outcome === 'fail') throw post.control!; throw post.control!; }The same pattern applies to lines 437-438, 457-458, and 474-475.
libs/ui/src/runtime/adapters/react.adapter.ts (1)
17-23: Consider more specific typing for Component property.The newly added
Component?property usesanytype. Per coding guidelines,anyshould only be used with strong justification, andunknownis preferred for generic type defaults.Consider if this could be typed more specifically:
interface ReactRuntime { // eslint-disable-next-line @typescript-eslint/no-explicit-any createElement: (...args: any[]) => any; - // eslint-disable-next-line @typescript-eslint/no-explicit-any - Component?: any; + Component?: typeof React.Component | unknown; }Alternatively, if React.Component is only used for
instanceofchecks or similar,unknownmay suffice. Please verify the actual usage requirements across supported React versions.As per coding guidelines: **/*.ts - Use
unknowninstead for generic type defaults without strong justification forany.eslint.config.mjs (1)
46-55: Consider tightening test file type safety.The new test-specific configuration disables
@typescript-eslint/no-explicit-anyentirely for test files. This contradicts the coding guideline: "Enable strict TypeScript mode with noanytypes without strong justification - useunknowninstead."While test mocks often need flexibility, completely disabling
anychecks may reduce test quality and type safety. Consider keeping it as 'warn' instead of 'off' to encourage proper typing in tests while allowing exceptions where truly needed.{ // Relax rules for test files files: ['**/*.test.ts', '**/*.spec.ts', '**/__tests__/**/*.ts', '**/__test-utils__/**/*.ts', '**/fixtures/**/*.ts', '**/mocks/**/*.ts'], rules: { '@typescript-eslint/no-empty-function': 'off', - '@typescript-eslint/no-explicit-any': 'off', + '@typescript-eslint/no-explicit-any': 'warn', '@typescript-eslint/no-unused-vars': 'warn', 'no-unused-private-class-members': 'off', }, },As per coding guidelines: **/*.ts - Enable strict TypeScript mode with no
anytypes without strong justification..github/workflows/publish-alpha.yml (2)
32-39: Consider adding error handling for the Nx output parsing.The command chain that resolves synchronized libraries is complex and could fail silently if Nx changes its output format or if no projects match the tag filter.
Consider adding validation to ensure projects are found:
- name: Resolve synchronized libs id: libs shell: bash run: | set -euo pipefail PROJECTS=$(npx nx show projects -p tag:versioning:synchronized --type lib --json | node -e "console.log(JSON.parse(require('fs').readFileSync(0,'utf8')).join(','))") + if [ -z "$PROJECTS" ]; then + echo "ERROR: No synchronized library projects found" + exit 1 + fi echo "projects=$PROJECTS" >> "$GITHUB_OUTPUT" echo "Found libs: $PROJECTS"
63-72: Add validation for registry URL format.The shell parameter expansion that strips the protocol from the registry URL could produce unexpected results if the URL format is non-standard or empty.
Consider adding validation:
- name: Configure npm for Verdaccio shell: bash run: | set -euo pipefail + if [ -z "${VERDACCIO_REGISTRY_URL}" ] || [ -z "${VERDACCIO_TOKEN}" ]; then + echo "ERROR: VERDACCIO_REGISTRY_URL and VERDACCIO_TOKEN must be set" + exit 1 + fi REGISTRY_HOST="${VERDACCIO_REGISTRY_URL#https://}" REGISTRY_HOST="${REGISTRY_HOST#http://}" + if [ -z "$REGISTRY_HOST" ]; then + echo "ERROR: Invalid registry URL format" + exit 1 + fi echo "//${REGISTRY_HOST}/:_authToken=${VERDACCIO_TOKEN}" >> ~/.npmrc env: VERDACCIO_REGISTRY_URL: ${{ secrets.VERDACCIO_REGISTRY_URL }} VERDACCIO_TOKEN: ${{ secrets.VERDACCIO_TOKEN }}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
libs/ui/src/build/__tests__/manifest-builder.test.tsis excluded by!**/build/**libs/ui/src/build/widget-manifest.tsis excluded by!**/build/**
📒 Files selected for processing (44)
.github/workflows/publish-alpha.yml(1 hunks).github/workflows/push.yml(2 hunks)apps/auth/demo-orchestrated-auth/jest.e2e.config.ts(0 hunks)apps/auth/demo-public/jest.e2e.config.ts(0 hunks)apps/auth/demo-transparent-auth/jest.e2e.config.ts(0 hunks)apps/demo/jest.config.ts(0 hunks)apps/ui/mdx-demo/jest.config.ts(0 hunks)apps/ui/react-demo/jest.config.ts(0 hunks)eslint.config.mjs(2 hunks)libs/adapters/src/openapi/openapi.utils.ts(1 hunks)libs/plugins/jest.config.ts(0 hunks)libs/plugins/package.json(1 hunks)libs/plugins/src/codecall/__tests__/codecall-config.provider.test.ts(1 hunks)libs/plugins/src/codecall/__tests__/mcp-result.test.ts(0 hunks)libs/plugins/src/codecall/__tests__/search.tool.test.ts(1 hunks)libs/sdk/jest.config.ts(0 hunks)libs/sdk/package.json(2 hunks)libs/sdk/src/__test-utils__/fixtures/hook.fixtures.ts(3 hunks)libs/sdk/src/__test-utils__/fixtures/oauth.fixtures.ts(1 hunks)libs/sdk/src/__test-utils__/mocks/hook-registry.mock.ts(1 hunks)libs/sdk/src/auth/session/record/session.stateless.ts(1 hunks)libs/sdk/src/common/decorators/decorator-utils.ts(1 hunks)libs/sdk/src/flows/flow.instance.ts(4 hunks)libs/sdk/src/logger/instances/instance.logger.ts(1 hunks)libs/sdk/src/provider/provider.registry.ts(0 hunks)libs/sdk/src/transport/adapters/transport.streamable-http.adapter.ts(1 hunks)libs/testing/jest.config.ts(0 hunks)libs/testing/src/matchers/matcher-types.ts(1 hunks)libs/ui/jest.config.ts(0 hunks)libs/ui/package.json(0 hunks)libs/ui/src/bridge/__tests__/adapter-registry.test.ts(2 hunks)libs/ui/src/bridge/__tests__/bridge-factory.test.ts(1 hunks)libs/ui/src/bridge/runtime/iife-generator.ts(1 hunks)libs/ui/src/bundler/sandbox/enclave-adapter.ts(1 hunks)libs/ui/src/react/utils.ts(0 hunks)libs/ui/src/render/prerender.ts(2 hunks)libs/ui/src/runtime/adapters/mdx.adapter.ts(0 hunks)libs/ui/src/runtime/adapters/react.adapter.ts(2 hunks)libs/ui/src/types/ui-runtime.ts(1 hunks)libs/ui/src/web-components/core/base-element.test.ts(2 hunks)libs/ui/src/web-components/core/base-element.ts(1 hunks)libs/ui/src/web-components/elements/elements.test.ts(2 hunks)libs/ui/src/web-components/types.ts(1 hunks)libs/ui/src/widgets/progress.ts(2 hunks)
💤 Files with no reviewable changes (15)
- libs/plugins/src/codecall/tests/mcp-result.test.ts
- apps/auth/demo-transparent-auth/jest.e2e.config.ts
- libs/ui/package.json
- apps/demo/jest.config.ts
- libs/plugins/jest.config.ts
- libs/sdk/src/provider/provider.registry.ts
- apps/ui/react-demo/jest.config.ts
- apps/auth/demo-orchestrated-auth/jest.e2e.config.ts
- libs/testing/jest.config.ts
- libs/ui/jest.config.ts
- libs/sdk/jest.config.ts
- libs/ui/src/react/utils.ts
- apps/ui/mdx-demo/jest.config.ts
- libs/ui/src/runtime/adapters/mdx.adapter.ts
- apps/auth/demo-public/jest.e2e.config.ts
🧰 Additional context used
📓 Path-based instructions (6)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Enable strict TypeScript mode with noanytypes without strong justification - useunknowninstead for generic type defaults
Avoid non-null assertions (!) - use proper error handling and throw specific errors instead
Use specific error classes with MCP error codes instead of generic errors
Use type parameters with constraints instead of unconstrained generics, and preferunknownoveranyfor generic type defaults
Follow the preset pattern for hierarchical configurations across the codebase
Files:
libs/sdk/src/__test-utils__/fixtures/oauth.fixtures.tslibs/ui/src/bundler/sandbox/enclave-adapter.tslibs/ui/src/bridge/__tests__/adapter-registry.test.tslibs/ui/src/widgets/progress.tslibs/sdk/src/__test-utils__/mocks/hook-registry.mock.tslibs/ui/src/web-components/core/base-element.test.tslibs/sdk/src/common/decorators/decorator-utils.tslibs/sdk/src/__test-utils__/fixtures/hook.fixtures.tslibs/sdk/src/transport/adapters/transport.streamable-http.adapter.tslibs/adapters/src/openapi/openapi.utils.tslibs/ui/src/render/prerender.tslibs/ui/src/web-components/core/base-element.tslibs/ui/src/web-components/elements/elements.test.tslibs/plugins/src/codecall/__tests__/codecall-config.provider.test.tslibs/testing/src/matchers/matcher-types.tslibs/ui/src/bridge/__tests__/bridge-factory.test.tslibs/sdk/src/logger/instances/instance.logger.tslibs/plugins/src/codecall/__tests__/search.tool.test.tslibs/sdk/src/auth/session/record/session.stateless.tslibs/ui/src/runtime/adapters/react.adapter.tslibs/ui/src/types/ui-runtime.tslibs/ui/src/bridge/runtime/iife-generator.tslibs/sdk/src/flows/flow.instance.tslibs/ui/src/web-components/types.ts
libs/{sdk,adapters,plugins,cli}/src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
libs/{sdk,adapters,plugins,cli}/src/**/*.ts: Return strictly typed MCP protocol responses (GetPromptResult, ReadResourceResult, etc.) instead ofunknownforexecute()andread()methods
Validate URIs per RFC 3986 at metadata level using Zod validation with custom refinements
UsegetCapabilities()for dynamic capability exposure instead of hardcoding capabilities in adapters
UsechangeScopeinstead ofscopefor change event properties to avoid confusion with the Scope class
Validate hooks match their entry type and fail fast with InvalidHookFlowError for unsupported flows
Don't mutate rawInput in flows - use state.set() for managing flow state instead
Files:
libs/sdk/src/__test-utils__/fixtures/oauth.fixtures.tslibs/sdk/src/__test-utils__/mocks/hook-registry.mock.tslibs/sdk/src/common/decorators/decorator-utils.tslibs/sdk/src/__test-utils__/fixtures/hook.fixtures.tslibs/sdk/src/transport/adapters/transport.streamable-http.adapter.tslibs/adapters/src/openapi/openapi.utils.tslibs/plugins/src/codecall/__tests__/codecall-config.provider.test.tslibs/sdk/src/logger/instances/instance.logger.tslibs/plugins/src/codecall/__tests__/search.tool.test.tslibs/sdk/src/auth/session/record/session.stateless.tslibs/sdk/src/flows/flow.instance.ts
libs/**
⚙️ CodeRabbit configuration file
libs/**: Contains publishable SDK libraries. Review for API correctness, breaking changes, and consistency with docs. When public APIs change, ensure there is a matching docs/draft/docs/** update (not direct edits under docs/docs/**).
Files:
libs/sdk/src/__test-utils__/fixtures/oauth.fixtures.tslibs/ui/src/bundler/sandbox/enclave-adapter.tslibs/ui/src/bridge/__tests__/adapter-registry.test.tslibs/ui/src/widgets/progress.tslibs/sdk/src/__test-utils__/mocks/hook-registry.mock.tslibs/ui/src/web-components/core/base-element.test.tslibs/sdk/src/common/decorators/decorator-utils.tslibs/sdk/src/__test-utils__/fixtures/hook.fixtures.tslibs/plugins/package.jsonlibs/sdk/src/transport/adapters/transport.streamable-http.adapter.tslibs/adapters/src/openapi/openapi.utils.tslibs/ui/src/render/prerender.tslibs/ui/src/web-components/core/base-element.tslibs/ui/src/web-components/elements/elements.test.tslibs/plugins/src/codecall/__tests__/codecall-config.provider.test.tslibs/testing/src/matchers/matcher-types.tslibs/ui/src/bridge/__tests__/bridge-factory.test.tslibs/sdk/src/logger/instances/instance.logger.tslibs/sdk/package.jsonlibs/plugins/src/codecall/__tests__/search.tool.test.tslibs/sdk/src/auth/session/record/session.stateless.tslibs/ui/src/runtime/adapters/react.adapter.tslibs/ui/src/types/ui-runtime.tslibs/ui/src/bridge/runtime/iife-generator.tslibs/sdk/src/flows/flow.instance.tslibs/ui/src/web-components/types.ts
libs/ui/src/**/*.ts
📄 CodeRabbit inference engine (libs/ui/CLAUDE.md)
libs/ui/src/**/*.ts: Always useescapeHtml()utility for all user-provided content to prevent XSS vulnerabilities
Hard-code CDN URLs only in theme presets; always reference theme.cdn configuration in component code
Files:
libs/ui/src/bundler/sandbox/enclave-adapter.tslibs/ui/src/bridge/__tests__/adapter-registry.test.tslibs/ui/src/widgets/progress.tslibs/ui/src/web-components/core/base-element.test.tslibs/ui/src/render/prerender.tslibs/ui/src/web-components/core/base-element.tslibs/ui/src/web-components/elements/elements.test.tslibs/ui/src/bridge/__tests__/bridge-factory.test.tslibs/ui/src/runtime/adapters/react.adapter.tslibs/ui/src/types/ui-runtime.tslibs/ui/src/bridge/runtime/iife-generator.tslibs/ui/src/web-components/types.ts
libs/ui/src/**/*.test.ts
📄 CodeRabbit inference engine (libs/ui/CLAUDE.md)
libs/ui/src/**/*.test.ts: Maintain 95%+ test coverage across statements, branches, functions, and lines
Test HTML escaping for user-provided content to prevent XSS attacks
Test behavior across platform configurations (OpenAI, Claude, Gemini, ngrok) where applicable
Files:
libs/ui/src/bridge/__tests__/adapter-registry.test.tslibs/ui/src/web-components/core/base-element.test.tslibs/ui/src/web-components/elements/elements.test.tslibs/ui/src/bridge/__tests__/bridge-factory.test.ts
**/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.ts: Achieve and maintain 95%+ test coverage across all metrics (statements, branches, functions, lines)
Test all code paths including errors, constructor validation, and error classinstanceofchecks
Files:
libs/ui/src/bridge/__tests__/adapter-registry.test.tslibs/ui/src/web-components/core/base-element.test.tslibs/ui/src/web-components/elements/elements.test.tslibs/plugins/src/codecall/__tests__/codecall-config.provider.test.tslibs/ui/src/bridge/__tests__/bridge-factory.test.tslibs/plugins/src/codecall/__tests__/search.tool.test.ts
🧠 Learnings (19)
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/**/*.test.ts : Test behavior across platform configurations (OpenAI, Claude, Gemini, ngrok) where applicable
Applied to files:
libs/ui/src/bundler/sandbox/enclave-adapter.tslibs/ui/src/bridge/__tests__/adapter-registry.test.tslibs/ui/src/web-components/core/base-element.test.tslibs/ui/src/web-components/elements/elements.test.tslibs/plugins/src/codecall/__tests__/codecall-config.provider.test.tslibs/testing/src/matchers/matcher-types.tslibs/ui/src/bridge/__tests__/bridge-factory.test.tslibs/plugins/src/codecall/__tests__/search.tool.test.ts
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to libs/{sdk,adapters,plugins,cli}/src/**/*.ts : Use `getCapabilities()` for dynamic capability exposure instead of hardcoding capabilities in adapters
Applied to files:
libs/ui/src/bridge/__tests__/adapter-registry.test.ts
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to libs/{sdk,adapters,plugins,cli}/src/**/*.ts : Validate hooks match their entry type and fail fast with InvalidHookFlowError for unsupported flows
Applied to files:
libs/sdk/src/__test-utils__/mocks/hook-registry.mock.tslibs/sdk/src/__test-utils__/fixtures/hook.fixtures.tslibs/sdk/src/flows/flow.instance.ts
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to libs/{sdk,adapters,plugins,cli}/src/**/*.ts : Use `changeScope` instead of `scope` for change event properties to avoid confusion with the Scope class
Applied to files:
libs/sdk/src/__test-utils__/mocks/hook-registry.mock.tslibs/sdk/src/common/decorators/decorator-utils.tslibs/sdk/src/__test-utils__/fixtures/hook.fixtures.tslibs/ui/src/web-components/types.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/**/*.test.ts : Test HTML escaping for user-provided content to prevent XSS attacks
Applied to files:
libs/ui/src/web-components/core/base-element.test.tslibs/ui/src/web-components/elements/elements.test.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/components/**/*.ts : Use pure HTML string generation without React/Vue/JSX - components return HTML strings only
Applied to files:
libs/ui/src/web-components/core/base-element.test.tslibs/ui/src/render/prerender.tslibs/ui/src/web-components/elements/elements.test.tslibs/ui/src/runtime/adapters/react.adapter.tslibs/ui/src/types/ui-runtime.tslibs/ui/src/web-components/types.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/components/**/*.test.ts : Write validation tests covering invalid variant/options, unknown properties, and valid option acceptance
Applied to files:
libs/ui/src/web-components/core/base-element.test.tslibs/testing/src/matchers/matcher-types.tslibs/plugins/src/codecall/__tests__/search.tool.test.ts
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to libs/{sdk,adapters,plugins,cli}/src/**/*.ts : Don't mutate rawInput in flows - use state.set() for managing flow state instead
Applied to files:
libs/sdk/src/__test-utils__/fixtures/hook.fixtures.tslibs/sdk/src/flows/flow.instance.ts
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to **/*.ts : Enable strict TypeScript mode with no `any` types without strong justification - use `unknown` instead for generic type defaults
Applied to files:
eslint.config.mjslibs/ui/src/web-components/types.ts
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to **/*.ts : Follow the preset pattern for hierarchical configurations across the codebase
Applied to files:
eslint.config.mjs
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/components/**/*.schema.ts : Every component must have a Zod schema with `.strict()` mode to reject unknown properties
Applied to files:
eslint.config.mjslibs/plugins/src/codecall/__tests__/search.tool.test.ts
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to **/*.ts : Avoid non-null assertions (`!`) - use proper error handling and throw specific errors instead
Applied to files:
eslint.config.mjs
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to **/*.ts : Use type parameters with constraints instead of unconstrained generics, and prefer `unknown` over `any` for generic type defaults
Applied to files:
eslint.config.mjs
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to **/*.test.ts : Test all code paths including errors, constructor validation, and error class `instanceof` checks
Applied to files:
eslint.config.mjslibs/testing/src/matchers/matcher-types.ts
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Use FrontMCP's TypeScript-first schema validation framework philosophy - all types should align with MCP protocol definitions
Applied to files:
libs/testing/src/matchers/matcher-types.tslibs/sdk/package.json
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to libs/{sdk,adapters,plugins,cli}/src/**/*.ts : Return strictly typed MCP protocol responses (GetPromptResult, ReadResourceResult, etc.) instead of `unknown` for `execute()` and `read()` methods
Applied to files:
libs/testing/src/matchers/matcher-types.ts
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Use the Nx build system for all build commands (nx build sdk, nx test ast-guard, nx run-many -t test)
Applied to files:
.github/workflows/push.yml
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/**/*.test.ts : Maintain 95%+ test coverage across statements, branches, functions, and lines
Applied to files:
libs/plugins/src/codecall/__tests__/search.tool.test.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/components/** : Organize components into schema.ts (definitions) and implementation .ts files with matching names
Applied to files:
libs/ui/src/web-components/types.ts
🧬 Code graph analysis (3)
libs/sdk/src/__test-utils__/fixtures/hook.fixtures.ts (2)
libs/sdk/src/context/frontmcp-context.ts (1)
flow(291-293)libs/sdk/src/common/metadata/hook.metadata.ts (1)
HookMetadata(17-25)
libs/testing/src/matchers/matcher-types.ts (2)
libs/testing/src/index.ts (1)
McpMatchers(173-173)libs/testing/src/matchers/index.ts (1)
McpMatchers(9-9)
libs/sdk/src/flows/flow.instance.ts (2)
libs/sdk/src/flows/flow.stages.ts (2)
StageMap(10-10)cloneStageMap(22-26)libs/sdk/src/common/interfaces/flow.interface.ts (1)
FlowControl(19-43)
🔇 Additional comments (29)
libs/adapters/src/openapi/openapi.utils.ts (1)
140-141: Appropriate ESLint suppression for security validation.The inline disable of
no-control-regexis justified here since the regex intentionally validates for control characters to prevent header injection attacks. The suppression is correctly scoped to just this line.libs/ui/src/types/ui-runtime.ts (1)
1235-1235: LGTM: ESLint directive cleanup improves readability.The eslint-disable directive is now properly positioned on a separate line before the namespace declaration, making the suppression intent clearer.
libs/ui/src/bridge/runtime/iife-generator.ts (1)
942-942: LGTM! The regex escaping is correct.The addition of square brackets to the punctuation minification regex is safe and correctly escaped. Removing whitespace around
[]characters doesn't affect JavaScript semantics.libs/ui/src/widgets/progress.ts (1)
471-483: LGTM—Good practice for case blocks with variable declarations.Adding braces around the default case is appropriate here since it contains local variable declarations (
linesHtml). This prevents scope-related issues and satisfies ESLint rules likeno-case-declarations.libs/sdk/src/__test-utils__/fixtures/oauth.fixtures.ts (1)
64-74: LGTM!The removal of the explicit type annotation is safe—TypeScript correctly infers
numberfrom the default value64. The function retains proper validation bounds and the JSDoc documentation accurately describes the parameter constraints.libs/sdk/src/__test-utils__/mocks/hook-registry.mock.ts (1)
57-66: LGTM!The type annotation removal on
priorityis safe—TypeScript infersnumberfrom the default value0. The mock entry factory provides appropriate test data for hook testing.libs/sdk/src/common/decorators/decorator-utils.ts (1)
202-239: LGTM!The type annotation removal is safe—TypeScript correctly infers
booleanfrom the default valuetrue. The method logic for scanning prototype and static members remains unchanged and correctly handles both consumption modes.libs/sdk/src/__test-utils__/fixtures/hook.fixtures.ts (3)
32-41: LGTM!The type annotation removal is safe—TypeScript infers
stringfrom the default value'CallToolFlow'. The factory function correctly produceswilltype hook metadata for testing.
46-55: LGTM!Consistent with
createWillHookMetadata—TypeScript infers the correct types from the string literal defaults.
60-69: LGTM!All three hook metadata factory functions now follow the same parameter style, maintaining consistency across the test fixtures.
libs/sdk/src/flows/flow.instance.ts (5)
180-199: Good security improvement for anonymous session isolation.The previous approach where all unauthenticated requests shared
'anonymous'as a session ID could cause cross-request data leakage. Generating uniqueanon:${randomUUID()}per request properly isolates anonymous sessions. The explicit handling for empty strings (since'' ?? 'fallback'returns'') is correctly addressed with the trim and length check.
232-237: LGTM!Using
constforstagesis correct—the reference isn't reassigned even though the object contents are mutated viamergeHookMetasIntoStageMap.
256-281: LGTM!The safe logging pattern using
e instanceof Error ? e.message : 'Unknown error'prevents Node.js serialization issues with complex error objects like Zod validation errors. Graceful handling of hook materialization failures with a warning is appropriate.
289-294: Acceptable ESLint workaround.The
eslint-disable-next-line prefer-constis needed becausecontextmust be declared before thematerializeAndMergeclosure but assigned after. The pattern is valid—this is a known ESLint limitation with deferred initialization.
446-486: Solid flow execution control structure.The refactored control flow correctly ensures:
- EXECUTE only runs if PRE didn't respond (
if (!responded)guard)- POST always runs for cleanup/finalization hooks
- FINALIZE is guaranteed to execute on all paths
- Error handling is consistent across phases with proper error stage invocation
libs/sdk/src/logger/instances/instance.logger.ts (1)
55-56: LGTM - Appropriate console.error usage in error handler.The removal of the ESLint disable comment is appropriate here. The
console.errorcall is legitimate as it's used as a fallback when transport logging itself fails, preventing cascading errors.libs/ui/src/bundler/sandbox/enclave-adapter.ts (1)
323-355: LGTM - Explicit no-op implementations improve clarity.The explicit
/* noop */comments make the intent clear that these console methods are intentionally non-functional in the sandbox environment. This aligns with the new ESLint rules and improves code maintainability.libs/ui/src/bridge/__tests__/adapter-registry.test.ts (1)
35-107: LGTM - Test mocks with explicit no-op implementations.The explicit no-op implementations in the test MockAdapter improve readability and intent. While the new test-specific ESLint config would allow empty functions here, the explicit approach is clearer.
libs/ui/src/web-components/types.ts (1)
118-118: LGTM - Necessary ESLint suppression for JSX namespace.The ESLint disable directive is required here since the JSX namespace augmentation is the standard pattern for adding custom element types to React/Preact TypeScript definitions. There's no alternative approach that avoids the namespace.
libs/ui/src/bridge/__tests__/bridge-factory.test.ts (1)
83-104: LGTM - Consistent test mock implementation.The explicit no-op implementations align with the pattern used across other test files in this PR, improving consistency and readability.
libs/ui/src/render/prerender.ts (3)
26-27: LGTM - Necessary suppression for type augmentation.The ESLint disable for triple-slash reference is appropriate here as these references are required for augmenting external type definitions that don't have their own type declarations.
82-84: LGTM - Dynamic require for optional dependency.The removal of the ESLint disable comment is appropriate. The dynamic require is necessary here to handle the optional
react-domdependency gracefully, and this pattern is consistent with how optional peer dependencies should be handled.
93-101: LGTM - Clean availability check for optional dependencies.The
isReactAvailable()function provides a clean way to check for React availability without throwing errors. The broad catch is appropriate here since we're only checking availability, not performing the actual require.eslint.config.mjs (2)
14-16: LGTM - New rules align with codebase changes.The three new warn-level rules for empty functions, interfaces, and object types align well with the explicit no-op implementations added throughout the codebase. Using 'warn' instead of 'error' provides a grace period for migration.
21-21: LGTM - Module boundary allowlist expansion.Adding
'@frontmcp/sdk'to the module boundary allowlist is appropriate for internal SDK dependencies.libs/ui/src/web-components/core/base-element.test.ts (1)
50-50: LGTM! Type inference improves code clarity.The removal of explicit type annotations in favor of TypeScript's type inference is appropriate here. Both
_innerHTMLand thecontentparameter are correctly inferred asstringfrom their initializers, reducing verbosity without sacrificing type safety.Also applies to: 192-192
libs/ui/src/web-components/elements/elements.test.ts (1)
44-44: LGTM! Consistent type inference refactor.Removing explicit type annotations here mirrors the changes in
base-element.test.ts. Both_innerHTMLand thecontentparameter are correctly inferred asstring, maintaining type safety while reducing verbosity.Also applies to: 163-163
libs/ui/src/web-components/core/base-element.ts (1)
120-120: LGTM! Type inference appropriate for core class field.Removing the explicit type annotation from
_contentis safe and consistent with the broader refactoring pattern. The type is correctly inferred asstringfrom the initializer, and this change doesn't affect the public API surface or introduce breaking changes..github/workflows/push.yml (1)
122-169: Well-structured matrix strategy for E2E test parallelization.The refactoring from separate batch jobs to a matrix-based approach is an excellent improvement that:
- Enables better parallelization across 13 E2E test projects
- Uses
fail-fast: falseto ensure all test results are visible even when some fail- Maintains consistency with the Nx build system (
npx nx run ${{ matrix.project }}:test)This approach scales much better than maintaining multiple batch job definitions.
libs/plugins/src/codecall/__tests__/codecall-config.provider.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
libs/plugins/src/codecall/__tests__/search.tool.test.ts (2)
48-48: Consider converting to ES6 import for consistency.Line 48 still uses CommonJS
require()syntax to accessToolSearchService, which is inconsistent with the ES6 imports at the top of the file.Add the import at the top with other imports:
import SearchTool from '../tools/search.tool'; import type { SearchToolOutput } from '../tools/search.schema'; import { searchToolInputSchema } from '../tools/search.schema'; +import { ToolSearchService } from '../services';Then update line 48:
- tool.setService(require('../services').ToolSearchService, mockSearchService); + tool.setService(ToolSearchService, mockSearchService);As per coding guidelines, TypeScript files should use modern ES6 module syntax throughout.
41-42: Consider typingtoolmore strictly.The
toolvariable is typed asany, which reduces type safety. While this is acceptable in test mocks, consider typing it asSearchToolor usingunknownif the exact type is unavailable.Example:
- let tool: any; + let tool: SearchTool; beforeEach(() => { jest.clearAllMocks(); - tool = new (SearchTool as any)(); + tool = new SearchTool();As per coding guidelines, prefer
unknownoveranyfor generic type defaults and avoidanywithout strong justification.Also applies to: 46-47
libs/sdk/src/flows/flow.instance.ts (4)
290-291: Misplaced eslint-disable comment.The
prefer-construle applies to variable declarations, not assignments. Sincecontextis declared on line 253 withlet, this disable comment on the assignment statement has no effect. Either remove it or move to line 253 if the warning originates there.- // eslint-disable-next-line prefer-const context = new FlowClass(this.metadata, input, scope, appendContextHooks, mergedDeps);
410-442: Avoid non-null assertions per coding guidelines.Lines 421 and 436 use
!non-null assertions oncontrol. Per coding guidelines, prefer proper error handling over non-null assertions. Consider adding a guard or using a discriminated union forStageResult.if (pre.outcome === 'unknown_error' || pre.outcome === 'fail') { try { await runErrorStage(); } finally { await runFinalizeStage(); } - throw pre.control!; + throw pre.control ?? new Error('Internal: missing control for fail/error outcome'); }
444-460: Same non-null assertion concern on line 455.Apply the same guard pattern as suggested for the PRE block.
462-477: Consider extracting repeated error handling pattern.The try/error/finally/throw pattern appears 4 times (PRE, EXECUTE, POST blocks). Extract into a helper for DRY and consistent error handling:
const handleErrorOutcome = async (result: StageResult): Promise<never> => { try { await runErrorStage(); } finally { await runFinalizeStage(); } throw result.control ?? new Error('Internal: missing control'); };Then each block simplifies to:
if (post.outcome === 'unknown_error' || post.outcome === 'fail') { await handleErrorOutcome(post); }libs/plugins/src/codecall/codecall.types.ts (1)
168-213: Consider using.prefault()for consistency with the plugin options schema.The embedding options schema uses
.optional().transform(...)to apply defaults, while the main plugin options schema at line 336 uses.prefault(DEFAULT_PLUGIN_OPTIONS). For consistency and maintainability, consider refactoring the embedding schema to use.prefault()as well.Apply this diff to use
.prefault()consistently:export const codeCallEmbeddingOptionsSchema = z .object({ - strategy: embeddingStrategySchema.optional(), - modelName: z.string().optional(), - cacheDir: z.string().optional(), - useHNSW: z.boolean().optional(), - synonymExpansion: z.union([z.literal(false), synonymExpansionConfigSchema]).optional(), + strategy: embeddingStrategySchema, + modelName: z.string(), + cacheDir: z.string(), + useHNSW: z.boolean(), + synonymExpansion: z.union([z.literal(false), synonymExpansionConfigSchema]), }) - .optional() - .transform((opts) => ({ - strategy: opts?.strategy ?? DEFAULT_EMBEDDING_OPTIONS.strategy, - modelName: opts?.modelName ?? DEFAULT_EMBEDDING_OPTIONS.modelName, - cacheDir: opts?.cacheDir ?? DEFAULT_EMBEDDING_OPTIONS.cacheDir, - useHNSW: opts?.useHNSW ?? DEFAULT_EMBEDDING_OPTIONS.useHNSW, - synonymExpansion: opts?.synonymExpansion ?? DEFAULT_EMBEDDING_OPTIONS.synonymExpansion, - })); + .prefault(DEFAULT_EMBEDDING_OPTIONS);This approach:
- Matches the pattern used at line 336 for the main plugin options
- Reduces code duplication and maintenance burden
- Leverages Zod 4's
.prefault()for pre-parse defaults
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
libs/plugins/src/codecall/__tests__/codecall-config.provider.test.ts(1 hunks)libs/plugins/src/codecall/__tests__/search.tool.test.ts(1 hunks)libs/plugins/src/codecall/codecall.types.ts(2 hunks)libs/sdk/package.json(1 hunks)libs/sdk/src/auth/session/record/session.stateless.ts(2 hunks)libs/sdk/src/flows/flow.instance.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/sdk/src/auth/session/record/session.stateless.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Enable strict TypeScript mode with noanytypes without strong justification - useunknowninstead for generic type defaults
Avoid non-null assertions (!) - use proper error handling and throw specific errors instead
Use specific error classes with MCP error codes instead of generic errors
Use type parameters with constraints instead of unconstrained generics, and preferunknownoveranyfor generic type defaults
Follow the preset pattern for hierarchical configurations across the codebase
Files:
libs/sdk/src/flows/flow.instance.tslibs/plugins/src/codecall/__tests__/search.tool.test.tslibs/plugins/src/codecall/__tests__/codecall-config.provider.test.tslibs/plugins/src/codecall/codecall.types.ts
libs/{sdk,adapters,plugins,cli}/src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
libs/{sdk,adapters,plugins,cli}/src/**/*.ts: Return strictly typed MCP protocol responses (GetPromptResult, ReadResourceResult, etc.) instead ofunknownforexecute()andread()methods
Validate URIs per RFC 3986 at metadata level using Zod validation with custom refinements
UsegetCapabilities()for dynamic capability exposure instead of hardcoding capabilities in adapters
UsechangeScopeinstead ofscopefor change event properties to avoid confusion with the Scope class
Validate hooks match their entry type and fail fast with InvalidHookFlowError for unsupported flows
Don't mutate rawInput in flows - use state.set() for managing flow state instead
Files:
libs/sdk/src/flows/flow.instance.tslibs/plugins/src/codecall/__tests__/search.tool.test.tslibs/plugins/src/codecall/__tests__/codecall-config.provider.test.tslibs/plugins/src/codecall/codecall.types.ts
libs/**
⚙️ CodeRabbit configuration file
libs/**: Contains publishable SDK libraries. Review for API correctness, breaking changes, and consistency with docs. When public APIs change, ensure there is a matching docs/draft/docs/** update (not direct edits under docs/docs/**).
Files:
libs/sdk/src/flows/flow.instance.tslibs/sdk/package.jsonlibs/plugins/src/codecall/__tests__/search.tool.test.tslibs/plugins/src/codecall/__tests__/codecall-config.provider.test.tslibs/plugins/src/codecall/codecall.types.ts
**/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.ts: Achieve and maintain 95%+ test coverage across all metrics (statements, branches, functions, lines)
Test all code paths including errors, constructor validation, and error classinstanceofchecks
Files:
libs/plugins/src/codecall/__tests__/search.tool.test.tslibs/plugins/src/codecall/__tests__/codecall-config.provider.test.ts
🧠 Learnings (9)
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to libs/{sdk,adapters,plugins,cli}/src/**/*.ts : Don't mutate rawInput in flows - use state.set() for managing flow state instead
Applied to files:
libs/sdk/src/flows/flow.instance.ts
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to libs/{sdk,adapters,plugins,cli}/src/**/*.ts : Validate hooks match their entry type and fail fast with InvalidHookFlowError for unsupported flows
Applied to files:
libs/sdk/src/flows/flow.instance.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/**/*.test.ts : Test behavior across platform configurations (OpenAI, Claude, Gemini, ngrok) where applicable
Applied to files:
libs/plugins/src/codecall/__tests__/search.tool.test.tslibs/plugins/src/codecall/__tests__/codecall-config.provider.test.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/components/**/*.test.ts : Write validation tests covering invalid variant/options, unknown properties, and valid option acceptance
Applied to files:
libs/plugins/src/codecall/__tests__/search.tool.test.tslibs/plugins/src/codecall/__tests__/codecall-config.provider.test.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/components/** : Organize components into schema.ts (definitions) and implementation .ts files with matching names
Applied to files:
libs/plugins/src/codecall/__tests__/search.tool.test.ts
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to libs/{sdk,adapters,plugins,cli}/src/**/*.ts : Use `changeScope` instead of `scope` for change event properties to avoid confusion with the Scope class
Applied to files:
libs/plugins/src/codecall/__tests__/search.tool.test.ts
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to **/*.test.ts : Test all code paths including errors, constructor validation, and error class `instanceof` checks
Applied to files:
libs/plugins/src/codecall/__tests__/search.tool.test.ts
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to libs/*/src/common/records/**/*.ts : Centralize record types in common/records and import from there instead of from module-specific files
Applied to files:
libs/plugins/src/codecall/__tests__/search.tool.test.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/components/**/*.schema.ts : Every component must have a Zod schema with `.strict()` mode to reject unknown properties
Applied to files:
libs/plugins/src/codecall/__tests__/search.tool.test.ts
🧬 Code graph analysis (1)
libs/sdk/src/flows/flow.instance.ts (2)
libs/sdk/src/flows/flow.stages.ts (2)
StageMap(10-10)cloneStageMap(22-26)libs/sdk/src/common/interfaces/flow.interface.ts (1)
FlowControl(19-43)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build Libraries
- GitHub Check: Lint & Format Checks
🔇 Additional comments (6)
libs/plugins/src/codecall/__tests__/search.tool.test.ts (1)
5-5: LGTM! ES6 import addresses previous feedback.The static ES6 import of
searchToolInputSchemacorrectly addresses the previous review comment about inconsistent module syntax.libs/sdk/src/flows/flow.instance.ts (3)
234-234: Good change toconstfor immutability.The use of
StageMap<any>is acceptable here given the runtime type erasure explanation in the nearby comment (lines 251-252).
268-273: Safe error logging pattern.The defensive logging approach correctly handles Zod error serialization issues.
479-482: LGTM - finalize always runs on normal path.The refactored flow correctly ensures
runFinalizeStage()executes in all code paths before returning or throwing.libs/plugins/src/codecall/__tests__/codecall-config.provider.test.ts (1)
3-12: LGTM! Import and type changes are correct.The changes properly integrate with the schema updates in
codecall.types.ts:
BaseConfigis now imported from the public SDK API (resolving the previous module boundary concern)CodeCallPluginOptionsInputis correctly used as the constructor parameter type, allowing partial/optional inputs- The constructor properly parses the input via
codeCallPluginOptionsSchema.parse(options)at line 9, ensuring defaults are appliedThe test coverage remains comprehensive, validating default initialization, custom options, and invalid inputs.
libs/sdk/package.json (1)
43-49: Add migration documentation for SDK peer dependency changes.The SDK now requires peer dependencies on Express ecosystem packages (
express,cors,raw-body,content-type). This is a breaking change for SDK consumers. Per coding guidelines, publishable SDK library changes require corresponding documentation underdocs/draft/docs/**.Currently, the documentation covers server configuration and deployment but does not explain the peer dependency requirements or how to upgrade existing SDKs. Consumers need clear guidance on:
- Required peer dependencies and versions
- Installation steps for SDK consumers
- Any breaking changes from previous versions
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
libs/sdk/src/flows/flow.instance.ts (2)
234-234: Consider adding a comment explaining theanytype usage.Per coding guidelines,
anytypes should have strong justification. While lines 251-252 explain runtime type erasure forcontext, this line lacks similar documentation. A brief inline comment would clarify whyStageMap<any>is necessary here.- const stages: StageMap<any> = cloneStageMap(baseStages); + // StageMap uses `any` due to runtime type erasure of FlowClass generic + const stages: StageMap<any> = cloneStageMap(baseStages);
422-424: Add defensive null check before type assertion.The coding guidelines advise against non-null assertions without proper error handling. While
controlshould always be defined for 'abort'/'next'/'handled' outcomes based onrunListlogic, a defensive check prevents confusing runtime errors if invariants are violated elsewhere.if (post.outcome === 'abort' || post.outcome === 'next' || post.outcome === 'handled') { await runFinalizeStage(); - throw post.control as FlowControl; + throw post.control ?? new Error(`Internal: missing control for ${post.outcome} outcome`); }Apply the same pattern to similar assertions at lines 439, 457, and 474.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libs/plugins/src/codecall/__tests__/search.tool.test.ts(2 hunks)libs/sdk/src/flows/flow.instance.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/plugins/src/codecall/tests/search.tool.test.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Enable strict TypeScript mode with noanytypes without strong justification - useunknowninstead for generic type defaults
Avoid non-null assertions (!) - use proper error handling and throw specific errors instead
Use specific error classes with MCP error codes instead of generic errors
Use type parameters with constraints instead of unconstrained generics, and preferunknownoveranyfor generic type defaults
Follow the preset pattern for hierarchical configurations across the codebase
Files:
libs/sdk/src/flows/flow.instance.ts
libs/{sdk,adapters,plugins,cli}/src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
libs/{sdk,adapters,plugins,cli}/src/**/*.ts: Return strictly typed MCP protocol responses (GetPromptResult, ReadResourceResult, etc.) instead ofunknownforexecute()andread()methods
Validate URIs per RFC 3986 at metadata level using Zod validation with custom refinements
UsegetCapabilities()for dynamic capability exposure instead of hardcoding capabilities in adapters
UsechangeScopeinstead ofscopefor change event properties to avoid confusion with the Scope class
Validate hooks match their entry type and fail fast with InvalidHookFlowError for unsupported flows
Don't mutate rawInput in flows - use state.set() for managing flow state instead
Files:
libs/sdk/src/flows/flow.instance.ts
libs/**
⚙️ CodeRabbit configuration file
libs/**: Contains publishable SDK libraries. Review for API correctness, breaking changes, and consistency with docs. When public APIs change, ensure there is a matching docs/draft/docs/** update (not direct edits under docs/docs/**).
Files:
libs/sdk/src/flows/flow.instance.ts
🧠 Learnings (2)
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to libs/{sdk,adapters,plugins,cli}/src/**/*.ts : Don't mutate rawInput in flows - use state.set() for managing flow state instead
Applied to files:
libs/sdk/src/flows/flow.instance.ts
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to libs/{sdk,adapters,plugins,cli}/src/**/*.ts : Validate hooks match their entry type and fail fast with InvalidHookFlowError for unsupported flows
Applied to files:
libs/sdk/src/flows/flow.instance.ts
🔇 Additional comments (3)
libs/sdk/src/flows/flow.instance.ts (3)
443-459: LGTM on stage execution flow.The EXECUTE stage handling follows the established pattern correctly. The control flow properly handles all outcome types and ensures error/finalize stages run appropriately. The type assertion issue at line 457 is addressed by the earlier suggestion.
461-481: Well-structured stage orchestration.The refactored flow execution with explicit PRE/EXECUTE/POST/FINALIZE phases is cleaner and easier to follow. Error handling consistently runs the error stage before finalization, and the control flow properly propagates outcomes through each phase.
262-273: No changes needed to the code; the flow validation already follows the learning guidelines.The learning requirement to "fail fast with InvalidHookFlowError for unsupported flows" is already implemented at class definition time in
ToolInstanceandResourceInstance. Runtime-injected hooks are a separate concern where graceful degradation (warning + skip) is the intended behavior for metadata materialization failures. The try-catch at lines 264-273 correctly handles this by catching metadata access errors and logging safely, not by catching flow type mismatches.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
libs/sdk/src/flows/flow.instance.ts (1)
234-234: Consider using a more specific type parameter.The
anytype parameter reduces type safety. While the generic type erasure mentioned at line 251-252 provides some justification forFlowBase<any>, consider whetherStageMap<FlowType>orStageMap<typeof FlowClass>would be more appropriate here.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libs/sdk/src/flows/flow.instance.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Enable strict TypeScript mode with noanytypes without strong justification - useunknowninstead for generic type defaults
Avoid non-null assertions (!) - use proper error handling and throw specific errors instead
Use specific error classes with MCP error codes instead of generic errors
Use type parameters with constraints instead of unconstrained generics, and preferunknownoveranyfor generic type defaults
Follow the preset pattern for hierarchical configurations across the codebase
Files:
libs/sdk/src/flows/flow.instance.ts
libs/{sdk,adapters,plugins,cli}/src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
libs/{sdk,adapters,plugins,cli}/src/**/*.ts: Return strictly typed MCP protocol responses (GetPromptResult, ReadResourceResult, etc.) instead ofunknownforexecute()andread()methods
Validate URIs per RFC 3986 at metadata level using Zod validation with custom refinements
UsegetCapabilities()for dynamic capability exposure instead of hardcoding capabilities in adapters
UsechangeScopeinstead ofscopefor change event properties to avoid confusion with the Scope class
Validate hooks match their entry type and fail fast with InvalidHookFlowError for unsupported flows
Don't mutate rawInput in flows - use state.set() for managing flow state instead
Files:
libs/sdk/src/flows/flow.instance.ts
libs/**
⚙️ CodeRabbit configuration file
libs/**: Contains publishable SDK libraries. Review for API correctness, breaking changes, and consistency with docs. When public APIs change, ensure there is a matching docs/draft/docs/** update (not direct edits under docs/docs/**).
Files:
libs/sdk/src/flows/flow.instance.ts
🧠 Learnings (2)
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to libs/{sdk,adapters,plugins,cli}/src/**/*.ts : Don't mutate rawInput in flows - use state.set() for managing flow state instead
Applied to files:
libs/sdk/src/flows/flow.instance.ts
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to libs/{sdk,adapters,plugins,cli}/src/**/*.ts : Validate hooks match their entry type and fail fast with InvalidHookFlowError for unsupported flows
Applied to files:
libs/sdk/src/flows/flow.instance.ts
🧬 Code graph analysis (1)
libs/sdk/src/flows/flow.instance.ts (2)
libs/sdk/src/flows/flow.stages.ts (2)
StageMap(10-10)cloneStageMap(22-26)libs/sdk/src/common/interfaces/flow.interface.ts (1)
FlowControl(19-43)
🔇 Additional comments (1)
libs/sdk/src/flows/flow.instance.ts (1)
268-268: LGTM: ESLint directive cleanup.The removal of the
no-consoledisable at line 268 is appropriate for logging failed hook materialization. The added explanation at line 290 clearly justifies whyprefer-constmust be disabled for the context declaration.Also applies to: 290-290
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
eslint.config.mjs (1)
54-63: Consider more selective rule relaxation for test files.While relaxing rules for test files is common, completely disabling
@typescript-eslint/no-explicit-anyand@typescript-eslint/no-unused-varsmay be too permissive. The main configuration already provides the underscore pattern (^_) for intentionally unused variables, which should suffice for tests as well.Based on learnings, the codebase prefers strict TypeScript mode with strong justification for
anytypes.Based on learnings, consider this more targeted approach:
{ // Relax rules for test files files: ['**/*.test.ts', '**/*.spec.ts', '**/__tests__/**/*.ts', '**/__test-utils__/**/*.ts', '**/fixtures/**/*.ts', '**/mocks/**/*.ts', '**/*.mock.ts'], rules: { '@typescript-eslint/no-empty-function': 'off', - '@typescript-eslint/no-explicit-any': 'off', - '@typescript-eslint/no-unused-vars': 'off', + '@typescript-eslint/no-explicit-any': 'warn', // Keep some type safety even in tests 'no-unused-private-class-members': 'off', }, },The underscore pattern from the main config (lines 20-22) will still apply in test files for intentionally unused variables, making the complete disable of
no-unused-varsunnecessary.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
eslint.config.mjs(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to **/*.ts : Enable strict TypeScript mode with no `any` types without strong justification - use `unknown` instead for generic type defaults
Applied to files:
eslint.config.mjs
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to **/*.ts : Follow the preset pattern for hierarchical configurations across the codebase
Applied to files:
eslint.config.mjs
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/components/**/*.schema.ts : Every component must have a Zod schema with `.strict()` mode to reject unknown properties
Applied to files:
eslint.config.mjs
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to **/*.test.ts : Test all code paths including errors, constructor validation, and error class `instanceof` checks
Applied to files:
eslint.config.mjs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build Libraries
🔇 Additional comments (3)
eslint.config.mjs (3)
14-16: LGTM! Good addition of empty construct warnings.These warning-level rules will help catch unintended empty functions, interfaces, and object types while maintaining flexibility in the codebase.
17-24: LGTM! Standard pattern for intentionally unused variables.The underscore prefix convention is a well-established pattern for signaling intentionally unused parameters, variables, and caught errors.
29-29: Verify that@frontmcp/sdkshould bypass module boundary enforcement.Adding
@frontmcp/sdkto the allow list permits it to be imported across all module boundaries. Ensure this aligns with your architectural constraints and that this package is intended to be universally accessible.Run the following script to verify the package structure and imports:
#!/bin/bash # Description: Verify @frontmcp/sdk package structure and usage patterns # Check if @frontmcp/sdk is defined in the workspace echo "=== Checking workspace structure ===" fd -t f 'package.json' | xargs grep -l '"name".*"@frontmcp/sdk"' || echo "SDK package not found in workspace" # Find all imports of @frontmcp/sdk to understand usage patterns echo -e "\n=== Finding @frontmcp/sdk imports ===" rg -n --type ts --type tsx "from ['\"]@frontmcp/sdk" -C 1 # Check if this is a recently added package echo -e "\n=== Checking git history for SDK package ===" git log --all --oneline --grep="@frontmcp/sdk" -n 5
…McpError in flow instance
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
libs/sdk/src/flows/flow.instance.ts (1)
234-234: Consider documenting theanytype justification inline.The
StageMap<any>type is intentional due to runtime type erasure (as explained forFlowBase<any>on lines 251-252). Adding a brief inline comment here would clarify this intentional deviation from theunknownpreference.- const stages: StageMap<any> = cloneStageMap(baseStages); + // StageMap<any> is intentional: FlowClass generic is erased at runtime, + // and stages are invoked dynamically via method calls + const stages: StageMap<any> = cloneStageMap(baseStages);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libs/sdk/src/flows/flow.instance.ts(5 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Enable strict TypeScript mode with noanytypes without strong justification - useunknowninstead for generic type defaults
Avoid non-null assertions (!) - use proper error handling and throw specific errors instead
Use specific error classes with MCP error codes instead of generic errors
Use type parameters with constraints instead of unconstrained generics, and preferunknownoveranyfor generic type defaults
Follow the preset pattern for hierarchical configurations across the codebase
Files:
libs/sdk/src/flows/flow.instance.ts
libs/{sdk,adapters,plugins,cli}/src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
libs/{sdk,adapters,plugins,cli}/src/**/*.ts: Return strictly typed MCP protocol responses (GetPromptResult, ReadResourceResult, etc.) instead ofunknownforexecute()andread()methods
Validate URIs per RFC 3986 at metadata level using Zod validation with custom refinements
UsegetCapabilities()for dynamic capability exposure instead of hardcoding capabilities in adapters
UsechangeScopeinstead ofscopefor change event properties to avoid confusion with the Scope class
Validate hooks match their entry type and fail fast with InvalidHookFlowError for unsupported flows
Don't mutate rawInput in flows - use state.set() for managing flow state instead
Files:
libs/sdk/src/flows/flow.instance.ts
libs/**
⚙️ CodeRabbit configuration file
libs/**: Contains publishable SDK libraries. Review for API correctness, breaking changes, and consistency with docs. When public APIs change, ensure there is a matching docs/draft/docs/** update (not direct edits under docs/docs/**).
Files:
libs/sdk/src/flows/flow.instance.ts
🧠 Learnings (6)
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to libs/{sdk,adapters,plugins,cli}/src/**/*.ts : Don't mutate rawInput in flows - use state.set() for managing flow state instead
Applied to files:
libs/sdk/src/flows/flow.instance.ts
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to **/*.ts : Avoid non-null assertions (`!`) - use proper error handling and throw specific errors instead
Applied to files:
libs/sdk/src/flows/flow.instance.ts
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to **/*.ts : Use specific error classes with MCP error codes instead of generic errors
Applied to files:
libs/sdk/src/flows/flow.instance.ts
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to libs/{sdk,adapters,plugins,cli}/src/**/*.ts : Return strictly typed MCP protocol responses (GetPromptResult, ReadResourceResult, etc.) instead of `unknown` for `execute()` and `read()` methods
Applied to files:
libs/sdk/src/flows/flow.instance.ts
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to libs/{sdk,adapters,plugins,cli}/src/**/*.ts : Validate hooks match their entry type and fail fast with InvalidHookFlowError for unsupported flows
Applied to files:
libs/sdk/src/flows/flow.instance.ts
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to **/*.test.ts : Test all code paths including errors, constructor validation, and error class `instanceof` checks
Applied to files:
libs/sdk/src/flows/flow.instance.ts
🧬 Code graph analysis (1)
libs/sdk/src/flows/flow.instance.ts (3)
libs/sdk/src/flows/flow.stages.ts (2)
StageMap(10-10)cloneStageMap(22-26)libs/sdk/src/errors/mcp.error.ts (1)
InternalMcpError(129-142)libs/sdk/src/common/interfaces/flow.interface.ts (1)
FlowControl(19-43)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build Libraries
- GitHub Check: Lint & Format Checks
🔇 Additional comments (3)
libs/sdk/src/flows/flow.instance.ts (3)
31-31: LGTM!Good addition of
InternalMcpErrorfor specific error signaling. This aligns with the coding guidelines requiring MCP-specific error classes instead of generic errors.
290-291: LGTM!The ESLint disable is justified. The
contextvariable must be declared before helper functions (materializeAndMerge,appendContextHooks) that close over it, but assignment happens after those functions are defined.
410-494: LGTM! Past review feedback has been addressed.The flow orchestration rewrite properly implements:
- Specific error classes: All fallback errors now use
InternalMcpErrorinstead of genericError- Runtime validation:
instanceof FlowControlchecks before throwing ensure type safetyThe consistent pattern across PRE/EXECUTE/POST phases with proper error→finalize sequencing and try-finally blocks ensures reliable cleanup.
Summary by CodeRabbit
New Features
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.