feat(core): Support split by app with multiple auth options #44
Conversation
WalkthroughAdds two demo MCP apps (Calculator with 14 math tools; Employee Time with providers, plugins, utils, and multiple tools). Propagates ScopeEntry through auth/scope code (per-scope fullPath/id), updates primary auth constructors, makes SSE transport scope-aware, and enables per-app splitting in main app config. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Scope
participant AuthRegistry
participant PrimaryAuth
Client->>Scope: initialize (SPLIT_BY_APP)
Scope->>Scope: compute routeBase & fullPath
Scope->>AuthRegistry: new(scope, providers, ...)
AuthRegistry->>PrimaryAuth: new(scope, providers, options)
PrimaryAuth->>PrimaryAuth: use scope.fullPath for issuer/paths
AuthRegistry-->>Scope: auth wired
sequenceDiagram
participant Employee
participant RegisterExitTool
participant Redis
participant Directory
Employee->>RegisterExitTool: execute({ employeeId, siteId })
RegisterExitTool->>Redis: read open session key
alt open session exists
RegisterExitTool->>RegisterExitTool: splitSessionByDay(start,end)
loop per day segment
RegisterExitTool->>Redis: incr hoursKey
RegisterExitTool->>Redis: lpush sessionsKey
end
RegisterExitTool->>Directory: ensure employee in site
RegisterExitTool->>Redis: delete open session key
RegisterExitTool-->>Employee: { employeeId, endedAt, durationMs }
else no session
RegisterExitTool-->>Employee: error (no open session)
end
sequenceDiagram
participant Client
participant ToolExecution
participant SiteAuthPlugin
Client->>ToolExecution: call execute(...)
ToolExecution->>SiteAuthPlugin: will-execute hook (priority 900)
SiteAuthPlugin->>SiteAuthPlugin: extract siteId & claims
alt authorized
SiteAuthPlugin->>ToolExecution: allow
ToolExecution-->>Client: result
else denied
SiteAuthPlugin-->>Client: error (unauthorized)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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: 1
🧹 Nitpick comments (10)
apps/demo/src/apps/employee-time/tools/register-entry.tool.ts (1)
7-7: Consider removing the helper function.The
nowMs()function is a trivial wrapper aroundDate.now(). Consider callingDate.now()directly at line 27 for clarity.apps/demo/src/apps/employee-time/tools/admin-add-entry.tool.ts (1)
20-35: Entry creation logic looks solid; consider atomic open-session creationThe overall flow (time normalization, pre‑existence check, setting the open key, updating the directory, and returning
startedAt) is consistent and correct for the other tools in this app.One potential edge case is concurrency:
exists(openKey)followed byset(openKey, ...)is not atomic, so two concurrent calls could both see “no open session” and create duplicate open sessions for the same employee/site. If you expect concurrent usage, consider adding an atomic “set if not exists” helper inEmployeeRedisProviderand using that here to enforce a single open session per employee/site.apps/demo/src/apps/employee-time/tools/list-employees.tool.ts (1)
5-35: Behavior is correct; tighten output typing/schema for better contractsThe execution logic and pagination math look correct and align with
EmployeeDirectoryProvider.listEmployees(including the 1..200, default‑50 limit clamping) and per‑employee site lookup.Two small improvements you might consider:
Explicit output schema
outputSchema: {} as anygives up type safety at the tool boundary. Defining a concrete Zod schema for:{ result: { total: number; items: { employeeId: string; sites: string[] }[]; page: { offset: number; limit: number }; } }would make the tool contract clearer and keep it in sync with the implementation.
Optional: Redis round‑trip optimization
Foritems.lengthup to 200 this is fine, but you may later want a directory method that returns{ employeeId, sites }in one pass to avoid one Redis call per employee in high‑latency environments.apps/demo/src/apps/employee-time/tools/admin-add-exit.tool.ts (1)
20-48: Exit handling is correct and consistent; watch for concurrent double‑processingThe core flow looks correct and matches
RegisterExitTool:
- Validates an existing open session and
end >= start.- Uses
splitSessionByDayand updateshoursKey/sessionsKeyaccordingly.- Ensures the employee is associated with the site and deletes the open key.
As with the entry tool, there’s a concurrency edge case: two
admin-add-exitcalls in parallel for the same employee/site could both read the same open session beforedel, each incrementing hours and pushing sessions, effectively double‑counting. If this tool is exposed in contexts where that can happen, consider an atomic “read‑and‑clear” or a simple locking mechanism (e.g., using a per‑employee/site mutex key) around this block.apps/demo/src/apps/employee-time/tools/report-hours.tool.ts (1)
22-90: Solid aggregation logic; consider a defensive period fallback and sharing the ms→hours helperThe execute implementation reads clean and the 4 scoping cases (site+employee, site-wide, employee across sites, all sites) look logically correct, assuming the helpers behave as expected. A couple of small improvements you might want to consider:
- Add a
defaultbranch in theswitch (input.period)to guard against unexpected values arriving outside the Tool schema path, and throw a clear error rather than potentially callingeachDayRangewith uninitialisedstart/end.- You repeat
Math.round((ms / 3600000) * 1000) / 1000several times here and again inGetEmployeeDetailsTool; a smalltoHours(ms)helper (either local or in a shared util) would reduce duplication and ensure consistent rounding semantics across tools.- If you care about manifest/schema introspection, replacing
outputSchema: {} as anywith a basicz.objectthat at least captures the top-level{ result: { scope, period, start, end, ... } }shape would make the tool’s contract clearer, even if you don’t model every variant in detail.apps/demo/src/apps/employee-time/tools/get-employee-details.tool.ts (1)
39-76: Move day-range computation outside the site loop and hardenopenSinceagainst bad dataThe overall behaviour looks right (per-site open check + today/week/month aggregation), but there are a couple of easy cleanups:
weekDaysandmonthDaysare recomputed inside thefor (const siteId of sites)loop even though they only depend onweekStart/EndandmonthStart/End. Precomputing them once before the loop avoids repeated allocations and minor CPU overhead, especially if employees can belong to many sites.openSinceis derived fromNumber(openStr). If the stored value is ever non-numeric, you’ll end up withNaNand propagate that throughMath.min, yieldingopenSince: NaNin the response. A simpleif (Number.isFinite(os))guard before updatingopenSincewould keep the output sane under corrupted data.Both changes are non-breaking and keep the logic the same while making the tool a bit more robust and efficient.
apps/demo/src/apps/employee-time/providers/redis.provider.ts (1)
11-88: Expose asremhelper to avoid callers poking at the private Redis clientThe wrapper around ioredis is clean, but because it doesn’t expose a
sremoperation, other code ends up castingEmployeeRedisProvidertoanyand reaching into the private.clientto callsrem. That’s brittle and bypasses the abstraction you’re building here.Consider adding a small helper on this provider, e.g.:
async srem(key: string, ...members: string[]): Promise<number> { // delegate to the underlying client return (this.client as any).srem(key, ...members); }so that consumers like
EmployeeDirectoryProvidercan stay within the typed API and you remain free to change the underlying client without breaking them.apps/demo/src/apps/employee-time/providers/employee-directory.provider.ts (1)
14-56: Avoid reaching into the Redis provider’s private client; rely on a helper instead
removeEmployeeFromSitecurrently does:const client: any = (this.redis as any).client; if (client?.srem) { await client.srem(...); }which breaks the encapsulation of
EmployeeRedisProviderand depends on an implementation detail (clientandsrem) that isn’t part of its public surface. This will be fragile if the provider ever changes how it wraps ioredis.Once
EmployeeRedisProviderexposes a typedsremhelper, you can simplify this to:await this.redis.srem(siteEmployeesSetKey(siteId), employeeId); await this.redis.srem(employeeSitesSetKey(employeeId), siteId);and drop the
anycast and optional chaining. The rest ofEmployeeDirectoryProvider(site listing, de-duplication, pagination, search) looks good for a demo-scale directory; if you expect large numbers of sites/employees, you might later revisitlistEmployeesto avoid sequentialsmembersacross all sites.apps/demo/src/apps/employee-time/utils/hours.ts (2)
31-38: Consider batching all Redis keys into a single mget call for better performance.Both
sumEmployeesRangeMs(line 31) andsumEmployeesRangeMsForSite(line 59) iterate sequentially over employees, making onemgetcall per employee. For N employees and D days, this results in N separate Redis round-trips.You could optimize by constructing all N×D keys upfront and fetching them in a single
mget, then partitioning the results by employee. For example, with 100 employees over a 30-day range, this would reduce from 100 Redis calls to 1.Since this is a demo app, the current implementation may be acceptable, but worth considering if this pattern is used in production code.
Also applies to: 59-66
40-66: Consider extracting shared logic to reduce duplication.The site-aware helpers (lines 40-66) duplicate the logic from their legacy counterparts (lines 13-38), differing only in key generation. You could refactor by extracting the core logic into shared helper functions that accept a key generator as a parameter.
For example:
async function getEmployeeDayMsInternal( redis: EmployeeRedisProvider, keyFn: () => string ): Promise<number> { const v = await redis.get(keyFn()); const n = v ? Number(v) : 0; return Number.isFinite(n) ? n : 0; }Since this is a demo app and the functions are relatively short, the current duplication may be acceptable for clarity.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (41)
apps/demo/src/apps/calculator/index.ts(1 hunks)apps/demo/src/apps/calculator/tools/abs.tool.ts(1 hunks)apps/demo/src/apps/calculator/tools/add.tool.ts(1 hunks)apps/demo/src/apps/calculator/tools/ceil.tool.ts(1 hunks)apps/demo/src/apps/calculator/tools/divide.tool.ts(1 hunks)apps/demo/src/apps/calculator/tools/exp.tool.ts(1 hunks)apps/demo/src/apps/calculator/tools/floor.tool.ts(1 hunks)apps/demo/src/apps/calculator/tools/max.tool.ts(1 hunks)apps/demo/src/apps/calculator/tools/min.tool.ts(1 hunks)apps/demo/src/apps/calculator/tools/modulo.tool.ts(1 hunks)apps/demo/src/apps/calculator/tools/multiply.tool.ts(1 hunks)apps/demo/src/apps/calculator/tools/pow.tool.ts(1 hunks)apps/demo/src/apps/calculator/tools/round.tool.ts(1 hunks)apps/demo/src/apps/calculator/tools/sqrt.tool.ts(1 hunks)apps/demo/src/apps/calculator/tools/subtract.tool.ts(1 hunks)apps/demo/src/apps/employee-time/index.ts(1 hunks)apps/demo/src/apps/employee-time/plugins/site-authorization.plugin.ts(1 hunks)apps/demo/src/apps/employee-time/providers/employee-directory.provider.ts(1 hunks)apps/demo/src/apps/employee-time/providers/redis.provider.ts(1 hunks)apps/demo/src/apps/employee-time/tools/admin-add-entry.tool.ts(1 hunks)apps/demo/src/apps/employee-time/tools/admin-add-exit.tool.ts(1 hunks)apps/demo/src/apps/employee-time/tools/get-employee-details.tool.ts(1 hunks)apps/demo/src/apps/employee-time/tools/list-employees.tool.ts(1 hunks)apps/demo/src/apps/employee-time/tools/register-entry.tool.ts(1 hunks)apps/demo/src/apps/employee-time/tools/register-exit.tool.ts(1 hunks)apps/demo/src/apps/employee-time/tools/report-hours.tool.ts(1 hunks)apps/demo/src/apps/employee-time/utils/hours.ts(1 hunks)apps/demo/src/apps/employee-time/utils/keys.ts(1 hunks)apps/demo/src/apps/employee-time/utils/time.ts(1 hunks)apps/demo/src/apps/expenses/index.ts(1 hunks)apps/demo/src/main.ts(1 hunks)libs/sdk/src/auth/auth.registry.ts(3 hunks)libs/sdk/src/auth/instances/instance.local-primary-auth.ts(2 hunks)libs/sdk/src/auth/instances/instance.remote-primary-auth.ts(2 hunks)libs/sdk/src/auth/jwks/jwks.service.ts(0 hunks)libs/sdk/src/common/entries/scope.entry.ts(1 hunks)libs/sdk/src/common/records/scope.record.ts(2 hunks)libs/sdk/src/scope/scope.instance.ts(3 hunks)libs/sdk/src/scope/scope.registry.ts(2 hunks)libs/sdk/src/scope/scope.utils.ts(2 hunks)libs/sdk/src/transport/adapters/transport.sse.adapter.ts(1 hunks)
💤 Files with no reviewable changes (1)
- libs/sdk/src/auth/jwks/jwks.service.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-05T15:00:47.800Z
Learnt from: frontegg-david
Repo: agentfront/frontmcp PR: 11
File: libs/core/src/auth/jwks/jwks.service.ts:62-0
Timestamp: 2025-11-05T15:00:47.800Z
Learning: In the FrontMCP codebase (libs/core/src/auth/jwks/jwks.service.ts), the verifyGatewayToken method intentionally skips signature verification when running in development-only no-auth mode, using decodeJwtPayloadSafe instead of jwtVerify. This is a deliberate design for local development convenience and should not be flagged as a security issue when the PR or code context indicates development/no-auth mode.
Applied to files:
libs/sdk/src/auth/instances/instance.local-primary-auth.ts
🧬 Code graph analysis (30)
apps/demo/src/apps/calculator/tools/multiply.tool.ts (2)
apps/demo/src/apps/calculator/tools/add.tool.ts (1)
Tool(4-16)apps/demo/src/apps/calculator/tools/divide.tool.ts (1)
Tool(4-16)
apps/demo/src/apps/employee-time/index.ts (3)
apps/demo/src/apps/employee-time/providers/redis.provider.ts (1)
createEmployeeRedisProvider(90-96)apps/demo/src/apps/employee-time/providers/employee-directory.provider.ts (1)
createEmployeeDirectoryProvider(59-65)apps/demo/src/apps/expenses/plugins/authorization.plugin.ts (1)
ExtendFrontMcpToolMetadata(5-7)
apps/demo/src/apps/expenses/index.ts (2)
apps/demo/src/apps/expenses/plugins/authorization.plugin.ts (1)
ExtendFrontMcpToolMetadata(5-7)apps/demo/src/apps/expenses/provders/config.provider.ts (2)
ExpenseConfigProvider(8-26)constructor(19-21)
apps/demo/src/apps/calculator/tools/max.tool.ts (1)
apps/demo/src/apps/calculator/tools/min.tool.ts (1)
Tool(4-16)
libs/sdk/src/scope/scope.instance.ts (1)
libs/sdk/src/auth/auth.registry.ts (1)
AuthRegistry(17-95)
apps/demo/src/apps/calculator/tools/pow.tool.ts (2)
apps/demo/src/apps/calculator/tools/add.tool.ts (1)
Tool(4-16)apps/demo/src/apps/calculator/tools/divide.tool.ts (1)
Tool(4-16)
apps/demo/src/apps/calculator/tools/ceil.tool.ts (3)
apps/demo/src/apps/calculator/tools/abs.tool.ts (1)
Tool(4-16)apps/demo/src/apps/calculator/tools/floor.tool.ts (1)
Tool(4-16)apps/demo/src/apps/calculator/tools/round.tool.ts (1)
Tool(4-23)
apps/demo/src/apps/employee-time/tools/list-employees.tool.ts (1)
apps/demo/src/apps/employee-time/providers/employee-directory.provider.ts (1)
EmployeeDirectoryProvider(5-57)
apps/demo/src/apps/employee-time/tools/admin-add-entry.tool.ts (5)
apps/demo/src/apps/employee-time/tools/admin-add-exit.tool.ts (1)
Tool(8-50)apps/demo/src/apps/employee-time/providers/redis.provider.ts (2)
EmployeeRedisProvider(11-88)exists(60-62)apps/demo/src/apps/employee-time/providers/employee-directory.provider.ts (1)
EmployeeDirectoryProvider(5-57)apps/demo/src/apps/employee-time/utils/time.ts (1)
toMs(1-7)apps/demo/src/apps/employee-time/utils/keys.ts (1)
openKey(14-16)
apps/demo/src/apps/calculator/tools/exp.tool.ts (2)
apps/demo/src/apps/employee-time/tools/register-entry.tool.ts (1)
Tool(9-33)apps/demo/src/apps/employee-time/tools/register-exit.tool.ts (1)
Tool(8-45)
apps/demo/src/apps/calculator/tools/modulo.tool.ts (4)
apps/demo/src/apps/calculator/tools/add.tool.ts (1)
Tool(4-16)apps/demo/src/apps/calculator/tools/divide.tool.ts (1)
Tool(4-16)apps/demo/src/apps/calculator/tools/multiply.tool.ts (1)
Tool(4-16)apps/demo/src/apps/calculator/tools/pow.tool.ts (1)
Tool(4-17)
apps/demo/src/apps/calculator/tools/round.tool.ts (1)
apps/demo/src/apps/calculator/tools/abs.tool.ts (1)
Tool(4-16)
apps/demo/src/apps/employee-time/tools/report-hours.tool.ts (4)
apps/demo/src/apps/employee-time/providers/redis.provider.ts (1)
EmployeeRedisProvider(11-88)apps/demo/src/apps/employee-time/providers/employee-directory.provider.ts (1)
EmployeeDirectoryProvider(5-57)apps/demo/src/apps/employee-time/utils/time.ts (8)
toMs(1-7)startOfDayMs(19-23)endOfDayMs(25-29)startOfWeekMs(31-38)endOfWeekMs(40-46)startOfMonthMs(48-53)endOfMonthMs(55-60)eachDayRange(62-73)apps/demo/src/apps/employee-time/utils/hours.ts (2)
sumEmployeeDaysMsForSite(47-57)sumEmployeesRangeMsForSite(59-66)
apps/demo/src/apps/employee-time/tools/get-employee-details.tool.ts (6)
apps/demo/src/apps/employee-time/tools/register-entry.tool.ts (1)
Tool(9-33)apps/demo/src/apps/employee-time/tools/register-exit.tool.ts (1)
Tool(8-45)apps/demo/src/apps/employee-time/providers/redis.provider.ts (1)
EmployeeRedisProvider(11-88)apps/demo/src/apps/employee-time/providers/employee-directory.provider.ts (1)
EmployeeDirectoryProvider(5-57)apps/demo/src/apps/employee-time/utils/time.ts (6)
formatDay(11-17)startOfWeekMs(31-38)endOfWeekMs(40-46)startOfMonthMs(48-53)endOfMonthMs(55-60)eachDayRange(62-73)apps/demo/src/apps/employee-time/utils/hours.ts (1)
getEmployeeDayMsForSite(41-45)
libs/sdk/src/auth/auth.registry.ts (3)
libs/sdk/src/scope/scope.instance.ts (1)
providers(132-134)libs/sdk/src/auth/instances/instance.remote-primary-auth.ts (1)
RemotePrimaryAuth(12-59)libs/sdk/src/auth/instances/instance.local-primary-auth.ts (1)
LocalPrimaryAuth(18-93)
apps/demo/src/apps/employee-time/providers/employee-directory.provider.ts (2)
apps/demo/src/apps/employee-time/providers/redis.provider.ts (2)
EmployeeRedisProvider(11-88)set(29-35)apps/demo/src/apps/employee-time/utils/keys.ts (3)
SITES_SET(3-3)siteEmployeesSetKey(6-8)employeeSitesSetKey(10-12)
apps/demo/src/apps/calculator/tools/subtract.tool.ts (1)
apps/demo/src/apps/calculator/tools/add.tool.ts (1)
Tool(4-16)
apps/demo/src/apps/employee-time/tools/register-entry.tool.ts (5)
apps/demo/src/apps/employee-time/tools/admin-add-entry.tool.ts (1)
Tool(8-37)apps/demo/src/apps/employee-time/tools/register-exit.tool.ts (1)
Tool(8-45)apps/demo/src/apps/employee-time/providers/redis.provider.ts (2)
EmployeeRedisProvider(11-88)exists(60-62)apps/demo/src/apps/employee-time/providers/employee-directory.provider.ts (1)
EmployeeDirectoryProvider(5-57)apps/demo/src/apps/employee-time/utils/keys.ts (1)
openKey(14-16)
apps/demo/src/apps/employee-time/tools/admin-add-exit.tool.ts (6)
apps/demo/src/apps/employee-time/tools/admin-add-entry.tool.ts (1)
Tool(8-37)apps/demo/src/apps/employee-time/tools/register-exit.tool.ts (1)
Tool(8-45)apps/demo/src/apps/employee-time/providers/redis.provider.ts (1)
EmployeeRedisProvider(11-88)apps/demo/src/apps/employee-time/providers/employee-directory.provider.ts (1)
EmployeeDirectoryProvider(5-57)apps/demo/src/apps/employee-time/utils/time.ts (2)
toMs(1-7)splitSessionByDay(77-94)apps/demo/src/apps/employee-time/utils/keys.ts (3)
openKey(14-16)hoursKey(18-20)sessionsKey(22-24)
apps/demo/src/apps/employee-time/tools/register-exit.tool.ts (4)
apps/demo/src/apps/employee-time/providers/redis.provider.ts (1)
EmployeeRedisProvider(11-88)apps/demo/src/apps/employee-time/providers/employee-directory.provider.ts (1)
EmployeeDirectoryProvider(5-57)apps/demo/src/apps/employee-time/utils/keys.ts (3)
openKey(14-16)hoursKey(18-20)sessionsKey(22-24)apps/demo/src/apps/employee-time/utils/time.ts (1)
splitSessionByDay(77-94)
apps/demo/src/apps/employee-time/utils/hours.ts (3)
apps/demo/src/apps/employee-time/providers/redis.provider.ts (1)
EmployeeRedisProvider(11-88)apps/demo/src/apps/employee-time/utils/time.ts (1)
eachDayRange(62-73)apps/demo/src/apps/employee-time/utils/keys.ts (1)
hoursKey(18-20)
apps/demo/src/apps/calculator/tools/floor.tool.ts (2)
apps/demo/src/apps/calculator/tools/abs.tool.ts (1)
Tool(4-16)apps/demo/src/apps/calculator/tools/ceil.tool.ts (1)
Tool(4-16)
apps/demo/src/apps/calculator/tools/min.tool.ts (1)
apps/demo/src/apps/calculator/tools/max.tool.ts (1)
Tool(4-16)
apps/demo/src/apps/calculator/tools/divide.tool.ts (2)
apps/demo/src/apps/calculator/tools/add.tool.ts (1)
Tool(4-16)apps/demo/src/apps/calculator/tools/modulo.tool.ts (1)
Tool(4-16)
apps/demo/src/apps/calculator/tools/add.tool.ts (3)
apps/demo/src/apps/calculator/tools/divide.tool.ts (1)
Tool(4-16)apps/demo/src/apps/calculator/tools/multiply.tool.ts (1)
Tool(4-16)apps/demo/src/apps/calculator/tools/subtract.tool.ts (1)
Tool(4-16)
apps/demo/src/apps/calculator/index.ts (1)
apps/demo/src/apps/expenses/index.ts (1)
App(11-44)
libs/sdk/src/scope/scope.registry.ts (1)
libs/sdk/src/scope/scope.instance.ts (1)
Scope(26-160)
libs/sdk/src/auth/instances/instance.remote-primary-auth.ts (1)
libs/sdk/src/scope/scope.instance.ts (1)
providers(132-134)
apps/demo/src/apps/calculator/tools/abs.tool.ts (4)
apps/demo/src/apps/calculator/tools/add.tool.ts (1)
Tool(4-16)apps/demo/src/apps/calculator/tools/ceil.tool.ts (1)
Tool(4-16)apps/demo/src/apps/calculator/tools/round.tool.ts (1)
Tool(4-23)apps/demo/src/apps/calculator/tools/sqrt.tool.ts (1)
Tool(4-16)
apps/demo/src/apps/calculator/tools/sqrt.tool.ts (2)
apps/demo/src/apps/calculator/tools/abs.tool.ts (1)
Tool(4-16)apps/demo/src/apps/calculator/tools/divide.tool.ts (1)
Tool(4-16)
🔇 Additional comments (49)
apps/demo/src/apps/expenses/index.ts (1)
37-41: LGTM! Per-app auth configuration properly structured.The per-app auth configuration is correctly structured and aligns with the PR's objective to support split-by-app with multiple auth options.
apps/demo/src/apps/calculator/tools/exp.tool.ts (1)
1-16: LGTM! ExpTool implementation is correct.The tool correctly implements the exponential function using
Math.expand follows the same pattern as other calculator tools.apps/demo/src/apps/calculator/tools/multiply.tool.ts (1)
1-16: LGTM! MultiplyTool implementation is correct.The multiplication logic is correct and follows the established pattern for calculator tools.
apps/demo/src/apps/calculator/tools/ceil.tool.ts (1)
1-16: LGTM! CeilTool implementation is correct.The tool correctly implements the ceiling function and the description accurately describes the mathematical operation.
apps/demo/src/apps/calculator/tools/round.tool.ts (1)
1-23: LGTM! RoundTool implementation handles both integer and decimal rounding.The implementation correctly handles:
- Simple integer rounding when
digitsis undefined- Decimal place rounding with proper floating-point precision handling using
Number.EPSILONThe constraints on
digits(0-15) are appropriate for JavaScript number precision limits.apps/demo/src/apps/employee-time/plugins/site-authorization.plugin.ts (5)
4-12: LGTM! Global augmentation properly extends tool metadata.The global augmentation correctly adds site-specific metadata fields for per-tool authorization configuration.
14-17: LGTM! Options interface is well-defined.The configuration options are appropriate for the plugin's authorization requirements, with sensible defaults indicated.
19-26: LGTM! Plugin decorator and constructor properly configured.The plugin is correctly decorated and the constructor provides sensible defaults for demo usage.
28-35: LGTM! getAllowedSites handles demo mode and various auth structures.The method appropriately handles:
- Multiple auth structure variations (sites vs tenants)
- Demo mode for testing without claims
- Type consistency by converting to strings
45-74: Security consideration: Input-driven admin requirement escalation.On line 55, the code allows
input?.admin === trueto trigger admin requirement checking. This means a tool caller could potentially escalate a request to require admin privileges by passingadmin: truein the input. While this might be intentional for certain use cases, it could also bypass intended access controls if not carefully managed.Consider whether allowing input-driven admin requirement escalation is the intended behavior. If admin requirements should only be determined by tool metadata, apply this diff:
- const adminRequired = meta.site?.adminRequired || input?.admin === true; + const adminRequired = meta.site?.adminRequired || false;If the input-driven escalation is intentional, consider adding documentation or a code comment explaining this design choice and its security implications.
apps/demo/src/apps/employee-time/utils/time.ts (7)
1-7: LGTM! toMs function handles all input types correctly.The function appropriately handles number, Date, and string inputs with proper error handling for invalid date strings.
9-17: LGTM! Date formatting functions are correct.The functions correctly pad numbers and format dates in YYYY-MM-DD format, properly handling JavaScript's 0-indexed months.
19-29: LGTM! Day boundary functions are correct.The functions correctly calculate start and end of day boundaries in local timezone.
31-46: LGTM! Week boundary functions correctly calculate Monday-Sunday weeks.The functions properly handle the week start as Monday and calculate the correct offsets for all days of the week.
48-60: LGTM! Month boundary functions are correct.The functions correctly calculate month boundaries, with
endOfMonthMsusing the standard technique of setting day 0 of the next month to get the last day of the current month.
62-73: LGTM! eachDayRange correctly generates inclusive day range.The function properly normalizes inputs to day boundaries and generates an inclusive list of days.
77-94: LGTM! splitSessionByDay correctly segments across day boundaries.The function properly:
- Validates input constraints
- Splits sessions that span multiple days
- Calculates per-day durations accurately
- Handles edge cases where session ends mid-day
libs/sdk/src/common/entries/scope.entry.ts (2)
12-12: LGTM! Imports path normalization utilities.The import correctly brings in the required normalization functions for the new
fullPathgetter.
20-24: LGTM! fullPath getter provides consistent path resolution.The getter correctly:
- Handles undefined/null values with safe fallbacks
- Normalizes both path components before combining
- Provides a computed full path for scope-based routing
libs/sdk/src/common/records/scope.record.ts (1)
1-1: LGTM! Token-based provisioning for split-by-app scopes.The type change from
Type<ScopeEntry>toTokenforSplitByAppScope.providealigns with the architectural shift to token-based provisioning for per-app scopes, as seen inscope.registry.tswhereSPLIT_BY_APPscopes are now instantiated directly vianew Scope()rather than through a provider factory.Also applies to: 12-12
libs/sdk/src/transport/adapters/transport.sse.adapter.ts (1)
17-18: LGTM! Scope-aware transport path construction.The dynamic path construction using
this.scope.fullPathproperly implements per-app routing for SSE transports, enabling the split-by-app architecture.libs/sdk/src/scope/scope.registry.ts (1)
98-98: LGTM! Direct instantiation for split-by-app scopes.The change from factory-based instantiation to direct
new Scope(rec, this.providers)forSPLIT_BY_APPscopes aligns with the token-based provisioning architecture introduced in this PR. TheMULTI_APPpath correctly maintains the factory pattern.apps/demo/src/apps/calculator/tools/floor.tool.ts (1)
1-16: LGTM! Clean implementation.The
FloorToolimplementation is correct and follows the same pattern as other calculator tools. The metadata accurately describes the operation, and the execution logic properly usesMath.floor.apps/demo/src/apps/calculator/tools/abs.tool.ts (1)
1-16: LGTM! Clean implementation.The
AbsToolimplementation is correct and follows the established pattern. The metadata accurately describes the operation, and the execution logic properly usesMath.abs.apps/demo/src/apps/calculator/index.ts (1)
19-50: LGTM! Clean app configuration.The Calculator MCP app configuration is well-structured with 14 mathematical tools and Redis-based caching. The empty
providersarray is intentional as the calculator doesn't require custom providers, unlike the employee-time app.libs/sdk/src/auth/instances/instance.remote-primary-auth.ts (1)
16-16: All instantiation sites have been correctly updated with the scope parameter.The search found one instantiation of
RemotePrimaryAuthinlibs/sdk/src/auth/auth.registry.ts:25. The site correctly passes thescopeparameter as the first argument:new RemotePrimaryAuth(scope, providers, primary), which aligns with the updated constructor signature. The breaking change has been properly addressed.apps/demo/src/apps/employee-time/index.ts (1)
24-27: Verify Redis configuration duplication - intentional design, but optimization opportunity.The Redis configuration is duplicated by design.
EmployeeRedisProvidermanages business logic data (employee hours, sessions, site membership) whileCachePluginprovides framework-level response caching for tools. Both appropriately connect tolocalhost:6379.However, the current implementation creates two separate Redis client connections. The
EmployeeRedisProviderexposes only privateclientfield, preventing reuse viaCachePlugin.init({ type: 'redis-client', client: ... }). This duplication is intentional but suboptimal; consider:
- Option A (minor refactor): Expose the ioredis client from
EmployeeRedisProvider(e.g.,getClient()method) and configureCachePluginto reuse it viatype: 'redis-client'to avoid duplicate connections.- Option B (current state): Keep as-is if operational overhead is acceptable and separation of concerns is preferred.
The configuration is not redundant since each serves a distinct purpose; this is a resource-efficiency consideration rather than a correctness issue.
apps/demo/src/apps/calculator/tools/modulo.tool.ts (1)
4-15: Modulo tool is consistent and correctly guarded against zero divisorThe tool metadata, non-zero check on
b, and use of JavaScript%align with the description and mirror the patterns in the other calculator tools. Return shape and types look correct.apps/demo/src/apps/calculator/tools/min.tool.ts (1)
4-15: MinTool implementation is correct and matches the calculator tool conventionsNon-empty array validation plus
Math.min(...values)gives the expected behavior, and the metadata/shape are consistent withMaxTooland the rest of the calculator tools. Looks good as-is for typical usage.libs/sdk/src/scope/scope.utils.ts (2)
33-44: Confirm intent around per-app token, scopeId, and auth overrideThe new
scopeId+Symbol(scopeId)token and settingprovideto that token look like the right direction for per-app SPLIT_BY_APP scoping, and wiringmetadata.idtoscopeIdkeeps things coherent. OverridingauthwithappMetadata.authwill effectively ignore anyauthdefined on the split-by-app metadata itself and make auth purely per-app for this scope type.If that’s the intended contract (SPLIT_BY_APP scopes always use the app’s own auth and a dedicated token), this implementation looks consistent; otherwise, you may want to merge or fall back to the metadata-level
auth.
62-68: Double-check that SPLIT_BY_APP having no discovery deps is acceptable for your graph/cycle detectionChanging
scopeDiscoveryDepsso thatScopeKind.SPLIT_BY_APPreturns an empty array means these scopes are no longer participating in whatever discovery/cycle graph is driven bydepsOfClass(..., 'discovery'). If SPLIT_BY_APP scopes truly have no discovery-time dependencies (or are intended to be leaf nodes), this is fine; otherwise, you might lose some cycle detection or wiring for them.Might be worth quickly reviewing the call sites of
scopeDiscoveryDepsto ensure this change matches expectations.apps/demo/src/apps/calculator/tools/sqrt.tool.ts (1)
4-15: SqrtTool validation and behavior look correctNon-negative validation on
xplusMath.sqrtgives the expected domain and result, and the tool metadata/return shape are aligned with the rest of the calculator toolset.apps/demo/src/apps/calculator/tools/max.tool.ts (1)
4-15: MaxTool mirrors MinTool and is implemented correctlyThe non-empty array check plus
Math.max(...values)is correct and consistent withMinTool, and the tool metadata and result shape match the rest of the calculator suite.apps/demo/src/apps/calculator/tools/add.tool.ts (1)
1-16: LGTM!The addition tool is correctly implemented with appropriate Zod schema validation for inputs and output.
apps/demo/src/apps/employee-time/tools/register-entry.tool.ts (1)
16-32: LGTM!The session registration logic correctly:
- Validates for duplicate open sessions
- Records the start time in Redis
- Updates the employee directory
The implementation follows the established pattern seen in
AdminAddEntryTool.apps/demo/src/apps/calculator/tools/divide.tool.ts (1)
1-16: LGTM!The division tool correctly validates against division by zero using Zod refinement, and the implementation is straightforward and correct.
apps/demo/src/apps/calculator/tools/pow.tool.ts (1)
1-17: LGTM!The power tool correctly uses
Math.pow()for exponentiation. The implementation handles edge cases appropriately through JavaScript's native behavior.apps/demo/src/apps/calculator/tools/subtract.tool.ts (1)
1-16: LGTM!The subtraction tool is correctly implemented with appropriate schema validation.
libs/sdk/src/scope/scope.instance.ts (2)
53-57: LGTM!The conditional
routeBaseinitialization correctly implements per-app route prefixing. WhensplitByAppis enabled, each app gets its own route namespace (e.g.,/calculator).
74-78: LGTM!The updated
AuthRegistryinstantiation now passes the scope instance as the first parameter, enabling scope-aware authentication. This change aligns with the refactoredAuthRegistryconstructor signature shown in the relevant snippets.libs/sdk/src/auth/instances/instance.local-primary-auth.ts (1)
27-32: LGTM!The constructor signature update and issuer computation correctly implement scope-aware authentication. The issuer now includes
scope.fullPath, enabling per-app JWT claims whensplitByAppis enabled.apps/demo/src/main.ts (2)
3-4: LGTM!The new demo apps (
CalculatorMcpAppandEmployeeTimeMcpApp) are correctly imported to demonstrate the per-app splitting feature.
9-11: Remote auth configuration removal is intentional and properly documented.The removal aligns with the architectural requirement documented in README.md: "With
splitByApp: true, define auth per@App(server-levelauthis disallowed)."All three apps are correctly configured:
- ExpenseMcpApp: retains remote auth at app level
- CalculatorMcpApp & EmployeeTimeMcpApp: omit auth (public access; EmployeeTime uses plugin-based auth via SiteAuthorizationPlugin)
Migration notes are documented in the SDK README. No dependent functionality is broken.
apps/demo/src/apps/employee-time/utils/keys.ts (1)
3-24: LGTM – centralized key builders are clear and consistentKey naming is consistent and matches the usages in
EmployeeDirectoryProviderand the tools. Centralizing these Redis keys here should make future schema changes straightforward.libs/sdk/src/auth/auth.registry.ts (1)
10-48: Constructor call sites verified—scope-aware primary auth wiring is correctVerification confirms all active
AuthRegistryinstantiations have been properly updated:
- scope.instance.ts:74 correctly calls
new AuthRegistry(this, this.scopeProviders, [], {...}, this.metadata.auth)withscopeas the first argument, followed byproviders,metadata,owner, andprimaryoptions.- app.local.instance.ts:33 is commented out and not active.
The implementation properly aligns the new signature with existing call sites.
apps/demo/src/apps/employee-time/tools/register-exit.tool.ts (1)
15-43: Exit flow and per-day aggregation look correctThe end-of-session flow is well structured:
- You validate that an open session exists and that
start/endare sane before doing any writes.splitSessionByDaysegments multi-day sessions correctly, and you accumulatedurationMswhile updating both the per-day hours counter and session log keys.- The open-session key is only deleted after all updates complete.
This matches the key patterns and time helpers in the utils and should behave correctly for same-day and multi-day sessions.
apps/demo/src/apps/employee-time/utils/hours.ts (3)
5-7: LGTM: Legacy key generators for backward compatibility.The legacy key functions are simple and serve their purpose for backward compatibility.
13-17: LGTM: Good defensive programming with safe numeric parsing.The finite check and fallback to 0 properly handle edge cases like NaN or non-numeric values in Redis.
19-29: LGTM: Efficient batch fetching with mget.Good use of
mgetto fetch all day keys in a single Redis round-trip, and proper handling of the empty array case.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
apps/demo/src/apps/employee-time/plugins/site-authorization.plugin.ts (2)
37-43: LGTM! Admin check logic is sound.The implementation correctly handles the missing user case with the double-negation operator, and the role-based admin detection covers common admin role names.
Note: The undefined
demoAllowAllIfNoClaimsconcern is addressed in the constructor comment above.
24-26: Normalize plugin options to ensure consistent defaults.The default parameter value only applies when no argument is passed to the constructor. If a partial options object is passed (e.g.,
{}or{ siteIdFieldName: 'custom' }), thendemoAllowAllIfNoClaimswill beundefinedrather thantrue, leading to "deny by default" behavior instead of the intended "allow by default" demo mode.Apply this diff to normalize options in the constructor body:
constructor(private readonly opts: SiteAuthorizationPluginOptions = { demoAllowAllIfNoClaims: true, siteIdFieldName: 'siteId' }) { super(); + this.opts.demoAllowAllIfNoClaims = this.opts.demoAllowAllIfNoClaims ?? true; + this.opts.siteIdFieldName = this.opts.siteIdFieldName ?? 'siteId'; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/demo/src/apps/employee-time/plugins/site-authorization.plugin.ts(1 hunks)
🔇 Additional comments (2)
apps/demo/src/apps/employee-time/plugins/site-authorization.plugin.ts (2)
14-17: LGTM!The interface definition is clear and appropriate for the plugin's configuration needs.
45-74: This review comment is incorrect and should be disregarded.The concern misinterprets the code's design. The
adminfield on line 55 is not a user injection vulnerability; it's an explicit part of the tool input schema (as seen inadmin-add-exit.tool.tsandadmin-add-entry.tool.ts, which both defineadmin: booleanas a formal parameter).The logic enables dual-mode tools: either admin is required by metadata (
meta.site?.adminRequired), or clients can request admin-level execution by settinginput.admin = true. The actual security enforcement occurs at lines 71–73, where the code validates that the user actually possesses admin privileges viathis.isAdmin(ctx.authInfo). A non-admin user settingadmin: truewill correctly fail this check.The suggested diff would break the intended feature of allowing clients to opt into admin-mode execution while still enforcing server-side privilege validation.
Likely an incorrect or invalid review comment.
apps/demo/src/apps/employee-time/plugins/site-authorization.plugin.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/demo/src/apps/employee-time/plugins/site-authorization.plugin.ts (1)
30-52: Consider improving type safety.The methods use
anytypes forauthInfo,user, androles, which reduces type safety and IntelliSense support. Consider defining interfaces for these types.Example refactor:
interface AuthUser { sites?: string[] | string; tenants?: string[] | string; isAdmin?: boolean; roles?: string[]; } interface AuthInfo { user?: AuthUser; } private getAllowedSites(authInfo: AuthInfo): string[] | 'ALL' { const user = authInfo?.user; // ... rest of implementation } private isAdmin(authInfo: AuthInfo): boolean { const user = authInfo?.user; // ... rest of implementation }Note: Line 43 has an unnecessary blank line before the closing brace.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/demo/src/apps/employee-time/plugins/site-authorization.plugin.ts(1 hunks)
🔇 Additional comments (3)
apps/demo/src/apps/employee-time/plugins/site-authorization.plugin.ts (3)
1-1: LGTM!All imports are properly used throughout the file.
3-11: LGTM!The global interface augmentation correctly extends
ExtendFrontMcpToolMetadatawith site-scoped metadata.
13-16: LGTM!Interface definition is clear and matches usage throughout the plugin.
apps/demo/src/apps/employee-time/plugins/site-authorization.plugin.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
docs/getting-started/quickstart.mdx (1)
46-58: Consider adding explicitoutputSchemafor consistency with SDK patterns.The
GreetToolexample works as written, but adding anoutputSchemato the@Tooldecorator would better document the expected return type and align with the patterns shown in other documentation (e.g.,docs/getting-started/welcome.mdx). Since the method returns a string, consider:@Tool({ name: 'greet', description: 'Greets a user by name', inputSchema: { name: z.string() }, + outputSchema: z.string(), })This addition is optional for this quickstart but would reinforce best practices to new users.
apps/demo/src/apps/employee-time/plugins/site-authorization.plugin.ts (1)
67-67: Consider: Metadata cannot explicitly disable site-scoping when siteId is present.The logic
meta.site?.siteScoped || (siteId !== undefined)means that if asiteIdfield exists in the input, the tool will be treated as site-scoped even ifmeta.site?.siteScopedis explicitly set tofalse. This prevents metadata from disabling site authorization when the field is present.This may be intentional for auto-detection convenience, but it could lead to unexpected behavior if a tool has a field named
siteId(or the configured field name) that isn't meant for authorization.Consider whether metadata should be the sole source of truth:
- const siteScoped = meta.site?.siteScoped || (siteId !== undefined); + const siteScoped = meta.site?.siteScoped === true;Or make the auto-detection opt-in via configuration if this flexibility is desired.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.coderabbit.yaml(1 hunks)apps/demo/src/apps/employee-time/plugins/site-authorization.plugin.ts(1 hunks)docs/getting-started/quickstart.mdx(1 hunks)docs/getting-started/welcome.mdx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
apps/demo/**
⚙️ CodeRabbit configuration file
apps/demo directory contains demo application to testing purposes, can be used for sdk usages examples
Files:
apps/demo/src/apps/employee-time/plugins/site-authorization.plugin.ts
docs/**
⚙️ CodeRabbit configuration file
Contains documentation for sdk, using mdx format and hosted by mintlify
Files:
docs/getting-started/welcome.mdxdocs/getting-started/quickstart.mdx
🔇 Additional comments (8)
docs/getting-started/welcome.mdx (1)
11-26: Good example of the new class-based tool pattern.The documentation clearly demonstrates the
@Tooldecorator usage with explicitToolContextextension, and the return type correctly matches the declaredoutputSchema. This serves as a solid reference for implementing tools using the new patterns introduced in this PR..coderabbit.yaml (1)
5-9: Good addition of path-specific review guidance.The new
path_instructionsentries provide helpful context for reviewers working on the demo app and documentation. This aligns well with the PR's introduction of demo MCP apps and documentation updates. The instructions are clear and match the actual purpose of these directories.apps/demo/src/apps/employee-time/plugins/site-authorization.plugin.ts (6)
1-11: LGTM!The imports are clean and the global type augmentation correctly extends the tool metadata interface to support site-scoped authorization.
13-16: LGTM!The interface is well-defined with clear optional properties and inline documentation of the default value.
18-32: LGTM!The constructor now correctly merges partial options with defaults, ensuring that
siteIdFieldNameanddemoAllowAllIfNoClaimsalways have defined values even when only partial options are provided.
34-48: LGTM!The method now properly handles the edge case where
sitesis a single string value (lines 43-44), converting it to a single-element array. The array and empty-array cases are also handled correctly.
50-56: LGTM!The method now consistently returns a boolean value using
!!this.opts.demoAllowAllIfNoClaims(line 52), addressing the previous concern about inconsistent fallback behavior.
68-87: LGTM!The
adminRequiredcheck (line 68) now correctly derives the requirement solely from metadata, addressing the previous security concern about allowing user input to influence authorization requirements. The subsequent validation logic (lines 76-87) is clear and correct.
Summary by CodeRabbit
New Features
Improvements