Skip to content

Commit fc5a184

Browse files
committed
fix(session-defaults): harden store, schema validation, and clear semantics
- Add cloneDefaults() helper for defensive copy on both read and write - Split clear() (profile-scoped) from clearAll() (nuke everything) - Reset active profile to global when clearing a named profile - Add nonEmptyString validation for all string session default fields - Reject empty string env keys in schema - Fix double revision increment in clearForProfile - Use clearAll() consistently in test beforeEach/afterEach
1 parent b90c0a6 commit fc5a184

File tree

7 files changed

+74
-36
lines changed

7 files changed

+74
-36
lines changed

src/mcp/tools/session-management/__tests__/session_clear_defaults.test.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import { schema, handler, sessionClearDefaultsLogic } from '../session_clear_def
44

55
describe('session-clear-defaults tool', () => {
66
beforeEach(() => {
7-
sessionStore.clear();
7+
sessionStore.clearAll();
88
sessionStore.setDefaults({
99
scheme: 'MyScheme',
1010
projectPath: '/path/to/proj.xcodeproj',
@@ -17,7 +17,7 @@ describe('session-clear-defaults tool', () => {
1717
});
1818

1919
afterEach(() => {
20-
sessionStore.clear();
20+
sessionStore.clearAll();
2121
});
2222

2323
describe('Export Field Validation', () => {
@@ -86,7 +86,7 @@ describe('session-clear-defaults tool', () => {
8686
const result = await sessionClearDefaultsLogic({});
8787
expect(result.isError).toBe(false);
8888

89-
expect(sessionStore.getAll()).toEqual({});
89+
expect(sessionStore.getAll().scheme).toBe('Global');
9090
expect(sessionStore.listProfiles()).toEqual([]);
9191

9292
sessionStore.setActiveProfile(null);

src/mcp/tools/session-management/__tests__/session_set_defaults.test.ts

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import type { CommandExecutor } from '../../../../utils/execution/index.ts';
1010
describe('session-set-defaults tool', () => {
1111
beforeEach(() => {
1212
__resetConfigStoreForTests();
13-
sessionStore.clear();
13+
sessionStore.clearAll();
1414
});
1515

1616
const cwd = '/repo';
@@ -99,6 +99,16 @@ describe('session-set-defaults tool', () => {
9999
expect(result.content[0].text).toContain('env');
100100
});
101101

102+
it('should reject empty string defaults for required string fields', async () => {
103+
const result = await handler({
104+
scheme: '',
105+
});
106+
107+
expect(result.isError).toBe(true);
108+
expect(result.content[0].text).toContain('Parameter validation failed');
109+
expect(result.content[0].text).toContain('scheme');
110+
});
111+
102112
it('should clear workspacePath when projectPath is set', async () => {
103113
sessionStore.setDefaults({ workspacePath: '/old/App.xcworkspace' });
104114
const result = await sessionSetDefaultsLogic(

src/mcp/tools/session-management/session_clear_defaults.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ export async function sessionClearDefaultsLogic(params: Params): Promise<ToolRes
3838
};
3939
}
4040

41-
sessionStore.clear();
41+
sessionStore.clearAll();
4242
return { content: [{ type: 'text', text: 'All session defaults cleared' }], isError: false };
4343
}
4444

@@ -73,7 +73,7 @@ export async function sessionClearDefaultsLogic(params: Params): Promise<ToolRes
7373
if (params.keys) {
7474
sessionStore.clear(params.keys);
7575
} else {
76-
sessionStore.clearForProfile(sessionStore.getActiveProfile());
76+
sessionStore.clear();
7777
}
7878

7979
return { content: [{ type: 'text', text: 'Session defaults cleared' }], isError: false };

src/utils/__tests__/session-aware-tool-factory.test.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ async function initConfigStoreForTest(overrides?: RuntimeConfigOverrides): Promi
2121

2222
describe('createSessionAwareTool', () => {
2323
beforeEach(async () => {
24-
sessionStore.clear();
24+
sessionStore.clearAll();
2525
await initConfigStoreForTest({ disableSessionDefaults: false });
2626
});
2727

@@ -159,7 +159,7 @@ describe('createSessionAwareTool', () => {
159159
expect(res.content[0].text).toBe('OK');
160160
});
161161

162-
it('exclusivePairs should NOT prune when user provides undefined (key present)', async () => {
162+
it('exclusivePairs should NOT prune when user provides undefined (treated as not provided)', async () => {
163163
const handlerWithExclusive = createSessionAwareTool<Params>({
164164
internalSchema,
165165
logicFunction: logic,

src/utils/__tests__/session-store.test.ts

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import { sessionStore } from '../session-store.ts';
33

44
describe('SessionStore', () => {
55
beforeEach(() => {
6-
sessionStore.clear();
6+
sessionStore.clearAll();
77
});
88

99
it('should set and get defaults', () => {
@@ -75,9 +75,12 @@ describe('SessionStore', () => {
7575
expect(sessionStore.getAll()).toMatchObject({ workspacePath: '/repo/MyApp.xcworkspace' });
7676
});
7777

78-
it('clear(keys) only affects active profile while clear() clears all profiles', () => {
78+
it('clear(keys) only affects active profile while clear() clears active profile and resets to global', () => {
79+
sessionStore.setDefaults({ scheme: 'GlobalApp' });
80+
7981
sessionStore.setActiveProfile('ios');
8082
sessionStore.setDefaults({ scheme: 'iOSApp', simulatorId: 'SIM-1' });
83+
8184
sessionStore.setActiveProfile('watch');
8285
sessionStore.setDefaults({ scheme: 'WatchApp', simulatorId: 'SIM-2' });
8386

@@ -89,10 +92,23 @@ describe('SessionStore', () => {
8992
sessionStore.setActiveProfile('watch');
9093
expect(sessionStore.getAll().simulatorId).toBe('SIM-2');
9194

95+
sessionStore.setActiveProfile('ios');
9296
sessionStore.clear();
93-
expect(sessionStore.getAll()).toEqual({});
9497
expect(sessionStore.getActiveProfile()).toBeNull();
95-
expect(sessionStore.listProfiles()).toEqual([]);
98+
expect(sessionStore.getAll()).toMatchObject({ scheme: 'GlobalApp' });
99+
100+
sessionStore.setActiveProfile('watch');
101+
expect(sessionStore.getAll()).toMatchObject({ scheme: 'WatchApp', simulatorId: 'SIM-2' });
102+
});
103+
104+
it('does not retain external env object references passed into setDefaults', () => {
105+
const env = { API_KEY: 'secret' };
106+
sessionStore.setDefaults({ env });
107+
108+
env.API_KEY = 'tampered';
109+
110+
const stored = sessionStore.getAll();
111+
expect(stored.env).toEqual({ API_KEY: 'secret' });
96112
});
97113

98114
it('getAll returns a detached copy of env so mutations do not affect stored defaults', () => {
Lines changed: 13 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
import * as z from 'zod';
22

3+
const nonEmptyString = z.string().min(1);
4+
35
export const sessionDefaultKeys = [
46
'projectPath',
57
'workspacePath',
@@ -22,41 +24,37 @@ export const sessionDefaultKeys = [
2224
export type SessionDefaultKey = (typeof sessionDefaultKeys)[number];
2325

2426
export const sessionDefaultsSchema = z.object({
25-
projectPath: z.string().optional().describe('xcodeproj path (xor workspacePath)'),
26-
workspacePath: z.string().optional().describe('xcworkspace path (xor projectPath)'),
27-
scheme: z.string().optional(),
28-
configuration: z
29-
.string()
27+
projectPath: nonEmptyString.optional().describe('xcodeproj path (xor workspacePath)'),
28+
workspacePath: nonEmptyString.optional().describe('xcworkspace path (xor projectPath)'),
29+
scheme: nonEmptyString.optional(),
30+
configuration: nonEmptyString
3031
.optional()
3132
.describe("Build configuration for Xcode and SwiftPM tools (e.g. 'Debug' or 'Release')."),
32-
simulatorName: z.string().optional(),
33-
simulatorId: z.string().optional(),
33+
simulatorName: nonEmptyString.optional(),
34+
simulatorId: nonEmptyString.optional(),
3435
simulatorPlatform: z
3536
.enum(['iOS Simulator', 'watchOS Simulator', 'tvOS Simulator', 'visionOS Simulator'])
3637
.optional()
3738
.describe('Cached inferred simulator platform.'),
38-
deviceId: z.string().optional(),
39+
deviceId: nonEmptyString.optional(),
3940
useLatestOS: z.boolean().optional(),
4041
arch: z.enum(['arm64', 'x86_64']).optional(),
4142
suppressWarnings: z.boolean().optional(),
42-
derivedDataPath: z
43-
.string()
43+
derivedDataPath: nonEmptyString
4444
.optional()
4545
.describe('Default DerivedData path for Xcode build/test/clean tools.'),
4646
preferXcodebuild: z
4747
.boolean()
4848
.optional()
4949
.describe('Prefer xcodebuild over incremental builds for Xcode build/test/clean tools.'),
50-
platform: z
51-
.string()
50+
platform: nonEmptyString
5251
.optional()
5352
.describe('Default device platform for device tools (e.g. iOS, watchOS).'),
54-
bundleId: z
55-
.string()
53+
bundleId: nonEmptyString
5654
.optional()
5755
.describe('Default bundle ID for launch/stop/log tools when working on a single app.'),
5856
env: z
59-
.record(z.string(), z.string())
57+
.record(nonEmptyString, z.string())
6058
.optional()
6159
.describe('Default environment variables to pass to launched apps.'),
6260
});

src/utils/session-store.ts

Lines changed: 23 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -29,11 +29,19 @@ class SessionStore {
2929
private activeProfile: string | null = null;
3030
private revision = 0;
3131

32+
private cloneDefaults(defaults: SessionDefaults): SessionDefaults {
33+
const copy = { ...defaults };
34+
if (copy.env) {
35+
copy.env = { ...copy.env };
36+
}
37+
return copy;
38+
}
39+
3240
private getProfileLabel(profile: string | null): string {
3341
return profile ?? 'global';
3442
}
3543

36-
private clearAll(): void {
44+
private clearAllInternal(): void {
3745
this.globalDefaults = {};
3846
this.profiles = {};
3947
this.activeProfile = null;
@@ -55,11 +63,12 @@ class SessionStore {
5563
}
5664

5765
private setDefaultsForResolvedProfile(profile: string | null, defaults: SessionDefaults): void {
66+
const storedDefaults = this.cloneDefaults(defaults);
5867
if (profile === null) {
59-
this.globalDefaults = defaults;
68+
this.globalDefaults = storedDefaults;
6069
return;
6170
}
62-
this.profiles[profile] = defaults;
71+
this.profiles[profile] = storedDefaults;
6372
}
6473

6574
setDefaults(partial: Partial<SessionDefaults>): void {
@@ -77,16 +86,25 @@ class SessionStore {
7786

7887
clear(keys?: (keyof SessionDefaults)[]): void {
7988
if (keys == null) {
80-
this.clearAll();
89+
this.clearForProfile(this.activeProfile);
8190
return;
8291
}
8392

8493
this.clearForProfile(this.activeProfile, keys);
8594
}
8695

96+
clearAll(): void {
97+
this.clearAllInternal();
98+
}
99+
87100
clearForProfile(profile: string | null, keys?: (keyof SessionDefaults)[]): void {
88101
if (keys == null) {
102+
const wasActiveNamedProfile = profile !== null && profile === this.activeProfile;
89103
this.clearAllForProfile(profile);
104+
if (wasActiveNamedProfile) {
105+
this.activeProfile = null;
106+
log('info', '[Session] Active defaults profile reset to global');
107+
}
90108
return;
91109
}
92110

@@ -115,11 +133,7 @@ class SessionStore {
115133

116134
getAllForProfile(profile: string | null): SessionDefaults {
117135
const defaults = profile === null ? this.globalDefaults : (this.profiles[profile] ?? {});
118-
const copy = { ...defaults };
119-
if (copy.env) {
120-
copy.env = { ...copy.env };
121-
}
122-
return copy;
136+
return this.cloneDefaults(defaults);
123137
}
124138

125139
listProfiles(): string[] {

0 commit comments

Comments
 (0)