refactor(10/12): rewire CLI, daemon, and MCP server boundaries#328
refactor(10/12): rewire CLI, daemon, and MCP server boundaries#328cameroncooke wants to merge 3 commits intorefactor/migrate-remaining-toolsfrom
Conversation
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: Nullish coalescing changes Xcode availability detection logic
- Changed nullish coalescing operator (??) back to logical OR (||) to properly handle empty strings in Xcode version field chain.
- ✅ Fixed: Unused
outputStylevariable after refactoring- Removed the unused outputStyle variable extraction that was left behind after refactoring removed printToolResponse().
Or push these changes by commenting:
@cursor push 5001d2a65c
Preview (5001d2a65c)
diff --git a/src/cli/register-tool-commands.ts b/src/cli/register-tool-commands.ts
--- a/src/cli/register-tool-commands.ts
+++ b/src/cli/register-tool-commands.ts
@@ -221,7 +221,6 @@
const jsonArg = argv.json as string | undefined;
const profileOverride = argv.profile as string | undefined;
const outputFormat = (argv.output as OutputFormat) ?? 'text';
- const outputStyle = (argv.style as OutputStyle) ?? 'normal';
const socketPath = argv.socket as string;
const logLevel = argv['log-level'] as string | undefined;
diff --git a/src/daemon.ts b/src/daemon.ts
--- a/src/daemon.ts
+++ b/src/daemon.ts
@@ -208,9 +208,9 @@
return { success: result.success, output: result.output };
});
const xcodeAvailable = Boolean(
- xcodeVersion.version ??
- xcodeVersion.buildVersion ??
- xcodeVersion.developerDir ??
+ xcodeVersion.version ||
+ xcodeVersion.buildVersion ||
+ xcodeVersion.developerDir ||
xcodeVersion.xcodebuildPath,
);
const axeVersion = await getAxeVersionMetadata(async (command) => {This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.
| xcodeVersion.developerDir || | ||
| xcodeVersion.version ?? | ||
| xcodeVersion.buildVersion ?? | ||
| xcodeVersion.developerDir ?? |
There was a problem hiding this comment.
Nullish coalescing changes Xcode availability detection logic
Medium Severity
Replacing || with ?? changes the semantics for empty strings. If xcodeVersion.version is "" (common when a command fails), ?? considers it non-nullish and stops evaluation, making Boolean("") return false. With ||, the chain would continue to check buildVersion, developerDir, and xcodebuildPath. This can incorrectly report xcodeAvailable as false even when later fields have valid values.
Reviewed by Cursor Bugbot for commit b91063e. Configure here.
b91063e to
e6d00fe
Compare
f8a5827 to
23575fb
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 3 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 e6d00fe. Configure here.
23575fb to
0561a67
Compare
1b4cd5c to
5da2a0b
Compare
01e6478 to
8390e80
Compare
651f77a to
18de083
Compare
8390e80 to
fd818c3
Compare
18de083 to
8096037
Compare
fd818c3 to
dad2143
Compare
a3c5515 to
f79a1f8
Compare
dad2143 to
87835ee
Compare
f79a1f8 to
7ba6db8
Compare
87835ee to
0cf0796
Compare
0cf0796 to
701bb70
Compare
7ba6db8 to
34ec659
Compare
701bb70 to
fb11021
Compare
34ec659 to
b3c094d
Compare
| if (verbose) { | ||
| childProcess.stdout?.pipe(process.stderr, { end: false }); | ||
| childProcess.stderr?.pipe(process.stderr, { end: false }); | ||
| } |
There was a problem hiding this comment.
Bug: The handlerContext is not passed correctly to the tool invoker, causing nextStepParams set by tools to be lost and breaking dynamic client-side template expansion.
Severity: HIGH
Suggested Fix
Ensure the handlerContext object from the daemon server is explicitly passed to invoker.invoke(). This will ensure the same object reference is used, allowing properties set by the tool handler to be correctly read after the invocation.
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/command.ts#L79-L82
Potential issue: In `daemon-server.ts` (lines 160-165), the `handlerContext` object is
not correctly propagated when calling `invoker.invoke()`. The invoker creates a new,
separate context object. As a result, when a tool handler sets properties like
`ctx.nextStepParams` on its context, these values are lost and remain `undefined` on the
original `handlerContext` object that the daemon server later inspects. This failure to
pass the context by reference breaks the mechanism for passing dynamic parameters for
subsequent steps, preventing template expansion on the client side.
fb11021 to
e31bf00
Compare
b3c094d to
13b58c1
Compare
13b58c1 to
b1364d7
Compare
e31bf00 to
57874da
Compare
| explicitArgs, | ||
| }); | ||
|
|
||
| // Invoke the tool | ||
| const response = await invoker.invokeDirect(tool, args, { | ||
| runtime: 'cli', | ||
| cliExposedWorkflowIds, | ||
| socketPath, | ||
| workspaceRoot: opts.workspaceRoot, | ||
| logLevel, | ||
| const missingRequiredFlags = requiredFlagNames.filter((flagName) => { | ||
| const camelKey = convertArgvToToolParams({ [flagName]: true }); | ||
| const [toolKey] = Object.keys(camelKey); | ||
| const value = args[toolKey]; | ||
| return value === undefined || value === null || value === ''; | ||
| }); | ||
|
|
There was a problem hiding this comment.
Bug: When using --output raw, the render session's output is buffered but never printed because the return value of session.finalize() is discarded.
Severity: HIGH
Suggested Fix
Capture the return value from session.finalize() and write it to process.stdout when the output format is 'raw' to ensure the buffered output is displayed to the user.
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/cli/register-tool-commands.ts#L276-L285
Potential issue: When the `--output raw` flag is used, the system configures a `'text'`
render session. This session type buffers all output in memory rather than writing it
directly to standard output. The buffered content is meant to be returned by the
`session.finalize()` method. However, the implementation calls `session.finalize()` but
discards its return value. Consequently, all generated output is collected and then
thrown away, resulting in no output being displayed to the user, rendering the `--output
raw` feature non-functional.




Summary
This is PR 10 of 12 in a stacked PR series that decouples the rendering pipeline from MCP transport. Depends on PR 9 (all tool migrations complete). This PR connects the dots -- it rewires the three output boundaries (CLI, daemon, MCP server) to use the render session from PR 3 with the event-based handlers from PRs 5-9.
Architecture: three boundaries, one rendering engine
The rendering pipeline has three consumers, each using the same render session differently:
CLI direct (
src/cli/register-tool-commands.ts): Creates a render session with the user's chosen strategy (text or JSON). Wrapsemitto write incrementally to stdout as events arrive (streaming output during builds). After the handler completes, callsfinalize()to flush any buffered content (grouped diagnostics). Sets exit code fromsession.isError().MCP server (
src/utils/tool-registry.ts): Creates a render session with text strategy. After the handler completes, callsfinalize()to get the complete rendered string, wraps it in aToolResponse(the MCP protocol type).ToolResponseconstruction happens here and only here -- it is not exported or used anywhere else in the codebase.Daemon (
src/daemon/daemon-server.ts): Creates a render session in record-only mode. Sends raw events over the wire to the CLI client. The CLI client re-renders locally using its own session, giving the same output as CLI direct mode.CLI changes
output.ts: DeletedprintToolResponse(),extractRenderedNextSteps(), and all_metacoordination logic. These were the main sources of complexity in the old rendering pipeline. KeptformatToolList()for--listoutput.register-tool-commands.ts: Now creates and manages the render session directly. Streaming output is handled by a thin wrapper aroundsession.emit().daemon-client.ts: Updated to receive events from daemon and render locally.Daemon protocol
{ events, attachments?, isError? }instead of pre-rendered content.MCP resources
All MCP resource handlers updated to use the simplified context pattern (10 files). These are simpler than tool handlers since resources don't emit pipeline events.
Other changes
src/integrations/xcode-tools-bridge/: Updated to work with the new handler context. Addedbridge-tool-result.tsfor structured bridge tool results.src/visibility/exposure.ts: Updated exposure predicates for new handler signatures.src/smoke-tests/: Updated test harness for new tool invocation pattern.CommandExecutor,FileSystemExecutor,execution/,xcode-state-reader,xcode-state-watcher,video-capture,log_capture.Stack navigation
Test plan
npx vitest runpasses -- boundary tests updatedToolResponsecontent