Skip to content

Conversation

@edwardsanchez
Copy link

Summary

This PR adds a new subsystemFilter parameter to the start_sim_log_cap tool, allowing users to capture logs from different subsystems beyond just their app's bundle ID.

Problem

Previously, the log capture functionality only filtered logs by the app's subsystem (subsystem == "${bundleId}"). This meant that logs from SwiftUI (like Self._printChanges()) and other system frameworks couldn't be captured, limiting debugging capabilities.

Solution

Added a new SubsystemFilter type with the following options:

  • 'app' (default): Only capture logs from the app's bundle ID subsystem (existing behavior)
  • 'all': Capture all system logs with no subsystem filtering
  • 'swiftui': Capture logs from app + SwiftUI subsystem (com.apple.SwiftUI) - useful for debugging view body re-evaluations with Self._printChanges()
  • string[]: Custom array of subsystem strings to capture (always includes the app's bundle ID)

Usage Examples

// Capture SwiftUI's Self._printChanges() output
{
  "bundleId": "com.example.myapp",
  "subsystemFilter": "swiftui"
}

// Capture all system logs
{
  "bundleId": "com.example.myapp",
  "subsystemFilter": "all"
}

// Capture specific subsystems
{
  "bundleId": "com.example.myapp",
  "subsystemFilter": ["com.apple.UIKit", "com.apple.CoreData"]
}

Changes

  • src/utils/log_capture.ts: Added SubsystemFilter type and buildLogPredicate() helper function
  • src/utils/log-capture/index.ts: Export the new type
  • src/mcp/tools/logging/start_sim_log_cap.ts: Added subsystemFilter parameter to the tool schema
  • src/mcp/tools/logging/__tests__/start_sim_log_cap.test.ts: Added tests for new functionality

Testing

  • All 61 logging tests pass
  • Build compiles successfully
  • Lint passes

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 31, 2025

Walkthrough

This change introduces subsystem-based filtering for OS log capture across the logging utilities and the start_sim_log_cap tool. A new SubsystemFilter type is added supporting predefined options ('app', 'all', 'swiftui') or custom subsystem identifiers as arrays. The startLogCapture function is extended with a subsystemFilter parameter to conditionally generate appropriate log predicates. The start_sim_log_cap tool exposes this capability through an updated schema. Test coverage is expanded to validate schema acceptance of filter values and verify handler output reflects the active subsystem filtering configuration.

Pre-merge checks

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: adding a subsystem filter option for log capture functionality.
Description check ✅ Passed The description is directly related to the changeset, providing clear context about the problem, solution, usage examples, and testing results.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
src/mcp/tools/logging/start_sim_log_cap.ts (1)

44-56: Consider simplifying the normalisation logic.

Since Zod validates subsystemFilter upstream against the union type, the explicit type checks are redundant. The schema guarantees only valid values reach this point.

🔎 Proposed simplification
-  // Normalize subsystem filter with explicit type handling for the union type
-  let subsystemFilter: SubsystemFilter = 'app';
-  if (params.subsystemFilter !== undefined) {
-    if (Array.isArray(params.subsystemFilter)) {
-      subsystemFilter = params.subsystemFilter;
-    } else if (
-      params.subsystemFilter === 'app' ||
-      params.subsystemFilter === 'all' ||
-      params.subsystemFilter === 'swiftui'
-    ) {
-      subsystemFilter = params.subsystemFilter;
-    }
-  }
+  const subsystemFilter: SubsystemFilter = params.subsystemFilter ?? 'app';
src/utils/log_capture.ts (1)

36-55: Consider escaping special characters in predicate strings.

The bundleId and custom subsystem strings are interpolated directly into the predicate without escaping. If a subsystem string contains a double quote (e.g., com.example"test), the predicate would be malformed.

While iOS bundle IDs follow strict naming conventions that exclude special characters, custom subsystem strings from the array input could potentially contain quotes. Consider sanitising or validating these values.

🔎 Proposed defensive fix
+/**
+ * Escape special characters in subsystem strings for use in log predicates.
+ */
+function escapeSubsystem(subsystem: string): string {
+  // Escape backslashes first, then double quotes
+  return subsystem.replace(/\\/g, '\\\\').replace(/"/g, '\\"');
+}
+
 function buildLogPredicate(bundleId: string, subsystemFilter: SubsystemFilter): string | null {
   if (subsystemFilter === 'all') {
     // No filtering - capture everything from this process
     return null;
   }

   if (subsystemFilter === 'app') {
-    return `subsystem == "${bundleId}"`;
+    return `subsystem == "${escapeSubsystem(bundleId)}"`;
   }

   if (subsystemFilter === 'swiftui') {
     // Include both app logs and SwiftUI logs (for Self._printChanges())
-    return `subsystem == "${bundleId}" OR subsystem == "com.apple.SwiftUI"`;
+    return `subsystem == "${escapeSubsystem(bundleId)}" OR subsystem == "com.apple.SwiftUI"`;
   }

   // Custom array of subsystems - always include the app's bundle ID
   const subsystems = new Set([bundleId, ...subsystemFilter]);
-  const predicates = Array.from(subsystems).map((s) => `subsystem == "${s}"`);
+  const predicates = Array.from(subsystems).map((s) => `subsystem == "${escapeSubsystem(s)}"`);
   return predicates.join(' OR ');
 }
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c502e79 and 5301d4d.

📒 Files selected for processing (4)
  • src/mcp/tools/logging/__tests__/start_sim_log_cap.test.ts
  • src/mcp/tools/logging/start_sim_log_cap.ts
  • src/utils/log-capture/index.ts
  • src/utils/log_capture.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.ts

📄 CodeRabbit inference engine (AGENTS.md)

**/*.ts: Use .ts file extensions for all relative imports of internal TypeScript files (e.g., import { tool } from './tool.ts')
Use .ts extensions for re-exports from internal modules (e.g., export { default } from '../shared/tool.ts')
Use .js extensions only for external package imports (e.g., import { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js')
Never use .js extensions for imports of internal TypeScript files

Files:

  • src/utils/log-capture/index.ts
  • src/utils/log_capture.ts
  • src/mcp/tools/logging/start_sim_log_cap.ts
  • src/mcp/tools/logging/__tests__/start_sim_log_cap.test.ts
**/*.test.ts

📄 CodeRabbit inference engine (AGENTS.md)

**/*.test.ts: Do NOT use Vitest mocking utilities (vi.mock(), vi.fn(), vi.spyOn(), etc.) - use Dependency Injection with injectable executors instead
Import core Logic functions from tool files and pass mock executors (createMockExecutor or createMockFileSystemExecutor) to simulate different outcomes

Files:

  • src/mcp/tools/logging/__tests__/start_sim_log_cap.test.ts
🧠 Learnings (1)
📚 Learning: 2025-12-31T21:42:22.704Z
Learnt from: CR
Repo: cameroncooke/XcodeBuildMCP PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T21:42:22.704Z
Learning: Applies to **/*.test.ts : Import core Logic functions from tool files and pass mock executors (createMockExecutor or createMockFileSystemExecutor) to simulate different outcomes

Applied to files:

  • src/mcp/tools/logging/start_sim_log_cap.ts
🧬 Code graph analysis (3)
src/utils/log_capture.ts (2)
src/utils/log-capture/index.ts (1)
  • SubsystemFilter (2-2)
src/utils/command.ts (2)
  • CommandExecutor (20-20)
  • getDefaultCommandExecutor (199-210)
src/mcp/tools/logging/start_sim_log_cap.ts (2)
src/utils/log_capture.ts (2)
  • SubsystemFilter (31-31)
  • startLogCapture (63-188)
src/types/common.ts (1)
  • createTextContent (57-59)
src/mcp/tools/logging/__tests__/start_sim_log_cap.test.ts (2)
src/test-utils/mock-executors.ts (1)
  • createMockExecutor (27-75)
src/mcp/tools/logging/start_sim_log_cap.ts (1)
  • start_sim_log_capLogic (38-90)
⏰ 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). (1)
  • GitHub Check: Seer Code Review
🔇 Additional comments (9)
src/utils/log-capture/index.ts (1)

2-2: LGTM!

The type re-export is correctly structured using export type syntax and follows the established pattern. The .ts extension usage aligns with the coding guidelines for internal imports.

src/mcp/tools/logging/start_sim_log_cap.ts (2)

71-81: LGTM!

The filter description logic covers all SubsystemFilter variants with clear, informative messages for users.


94-97: LGTM!

The updated tool description accurately reflects the new subsystemFilter capability with clear examples of available options.

src/utils/log_capture.ts (2)

24-31: LGTM!

The SubsystemFilter type is well-documented with clear explanations of each option.


130-145: LGTM!

The command building logic correctly uses the predicate builder and conditionally adds the --predicate argument only when filtering is required.

src/mcp/tools/logging/__tests__/start_sim_log_cap.test.ts (4)

45-75: LGTM!

Comprehensive schema validation tests covering valid enum values, valid array inputs, and correctly rejecting invalid values.


149-224: LGTM!

Good test coverage for each subsystemFilter variant. The tests correctly use dependency injection with logCaptureStub rather than Vitest mocks, following the coding guidelines.


251-331: Note on test scope.

These tests verify that start_sim_log_capLogic passes correct parameters to the injected logCaptureFunction, which is appropriate for unit testing. The stub's manually-constructed spawnCalls demonstrate expected command structures but don't exercise the actual buildLogPredicate logic.

Consider adding unit tests for buildLogPredicate in a separate test file (e.g., log_capture.test.ts) to verify predicate generation for each SubsystemFilter variant, particularly for edge cases like empty arrays or subsystems containing special characters.


247-249: LGTM!

The updated assertions correctly verify the console capture messaging format.

@pkg-pr-new
Copy link

pkg-pr-new bot commented Jan 1, 2026

Open in StackBlitz

npm i https://pkg.pr.new/cameroncooke/XcodeBuildMCP/xcodebuildmcp@158

commit: f671764

@cameroncooke
Copy link
Owner

Are you able to resolve the merge conflicts so I can get this merged? Thanks.

Resolved conflict in src/mcp/tools/logging/start_sim_log_cap.ts:
- Kept the subsystemFilter feature from PR 158
- Used 'import * as z from zod' style from upstream (Zod 4 convention)
@edwardsanchez
Copy link
Author

Are you able to resolve the merge conflicts so I can get this merged? Thanks.

Done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants