feat: Add ConfigPlugin for environment variable management and update related configurations #204
Conversation
… related configurations
📝 WalkthroughWalkthroughAdds a typed ConfigPlugin with env/.env.local and optional YAML loading, a ConfigService with nested access and validation, env/YAML loaders and utilities, a context-aware ConfigResolver with three-level fallbacks, CLI env loading, adapter wiring to propagate entity context for config resolution, a compatibility plugin package, extensive tests, docs, and an E2E demo app. Changes
Sequence Diagram(s)sequenceDiagram
participant Tool as Tool (GetConfig / TestConfigFallback)
participant ConfigSvc as ConfigService
participant EnvLoader as Env/YAML Loader
participant ProcessEnv as process.env
Tool->>ConfigSvc: request value (path / key / fallbacks)
alt Schema-backed loader present
ConfigSvc->>EnvLoader: load YAML (opt) & loadEnvFiles()
EnvLoader->>ProcessEnv: read process.env + .env/.env.local
EnvLoader-->>ConfigSvc: merged flat env -> nested mapping
ConfigSvc->>ConfigSvc: validate against Zod schema
ConfigSvc-->>Tool: typed value or throw ConfigValidationError
else Legacy env-only mode
ConfigSvc->>ProcessEnv: read process.env (populated)
ConfigSvc-->>Tool: value or undefined
end
sequenceDiagram
participant Agent as Agent instance
participant DI as Provider registry
participant AdapterFactory as createAdapter
participant Resolver as ConfigResolver
participant ConfigSvc as ConfigService
Agent->>DI: resolve(ConfigService)
alt ConfigService available
DI-->>Agent: ConfigService
Agent->>AdapterFactory: createAdapter(config, { entityContext })
AdapterFactory->>Resolver: createContextResolver(entityContext)
AdapterFactory->>Resolver: resolveWithFallbacks(path)
Resolver->>ConfigSvc: tryGet(generated paths)
ConfigSvc-->>Resolver: value or undefined
Resolver-->>AdapterFactory: resolved value or error (lists tried paths)
else ConfigService absent
DI-->>Agent: undefined
Agent->>AdapterFactory: createAdapter(config) (withConfig resolution disabled)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (4)
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Fix all issues with AI agents
In @apps/e2e/demo-e2e-config/src/apps/config/tools/check-config.tool.ts:
- Around line 29-41: The isBoolean field in execute() is misleading because it
uses exists rather than validating the value; update the logic in execute
(check-config.tool.ts) to either rename isBoolean to existsForBoolean and adjust
returned payload and comment, or implement real boolean validation by inspecting
the raw config string (use config.get or similar) and accept
"true"/"false"/"1"/"0" (case-insensitive) before setting isBoolean, and ensure
booleanValue is only provided when that validation passes; update the comment
near getBoolean/getNumber to reflect the chosen behavior.
In @apps/e2e/demo-e2e-config/webpack.config.js:
- Around line 4-12: The current code unconditionally treats config.externals as
an array and calls .push(), which will throw if externals is an
object/function/string (all valid webpack types); update the composePlugins
callback to safely normalize/merge: compute a small externalsEntry = {
bufferutil: 'commonjs bufferutil', 'utf-8-validate': 'commonjs utf-8-validate'
}, then inspect config.externals: if Array.isArray(config.externals) push
externalsEntry only if not already present; else if config.externals is a plain
object (typeof === 'object' && config.externals !== null) create a new object =
{ ...config.externals, ...externalsEntry } (avoid mutating original) and assign
it; otherwise (function/string/undefined/other) set config.externals =
[config.externals, externalsEntry]. Make this change inside the same
composePlugins(...) callback that uses withNx() so the merge is safe and
idempotent.
In @docs/live/docs/extensibility/config.mdx:
- Around line 198-204: The example declares the same constant twice causing a
syntax error; change the first declaration to a distinct name (e.g., portOrNaN)
so both lines can coexist and still demonstrate this.config.getNumber behavior;
update the first occurrence of the variable name used with
this.config.getNumber('PORT') to a different identifier while leaving the second
const port = this.config.getNumber('PORT', 3000) unchanged.
In @libs/sdk/package.json:
- Line 79: Update the js-yaml dependency in libs/sdk/package.json from "^4.1.0"
to "^4.1.1" (or newer) to address CVE-2025-64718; after changing the "js-yaml"
entry, regenerate the lockfile (npm install / yarn install) and run CI/tests to
ensure no breakages.
In @libs/sdk/src/builtin/config/config-resolver.ts:
- Around line 152-169: Update the docstring for generateEnvFallbacks to match
the actual output of normalizeNameForEnv: the example should show keys without
an extra underscore between "OPENAI" and "KEY" because
normalizeNameForEnv('openaiKey') yields "OPENAIKEY"; change the example return
to ['AGENTS_RESEARCH_AGENT_OPENAIKEY', 'AGENTS_OPENAIKEY', 'OPENAIKEY'] and
mention generateEnvFallbacks and normalizeNameForEnv to help locate the logic.
In @libs/sdk/src/builtin/config/providers/config.service.ts:
- Around line 8-30: Add comprehensive constructor tests for
ConfigValidationError: construct a real z.ZodError (use zod to create a failing
parse or build a ZodError with sample issues) and instantiate
ConfigValidationError with a message and that ZodError, then assert the instance
is instanceof ConfigValidationError and instanceof Error, that its name ===
'ConfigValidationError', that the zodError property === the original ZodError,
and that the error.message contains the formatted issues (each issue path and
message) to validate the formatting logic in the constructor.
In @libs/sdk/src/builtin/config/providers/env-loader.ts:
- Around line 110-116: The populateProcessEnv function is duplicated (also
present in parseEnvContent refactor) — consolidate by extracting a single
exported helper (e.g., populateProcessEnv) into a shared utility module and
import it from both places; update the implementations in
libs/sdk/src/builtin/config/providers/env-loader.ts and
libs/cli/src/utils/env.ts to remove the local duplicate and call the shared
function, preserving the signature (env: Record<string,string>, override?:
boolean) and behavior (only set when override true or process.env[key]
undefined).
- Around line 17-60: The parseEnvContent implementation is duplicated; keep the
canonical parseEnvContent in the SDK and remove the duplicate in the CLI,
updating the CLI to import/re-export parseEnvContent from the SDK instead of
redefining it; also replace the RegExp-based placeholder restoration
(.replace(new RegExp(PLACEHOLDER, 'g'), '\\')) with a safer string operation
like .replaceAll(PLACEHOLDER, '\\') (or split/join) to avoid the static ReDoS
warning while preserving the existing escape-sequence handling and use of the
PLACEHOLDER constant.
In @libs/sdk/src/common/interfaces/execution-context.interface.ts:
- Around line 193-207: The getter for ConfigService uses an unnecessary double
cast; update the ConfigService getter in execution-context.interface.ts to
remove "as unknown as Token<ConfigService>" and call this.providers.get with the
ConfigService token directly (i.e., use providers.get(ConfigService)) so the
return type remains ConfigService and matches usage elsewhere like
agent.instance.ts.
In @plugins/plugin-config/src/__tests__/config.plugin.test.ts:
- Around line 81-84: The test for ConfigValidationError is missing an instanceof
check to ensure it inherits from Error; update the test case in
config.plugin.test.ts for the ConfigValidationError spec to create an instance
(new ConfigValidationError(...)) and assert it is an instance of Error and of
ConfigValidationError (use expect(err instanceof Error).toBeTruthy() or
expect(err).toBeInstanceOf(Error) and
expect(err).toBeInstanceOf(ConfigValidationError)), keeping the existing checks
that ConfigValidationError is defined and a function.
In @plugins/plugin-config/src/__tests__/config.service.test.ts:
- Around line 214-220: The test only asserts export of ConfigValidationError;
add comprehensive constructor and instanceof checks by importing z from 'zod',
creating a ZodError (e.g., via z.string().parse with try/catch or using
z.ZodError directly), passing that ZodError into a new ConfigValidationError,
then assert the instance is instanceof ConfigValidationError and instanceof
Error, that it exposes the original ZodError (e.g., via a .cause or .zodError
property used by the class), and that the error message includes expected text;
reference ConfigValidationError and ZodError/z to locate where to change the
test.
In @plugins/plugin-config/src/__tests__/env-loader.test.ts:
- Around line 105-113: Add integration tests for loadEnvFiles: write async Jest
tests that call loadEnvFiles and assert it resolves (await
expect(loadEnvFiles(...)).resolves.toBeUndefined()), create temporary .env files
in a temp dir to verify file loading behavior (no files yields no changes,
presence of .env and .env.local shows .env.local overrides .env), test custom
path parameter by passing the temp dir path to loadEnvFiles, and clean up temp
files after each test; use the loadEnvFiles symbol to locate the re-export and
ensure tests cover async resolution, file precedence, and custom path handling.
🧹 Nitpick comments (17)
apps/e2e/demo-e2e-config/webpack.config.js (1)
7-11: Add a short rationale comment for these externals.A quick note like “exclude optional native ws deps from bundling” will make it clearer why these specific modules are being externalized.
apps/e2e/demo-e2e-config/project.json (1)
36-52: Redundant test targets with nearly identical configuration.Both
testandtest:e2etargets use the same Jest configuration file (jest.e2e.config.ts), differing only in therunInBandoption. This duplication may cause confusion about which target to use and could lead to inconsistent test execution.Consider consolidating to a single
testtarget withrunInBand: truesince this is an E2E-focused project, or differentiate the targets more clearly if they serve distinct purposes.♻️ Proposed consolidation
- "test": { - "executor": "@nx/jest:jest", - "outputs": ["{workspaceRoot}/coverage/apps/e2e/demo-e2e-config"], - "options": { - "jestConfig": "apps/e2e/demo-e2e-config/jest.e2e.config.ts", - "passWithNoTests": true - } - }, - "test:e2e": { + "test": { "executor": "@nx/jest:jest", - "outputs": ["{workspaceRoot}/coverage/apps/e2e/demo-e2e-config-e2e"], + "outputs": ["{workspaceRoot}/coverage/apps/e2e/demo-e2e-config"], "options": { "jestConfig": "apps/e2e/demo-e2e-config/jest.e2e.config.ts", "runInBand": true, "passWithNoTests": true } }plugins/plugin-config/src/__tests__/config.service.test.ts (1)
59-68: Refactor to avoid deprecatedfail()function.The
fail()function is deprecated in Jest. Restructure the test to avoid the try-catch pattern or use proper Jest assertions.♻️ Proposed refactor
it('should include key in error message', () => { - try { - service.getRequired('MISSING_KEY'); - fail('Expected error to be thrown'); - } catch (e) { - expect(e).toBeInstanceOf(ConfigMissingError); - expect((e as ConfigMissingError).key).toBe('MISSING_KEY'); - expect((e as ConfigMissingError).message).toContain('MISSING_KEY'); - } + expect(() => service.getRequired('MISSING_KEY')).toThrow(ConfigMissingError); + + try { + service.getRequired('MISSING_KEY'); + } catch (e) { + expect(e).toBeInstanceOf(ConfigMissingError); + expect((e as ConfigMissingError).key).toBe('MISSING_KEY'); + expect((e as ConfigMissingError).message).toContain('MISSING_KEY'); + } });libs/sdk/src/agent/agent.instance.ts (2)
229-237: Redundant null check afterproviders.get().The
providers.get()method throws if the token isn't found, so if line 230 succeeds,configServiceis guaranteed to be non-null. Theif (configService)check on line 231 is redundant.Proposed simplification
// Try to get ConfigService for config resolution try { const configService = this.providers.get(ConfigService); - if (configService) { - adapterOptions.configResolver = this.createConfigResolver(configService); - } + adapterOptions.configResolver = this.createConfigResolver(configService); } catch { // ConfigPlugin not installed - config resolution via withConfig will not be available this.scope.logger.debug(`ConfigService not available for agent ${this.name} - withConfig resolution disabled`); }
256-267: Consider extracting sharedgetNestedValueutility to avoid duplication.This implementation duplicates the
getNestedValuefunction fromlibs/sdk/src/builtin/config/providers/env-loader.ts(lines 152-165). While the current approach avoids potential circular dependency issues, consider extracting this utility to a shared location (e.g.,@frontmcp/utilsor a common module) to maintain DRY principles.plugins/plugin-config/jest.config.ts (1)
39-46: Branch coverage threshold is below the 95% guideline.The coding guidelines specify "95%+ test coverage across all metrics." The branches threshold is set to 90%, which is below the stated requirement. Consider raising it to 95% to align with the coverage guidelines.
Proposed fix
coverageThreshold: { global: { statements: 95, - branches: 90, + branches: 95, functions: 95, lines: 95, }, },plugins/plugin-config/project.json (1)
60-67: Consider removingpassWithNoTests: trueto enforce test requirements.With
passWithNoTests: true, the test target will pass even if no tests exist. Given the 95%+ coverage requirement, this setting could mask situations where tests are missing. Consider removing this option or setting it tofalseto ensure tests are always present.Proposed change
"test": { "executor": "@nx/jest:jest", "outputs": ["{workspaceRoot}/coverage/unit/plugin-config"], "options": { - "jestConfig": "plugins/plugin-config/jest.config.ts", - "passWithNoTests": true + "jestConfig": "plugins/plugin-config/jest.config.ts" } },apps/e2e/demo-e2e-config/src/apps/config/tools/test-config-fallback.tool.ts (1)
61-64: Optional: Remove redundant path transformation in custom fallback env key generation.The
.replace(/\./g, '_')on line 64 is redundant becausenormalizeNameForEnvalready replaces all non-alphanumeric characters (including dots) with underscores. The double transformation doesn't change the outcome but adds unnecessary processing.♻️ Simplified custom fallback env key generation
} else if (customFallbacks && customFallbacks.length > 0) { // Custom fallbacks paths = customFallbacks; - envKeys = customFallbacks.map((p) => normalizeNameForEnv(p.replace(/\./g, '_'))); + envKeys = customFallbacks.map((p) => normalizeNameForEnv(p)); } else {libs/sdk/src/builtin/config/index.ts (1)
88-109: Helper functions use unchecked type assertions.Lines 97 and 106 cast the return values from
ctx.get()andctx.tryGet()toConfigServicewithout runtime validation. While this is likely safe in a properly configured DI system, it could fail at runtime if:
- The DI container is misconfigured
- A different service is registered with the ConfigService token
- The context implementation doesn't properly implement the get/tryGet contract
As per coding guidelines: "Avoid non-null assertions (
!) - use proper error handling and type guards instead."Consider adding runtime type checking or documenting the assumption that the DI container is correctly configured.
💡 Proposed fix: Add runtime validation
export function getConfig<T extends { get: (token: unknown) => unknown }>(ctx: T): ConfigService { - return ctx.get(ConfigService) as ConfigService; + const service = ctx.get(ConfigService); + if (!service || typeof (service as any).get !== 'function') { + throw new Error('ConfigService not properly registered. Ensure ConfigPlugin is installed.'); + } + return service as ConfigService; } export function tryGetConfig<T extends { tryGet?: (token: unknown) => unknown }>(ctx: T): ConfigService | undefined { if (typeof ctx.tryGet === 'function') { - return ctx.tryGet(ConfigService) as ConfigService | undefined; + const service = ctx.tryGet(ConfigService); + if (service && typeof (service as any).get === 'function') { + return service as ConfigService; + } } return undefined; }libs/cli/src/utils/env.ts (1)
39-42: Edge case: Single-character quoted values may be incorrectly stripped.The quote stripping logic checks if value starts AND ends with the same quote type. For a value like
KEY="a", this works correctly. However, for edge cases likeKEY="(unclosed quote), the conditionvalue.startsWith('"') && value.endsWith('"')would be false (correct), but forKEY='it would also be false (correct).The logic is sound, but consider adding a length check to avoid edge cases with very short values:
♻️ Optional defensive check
// Handle quoted values - if ((value.startsWith('"') && value.endsWith('"')) || (value.startsWith("'") && value.endsWith("'"))) { + if (value.length >= 2 && ((value.startsWith('"') && value.endsWith('"')) || (value.startsWith("'") && value.endsWith("'")))) { value = value.slice(1, -1); }libs/sdk/src/builtin/config/config.plugin.ts (1)
139-161: Legacy mode implementation is correct but type cast could be clearer.The legacy mode correctly handles flat environment variables when no schema is provided. The type cast at line 160 (
merged as unknown as T) is necessary becausemergedisRecord<string, string>butConfigService<T>expectsT.Consider adding a comment explaining this cast is intentional for legacy compatibility:
📝 Add clarifying comment
- return new ConfigService<T>(merged as unknown as T); + // Legacy mode: treat flat env vars as TConfig (string values only) + return new ConfigService<T>(merged as unknown as T);libs/sdk/src/builtin/config/config-resolver.ts (1)
220-227: Consider using a specific error class instead of genericError.Per SDK coding guidelines, specific error classes with MCP error codes should be used instead of generic errors. Consider using
ConfigMissingErrorfromconfig.service.tsfor consistency:♻️ Use ConfigMissingError
+import { ConfigMissingError } from './providers/config.service'; + // In createContextResolver.get(): if (value === undefined) { - throw new Error(`Config "${path}" not found. Tried: ${fallbacks.join(', ')}`); + throw new ConfigMissingError(`${path}" (tried: ${fallbacks.join(', ')})`); } // In createDirectResolver.get(): if (value === undefined) { - throw new Error(`Config "${path}" not found`); + throw new ConfigMissingError(path); }libs/sdk/src/builtin/config/providers/config.service.ts (1)
82-95: Clarify behavior:nullvalues are treated as missing.Line 94 uses
result ?? defaultValue, which means bothnullandundefinedwill trigger the default value. This might be surprising if a config intentionally hasnullas a value.If
nullshould be preserved as a valid config value, consider:📝 Preserve null values (if intended)
- return (result ?? defaultValue) as PathValue<TConfig, P>; + return (result === undefined ? defaultValue : result) as PathValue<TConfig, P>;If the current behavior is intentional (treating
nullas "not set"), consider adding a brief comment to document this design decision.libs/sdk/src/agent/adapters/adapter.factory.ts (1)
114-131: Consider extracting duplicated error path building logic.The logic for determining
triedPathsin error messages is duplicated betweenresolveApiKey(lines 116-125) andresolveStringValue(lines 168-177). Consider extracting this to a helper function:♻️ Extract helper to reduce duplication
function getTriedPaths( withConfig: WithConfig<unknown>, entityContext?: ConfigResolutionContext, ): string[] { if (withConfig.fallbacks === false) { return [withConfig.configPath]; } if (Array.isArray(withConfig.fallbacks)) { return withConfig.fallbacks; } if (entityContext) { return generateFallbacks(withConfig.configPath, entityContext); } return [withConfig.configPath]; }Then use in both functions:
- let triedPaths: string[]; - if (config.fallbacks === false) { - triedPaths = [config.configPath]; - } else if (Array.isArray(config.fallbacks)) { - triedPaths = config.fallbacks; - } else if (entityContext) { - triedPaths = generateFallbacks(config.configPath, entityContext); - } else { - triedPaths = [config.configPath]; - } + const triedPaths = getTriedPaths(config, entityContext);libs/sdk/src/builtin/config/providers/env-loader.ts (3)
71-93: Consider adding error handling for file read operations.The function correctly implements precedence (
.env.localoverrides.env), but doesn't handle potential file read errors. IfreadFilefails due to permissions or file corruption, the error will propagate unhandled. For a configuration loading utility, graceful error handling with informative messages would improve robustness.♻️ Example with try-catch blocks
export async function loadEnvFiles( basePath: string = process.cwd(), envPath: string = '.env', localEnvPath: string = '.env.local', ): Promise<Record<string, string>> { const result: Record<string, string> = {}; // Load base .env file const envFile = path.resolve(basePath, envPath); if (await fileExists(envFile)) { - const content = await readFile(envFile); - Object.assign(result, parseEnvContent(content)); + try { + const content = await readFile(envFile); + Object.assign(result, parseEnvContent(content)); + } catch (error) { + throw new Error(`Failed to read env file ${envFile}: ${error instanceof Error ? error.message : String(error)}`); + } } // Load .env.local (overrides base) const localFile = path.resolve(basePath, localEnvPath); if (await fileExists(localFile)) { - const content = await readFile(localFile); - Object.assign(result, parseEnvContent(content)); + try { + const content = await readFile(localFile); + Object.assign(result, parseEnvContent(content)); + } catch (error) { + throw new Error(`Failed to read env file ${localFile}: ${error instanceof Error ? error.message : String(error)}`); + } } return result; }
99-101: Unnecessary wrapper function.
parseEnvContentSyncis just a direct wrapper aroundparseEnvContent, which is already synchronous. This adds no functional value and may confuse users into thinking there's an async version ofparseEnvContentwhen there isn't (the async operation is inloadEnvFilesinstead).Consider:
- Removing
parseEnvContentSyncentirely and documenting thatparseEnvContentis synchronous- Or, if you need to distinguish between the content parser and file loader, update the documentation to clarify the distinction
216-233: Add ESLint disable comment for consistency.Line 218 uses
anyto access Zod internals (similar to lines 180 and 193), but is missing the ESLint disable comment. For consistency with the rest of the file, add the disable comment.♻️ Add ESLint disable
function unwrapZodType(schema: z.ZodType): z.ZodType { + // eslint-disable-next-line @typescript-eslint/no-explicit-any const def = (schema as any)._zod?.def ?? (schema as any)._def; if (!def) return schema;
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @libs/sdk/package.json:
- Line 79: Add a new documentation page under docs/draft/docs explaining the
ConfigPlugin's new public option: loadYaml on the loadConfig export; describe
what loadYaml does (enables loading YAML config files via js-yaml), its default
value, accepted file extensions, example usage snippet showing how to call
loadConfig({ loadYaml: true }), any related environment or error behaviors, and
mention the dependency on js-yaml (version ^4.1.1); ensure the page references
ConfigPlugin and loadConfig so the new public API is discoverable per libs/**
documentation guidelines.
In @libs/sdk/src/builtin/config/config-resolver.ts:
- Around line 161-172: The inline comments in generateEnvFallbacks() are
inaccurate because normalizeNameForEnv() strips word boundaries, so update the
three comments to reflect actual outputs (e.g.,
`${normalizedType}_${normalizedName}_${normalizedKey}` ->
AGENTS_RESEARCH_AGENT_OPENAIKEY, `${normalizedType}_${normalizedKey}` ->
AGENTS_OPENAIKEY, and `normalizedKey` -> OPENAIKEY) and mention
normalizeNameForEnv by name so future readers know the format comes from that
function rather than word-preserving behavior.
In @libs/sdk/src/builtin/config/providers/__tests__/env-loader.test.ts:
- Around line 1-10: Add unit tests to cover the remaining exported functions in
env-loader: for parseEnvContent, create cases for unquoted, single- and
double-quoted values, escaped characters (\\n, \\r, \\\", \\\\) and
comment/empty-line handling; for parseEnvContentSync verify same behavior
synchronously; for loadEnvFiles test async loading of multiple files including
non-existent files, merging precedence and that later files override earlier
ones; for populateProcessEnv assert it correctly sets process.env entries and
respects overwrite/skip behavior; for extractSchemaPaths provide zod schema
examples (objects, arrays, optional, nullable, defaults) and assert it returns
correct path vectors; for unwrapZodType test unwrapping of
optional/nullable/brand/wrapped zod types to base types. Reference these
functions by name (parseEnvContent, parseEnvContentSync, loadEnvFiles,
populateProcessEnv, extractSchemaPaths, unwrapZodType) and ensure edge cases and
error paths are asserted to drive coverage above 95%.
🧹 Nitpick comments (3)
libs/sdk/src/builtin/config/providers/env-loader.ts (2)
95-101: Consider removing the sync wrapper if unused.
parseEnvContentSyncis functionally identical toparseEnvContentsince the underlying function is already synchronous. If this is intentional API clarity for CLI consumers, the current implementation is fine. Otherwise, it adds unnecessary surface area.
210-246: UseObject.hasOwnorhasOwnPropertycheck in schema path extraction.The
for (const key in shape)loop on line 222 may iterate over inherited properties ifshapehas a prototype chain. While unlikely with Zod schema shapes, defensive coding would useObject.hasOwn():♻️ Suggested fix
if (shape && typeof shape === 'object') { const paths: string[] = []; - for (const key in shape) { + for (const key of Object.keys(shape)) { const fieldPath = prefix ? `${prefix}.${key}` : key;apps/e2e/demo-e2e-config/src/apps/config/tools/check-config.tool.ts (1)
10-16: Consider adding.strict()to outputSchema for consistency.The
inputSchemauses.strict()(line 8), butoutputSchemadoesn't. While this may be intentional, adding.strict()to the output schema would ensure consistency and prevent accidental property additions.♻️ Proposed consistency fix
const outputSchema = z.object({ key: z.string(), exists: z.boolean(), isNumber: z.boolean(), numberValue: z.number().nullable(), booleanValue: z.boolean().nullable(), -}); +}).strict();
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/e2e/demo-e2e-config/src/apps/config/tools/check-config.tool.tsapps/e2e/demo-e2e-config/webpack.config.jslibs/sdk/package.jsonlibs/sdk/src/builtin/config/config-resolver.tslibs/sdk/src/builtin/config/providers/__tests__/env-loader.test.tslibs/sdk/src/builtin/config/providers/env-loader.tslibs/sdk/src/common/interfaces/execution-context.interface.tsplugins/plugin-config/src/__tests__/config.service.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- libs/sdk/src/common/interfaces/execution-context.interface.ts
- apps/e2e/demo-e2e-config/webpack.config.js
- plugins/plugin-config/src/tests/config.service.test.ts
🧰 Additional context used
📓 Path-based instructions (5)
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/package.jsonlibs/sdk/src/builtin/config/config-resolver.tslibs/sdk/src/builtin/config/providers/env-loader.tslibs/sdk/src/builtin/config/providers/__tests__/env-loader.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use strict TypeScript mode with noanytypes without strong justification
Avoid non-null assertions (!) - use proper error handling and type guards instead
Use@frontmcp/utilsfor cryptographic operations instead ofnode:cryptodirectly
Use@frontmcp/utilsfor file system operations instead offs/promisesornode:fsdirectly
Files:
apps/e2e/demo-e2e-config/src/apps/config/tools/check-config.tool.tslibs/sdk/src/builtin/config/config-resolver.tslibs/sdk/src/builtin/config/providers/env-loader.tslibs/sdk/src/builtin/config/providers/__tests__/env-loader.test.ts
libs/{sdk,adapters}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
libs/{sdk,adapters}/**/*.{ts,tsx}: Use specific error classes with MCP error codes instead of generic errors
UsegetCapabilities()for dynamic capability exposure instead of hardcoding capabilities
Files:
libs/sdk/src/builtin/config/config-resolver.tslibs/sdk/src/builtin/config/providers/env-loader.tslibs/sdk/src/builtin/config/providers/__tests__/env-loader.test.ts
libs/sdk/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
libs/sdk/**/*.{ts,tsx}: Preferinterfacefor defining object shapes in TypeScript, avoidanytypes
Use type parameters with constraints instead of unconstrained generics withanydefaults
Validate URIs per RFC 3986 at metadata level usingisValidMcpUrirefinement
UsechangeScopeinstead ofscopein change event properties to avoid confusion with Scope class
Fail fast on invalid hook flows by validating hooks match their entry type
Centralize record types in common/records and import from there, not from module-specific files
Return strictly typed MCP protocol responses (e.g.,Promise<GetPromptResult>) instead ofPromise<unknown>for MCP entry methods
Useunknowninstead ofanyfor generic type parameter defaults (not for MCP protocol types)
Create shared base classes for common functionality like ExecutionContextBase for ToolContext and ResourceContext
Do not mutate rawInput in flows - use state.set() for managing flow state instead
Validate inputs and outputs through parseOutput/safeParseOutput methods in validation flows
Files:
libs/sdk/src/builtin/config/config-resolver.tslibs/sdk/src/builtin/config/providers/env-loader.tslibs/sdk/src/builtin/config/providers/__tests__/env-loader.test.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.{ts,tsx}: Achieve 95%+ test coverage across all metrics (statements, branches, functions, lines)
Use Jest for testing with 95%+ coverage requirement and 100% test pass rate
Do not use prefixes like 'PT-001' in test names
Do not skip constructor validation tests for error classes and types
Includeinstanceofchecks in tests for error classes to verify proper error hierarchy
Files:
libs/sdk/src/builtin/config/providers/__tests__/env-loader.test.ts
🧠 Learnings (3)
📚 Learning: 2026-01-06T17:16:04.304Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T17:16:04.304Z
Learning: Applies to libs/sdk/**/*.{ts,tsx} : Create shared base classes for common functionality like ExecutionContextBase for ToolContext and ResourceContext
Applied to files:
apps/e2e/demo-e2e-config/src/apps/config/tools/check-config.tool.ts
📚 Learning: 2026-01-06T02:34:55.689Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/plugins/CLAUDE.md:0-0
Timestamp: 2026-01-06T02:34:55.689Z
Learning: Applies to libs/plugins/**/*.ts : Never hardcode encryption keys; use environment variables for key management
Applied to files:
libs/sdk/src/builtin/config/providers/env-loader.tslibs/sdk/src/builtin/config/providers/__tests__/env-loader.test.ts
📚 Learning: 2026-01-04T14:35:18.366Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/uipack/CLAUDE.md:0-0
Timestamp: 2026-01-04T14:35:18.366Z
Learning: Applies to libs/uipack/**/{theme,adapters,bundler}/**/*.{test,spec}.{ts,tsx,js,jsx} : Test behavior across all supported platform configurations (OpenAI, Claude, etc.)
Applied to files:
libs/sdk/src/builtin/config/providers/__tests__/env-loader.test.ts
🧬 Code graph analysis (1)
apps/e2e/demo-e2e-config/src/apps/config/tools/check-config.tool.ts (4)
apps/e2e/demo-e2e-config/src/apps/config/tools/get-config.tool.ts (1)
Tool(21-39)apps/e2e/demo-e2e-config/src/apps/config/tools/get-all-config.tool.ts (1)
Tool(15-41)apps/e2e/demo-e2e-config/src/apps/config/tools/get-required-config.tool.ts (1)
Tool(20-44)apps/e2e/demo-e2e-config/src/apps/config/tools/test-config-fallback.tool.ts (1)
Tool(41-108)
🪛 ast-grep (0.40.4)
libs/sdk/src/builtin/config/providers/env-loader.ts
[warning] 51-51: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(PLACEHOLDER, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ 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). (3)
- GitHub Check: Lint & Format Checks
- GitHub Check: Build Libraries
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (16)
libs/sdk/src/builtin/config/providers/__tests__/env-loader.test.ts (2)
31-73: Thorough prototype pollution protection tests.The test suite comprehensively covers prototype pollution vectors including
__proto__,constructor,prototype, and nested variants. The tests for safe similar-looking keys (__proto,proto__,constructorValue) verify the protection isn't overly aggressive.
116-148: LGTM - mapEnvToNestedConfig tests cover core scenarios.Tests verify both the happy path (mapping flat env vars to nested config) and edge case handling (skipping missing env vars).
libs/sdk/src/builtin/config/config-resolver.ts (3)
186-198: Theanycast is justified but consider documenting the limitation.The cast on line 192 is necessary because the dynamic
pathstring cannot satisfy the type-safe key constraint ofConfigService.get(). This is an acceptable trade-off for the flexible fallback resolution pattern. The eslint-disable comment is appropriate.
218-236: Well-designed context resolver with clear fallback semantics.The
createContextResolverfunction correctly implements the 3-level fallback chain and provides meaningful error messages that include all attempted paths. The implementation is clean and follows the documented behavior.
244-259: LGTM - Direct resolver provides simple lookup without fallbacks.Clean implementation that complements the context-aware resolver for cases where fallback behavior isn't needed.
libs/sdk/src/builtin/config/providers/env-loader.ts (7)
44-53: Static analysis false positive - PLACEHOLDER is a constant, not user input.The ast-grep warning about ReDoS from
new RegExp(PLACEHOLDER, 'g')is a false positive.PLACEHOLDERis defined as a constant string'\x00BACKSLASH\x00'on line 46, not derived from user input. The pattern contains no quantifiers or alternation that could cause catastrophic backtracking.
1-3: Correct use of @frontmcp/utils for file operations.The imports use
@frontmcp/utilsforreadFileandfileExistsas required by the coding guidelines instead of directfs/promisesusage.
17-60: Robust dotenv parsing implementation.The
parseEnvContentfunction handles the key dotenv parsing scenarios:
- Comment lines with
#- Quoted values (single and double)
- Escape sequence expansion for double-quoted values
- The placeholder technique for handling
\\before other escapes is a sound approach
71-93: File loading correctly prioritizes.env.localover.env.The merge order ensures local overrides take precedence. Existence checks prevent errors for missing files.
122-134: Effective prototype pollution protection.The
UNSAFE_KEYSset covers the three primary prototype pollution vectors (__proto__,constructor,prototype). TheisSafeKeycheck is applied consistently in bothsetNestedValueandgetNestedValue.
248-269: Zod v3/v4 compatibility handling is appropriate.The
unwrapZodTypefunction correctly handles both_zod.def(v4) and_def(v3) accessor patterns. The recursive unwrapping forinnerType(ZodDefault/ZodOptional) andschema(ZodEffects) covers the common wrapper types.
279-295: LGTM - Clean env-to-nested mapping implementation.The
mapEnvToNestedConfigfunction correctly converts flat environment variables to nested config structures using the provided schema paths, delegating to the well-testedsetNestedValuefor safe assignment.apps/e2e/demo-e2e-config/src/apps/config/tools/check-config.tool.ts (4)
1-2: LGTM!Imports are clean and follow the established patterns from other config tools in the suite.
18-19: LGTM!Type aliases correctly use Zod's type inference, consistent with the patterns in other config tools.
21-27: LGTM!The tool decorator and class definition follow the established pattern from other config tools (GetConfigTool, GetRequiredConfigTool, etc.) and properly utilize TypeScript generics for type safety.
28-40: Code implementation is correct.The config service methods handle non-existent keys gracefully as expected:
getNumber()returns NaN for non-existent keys (line 169 in config.service.ts)getBoolean()returns false for non-existent keys (line 189 in config.service.ts)The implementation correctly leverages these behaviors:
- Uses
!isNaN()for number validation (where NaN signals a non-existent or invalid value)- Uses the
existscheck for boolean values (to distinguish non-existent keys from keys with valuefalse)No changes needed.
libs/sdk/src/builtin/config/providers/__tests__/env-loader.test.ts
Outdated
Show resolved
Hide resolved
…nce documentation
# Conflicts: # libs/plugins/package.json
…nce documentation
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @docs/draft/docs/extensibility/config-yaml.mdx:
- Around line 209-253: The example repeats the Zod v4 default-shape issue and
doesn't demonstrate typed getters: update configSchema so the features object
has a proper default matching its output type (e.g., provide the full default
object instead of .default({}) on the z.object) and adjust MyTool.execute to use
the typed accessors (this.config.getNumber('database.port'),
this.config.getBoolean('features.darkMode') or this.config.getRequired(...) as
appropriate) and ensure ConfigPlugin.init still passes schema: configSchema;
reference configSchema, ConfigPlugin.init, and MyTool.execute to locate and
change the schema default and replace this.config.get(...) calls with the typed
getters.
- Around line 57-75: The schema uses z.object({...}).default({}) for the pool
field which fails Zod v4’s “default must match output type”; update the pool
default to match the object shape (e.g., provide a default object with min and
max keys) or remove the .default and make pool optional with explicit defaults
for its properties; locate the configSchema and the pool definition and replace
the .default({}) on that z.object with a default value whose type matches the
object output (or switch to .optional() and rely on the nested .default(...) on
min/max).
- Around line 22-35: The example uses the decorator symbol App but doesn't
import it; update the import line to include App (e.g., import { App,
ConfigPlugin } from '@frontmcp/sdk') so the decorator @App and the
ConfigPlugin.init call compile correctly in the MyApp snippet.
In @libs/sdk/src/builtin/config/providers/__tests__/env-loader.test.ts:
- Around line 391-401: The test for precedence in loadEnvFiles should assert the
variable exists and has the expected value rather than conditionally skipping;
replace the conditional block in the 'should have .env.local values override
.env values' test with a direct assertion that result.OVERRIDDEN_VAR is defined
and equals 'local_value' (use expect(result.OVERRIDDEN_VAR).toBeDefined() or
expect(result.OVERRIDDEN_VAR).toBe('local_value')) so missing/misconfigured
fixtures cause a failing test; reference the loadEnvFiles call and the
OVERRIDDEN_VAR check in the test.
🧹 Nitpick comments (5)
libs/sdk/src/builtin/config/providers/__tests__/env-loader.test.ts (3)
373-373: Consider extracting the fixture path to a constant.The deeply nested relative path is brittle and could break if the directory structure changes.
♻️ Suggested refactor
+const FIXTURES_PATH = path.resolve(__dirname, '../../../../../../apps/e2e/demo-e2e-config/src/apps/config'); + describe('loadEnvFiles', () => { - const fixturesPath = path.resolve(__dirname, '../../../../../../apps/e2e/demo-e2e-config/src/apps/config'); + const fixturesPath = FIXTURES_PATH;
469-481: Verify inner path extraction for nullable nested objects.The test checks that
'nested'is in the paths, but doesn't verify whether'nested.inner'is also extracted. Based on the implementation ofextractSchemaPathswhich unwraps nullable types, the inner path should be included.🧪 Suggested enhancement
it('should handle schemas with nullable fields', () => { const schema = z.object({ nullable: z.string().nullable(), nested: z .object({ inner: z.number().nullable(), }) .nullable(), }); const paths = extractSchemaPaths(schema); expect(paths).toContain('nullable'); expect(paths).toContain('nested'); + expect(paths).toContain('nested.inner'); });
372-402: Consider adding error scenario tests.The
loadEnvFilestest suite covers happy paths and missing files well, but could benefit from testing error scenarios such as:
- File exists but cannot be read (permission denied)
- Malformed file content that causes parsing errors
- Symbolic links or special file types
These tests would improve overall robustness and coverage.
docs/draft/docs/extensibility/config-yaml.mdx (2)
81-100:await loadConfig(...)at top-level may not work for all TS/Node module targets.
Consider wrapping in an async IIFE (or add a note that the snippet assumes top-level await / ESM).
118-131: “Accepted File Extensions” wording is a bit inconsistent.
IfconfigPathincludes an extension, the loader shouldn’t “try” extensions—there’s nothing to probe. Suggest rewording to: “IfconfigPathhas no extension, we probe.ymlthen.yaml; otherwise we use the provided path as-is.”
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/e2e/demo-e2e-config/src/apps/config/tools/check-config.tool.tsdocs/draft/docs/extensibility/config-yaml.mdxlibs/sdk/src/builtin/config/config-resolver.tslibs/sdk/src/builtin/config/providers/__tests__/env-loader.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/sdk/src/builtin/config/config-resolver.ts
- apps/e2e/demo-e2e-config/src/apps/config/tools/check-config.tool.ts
🧰 Additional context used
📓 Path-based instructions (7)
docs/draft/docs/**
⚙️ CodeRabbit configuration file
docs/draft/docs/**: This folder holds the draft/source docs that humans are expected to edit. When authors want to add or change documentation, they should do it here. The Codex workflow uses these drafts, together with the code diff, to generate the latest docs under docs/docs/. As a reviewer: - Encourage contributors to add/update content here instead of docs/docs/. - It is fine to do structural/content feedback here (clarity, examples, etc).
Files:
docs/draft/docs/extensibility/config-yaml.mdx
docs/**
⚙️ CodeRabbit configuration file
docs/**: Repository documentation for the SDK, using MDX and hosted by Mintlify. See more specific rules for: - docs/docs/** (latest rendered docs, automation-only) - docs/v/** (archived versions, read-only) - docs/draft/docs/** (human-editable drafts) - docs/blogs/** (blogs, human edited) - docs/docs.json (Mintlify navigation)
Files:
docs/draft/docs/extensibility/config-yaml.mdx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use strict TypeScript mode with noanytypes without strong justification
Avoid non-null assertions (!) - use proper error handling and type guards instead
Use@frontmcp/utilsfor cryptographic operations instead ofnode:cryptodirectly
Use@frontmcp/utilsfor file system operations instead offs/promisesornode:fsdirectly
Files:
libs/sdk/src/builtin/config/providers/__tests__/env-loader.test.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.{ts,tsx}: Achieve 95%+ test coverage across all metrics (statements, branches, functions, lines)
Use Jest for testing with 95%+ coverage requirement and 100% test pass rate
Do not use prefixes like 'PT-001' in test names
Do not skip constructor validation tests for error classes and types
Includeinstanceofchecks in tests for error classes to verify proper error hierarchy
Files:
libs/sdk/src/builtin/config/providers/__tests__/env-loader.test.ts
libs/{sdk,adapters}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
libs/{sdk,adapters}/**/*.{ts,tsx}: Use specific error classes with MCP error codes instead of generic errors
UsegetCapabilities()for dynamic capability exposure instead of hardcoding capabilities
Files:
libs/sdk/src/builtin/config/providers/__tests__/env-loader.test.ts
libs/sdk/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
libs/sdk/**/*.{ts,tsx}: Preferinterfacefor defining object shapes in TypeScript, avoidanytypes
Use type parameters with constraints instead of unconstrained generics withanydefaults
Validate URIs per RFC 3986 at metadata level usingisValidMcpUrirefinement
UsechangeScopeinstead ofscopein change event properties to avoid confusion with Scope class
Fail fast on invalid hook flows by validating hooks match their entry type
Centralize record types in common/records and import from there, not from module-specific files
Return strictly typed MCP protocol responses (e.g.,Promise<GetPromptResult>) instead ofPromise<unknown>for MCP entry methods
Useunknowninstead ofanyfor generic type parameter defaults (not for MCP protocol types)
Create shared base classes for common functionality like ExecutionContextBase for ToolContext and ResourceContext
Do not mutate rawInput in flows - use state.set() for managing flow state instead
Validate inputs and outputs through parseOutput/safeParseOutput methods in validation flows
Files:
libs/sdk/src/builtin/config/providers/__tests__/env-loader.test.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/builtin/config/providers/__tests__/env-loader.test.ts
🧠 Learnings (7)
📚 Learning: 2026-01-04T14:35:18.366Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/uipack/CLAUDE.md:0-0
Timestamp: 2026-01-04T14:35:18.366Z
Learning: Applies to libs/uipack/**/{theme,adapters,bundler}/**/*.{test,spec}.{ts,tsx,js,jsx} : Test behavior across all supported platform configurations (OpenAI, Claude, etc.)
Applied to files:
libs/sdk/src/builtin/config/providers/__tests__/env-loader.test.ts
📚 Learning: 2026-01-06T02:34:55.689Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/plugins/CLAUDE.md:0-0
Timestamp: 2026-01-06T02:34:55.689Z
Learning: Applies to libs/plugins/**/*.ts : Never hardcode encryption keys; use environment variables for key management
Applied to files:
libs/sdk/src/builtin/config/providers/__tests__/env-loader.test.ts
📚 Learning: 2026-01-06T17:16:04.304Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T17:16:04.304Z
Learning: Applies to **/*.test.{ts,tsx} : Achieve 95%+ test coverage across all metrics (statements, branches, functions, lines)
Applied to files:
libs/sdk/src/builtin/config/providers/__tests__/env-loader.test.ts
📚 Learning: 2025-12-24T00:41:41.819Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-12-24T00:41:41.819Z
Learning: Applies to libs/ui/src/**/*.test.{ts,tsx} : Test coverage must be 95%+ across statements, branches, functions, and lines
Applied to files:
libs/sdk/src/builtin/config/providers/__tests__/env-loader.test.ts
📚 Learning: 2026-01-06T17:16:04.304Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T17:16:04.304Z
Learning: Applies to **/*.test.{ts,tsx} : Use Jest for testing with 95%+ coverage requirement and 100% test pass rate
Applied to files:
libs/sdk/src/builtin/config/providers/__tests__/env-loader.test.ts
📚 Learning: 2026-01-04T14:35:18.366Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/uipack/CLAUDE.md:0-0
Timestamp: 2026-01-04T14:35:18.366Z
Learning: Applies to libs/uipack/**/{src,tests}/**/*.{ts,tsx,js,jsx} : Maintain 95%+ code coverage across statements, branches, functions, and lines
Applied to files:
libs/sdk/src/builtin/config/providers/__tests__/env-loader.test.ts
📚 Learning: 2026-01-06T02:34:55.689Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/plugins/CLAUDE.md:0-0
Timestamp: 2026-01-06T02:34:55.689Z
Learning: Applies to libs/plugins/**/*.test.ts : Tests must achieve 95%+ code coverage across all metrics; use `MockStore` implementing `RememberStoreInterface` for RememberPlugin testing
Applied to files:
libs/sdk/src/builtin/config/providers/__tests__/env-loader.test.ts
🧬 Code graph analysis (1)
libs/sdk/src/builtin/config/providers/__tests__/env-loader.test.ts (1)
libs/sdk/src/builtin/config/providers/env-loader.ts (9)
pathToEnvKey(140-142)setNestedValue(151-172)getNestedValue(181-202)mapEnvToNestedConfig(279-295)parseEnvContent(17-60)parseEnvContentSync(99-101)populateProcessEnv(110-116)loadEnvFiles(71-93)extractSchemaPaths(210-246)
⏰ 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). (3)
- GitHub Check: Lint & Format Checks
- GitHub Check: Build Libraries
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (8)
libs/sdk/src/builtin/config/providers/__tests__/env-loader.test.ts (6)
1-22: LGTM: Clean imports and basic test structure.The imports are appropriate, and the
pathToEnvKeytests cover the basic conversion functionality adequately.
43-85: Excellent prototype pollution protection tests.The comprehensive coverage of prototype pollution vectors (
__proto__,constructor,prototype) and the verification that similar-looking safe keys still work demonstrates thorough security awareness.
88-126: LGTM: Comprehensive getter tests with security coverage.The tests thoroughly cover both functionality and security concerns, maintaining consistency with the setter tests.
162-307: Excellent comprehensive coverage of parsing logic.The test suite thoroughly exercises all parsing scenarios including quotes, escape sequences, comments, empty lines, and edge cases. The organization with nested describe blocks enhances readability.
324-370: LGTM: Proper environment variable testing with cleanup.The tests correctly handle
process.envmutations with appropriate setup and teardown, ensuring test isolation.
1-508: Overall: Comprehensive and well-structured test suite.This test file demonstrates excellent coverage of the env-loader module with particular strengths in:
- Thorough prototype pollution protection testing
- Comprehensive parsing logic coverage with edge cases
- Proper test isolation for environment variable mutations
- Well-organized test structure with descriptive names
The test suite should achieve the required 95%+ coverage target. As per coding guidelines for libs/sdk test files, the coverage and structure align with project standards.
docs/draft/docs/extensibility/config-yaml.mdx (2)
147-162: The documentation example is correct and accurately reflects the SDK's env-var-to-nested-key mapping convention. The implementation usespathToEnvKey()which converts schema paths to environment variable names by converting to uppercase and replacing dots with underscores (e.g.,database.url→DATABASE_URL).
165-188: Documentation statement about silent file handling is accurate—no verification needed.The implementation shows that
loadYamlConfigconsistently returns an empty object when no YAML file is found (lines 95–115 of config-loader.ts), regardless of whetherconfigPathuses the default value or is explicitly provided. The function iterates through possible extensions and returns{}if none exist, so the behavior does not vary by mode.YAML parse errors from
js-yamlare thrown as expected (line 105), and schema validation catches any resulting issues. The documented behavior is correct.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/push.yml (1)
3-4: Verify removal of branch filter—workflow now runs on all branches.Confirmed: removing the
branchesfilter from lines 3-4 causes this workflow to trigger on pushes to any branch. The lint, build, unit tests, and 14 E2E test projects (including the newly added demo-e2e-config) all run on every push to any branch. Only the coverage job is gated to the main branch (if: github.ref == 'refs/heads/main'on line 219). This design significantly increases CI costs, resource consumption, and queue times compared to the previous branch-filtered approach.Consider whether this broad triggering is intentional. If you want to limit costs on feature branches while maintaining CI coverage, you could:
- Restore the branch filter and rely on pull request workflows for feature branch testing
- Add conditionals to skip expensive jobs on non-protected branches (e.g.,
if: github.ref == 'refs/heads/main' || startsWith(github.ref, 'refs/heads/next/'))- Use separate workflows for different branch types with different job configurations
🧹 Nitpick comments (1)
libs/sdk/src/builtin/config/providers/__tests__/env-loader.test.ts (1)
372-376: Consider a more robust fixture path resolution.The path calculation using 7 levels up from
__dirnameis fragile and will break if the directory structure changes. Consider using a more resilient approach:
- Use
path.join(__dirname, '../../../../../../..', 'apps/e2e/demo-e2e-config')for clarity- Or define a helper that locates the project root (e.g., by finding package.json)
- Or document this dependency more explicitly in test setup
♻️ Alternative approach using explicit segments
- // Path to demo-e2e-config root where .env files exist - // From __dirname (libs/sdk/src/builtin/config/providers/__tests__) need 7 levels up to reach project root - const fixturesPath = path.resolve(__dirname, '../../../../../../../apps/e2e/demo-e2e-config'); + // Path to demo-e2e-config root where .env files exist + // From __dirname (libs/sdk/src/builtin/config/providers/__tests__) to project root to fixtures + const fixturesPath = path.join( + __dirname, + '../../../../../../..', // Up to project root + 'apps/e2e/demo-e2e-config' // Down to fixtures + );
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/push.ymldocs/draft/docs/extensibility/config-yaml.mdxlibs/adapters/package.jsonlibs/sdk/src/builtin/config/providers/__tests__/env-loader.test.tslibs/sdk/src/builtin/config/providers/env-loader.ts
💤 Files with no reviewable changes (1)
- libs/adapters/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/draft/docs/extensibility/config-yaml.mdx
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use strict TypeScript mode with noanytypes without strong justification
Avoid non-null assertions (!) - use proper error handling and type guards instead
Use@frontmcp/utilsfor cryptographic operations instead ofnode:cryptodirectly
Use@frontmcp/utilsfor file system operations instead offs/promisesornode:fsdirectly
Files:
libs/sdk/src/builtin/config/providers/__tests__/env-loader.test.tslibs/sdk/src/builtin/config/providers/env-loader.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.{ts,tsx}: Achieve 95%+ test coverage across all metrics (statements, branches, functions, lines)
Use Jest for testing with 95%+ coverage requirement and 100% test pass rate
Do not use prefixes like 'PT-001' in test names
Do not skip constructor validation tests for error classes and types
Includeinstanceofchecks in tests for error classes to verify proper error hierarchy
Files:
libs/sdk/src/builtin/config/providers/__tests__/env-loader.test.ts
libs/{sdk,adapters}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
libs/{sdk,adapters}/**/*.{ts,tsx}: Use specific error classes with MCP error codes instead of generic errors
UsegetCapabilities()for dynamic capability exposure instead of hardcoding capabilities
Files:
libs/sdk/src/builtin/config/providers/__tests__/env-loader.test.tslibs/sdk/src/builtin/config/providers/env-loader.ts
libs/sdk/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
libs/sdk/**/*.{ts,tsx}: Preferinterfacefor defining object shapes in TypeScript, avoidanytypes
Use type parameters with constraints instead of unconstrained generics withanydefaults
Validate URIs per RFC 3986 at metadata level usingisValidMcpUrirefinement
UsechangeScopeinstead ofscopein change event properties to avoid confusion with Scope class
Fail fast on invalid hook flows by validating hooks match their entry type
Centralize record types in common/records and import from there, not from module-specific files
Return strictly typed MCP protocol responses (e.g.,Promise<GetPromptResult>) instead ofPromise<unknown>for MCP entry methods
Useunknowninstead ofanyfor generic type parameter defaults (not for MCP protocol types)
Create shared base classes for common functionality like ExecutionContextBase for ToolContext and ResourceContext
Do not mutate rawInput in flows - use state.set() for managing flow state instead
Validate inputs and outputs through parseOutput/safeParseOutput methods in validation flows
Files:
libs/sdk/src/builtin/config/providers/__tests__/env-loader.test.tslibs/sdk/src/builtin/config/providers/env-loader.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/builtin/config/providers/__tests__/env-loader.test.tslibs/sdk/src/builtin/config/providers/env-loader.ts
🧠 Learnings (8)
📚 Learning: 2026-01-04T14:35:18.366Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/uipack/CLAUDE.md:0-0
Timestamp: 2026-01-04T14:35:18.366Z
Learning: Applies to libs/uipack/**/{theme,adapters,bundler}/**/*.{test,spec}.{ts,tsx,js,jsx} : Test behavior across all supported platform configurations (OpenAI, Claude, etc.)
Applied to files:
libs/sdk/src/builtin/config/providers/__tests__/env-loader.test.ts
📚 Learning: 2026-01-06T17:16:04.304Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T17:16:04.304Z
Learning: Applies to **/*.test.{ts,tsx} : Use Jest for testing with 95%+ coverage requirement and 100% test pass rate
Applied to files:
libs/sdk/src/builtin/config/providers/__tests__/env-loader.test.ts
📚 Learning: 2026-01-06T02:34:55.689Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/plugins/CLAUDE.md:0-0
Timestamp: 2026-01-06T02:34:55.689Z
Learning: Applies to libs/plugins/**/*.ts : Never hardcode encryption keys; use environment variables for key management
Applied to files:
libs/sdk/src/builtin/config/providers/__tests__/env-loader.test.tslibs/sdk/src/builtin/config/providers/env-loader.ts
📚 Learning: 2025-12-24T00:41:41.819Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-12-24T00:41:41.819Z
Learning: Applies to libs/ui/src/**/*.test.{ts,tsx} : Use React Testing Library for component tests and include SSR/hydration tests for all interactive components
Applied to files:
libs/sdk/src/builtin/config/providers/__tests__/env-loader.test.ts
📚 Learning: 2026-01-06T17:16:04.304Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T17:16:04.304Z
Learning: Applies to **/*.test.{ts,tsx} : Achieve 95%+ test coverage across all metrics (statements, branches, functions, lines)
Applied to files:
libs/sdk/src/builtin/config/providers/__tests__/env-loader.test.ts
📚 Learning: 2025-12-24T00:41:41.819Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-12-24T00:41:41.819Z
Learning: Applies to libs/ui/src/**/*.test.{ts,tsx} : Test coverage must be 95%+ across statements, branches, functions, and lines
Applied to files:
libs/sdk/src/builtin/config/providers/__tests__/env-loader.test.ts
📚 Learning: 2026-01-04T14:35:18.366Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/uipack/CLAUDE.md:0-0
Timestamp: 2026-01-04T14:35:18.366Z
Learning: Applies to libs/uipack/**/{src,tests}/**/*.{ts,tsx,js,jsx} : Maintain 95%+ code coverage across statements, branches, functions, and lines
Applied to files:
libs/sdk/src/builtin/config/providers/__tests__/env-loader.test.ts
📚 Learning: 2026-01-06T02:34:55.689Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/plugins/CLAUDE.md:0-0
Timestamp: 2026-01-06T02:34:55.689Z
Learning: Applies to libs/plugins/**/*.test.ts : Tests must achieve 95%+ code coverage across all metrics; use `MockStore` implementing `RememberStoreInterface` for RememberPlugin testing
Applied to files:
libs/sdk/src/builtin/config/providers/__tests__/env-loader.test.ts
🧬 Code graph analysis (1)
libs/sdk/src/builtin/config/providers/__tests__/env-loader.test.ts (1)
libs/sdk/src/builtin/config/providers/env-loader.ts (9)
pathToEnvKey(140-142)setNestedValue(152-183)getNestedValue(192-217)mapEnvToNestedConfig(294-310)parseEnvContent(17-60)parseEnvContentSync(99-101)populateProcessEnv(110-116)loadEnvFiles(71-93)extractSchemaPaths(225-261)
🪛 ast-grep (0.40.4)
libs/sdk/src/builtin/config/providers/env-loader.ts
[warning] 51-51: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(PLACEHOLDER, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ 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). (4)
- GitHub Check: Lint & Format Checks
- GitHub Check: Build Libraries
- GitHub Check: Package E2E (Verdaccio)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (7)
.github/workflows/push.yml (1)
172-172: Project is properly configured. Thedemo-e2e-configaddition to the E2E test matrix is valid. The project includes proper configuration (project.json with jest targets), test files (config.e2e.test.ts), and follows the same pattern as other demo-e2e-* projects.libs/sdk/src/builtin/config/providers/__tests__/env-loader.test.ts (1)
15-508: Excellent test coverage and structure!The test suite is comprehensive and well-organized, with exceptional coverage that exceeds the 95% requirement. Key strengths:
- Thorough prototype pollution security testing
- Comprehensive edge case coverage for parsing (quotes, escapes, comments)
- Proper test isolation with cleanup in populateProcessEnv tests
- Good variety of Zod schema shapes in extractSchemaPaths tests
The tests follow Jest best practices and comply with all coding guidelines.
libs/sdk/src/builtin/config/providers/env-loader.ts (5)
41-53: Static analysis false positive: RegExp from constant is safe.The static analysis tool warns about
new RegExp(PLACEHOLDER, 'g')on line 52, but this is a false positive. ThePLACEHOLDERvariable is a constant string'\x00BACKSLASH\x00'defined on line 46, not user input, so there's no ReDoS risk.The escape sequence handling logic is well-designed: handling double-backslash first as a placeholder prevents overlapping pattern issues during subsequent replacements.
Based on static analysis hints.
152-183: Excellent multi-layered security implementation!The
setNestedValuefunction demonstrates defense-in-depth against prototype pollution with:
- Upfront validation of all keys (lines 156-160)
- Explicit inline checks during traversal (lines 167-169) to satisfy CodeQL flow analysis
- Final key validation before assignment (lines 179-181)
- Use of
Object.create(null)(line 172) to create objects without prototype chainsThis layered approach ensures security even if one check fails, and the explicit inline checks help static analysis tools understand the safety guarantees.
192-217: Consistent security approach in getNestedValue.The
getNestedValuefunction mirrors the security protections fromsetNestedValue, with upfront validation and inline checks. The safe default of returningundefinedfor unsafe paths prevents information leakage through prototype chain access.
225-261: Well-justified use ofanyfor Zod introspection.The use of
anytypes inextractSchemaPathsandunwrapZodTypeis appropriate and well-documented:
- Required for runtime introspection of Zod internal structure
- Supports both Zod v3 (
_def) and v4 (_zod.def) for compatibility- Each usage has an eslint-disable comment explaining the necessity
- JSDoc on line 266 explicitly documents version compatibility
The implementation correctly handles wrapper types (default, optional, nullable, effects) and recursively extracts paths from nested objects.
As per coding guidelines,
anyshould only be used with strong justification—this case qualifies due to library introspection requirements.Also applies to: 263-284
1-310: High-quality implementation with strong security guarantees.This env-loader module demonstrates excellent engineering practices:
- ✓ Uses
@frontmcp/utilsfor file operations (complies with guidelines)- ✓ Comprehensive prototype pollution protections with multiple defensive layers
- ✓ Well-documented with JSDoc including security annotations
- ✓ Proper TypeScript typing with justified
anyusage only for Zod internals- ✓ Zod v3/v4 compatibility
- ✓ Clean separation of concerns (parsing, loading, mapping, introspection)
The test coverage validates all edge cases and security protections thoroughly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
apps/e2e/demo-e2e-config/src/apps/config/tools/get-config.tool.ts (3)
4-9: Consider validatingkeyas non-empty (and maybe a format). Empty string keys will currently pass schema validation.Proposed tweak
const inputSchema = z .object({ - key: z.string().describe('Environment variable key to retrieve'), + key: z.string().min(1).describe('Environment variable key to retrieve'), defaultValue: z.string().optional().describe('Default value if key is not found'), }) .strict();
11-16: Optionally makeoutputSchemastrict for consistency.Proposed tweak
-const outputSchema = z.object({ +const outputSchema = z + .object({ key: z.string(), value: z.string().nullable(), found: z.boolean(), defaultUsed: z.boolean(), -}); + }) + .strict();
28-39: Verifyconfig.get(key, default)semantics + consider avoiding double lookup. Ifthis.config.get()can’t throw and returnsundefinedwhen missing, you can derivefoundfrom the retrieved value and skiphas()(unlesshas()has different semantics).Possible simplification (if `get()` returns `string | undefined` and doesn’t throw)
async execute(input: Input): Promise<Output> { - const found = this.config.has(input.key); - // Use default value only if provided, otherwise get raw value - const value = - input.defaultValue !== undefined ? this.config.get(input.key, input.defaultValue) : this.config.get(input.key); + const value = + input.defaultValue !== undefined + ? this.config.get(input.key, input.defaultValue) + : this.config.get(input.key); + const found = value !== undefined || this.config.has(input.key); return { key: input.key, value: value ?? null, found, defaultUsed: !found && input.defaultValue !== undefined, }; }libs/sdk/src/builtin/config/providers/__tests__/env-loader.test.ts (1)
324-370: Consider improving environment restoration in tests.The
afterEachhook deletes test variables but doesn't restore the original environment state captured inoriginalEnv(line 325). While this likely works fine because the tests use unique variable names, a more robust approach would fully restoreprocess.envto prevent any potential test pollution.♻️ Proposed enhancement
describe('populateProcessEnv', () => { const originalEnv = { ...process.env }; beforeEach(() => { // Clean up test vars delete process.env.TEST_POPULATE_VAR; delete process.env.TEST_EXISTING_VAR; delete process.env.TEST_OVERRIDE_VAR; }); afterEach(() => { - // Restore original env + // Clean up test vars and restore original env delete process.env.TEST_POPULATE_VAR; delete process.env.TEST_EXISTING_VAR; delete process.env.TEST_OVERRIDE_VAR; + delete process.env.TEST_VAR_A; + delete process.env.TEST_VAR_B; });libs/cli/src/utils/env.ts (1)
16-62: Code duplication between CLI and SDK implementations.The
parseEnvContentfunction is duplicated betweenlibs/cli/src/utils/env.tsandlibs/sdk/src/builtin/config/providers/env-loader.ts(lines 17-60). Both implementations are nearly identical. Consider extracting this to a shared utility or having one implementation reference the other to maintain DRY principles.The duplication includes:
- Line-by-line parsing logic
- Quote handling
- Escape sequence expansion
- Comment and empty line skipping
Since both CLI and SDK need this functionality, consider:
- Making the SDK version the canonical implementation and having CLI import it, or
- Creating a shared package if cross-dependency is a concern
libs/sdk/src/builtin/config/providers/env-loader.ts (1)
17-60: Code duplication with CLI implementation.This
parseEnvContentimplementation is duplicated inlibs/cli/src/utils/env.ts(lines 16-59). Consider consolidating to a single canonical implementation to maintain DRY principles and reduce maintenance burden.Since this SDK implementation uses the same logic as the CLI version, consider making this the canonical implementation and having the CLI import from the SDK, or vice versa depending on dependency preferences.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/e2e/demo-e2e-config/src/apps/config/tools/get-config.tool.tslibs/cli/src/utils/env.tslibs/sdk/src/builtin/config/providers/__tests__/env-loader.test.tslibs/sdk/src/builtin/config/providers/env-loader.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use strict TypeScript mode with noanytypes without strong justification
Avoid non-null assertions (!) - use proper error handling and type guards instead
Use@frontmcp/utilsfor cryptographic operations instead ofnode:cryptodirectly
Use@frontmcp/utilsfor file system operations instead offs/promisesornode:fsdirectly
Files:
apps/e2e/demo-e2e-config/src/apps/config/tools/get-config.tool.tslibs/sdk/src/builtin/config/providers/__tests__/env-loader.test.tslibs/sdk/src/builtin/config/providers/env-loader.tslibs/cli/src/utils/env.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.{ts,tsx}: Achieve 95%+ test coverage across all metrics (statements, branches, functions, lines)
Use Jest for testing with 95%+ coverage requirement and 100% test pass rate
Do not use prefixes like 'PT-001' in test names
Do not skip constructor validation tests for error classes and types
Includeinstanceofchecks in tests for error classes to verify proper error hierarchy
Files:
libs/sdk/src/builtin/config/providers/__tests__/env-loader.test.ts
libs/{sdk,adapters}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
libs/{sdk,adapters}/**/*.{ts,tsx}: Use specific error classes with MCP error codes instead of generic errors
UsegetCapabilities()for dynamic capability exposure instead of hardcoding capabilities
Files:
libs/sdk/src/builtin/config/providers/__tests__/env-loader.test.tslibs/sdk/src/builtin/config/providers/env-loader.ts
libs/sdk/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
libs/sdk/**/*.{ts,tsx}: Preferinterfacefor defining object shapes in TypeScript, avoidanytypes
Use type parameters with constraints instead of unconstrained generics withanydefaults
Validate URIs per RFC 3986 at metadata level usingisValidMcpUrirefinement
UsechangeScopeinstead ofscopein change event properties to avoid confusion with Scope class
Fail fast on invalid hook flows by validating hooks match their entry type
Centralize record types in common/records and import from there, not from module-specific files
Return strictly typed MCP protocol responses (e.g.,Promise<GetPromptResult>) instead ofPromise<unknown>for MCP entry methods
Useunknowninstead ofanyfor generic type parameter defaults (not for MCP protocol types)
Create shared base classes for common functionality like ExecutionContextBase for ToolContext and ResourceContext
Do not mutate rawInput in flows - use state.set() for managing flow state instead
Validate inputs and outputs through parseOutput/safeParseOutput methods in validation flows
Files:
libs/sdk/src/builtin/config/providers/__tests__/env-loader.test.tslibs/sdk/src/builtin/config/providers/env-loader.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/builtin/config/providers/__tests__/env-loader.test.tslibs/sdk/src/builtin/config/providers/env-loader.tslibs/cli/src/utils/env.ts
🧠 Learnings (9)
📚 Learning: 2026-01-06T02:34:55.689Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/plugins/CLAUDE.md:0-0
Timestamp: 2026-01-06T02:34:55.689Z
Learning: Applies to libs/plugins/**/*.ts : Extend tool metadata using `declare global` pattern to allow tools to specify plugin-specific options in their decorators
Applied to files:
apps/e2e/demo-e2e-config/src/apps/config/tools/get-config.tool.ts
📚 Learning: 2026-01-04T14:35:18.366Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/uipack/CLAUDE.md:0-0
Timestamp: 2026-01-04T14:35:18.366Z
Learning: Applies to libs/uipack/**/{theme,adapters,bundler}/**/*.{test,spec}.{ts,tsx,js,jsx} : Test behavior across all supported platform configurations (OpenAI, Claude, etc.)
Applied to files:
libs/sdk/src/builtin/config/providers/__tests__/env-loader.test.ts
📚 Learning: 2026-01-06T17:16:04.304Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T17:16:04.304Z
Learning: Applies to **/*.test.{ts,tsx} : Use Jest for testing with 95%+ coverage requirement and 100% test pass rate
Applied to files:
libs/sdk/src/builtin/config/providers/__tests__/env-loader.test.ts
📚 Learning: 2026-01-06T02:34:55.689Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/plugins/CLAUDE.md:0-0
Timestamp: 2026-01-06T02:34:55.689Z
Learning: Applies to libs/plugins/**/*.ts : Never hardcode encryption keys; use environment variables for key management
Applied to files:
libs/sdk/src/builtin/config/providers/__tests__/env-loader.test.tslibs/sdk/src/builtin/config/providers/env-loader.tslibs/cli/src/utils/env.ts
📚 Learning: 2025-12-24T00:41:41.819Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-12-24T00:41:41.819Z
Learning: Applies to libs/ui/src/**/*.test.{ts,tsx} : Use React Testing Library for component tests and include SSR/hydration tests for all interactive components
Applied to files:
libs/sdk/src/builtin/config/providers/__tests__/env-loader.test.ts
📚 Learning: 2026-01-06T17:16:04.304Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T17:16:04.304Z
Learning: Applies to **/*.test.{ts,tsx} : Achieve 95%+ test coverage across all metrics (statements, branches, functions, lines)
Applied to files:
libs/sdk/src/builtin/config/providers/__tests__/env-loader.test.ts
📚 Learning: 2025-12-24T00:41:41.819Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-12-24T00:41:41.819Z
Learning: Applies to libs/ui/src/**/*.test.{ts,tsx} : Test coverage must be 95%+ across statements, branches, functions, and lines
Applied to files:
libs/sdk/src/builtin/config/providers/__tests__/env-loader.test.ts
📚 Learning: 2026-01-04T14:35:18.366Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/uipack/CLAUDE.md:0-0
Timestamp: 2026-01-04T14:35:18.366Z
Learning: Applies to libs/uipack/**/{src,tests}/**/*.{ts,tsx,js,jsx} : Maintain 95%+ code coverage across statements, branches, functions, and lines
Applied to files:
libs/sdk/src/builtin/config/providers/__tests__/env-loader.test.ts
📚 Learning: 2026-01-06T02:34:55.689Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/plugins/CLAUDE.md:0-0
Timestamp: 2026-01-06T02:34:55.689Z
Learning: Applies to libs/plugins/**/*.test.ts : Tests must achieve 95%+ code coverage across all metrics; use `MockStore` implementing `RememberStoreInterface` for RememberPlugin testing
Applied to files:
libs/sdk/src/builtin/config/providers/__tests__/env-loader.test.ts
🧬 Code graph analysis (2)
apps/e2e/demo-e2e-config/src/apps/config/tools/get-config.tool.ts (5)
libs/cli/src/templates/3rd-party-integration/src/tools/example.list.ts (3)
inputSchema(12-17)Input(19-19)Output(41-41)apps/e2e/demo-e2e-config/src/apps/config/tools/check-config.tool.ts (1)
Tool(23-43)apps/e2e/demo-e2e-config/src/apps/config/tools/get-all-config.tool.ts (1)
Tool(15-41)apps/e2e/demo-e2e-config/src/apps/config/tools/get-required-config.tool.ts (1)
Tool(20-44)apps/e2e/demo-e2e-config/src/apps/config/tools/test-config-fallback.tool.ts (1)
Tool(41-108)
libs/sdk/src/builtin/config/providers/__tests__/env-loader.test.ts (2)
libs/sdk/src/builtin/config/providers/env-loader.ts (8)
pathToEnvKey(140-142)setNestedValue(152-183)getNestedValue(192-217)mapEnvToNestedConfig(294-310)parseEnvContent(17-60)populateProcessEnv(110-116)loadEnvFiles(71-93)extractSchemaPaths(225-261)libs/cli/src/utils/env.ts (2)
parseEnvContent(20-62)populateProcessEnv(104-110)
🪛 ast-grep (0.40.4)
libs/sdk/src/builtin/config/providers/env-loader.ts
[warning] 51-51: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(PLACEHOLDER, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
libs/cli/src/utils/env.ts
[warning] 53-53: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(PLACEHOLDER, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🪛 Gitleaks (8.30.0)
libs/sdk/src/builtin/config/providers/__tests__/env-loader.test.ts
[high] 400-400: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ 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 (9)
apps/e2e/demo-e2e-config/src/apps/config/tools/get-config.tool.ts (1)
1-3: Imports look good and lightweight.libs/sdk/src/builtin/config/providers/__tests__/env-loader.test.ts (2)
15-509: Comprehensive test coverage achieved.The test suite provides excellent coverage of all env-loader functions including edge cases, error handling, and security concerns (prototype pollution). The tests are well-structured and follow Jest best practices.
395-403: Gitleaks false positive - test data, not a real API key.The static analysis tool flagged
'local-override-key'as a potential API key, but this is test data used to verify environment variable override behavior. This is a false positive and can be safely ignored.libs/cli/src/utils/env.ts (2)
4-4: Consider alignment with coding guidelines for file system operations.The coding guidelines specify using
@frontmcp/utilsfor file system operations instead offs/promisesornode:fsdirectly. However, this CLI utility requires synchronous operations (fs.existsSync,fs.readFileSync) which may not be available in@frontmcp/utils.If
@frontmcp/utilsprovides synchronous file operation utilities suitable for CLI use, consider updating to align with guidelines. Otherwise, this exception is reasonable given the CLI's synchronous requirements.As per coding guidelines: "Use
@frontmcp/utilsfor file system operations instead offs/promisesornode:fsdirectly"
48-54: Static analysis false positive - PLACEHOLDER is a constant string.The ast-grep warning about RegExp constructed from variable input is a false positive. The
PLACEHOLDERvariable (line 48) is a compile-time constant string'\x00BACKSLASH\x00', not user input, so there's no ReDoS risk.libs/sdk/src/builtin/config/providers/env-loader.ts (4)
1-3: Correct use of @frontmcp/utils for file operations.The implementation correctly uses
@frontmcp/utilsfor file system operations (readFile,fileExists) as specified in the coding guidelines.As per coding guidelines: "Use
@frontmcp/utilsfor file system operations instead offs/promisesornode:fsdirectly"
122-183: Excellent prototype pollution protection.The implementation includes robust security measures against prototype pollution attacks:
- UNSAFE_KEYS validation (line 126)
- Dual validation (isSafeKey + explicit checks)
- Use of
Object.create(null)for nested objects (line 172)- Comprehensive documentation
This defense-in-depth approach is exemplary.
225-261: Justified use ofanyfor Zod internal inspection.The use of
anytypes witheslint-disablecomments (lines 230-231, 244, 269) is justified here because:
- The code needs to inspect Zod's internal structure (
_def,_zod.def,shape)- Zod doesn't expose stable typed APIs for these internals
- The code handles both Zod v3 and v4 compatibility
- The usage is localized and well-documented
This is an acceptable exception to the strict TypeScript guidelines.
As per coding guidelines: "Use strict TypeScript mode with no
anytypes without strong justification"Also applies to: 267-284
46-52: Static analysis false positive - PLACEHOLDER is a constant string.The ast-grep warning about RegExp constructed from variable input is a false positive. The
PLACEHOLDERvariable (line 46) is a compile-time constant string'\x00BACKSLASH\x00', not user input, so there's no ReDoS vulnerability.
…s and modify build script for plugin configuration
…s and modify build script for plugin configuration
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
libs/cli/e2e/run-e2e.sh (3)
76-87: Usemapfilefor more robust array population.The current command substitution approach is vulnerable to word-splitting issues if package names contain spaces or special characters. Additionally, the
2>/dev/nullsuppresses errors that could indicatenxcommand failures.♻️ More robust implementation using mapfile
-LIBS_PACKAGES=($(npx nx show projects --json 2>/dev/null | node --input-type=module -e " +mapfile -t LIBS_PACKAGES < <(npx nx show projects --json | node --input-type=module -e " import fs from 'fs'; const data = JSON.parse(fs.readFileSync('/dev/stdin', 'utf8')); const libs = data.filter(p => { if (p.startsWith('demo')) return false; if (p.startsWith('plugin-')) return false; if (p.startsWith('@')) return false; if (p === 'plugins') return false; return true; }); console.log(libs.join(' ')); -")) +" 2>&1 || { echo "❌ Failed to discover libs packages"; exit 1; })This approach:
- Uses process substitution with
mapfileto avoid word-splitting (addresses SC2207)- Preserves error messages from
nxcommand- Adds explicit error handling if the discovery fails
90-90: Usemapfilefor more robust array population.Same word-splitting concern as the LIBS_PACKAGES discovery above.
♻️ More robust implementation using mapfile
-PLUGIN_PACKAGES=($(npx nx show projects --projects "plugin-*" 2>/dev/null)) +mapfile -t PLUGIN_PACKAGES < <(npx nx show projects --projects "plugin-*" 2>&1 || { echo "❌ Failed to discover plugin packages"; exit 1; })
92-93: Validate that package discovery succeeded.After dynamic discovery, the script should verify that the arrays are not empty before proceeding. If discovery fails silently, subsequent build and publish steps will produce confusing failures.
✅ Add validation after discovery
echo " Libs: ${LIBS_PACKAGES[*]}" echo " Plugins: ${PLUGIN_PACKAGES[*]}" + +# Validate discovery succeeded +if [ ${#LIBS_PACKAGES[@]} -eq 0 ]; then + echo "❌ No libs packages discovered" + exit 1 +fi + +if [ ${#PLUGIN_PACKAGES[@]} -eq 0 ]; then + echo "❌ No plugin packages discovered" + exit 1 +fi
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/push.ymllibs/cli/e2e/run-e2e.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/push.yml
🧰 Additional context used
📓 Path-based instructions (1)
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/cli/e2e/run-e2e.sh
🧠 Learnings (8)
📚 Learning: 2026-01-06T17:16:04.304Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T17:16:04.304Z
Learning: Nx-based monorepo build system - use commands like `nx build sdk`, `nx test ast-guard`, `nx run-many -t test`
Applied to files:
libs/cli/e2e/run-e2e.sh
📚 Learning: 2026-01-04T14:35:18.366Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/uipack/CLAUDE.md:0-0
Timestamp: 2026-01-04T14:35:18.366Z
Learning: Applies to libs/uipack/**/{theme,adapters,bundler}/**/*.{test,spec}.{ts,tsx,js,jsx} : Test behavior across all supported platform configurations (OpenAI, Claude, etc.)
Applied to files:
libs/cli/e2e/run-e2e.sh
📚 Learning: 2026-01-06T02:34:55.689Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/plugins/CLAUDE.md:0-0
Timestamp: 2026-01-06T02:34:55.689Z
Learning: Applies to libs/plugins/**/*.ts : Use proper ES module imports instead of `require()` for SDK imports; avoid dynamic require of `frontmcp/sdk` modules
Applied to files:
libs/cli/e2e/run-e2e.sh
📚 Learning: 2026-01-06T17:16:04.304Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T17:16:04.304Z
Learning: Applies to libs/{sdk,adapters}/**/*.{ts,tsx} : Use `getCapabilities()` for dynamic capability exposure instead of hardcoding capabilities
Applied to files:
libs/cli/e2e/run-e2e.sh
📚 Learning: 2026-01-04T14:35:18.366Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/uipack/CLAUDE.md:0-0
Timestamp: 2026-01-04T14:35:18.366Z
Learning: Applies to libs/uipack/**/{theme,build,bundler}/**/*.{ts,tsx,js,jsx} : Do not hard-code CDN URLs - use theme.cdn configuration instead
Applied to files:
libs/cli/e2e/run-e2e.sh
📚 Learning: 2026-01-04T14:35:18.366Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/uipack/CLAUDE.md:0-0
Timestamp: 2026-01-04T14:35:18.366Z
Learning: Applies to libs/uipack/**/index.{ts,js} : Export all public APIs through appropriate entry points (frontmcp/uipack, frontmcp/uipack/adapters, frontmcp/uipack/theme, etc.)
Applied to files:
libs/cli/e2e/run-e2e.sh
📚 Learning: 2026-01-04T14:35:18.366Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/uipack/CLAUDE.md:0-0
Timestamp: 2026-01-04T14:35:18.366Z
Learning: Applies to libs/uipack/**/{src,tests}/**/*.{ts,tsx,js,jsx} : Maintain 95%+ code coverage across statements, branches, functions, and lines
Applied to files:
libs/cli/e2e/run-e2e.sh
📚 Learning: 2026-01-04T14:35:18.366Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/uipack/CLAUDE.md:0-0
Timestamp: 2026-01-04T14:35:18.366Z
Learning: Applies to libs/uipack/**/{package.json,*.ts,*.tsx,*.js,*.jsx} : Do not add React dependencies to frontmcp/uipack - it must remain React-free. Use frontmcp/ui for React components.
Applied to files:
libs/cli/e2e/run-e2e.sh
🪛 Shellcheck (0.11.0)
libs/cli/e2e/run-e2e.sh
[warning] 76-87: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
[warning] 90-90: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
⏰ 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). (4)
- GitHub Check: Lint & Format Checks
- GitHub Check: Build Libraries
- GitHub Check: Package E2E (Verdaccio)
- GitHub Check: Analyze (javascript-typescript)
Summary by CodeRabbit
New Features
User-visible Tools
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.