-
-
Notifications
You must be signed in to change notification settings - Fork 193
Support persisting custom env vars in session defaults #207
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?
Conversation
commit: |
WalkthroughThis pull request introduces support for environment variables in session defaults. The changes extend the session defaults schema by adding an optional 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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 |
|
@kamal Thanks for this, would you mind fixing up the typecheck issue. In general it's recommended you install the pre-commit hook into this repo see: https://github.com/cameroncooke/XcodeBuildMCP/blob/main/docs/dev/CONTRIBUTING.md#local-development-setup |
3a47f1f to
40bc529
Compare
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.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| // (user-provided keys take precedence on conflict) | ||
| if (sessionDefaults.env && typeof sanitizedArgs.env === 'object' && sanitizedArgs.env) { | ||
| merged.env = { ...sessionDefaults.env, ...(sanitizedArgs.env as Record<string, string>) }; | ||
| } |
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.
Env validation bypass via deep merge
Medium Severity
The env deep-merge guard treats any object-like value as mergeable, so arrays or other non-record objects in sanitizedArgs.env get spread into merged.env before internalSchema.parse. When session defaults contain env, invalid user input can be silently accepted or transformed instead of failing validation.
| // (user-provided keys take precedence on conflict) | ||
| if (sessionDefaults.env && typeof sanitizedArgs.env === 'object' && sanitizedArgs.env) { | ||
| merged.env = { ...sessionDefaults.env, ...(sanitizedArgs.env as Record<string, string>) }; | ||
| } |
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.
| preferXcodebuild?: boolean; | ||
| platform?: string; | ||
| bundleId?: string; | ||
| env?: Record<string, string>; |
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.
Mutable env leaks through session store
Medium Severity
Adding env as Record<string, string> introduces shared-reference state in sessionStore. setDefaults/getAll are shallow, so mutating a previously passed or retrieved env object can silently change stored defaults without calling sessionStore.setDefaults, bypassing revision tracking.


As a follow up to #203, this adds support for persisting custom environment variables into session defaults