refactor(3/12): add rendering engine and output formatting#321
refactor(3/12): add rendering engine and output formatting#321cameroncooke merged 3 commits intomainfrom
Conversation
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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.
There was a problem hiding this comment.
Event formatting is a pure rendering utility, not a tool handler. Threading a FileSystemExecutor through the rendering pipeline would add complexity for no practical benefit. The fs usage here is limited to path resolution for diagnostic display.
There was a problem hiding this comment.
Event formatting is a rendering utility, not a tool handler. The FileSystemExecutor pattern applies to tool-level operations. No change needed.
| 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.
There was a problem hiding this comment.
The color cache checks process.stdout.isTTY and NO_COLOR, both of which are stable for the process lifetime. No invalidation needed by design.
There was a problem hiding this comment.
process.stdout.isTTY and NO_COLOR are both stable for the process lifetime. A daemon restart creates a new process with fresh module state. No change needed.
cameroncooke
force-pushed
the
refactor/pipeline-event-types
branch
from
7a6f581 to
477dab1
Compare
cameroncooke
force-pushed
the
refactor/rendering-engine
branch
from
d00ff56 to
40b05c5
Compare
cameroncooke
force-pushed
the
refactor/rendering-engine
branch
from
40b05c5 to
df81928
Compare
cameroncooke
force-pushed
the
refactor/rendering-engine
branch
from
df81928 to
a0f22a7
Compare
cameroncooke
force-pushed
the
refactor/pipeline-event-types
branch
from
a3f99ae to
1e526e6
Compare
cameroncooke
force-pushed
the
refactor/rendering-engine
branch
from
a0f22a7 to
b7f35d4
Compare
cameroncooke
force-pushed
the
refactor/pipeline-event-types
branch
from
1e526e6 to
255ecb4
Compare
cameroncooke
changed the base branch from
refactor/pipeline-event-types
to
graphite-base/321
cameroncooke
force-pushed
the
graphite-base/321
branch
from
1e526e6 to
904c110
Compare
cameroncooke
force-pushed
the
refactor/rendering-engine
branch
from
b7f35d4 to
67d1e46
Compare
cameroncooke
changed the base branch from
graphite-base/321
to
refactor/pipeline-event-types
cameroncooke
force-pushed
the
refactor/rendering-engine
branch
9 times, most recently
from
77c7b0b to
a173ad8
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 4 total unresolved issues (including 3 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 12682e1. Configure here.
| '**/dist/**', | ||
| '**/DerivedData/**', | ||
| ]; | ||
| const resolvedDiagnosticPathCache = new Map<string, string | null>(); |
There was a problem hiding this comment.
Module-level cache persists across tests without reset
Low Severity
resolvedDiagnosticPathCache is a module-level Map that accumulates entries across all invocations with no mechanism to clear or reset it. In long-running server processes (MCP server, daemon), stale cached path resolutions from earlier builds could return incorrect results if files are added, removed, or renamed between tool invocations.
Reviewed by Cursor Bugbot for commit 12682e1. Configure here.
There was a problem hiding this comment.
The cache is a performance optimization for an expensive globSync in a narrow diagnostic display path. Staleness only affects path prettification, not correctness. The cache key includes baseDir so different build configurations get separate entries. No change needed.
glob is imported in event-formatting.ts but was only available as a transitive dev dependency. With unbundled builds this would cause ERR_MODULE_NOT_FOUND at runtime for end users.
reconciledPassedTests was subtracting the raw failedTests counter instead of reconciledFailedTests, inflating the passed count when individual test failures outnumber the aggregate counter.
cameroncooke
force-pushed
the
refactor/rendering-engine
branch
from
27c8907 to
6716c5b
Compare
Merge activity
|
1 participant
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