feat(xcodebuildmcp): add support for session-default persistance #190
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
commit: |
WalkthroughThis pull request introduces a project-level configuration system for XcodeBuildMCP. A new YAML-based configuration file at 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 9
🤖 Fix all issues with AI agents
In `@docs/CONFIGURATION.md`:
- Around line 63-65: Update the fenced code block that contains
"<workspace-root>/.xcodebuildmcp/config.yaml" to include a language specifier
(e.g., change ``` to ```text) so the block is consistently marked as plaintext;
locate the fenced block in docs/CONFIGURATION.md and replace the opening
backticks accordingly.
- Around line 97-99: Add a comma after the introductory phrase in the sentence
that begins "By default when the agent calls `session_set_defaults`" so it reads
"By default, when the agent calls `session_set_defaults`..." — update the
sentence in the docs where `session_set_defaults` and the `persist` flag are
explained to include this comma for correct punctuation.
In `@docs/dev/PROJECT_CONFIG_PLAN.md`:
- Line 66: The sentence "Resolve relative paths for: `projectPath`,
`workspacePath`, `derivedDataPath`." reads awkwardly; remove the colon and fold
the list into the sentence so it reads e.g. "Resolve relative paths for
`projectPath`, `workspacePath` and `derivedDataPath`." Update the line in
PROJECT_CONFIG_PLAN.md to that wording, keeping the three identifiers
(`projectPath`, `workspacePath`, `derivedDataPath`) exactly as shown.
- Line 121: Edit the SESSION_DEFAULTS document where it mentions config
auto-load and the `persist` flag: replace the hyphenated term "auto-load" with
the single word "autoload" everywhere in that sentence/section (e.g., the line
that currently reads 'config auto-load + `persist` flag') so the docs
consistently use "autoload" while preserving the reference to the `persist`
flag.
In `@docs/SESSION_DEFAULTS.md`:
- Line 3: The sentence "This reduces schema size and repeated payloads and
ensure a more deterministic experience." has incorrect subject–verb agreement;
change "ensure" to "ensures" so it reads "...and ensures a more deterministic
experience." Update the phrase in the SESSION_DEFAULTS.md content to use
"ensures" to match the singular subject.
- Around line 26-30: Fix the typo and add punctuation in the paragraph of
SESSION_DEFAULTS.md that begins "You can also manually create the config file to
essentually seed the defaults at startup see [CONFIGURATION.md]". Correct
"essentually" to "essentially" and insert a period (or comma and a pause) before
the reference so it reads e.g. "...seed the defaults at startup. See
[CONFIGURATION.md] for more information."
In `@skills/xcodebuildmcp/SKILL.md`:
- Line 16: Fix the typo in the SKILL.md sentence that reads "Before you call any
other tools, call `session_show_defaults` to show the current defaults, ensure
you then fill in the apropiate missing defaults." — change "apropiate" to
"appropriate" so it reads "ensure you then fill in the appropriate missing
defaults" and keep the rest of the sentence unchanged; reference the sentence
containing `session_show_defaults` when making the edit.
In `@src/utils/project-config.ts`:
- Around line 151-163: The try/catch already covers both options.fs.readFile and
parseProjectConfig, but the warning message only mentions parsing; update the
catch for the block around options.fs.existsSync / options.fs.readFile /
parseProjectConfig so the log call (log(...)) clearly states it failed to read
or parse the project config and include the error details (e.g., error.message
or error.stack) and context (configPath) to aid debugging, referencing the
existing symbols options.fs.existsSync, options.fs.readFile, parseProjectConfig,
log, and baseConfig.
- Around line 114-139: The function loadProjectConfig currently calls
options.fs.readFile and parseProjectConfig (and subsequent normalizers) without
guarding against runtime errors; wrap the file read + parse + normalize/resolve
steps in a try-catch around the block that calls options.fs.readFile,
parseProjectConfig, normalizeMutualExclusivity, and resolveRelativeSessionPaths,
and on error return a structured LoadProjectConfigResult indicating failure (for
example include an error property or { found: false, error: err }) instead of
letting exceptions bubble, or alternatively rethrow with added context—ensure
callers can distinguish "not found" vs "read/parse error" similar to how
persistSessionDefaultsToProjectConfig handles parse errors.
🧹 Nitpick comments (6)
example_projects/iOS_Calculator/.xcodebuildmcp/config.yaml (1)
3-11: Consider relative paths to keep the example portable.Using project-relative paths makes the example easier to reuse across machines.
Proposed edit
- workspacePath: /Volumes/Developer/XcodeBuildMCP/example_projects/iOS_Calculator/CalculatorApp.xcworkspace + workspacePath: ./CalculatorApp.xcworkspace ... - derivedDataPath: /Volumes/Developer/XcodeBuildMCP/example_projects/iOS_Calculator/.derivedData + derivedDataPath: ./.derivedDatasrc/mcp/tools/session-management/session_set_defaults.ts (3)
25-33: Consider extractingremoveUndefinedto a shared utility.This helper function is duplicated in
src/utils/project-config.ts(lines 55-63). To adhere to DRY principles, consider extracting it to a shared utility module and importing it in both files.
20-23: Consider exportingSessionSetDefaultsContextfor testability.The
SessionSetDefaultsContexttype is used as a parameter in the exportedsessionSetDefaultsLogicfunction. Exporting this type would improve type safety for consumers and test code that need to construct the context object.Suggested change
-type SessionSetDefaultsContext = { +export type SessionSetDefaultsContext = { fs: FileSystemExecutor; cwd: string; };
46-57: Redundant checks afterremoveUndefined.Since
removeUndefinedalready strips keys withundefinedvalues, the&& nextParams.projectPath !== undefinedconditions are redundant when checkinghasOwnProperty. The key will only exist innextParamsif the value was defined.Simplified version
const hasProjectPath = - Object.prototype.hasOwnProperty.call(nextParams, 'projectPath') && - nextParams.projectPath !== undefined; + Object.prototype.hasOwnProperty.call(nextParams, 'projectPath'); const hasWorkspacePath = - Object.prototype.hasOwnProperty.call(nextParams, 'workspacePath') && - nextParams.workspacePath !== undefined; + Object.prototype.hasOwnProperty.call(nextParams, 'workspacePath'); const hasSimulatorId = - Object.prototype.hasOwnProperty.call(nextParams, 'simulatorId') && - nextParams.simulatorId !== undefined; + Object.prototype.hasOwnProperty.call(nextParams, 'simulatorId'); const hasSimulatorName = - Object.prototype.hasOwnProperty.call(nextParams, 'simulatorName') && - nextParams.simulatorName !== undefined; + Object.prototype.hasOwnProperty.call(nextParams, 'simulatorName');src/utils/project-config.ts (2)
12-17: Consider using.strict()instead of.passthrough()for better validation.Using
.passthrough()allows any unknown keys to pass through without validation or warning. If forward compatibility is the goal (allowing future schema versions to add fields), this is acceptable. However, if you want to catch typos in configuration keys (e.g.,sessionDefaultinstead ofsessionDefaults), consider using.strict()or at least logging unknown keys.
19-19: Unused type definition.
ProjectConfigSchemais inferred but not used anywhere in the file. Consider removing it or using it as the return type annotation forparseProjectConfig.Option: Use as return type
-function parseProjectConfig(rawText: string): ProjectConfigSchema { +function parseProjectConfig(rawText: string): z.infer<typeof projectConfigSchema> {Or simply remove line 19 if not needed.
| export async function loadProjectConfig( | ||
| options: LoadProjectConfigOptions, | ||
| ): Promise<LoadProjectConfigResult> { | ||
| const configPath = getConfigPath(options.cwd); | ||
|
|
||
| if (!options.fs.existsSync(configPath)) { | ||
| return { found: false }; | ||
| } | ||
|
|
||
| const rawText = await options.fs.readFile(configPath, 'utf8'); | ||
| const parsed = parseProjectConfig(rawText); | ||
|
|
||
| if (!parsed.sessionDefaults) { | ||
| return { found: true, path: configPath, config: parsed, notices: [] }; | ||
| } | ||
|
|
||
| const { normalized, notices } = normalizeMutualExclusivity(parsed.sessionDefaults); | ||
| const resolved = resolveRelativeSessionPaths(normalized, options.cwd); | ||
|
|
||
| const config: ProjectConfig = { | ||
| ...parsed, | ||
| sessionDefaults: resolved, | ||
| }; | ||
|
|
||
| return { found: true, path: configPath, config, notices }; | ||
| } |
There was a problem hiding this comment.
Unhandled errors in loadProjectConfig could cause unexpected failures.
Unlike persistSessionDefaultsToProjectConfig which catches parse errors, loadProjectConfig will throw if readFile fails (other than file not existing) or if parseProjectConfig throws on malformed YAML. Consider wrapping in try-catch and returning a structured error result, or document that callers must handle these exceptions.
Suggested approach with error handling
export async function loadProjectConfig(
options: LoadProjectConfigOptions,
-): Promise<LoadProjectConfigResult> {
+): Promise<LoadProjectConfigResult | { found: true; error: string }> {
const configPath = getConfigPath(options.cwd);
if (!options.fs.existsSync(configPath)) {
return { found: false };
}
+ try {
const rawText = await options.fs.readFile(configPath, 'utf8');
const parsed = parseProjectConfig(rawText);
// ... rest of logic
+ } catch (error) {
+ log('warning', `Failed to load project config at ${configPath}: ${error}`);
+ return { found: true, error: String(error) };
+ }🤖 Prompt for AI Agents
In `@src/utils/project-config.ts` around lines 114 - 139, The function
loadProjectConfig currently calls options.fs.readFile and parseProjectConfig
(and subsequent normalizers) without guarding against runtime errors; wrap the
file read + parse + normalize/resolve steps in a try-catch around the block that
calls options.fs.readFile, parseProjectConfig, normalizeMutualExclusivity, and
resolveRelativeSessionPaths, and on error return a structured
LoadProjectConfigResult indicating failure (for example include an error
property or { found: false, error: err }) instead of letting exceptions bubble,
or alternatively rethrow with added context—ensure callers can distinguish "not
found" vs "read/parse error" similar to how
persistSessionDefaultsToProjectConfig handles parse errors.
| if (options.fs.existsSync(configPath)) { | ||
| try { | ||
| const rawText = await options.fs.readFile(configPath, 'utf8'); | ||
| const parsed = parseProjectConfig(rawText); | ||
| baseConfig = { ...parsed, schemaVersion: 1 }; | ||
| } catch (error) { | ||
| log( | ||
| 'warning', | ||
| `Failed to parse project config at ${configPath}. Overwriting with new config. ${error}`, | ||
| ); | ||
| baseConfig = { schemaVersion: 1 }; | ||
| } | ||
| } |
There was a problem hiding this comment.
Inconsistent error handling between existsSync and readFile.
If existsSync returns true but readFile fails (e.g., due to permissions or race condition), the error would propagate unhandled. The catch block only handles parseProjectConfig errors. Consider catching readFile errors as well.
Suggested fix
if (options.fs.existsSync(configPath)) {
try {
const rawText = await options.fs.readFile(configPath, 'utf8');
const parsed = parseProjectConfig(rawText);
baseConfig = { ...parsed, schemaVersion: 1 };
} catch (error) {
log(
'warning',
- `Failed to parse project config at ${configPath}. Overwriting with new config. ${error}`,
+ `Failed to read or parse project config at ${configPath}. Overwriting with new config. ${error}`,
);
baseConfig = { schemaVersion: 1 };
}
}The current code structure does handle this correctly since both readFile and parseProjectConfig are within the try block. The log message could be updated to reflect that reading could also fail.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (options.fs.existsSync(configPath)) { | |
| try { | |
| const rawText = await options.fs.readFile(configPath, 'utf8'); | |
| const parsed = parseProjectConfig(rawText); | |
| baseConfig = { ...parsed, schemaVersion: 1 }; | |
| } catch (error) { | |
| log( | |
| 'warning', | |
| `Failed to parse project config at ${configPath}. Overwriting with new config. ${error}`, | |
| ); | |
| baseConfig = { schemaVersion: 1 }; | |
| } | |
| } | |
| if (options.fs.existsSync(configPath)) { | |
| try { | |
| const rawText = await options.fs.readFile(configPath, 'utf8'); | |
| const parsed = parseProjectConfig(rawText); | |
| baseConfig = { ...parsed, schemaVersion: 1 }; | |
| } catch (error) { | |
| log( | |
| 'warning', | |
| `Failed to read or parse project config at ${configPath}. Overwriting with new config. ${error}`, | |
| ); | |
| baseConfig = { schemaVersion: 1 }; | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@src/utils/project-config.ts` around lines 151 - 163, The try/catch already
covers both options.fs.readFile and parseProjectConfig, but the warning message
only mentions parsing; update the catch for the block around
options.fs.existsSync / options.fs.readFile / parseProjectConfig so the log call
(log(...)) clearly states it failed to read or parse the project config and
include the error details (e.g., error.message or error.stack) and context
(configPath) to aid debugging, referencing the existing symbols
options.fs.existsSync, options.fs.readFile, parseProjectConfig, log, and
baseConfig.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| deleteKeys: Array.from(toClear), | ||
| }); | ||
| notices.push(`Persisted defaults to ${path}`); | ||
| } |
There was a problem hiding this comment.
Session changes not rolled back if persistence fails
Low Severity
When persist: true is passed, in-memory session defaults are modified (via sessionStore.clear() and sessionStore.setDefaults()) before persistSessionDefaultsToProjectConfig() is called. If persistence fails (e.g., disk full, permission denied), the error propagates but the session changes remain applied. This leads to inconsistent behavior where the user sees an error response, but their session was actually modified - only the disk persistence failed.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
cameroncooke
force-pushed
the
01-27-feat_xcodebuildmcp_add_support_for_session-default_persistance
branch
from
626a80b to
a754339
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| if (Object.keys(nextParams).length > 0) { | ||
| sessionStore.setDefaults(nextParams as Partial<SessionDefaults>); | ||
| } | ||
|
|
||
| if (persist) { | ||
| if (Object.keys(nextParams).length === 0 && toClear.size === 0) { | ||
| notices.push('No defaults provided to persist.'); | ||
| } else { | ||
| const { path } = await persistSessionDefaultsToProjectConfig({ | ||
| fs: context.fs, |
There was a problem hiding this comment.
Bug: When session_set_defaults persists a relative path, it remains relative for the current session, causing tools to fail. After restart, the path is correctly resolved to absolute.
Severity: MEDIUM
Suggested Fix
Resolve relative paths to absolute paths within the session_set_defaults tool before storing them in sessionStore and persisting them. This can be done using path.resolve() and would align its behavior with the loadProjectConfig function, ensuring path consistency.
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/mcp/tools/session-management/session_set_defaults.ts#L107-L116
Potential issue: There is a path resolution inconsistency when using
`session_set_defaults` with `persist: true`. Relative paths provided in `nextParams` are
stored directly in `sessionStore` and persisted to the config file without being
resolved to absolute paths. Tools that subsequently use these paths within the same
session will receive the unresolved relative path, which can cause commands like
`xcodebuild` to fail as they interpret the path relative to their own working directory.
However, after a server restart, `loadProjectConfig` correctly resolves these paths to
absolute ones, leading to inconsistent behavior (fails before restart, works after).
Did we get this right? 👍 / 👎 to inform future reviews.
cameroncooke
deleted the
01-27-feat_xcodebuildmcp_add_support_for_session-default_persistance
branch
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| useLatestOS: true | ||
| arch: arm64 | ||
| suppressWarnings: false | ||
| derivedDataPath: ./iOS_Calculator/.derivedData |
There was a problem hiding this comment.
Example config has incorrect relative paths
High Severity
The example config file contains relative paths with a duplicate directory component (./iOS_Calculator/...). Since the config is located at <workspace-root>/.xcodebuildmcp/config.yaml where workspace-root is already example_projects/iOS_Calculator/, path resolution will prepend the directory again, resulting in non-existent paths like /example_projects/iOS_Calculator/iOS_Calculator/CalculatorApp.xcworkspace. The paths should be ./CalculatorApp.xcworkspace and ./.derivedData instead.
| ...baseConfig, | ||
| schemaVersion: 1, | ||
| sessionDefaults: nextSessionDefaults, | ||
| }; |
There was a problem hiding this comment.
Persist function doesn't normalize mutual exclusivity
Medium Severity
The persistSessionDefaultsToProjectConfig function merges patches and deletes specified keys but doesn't call normalizeMutualExclusivity before writing. If someone manually edits the config file to have both projectPath and workspacePath (or both simulatorId and simulatorName), then persists other changes, the invalid mutual exclusivity state is preserved in the written config. The load function normalizes these conflicts (line 128), but the persist function doesn't, creating inconsistency between what's loaded and what's written.
Add Project Config for Session Defaults
This PR adds support for a project-level configuration file at
.xcodebuildmcp/config.yamlthat:session_set_defaultsusing thepersist: trueflagThe config file uses a simple YAML format:
Key features:
.derivedDatato.gitignoreResolves issue #180
Note
Medium Risk
Medium risk because it changes server startup behavior and introduces config file read/write paths that affect how defaults are applied across sessions; failures are handled as warn-and-continue but could lead to unexpected tool defaults.
Overview
Adds support for a project-level
.xcodebuildmcp/config.yamlthat preloads session defaults at server startup and allowssession-set-defaultsto persist provided defaults back to that file (patch-only), enforcing mutual exclusivity rules and resolving relative paths.Introduces shared Zod schema/utilities for session defaults (
session-defaults-schema.ts,remove-undefined.ts), a newproject-config.tsloader/writer with passthrough support for unknown sections, updatessession-clear-defaultskey coverage, adds tests, and documents the new config/persist behavior; also adds theyamldependency and ignores.derivedData.Written by Cursor Bugbot for commit a754339. This will update automatically on new commits. Configure here.