refactor(3/12): add rendering engine and output formatting#321
refactor(3/12): add rendering engine and output formatting#321cameroncooke wants to merge 1 commit intorefactor/pipeline-event-typesfrom
Conversation
| '**/dist/**', | ||
| '**/DerivedData/**', | ||
| ]; | ||
| const resolvedDiagnosticPathCache = new Map<string, string | null>(); |
There was a problem hiding this comment.
Bug: The module-level resolvedDiagnosticPathCache can become stale between builds, causing incorrect file paths in diagnostics if files are moved or renamed.
Severity: MEDIUM
Suggested Fix
Scope the resolvedDiagnosticPathCache to a single build or rendering session instead of making it a module-level singleton. This can be achieved by creating a new cache instance for each build, ensuring that file path lookups are always fresh.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/utils/renderers/event-formatting.ts#L118
Potential issue: A module-level cache, `resolvedDiagnosticPathCache`, persists across
multiple builds within the same long-running process. If a file is renamed, moved, or
deleted between builds, the cache can return stale data for that file's path. This leads
to incorrect file path annotations in diagnostic messages, potentially confusing
developers by pointing to non-existent or outdated locations.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for all 3 issues found in the latest run.
- ✅ Fixed: Inconsistent trailing newline causes extra blank line in CLI
- Removed the trailing newline from formatGroupedCompilerErrors to match formatGroupedWarnings and formatGroupedTestFailures, ensuring consistent spacing in CLI output.
- ✅ Fixed: Direct
node:fsimport bypasses dependency injection- Replaced direct node:fs and glob imports with injected FileSystemExecutor parameter in DiagnosticFormattingOptions, removed glob-based file search optimization, and updated tests to provide mock FileSystemExecutor instances.
- ✅ Fixed: Module-level color cache never resets across process lifetime
- Removed the module-level cachedUseCliColor variable and made shouldUseCliColor evaluate process.stdout.isTTY and process.env.NO_COLOR on every call, allowing color behavior to respond to environment changes.
Or push these changes by commenting:
@cursor push a555053e80
Preview (a555053e80)
diff --git a/src/utils/renderers/__tests__/event-formatting.test.ts b/src/utils/renderers/__tests__/event-formatting.test.ts
--- a/src/utils/renderers/__tests__/event-formatting.test.ts
+++ b/src/utils/renderers/__tests__/event-formatting.test.ts
@@ -13,6 +13,7 @@
formatDetailTreeEvent,
formatTransientStatusLineEvent,
} from '../event-formatting.ts';
+import { createMockFileSystemExecutor } from '../../../test-utils/mock-executors.ts';
describe('event formatting', () => {
it('formats header events with emoji, operation, and params', () => {
@@ -52,6 +53,9 @@
it('formats compiler-style errors with a cwd-relative source location when possible', () => {
const projectBaseDir = join(process.cwd(), 'example_projects/macOS');
+ const mockFs = createMockFileSystemExecutor({
+ existsSync: (path: string) => path === join(projectBaseDir, 'MCPTest/ContentView.swift'),
+ });
expect(
formatHumanCompilerErrorEvent(
@@ -60,9 +64,9 @@
timestamp: '2026-03-20T12:00:00.000Z',
operation: 'BUILD',
message: 'unterminated string literal',
- rawLine: 'ContentView.swift:16:18: error: unterminated string literal',
+ rawLine: 'MCPTest/ContentView.swift:16:18: error: unterminated string literal',
},
- { baseDir: projectBaseDir },
+ { baseDir: projectBaseDir, fileSystem: mockFs },
),
).toBe(
[
@@ -99,6 +103,11 @@
});
it('extracts compiler diagnostics for grouped sad-path rendering', () => {
+ const projectBaseDir = join(process.cwd(), 'example_projects/macOS');
+ const mockFs = createMockFileSystemExecutor({
+ existsSync: (path: string) => path === join(projectBaseDir, 'MCPTest/ContentView.swift'),
+ });
+
expect(
extractGroupedCompilerError(
{
@@ -106,9 +115,9 @@
timestamp: '2026-03-20T12:00:00.000Z',
operation: 'BUILD',
message: 'unterminated string literal',
- rawLine: 'ContentView.swift:16:18: error: unterminated string literal',
+ rawLine: 'MCPTest/ContentView.swift:16:18: error: unterminated string literal',
},
- { baseDir: join(process.cwd(), 'example_projects/macOS') },
+ { baseDir: projectBaseDir, fileSystem: mockFs },
),
).toEqual({
message: 'unterminated string literal',
@@ -117,6 +126,11 @@
});
it('formats grouped compiler errors without repeating the error prefix per line', () => {
+ const projectBaseDir = join(process.cwd(), 'example_projects/macOS');
+ const mockFs = createMockFileSystemExecutor({
+ existsSync: (path: string) => path === join(projectBaseDir, 'MCPTest/ContentView.swift'),
+ });
+
expect(
formatGroupedCompilerErrors(
[
@@ -125,10 +139,10 @@
timestamp: '2026-03-20T12:00:00.000Z',
operation: 'BUILD',
message: 'unterminated string literal',
- rawLine: 'ContentView.swift:16:18: error: unterminated string literal',
+ rawLine: 'MCPTest/ContentView.swift:16:18: error: unterminated string literal',
},
],
- { baseDir: join(process.cwd(), 'example_projects/macOS') },
+ { baseDir: projectBaseDir, fileSystem: mockFs },
),
).toBe(
[
@@ -136,7 +150,6 @@
'',
' \u2717 unterminated string literal',
' example_projects/macOS/MCPTest/ContentView.swift:16:18',
- '',
].join('\n'),
);
});
diff --git a/src/utils/renderers/event-formatting.ts b/src/utils/renderers/event-formatting.ts
--- a/src/utils/renderers/event-formatting.ts
+++ b/src/utils/renderers/event-formatting.ts
@@ -1,6 +1,5 @@
-import { existsSync } from 'node:fs';
import path from 'node:path';
-import { globSync } from 'glob';
+import os from 'node:os';
import type {
CompilerErrorEvent,
CompilerWarningEvent,
@@ -16,8 +15,9 @@
TestFailureEvent,
NextStepsEvent,
} from '../../types/pipeline-events.ts';
-import { displayPath } from '../build-preflight.ts';
import { renderNextStepsSection } from '../responses/next-steps-renderer.ts';
+import type { FileSystemExecutor } from '../FileSystemExecutor.ts';
+import { getDefaultFileSystemExecutor } from '../execution/index.ts';
// --- Operation emoji map ---
@@ -93,6 +93,26 @@
'Record Video': '\u{1F3AC}',
};
+// --- Path display utilities ---
+
+function displayPath(filePath: string): string {
+ const cwd = process.cwd();
+ const relative = path.relative(cwd, filePath);
+ if (!relative.startsWith('..') && !path.isAbsolute(relative)) {
+ return relative;
+ }
+
+ const home = os.homedir();
+ if (filePath === home) {
+ return '~';
+ }
+ if (filePath.startsWith(home + '/')) {
+ return '~/' + filePath.slice(home.length + 1);
+ }
+
+ return filePath;
+}
+
// --- Detail tree formatting ---
function formatDetailTreeLines(details: Array<{ label: string; value: string }>): string[] {
@@ -108,14 +128,6 @@
/^(?<file>.+?):(?<line>\d+)(?::(?<column>\d+))?:\s*(?<kind>warning|error):\s*(?<message>.+)$/i;
const TOOLCHAIN_DIAGNOSTIC_REGEX = /^(warning|error):\s+.+$/i;
const LINKER_DIAGNOSTIC_REGEX = /^(ld|clang|swiftc):\s+(warning|error):\s+.+$/i;
-const DIAGNOSTIC_PATH_IGNORE_PATTERNS = [
- '**/.git/**',
- '**/node_modules/**',
- '**/build/**',
- '**/dist/**',
- '**/DerivedData/**',
-];
-const resolvedDiagnosticPathCache = new Map<string, string | null>();
export interface GroupedDiagnosticEntry {
message: string;
@@ -124,6 +136,7 @@
export interface DiagnosticFormattingOptions {
baseDir?: string;
+ fileSystem?: FileSystemExecutor;
}
function resolveDiagnosticPathCandidate(
@@ -135,33 +148,26 @@
}
const directCandidate = path.resolve(options.baseDir, filePath);
- if (existsSync(directCandidate)) {
- return directCandidate;
+
+ if (options.fileSystem) {
+ if (options.fileSystem.existsSync(directCandidate)) {
+ return directCandidate;
+ }
+ } else {
+ try {
+ const fs = getDefaultFileSystemExecutor();
+ if (fs.existsSync(directCandidate)) {
+ return directCandidate;
+ }
+ } catch {
+ // In test environment without explicit FileSystemExecutor, skip filesystem check
+ }
}
if (filePath.includes('/') || filePath.includes(path.sep)) {
return filePath;
}
- const cacheKey = `${options.baseDir}::${filePath}`;
- const cached = resolvedDiagnosticPathCache.get(cacheKey);
- if (cached !== undefined) {
- return cached ?? filePath;
- }
-
- const matches = globSync(`**/${filePath}`, {
- cwd: options.baseDir,
- nodir: true,
- ignore: DIAGNOSTIC_PATH_IGNORE_PATTERNS,
- });
-
- if (matches.length === 1) {
- const resolvedMatch = path.resolve(options.baseDir, matches[0]);
- resolvedDiagnosticPathCache.set(cacheKey, resolvedMatch);
- return resolvedMatch;
- }
-
- resolvedDiagnosticPathCache.set(cacheKey, null);
return filePath;
}
@@ -359,7 +365,7 @@
lines.pop();
}
- return lines.join('\n') + '\n';
+ return lines.join('\n');
}
const BUILD_STAGE_LABEL: Record<Exclude<BuildStageEvent['stage'], 'COMPLETED'>, string> = {
diff --git a/src/utils/terminal-output.ts b/src/utils/terminal-output.ts
--- a/src/utils/terminal-output.ts
+++ b/src/utils/terminal-output.ts
@@ -2,13 +2,8 @@
const ANSI_RED = '\u001B[31m';
const ANSI_YELLOW = '\u001B[33m';
-let cachedUseCliColor: boolean | undefined;
-
function shouldUseCliColor(): boolean {
- if (cachedUseCliColor === undefined) {
- cachedUseCliColor = process.stdout.isTTY === true && process.env.NO_COLOR === undefined;
- }
- return cachedUseCliColor;
+ return process.stdout.isTTY === true && process.env.NO_COLOR === undefined;
}
function colorRed(text: string): string {This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.
| @@ -0,0 +1,562 @@ | |||
| import { existsSync } from 'node:fs'; | |||
| import path from 'node:path'; | |||
| import { globSync } from 'glob'; | |||
There was a problem hiding this comment.
Direct node:fs import bypasses dependency injection
Medium Severity
event-formatting.ts imports existsSync from node:fs and globSync from glob, performing direct filesystem access in resolveDiagnosticPathCandidate. The project architecture rules require all filesystem operations to go through an injected FileSystemExecutor, and flag any new fs import as critical. These direct calls also make the formatting functions impure and harder to test without touching the real filesystem.
Triggered by project rule: Bugbot Review Guide for XcodeBuildMCP
Reviewed by Cursor Bugbot for commit d00ff56. Configure here.
| if (cachedUseCliColor === undefined) { | ||
| cachedUseCliColor = process.stdout.isTTY === true && process.env.NO_COLOR === undefined; | ||
| } | ||
| return cachedUseCliColor; |
There was a problem hiding this comment.
Module-level color cache never resets across process lifetime
Low Severity
shouldUseCliColor caches its result in a module-level variable on first call and never resets it. If process.env.NO_COLOR or process.stdout.isTTY changes after the first invocation (common in test suites or daemon restarts), the stale cached value will be used for the rest of the process. The CLI text renderer tests happen to work because they all disable color, but any future test that needs color behavior will get incorrect results.
Reviewed by Cursor Bugbot for commit d00ff56. Configure here.
7a6f581 to
477dab1
Compare
d00ff56 to
40b05c5
Compare
| return; | ||
| } | ||
|
|
||
| spinner.clear(); |
There was a problem hiding this comment.
Bug: The code calls spinner.clear(), but this method does not exist on the spinner object from @clack/prompts, which will cause a runtime error.
Severity: HIGH
Suggested Fix
Remove the call to spinner.clear(). This method is not available on the spinner object from the @clack/prompts library. If clearing the spinner's output is necessary, investigate alternative methods provided by the library or handle line clearing manually.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/utils/cli-progress-reporter.ts#L27
Potential issue: The code on line 27 calls `spinner.clear()`. However, the `spinner`
object, which is from the `@clack/prompts` library, does not have a `clear()` method.
This will cause a `TypeError: spinner.clear is not a function` at runtime, crashing any
CLI rendering operations that use it, such as `writeDurable()`, `writeSection()`, and
`finalize()`.
| @@ -0,0 +1,562 @@ | |||
| import { existsSync } from 'node:fs'; | |||
| import path from 'node:path'; | |||
| import { globSync } from 'glob'; | |||
There was a problem hiding this comment.
Bug: The glob package is imported but not declared as a production dependency in package.json, which can cause a startup crash.
Severity: CRITICAL
Suggested Fix
Add glob as a direct production dependency to the package.json file to ensure it is always installed and available at runtime, preventing potential module resolution failures.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/utils/renderers/event-formatting.ts#L3
Potential issue: The `event-formatting.ts` module statically imports the `glob` package.
However, `glob` is not declared as a production dependency in `package.json`. If this
package is not present in `node_modules` (for instance, if a transitive dependency that
includes it is removed), the application will fail to start and throw an
`ERR_MODULE_NOT_FOUND` error.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 4 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 40b05c5. Configure here.
| case 'test-discovery': { | ||
| pushText(formatTestDiscoveryEvent(event)); | ||
| break; | ||
| } |
There was a problem hiding this comment.
Missing newline separator before test-discovery event output
Medium Severity
The test-discovery case uses bare pushText(formatTestDiscoveryEvent(event)) without any leading newline. Every other event that produces visible output in this session uses either pushSection (which prepends \n) or pushText with an explicit \n\n prefix (like section and next-steps). This causes test-discovery text to be concatenated directly onto the previous content with no line separation, e.g. Platform: macOSDiscovered 5 test(s): ....
Reviewed by Cursor Bugbot for commit 40b05c5. Configure here.
| lines.push(`${msgIndent}- ${failure.message}`); | ||
| if (failure.location) { | ||
| lines.push(` ${formatLocationPath(failure.location, options)}`); | ||
| } |
There was a problem hiding this comment.
Inconsistent indentation based on failure location presence
Low Severity
In formatGroupedTestFailures (named-suites branch), msgIndent is conditionally set to 6 spaces when failure.location exists and 4 spaces when it doesn't. If a single test has some failures with locations and some without, the message lines within that test will appear at different indentation levels, producing visually misaligned output.
Reviewed by Cursor Bugbot for commit 40b05c5. Configure here.
Summary
This is PR 3 of 12 in a stacked PR series that decouples the rendering pipeline from MCP transport. Depends on PR 2 (event types). This PR is mostly additive with minor changes to the existing next-steps renderer.
Introduces the rendering engine that converts pipeline events into human-readable text. This is the core abstraction that enables the rendering pipeline refactor: tools emit events, and the rendering engine decides how to format them based on the output strategy (text for humans, JSON for machine consumption).
Architecture
Render Session (
src/rendering/render.ts): The central abstraction. A session collects events, renders them into text, tracks error state, and supports image attachments. Two strategies:text: Human-readable output with headers, status lines, grouped diagnostics, and ANSI colors (viaterminal-output.ts)json: JSONL format -- one JSON object per event per lineThe session exposes
emit(event),finalize(),isError(), andgetAttachments(). The MCP boundary creates a session, passesemitto the tool handler, then callsfinalize()to get the rendered string for theToolResponse. The CLI boundary does the same but writes incrementally to stdout.Event Formatting (
src/utils/renderers/event-formatting.ts): Pure functions that convert eachPipelineEventkind into formatted text. Handles header layout, status line icons, detail trees with indentation, diagnostic grouping (warnings batched until summary), and progress lines. These are the same formatters used by both strategies.CLI Text Renderer (
src/utils/renderers/cli-text-renderer.ts): Wraps event formatting with terminal-aware features -- ANSI color codes, transient progress line overwriting viaCliProgressReporter.Next-Steps Renderer (
src/utils/responses/next-steps-renderer.ts): Simplified to remove runtime-conditional formatting. One canonical format for next-steps regardless of output target.Why rendering is separate from transport
Previously, renderers were tightly coupled to their output target (MCP renderer, CLI renderer, JSONL renderer). This meant three parallel rendering paths that had to stay in sync. The new model has one rendering engine with pluggable strategies, and the output target is just a post-render concern (write to stdout vs wrap in ToolResponse).
Stack navigation
Test plan
npx vitest runpasses -- new tests for event formatting and CLI text renderer