feat: Add suppressWarnings option to reduce context window usage #147
Conversation
WalkthroughA new warning suppression feature is introduced. The Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/mcp/tools/macos/test_macos.ts (1)
260-268: Critical:suppressWarningsparameter not passed toexecuteXcodeBuildCommand.The
suppressWarningsparameter is accepted in the schema but not passed through toexecuteXcodeBuildCommand, rendering it non-functional.Apply this diff to fix:
const testResult = await executeXcodeBuildCommand( { projectPath: params.projectPath, workspacePath: params.workspacePath, scheme: params.scheme, configuration: params.configuration ?? 'Debug', derivedDataPath: params.derivedDataPath, extraArgs, + suppressWarnings: params.suppressWarnings, }, {src/mcp/tools/utilities/clean.ts (1)
117-127: Critical:suppressWarningsparameter not passed toexecuteXcodeBuildCommand.The
suppressWarningsparameter is accepted in the schema but not included intypedParams, which is passed toexecuteXcodeBuildCommand, rendering it non-functional.Apply this diff to fix:
const typedParams: SharedBuildParams = { ...(hasProjectPath ? { projectPath: params.projectPath as string } : { workspacePath: params.workspacePath as string }), scheme: params.scheme ?? '', configuration: params.configuration ?? 'Debug', derivedDataPath: params.derivedDataPath, extraArgs: params.extraArgs, + suppressWarnings: params.suppressWarnings, };src/mcp/tools/device/test_device.ts (1)
209-217: Critical:suppressWarningsparameter not passed toexecuteXcodeBuildCommand.The
suppressWarningsparameter is accepted in the schema but not passed through toexecuteXcodeBuildCommand, rendering it non-functional.Apply this diff to fix:
const testResult = await executeXcodeBuildCommand( { projectPath: params.projectPath, workspacePath: params.workspacePath, scheme: params.scheme, configuration: params.configuration ?? 'Debug', derivedDataPath: params.derivedDataPath, extraArgs, + suppressWarnings: params.suppressWarnings, }, {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/mcp/tools/device/build_device.ts(1 hunks)src/mcp/tools/device/test_device.ts(1 hunks)src/mcp/tools/macos/build_macos.ts(1 hunks)src/mcp/tools/macos/build_run_macos.ts(1 hunks)src/mcp/tools/macos/test_macos.ts(1 hunks)src/mcp/tools/simulator/build_run_sim.ts(1 hunks)src/mcp/tools/simulator/build_sim.ts(1 hunks)src/mcp/tools/simulator/test_sim.ts(2 hunks)src/mcp/tools/utilities/clean.ts(1 hunks)src/types/common.ts(1 hunks)src/utils/__tests__/build-utils-suppress-warnings.test.ts(1 hunks)src/utils/build-utils.ts(1 hunks)src/utils/test-common.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.ts: Use .ts extensions for all internal relative imports (e.g., import { tool } from './tool.ts')
Use .ts extensions in re-exports for internal files (e.g., export { default } from '../shared/tool.ts')
Use .js extension only for external package entry points (e.g., import ... from '@scope/pkg/file.js')
Never import internal project files using .js extensions
Files:
src/mcp/tools/device/test_device.tssrc/utils/test-common.tssrc/utils/build-utils.tssrc/mcp/tools/simulator/build_sim.tssrc/mcp/tools/macos/build_macos.tssrc/mcp/tools/macos/build_run_macos.tssrc/types/common.tssrc/mcp/tools/utilities/clean.tssrc/utils/__tests__/build-utils-suppress-warnings.test.tssrc/mcp/tools/macos/test_macos.tssrc/mcp/tools/simulator/build_run_sim.tssrc/mcp/tools/device/build_device.tssrc/mcp/tools/simulator/test_sim.ts
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{test,spec}.ts: Do not use Vitest mocking APIs in tests (no vi.mock, vi.fn, vi.spyOn, etc.)
In tests, import the core ...Logic function from the tool and use createMockExecutor/createMockFileSystemExecutor to simulate outcomes
Files:
src/utils/__tests__/build-utils-suppress-warnings.test.ts
🧠 Learnings (2)
📚 Learning: 2025-09-21T19:27:22.795Z
Learnt from: CR
Repo: cameroncooke/XcodeBuildMCP PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-21T19:27:22.795Z
Learning: Applies to **/*.{test,spec}.ts : In tests, import the core ...Logic function from the tool and use createMockExecutor/createMockFileSystemExecutor to simulate outcomes
Applied to files:
src/utils/test-common.tssrc/utils/__tests__/build-utils-suppress-warnings.test.ts
📚 Learning: 2025-09-21T19:27:22.795Z
Learnt from: CR
Repo: cameroncooke/XcodeBuildMCP PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-21T19:27:22.795Z
Learning: Applies to **/*.{test,spec}.ts : Do not use Vitest mocking APIs in tests (no vi.mock, vi.fn, vi.spyOn, etc.)
Applied to files:
src/utils/__tests__/build-utils-suppress-warnings.test.ts
🧬 Code graph analysis (1)
src/utils/__tests__/build-utils-suppress-warnings.test.ts (1)
src/utils/build-utils.ts (1)
executeXcodeBuildCommand(44-367)
🔇 Additional comments (11)
src/types/common.ts (1)
97-97: LGTM!The addition of the optional
suppressWarningsparameter toSharedBuildParamsis straightforward and appropriate for enabling warning suppression across build and test operations.src/utils/build-utils.ts (1)
235-237: LGTM!The warning suppression logic is correctly implemented. When
suppressWarningsis true, warnings are filtered out while errors remain visible. The early return pattern is clean and efficient.src/mcp/tools/macos/test_macos.ts (1)
48-51: LGTM!The schema definition for
suppressWarningsis clear and correctly describes the parameter's purpose.src/mcp/tools/utilities/clean.ts (1)
51-54: LGTM!The schema definition for
suppressWarningsis clear and consistent with other tools.src/utils/test-common.ts (1)
162-162: LGTM!The
suppressWarningsparameter is correctly added to the function signature, and the spread operator at line 187 (...params) ensures it's properly passed toexecuteXcodeBuildCommand.src/mcp/tools/device/test_device.ts (1)
47-50: LGTM!The schema definition for
suppressWarningsis clear and consistent with other test tools.src/mcp/tools/macos/build_run_macos.ts (1)
37-40: LGTM!The schema definition for
suppressWarningsis correctly added, and the spread operator at line 74 (...params) ensures it's properly passed toexecuteXcodeBuildCommand.src/mcp/tools/simulator/build_sim.ts (1)
49-52: LGTM!The schema definition for
suppressWarningsis correctly added, and the spread operator at line 108 (...params) ensures it's properly passed toexecuteXcodeBuildCommand.src/mcp/tools/device/build_device.ts (1)
25-29:suppressWarningsis correctly surfaced and forwardedAdding
suppressWarningsto the schema and then spreadingparamsintoprocessedParamsensures the flag reachesexecuteXcodeBuildCommandwithout extra plumbing. This segment looks good and aligns with the shared build utility behavior.Also applies to: 47-66
src/mcp/tools/simulator/test_sim.ts (1)
64-68: Consistent wiring ofsuppressWarningsinto simulator test flowSchema now exposes
suppressWarnings, andtest_simLogicforwards it intohandleTestLogic, so warning suppression should work for simulator tests in the same way as other tools. Public schema also keeps the field visible to callers. No issues spotted here.Also applies to: 103-118
src/mcp/tools/macos/build_macos.ts (1)
46-50: macOS build now correctly exposes and forwardssuppressWarnings
suppressWarningsis added to the macOS build schema and, via spreadingparamsintoprocessedParams, will be passed through toexecuteXcodeBuildCommand. The public schema also retains the field, so the API surface and runtime behavior are consistent with the rest of the tools.Also applies to: 76-99
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mcp/tools/simulator/__tests__/build_run_sim.test.ts (1)
47-52: build_run_sim schema keys look right; reformat expectation to satisfy linterIncluding
suppressWarningsin the public schema keys is aligned with the new shared flag and keeps simulator tools consistent. The failing check is just formatting: thetoEqualargument needs to be wrapped as indicated by the static analysis.If you want parity with other option fields, you could also add a simple
safeParsesuccess/failure pair forsuppressWarningsbeingbooleanvs non-boolean, but that’s not required for this PR.- const schemaKeys = Object.keys(buildRunSim.schema).sort(); - expect(schemaKeys).toEqual(['derivedDataPath', 'extraArgs', 'preferXcodebuild', 'suppressWarnings'].sort()); + const schemaKeys = Object.keys(buildRunSim.schema).sort(); + expect(schemaKeys).toEqual( + ['derivedDataPath', 'extraArgs', 'preferXcodebuild', 'suppressWarnings'].sort(), + );
🧹 Nitpick comments (1)
src/mcp/tools/device/__tests__/test_device.test.ts (1)
35-58: Schema validation update looks good.The addition of
'suppressWarnings'to the expected schema keys is correct and properly positioned alphabetically.Consider adding a test case that actually passes the
suppressWarningsparameter totestDeviceLogicto verify end-to-end integration. While the PR includes unit tests for the filtering logic inbuild-utils-suppress-warnings.test.ts, adding an integration test here would verify the parameter flows correctly through the test_device tool.Example test case:
+ it('should pass suppressWarnings parameter through to logic', async () => { + const mockExecutor = createMockExecutor({ + success: true, + output: JSON.stringify({ + title: 'Tests', + result: 'SUCCESS', + totalTestCount: 1, + passedTests: 1, + failedTests: 0, + skippedTests: 0, + expectedFailures: 0, + }), + }); + + const result = await testDeviceLogic( + { + projectPath: '/path/to/project.xcodeproj', + scheme: 'MyScheme', + deviceId: 'test-device-123', + suppressWarnings: true, + }, + mockExecutor, + createMockFileSystemExecutor({ + mkdtemp: async () => '/tmp/xcodebuild-test-123456', + tmpdir: () => '/tmp', + stat: async () => ({ isFile: () => true }), + rm: async () => {}, + }), + ); + + expect(result.isError).toBe(false); + expect(result.content[0].text).toContain('✅'); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/mcp/tools/device/__tests__/build_device.test.ts(1 hunks)src/mcp/tools/device/__tests__/test_device.test.ts(1 hunks)src/mcp/tools/macos/__tests__/build_macos.test.ts(1 hunks)src/mcp/tools/macos/__tests__/build_run_macos.test.ts(1 hunks)src/mcp/tools/macos/__tests__/test_macos.test.ts(1 hunks)src/mcp/tools/simulator/__tests__/build_run_sim.test.ts(1 hunks)src/mcp/tools/simulator/__tests__/test_sim.test.ts(1 hunks)src/mcp/tools/utilities/__tests__/clean.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{test,spec}.ts: Do not use Vitest mocking APIs in tests (no vi.mock, vi.fn, vi.spyOn, etc.)
In tests, import the core ...Logic function from the tool and use createMockExecutor/createMockFileSystemExecutor to simulate outcomes
Files:
src/mcp/tools/device/__tests__/build_device.test.tssrc/mcp/tools/macos/__tests__/build_macos.test.tssrc/mcp/tools/simulator/__tests__/build_run_sim.test.tssrc/mcp/tools/macos/__tests__/build_run_macos.test.tssrc/mcp/tools/simulator/__tests__/test_sim.test.tssrc/mcp/tools/utilities/__tests__/clean.test.tssrc/mcp/tools/macos/__tests__/test_macos.test.tssrc/mcp/tools/device/__tests__/test_device.test.ts
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.ts: Use .ts extensions for all internal relative imports (e.g., import { tool } from './tool.ts')
Use .ts extensions in re-exports for internal files (e.g., export { default } from '../shared/tool.ts')
Use .js extension only for external package entry points (e.g., import ... from '@scope/pkg/file.js')
Never import internal project files using .js extensions
Files:
src/mcp/tools/device/__tests__/build_device.test.tssrc/mcp/tools/macos/__tests__/build_macos.test.tssrc/mcp/tools/simulator/__tests__/build_run_sim.test.tssrc/mcp/tools/macos/__tests__/build_run_macos.test.tssrc/mcp/tools/simulator/__tests__/test_sim.test.tssrc/mcp/tools/utilities/__tests__/clean.test.tssrc/mcp/tools/macos/__tests__/test_macos.test.tssrc/mcp/tools/device/__tests__/test_device.test.ts
🧠 Learnings (1)
📚 Learning: 2025-09-21T19:27:22.795Z
Learnt from: CR
Repo: cameroncooke/XcodeBuildMCP PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-21T19:27:22.795Z
Learning: Applies to **/*.{test,spec}.ts : In tests, import the core ...Logic function from the tool and use createMockExecutor/createMockFileSystemExecutor to simulate outcomes
Applied to files:
src/mcp/tools/device/__tests__/build_device.test.tssrc/mcp/tools/simulator/__tests__/build_run_sim.test.tssrc/mcp/tools/macos/__tests__/build_run_macos.test.tssrc/mcp/tools/simulator/__tests__/test_sim.test.tssrc/mcp/tools/utilities/__tests__/clean.test.ts
🪛 GitHub Actions: CI
src/mcp/tools/device/__tests__/build_device.test.ts
[error] 40-40: prettier/prettier: Replace multi-line formatting for the object keys 'derivedDataPath', 'extraArgs', 'preferXcodebuild', 'suppressWarnings' with a multi-line form.
🪛 GitHub Check: build-and-test (24.x)
src/mcp/tools/device/__tests__/build_device.test.ts
[failure] 40-40:
Replace 'derivedDataPath',·'extraArgs',·'preferXcodebuild',·'suppressWarnings' with ⏎········'derivedDataPath',⏎········'extraArgs',⏎········'preferXcodebuild',⏎········'suppressWarnings',⏎······
src/mcp/tools/macos/__tests__/build_macos.test.ts
[failure] 49-49:
Replace ['derivedDataPath',·'extraArgs',·'preferXcodebuild',·'suppressWarnings'].sort() with ⏎········['derivedDataPath',·'extraArgs',·'preferXcodebuild',·'suppressWarnings'].sort(),⏎······
src/mcp/tools/simulator/__tests__/build_run_sim.test.ts
[failure] 48-48:
Replace ['derivedDataPath',·'extraArgs',·'preferXcodebuild',·'suppressWarnings'].sort() with ⏎········['derivedDataPath',·'extraArgs',·'preferXcodebuild',·'suppressWarnings'].sort(),⏎······
src/mcp/tools/macos/__tests__/build_run_macos.test.ts
[failure] 42-42:
Replace ['derivedDataPath',·'extraArgs',·'preferXcodebuild',·'suppressWarnings'].sort() with ⏎········['derivedDataPath',·'extraArgs',·'preferXcodebuild',·'suppressWarnings'].sort(),⏎······
src/mcp/tools/simulator/__tests__/test_sim.test.ts
[failure] 49-49:
Replace 'derivedDataPath',·'extraArgs',·'preferXcodebuild',·'suppressWarnings',·'testRunnerEnv' with ⏎··········'derivedDataPath',⏎··········'extraArgs',⏎··········'preferXcodebuild',⏎··········'suppressWarnings',⏎··········'testRunnerEnv',⏎········
src/mcp/tools/macos/__tests__/test_macos.test.ts
[failure] 50-50:
Replace 'derivedDataPath',·'extraArgs',·'preferXcodebuild',·'suppressWarnings',·'testRunnerEnv' with ⏎··········'derivedDataPath',⏎··········'extraArgs',⏎··········'preferXcodebuild',⏎··········'suppressWarnings',⏎··········'testRunnerEnv',⏎········
🔇 Additional comments (1)
src/mcp/tools/utilities/__tests__/clean.test.ts (1)
29-32: Schema keys expectation correctly updated forsuppressWarnings.The assertion now matching
['derivedDataPath', 'extraArgs', 'platform', 'preferXcodebuild', 'suppressWarnings'](with.sort()on both sides) is consistent with the new public schema surface and keeps the order-agnostic check. No issues here; the test continues to validate the full set of exposed keys while following the existing pattern.
Based on learnings, this test also correctly uses the corecleanLogicfunction pluscreateMockExecutor, in line with the repo’s testing guidelines.
commit: |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/mcp/tools/simulator/__tests__/build_run_sim.test.ts (1)
48-50: Schema keys expectation correctly updated to includesuppressWarningsThe assertion now matches the extended public schema and the use of
.sort()keeps it stable against key-order changes. If you want parity with the other fields’ coverage, you could optionally add positive/negative parse tests forsuppressWarningsitself (e.g.,true/falsevs non-boolean), but the current change is sound as-is.src/mcp/tools/macos/__tests__/build_run_macos.test.ts (1)
25-45: Consider adding schema validation tests forsuppressWarnings.While the schema keys check includes
suppressWarnings, there are no validation tests to verify it accepts boolean values or rejects invalid types. Consider adding assertions similar to lines 37-39 for consistency:expect(schema.safeParse({ suppressWarnings: 'yes' }).success).toBe(false); expect(schema.safeParse({ suppressWarnings: true }).success).toBe(true);This would ensure the schema correctly validates the new parameter.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/mcp/tools/device/__tests__/build_device.test.ts(1 hunks)src/mcp/tools/device/test_device.ts(2 hunks)src/mcp/tools/macos/__tests__/build_macos.test.ts(1 hunks)src/mcp/tools/macos/__tests__/build_run_macos.test.ts(1 hunks)src/mcp/tools/macos/__tests__/test_macos.test.ts(1 hunks)src/mcp/tools/macos/test_macos.ts(2 hunks)src/mcp/tools/simulator/__tests__/build_run_sim.test.ts(1 hunks)src/mcp/tools/simulator/__tests__/test_sim.test.ts(1 hunks)src/mcp/tools/utilities/clean.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- src/mcp/tools/macos/tests/test_macos.test.ts
- src/mcp/tools/device/tests/build_device.test.ts
- src/mcp/tools/macos/test_macos.ts
- src/mcp/tools/device/test_device.ts
- src/mcp/tools/macos/tests/build_macos.test.ts
- src/mcp/tools/simulator/tests/test_sim.test.ts
- src/mcp/tools/utilities/clean.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{test,spec}.ts: Do not use Vitest mocking APIs in tests (no vi.mock, vi.fn, vi.spyOn, etc.)
In tests, import the core ...Logic function from the tool and use createMockExecutor/createMockFileSystemExecutor to simulate outcomes
Files:
src/mcp/tools/simulator/__tests__/build_run_sim.test.tssrc/mcp/tools/macos/__tests__/build_run_macos.test.ts
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.ts: Use .ts extensions for all internal relative imports (e.g., import { tool } from './tool.ts')
Use .ts extensions in re-exports for internal files (e.g., export { default } from '../shared/tool.ts')
Use .js extension only for external package entry points (e.g., import ... from '@scope/pkg/file.js')
Never import internal project files using .js extensions
Files:
src/mcp/tools/simulator/__tests__/build_run_sim.test.tssrc/mcp/tools/macos/__tests__/build_run_macos.test.ts
🧠 Learnings (1)
📚 Learning: 2025-09-21T19:27:22.795Z
Learnt from: CR
Repo: cameroncooke/XcodeBuildMCP PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-21T19:27:22.795Z
Learning: Applies to **/*.{test,spec}.ts : In tests, import the core ...Logic function from the tool and use createMockExecutor/createMockFileSystemExecutor to simulate outcomes
Applied to files:
src/mcp/tools/macos/__tests__/build_run_macos.test.ts
🔇 Additional comments (1)
src/mcp/tools/macos/__tests__/build_run_macos.test.ts (1)
42-44: LGTM! Schema keys updated correctly.The addition of
suppressWarningsto the expected schema keys is correct and aligns with the PR objectives. The multi-line formatting also satisfies the linter requirements mentioned in the past review comments.
|
Thanks for the contribution @codeman9 - In the name of reducing context usage and therefore tool schema surfaces, I wonder if this would be best placed as a session-aware option instead of per tool. I feel like either you will want warnings for an entire session or you won't. Unless you disagree, can you update this to be set via session-defaults tool instead? |
Adds a new optional 'suppressWarnings' parameter to build and test tools that filters warning messages from output, helping AI assistants conserve context window space when warnings are not relevant to the task. Affected tools: - build_device, test_device - build_macos, build_run_macos, test_macos - build_sim, build_run_sim, test_sim - clean Includes unit tests for the warning suppression logic.
…t_macos, test_device, and clean
Per PR feedback, suppressWarnings is now a session-level setting instead of a per-tool parameter. This reduces tool schema surface area and aligns with the session-aware architecture. Changes: - Add suppressWarnings to SessionDefaults type and session_set_defaults - Remove suppressWarnings from all individual build/test tool schemas - Update build-utils.ts to read from sessionStore - Update tests to reflect the new session-based approach
codeman9
force-pushed
the
codeman9/feat-suppress-warnings
branch
from
4ef361b to
713a821
Compare
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
Perfect, LGTM! |
2 participants
Summary
Adds a new optional
suppressWarningsparameter to build and test tools that filters warning messages from output, helping AI assistants conserve context window space when warnings are not relevant to the task.Motivation
When working with AI assistants, build warnings can consume significant context window space without providing actionable information. This option allows users to suppress warnings while still surfacing errors.
Changes
suppressWarningsboolean parameter to all build/test tool schemasSharedBuildParamsinterface incommon.tsbuild-utils.tsAffected tools:
build_device,test_devicebuild_macos,build_run_macos,test_macosbuild_sim,build_run_sim,test_simcleanTesting
build-utils-suppress-warnings.test.tssuppressWarnings: falsesuppressWarnings: true🤖 This code was created with assistance from AI tooling (Amazon Q Developer)
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.
Note
Adds a session-configurable
suppressWarningsflag that filters warning lines from Xcode build output, with schema/store updates and unit tests.src/utils/build-utils.tsusing session defaultsuppressWarningsfromsessionStore.suppressWarningstoSessionDefaultsinsrc/utils/session-store.tsand expose viasession-set-defaultstool schema insrc/mcp/tools/session-management/session_set_defaults.ts.src/utils/__tests__/build-utils-suppress-warnings.test.tscovering inclusion/exclusion of warnings based onsuppressWarnings.Written by Cursor Bugbot for commit 713a821. This will update automatically on new commits. Configure here.