Repository navigation
chore: migrate worker and mocks into nx packages - #22
Conversation
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR converts the repo to an Nx monorepo layout: moving Worker code to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Duplicate env key appended instead of replaced
- Replaced or appended the COOKIE_SECRET entry via a helper so empty existing keys are updated instead of duplicated.
- ✅ Fixed: New
getConfigArgduplicates existinggetPortArglogic- Extracted shared argument parsing into a reusable helper and reused it for both flags.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
mcp/capabilities/meta/meta-run-skill.ts (1)
36-36: Cosmetic change unrelated to monorepo migration.The formatting change to the description string (unescaping the JSON example quotes) improves readability, but this modification doesn't appear related to the PR's stated objective of migrating to an Nx monorepo structure.
If this was an incidental change during the migration, consider whether it should be in a separate commit or PR for clarity.
💅 Optional: Consider using a template literal for improved readability
For long, multi-sentence strings with embedded examples, template literals can improve maintainability:
- 'Execute a saved skill\'s codemode in the same sandbox as the MCP execute tool. When the skill defines parameters, pass them in params; the code receives them via the params variable or the first function argument. Example: meta_run_skill({ "skill_id": "<id>", "params": { "owner": "kentcdodds", "days": 3 } }). On failure, the structured result includes a hint for updating the skill (meta_update_skill).', + `Execute a saved skill's codemode in the same sandbox as the MCP execute tool. When the skill defines parameters, pass them in params; the code receives them via the params variable or the first function argument. Example: meta_run_skill({ "skill_id": "<id>", "params": { "owner": "kentcdodds", "days": 3 } }). On failure, the structured result includes a hint for updating the skill (meta_update_skill).`,This eliminates the need to escape the apostrophe in "skill's" while maintaining the same output.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp/capabilities/meta/meta-run-skill.ts` at line 36, The description string for meta_run_skill was cosmetically changed (unescaped example quotes) which is unrelated to the monorepo migration; either revert that string back to its original formatting or move this cosmetic change into its own commit/PR for separation of concerns, and if you prefer readability use a template literal for the description (the string assigned to the meta_run_skill capability) so you can avoid escaping the apostrophe and keep the same output.packages/worker/src/ai-runtime.ts (1)
9-14: LGTM! Import paths correctly updated for monorepo structure.The relative import paths are accurate for the new
packages/worker/src/location, correctly navigating up three directory levels to reach the root-levelshared/directory.💡 Optional: Consider preserving path aliases for maintainability
While the relative paths are correct, moving from path aliases (
#shared/*) to relative imports (../../../shared/*) can make the codebase more fragile when files are relocated. If the build tooling supports it, you might consider configuring path aliases in the Nx/TypeScript configuration to maintain cleaner, more maintainable import statements across the monorepo.This is purely a maintenance consideration and doesn't affect correctness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/ai-runtime.ts` around lines 9 - 14, No functional change required—the imports in ai-runtime.ts (the three relative imports: getRemoteAiLocalDevCredentialsError, AiMode, buildMockAiScenario/MockAiResponse) are correct for the new packages/worker/src location; leave them as-is but optionally consider restoring a path alias (e.g., '#shared/*') in the monorepo TypeScript/Nx config to simplify imports and prevent fragility if files move.packages/mock-servers/ai/package.json (1)
1-5: LGTM! Minimal but appropriate package manifest.The package manifest correctly configures the mock AI server as a private ES module package with a properly scoped name (
@kody/mock-ai). This is suitable for an internal monorepo package.📦 Optional: Consider adding descriptive metadata
For better developer experience, you might consider adding optional fields like:
{ "name": "@kody/mock-ai", "version": "0.0.0", "description": "Mock AI server for local development", "private": true, "type": "module" }This is entirely optional and doesn't affect functionality, but can help document the package's purpose.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/mock-servers/ai/package.json` around lines 1 - 5, Add optional descriptive metadata to the package manifest by updating the JSON object to include "version" and "description" fields alongside the existing "name", "private", and "type" keys; specifically, add a "version" (e.g., "0.0.0") and a short "description" (e.g., "Mock AI server for local development") to package.json to improve developer discoverability while keeping "private": true and "type": "module" unchanged.tools/prepare-e2e-env.ts (1)
7-16: Consider handling quoted values in dotenv parsing.The
parseDotenvValuehelper doesn't strip surrounding quotes from values. If.envcontainsCOOKIE_SECRET="some-value"orCOOKIE_SECRET='some-value', the returned value will include the quotes, which could cause issues when the secret is used.🔧 Optional fix to handle quoted values
function parseDotenvValue(content: string, key: string) { for (const rawLine of content.split(/\r?\n/)) { const line = rawLine.trim() if (!line || line.startsWith('#')) continue const withoutExport = line.startsWith('export ') ? line.slice(7) : line if (!withoutExport.startsWith(`${key}=`)) continue - return withoutExport.slice(key.length + 1).trim() + const value = withoutExport.slice(key.length + 1).trim() + if ((value.startsWith('"') && value.endsWith('"')) || + (value.startsWith("'") && value.endsWith("'"))) { + return value.slice(1, -1) + } + return value } return null }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/prepare-e2e-env.ts` around lines 7 - 16, The parseDotenvValue function currently returns values with surrounding quotes intact (e.g., COOKIE_SECRET="x"), causing downstream issues; update parseDotenvValue to detect and strip matching surrounding single or double quotes from the extracted value (and unescape common escaped characters if present) before returning, making sure to still trim whitespace and handle empty/null correctly so callers of parseDotenvValue receive the actual unquoted secret.packages/worker/src/oauth-handlers.ts (1)
5-14: Same import path strategy concern asindex.ts.These imports were also changed from alias-based (
#server/*) to relative paths (../../../server/*), creating the same fragility and maintenance concerns discussed inpackages/worker/src/index.ts.The
../../../paths make the code brittle to directory restructuring. Consider either:
- Using the defined aliases consistently for cleaner imports, OR
- Removing the unused aliases from
package.jsonif the team prefers relative importsSee the detailed verification script in the
index.tsreview comment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/oauth-handlers.ts` around lines 5 - 14, The imports in oauth-handlers.ts use brittle relative paths (e.g., '../../../server/auth-session.ts')—update them to use the project path alias (e.g., import { readAuthSessionResult, setAuthSessionSecret } from '#server/auth-session') for all referenced symbols (getRequestIp, logAuditEvent, readAuthSessionResult, setAuthSessionSecret, getEnv, toHex, verifyPassword, Layout, render) so they match the rest of the codebase; alternatively, if the team intentionally prefers relative imports, remove/adjust the path aliases in package.json/tsconfig so the alias is not expected—pick one approach and apply it consistently across oauth-handlers.ts (and mirror the same change made to index.ts).packages/worker/src/index.ts (1)
3-7: Update imports to use the defined aliases for consistency.The imports use relative paths (
../../../...) while other files across the codebase consistently use the defined aliases (#worker/*,#sentry/*,#mcp/*,#shared/*,#server/*). This file should follow the same pattern for consistency and to avoid brittle relative paths.Change lines 3-7 to:
import { getWorkerSentryOptions } from '#sentry/cloudflare-options.ts' import { ChatAgent } from '#worker/chat-agent.ts' import { MCP } from '#mcp/index.ts' import { chatAgentBasePath } from '#shared/chat-routes.ts' import { handleRequest } from '#server/handler.ts'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/index.ts` around lines 3 - 7, Replace the brittle relative imports with the project alias imports: update the import statements that reference getWorkerSentryOptions, ChatAgent, MCP, chatAgentBasePath, and handleRequest to use the aliases (`#sentry/`*, `#worker/`*, `#mcp/`*, `#shared/`*, `#server/`*) instead of ../../../ paths so they match the rest of the codebase and resolve consistently.wrangler-env.ts (1)
186-201: Consider adding a notice when overwriting the config directory.env.The
syncDotenvForConfigfunction silently overwrites any existing.envfile in the config directory (e.g.,packages/worker/.env). If a developer has customized that file, their changes would be lost without warning.This might be intentional (always using root
.envas source of truth), but consider either:
- Logging a notice when copying (so developers know it happened)
- Only copying if the target doesn't exist or is older
💡 Optional: Add a notice when copying
function syncDotenvForConfig(configPath: string | undefined) { if (!configPath) return const rootEnvPath = path.join(process.cwd(), '.env') if (!existsSync(rootEnvPath)) return const resolvedConfigPath = path.resolve(process.cwd(), configPath) const configDir = path.dirname(resolvedConfigPath) const configEnvPath = path.join(configDir, '.env') if (path.resolve(configEnvPath) === path.resolve(rootEnvPath)) { return } + console.error(`Syncing .env to ${configEnvPath}`) copyFileSync(rootEnvPath, configEnvPath) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wrangler-env.ts` around lines 186 - 201, The syncDotenvForConfig function currently unconditionally overwrites the config directory .env via copyFileSync; update it to avoid silent overwrites by either (A) logging a clear notice via the existing logger before copying when configEnvPath already exists, or (B) only performing the copy when configEnvPath does not exist or when rootEnvPath is newer than configEnvPath (compare mtimes) — modify syncDotenvForConfig to check existsSync(configEnvPath) and statSync(...).mtime as appropriate and use processLogger (or the module logger) to emit a concise message referencing rootEnvPath and configEnvPath before calling copyFileSync.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@types/worker-configuration.d.ts`:
- Line 28: The generated ProcessEnv typing was narrowed to only
AI_MODE/AI_MODEL/SENTRY_ENVIRONMENT which removed required keys like
COOKIE_SECRET used in types/env-schema.ts and server/handler.ts; restore the
missing env keys by either (a) updating the wrangler config / types input and
regenerating the declarations so ProcessEnv (the interface using
StringifyValues<Pick<Cloudflare.Env, ...>>) includes COOKIE_SECRET and any other
required vars, or (b) add a stable ambient augmentation file that extends the
ProcessEnv interface (or Cloudflare.Env) to declare COOKIE_SECRET and other
non-wrangler-managed secrets so the type contract matches types/env-schema.ts
and usages in server/handler.ts.
---
Nitpick comments:
In `@mcp/capabilities/meta/meta-run-skill.ts`:
- Line 36: The description string for meta_run_skill was cosmetically changed
(unescaped example quotes) which is unrelated to the monorepo migration; either
revert that string back to its original formatting or move this cosmetic change
into its own commit/PR for separation of concerns, and if you prefer readability
use a template literal for the description (the string assigned to the
meta_run_skill capability) so you can avoid escaping the apostrophe and keep the
same output.
In `@packages/mock-servers/ai/package.json`:
- Around line 1-5: Add optional descriptive metadata to the package manifest by
updating the JSON object to include "version" and "description" fields alongside
the existing "name", "private", and "type" keys; specifically, add a "version"
(e.g., "0.0.0") and a short "description" (e.g., "Mock AI server for local
development") to package.json to improve developer discoverability while keeping
"private": true and "type": "module" unchanged.
In `@packages/worker/src/ai-runtime.ts`:
- Around line 9-14: No functional change required—the imports in ai-runtime.ts
(the three relative imports: getRemoteAiLocalDevCredentialsError, AiMode,
buildMockAiScenario/MockAiResponse) are correct for the new packages/worker/src
location; leave them as-is but optionally consider restoring a path alias (e.g.,
'#shared/*') in the monorepo TypeScript/Nx config to simplify imports and
prevent fragility if files move.
In `@packages/worker/src/index.ts`:
- Around line 3-7: Replace the brittle relative imports with the project alias
imports: update the import statements that reference getWorkerSentryOptions,
ChatAgent, MCP, chatAgentBasePath, and handleRequest to use the aliases
(`#sentry/`*, `#worker/`*, `#mcp/`*, `#shared/`*, `#server/`*) instead of ../../../ paths
so they match the rest of the codebase and resolve consistently.
In `@packages/worker/src/oauth-handlers.ts`:
- Around line 5-14: The imports in oauth-handlers.ts use brittle relative paths
(e.g., '../../../server/auth-session.ts')—update them to use the project path
alias (e.g., import { readAuthSessionResult, setAuthSessionSecret } from
'#server/auth-session') for all referenced symbols (getRequestIp, logAuditEvent,
readAuthSessionResult, setAuthSessionSecret, getEnv, toHex, verifyPassword,
Layout, render) so they match the rest of the codebase; alternatively, if the
team intentionally prefers relative imports, remove/adjust the path aliases in
package.json/tsconfig so the alias is not expected—pick one approach and apply
it consistently across oauth-handlers.ts (and mirror the same change made to
index.ts).
In `@tools/prepare-e2e-env.ts`:
- Around line 7-16: The parseDotenvValue function currently returns values with
surrounding quotes intact (e.g., COOKIE_SECRET="x"), causing downstream issues;
update parseDotenvValue to detect and strip matching surrounding single or
double quotes from the extracted value (and unescape common escaped characters
if present) before returning, making sure to still trim whitespace and handle
empty/null correctly so callers of parseDotenvValue receive the actual unquoted
secret.
In `@wrangler-env.ts`:
- Around line 186-201: The syncDotenvForConfig function currently
unconditionally overwrites the config directory .env via copyFileSync; update it
to avoid silent overwrites by either (A) logging a clear notice via the existing
logger before copying when configEnvPath already exists, or (B) only performing
the copy when configEnvPath does not exist or when rootEnvPath is newer than
configEnvPath (compare mtimes) — modify syncDotenvForConfig to check
existsSync(configEnvPath) and statSync(...).mtime as appropriate and use
processLogger (or the module logger) to emit a concise message referencing
rootEnvPath and configEnvPath before calling copyFileSync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 072a557a-29aa-4496-8b04-7cbd465ddbdd
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (71)
.cursor/CLOUD.md.github/workflows/deploy.yml.github/workflows/preview.yml.gitignoreREADME.mddocs/agents/cloudflare-agents-sdk.mddocs/agents/end-to-end-testing.mddocs/agents/mcp-apps-starter-guide.mddocs/agents/mock-api-servers.mddocs/agents/remix/data-table-sqlite.mddocs/agents/remix/data-table.mddocs/agents/remix/index.mddocs/agents/setup.mddocs/agents/typescript-setup.mddocs/architecture/authentication.mddocs/architecture/data-storage.mddocs/architecture/index.mddocs/architecture/request-lifecycle.mddocs/cloudflare-offerings.mddocs/environment-variables.mddocs/getting-started.mddocs/setup-manifest.mdmcp/capabilities/coding/coding-capabilities.test.tsmcp/capabilities/meta/meta-run-skill.tsmcp/cursor/cursor-cloud-client.test.tsmcp/github/github-client.test.tsmcp/github/github-graphql-client.test.tsmcp/mcp-server-e2e.test.tsnx.jsonpackage.jsonpackages/mock-servers/ai/package.jsonpackages/mock-servers/ai/project.jsonpackages/mock-servers/ai/src/worker.tspackages/mock-servers/ai/wrangler.jsoncpackages/mock-servers/cursor/package.jsonpackages/mock-servers/cursor/project.jsonpackages/mock-servers/cursor/src/worker.tspackages/mock-servers/cursor/wrangler.jsoncpackages/mock-servers/github/package.jsonpackages/mock-servers/github/project.jsonpackages/mock-servers/github/src/github-mock.test.tspackages/mock-servers/github/src/worker.tspackages/mock-servers/github/wrangler.jsoncpackages/mock-servers/resend/package.jsonpackages/mock-servers/resend/project.jsonpackages/mock-servers/resend/src/resend-mock.test.tspackages/mock-servers/resend/src/worker.tspackages/mock-servers/resend/wrangler.jsoncpackages/worker/package.jsonpackages/worker/project.jsonpackages/worker/src/ai-runtime.test.tspackages/worker/src/ai-runtime.tspackages/worker/src/capability-maintenance.tspackages/worker/src/chat-agent-routing.tspackages/worker/src/chat-agent.tspackages/worker/src/d1-data-table-adapter.tspackages/worker/src/db.tspackages/worker/src/index.tspackages/worker/src/mcp-auth.test.tspackages/worker/src/mcp-auth.tspackages/worker/src/oauth-handlers.test.tspackages/worker/src/oauth-handlers.tspackages/worker/src/utils.tspackages/worker/wrangler.jsonctools/ci/preview-resources.tstools/ci/production-resources.tstools/prepare-e2e-env.tstools/seed-test-data.test.tstypes/tsconfig-worker.jsontypes/worker-configuration.d.tswrangler-env.ts
| }; | ||
| declare namespace NodeJS { | ||
| interface ProcessEnv extends StringifyValues<Pick<Cloudflare.Env, "AI_MODE" | "AI_MODEL" | "SENTRY_ENVIRONMENT" | "COOKIE_SECRET" | "APP_BASE_URL" | "RESEND_API_BASE_URL" | "RESEND_API_KEY" | "RESEND_FROM_EMAIL" | "AI_GATEWAY_ID" | "CLOUDFLARE_API_TOKEN" | "CLOUDFLARE_ACCOUNT_ID" | "SENTRY_DSN">> {} | ||
| interface ProcessEnv extends StringifyValues<Pick<Cloudflare.Env, "AI_MODE" | "AI_MODEL" | "SENTRY_ENVIRONMENT">> {} |
There was a problem hiding this comment.
Restore required env keys in the generated typing contract.
Line 28 reflects a narrowed env surface, but the app still expects additional vars (for example COOKIE_SECRET is required in types/env-schema.ts and used in server/handler.ts). This creates type/runtime contract drift after the move.
Prefer fixing the source of generation (wrangler config/types input) and regenerating, or add a stable augmentation file for non-Wrangler-managed env keys so required secrets remain typed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@types/worker-configuration.d.ts` at line 28, The generated ProcessEnv typing
was narrowed to only AI_MODE/AI_MODEL/SENTRY_ENVIRONMENT which removed required
keys like COOKIE_SECRET used in types/env-schema.ts and server/handler.ts;
restore the missing env keys by either (a) updating the wrangler config / types
input and regenerating the declarations so ProcessEnv (the interface using
StringifyValues<Pick<Cloudflare.Env, ...>>) includes COOKIE_SECRET and any other
required vars, or (b) add a stable ambient augmentation file that extends the
ProcessEnv interface (or Cloudflare.Env) to declare COOKIE_SECRET and other
non-wrangler-managed secrets so the type contract matches types/env-schema.ts
and usages in server/handler.ts.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
|
🔎 Preview deployed: https://kody-pr-22.kentcdodds.workers.dev Worker: Mocks:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/prepare-e2e-env.ts`:
- Around line 7-31: parseDotenvValue and setDotenvValue currently only match
exact `KEY=` with no spaces and take the first match; update them to mirror
runtime parsing by allowing optional whitespace around `=` and after `export`
and by letting the last assignment win. In parseDotenvValue, change the logic to
accept lines like `export? <key>\\s*=\\s*value` (trim quotes if present) and
iterate all lines returning the last matched value instead of the first. In
setDotenvValue, adjust the keyPattern to allow optional spaces around the `=`
and `export`, and when replacing, target the last occurrence of that key (not
the first) so you overwrite the effective runtime value; if no match, append the
new `KEY=value` line as before. Reference functions: parseDotenvValue and
setDotenvValue.
- Around line 33-44: Current logic aborts if .env.example is missing before
checking .env and copies .env.example then exits before validating
COOKIE_SECRET; change flow to first check envPath (existsSync(envPath)) and only
consult/copy examplePath (existsSync(examplePath), copyFileSync(examplePath,
envPath)) when env is missing, and after ensuring env exists (original or
copied) run the COOKIE_SECRET presence/validation (the check around
COOKIE_SECRET at lines 52-58) so that having a pre-existing .env bypasses the
need for .env.example and copied files don’t short-circuit the validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bbb4b8b0-b7c9-45b1-8fe8-756f748a07b3
📒 Files selected for processing (2)
tools/prepare-e2e-env.tswrangler-env.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- wrangler-env.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wrangler-env.ts`:
- Around line 160-174: The getArgValue function currently uses
inlineArg.split('=') which drops any '=' characters in the value; change the
parsing to only split on the first '=' (e.g., find the first '=' with indexOf
and slice, or use split with a limit of 2) so that inlineArg yields the full
value (e.g., keep reference to getArgValue and the inlineArg variable), and
return undefined if no value after the first '=' as before.
- Around line 176-191: The syncDotenvForConfig function currently
unconditionally calls copyFileSync and can overwrite an existing package .env;
update syncDotenvForConfig to first check if configEnvPath exists and if so read
and compare its contents to rootEnvPath (using rootEnvPath and configEnvPath)
and only perform copyFileSync when the files differ; if the target exists and is
identical, do nothing, and if it exists and differs prefer skipping the
overwrite and emit a clear warning/log message (instead of silently copying) so
developers' custom .env entries (and vars declared in wrangler.jsonc) are
preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d5c3ec04-e08b-4b19-bd4c-3f3d670299f5
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
wrangler-env.ts
| function syncDotenvForConfig(configPath: string | undefined) { | ||
| if (!configPath) return | ||
|
|
||
| const rootEnvPath = path.join(process.cwd(), '.env') | ||
| if (!existsSync(rootEnvPath)) return | ||
|
|
||
| const resolvedConfigPath = path.resolve(process.cwd(), configPath) | ||
| const configDir = path.dirname(resolvedConfigPath) | ||
| const configEnvPath = path.join(configDir, '.env') | ||
|
|
||
| if (path.resolve(configEnvPath) === path.resolve(rootEnvPath)) { | ||
| return | ||
| } | ||
|
|
||
| copyFileSync(rootEnvPath, configEnvPath) | ||
| } |
There was a problem hiding this comment.
Unconditional overwrite may cause unexpected behavior.
The function overwrites packages/worker/.env every time the script runs, regardless of whether the target file already exists or has different content. This could silently discard a developer's custom .env in the worker package.
Additionally, per packages/worker/wrangler.jsonc (lines 22-76), the config already declares vars like AI_MODE and AI_MODEL. When wrangler loads both the copied .env and the hardcoded vars, the precedence may cause unexpected behavior depending on wrangler's loading order.
Consider:
- Skipping the copy if the target
.envalready exists (or logging a warning) - Comparing file contents before overwriting
🛡️ Optional: Skip copy if target exists
function syncDotenvForConfig(configPath: string | undefined) {
if (!configPath) return
const rootEnvPath = path.join(process.cwd(), '.env')
if (!existsSync(rootEnvPath)) return
const resolvedConfigPath = path.resolve(process.cwd(), configPath)
const configDir = path.dirname(resolvedConfigPath)
const configEnvPath = path.join(configDir, '.env')
if (path.resolve(configEnvPath) === path.resolve(rootEnvPath)) {
return
}
+ if (existsSync(configEnvPath)) {
+ // Target .env already exists; skip to avoid overwriting custom config
+ return
+ }
+
copyFileSync(rootEnvPath, configEnvPath)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function syncDotenvForConfig(configPath: string | undefined) { | |
| if (!configPath) return | |
| const rootEnvPath = path.join(process.cwd(), '.env') | |
| if (!existsSync(rootEnvPath)) return | |
| const resolvedConfigPath = path.resolve(process.cwd(), configPath) | |
| const configDir = path.dirname(resolvedConfigPath) | |
| const configEnvPath = path.join(configDir, '.env') | |
| if (path.resolve(configEnvPath) === path.resolve(rootEnvPath)) { | |
| return | |
| } | |
| copyFileSync(rootEnvPath, configEnvPath) | |
| } | |
| function syncDotenvForConfig(configPath: string | undefined) { | |
| if (!configPath) return | |
| const rootEnvPath = path.join(process.cwd(), '.env') | |
| if (!existsSync(rootEnvPath)) return | |
| const resolvedConfigPath = path.resolve(process.cwd(), configPath) | |
| const configDir = path.dirname(resolvedConfigPath) | |
| const configEnvPath = path.join(configDir, '.env') | |
| if (path.resolve(configEnvPath) === path.resolve(rootEnvPath)) { | |
| return | |
| } | |
| if (existsSync(configEnvPath)) { | |
| // Target .env already exists; skip to avoid overwriting custom config | |
| return | |
| } | |
| copyFileSync(rootEnvPath, configEnvPath) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wrangler-env.ts` around lines 176 - 191, The syncDotenvForConfig function
currently unconditionally calls copyFileSync and can overwrite an existing
package .env; update syncDotenvForConfig to first check if configEnvPath exists
and if so read and compare its contents to rootEnvPath (using rootEnvPath and
configEnvPath) and only perform copyFileSync when the files differ; if the
target exists and is identical, do nothing, and if it exists and differs prefer
skipping the overwrite and emit a clear warning/log message (instead of silently
copying) so developers' custom .env entries (and vars declared in
wrangler.jsonc) are preserved.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Misleading error message after control flow refactoring
- Updated the error message to only reference the missing .env.example file in the current control flow.
- ✅ Fixed: MCP e2e test bypasses dotenv sync for wrangler
- Synced the root .env into the worker config directory before each wrangler spawn in the MCP e2e tests.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
There was a problem hiding this comment.
♻️ Duplicate comments (2)
tools/prepare-e2e-env.ts (2)
33-44:⚠️ Potential issue | 🟠 MajorDon’t fail fast on missing
.env.examplebefore checking existing.env.Line 33 blocks valid setups where
.envalready hasCOOKIE_SECRET, and Line 43 exits success right after copy, skipping the validation path in Lines 52-58 for newly created.env.Suggested flow fix
-if (!existsSync(examplePath)) { - console.error( - 'Missing .env.example; cannot prepare E2E environment.', - ) - process.exit(1) -} - if (!existsSync(envPath)) { + if (!existsSync(examplePath)) { + console.error('Missing .env.example; cannot prepare E2E environment.') + process.exit(1) + } copyFileSync(examplePath, envPath) console.log('Created .env from .env.example for E2E tests.') - process.exit(0) } const envContents = readFileSync(envPath, 'utf8') const existingCookieSecret = parseDotenvValue(envContents, 'COOKIE_SECRET') if (existingCookieSecret && existingCookieSecret.length > 0) { process.exit(0) } +if (!existsSync(examplePath)) { + console.error('Missing .env.example; cannot prepare E2E environment.') + process.exit(1) +} const exampleContents = readFileSync(examplePath, 'utf8')Also applies to: 52-58
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/prepare-e2e-env.ts` around lines 33 - 44, Rework the control flow around examplePath and envPath so we don't fail fast: first check if envPath exists and validate it contains COOKIE_SECRET (reuse the existing validation logic present later in the file), and only if envPath is missing attempt to copy from examplePath; if examplePath is missing then fail, otherwise copy examplePath to envPath and continue to the same validation path (do not process.exit(0) immediately after copy). In short: change the order to prefer validating an existing envPath, remove the early exit after copy, and ensure the subsequent COOKIE_SECRET validation (the validation block referenced later in the file) runs for both preexisting and newly copied .env files.
7-31:⚠️ Potential issue | 🟠 MajorMatch dotenv parsing/update semantics to effective runtime values.
Line 12/20 require
KEY=(no spaces), and Line 13 returns the first match while dotenv-style configs can includeKEY = valueand later overrides. This can read/update the wrongCOOKIE_SECRET.Suggested fix
function parseDotenvValue(content: string, key: string) { - for (const rawLine of content.split(/\r?\n/)) { - const line = rawLine.trim() - if (!line || line.startsWith('#')) continue - const withoutExport = line.startsWith('export ') ? line.slice(7) : line - if (!withoutExport.startsWith(`${key}=`)) continue - return withoutExport.slice(key.length + 1).trim() - } - return null + const keyPattern = new RegExp( + `^\\s*(?:export\\s+)?${key}\\s*=\\s*(.*)$`, + ) + let lastValue: string | null = null + for (const rawLine of content.split(/\r?\n/)) { + const line = rawLine.trim() + if (!line || line.startsWith('#')) continue + const match = rawLine.match(keyPattern) + if (!match) continue + lastValue = match[1]?.trim() ?? '' + } + return lastValue } function setDotenvValue(content: string, key: string, value: string) { - const keyPattern = new RegExp( - `(^|\\r?\\n)(\\s*(?:export\\s+)?${key}=)[^\\r\\n]*`, - ) - if (keyPattern.test(content)) { - return content.replace( - keyPattern, - (_match, leading, prefix) => `${leading}${prefix}${value}`, - ) - } - return content.endsWith('\n') - ? `${content}${key}=${value}\n` - : `${content}\n${key}=${value}\n` + const lines = content.split(/\r?\n/) + const keyPattern = new RegExp(`^\\s*((?:export\\s+)?${key}\\s*=\\s*).*$`) + let lastIndex = -1 + let prefix = `${key}=` + for (let i = 0; i < lines.length; i++) { + const match = lines[i]?.match(keyPattern) + if (!match) continue + lastIndex = i + prefix = match[1] ?? prefix + } + if (lastIndex >= 0) { + lines[lastIndex] = `${prefix}${value}` + return `${lines.join('\n').replace(/\n*$/, '\n')}` + } + return `${content.replace(/\n*$/, '\n')}${key}=${value}\n` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/prepare-e2e-env.ts` around lines 7 - 31, parseDotenvValue and setDotenvValue currently require "KEY=" with no spaces and only act on the first match; change both to handle dotenv semantics: allow optional whitespace around the equals (e.g. "KEY = value"), support quoted values and trimming, and treat the last occurrence as the effective override (so parseDotenvValue should scan all lines and return the last matched value for the given key, and setDotenvValue should update the last occurrence if present). Update the matching logic in parseDotenvValue to use a regex that accepts optional spaces and captures the value (then trim surrounding quotes/spaces), and change setDotenvValue to search globally for matches of the same pattern and replace only the last match (preserving any "export " prefix and surrounding spacing), otherwise append a new "KEY=value" line.
🧹 Nitpick comments (1)
packages/worker/src/mcp/mcp-server-e2e.test.ts (1)
47-59: Consider reusing the shared dotenv sync helper to avoid drift.This function is effectively the same behavior as
wrangler-env.ts:176-191(syncDotenvForConfig). Duplicating this logic in tests increases maintenance risk when sync rules change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/mcp-server-e2e.test.ts` around lines 47 - 59, The syncDotenvForWranglerConfig function duplicates logic from the shared helper syncDotenvForConfig (in wrangler-env.ts); replace the local function by importing and calling the shared syncDotenvForConfig to keep behavior centralized and avoid drift—remove syncDotenvForWranglerConfig and add an import for syncDotenvForConfig, then invoke syncDotenvForConfig() where the local function was used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tools/prepare-e2e-env.ts`:
- Around line 33-44: Rework the control flow around examplePath and envPath so
we don't fail fast: first check if envPath exists and validate it contains
COOKIE_SECRET (reuse the existing validation logic present later in the file),
and only if envPath is missing attempt to copy from examplePath; if examplePath
is missing then fail, otherwise copy examplePath to envPath and continue to the
same validation path (do not process.exit(0) immediately after copy). In short:
change the order to prefer validating an existing envPath, remove the early exit
after copy, and ensure the subsequent COOKIE_SECRET validation (the validation
block referenced later in the file) runs for both preexisting and newly copied
.env files.
- Around line 7-31: parseDotenvValue and setDotenvValue currently require "KEY="
with no spaces and only act on the first match; change both to handle dotenv
semantics: allow optional whitespace around the equals (e.g. "KEY = value"),
support quoted values and trimming, and treat the last occurrence as the
effective override (so parseDotenvValue should scan all lines and return the
last matched value for the given key, and setDotenvValue should update the last
occurrence if present). Update the matching logic in parseDotenvValue to use a
regex that accepts optional spaces and captures the value (then trim surrounding
quotes/spaces), and change setDotenvValue to search globally for matches of the
same pattern and replace only the last match (preserving any "export " prefix
and surrounding spacing), otherwise append a new "KEY=value" line.
---
Nitpick comments:
In `@packages/worker/src/mcp/mcp-server-e2e.test.ts`:
- Around line 47-59: The syncDotenvForWranglerConfig function duplicates logic
from the shared helper syncDotenvForConfig (in wrangler-env.ts); replace the
local function by importing and calling the shared syncDotenvForConfig to keep
behavior centralized and avoid drift—remove syncDotenvForWranglerConfig and add
an import for syncDotenvForConfig, then invoke syncDotenvForConfig() where the
local function was used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ff01c3f8-b1e7-4ea3-803f-e2fb10107946
📒 Files selected for processing (86)
docs/agents/adding-capabilities.mddocs/agents/cloudflare-agents-sdk.mddocs/agents/mcp-apps-starter-guide.mddocs/agents/testing-principles.mddocs/agents/typescript-setup.mddocs/architecture/data-storage.mddocs/architecture/index.mddocs/architecture/request-lifecycle.mddocs/environment-variables.mddocs/mcp-server-patterns.mddocs/setup-manifest.mdpackage.jsonpackages/worker/package.jsonpackages/worker/src/capability-maintenance.tspackages/worker/src/chat-agent.tspackages/worker/src/index.tspackages/worker/src/mcp-auth.tspackages/worker/src/mcp/apps/calculator-ui-entry-point.tspackages/worker/src/mcp/capabilities/build-capability-registry.test.tspackages/worker/src/mcp/capabilities/build-capability-registry.tspackages/worker/src/mcp/capabilities/builtin-domains.tspackages/worker/src/mcp/capabilities/capability-reindex.tspackages/worker/src/mcp/capabilities/capability-search.test.tspackages/worker/src/mcp/capabilities/capability-search.tspackages/worker/src/mcp/capabilities/coding/coding-capabilities.test.tspackages/worker/src/mcp/capabilities/coding/cursor-cloud-agent-docs.tspackages/worker/src/mcp/capabilities/coding/cursor-cloud-rest.tspackages/worker/src/mcp/capabilities/coding/domain.tspackages/worker/src/mcp/capabilities/coding/fetch-markdown-doc.tspackages/worker/src/mcp/capabilities/coding/github-docs-path.tspackages/worker/src/mcp/capabilities/coding/github-graphql-api-docs.tspackages/worker/src/mcp/capabilities/coding/github-graphql.tspackages/worker/src/mcp/capabilities/coding/github-rest-api-docs.tspackages/worker/src/mcp/capabilities/coding/github-rest.tspackages/worker/src/mcp/capabilities/coding/index.tspackages/worker/src/mcp/capabilities/define-capability.tspackages/worker/src/mcp/capabilities/define-domain-capability.tspackages/worker/src/mcp/capabilities/define-domain.tspackages/worker/src/mcp/capabilities/domain-metadata.tspackages/worker/src/mcp/capabilities/math/do-math.tspackages/worker/src/mcp/capabilities/math/domain.tspackages/worker/src/mcp/capabilities/math/index.tspackages/worker/src/mcp/capabilities/meta/domain.tspackages/worker/src/mcp/capabilities/meta/meta-delete-skill.tspackages/worker/src/mcp/capabilities/meta/meta-get-skill.tspackages/worker/src/mcp/capabilities/meta/meta-run-skill.tspackages/worker/src/mcp/capabilities/meta/meta-save-skill.tspackages/worker/src/mcp/capabilities/meta/meta-update-skill.tspackages/worker/src/mcp/capabilities/meta/require-user.tspackages/worker/src/mcp/capabilities/registry.tspackages/worker/src/mcp/capabilities/types.tspackages/worker/src/mcp/capabilities/unified-search.test.tspackages/worker/src/mcp/capabilities/unified-search.tspackages/worker/src/mcp/context.test.tspackages/worker/src/mcp/context.tspackages/worker/src/mcp/cursor/cursor-cloud-client.test.tspackages/worker/src/mcp/cursor/cursor-cloud-client.tspackages/worker/src/mcp/executor.tspackages/worker/src/mcp/github/github-client.test.tspackages/worker/src/mcp/github/github-graphql-client.test.tspackages/worker/src/mcp/github/github-graphql-client.tspackages/worker/src/mcp/github/github-rest-client.tspackages/worker/src/mcp/index.tspackages/worker/src/mcp/mcp-server-e2e.test.tspackages/worker/src/mcp/observability.test.tspackages/worker/src/mcp/observability.tspackages/worker/src/mcp/register-resources.tspackages/worker/src/mcp/register-tools.tspackages/worker/src/mcp/resources/calculator-app-resource.tspackages/worker/src/mcp/run-codemode-registry.tspackages/worker/src/mcp/skills/infer-codemode-capabilities.test.tspackages/worker/src/mcp/skills/infer-codemode-capabilities.tspackages/worker/src/mcp/skills/mcp-skills-repo.tspackages/worker/src/mcp/skills/mcp-skills-types.tspackages/worker/src/mcp/skills/skill-embed-and-flags.test.tspackages/worker/src/mcp/skills/skill-embed-and-flags.tspackages/worker/src/mcp/skills/skill-mutation.tspackages/worker/src/mcp/skills/skill-parameters.test.tspackages/worker/src/mcp/skills/skill-parameters.tspackages/worker/src/mcp/skills/skill-vectorize.tspackages/worker/src/mcp/tools/execute.tspackages/worker/src/mcp/tools/open-calculator-ui.tspackages/worker/src/mcp/tools/search.tstools/prepare-e2e-env.tstypes/tsconfig-tools.jsontypes/tsconfig-worker.json
✅ Files skipped from review due to trivial changes (24)
- packages/worker/src/mcp/github/github-client.test.ts
- packages/worker/src/mcp/index.ts
- packages/worker/src/mcp/observability.ts
- types/tsconfig-tools.json
- packages/worker/src/mcp/github/github-graphql-client.test.ts
- packages/worker/src/capability-maintenance.ts
- packages/worker/src/mcp/capabilities/meta/require-user.ts
- packages/worker/src/chat-agent.ts
- docs/agents/testing-principles.md
- docs/architecture/index.md
- docs/agents/typescript-setup.md
- docs/mcp-server-patterns.md
- docs/agents/adding-capabilities.md
- packages/worker/src/mcp/capabilities/coding/coding-capabilities.test.ts
- docs/agents/cloudflare-agents-sdk.md
- packages/worker/src/mcp/run-codemode-registry.ts
- packages/worker/src/mcp-auth.ts
- packages/worker/src/mcp/capabilities/types.ts
- packages/worker/src/mcp/capabilities/meta/meta-run-skill.ts
- packages/worker/src/mcp/cursor/cursor-cloud-client.test.ts
- packages/worker/src/index.ts
- types/tsconfig-worker.json
- packages/worker/src/mcp/context.ts
- packages/worker/package.json
🚧 Files skipped from review as they are similar to previous changes (5)
- docs/architecture/data-storage.md
- docs/architecture/request-lifecycle.md
- docs/setup-manifest.md
- package.json
- docs/agents/mcp-apps-starter-guide.md
There was a problem hiding this comment.
♻️ Duplicate comments (2)
tools/prepare-e2e-env.ts (2)
33-42:⚠️ Potential issue | 🟠 MajorDon’t require
.env.examplebefore you know you need it.This still fails the happy path when
.envalready exists with a validCOOKIE_SECRETbut.env.exampleis absent, and the early success exit after copying means the copied.envis never validated. Only consult.env.examplein the branches that actually need fallback data, then run theCOOKIE_SECRETcheck after.envis guaranteed to exist.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/prepare-e2e-env.ts` around lines 33 - 42, The script currently exits if .env.example is missing before verifying whether .env exists and contains COOKIE_SECRET, and it exits immediately after copying .env from example without validating COOKIE_SECRET; change the flow so you first check for envPath and only if envPath is missing consult examplePath to copy examplePath -> envPath (using examplePath), remove the premature process.exit(0) after copy, and then run the COOKIE_SECRET validation after envPath is guaranteed to exist; locate and update the blocks referencing examplePath, envPath and the COOKIE_SECRET check to implement this new ordering.
7-16:⚠️ Potential issue | 🟠 MajorMatch dotenv parsing and replacement to the effective assignment.
parseDotenvValuestill only recognizes the first exactKEY=form, andsetDotenvValuestill rewrites the first match. Lines likeCOOKIE_SECRET = ...,export COOKIE_SECRET=..., or a later override in the file will be handled incorrectly, so this script can skip or backfill against the wrong value.Also applies to: 18-26
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/prepare-e2e-env.ts` around lines 7 - 16, parseDotenvValue currently only matches an exact "KEY=" at line start and returns the first hit, which misses variants like leading "export", extra spaces around the key or "=", and later overrides; update parseDotenvValue to scan all lines with a regex that accepts optional "export", any whitespace around the key and "=", and captures the value (handle optional single/double quotes and trimming), and return the last matching assignment (the effective value). Similarly update setDotenvValue to locate and replace the last matching assignment (using the same flexible regex) rather than the first, and if no match exists append a normalized assignment ("KEY=value") to the end; reference the functions parseDotenvValue and setDotenvValue when applying these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tools/prepare-e2e-env.ts`:
- Around line 33-42: The script currently exits if .env.example is missing
before verifying whether .env exists and contains COOKIE_SECRET, and it exits
immediately after copying .env from example without validating COOKIE_SECRET;
change the flow so you first check for envPath and only if envPath is missing
consult examplePath to copy examplePath -> envPath (using examplePath), remove
the premature process.exit(0) after copy, and then run the COOKIE_SECRET
validation after envPath is guaranteed to exist; locate and update the blocks
referencing examplePath, envPath and the COOKIE_SECRET check to implement this
new ordering.
- Around line 7-16: parseDotenvValue currently only matches an exact "KEY=" at
line start and returns the first hit, which misses variants like leading
"export", extra spaces around the key or "=", and later overrides; update
parseDotenvValue to scan all lines with a regex that accepts optional "export",
any whitespace around the key and "=", and captures the value (handle optional
single/double quotes and trimming), and return the last matching assignment (the
effective value). Similarly update setDotenvValue to locate and replace the last
matching assignment (using the same flexible regex) rather than the first, and
if no match exists append a normalized assignment ("KEY=value") to the end;
reference the functions parseDotenvValue and setDotenvValue when applying these
changes.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
| "type": "module", | ||
| "imports": { | ||
| "#worker/*": "../../worker/src/*", | ||
| "#shared/*": "../../shared/*" |
There was a problem hiding this comment.
Mock server #shared/* import alias resolves wrong directory
High Severity
The #shared/* import alias in the mock-ai and mock-resend package.json files maps to ../../shared/*, which resolves to packages/shared/*. The actual shared/ directory lives at the repo root, so the correct mapping is ../../../shared/*. Both mock workers actively use this alias (e.g., #shared/mock-ai.ts and #shared/resend-email.ts), so these workers will fail to start.
Additional Locations (1)
There was a problem hiding this comment.
@cursor, is there a reason we don't just have a @kody-internal/shared package that these packages can use in their deps to reference each other?
Let's fix this issue. Also make sure to take a look at CI and ensure that still works.
- Move root shared/ into packages/shared with workspace exports - Replace #shared and relative shared imports with @kody-internal/shared - Export @kody/worker for mock servers; use @kody/worker/db in mocks - Update tsconfigs, README, test script, and lockfile Made-with: Cursor




Summary
packages/workerand each mock worker intopackages/mock-servers/*packages/worker/src/mcp/*so the worker package owns the server it servesbun.lockwith the workspace manifests so CIbun install --frozen-lockfilesucceeds again.envbackfills follow dotenv precedence rules and packaged Wrangler runs share one sync pathTesting
bun install --frozen-lockfilebun run formatbun run format:checkbun test ./packages/worker/src/mcp/mcp-server-e2e.test.tsbun run test:e2ebun run testbun run test:mcpbun run validate✅ Validate🔎 PreviewSummary by CodeRabbit
Chores
New Features
Tools
Tests
Types
Note
Medium Risk
Medium risk because it restructures the repo into an Nx/Bun monorepo and rewires CI deploy/test paths and Wrangler config locations, which can break builds, deployments, and local env loading if any path assumptions are missed.
Overview
Migrates the codebase to an Nx monorepo with Bun workspaces, moving the main Worker to
packages/worker(including MCP server underpackages/worker/src/mcp/*) and mock Workers topackages/mock-servers/*with per-packageproject.jsonand manifests.Updates Wrangler configuration and CI workflows to point at the new config locations and generated config outputs (now under
packages/worker/), including secrets syncing, D1 migrations, preview mock deployment loops, and.gitignoreentries.Rewires root scripts, import aliases, and tests to use the new package paths, and adjusts MCP E2E to run Wrangler with
--config packages/worker/wrangler.jsoncwhile copying.envinto the package to preserve local secret loading; docs are updated throughout to reflect the new structure, andbun.lockis refreshed (addsnxand workspace packages).Written by Cursor Bugbot for commit a091583. This will update automatically on new commits. Configure here.