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 1 commit 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.
| process.exitCode = 1; | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
Unused outputStyle variable after refactoring
Low Severity
The outputStyle variable is extracted from argv but never used. The old code passed it to printToolResponse(), which was removed during this refactor. The variable is now dead code sitting in the handler.
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.
| logLevel, | ||
| }); | ||
|
|
||
| session.finalize(); |
There was a problem hiding this comment.
Raw output format discards all rendered session text
Medium Severity
When outputFormat is 'raw', a 'text' render strategy session is created. Unlike 'cli-text' (which streams to stdout during emit) or 'cli-json' (which writes JSONL during emit), the 'text' strategy buffers all content internally and only returns it from finalize(). On line 317, session.finalize() is called but its return value is discarded, meaning no tool output ever reaches stdout in raw mode.
Reviewed by Cursor Bugbot for commit e6d00fe. Configure here.
| const unexpectedArgs = (argv._ as unknown[]) | ||
| .slice(2) | ||
| .filter((value): value is string => typeof value === 'string' && value.startsWith('-')); | ||
|
|
||
| if (unexpectedArgs.length > 0) { |
There was a problem hiding this comment.
Bug: The check for unexpected arguments incorrectly uses argv._.slice(2), assuming command names are present. Since yargs removes them, this logic fails, silently ignoring unknown flags.
Severity: MEDIUM
Suggested Fix
Remove the .slice(2) call from the argument parsing logic. Since yargs does not include command names in argv._ within the handler, the slice is unnecessary and incorrect. The remaining logic to filter for arguments starting with - will correctly identify unknown flags that have been converted to positional arguments.
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#L208-L212
Potential issue: The handler for tool subcommands attempts to detect and report
unexpected command-line arguments. It uses `argv._.slice(2)` to strip what it assumes
are the two command names (e.g., `simulator` and `run-tool`) from the list of positional
arguments. However, `yargs` consumes command names during routing, so they are not
present in `argv._` when the handler is executed. As a result, `.slice(2)` removes the
first two actual positional arguments, which could include unknown flags converted to
arguments by `unknown-options-as-args: true`. This causes the validation to fail,
allowing users to pass invalid flags without any error message, potentially hiding typos
or incorrect usage.
Did we get this right? 👍 / 👎 to inform future reviews.
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