fix(cli): various mcp server fixes #2430
Conversation
|
WalkthroughThis PR makes multiple focused changes: removes CI from the env passed to the spawned deploy process; changes GitHub rules manifest loader to fetch files from a rules/ subdirectory; makes trace span schema fields optional/unknown (data.events optional, data.output -> z.unknown().optional()); limits MCP install prompts/install to TTY terminals; replaces lodash.get with @jsonhero/path and removes the dependency; adds MAXIMUM_TRACE_DETAILED_SUMMARY_VIEW_COUNT and uses it for SQL limits; and introduces configurable trace-line truncation, event-summary rendering, and a run-list formatter in CLI output. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/core/src/v3/schemas/api.ts (1)
1236-1236: Downstream consumers handle optionaleventssafely
- packages/cli-v3/src/mcp/formatters.ts (lines 243–252) first checks
if (span.data.events && span.data.events.length > 0)before iterating.- apps/webapp/app/utils/timelineSpanEvents.ts’s
createTimelineSpanEventsFromSpanEventsimmediately returns[]when passedundefined.- apps/webapp/app/presenters/v3/RunPresenter.server.ts (lines 174–177) and the route components (lines 957, 1055) always funnel
data.eventsthrough this helper, so they’ll get an array.Only caveat: if the API ever returns
nullforevents,z.array(...).optional()will throw during parsing. To accept bothundefinedandnulland give consumers a consistent empty‐array fallback, you can optionally adjust the schema:- events: z.array(z.any()).optional(), + events: z.array(z.any()).nullish().default([]),Be aware that defaulting may mask the difference between an omitted/null field and an intentionally empty array.
packages/cli-v3/src/rules/manifest.ts (1)
129-129: Nit: URL-encoderelativePathto avoid issues with spaces or special characters.Safer interpolation, preserves slashes.
Apply:
- `https://raw.githubusercontent.com/triggerdotdev/trigger.dev/refs/heads/${this.branch}/rules/${relativePath}` + `https://raw.githubusercontent.com/triggerdotdev/trigger.dev/refs/heads/${this.branch}/rules/${encodeURI(relativePath)}`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/cli-v3/src/mcp/tools/deploys.ts(0 hunks)packages/cli-v3/src/rules/manifest.ts(1 hunks)packages/core/src/v3/schemas/api.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/cli-v3/src/mcp/tools/deploys.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations
Files:
packages/cli-v3/src/rules/manifest.tspackages/core/src/v3/schemas/api.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
We use zod a lot in packages/core and in the webapp
Files:
packages/core/src/v3/schemas/api.ts
⏰ 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). (23)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
packages/cli-v3/src/rules/manifest.ts (1)
128-130: LGTM: Correctly scopes rules to therules/directory on GitHub.This aligns the remote loader with the manifest location and mirrors the local loader resolving relative to the manifest directory.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/cli-v3/src/commands/dev.ts (2)
110-154: Log when skipping prompts in non-TTY to aid diagnosticsWhen running in CI or other non-interactive contexts, it’s helpful to emit a debug log so users aren’t surprised that no prompts ran.
Apply this diff near the TTY guard:
// Only show these install prompts if the user is in a terminal (not in a Coding Agent) + if (!process.stdout.isTTY) { + logger.debug("Skipping MCP and Rules install prompts (stdout is not a TTY)"); + } if (process.stdout.isTTY) { const skipMCPInstall = typeof options.skipMCPInstall === "boolean" && options.skipMCPInstall;
145-153: Rules install wizard is now also TTY-gated; consider a non-interactive install pathGating the rules wizard behind TTY prevents interactive prompts in CI, which is good. If you want to support explicit non-interactive installs (e.g., in CI), consider adding a flag that installs rules without prompting (e.g., --install-rules-now) and bypasses confirm() when not a TTY.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/cli-v3/src/commands/dev.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations
Files:
packages/cli-v3/src/commands/dev.ts
🧬 Code Graph Analysis (1)
packages/cli-v3/src/commands/dev.ts (3)
packages/cli-v3/src/utilities/configFiles.ts (2)
readConfigHasSeenMCPInstallPrompt(107-112)writeConfigHasSeenMCPInstallPrompt(114-121)packages/cli-v3/src/commands/install-mcp.ts (1)
installMcpServer(190-254)packages/cli-v3/src/commands/install-rules.ts (1)
initiateRulesInstallWizard(160-228)
⏰ 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). (23)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: typecheck / typecheck
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
packages/cli-v3/src/commands/dev.ts (2)
110-112: Good call gating interactive prompts on TTYGuarding the MCP (and rules) prompts behind process.stdout.isTTY prevents hangs in non-interactive environments. This aligns with expected CLI behavior.
130-141: tryCatch returns [error, result]; destructuring is correctThe
tryCatchimplementation inpackages/core/src/v3/tryCatch.tsreturns[null, data]on success and[error, null]on failure—so the first element is always the error. Therefore:
const [installError] = await tryCatch(...)correctly assigns the error.- No changes to the destructuring are needed.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
packages/core/package.json(0 hunks)packages/core/src/v3/utils/ioSerialization.ts(3 hunks)
💤 Files with no reviewable changes (1)
- packages/core/package.json
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations
Files:
packages/core/src/v3/utils/ioSerialization.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
We use zod a lot in packages/core and in the webapp
Files:
packages/core/src/v3/utils/ioSerialization.ts
⏰ 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). (23)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
packages/core/src/v3/utils/ioSerialization.ts (2)
554-559: Return the value (not the node), and guard against path/grammar mismatches
- Return
node?.valueinstead of the node itself so callers can correctly compare againstundefined.- Consider a try/catch to avoid a hard failure on unexpected path formats (SuperJSON meta keys may not strictly match JSON Hero’s path grammar).
- If JSONHeroPath expects a leading
$but SuperJSON meta keys are plain dot/bracket paths, normalize them (example shown as optional; verify before adopting).[ suggest_essential_refactor ]
Proposed fix:-function getKeyFromObject(object: unknown, key: string) { - const jsonHeroPath = new JSONHeroPath(key); - - return jsonHeroPath.first(object); -} +function getKeyFromObject(object: unknown, key: string): unknown { + try { + // Optional normalization if needed (verify with @jsonhero/path docs and SuperJSON meta keys): + // const pathKey = key.startsWith("$") ? key : `$.${key}`; + const jsonHeroPath = new JSONHeroPath(key); + const node = jsonHeroPath.first(object); + return node?.value; + } catch { + // If the path string doesn't conform to JSON Hero's grammar, fall back to "not present" + return undefined; + } +}If SuperJSON meta keys don’t directly match JSON Hero path syntax, we’ll need normalization. Please confirm the shape of
meta.valueskeys and JSONHeroPath expectations. If normalization is required, we can add the$.prefix or perform a translation step here and add unit tests to lock behavior.
15-15: No changes needed for JSONHeroPath importVerified that
JSONHeroPathis indeed a named export in@jsonhero/pathand can be instantiated withimport { JSONHeroPath } from '@jsonhero/path'; const path = new JSONHeroPath('$.people.*.name');(as shown in the official README¹²).
| const overridenUndefinedKeys = originalUndefinedKeys.filter( | ||
| (key) => get(newPayloadObject, key) !== undefined | ||
| (key) => getKeyFromObject(newPayloadObject, key) !== undefined | ||
| ); | ||
|
|
There was a problem hiding this comment.
Bug: Path lookup returns a Node object, so !== undefined becomes true even when the value is undefined
getKeyFromObject() currently returns the result of jsonHeroPath.first(object), which (per library conventions) is a node wrapper, not the raw value. If the path exists but the value at the path is actually undefined, the node is still truthy, so this condition incorrectly treats it as “overridden”.
This would cause us to strip “undefined” metadata from SuperJSON even when the new payload still contains undefined at that path.
Apply this minimal fix (and optional typo fix) to the filter logic:
- const overridenUndefinedKeys = originalUndefinedKeys.filter(
- (key) => getKeyFromObject(newPayloadObject, key) !== undefined
- );
+ const overriddenUndefinedKeys = originalUndefinedKeys.filter(
+ (key) => getKeyFromObject(newPayloadObject, key) !== undefined
+ );Note: This relies on getKeyFromObject returning the raw value (undefined when the value is undefined). See suggested fix to getKeyFromObject in a separate comment.
Optionally fix the spelling: “overridden” instead of “overriden” for clarity.
📝 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.
| const overridenUndefinedKeys = originalUndefinedKeys.filter( | |
| (key) => get(newPayloadObject, key) !== undefined | |
| (key) => getKeyFromObject(newPayloadObject, key) !== undefined | |
| ); | |
| const overriddenUndefinedKeys = originalUndefinedKeys.filter( | |
| (key) => getKeyFromObject(newPayloadObject, key) !== undefined | |
| ); |
🤖 Prompt for AI Agents
In packages/core/src/v3/utils/ioSerialization.ts around lines 538 to 541, the
filter misdetects overrides because getKeyFromObject returns a jsonHero Node
(truthy even when its value is undefined); change the check to inspect the
node's actual value (e.g. use getKeyFromObject(newPayloadObject, key)?.value !==
undefined) so only truly defined values count as overrides, and optionally
correct the variable/name from "overriden" to "overridden".
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)
packages/cli-v3/src/mcp/formatters.ts (1)
244-251: Show output when it’s defined, even if falsy (0/false/empty string).The current check skips valid outputs like 0 or false. Prefer an undefined check.
Apply this diff:
- if (span.data.output) { + if (span.data.output !== undefined) { lines.push( `${indent} Output: ${JSON.stringify(span.data.output, null, 2).replace( /\n/g, "\n" + indent + " " )}` ); }
🧹 Nitpick comments (6)
packages/cli-v3/src/mcp/formatters.ts (6)
8-9: Consider making MAX_TRACE_LINES configurable.Hard-coding 1000 is fine as a safe default, but consider allowing callers to pass the cap or deriving it from a CLI flag/env. This will improve flexibility for debugging vs. standard usage.
Apply a minimal change to thread an optional maxLines param:
-const MAX_TRACE_LINES = 1000; +const MAX_TRACE_LINES = 1000; // default + +// Optionally accept a custom cap in the API without breaking existing callers: +export function formatRunTrace( + trace: RetrieveRunTraceResponseBody["trace"], + maxLines: number = MAX_TRACE_LINES +): string { + const lines: string[] = []; + lines.push(`Trace ID: ${trace.traceId}`); + lines.push(""); + const reachedMaxLines = formatSpan(trace.rootSpan, lines, 0, maxLines); + if (reachedMaxLines) { + lines.push(`(truncated logs to ${maxLines} lines)`); + } + return lines.join("\n"); +}
182-186: Truncation notice is appended, but total output can exceed the cap.Because many pushes happen per span (and some strings contain embedded newlines), the actual displayed line count can exceed MAX_TRACE_LINES before the truncation flag bubbles up. If you need a strict “visible lines” cap, consider a guarded push helper that counts embedded newlines.
Example helper (add near top) and usage pattern:
function pushLine(lines: string[], line: string, maxLines: number): boolean { // Count display lines including embedded newlines const lineCount = 1 + (line.match(/\n/g)?.length ?? 0); const remaining = maxLines - lines.reduce((acc, l) => acc + 1 + (l.match(/\n/g)?.length ?? 0), 0); if (remaining <= 0) return true; if (lineCount <= remaining) { lines.push(line); return false; } // Push a truncated slice if needed (optional), or skip pushing further: const slice = line.split("\n").slice(0, remaining).join("\n"); lines.push(slice); return true; }Then replace lines.push(...) calls with:
- if (pushLine(lines, value, maxLines)) return true;
This ensures you never exceed the cap in terms of visible lines.
191-200: Early return alone won’t prevent mid-function overflow.The early check only helps if we already hit the cap before writing the span. You still push several lines afterwards without guarding, so you can overshoot before recursion returns.
Wrap each push with a guard:
- lines.push(`${indent}${prefix} ${span.message} ${statusIndicator}`); + if (pushLine(lines, `${indent}${prefix} ${span.message} ${statusIndicator}`, maxLines)) return true; - lines.push(`${indent} Duration: ${duration}`); + if (pushLine(lines, `${indent} Duration: ${duration}`, maxLines)) return true; - lines.push(`${indent} Started: ${startTime}`); + if (pushLine(lines, `${indent} Started: ${startTime}`, maxLines)) return true;Repeat for subsequent pushes in this function.
271-274: Avoid adding spacing once the cap is reached.This unconditional blank line can overshoot the cap by one (or more if newline counting is made precise).
Apply this diff:
- if (span.children && span.children.length > 0) { - lines.push(""); - } + if (span.children && span.children.length > 0 && lines.length < maxLines) { + lines.push(""); + }
277-288: Variable shadowing makes readability harder.The reachedMaxLines variable name is reused inside the some() callback, shadowing the parent name. Rename the inner one to avoid confusion.
Apply this diff:
- const reachedMaxLines = span.children.some((child, index) => { - const reachedMaxLines = formatSpan(child, lines, depth + 1, maxLines); + const reachedMaxLines = span.children.some((child, index) => { + const childTruncated = formatSpan(child, lines, depth + 1, maxLines); // Add spacing between sibling spans (except for the last one) - if (index < span.children.length - 1 && !reachedMaxLines) { + if (index < span.children.length - 1 && !childTruncated) { lines.push(""); } - - return reachedMaxLines; + return childTruncated; });
201-209: Tree-drawing prefix is constant for all children; consider last-child glyph for readability.You always use "├─" for nested spans; using "└─" for the last child improves legibility. Not a blocker.
One approach is to thread an isLast flag into formatSpan and pick "└─" vs "├─" accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
apps/webapp/app/env.server.ts(1 hunks)apps/webapp/app/v3/taskEventStore.server.ts(2 hunks)packages/cli-v3/src/mcp/formatters.ts(4 hunks)packages/core/src/v3/schemas/api.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/v3/schemas/api.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations
Files:
apps/webapp/app/v3/taskEventStore.server.tspackages/cli-v3/src/mcp/formatters.tsapps/webapp/app/env.server.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
We use zod a lot in packages/core and in the webapp
Files:
apps/webapp/app/v3/taskEventStore.server.tsapps/webapp/app/env.server.ts
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/webapp.mdc)
apps/webapp/**/*.{ts,tsx}: In the webapp, all environment variables must be accessed through theenvexport ofenv.server.ts, instead of directly accessingprocess.env.
When importing from@trigger.dev/corein the webapp, never import from the root@trigger.dev/corepath; always use one of the subpath exports as defined in the package's package.json.
Files:
apps/webapp/app/v3/taskEventStore.server.tsapps/webapp/app/env.server.ts
⏰ 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). (23)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
apps/webapp/app/env.server.ts (1)
439-439: Confirm detailed‐trace usage and default limit
- Verified that MAXIMUM_TRACE_DETAILED_SUMMARY_VIEW_COUNT is only referenced in apps/webapp/app/v3/taskEventStore.server.ts (lines 288 & 323) for detailed trace queries.
- Summary queries continue to use the separate MAXIMUM_TRACE_SUMMARY_VIEW_COUNT setting.
Please confirm that a default limit of 10,000 for detailed traces is acceptable in production and won’t impose undue load on your database.
apps/webapp/app/v3/taskEventStore.server.ts (3)
288-289: Use of the new detailed limit looks correct (partitioned).Switching the partitioned-detailed path to MAXIMUM_TRACE_DETAILED_SUMMARY_VIEW_COUNT aligns with the new env variable’s purpose.
323-324: Use of the new detailed limit looks correct (non-partitioned).The non-partitioned detailed path also uses MAXIMUM_TRACE_DETAILED_SUMMARY_VIEW_COUNT. Consistent with the partitioned path.
288-289: No changes needed: summary and detailed limits are applied correctlyVerified that across both branches in
taskEventStore.server.ts:
findTraceEventsusesenv.MAXIMUM_TRACE_SUMMARY_VIEW_COUNT(lines 203, 232)findDetailedTraceEventsusesenv.MAXIMUM_TRACE_DETAILED_SUMMARY_VIEW_COUNT(lines 288, 323)Since summary vs. detailed limits are intentionally separate, no updates are required.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli-v3/src/mcp/formatters.ts (1)
247-254: Don’t hide falsy-but-valid outputs (e.g., 0, false, null)The truthy check skips legitimate values like 0/false. Check for undefined instead so we render any explicit output.
Apply this diff:
- if (span.data.output) { + if (span.data.output !== undefined) { lines.push( `${indent} Output: ${JSON.stringify(span.data.output, null, 2).replace( /\n/g, "\n" + indent + " " )}` ); }
🧹 Nitpick comments (4)
packages/cli-v3/src/commands/install-mcp.ts (2)
235-242: Tighten example text; drop nested quotes for cleaner UXThe inner quotes read a bit noisy in terminals. Consider printing the phrases without embedded quotes.
Apply this diff:
- log.message(` • ${chalk.green('"Trigger the hello-world task"')}`); - log.message(` • ${chalk.green('"Can you help me debug the prod run run_1234"')}`); - log.message(` • ${chalk.green('"Deploy my trigger project to staging"')}`); - log.message(` • ${chalk.green('"What trigger task handles uploading files to S3?"')}`); + log.message(` • ${chalk.green("Trigger the hello-world task")}`); + log.message(` • ${chalk.green("Can you help me debug the prod run run_1234")}`); + log.message(` • ${chalk.green("Deploy my trigger project to staging")}`); + log.message(` • ${chalk.green("What trigger task handles uploading files to S3?")}`);
323-329: Remove dead/commented spinner linesSince the spinner UI is intentionally removed, delete the commented lines to avoid confusion for future readers.
Apply this diff:
- // clientSpinner.message(`Installing in ${scope.scope} scope at ${scope.location}`); const configPath = await performInstallForClient(clientName, scope, options); - // clientSpinner.stop(`Successfully installed in ${clientName} (${configPath})`);packages/cli-v3/src/mcp/formatters.ts (2)
8-9: LGTM: sensible default for trace line capDefault of 500 is a reasonable starting point for CLI output. Consider exporting this constant if other modules (e.g., schema descriptions, help text) should reference a single source of truth.
194-203: Early-out within formatSpan to reduce work and prevent overshoot while traversingRight now we only check the cap at entry and after children traversal begins. Introduce a small helper (e.g., shouldStop = () => lines.length >= maxLines) and return early after major push blocks (header, properties, output, events, pre-children spacer). This avoids unnecessary stringifying/indent work and keeps traversal predictable when near the limit.
Example (illustrative edit inside formatSpan):
const shouldStop = () => lines.length >= maxLines; // ... after header fields if (shouldStop()) return true; // ... after properties/output/events blocks if (shouldStop()) return true; // ... after pushing the blank line before children if (shouldStop()) return true;If you’d like, I can provide a concrete diff placing these guards at the right spots.
Also applies to: 279-293
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
packages/cli-v3/src/commands/install-mcp.ts(2 hunks)packages/cli-v3/src/mcp/formatters.ts(4 hunks)packages/cli-v3/src/mcp/schemas.ts(1 hunks)packages/cli-v3/src/mcp/tools/runs.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations
Files:
packages/cli-v3/src/mcp/schemas.tspackages/cli-v3/src/mcp/tools/runs.tspackages/cli-v3/src/commands/install-mcp.tspackages/cli-v3/src/mcp/formatters.ts
🧬 Code graph analysis (3)
packages/cli-v3/src/mcp/tools/runs.ts (1)
packages/cli-v3/src/mcp/formatters.ts (1)
formatRunTrace(175-192)
packages/cli-v3/src/commands/install-mcp.ts (1)
packages/cli-v3/src/mcp/logger.ts (1)
log(14-23)
packages/cli-v3/src/mcp/formatters.ts (1)
packages/core/src/v3/schemas/api.ts (2)
RetrieveRunTraceResponseBody(1261-1266)RetrieveRunTraceResponseBody(1268-1268)
⏰ 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). (23)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
| export function formatRunTrace( | ||
| trace: RetrieveRunTraceResponseBody["trace"], | ||
| maxTraceLines: number = DEFAULT_MAX_TRACE_LINES | ||
| ): string { | ||
| const lines: string[] = []; | ||
|
|
||
| lines.push(`Trace ID: ${trace.traceId}`); | ||
| lines.push(""); | ||
|
|
||
| // Format the root span and its children recursively | ||
| formatSpan(trace.rootSpan, lines, 0); | ||
| const reachedMaxLines = formatSpan(trace.rootSpan, lines, 0, maxTraceLines); | ||
|
|
||
| if (reachedMaxLines) { | ||
| lines.push(`(truncated logs to ${maxTraceLines} lines)`); | ||
| } | ||
|
|
||
| return lines.join("\n"); | ||
| } |
There was a problem hiding this comment.
Line-cap can be exceeded mid-span; enforce a hard cap and reserve room for the notice
Currently, a single large span can push many lines before the recursive call returns, resulting in outputs that exceed the intended cap. Enforce the cap after formatting and ensure the truncation notice is included deterministically.
Apply this diff:
export function formatRunTrace(
trace: RetrieveRunTraceResponseBody["trace"],
maxTraceLines: number = DEFAULT_MAX_TRACE_LINES
): string {
const lines: string[] = [];
lines.push(`Trace ID: ${trace.traceId}`);
lines.push("");
// Format the root span and its children recursively
- const reachedMaxLines = formatSpan(trace.rootSpan, lines, 0, maxTraceLines);
-
- if (reachedMaxLines) {
- lines.push(`(truncated logs to ${maxTraceLines} lines)`);
- }
+ const reachedMaxLines = formatSpan(trace.rootSpan, lines, 0, maxTraceLines);
+
+ // Enforce a hard cap and include the truncation notice within the cap
+ const truncated = reachedMaxLines || lines.length > maxTraceLines;
+ if (truncated) {
+ if (lines.length >= maxTraceLines) {
+ // leave room for the truncation notice line
+ lines.length = Math.max(0, maxTraceLines - 1);
+ }
+ lines.push(`(truncated logs to ${maxTraceLines} lines)`);
+ }
return lines.join("\n");
}As an optional follow-up, we can short-circuit earlier inside formatSpan after heavy sections (properties/output/events) to avoid extra work. Happy to provide that refinement if you want it.
📝 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.
| export function formatRunTrace( | |
| trace: RetrieveRunTraceResponseBody["trace"], | |
| maxTraceLines: number = DEFAULT_MAX_TRACE_LINES | |
| ): string { | |
| const lines: string[] = []; | |
| lines.push(`Trace ID: ${trace.traceId}`); | |
| lines.push(""); | |
| // Format the root span and its children recursively | |
| formatSpan(trace.rootSpan, lines, 0); | |
| const reachedMaxLines = formatSpan(trace.rootSpan, lines, 0, maxTraceLines); | |
| if (reachedMaxLines) { | |
| lines.push(`(truncated logs to ${maxTraceLines} lines)`); | |
| } | |
| return lines.join("\n"); | |
| } | |
| export function formatRunTrace( | |
| trace: RetrieveRunTraceResponseBody["trace"], | |
| maxTraceLines: number = DEFAULT_MAX_TRACE_LINES | |
| ): string { | |
| const lines: string[] = []; | |
| lines.push(`Trace ID: ${trace.traceId}`); | |
| lines.push(""); | |
| // Format the root span and its children recursively | |
| const reachedMaxLines = formatSpan(trace.rootSpan, lines, 0, maxTraceLines); | |
| // Enforce a hard cap and include the truncation notice within the cap | |
| const truncated = reachedMaxLines || lines.length > maxTraceLines; | |
| if (truncated) { | |
| if (lines.length >= maxTraceLines) { | |
| // leave room for the truncation notice line | |
| lines.length = Math.max(0, maxTraceLines - 1); | |
| } | |
| lines.push(`(truncated logs to ${maxTraceLines} lines)`); | |
| } | |
| return lines.join("\n"); | |
| } |
🤖 Prompt for AI Agents
In packages/cli-v3/src/mcp/formatters.ts around lines 175 to 192, a single large
span can produce more lines than maxTraceLines because formatting happens
recursively without enforcing a hard cap or reserving space for the truncation
notice; after building the full lines array (or after formatSpan returns)
truncate the lines to at most (maxTraceLines - 1) entries to reserve one line
for the notice, detect if truncation occurred, and if so replace the tail with
the reserved notice "(truncated logs to X lines)"; ensure the final returned
string always respects the maxTraceLines limit and deterministically includes
the notice when any truncation happened.
| export const GetRunDetailsInput = CommonRunsInput.extend({ | ||
| maxTraceLines: z | ||
| .number() | ||
| .int() | ||
| .describe("The maximum number of lines to show in the trace. Defaults to 500") | ||
| .optional(), | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Constrain and coerce maxTraceLines to safe bounds
Good addition. To prevent pathological inputs (negative, zero, NaN, huge), coerce and bound the value. Also clarify the described default range.
Apply this diff:
export const GetRunDetailsInput = CommonRunsInput.extend({
- maxTraceLines: z
- .number()
- .int()
- .describe("The maximum number of lines to show in the trace. Defaults to 500")
- .optional(),
+ maxTraceLines: z
+ .coerce.number()
+ .int()
+ .min(1)
+ .max(10000)
+ .describe("The maximum number of lines to show in the trace. Defaults to 500 (range: 1–10,000)")
+ .optional(),
});📝 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.
| export const GetRunDetailsInput = CommonRunsInput.extend({ | |
| maxTraceLines: z | |
| .number() | |
| .int() | |
| .describe("The maximum number of lines to show in the trace. Defaults to 500") | |
| .optional(), | |
| }); | |
| export const GetRunDetailsInput = CommonRunsInput.extend({ | |
| maxTraceLines: z | |
| .coerce.number() | |
| .int() | |
| .min(1) | |
| .max(10000) | |
| .describe("The maximum number of lines to show in the trace. Defaults to 500 (range: 1–10,000)") | |
| .optional(), | |
| }); |
🤖 Prompt for AI Agents
In packages/cli-v3/src/mcp/schemas.ts around lines 126 to 132, the maxTraceLines
schema currently allows pathological inputs; change it to coerce and constrain
the value (e.g. use z.coerce.number().int() with a sensible min and max such as
.min(1).max(10000) and set .default(500) so missing values get the default), and
update the .describe text to state the enforced range and default (e.g. "The
maximum number of lines to show in the trace; coerced to an integer between 1
and 10000, defaults to 500"). Ensure the schema still accepts omitted values by
using .default(500) (or .optional() with a transform to default) and rejects
NaN, non-numeric strings, zero, negatives, and excessively large numbers.
| const formattedTrace = formatRunTrace(traceResult.trace, input.maxTraceLines); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clamp user-supplied maxTraceLines before passing to formatter
Even with Zod validation, clamping at the call site makes the behavior robust and aligns with the schema bounds.
Apply this diff:
- const formattedTrace = formatRunTrace(traceResult.trace, input.maxTraceLines);
+ const maxTraceLines =
+ typeof input.maxTraceLines === "number" && Number.isFinite(input.maxTraceLines)
+ ? Math.min(Math.max(Math.trunc(input.maxTraceLines), 1), 10_000)
+ : undefined;
+ const formattedTrace = formatRunTrace(traceResult.trace, maxTraceLines);📝 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.
| const formattedTrace = formatRunTrace(traceResult.trace, input.maxTraceLines); | |
| const maxTraceLines = | |
| typeof input.maxTraceLines === "number" && Number.isFinite(input.maxTraceLines) | |
| ? Math.min(Math.max(Math.trunc(input.maxTraceLines), 1), 10_000) | |
| : undefined; | |
| const formattedTrace = formatRunTrace(traceResult.trace, maxTraceLines); |
🤖 Prompt for AI Agents
packages/cli-v3/src/mcp/tools/runs.ts lines 38-39: clamp input.maxTraceLines
before passing to formatRunTrace by parsing it to a number, bounding it with
Math.max/Math.min against the schema limits (e.g., min 0 and the schema's
MAX_TRACE_LINES constant or value), assign to a local variable like
clampedMaxTraceLines and pass that to formatRunTrace so the formatter always
receives a value within allowed bounds.
No description provided.