-
-
Notifications
You must be signed in to change notification settings - Fork 163
feat: Add subsystem filter option for log capture #158
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat: Add subsystem filter option for log capture #158
Conversation
WalkthroughThis 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)
✅ Passed checks (2 passed)
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. Comment |
There was a problem hiding this 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
subsystemFilterupstream 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
bundleIdand 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
📒 Files selected for processing (4)
src/mcp/tools/logging/__tests__/start_sim_log_cap.test.tssrc/mcp/tools/logging/start_sim_log_cap.tssrc/utils/log-capture/index.tssrc/utils/log_capture.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.ts: Use.tsfile extensions for all relative imports of internal TypeScript files (e.g.,import { tool } from './tool.ts')
Use.tsextensions for re-exports from internal modules (e.g.,export { default } from '../shared/tool.ts')
Use.jsextensions only for external package imports (e.g.,import { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js')
Never use.jsextensions for imports of internal TypeScript files
Files:
src/utils/log-capture/index.tssrc/utils/log_capture.tssrc/mcp/tools/logging/start_sim_log_cap.tssrc/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 typesyntax and follows the established pattern. The.tsextension 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
SubsystemFiltervariants with clear, informative messages for users.
94-97: LGTM!The updated tool description accurately reflects the new
subsystemFiltercapability with clear examples of available options.src/utils/log_capture.ts (2)
24-31: LGTM!The
SubsystemFiltertype 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
--predicateargument 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
subsystemFiltervariant. The tests correctly use dependency injection withlogCaptureStubrather than Vitest mocks, following the coding guidelines.
251-331: Note on test scope.These tests verify that
start_sim_log_capLogicpasses correct parameters to the injectedlogCaptureFunction, which is appropriate for unit testing. The stub's manually-constructedspawnCallsdemonstrate expected command structures but don't exercise the actualbuildLogPredicatelogic.Consider adding unit tests for
buildLogPredicatein a separate test file (e.g.,log_capture.test.ts) to verify predicate generation for eachSubsystemFiltervariant, 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.
commit: |
|
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)
Done! |
Summary
This PR adds a new
subsystemFilterparameter to thestart_sim_log_captool, 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 (likeSelf._printChanges()) and other system frameworks couldn't be captured, limiting debugging capabilities.Solution
Added a new
SubsystemFiltertype 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 withSelf._printChanges()string[]: Custom array of subsystem strings to capture (always includes the app's bundle ID)Usage Examples
Changes
src/utils/log_capture.ts: AddedSubsystemFiltertype andbuildLogPredicate()helper functionsrc/utils/log-capture/index.ts: Export the new typesrc/mcp/tools/logging/start_sim_log_cap.ts: AddedsubsystemFilterparameter to the tool schemasrc/mcp/tools/logging/__tests__/start_sim_log_cap.test.ts: Added tests for new functionalityTesting